From 3692510ae4ccc0e60669470f9b4027868d25b707 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 17 Jun 2022 19:12:42 +0200 Subject: [PATCH] tools: refactor `avoid-prototype-pollution` lint rule The lint rule was not catching all occurences of unsafe primordials use, and was too strict on some methods. PR-URL: https://github.com/nodejs/node/pull/43476 Backport-PR-URL: https://github.com/nodejs/node/pull/44926 Reviewed-By: Moshe Atlow --- lib/internal/net.js | 6 ++ .../test-eslint-avoid-prototype-pollution.js | 23 +++++++ .../eslint-rules/avoid-prototype-pollution.js | 65 ++++++++++++------- 3 files changed, 71 insertions(+), 23 deletions(-) diff --git a/lib/internal/net.js b/lib/internal/net.js index 8ae3170228d..625377acd57 100644 --- a/lib/internal/net.js +++ b/lib/internal/net.js @@ -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); } diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js index f10d6ea973b..c30928f56ce 100644 --- a/test/parallel/test-eslint-avoid-prototype-pollution.js +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -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/ }], diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index d59b62f9502..d26a410b1d9 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -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,7 +165,7 @@ module.exports = { }); }, - [`${CallExpression}[expression.callee.name=PromisePrototypeCatch]`](node) { + [CallExpression('PromisePrototypeCatch')](node) { context.report({ node, message: '%Promise.prototype.catch% look up the `then` property of ' + @@ -154,7 +173,7 @@ module.exports = { }); }, - [`${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}`, }); }, };