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.
  • Loading branch information
@aduh95
aduh95 committed Jun 18, 2022
1 parent 1737c77 commit 71cf9de
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 19 deletions.
23 changes: 23 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
View file
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
57 changes: 38 additions & 19 deletions tools/eslint-rules/avoid-prototype-pollution.js
View file
@@ -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 Down

0 comments on commit 71cf9de

Please sign in to comment.