Skip to content

Commit

Permalink
tools: report unsafe string and regex primordials as lint errors
Browse files Browse the repository at this point in the history
| The string method             | looks up the property |
| ----------------------------- | --------------------- |
| `String.prototype.match`      | `Symbol.match`        |
| `String.prototype.matchAll`   | `Symbol.matchAll`     |
| `String.prototype.replace`    | `Symbol.replace`      |
| `String.prototype.replaceAll` | `Symbol.replace`      |
| `String.prototype.search`     | `Symbol.search`       |
| `String.prototype.split`      | `Symbol.split`        |

Functions that lookup the `exec` property on the prototype chain:

* `RegExp.prototype[Symbol.match]`
* `RegExp.prototype[Symbol.matchAll]`
* `RegExp.prototype[Symbol.replace]`
* `RegExp.prototype[Symbol.search]`
* `RegExp.prototype[Symbol.split]`
* `RegExp.prototype.test`

`RegExp.prototype[Symbol.replace]` and `RegExp.prototype[Symbol.split]`
are still allowed for a lack of a better solution.

PR-URL: nodejs/node#43393
Backport-PR-URL: nodejs/node#44081
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
aduh95 authored and guangwong committed Oct 10, 2022
1 parent e404223 commit 549bdb2
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 17 deletions.
32 changes: 16 additions & 16 deletions lib/_tls_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const {
ArrayPrototypePush,
JSONParse,
ObjectCreate,
StringPrototypeReplace,
RegExpPrototypeSymbolReplace,
} = primordials;

const {
Expand Down Expand Up @@ -142,21 +142,21 @@ function translatePeerCertificate(c) {
c.infoAccess = ObjectCreate(null);

// XXX: More key validation?
StringPrototypeReplace(info, /([^\n:]*):([^\n]*)(?:\n|$)/g,
(all, key, val) => {
if (val.charCodeAt(0) === 0x22) {
// The translatePeerCertificate function is only
// used on internally created legacy certificate
// objects, and any value that contains a quote
// will always be a valid JSON string literal,
// so this should never throw.
val = JSONParse(val);
}
if (key in c.infoAccess)
ArrayPrototypePush(c.infoAccess[key], val);
else
c.infoAccess[key] = [val];
});
RegExpPrototypeSymbolReplace(/([^\n:]*):([^\n]*)(?:\n|$)/g, info,
(all, key, val) => {
if (val.charCodeAt(0) === 0x22) {
// The translatePeerCertificate function is only
// used on internally created legacy certificate
// objects, and any value that contains a quote
// will always be a valid JSON string literal,
// so this should never throw.
val = JSONParse(val);
}
if (key in c.infoAccess)
ArrayPrototypePush(c.infoAccess[key], val);
else
c.infoAccess[key] = [val];
});
}
return c;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ function REPLServer(prompt,

// This will set the values from `savedRegExMatches` to corresponding
// predefined RegExp properties `RegExp.$1`, `RegExp.$2` ... `RegExp.$9`
RegExpPrototypeTest(regExMatcher,
RegExpPrototypeExec(regExMatcher,
ArrayPrototypeJoin(savedRegExMatches, sep));

let finished = false;
Expand Down
40 changes: 40 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,5 +143,45 @@ new RuleTester({
code: 'ReflectDefineProperty({}, "key", { enumerable: true })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'RegExpPrototypeTest(/some regex/, "some string")',
errors: [{ message: /looks up the "exec" property/ }],
},
{
code: 'RegExpPrototypeSymbolMatch(/some regex/, "some string")',
errors: [{ message: /looks up the "exec" property/ }],
},
{
code: 'RegExpPrototypeSymbolMatchAll(/some regex/, "some string")',
errors: [{ message: /looks up the "exec" property/ }],
},
{
code: 'RegExpPrototypeSymbolSearch(/some regex/, "some string")',
errors: [{ message: /looks up the "exec" property/ }],
},
{
code: 'StringPrototypeMatch("some string", /some regex/)',
errors: [{ message: /looks up the Symbol\.match property/ }],
},
{
code: 'StringPrototypeMatchAll("some string", /some regex/)',
errors: [{ message: /looks up the Symbol\.matchAll property/ }],
},
{
code: 'StringPrototypeReplace("some string", /some regex/, "some replacement")',
errors: [{ message: /looks up the Symbol\.replace property/ }],
},
{
code: 'StringPrototypeReplaceAll("some string", /some regex/, "some replacement")',
errors: [{ message: /looks up the Symbol\.replace property/ }],
},
{
code: 'StringPrototypeSearch("some string", /some regex/)',
errors: [{ message: /looks up the Symbol\.search property/ }],
},
{
code: 'StringPrototypeSplit("some string", /some regex/)',
errors: [{ message: /looks up the Symbol\.split property/ }],
},
]
});
41 changes: 41 additions & 0 deletions tools/eslint-rules/avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ function checkPropertyDescriptor(context, node) {
});
}

function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
return {
[`${CallExpression}[expression.callee.name=${JSON.stringify(name)}]`](node) {
context.report({
node,
message: `${name} looks up the ${lookedUpProperty} property on the first argument`,
});
}
};
}

const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]';
module.exports = {
meta: { hasSuggestions: true },
Expand All @@ -87,6 +98,36 @@ module.exports = {
[`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) {
checkProperties(context, node.expression.arguments[1]);
},
[`${CallExpression}[expression.callee.name="RegExpPrototypeTest"]`](node) {
context.report({
node,
message: '%RegExp.prototype.test% looks up the "exec" property of `this` value',
suggest: [{
desc: 'Use RegexpPrototypeExec instead',
fix(fixer) {
const testRange = { ...node.range };
testRange.start = testRange.start + 'RegexpPrototype'.length;
testRange.end = testRange.start + 'Test'.length;
return [
fixer.replaceTextRange(node.range, 'Exec'),
fixer.insertTextAfter(node, ' !== null'),
];
}
}]
});
},
[`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) {
context.report({
node,
message: node.expression.callee.name + ' looks up the "exec" property of `this` value',
});
},
...createUnsafeStringMethodReport(context, 'StringPrototypeMatch', 'Symbol.match'),
...createUnsafeStringMethodReport(context, 'StringPrototypeMatchAll', 'Symbol.matchAll'),
...createUnsafeStringMethodReport(context, 'StringPrototypeReplace', 'Symbol.replace'),
...createUnsafeStringMethodReport(context, 'StringPrototypeReplaceAll', 'Symbol.replace'),
...createUnsafeStringMethodReport(context, 'StringPrototypeSearch', 'Symbol.search'),
...createUnsafeStringMethodReport(context, 'StringPrototypeSplit', 'Symbol.split'),
};
},
};

0 comments on commit 549bdb2

Please sign in to comment.