fix(core): reparse current markup in sanitizer stabilization loop#69836
Open
arshsmith1 wants to merge 1 commit into
Open
fix(core): reparse current markup in sanitizer stabilization loop#69836arshsmith1 wants to merge 1 commit into
arshsmith1 wants to merge 1 commit into
Conversation
The mXSS stabilization loop read `parsedHtml` from an inert body element that was parsed from the previous iteration's string, so from the second iteration onward it recomputed the value it had just assigned to `unsafeHtml` and the comparison was always trivially equal. The loop exited after two iterations whether or not the markup reached a parse fixed point, which also left the `mXSSAttempts` guard unreachable. Reparse the current string inside the loop before comparing it. `DOMParserHelper` separately handed the `<body><remove></remove>` prefixed markup to the `InertDocumentHelper` fallback, which does not strip that sentinel, so the fallback accumulated another `<remove>` on every call and could never reach a fixed point. Keep the prefix in a separate local so the fallback receives the original markup.
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
1 participant
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.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
In
_sanitizeHtml,parsedHtmlis read from aninertBodyElementthat was parsed from the previous iteration's string. From the second iteration onward it recomputes the value just assigned tounsafeHtml, sowhile (unsafeHtml !== parsedHtml)compares a value against itself and the loop always exits after exactly two iterations, whether or not the markup reached a parse fixed point. A consequence worth calling out:mXSSAttemptscan never reach 0, so theinput is unstablethrow is unreachable today.DOMParserHelper.getInertBodyElementreassignshtmlto the<body><remove></remove>prefixed form and then passes that prefixed string to theInertDocumentHelperfallback, which never strips the sentinel. Each call adds another<remove>, so the fallback path has no fixed point either. The broken loop above was masking this.<plaintext>ashows the first one: the tokenizer never leaves the PLAINTEXT state, so each reparse appends another literal</plaintext>and the markup never stabilizes, but sanitization currently reports it as stable and serializes a tree it never re-checked.What is the new behavior?
The loop reparses the current string before comparing, so stabilization is measured against the value actually being tested, and the attempt guard becomes reachable. The fallback keeps the prefix in a separate local and receives the original markup.
I checked the blast radius before/after over 20k generated tag-soup inputs against the real sanitizer: for every input that stabilizes, output is byte-identical. The only inputs whose behavior changes are ones that genuinely have no fixed point (
<plaintext>), which now hit the guard instead of silently returning. Excluding<plaintext>from the generator, 20000/20000 are identical. Existinghtml_sanitizer_specandurl_sanitizer_specpass unchanged; the added spec fails onmainand passes here.Happy to drop the added
<plaintext>spec if you would rather not pin that behavior, since it is the one case where callers see a throw they did not see before.Does this PR introduce a breaking change?
Other information