fix(core): escape overlapping comment delimiters in escapeCommentText - #69343
thePunderWoman merged 1 commit into
Conversation
Sorry, something went wrong.
|
Before we get such a fix in, it would be interesting to understand how this could be used as a attack vector and that vector be covered by a test. |
Sorry, something went wrong.
|
This is only a dev-mode thing so this is not considered a vulnerability, but it's a good thing to do. |
Sorry, something went wrong.
alan-agius4
left a comment
There was a problem hiding this comment.
Instead of doing a loop the regular expression, the ^ anchor should be removed.
Sorry, something went wrong.
`COMMENT_DISALLOWED` is matched globally, so overlapping delimiter sequences are skipped: `<!-->` only escapes the leading `<!--` and leaves a live `-->` that can close a programmatically created comment node early. Drop the `^` anchors so a standalone `>`/`->` is escaped wherever it appears, which neutralizes the trailing delimiter left behind by an earlier match.
|
dropped the loop and removed the ^ anchors instead, so a lone > or -> gets escaped wherever it sits. that catches the trailing > left over after <!-- matches in <!-->, which is the case the loop was working around. only spec churn is the two .>/.-> cases that now get escaped. on the vector: escapeCommentText runs on values written into programmatically created comment nodes (the SSR path called out in the docstring). with the old regex escapeCommentText('<!-->') keeps a live -->, so <!--><img src=x onerror=...> serializes to a comment that closes early and the <img> lands as live markup. added a test that round-trips a comment node through innerHTML and checks the payload stays a single comment node with nothing parsed out of it. and agreed, this is dev-mode only and not a real vuln, just tightening the helper. |
Sorry, something went wrong.
alan-agius4
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
This PR was merged into the repository. The changes were merged into the following branches: |
Sorry, something went wrong.
|
@rootvector2 Looks like we have to revert this due to several broken targets inside G3. The revert is here: #69426 |
Sorry, something went wrong.
|
makes sense. the breakage is from dropping the ^ anchors: a lone > or -> now gets escaped anywhere in comment text, not just at a delimiter boundary, so any comment that contains a > changes output and trips the snapshot/golden targets in g3. the overlap can be closed without that churn by keeping the anchored regex and re-running the replacement until it stabilizes. non-overlapping text stays byte-for-byte identical and only <!--> / <!--!> get touched. happy to send that as a follow-up if you want it. |
Sorry, something went wrong.
|
This pull request has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sorry, something went wrong.
PR Checklist
PR Type
What is the current behavior?
Issue Number: N/A
escapeCommentText matches COMMENT_DISALLOWED globally, and global matches never overlap. when two delimiter sequences share characters the second is skipped, so escapeCommentText('<!-->') only escapes the leading <!-- and returns text that still contains a live --> (same for <!--!> and any value embedding such a sequence). the helper exists to keep programmatically created comment nodes from closing early, which its own docstring calls out as an XSS vector, so that surviving --> lets a value like <!--><img onerror=...> close the comment and run as markup.
found while auditing the comment/dom escaping helpers.
What is the new behavior?
the existing replacement is re-run until the text stabilizes, so overlapping delimiters are all neutralized. non-overlapping inputs are byte-for-byte unchanged (existing specs still pass).
Does this PR introduce a breaking change?
Other information
added regression tests in dom_spec.ts for the overlapping cases.