Fix grapheme-vs-UTF-16 range bug in RichContentFormatter - #25833
Merged
Conversation
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 34358 | |
| Version | PR #25833 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 1235488 | |
| Installation URL | 396c4kc1u198g |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 34358 | |
| Version | PR #25833 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 1235488 | |
| Installation URL | 22bhrg98q33kg |
jkmassel
force-pushed
the
jkmassel/richcontentformatter-utf16-range
branch
6 times, most recently
from
July 24, 2026 18:34
af67b42 to
ea3df4b
Compare
jkmassel
force-pushed
the
jkmassel/richcontentformatter-utf16-range
branch
from
September 1, 2026 23:30
ea3df4b to
f472041
Compare
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
jkmassel
force-pushed
the
jkmassel/richcontentformatter-utf16-range
branch
from
September 2, 2026 00:21
f472041 to
53a1bf3
Compare
jkmassel
force-pushed
the
jkmassel/richcontentformatter-utf16-range
branch
2 times, most recently
from
September 3, 2026 03:36
ab1cdc9 to
abfb667
Compare
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.
3 tasks
jkmassel
marked this pull request as ready for review
September 3, 2026 03:52
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
force-pushed
the
jkmassel/richcontentformatter-utf16-range
branch
from
September 3, 2026 21:32
abfb667 to
1235488
Compare
crazytonyli
approved these changes
Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


What this fixes
RichContentFormattersized theNSRangeit hands toNSRegularExpressionfromcontent.count— Swift's grapheme-cluster count — butNSRegularExpressionmatches 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-imagesrcnear 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
removeForbiddenTags,normalizeParagraphs,filterNewLines, andremoveInlineStylesnow come fromString.utf16.count, andremoveTrailingBreakTagsranges over UTF-16 too.removeTrailingBreakTagsno longer mixes offset spaces. It fed a UTF-16match.range.locationtoString.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 withRange(match.range, in: content).resizeGalleryImageURLhad the same bug. This display-pipeline step rewrites a gallery image'ssrcto a Photon/resized URL; it sized the replacement range fromimgElementStr.count, so asrcsitting past a multibyte cluster was never rewritten and the full-size original loaded instead. It runs on rendered comments (CommentService→formatContentString). Now sized in UTF-16.parseValueForAttributeagainst malformed markup. A separate crash in the same file: it searched for an attribute's closing quote and passed the result tosubstring(with:)unguarded, so an opening quote with no close (NSNotFound) underflowed the range length to an out-of-boundsNSRangeand trapped. It now returns""when the closing quote is missing, matching the attribute-not-found default.RichContentFormattertest files move from XCTest to@Test/#expect, matching the rest of theWordPressSharedTeststarget — same inputs, same assertions.formatGutenbergGalleryandformatVideoTagsalready ranged overNSString.lengthand are untouched.The whole-string ranges are sized with
content.utf16.countinline, 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
parseValueForAttributehardening.Tests
One isolated test per fix site — each forbidden-tag regex, each
normalizeParagraphssub-site, the threefilterNewLinespaths, 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.parseValueForAttributegets three direct tests, including a malformed element (opening quote, no close) that crashes the pre-fix code with an out-of-boundsNSRangeand returns""after the fix.resizeGalleryImageURL's regression lives inRichContentFormatterUITests— it needs UIKit, so it runs in the iOSWordPressUnitTestsplan, not the macOSswift testset.Stacking
Stacked on #25832 (base
jkmassel/wordpressdata-swift-test), which removes WordPressData'sWordPressUI/WordPressSharedUIdependencies and, as part of that, splitsRichContentFormatter's platform-independent core into cross-platformWordPressShared— which is what lets this fix and its regression tests run on macOS underswift test. The bug is pre-existing; it's also ontrunk, in the pre-splitWordPressSharedUIcopy. This PR retargets totrunkonce #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); theparseValueForAttributemissing-closing-quote test traps the pre-fix code with an out-of-boundsNSRange.WordPressUnitTestsplan (or rely on CI):RichContentFormatterUITests.testResizeGalleryImageURLReplacesSrcPastMultibyteCluster— a gallery<img data-orig-file=… alt="😀😀😀😀😀" src=…/>comes back with itssrcswapped to a*.wp.comURL (0 replacements on the pre-fix source).