fix: escape processing-instruction content under fallback raw-content elements - #37
Open
SnowingFox wants to merge 1 commit into
Open
fix: escape processing-instruction content under fallback raw-content elements#37SnowingFox wants to merge 1 commit into
SnowingFox wants to merge 1 commit into
Conversation
… elements Fixes #70146
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Fixes angular/angular#70146
What
This is the remaining piece of the #70050 / #70055 fallback raw-content escaping work. Those fixes (
fc7e40a,e0779df) added closing-tag escaping to the element, text and comment branches ofserializeOne(), but the PROCESSING_INSTRUCTION_NODE branch (case 7) was never covered.escapeProcessingInstructionContent()escapes only>and deliberately leaves<alone. Under a fallback raw-content ancestor (noscript,iframe,noembed,noframes) the browser tokenizer looks only for</tag; a processing-instruction payload such as</noscriptpassescreateProcessingInstruction()validation (only?>is rejected inlib/Document.js) and is emitted unescaped. The fallback element then closes early in the browser and sibling markup (e.g.<img src=x onerror=alert(1)>) is re-parsed as live HTML.Fix
Mirror the comment branch (
case 8): afterescapeProcessingInstructionContent(), when the content contains</, escape the matching closing tag for every fallback raw-content ancestor returned byfallbackRawContentTags(parent).For the issue's payload the output becomes
<?x </noscript ?>instead of<?x </noscript ?>. The 11 breaking shapes described in the issue are covered, and the control shapes from #70050 / #70055 are unchanged.Verification
Added regression tests in
test/domino.js. They are deliberately kept free of the puppeteer/browser dependency thattest/xss.jsuses (thebeforehook there launches a browser), so the serializer behavior is exercised directly and deterministically:processingInstructionClosingTagEscapedInNoscriptreproduces the exact issue shape (<noscript><?x </noscript ?><img src="x" onerror="alert(1)">), asserting the escaped serialized output and that the PI payload leaks no unescaped<?x </noscript.processingInstructionClosingTagEscapedForAllFallbackElementscovers all four fallback elements (noscript,iframe,noembed,noframes) with a</tag/payload.Verified with
pnpm exec mocha test/domino.js -g processingInstruction: the tests fail onmain(the</noscriptprefix is emitted unescaped) and pass with the fix.Notes
The issue is tracked in angular/angular; the fix itself lives in the vendored
dominoserializer, so this PR targets angular/domino (this is where the #70050 / #70055 fixes also landed). A follow-up dependency bump in angular/angular will be needed to pick this up once released.