Skip to content

Fix grapheme-vs-UTF-16 range bug in RichContentFormatter - #25833

Merged
jkmassel merged 3 commits into
trunkfrom
jkmassel/richcontentformatter-utf16-range
Sep 3, 2026
Merged

Fix grapheme-vs-UTF-16 range bug in RichContentFormatter#25833
jkmassel merged 3 commits into
trunkfrom
jkmassel/richcontentformatter-utf16-range

Conversation

@jkmassel

@jkmassel jkmassel commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What this fixes

RichContentFormatter sized the NSRange it hands to NSRegularExpression from content.count — Swift's grapheme-cluster count — but NSRegularExpression matches over UTF-16. When content contains multi-code-unit characters (emoji, flags, ZWJ/combining sequences) the grapheme count is shorter than the UTF-16 length, so the search range stops short of the end and any forbidden tag, inline style, or gallery-image src near the end is silently left untouched — unsanitized markup then reaches the rendered Reader post or comment.

A single family emoji 👨‍👩‍👧‍👦 is one grapheme but eleven UTF-16 units, so a <script> placed right after it begins ten units past where the grapheme-count range ends: the whole tag falls outside the search and survives.

ASCII content is unaffected — there the grapheme count equals the UTF-16 length, which is why this went unnoticed.

Changes

  • Size every search range in UTF-16. The whole-string ranges across removeForbiddenTags, normalizeParagraphs, filterNewLines, and removeInlineStyles now come from String.utf16.count, and removeTrailingBreakTags ranges over UTF-16 too.
  • removeTrailingBreakTags no longer mixes offset spaces. It fed a UTF-16 match.range.location to String.index(_:offsetBy:), which counts graphemes — already wrong for multibyte content, and a hard crash (String index is out of bounds) once the range widened. It now converts the match with Range(match.range, in: content).
  • resizeGalleryImageURL had the same bug. This display-pipeline step rewrites a gallery image's src to a Photon/resized URL; it sized the replacement range from imgElementStr.count, so a src sitting past a multibyte cluster was never rewritten and the full-size original loaded instead. It runs on rendered comments (CommentServiceformatContentString). Now sized in UTF-16.
  • Harden parseValueForAttribute against malformed markup. A separate crash in the same file: it searched for an attribute's closing quote and passed the result to substring(with:) unguarded, so an opening quote with no close (NSNotFound) underflowed the range length to an out-of-bounds NSRange and trapped. It now returns "" when the closing quote is missing, matching the attribute-not-found default.
  • Migrate the tests to Swift Testing. The RichContentFormatter test files move from XCTest to @Test/#expect, matching the rest of the WordPressSharedTests target — same inputs, same assertions.

formatGutenbergGallery and formatVideoTags already ranged over NSString.length and are untouched.

The whole-string ranges are sized with content.utf16.count inline, matching the convention #25341 established across the rest of the codebase.

Split into three commits for review: the fix (with regression tests), the Swift Testing migration, and the parseValueForAttribute hardening.

Tests

One isolated test per fix site — each forbidden-tag regex, each normalizeParagraphs sub-site, the three filterNewLines paths, the inline-style site, and the trailing-break range plus its index-offset cut — using a spread of multi-code-unit clusters (astral emoji, a ZWJ family, a flag, a keycap, a skin-tone modifier, and an NFD combining mark), positioned so the target token lands in the tail the grapheme-count range dropped. Each input holds one tag type, so reverting any single site breaks exactly one test; the exact-output assertions also confirm the cluster survives byte-for-byte. Two further tests pin the exact off-by-one boundary and confirm the corrected range strips the intended tag rather than everything.

parseValueForAttribute gets three direct tests, including a malformed element (opening quote, no close) that crashes the pre-fix code with an out-of-bounds NSRange and returns "" after the fix.

resizeGalleryImageURL's regression lives in RichContentFormatterUITests — it needs UIKit, so it runs in the iOS WordPressUnitTests plan, not the macOS swift test set.

Stacking

Stacked on #25832 (base jkmassel/wordpressdata-swift-test), which removes WordPressData's WordPressUI/WordPressSharedUI dependencies and, as part of that, splits RichContentFormatter's platform-independent core into cross-platform WordPressShared — which is what lets this fix and its regression tests run on macOS under swift test. The bug is pre-existing; it's also on trunk, in the pre-split WordPressSharedUI copy. This PR retargets to trunk once #25832 merges.

Test plan

  • swift test --filter RichContentFormatterTests (macOS host) → 26/26. 15 of the 16 multibyte tests fail on the pre-fix source (the 16th is a no-tags idempotence guard that passes either way); the parseValueForAttribute missing-closing-quote test traps the pre-fix code with an out-of-bounds NSRange.
  • Run the iOS WordPressUnitTests plan (or rely on CI): RichContentFormatterUITests.testResizeGalleryImageURLReplacesSrcPastMultibyteCluster — a gallery <img data-orig-file=… alt="😀😀😀😀😀" src=…/> comes back with its src swapped to a *.wp.com URL (0 replacements on the pre-fix source).

@wpmobilebot

