Skip to content

Commit ea1a3ed

Browse files
rootvector2thePunderWoman
authored andcommitted
fix(core): escape overlapping comment delimiters in escapeCommentText
`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.
1 parent ea18ab2 commit ea1a3ed

2 files changed

Lines changed: 29 additions & 4 deletions

File tree

packages/core/src/util/dom.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
*
1212
* see: https://html.spec.whatwg.org/multipage/syntax.html#comments
1313
*/
14-
const COMMENT_DISALLOWED = /^>|^->|<!--|-->|--!>|<!-$/g;
14+
const COMMENT_DISALLOWED = />|->|<!--|-->|--!>|<!-$/g;
1515
/**
1616
* Delimiter in the disallowed strings which needs to be wrapped with zero with character.
1717
*/

packages/core/test/util/dom_spec.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,35 @@ describe('comment node text escaping', () => {
4040
expect(escapeCommentText('--!>')).toEqual('--!\u200b>\u200b');
4141
expect(escapeCommentText('<!-')).toEqual('\u200b<\u200b!-');
4242

43-
// Things which are OK
44-
expect(escapeCommentText('.>')).toEqual('.>');
45-
expect(escapeCommentText('.->')).toEqual('.->');
43+
// A standalone `>` or `->` is escaped regardless of position so that a delimiter which
44+
// overlaps an earlier match (e.g. the `-->` in `<!-->`) can't survive a single pass.
45+
expect(escapeCommentText('.>')).toEqual('.\u200b>\u200b');
46+
expect(escapeCommentText('.->')).toEqual('.-\u200b>\u200b');
4647
expect(escapeCommentText('<!-.')).toEqual('<!-.');
4748
});
49+
50+
it('should escape delimiters that overlap an earlier match', () => {
51+
// `<!-->` contains both `<!--` and a `-->` that shares its `--`; both must be neutralized.
52+
expect(escapeCommentText('<!-->')).toEqual('\u200b<\u200b!--\u200b>\u200b');
53+
expect(escapeCommentText('<!--!>')).toEqual('\u200b<\u200b!--!\u200b>\u200b');
54+
expect(escapeCommentText('a<!-->b')).toEqual('a\u200b<\u200b!--\u200b>\u200bb');
55+
});
56+
57+
it('should keep an injected payload inside a programmatically created comment', () => {
58+
// `<!-->` closes a comment immediately (the `-->` overlaps the `<!--`), so without escaping
59+
// the trailing markup leaks out of the comment and runs. Round-trip a comment node through
60+
// serialization + re-parsing to confirm the payload stays inert comment text.
61+
const host = document.createElement('div');
62+
host.appendChild(
63+
document.createComment(escapeCommentText('<!--><img src=x onerror="alert(1)">')),
64+
);
65+
66+
const reparsed = document.createElement('div');
67+
reparsed.innerHTML = host.innerHTML;
68+
69+
expect(reparsed.childNodes.length).toBe(1);
70+
expect(reparsed.firstChild!.nodeType).toBe(Node.COMMENT_NODE);
71+
expect(reparsed.getElementsByTagName('img').length).toBe(0);
72+
});
4873
});
4974
});

0 commit comments

Comments
 (0)