Harden RichContentFormatter sanitization + add a comprehensive test suite - #25985
Merged
Conversation
jkmassel
changed the title
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 34359 | |
| Version | PR #25985 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 7553149 | |
| Installation URL | 7skgdee4ue210 |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 34359 | |
| Version | PR #25985 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 7553149 | |
| Installation URL | 2kjskvp50s0jo |
jkmassel
changed the title
jkmassel
force-pushed
the
jkmassel/richcontentformatter-tests
branch
from
September 3, 2026 03:46
e907839 to
76e1945
Compare
jkmassel
changed the base branch from
jkmassel/wordpressdata-swift-test
to
jkmassel/richcontentformatter-utf16-range
September 3, 2026 03:46
jkmassel
changed the title
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.
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.
~75 parameterized Swift Testing cases across every sanitization method (quote, whitespace, and case variants, multiple matches, empty input, no-ops), plus an iOS srcset-preservation regression test for resizeGalleryImageURL.
jkmassel
force-pushed
the
jkmassel/richcontentformatter-tests
branch
from
September 3, 2026 21:32
76e1945 to
7553149
Compare
crazytonyli
approved these changes
Sep 3, 2026
Base automatically changed from
jkmassel/richcontentformatter-utf16-range
to
trunk
September 3, 2026 22:12
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
3 participants
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.
Summary
Stacked on #25833, which fixes the
String.count/UTF-16NSRangebug, hardensparseValueForAttributeagainst a missing closing quote, and migrates theRichContentFormattertests to Swift Testing. This PR adds the remaining sanitization fixes the tests + a follow-up audit exposed, plus a comprehensive sanitization test suite.Fixes (on top of #25833)
<script>/<style>are now stripped through end of input, not only closed pairs (fail-closed).style=now requires a whitespace boundary so it no longer matches inside a longer name likedata-style/data-mce-style.formatVideoTagsdetects thecontrolsattribute as a word-boundaried, case-insensitive token — so a value/name containingcontrols(poster="…/controls.jpg",class="…-controls",data-controls,controlslist) no longer leaves a video uncontrolled, andCONTROLSisn't duplicated; the<video>match is tightened against<videoxyz>, andcontrolsis inserted in place, preserving the tag's casing.parseValueForAttributematches the attribute name on a word boundary, so"rc"no longer returnssrc's value. (Upgrades Fix grapheme-vs-UTF-16 range bug in RichContentFormatter #25833's crash guard to a value-capturing regex, which also keeps its no-crash guarantee.)resizeGalleryImageURLanchors thesrcrewrite to the wholesrc="…"token, so it can't also rewrite the same URL where it appears insrcset.Tests
A ~75-case parameterized Swift Testing suite (
RichContentFormatterSanitizationTests) covering every sanitization method — quote/whitespace/case variants, multiple matches, empty input, no-ops — plus an iOSsrcset-preservation regression test. It coexists with #25833's migratedRichContentFormatterTests; all suites run green together (39 tests / 10 suites on the macOS host).Owned by #25833 (not here)
The
String.count→UTF-16NSRangefix, theremoveTrailingBreakTagsoffsetBy→Range(_:in:)fix, theparseValueForAttributemissing-quote guard, and the Swift Testing test migration.Known limitation (not addressed)
This is not an XSS sanitizer: event-handler attributes (
onerror/onclick),javascript:/data:URLs, and<iframe>/<object>pass through — a best-effort regex limitation, relevant only if output reaches a JS-enabledWKWebView. A follow-up rewrite on SwiftSoup (already aWordPressShareddependency) would dissolve the whole regex-fragility class; this suite is the behavioral contract for it.Test plan
swift test --filter RichContentFormatter(macOS host) — 39 tests / 10 suites pass (Fix grapheme-vs-UTF-16 range bug in RichContentFormatter #25833's migrated suite + this suite), no known issues, no crash.swift-formatstable; SwiftLint clean on all changed files.KeystoneTestsgallery tests that exerciseresizeGalleryImageURL).