wpmobilebot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number34358
VersionPR #25833
Bundle IDorg.wordpress.alpha
Commit1235488
Installation URL396c4kc1u198g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number34358
VersionPR #25833
Bundle IDcom.jetpack.alpha
Commit1235488
Installation URL22bhrg98q33kg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jkmassel
jkmassel force-pushed the jkmassel/richcontentformatter-utf16-range branch 6 times, most recently from af67b42 to ea3df4b Compare July 24, 2026 18:34
@jkmassel
jkmassel force-pushed the jkmassel/richcontentformatter-utf16-range branch from ea3df4b to f472041 Compare September 1, 2026 23:30
@wpmobilebot

Copy link
Copy Markdown
Contributor

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

@jkmassel
jkmassel force-pushed the jkmassel/richcontentformatter-utf16-range branch from f472041 to 53a1bf3 Compare September 2, 2026 00:21
@jkmassel
jkmassel force-pushed the jkmassel/richcontentformatter-utf16-range branch 2 times, most recently from ab1cdc9 to abfb667 Compare September 3, 2026 03:36
jkmassel added a commit that referenced this pull request Sep 3, 2026
Stacked on #25833 (the String.count/UTF-16 NSRange fix + the missing-closing-quote
guard). Adds the remaining sanitization fixes:

- Strip unclosed <script>/<style> through end of input, not only closed pairs.
- Strip single-quoted inline styles, and require a whitespace boundary so `style=`
  no longer matches inside a longer name like `data-style`.
- Detect the <video> controls attribute as a word-boundaried, case-insensitive token
  (not a case-sensitive substring: poster="…/controls.jpg", class="…-controls",
  data-controls, controlslist), and tighten the <video> match so it can't over-match
  <videoxyz>; insert controls in place, preserving the tag's casing.
- Match parseValueForAttribute's name on a word boundary so "rc" no longer matches
  inside "src".
- Anchor resizeGalleryImageURL's src rewrite to the whole src="…" token so it can't
  also rewrite the same URL where it appears in srcset.
@jkmassel jkmassel self-assigned this Sep 3, 2026
@jkmassel jkmassel added this to the 27.3 milestone Sep 3, 2026
@jkmassel
jkmassel marked this pull request as ready for review September 3, 2026 03:52
@jkmassel
jkmassel requested a review from crazytonyli September 3, 2026 03:52
Base automatically changed from jkmassel/wordpressdata-swift-test to trunk September 3, 2026 21:32
RichContentFormatter built its NSRanges from `content.count` (Swift grapheme count), but NSRegularExpression matches over UTF-16. With multi-code-unit characters (emoji, flags, combining sequences) the grapheme count is shorter than the UTF-16 length, so the search range was truncated and any tag or style near the end silently escaped stripping. removeTrailingBreakTags also fed a UTF-16 match offset to String.index(_:offsetBy:), which counts graphemes — right for ASCII, a crash once the range was corrected. resizeGalleryImageURL, in the display pipeline, carried the same confusion: it sized the src-rewrite range from `imgElementStr.count`, so a gallery image's src could slip past the range and never be swapped for its resized URL.

Range over UTF-16 via `String.utf16.count`, and convert the trailing-BR match with Range(_:in:).

Adds one isolated test per fix site — each forbidden-tag, div/paragraph, filterNewLines, inline-style, and trailing-break site, plus the trailing-break index-offset cut and the gallery-image src rewrite — using astral emoji, ZWJ sequences, flags, keycaps, skin-tone modifiers, and an NFD combining mark, so reverting any single site breaks exactly one test. Two further tests pin the exact off-by-one boundary and confirm the corrected range strips the intended tag rather than everything. Each fails on the old code and passes now, and the exact-output assertions confirm the clusters survive byte-for-byte.
Convert RichContentFormatterTests and RichContentFormatterUITests from XCTest to Swift Testing (@test / #expect), matching the rest of the WordPressSharedTests target. Same inputs and assertions; no coverage change.
parseValueForAttribute located an attribute's closing quote and fed the result straight into substring(with:). When the closing quote is absent — malformed markup with an opening quote and no close — the search returns NSNotFound, so the range length underflowed to NSIntegerMax and crashed with an out-of-bounds NSRange. Guard on the closing quote and return "" when it's missing, matching the attribute-not-found default.
jkmassel added a commit that referenced this pull request Sep 3, 2026
Stacked on #25833 (the String.count/UTF-16 NSRange fix + the missing-closing-quote
guard). Adds the remaining sanitization fixes:

- Strip unclosed <script>/<style> through end of input, not only closed pairs.
- Strip single-quoted inline styles, and require a whitespace boundary so `style=`
  no longer matches inside a longer name like `data-style`.
- Detect the <video> controls attribute as a word-boundaried, case-insensitive token
  (not a case-sensitive substring: poster="…/controls.jpg", class="…-controls",
  data-controls, controlslist), and tighten the <video> match so it can't over-match
  <videoxyz>; insert controls in place, preserving the tag's casing.
- Match parseValueForAttribute's name on a word boundary so "rc" no longer matches
  inside "src".
- Anchor resizeGalleryImageURL's src rewrite to the whole src="…" token so it can't
  also rewrite the same URL where it appears in srcset.
@jkmassel
jkmassel force-pushed the jkmassel/richcontentformatter-utf16-range branch from abfb667 to 1235488 Compare September 3, 2026 21:32
@jkmassel
jkmassel added this pull request to the merge queue Sep 3, 2026
Merged via the queue into trunk with commit 2da435e Sep 3, 2026
28 checks passed
@jkmassel
jkmassel deleted the jkmassel/richcontentformatter-utf16-range branch September 3, 2026 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants