tools: report unsafe string and regex primordials as lint errors

| 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: #43393 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Jun 16, 2022 · ac9599a · ac9599a
1 parent 0bb84b0
commit ac9599a
Show file tree

Hide file tree

Showing 4 changed files with 98 additions and 17 deletions.
diff --git a/lib/_tls_common.js b/lib/_tls_common.js
@@ -27,7 +27,7 @@ const {
   ArrayPrototypePush,
   JSONParse,
   ObjectCreate,
-  StringPrototypeReplace,
+  RegExpPrototypeSymbolReplace,
 } = primordials;
 
 const {
@@ -134,21 +134,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
@@ -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
@@ -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
@@ -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'),
     };
   },
 };