diff --git a/lib/NodeUtils.js b/lib/NodeUtils.js index 325311e..2c5c183 100644 --- a/lib/NodeUtils.js +++ b/lib/NodeUtils.js @@ -229,18 +229,41 @@ function findCommentEnd(rawText, index) { const CLOSING_COMMENT_REGEXP = /--!?>/; +/** + * Escapes a comment content that abruptly closes the comment. + * + * A comment can not carry content that starts with `>` or `->`: the parser + * closes the comment as soon as it comes across such a sequence right after + * `-->`, which de-serializes into an empty + * comment followed by a live `` element. Escaping the leading `>` keeps + * the content inside the comment, where it stays inert. + */ +function escapeAbruptClosingCommentTag(rawContent) { + if (rawContent.startsWith('>')) { + return '>' + rawContent.slice(1); + } + if (rawContent.startsWith('->')) { + return '->' + rawContent.slice(2); + } + return rawContent; // fast path +} + /** * Escapes closing comment tag in a comment content. * * For example, given `#comment('-->')`, the content of a comment would be * updated to `-->` to avoid unexpected and unsafe behavior after - * de-serialization. + * de-serialization. Content that abruptly closes an empty comment is + * escaped as well, see `escapeAbruptClosingCommentTag()`. */ function escapeClosingCommentTag(rawContent) { - if (!CLOSING_COMMENT_REGEXP.test(rawContent)) { - return rawContent; // fast path + const content = escapeAbruptClosingCommentTag(rawContent); + if (!CLOSING_COMMENT_REGEXP.test(content)) { + return content; // fast path } - return rawContent.replace(/(--\!?)>/g, '$1>'); + return content.replace(/(--\!?)>/g, '$1>'); } /** diff --git a/test/xss.js b/test/xss.js index aa71a01..066aee9 100644 --- a/test/xss.js +++ b/test/xss.js @@ -572,6 +572,20 @@ exports.verifyEscapeClosingCommentTag = function () { [' ', ' ', 'b', '->a-->b'], + ['--->', '--->'], + + // A `>` that does not start the content does not close the comment. + ['a>', 'a>'], + ['-a>', '-a>'], + [' >', ' >'], + ['a->b', 'a->b'], ]; for (const [rawContent, expected] of cases) { NodeUtils.ɵescapeClosingCommentTag(rawContent).should.equal(expected); @@ -987,3 +1001,79 @@ exports.fallbackRawTextProcessingInstructionEscapesAncestorClosingTag = async fu } }; +exports.commentNodeEscapesAbruptClosingComment = async function () { + // A comment content that starts with `>` or `->` is closed by the parser + // right away (the "abrupt-closing-of-empty-comment" parse error), so the rest + // of the content is parsed as live markup. An inert `Comment` node would + // therefore turn into an XSS vector once an SSR response is parsed by the + // browser, unless the leading `>` is escaped. + const cases = [ + { + data: '>', + expected: '', + }, + { + data: '->', + expected: '', + }, + { + data: '>', + expected: '', + }, + ]; + + for (const testCase of cases) { + const document = domino.createDocument(''); + document.body.appendChild(document.createComment(testCase.data)); + + const serialized = document.body.serialize(); + serialized.should.equal(testCase.expected); + + // The comment stays a single, inert comment node across the round trip. + const reparsedDocument = domino.createDocument('' + serialized + ''); + reparsedDocument.getElementsByTagName('img').length.should.equal(0); + reparsedDocument.getElementsByTagName('script').length.should.equal(0); + reparsedDocument.body.childNodes.length.should.equal(1); + reparsedDocument.body.childNodes[0].nodeType.should.equal(8 /* COMMENT_NODE */); + + const html = document.serialize(); + const alerted = await alertFired(html); + alerted.should.equal(false, 'alert fired for: ' + html); + } +}; + +exports.commentNodePreservesNonClosingAngleBrackets = function () { + // Only a `>` that starts the content closes the comment, everything else + // must be left untouched to keep the serialized comment readable. + const document = domino.createDocument(''); + document.body.appendChild(document.createComment(' a > b -> c ')); + + document.body.serialize().should.equal(''); +}; + +exports.fallbackRawTextCommentNodeEscapesAbruptClosingComment = async function () { + const fallbackTags = ['noscript', 'iframe', 'noembed', 'noframes']; + + for (const tag of fallbackTags) { + // An abrupt comment close combined with an ancestor closing tag has to + // escape both, otherwise the payload breaks out of the comment and out of + // the fallback raw-content element. + const document = domino.createDocument(''); + const el = document.createElement(tag); + el.appendChild(document.createComment(`>`)); + document.body.appendChild(el); + + const serialized = document.body.serialize(); + serialized.should.equal(`<${tag}>`); + + const reparsedDocument = domino.createDocument('' + serialized + ''); + reparsedDocument.getElementsByTagName('img').length.should.equal(0); + + const reparsed = reparsedDocument.body.innerHTML; + const alerted = await alertFired(reparsed); + alerted.should.equal( + false, + `alert fired after normal HTML reparse for abrupt comment in <${tag}>: ` + reparsed, + ); + } +};