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: #43476
Backport-PR-URL: #44926
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
  • Loading branch information
aduh95 authored and richardlau committed Nov 23, 2022
1 parent db31de6 commit 254358c
Showing 3 changed files with 71 additions and 23 deletions.
6 changes: 6 additions & 0 deletions lib/internal/net.js
Original file line number Diff line number Diff line change
@@ -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);
}

23 changes: 23 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
@@ -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 })',
@@ -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/ }],
65 changes: 42 additions & 23 deletions tools/eslint-rules/avoid-prototype-pollution.js
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' &&
@@ -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`,
@@ -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',
@@ -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) {
@@ -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 ' +
@@ -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}`,
});
},
};

0 comments on commit 254358c

Please sign in to comment.