From 549bdb289d9c28500abdaaff05ba212f7694b8c7 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 15 Jun 2022 23:21:57 +0100 Subject: [PATCH] tools: report unsafe string and regex primordials as lint errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit | 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: https://github.com/nodejs/node/pull/43393 Backport-PR-URL: https://github.com/nodejs/node/pull/44081 Reviewed-By: Tobias Nießen Reviewed-By: James M Snell --- lib/_tls_common.js | 32 +++++++-------- lib/repl.js | 2 +- .../test-eslint-avoid-prototype-pollution.js | 40 ++++++++++++++++++ .../eslint-rules/avoid-prototype-pollution.js | 41 +++++++++++++++++++ 4 files changed, 98 insertions(+), 17 deletions(-) diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 2b5b17436bf..eac85c16b5e 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -27,7 +27,7 @@ const { ArrayPrototypePush, JSONParse, ObjectCreate, - StringPrototypeReplace, + RegExpPrototypeSymbolReplace, } = primordials; const { @@ -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; } diff --git a/lib/repl.js b/lib/repl.js index 20ea65768d2..3d0980dd25d 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -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; diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js index 047def545e9..0af4a0a07a2 100644 --- a/test/parallel/test-eslint-avoid-prototype-pollution.js +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -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/ }], + }, ] }); diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index bf6bd1e0a81..0759960349a 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -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 }, @@ -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'), }; }, };