Skip to content

Commit

Permalink
tools: refactor avoid-prototype-pollution lint rule
Browse files Browse the repository at this point in the history 
The lint rule was not catching all occurences of unsafe primordials use,
and was too strict on some methods.

PR-URL: nodejs#43476
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
  • Loading branch information
@aduh95 @Fyko
aduh95 authored and Fyko committed Sep 15, 2022
1 parent 935a633 commit 57d5b46
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 23 deletions.
6 changes: 6 additions & 0 deletions lib/internal/net.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,16 @@ const IPv6Reg = new RegExp('^(' +
')(%[0-9a-zA-Z-.:]{1,})?$');

function isIPv4(s) {
// TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it
// no longer creates a perf regression in the dns benchmark.
// eslint-disable-next-line node-core/avoid-prototype-pollution
return RegExpPrototypeTest(IPv4Reg, s);
}

function isIPv6(s) {
// TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it
// no longer creates a perf regression in the dns benchmark.
// eslint-disable-next-line node-core/avoid-prototype-pollution
return RegExpPrototypeTest(IPv6Reg, s);
}

Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ new RuleTester({
'ReflectDefineProperty({}, "key", { "__proto__": null })',
'ObjectDefineProperty({}, "key", { \'__proto__\': null })',
'ReflectDefineProperty({}, "key", { \'__proto__\': null })',
'StringPrototypeReplace("some string", "some string", "some replacement")',
'StringPrototypeReplaceAll("some string", "some string", "some replacement")',
'StringPrototypeSplit("some string", "some string")',
'new Proxy({}, otherObject)',
'new Proxy({}, someFactory())',
'new Proxy({}, { __proto__: null })',
Expand Down Expand Up @@ -167,18 +170,38 @@ new RuleTester({
code: 'StringPrototypeMatch("some string", /some regex/)',
errors: [{ message: /looks up the Symbol\.match property/ }],
},
{
code: 'let v = StringPrototypeMatch("some string", /some regex/)',
errors: [{ message: /looks up the Symbol\.match property/ }],
},
{
code: 'let v = StringPrototypeMatch("some string", new RegExp("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: 'let v = StringPrototypeMatchAll("some string", new RegExp("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: 'StringPrototypeReplace("some string", new RegExp("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: 'StringPrototypeReplaceAll("some string", new RegExp("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/ }],
Expand Down
65 changes: 42 additions & 23 deletions tools/eslint-rules/avoid-prototype-pollution.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

const CallExpression = (fnName) => `CallExpression[callee.name=${fnName}]`;

function checkProperties(context, node) {
if (
node.type === 'CallExpression' &&
Expand Down Expand Up @@ -64,8 +66,10 @@ function checkPropertyDescriptor(context, node) {
}

function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
const lastDotPosition = '$String.prototype.'.length;
const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`;
return {
[`${CallExpression}[expression.callee.name=${JSON.stringify(name)}]`](node) {
[CallExpression(unsafePrimordialName)](node) {
context.report({
node,
message: `${name} looks up the ${lookedUpProperty} property on the first argument`,
Expand All @@ -74,31 +78,46 @@ function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
};
}

const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]';
function createUnsafeStringMethodOnRegexReport(context, name, lookedUpProperty) {
const dotPosition = 'Symbol.'.length;
const safePrimordialName = `RegExpPrototypeSymbol${lookedUpProperty.charAt(dotPosition).toUpperCase()}${lookedUpProperty.slice(dotPosition + 1)}`;
const lastDotPosition = '$String.prototype.'.length;
const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`;
return {
[[
`${CallExpression(unsafePrimordialName)}[arguments.1.type=Literal][arguments.1.regex]`,
`${CallExpression(unsafePrimordialName)}[arguments.1.type=NewExpression][arguments.1.callee.name=RegExp]`,
].join(',')](node) {
context.report({
node,
message: `${name} looks up the ${lookedUpProperty} property of the passed regex, use ${safePrimordialName} directly`,
});
}
};
}

module.exports = {
meta: { hasSuggestions: true },
create(context) {
return {
[`${CallExpression}[expression.callee.name=${/^(Object|Reflect)DefinePropert(ies|y)$/}]`](
node
) {
switch (node.expression.callee.name) {
[CallExpression(/^(Object|Reflect)DefinePropert(ies|y)$/)](node) {
switch (node.callee.name) {
case 'ObjectDefineProperties':
checkProperties(context, node.expression.arguments[1]);
checkProperties(context, node.arguments[1]);
break;
case 'ReflectDefineProperty':
case 'ObjectDefineProperty':
checkPropertyDescriptor(context, node.expression.arguments[2]);
checkPropertyDescriptor(context, node.arguments[2]);
break;
default:
throw new Error('Unreachable');
}
},

[`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) {
checkProperties(context, node.expression.arguments[1]);
[`${CallExpression('ObjectCreate')}[arguments.length=2]`](node) {
checkProperties(context, node.arguments[1]);
},
[`${CallExpression}[expression.callee.name="RegExpPrototypeTest"]`](node) {
[CallExpression('RegExpPrototypeTest')](node) {
context.report({
node,
message: '%RegExp.prototype.test% looks up the "exec" property of `this` value',
Expand All @@ -116,18 +135,18 @@ module.exports = {
}],
});
},
[`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) {
[CallExpression(/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/)](node) {
context.report({
node,
message: node.expression.callee.name + ' looks up the "exec" property of `this` value',
message: node.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'),
...createUnsafeStringMethodReport(context, '%String.prototype.match%', 'Symbol.match'),
...createUnsafeStringMethodReport(context, '%String.prototype.matchAll%', 'Symbol.matchAll'),
...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replace%', 'Symbol.replace'),
...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replaceAll%', 'Symbol.replace'),
...createUnsafeStringMethodReport(context, '%String.prototype.search%', 'Symbol.search'),
...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.split%', 'Symbol.split'),

'NewExpression[callee.name="Proxy"][arguments.1.type="ObjectExpression"]'(node) {
for (const { key, value } of node.arguments[1].properties) {
Expand All @@ -146,15 +165,15 @@ module.exports = {
});
},

[`${CallExpression}[expression.callee.name=PromisePrototypeCatch]`](node) {
[CallExpression('PromisePrototypeCatch')](node) {
context.report({
node,
message: '%Promise.prototype.catch% look up the `then` property of ' +
'the `this` argument, use PromisePrototypeThen instead',
});
},

[`${CallExpression}[expression.callee.name=PromisePrototypeFinally]`](node) {
[CallExpression('PromisePrototypeFinally')](node) {
context.report({
node,
message: '%Promise.prototype.finally% look up the `then` property of ' +
Expand All @@ -163,10 +182,10 @@ module.exports = {
});
},

[`${CallExpression}[expression.callee.name=${/^Promise(All(Settled)?|Any|Race)/}]`](node) {
[CallExpression(/^Promise(All(Settled)?|Any|Race)/)](node) {
context.report({
node,
message: `Use Safe${node.expression.callee.name} instead of ${node.expression.callee.name}`,
message: `Use Safe${node.callee.name} instead of ${node.callee.name}`,
});
},
};
Expand Down

0 comments on commit 57d5b46

Please sign in to comment.