Skip to content

fix: escape processing-instruction content under fallback raw-content elements - #37

Open
SnowingFox wants to merge 1 commit into
angular:mainfrom
SnowingFox:fix/70146-pi-noscript-breakout
Open

fix: escape processing-instruction content under fallback raw-content elements#37
SnowingFox wants to merge 1 commit into
angular:mainfrom
SnowingFox:fix/70146-pi-noscript-breakout

Conversation

@SnowingFox

Copy link
Copy Markdown

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 of serializeOne(), 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 </noscript passes createProcessingInstruction() validation (only ?> is rejected in lib/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): after escapeProcessingInstructionContent(), when the content contains </, escape the matching closing tag for every fallback raw-content ancestor returned by fallbackRawContentTags(parent).

case 7: //PROCESSING_INSTRUCTION_NODE
  let content = escapeProcessingInstructionContent(kid.data);
  if (content.includes('</')) {
    const fallbackTags = fallbackRawContentTags(parent);
    for (const fallbackTag of fallbackTags) {
      content = escapeMatchingClosingTag(content, fallbackTag);
    }
  }
  s += '<?' + kid.target + ' ' + content + '?>';
  break;

For the issue's payload the output becomes <?x &lt;/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 that test/xss.js uses (the before hook there launches a browser), so the serializer behavior is exercised directly and deterministically:

  • processingInstructionClosingTagEscapedInNoscript reproduces 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.
  • processingInstructionClosingTagEscapedForAllFallbackElements covers 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 on main (the </noscript prefix is emitted unescaped) and pass with the fix.

Notes

The issue is tracked in angular/angular; the fix itself lives in the vendored domino serializer, 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.

@google-cla

google-cla Bot commented Aug 11, 2026

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incomplete fix for #70050 / #70055: </noscript> breakout still reachable through processing instructions

1 participant