Skip to content

Harden RichContentFormatter sanitization + add a comprehensive test suite - #25985

Open
jkmassel wants to merge 2 commits into
jkmassel/richcontentformatter-utf16-rangefrom
jkmassel/richcontentformatter-tests
Open

Harden RichContentFormatter sanitization + add a comprehensive test suite#25985
jkmassel wants to merge 2 commits into
jkmassel/richcontentformatter-utf16-rangefrom
jkmassel/richcontentformatter-tests

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Stacked on #25833, which fixes the String.count/UTF-16 NSRange bug, hardens parseValueForAttribute against a missing closing quote, and migrates the RichContentFormatter tests to Swift Testing. This PR adds the remaining sanitization fixes the tests + a follow-up audit exposed, plus a comprehensive sanitization test suite.

Base is jkmassel/richcontentformatter-utf16-range (#25833) → which stacks on #25832 → trunk. Retargets up the stack as each parent merges.

Fixes (on top of #25833)

  • Unclosed <script>/<style> are now stripped through end of input, not only closed pairs (fail-closed).
  • Single-quoted inline styles are stripped, and style= now requires a whitespace boundary so it no longer matches inside a longer name like data-style / data-mce-style.
  • formatVideoTags detects the controls attribute as a word-boundaried, case-insensitive token — so a value/name containing controls (poster="…/controls.jpg", class="…-controls", data-controls, controlslist) no longer leaves a video uncontrolled, and CONTROLS isn't duplicated; the <video> match is tightened against <videoxyz>, and controls is inserted in place, preserving the tag's casing.
  • parseValueForAttribute matches the attribute name on a word boundary, so "rc" no longer returns src'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.)
  • resizeGalleryImageURL anchors the src rewrite to the whole src="…" token, so it can't also rewrite the same URL where it appears in srcset.

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 iOS srcset-preservation regression test. It coexists with #25833's migrated RichContentFormatterTests; all suites run green together (39 tests / 10 suites on the macOS host).

Owned by #25833 (not here)

The String.count→UTF-16 NSRange fix, the removeTrailingBreakTags offsetByRange(_:in:) fix, the parseValueForAttribute missing-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-enabled WKWebView. A follow-up rewrite on SwiftSoup (already a WordPressShared dependency) 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-format stable; SwiftLint clean on all changed files.
  • CI green (incl. the iOS-hosted KeystoneTests gallery tests that exercise resizeGalleryImageURL).

@jkmassel jkmassel added the Testing Unit and UI Tests and Tooling label Sep 3, 2026
@jkmassel jkmassel self-assigned this Sep 3, 2026
@jkmassel jkmassel added this to the 27.3 milestone Sep 3, 2026
@jkmassel jkmassel changed the title Add RichContentFormatter sanitization test suite Add RichContentFormatter sanitization tests and fix an attribute-parsing crash Sep 3, 2026
@wpmobilebot

wpmobilebot commented Sep 3, 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 Number34347
VersionPR #25985
Bundle IDorg.wordpress.alpha
Commit76e1945
Installation URL6u2crami4tctg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Sep 3, 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 Number34347
VersionPR #25985
Bundle IDcom.jetpack.alpha
Commit76e1945
Installation URL2npqogdffqus8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jkmassel jkmassel changed the title Add RichContentFormatter sanitization tests and fix an attribute-parsing crash Test RichContentFormatter sanitization and fix the gaps it exposes 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.
~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
jkmassel force-pushed the jkmassel/richcontentformatter-tests branch from e907839 to 76e1945 Compare September 3, 2026 03:46
@jkmassel
jkmassel changed the base branch from jkmassel/wordpressdata-swift-test to jkmassel/richcontentformatter-utf16-range September 3, 2026 03:46
@jkmassel jkmassel changed the title Test RichContentFormatter sanitization and fix the gaps it exposes Harden RichContentFormatter sanitization + add a comprehensive test suite Sep 3, 2026
@jkmassel
jkmassel requested a review from crazytonyli September 3, 2026 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing Unit and UI Tests and Tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants