Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: escape unsafe patterns in elements, comment nodes and processing instructions #18

Merged
merged 1 commit into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ var domino = require('domino');
Domino includes test from the [W3C DOM Conformance Suites](http://www.w3.org/DOM/Test/)
as well as tests from [HTML Working Group](http://www.w3.org/html/wg/wiki/Testing).

When you checkout this repository for the first time, run the following command to also check out code for the mentioned tests:

```
git submodule update --init --recursive
```

The tests can be run via `npm test` or directly though the [Mocha](http://mochajs.org/) command line:

![Screenshot](http://fgnass.github.com/images/domino.png)
Expand Down
66 changes: 63 additions & 3 deletions lib/NodeUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ module.exports = {
// `NodeUtils.serializeOne` to get to the function and reduce pressure
// on the megamorphic stub cache.
// See https://github.com/fgnass/domino/pull/142 for more information.
serializeOne: serializeOne
serializeOne: serializeOne,

// Export util functions so that we can run extra test for them.
// Note: we prefix function names with `ɵ`, similar to what we do
// with internal functions in Angular packages.
ɵescapeMatchingClosingTag: escapeMatchingClosingTag,
ɵescapeClosingCommentTag: escapeClosingCommentTag,
ɵescapeProcessingInstructionContent: escapeProcessingInstructionContent
};

var utils = require('./utils');
Expand Down Expand Up @@ -116,6 +123,53 @@ function attrname(a) {
return a.name;
}

/**
* Escapes matching closing tag in a raw text.
*
* For example, given `<style>#text(</style><script></script>)</style>`,
* the parent tag would by "style" and the raw text is
* "</style><script></script>". If we come across a matching closing tag
* (in out case `</style>`) - replace `<` with `&lt;` to avoid unexpected
* and unsafe behavior after de-serialization.
*/
function escapeMatchingClosingTag(rawText, parentTag) {
AndrewKushnir marked this conversation as resolved.
Show resolved Hide resolved
const parentClosingTag = '</' + parentTag;
if (!rawText.toLowerCase().includes(parentClosingTag)) {
return rawText; // fast path
}
const result = [...rawText];
const matches = rawText.matchAll(new RegExp(parentClosingTag, 'ig'));
for (const match of matches) {
result[match.index] = '&lt;';
}
return result.join('');
}

const CLOSING_COMMENT_REGEXP = /--!?>/;

/**
* Escapes closing comment tag in a comment content.
*
* For example, given `#comment('-->')`, the content of a comment would be
* updated to `--&gt;` to avoid unexpected and unsafe behavior after
* de-serialization.
*/
function escapeClosingCommentTag(rawContent) {
AndrewKushnir marked this conversation as resolved.
Show resolved Hide resolved
if (!CLOSING_COMMENT_REGEXP.test(rawContent)) {
return rawContent; // fast path
}
return rawContent.replace(/(--\!?)>/g, '$1&gt;');
}

/**
* Escapes processing instruction content by replacing `>` with `&gt`.
*/
function escapeProcessingInstructionContent(rawContent) {
return rawContent.includes('>')
? rawContent.replaceAll('>', '&gt;')
: rawContent;
}

function serializeOne(kid, parent) {
var s = '';
switch(kid.nodeType) {
Expand All @@ -135,6 +189,11 @@ function serializeOne(kid, parent) {

if (!(html && emptyElements[tagname])) {
var ss = kid.serialize();
// If an element can have raw content, this content may
// potentially require escaping to avoid XSS.
if (hasRawContent[tagname.toUpperCase()]) {
ss = escapeMatchingClosingTag(ss, tagname);
}
if (html && extraNewLine[tagname] && ss.charAt(0)==='\n') s += '\n';
// Serialize children and add end tag for all others
s += ss;
Expand All @@ -158,10 +217,11 @@ function serializeOne(kid, parent) {
}
break;
case 8: //COMMENT_NODE
s += '<!--' + kid.data + '-->';
s += '<!--' + escapeClosingCommentTag(kid.data) + '-->';
break;
case 7: //PROCESSING_INSTRUCTION_NODE
s += '<?' + kid.target + ' ' + kid.data + '?>';
const content = escapeProcessingInstructionContent(kid.data);
s += '<?' + kid.target + ' ' + content + '?>';
break;
case 10: //DOCUMENT_TYPE_NODE
s += '<!DOCTYPE ' + kid.name;
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"devDependencies": {
"jquery": "^3.5.1",
"mocha": "^6.2.3",
"puppeteer": "^21.3.5",
"should": "^13.2.3"
}
}
Loading