fix(core): escape overlapping comment delimiters in escapeCommentText#69343
fix(core): escape overlapping comment delimiters in escapeCommentText#69343rootvector2 wants to merge 1 commit into
Conversation
|
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. |
|
This is only a dev-mode thing so this is not considered a vulnerability, but it's a good thing to do. |
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.
`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.
7ebdeea to
0eb2c24
Compare
|
dropped the loop and removed the on the vector: and agreed, this is dev-mode only and not a real vuln, just tightening the helper. |
PR Checklist
PR Type
What is the current behavior?
Issue Number: N/A
escapeCommentTextmatchesCOMMENT_DISALLOWEDglobally, and global matches never overlap. when two delimiter sequences share characters the second is skipped, soescapeCommentText('<!-->')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.tsfor the overlapping cases.