tools: refactor avoid-prototype-pollution lint rule

lib/internal/debugger/inspect_repl.js

@@ -23,7 +23,6 @@ const {
   ObjectKeys,
   ObjectValues,
   Promise,
-  PromisePrototypeCatch,
   PromisePrototypeThen,
   PromiseResolve,
   ReflectGetOwnPropertyDescriptor,
@@ -653,8 +652,8 @@ function createRepl(inspector) {
     }
 
     const inspectValue = (expr) =>
-      PromisePrototypeCatch(evalInCurrentContext(expr),
-                            (error) => `<${error.message}>`);
+      PromisePrototypeThen(evalInCurrentContext(expr), undefined,
+                           (error) => `<${error.message}>`);
     const lastIndex = watchedExpressions.length - 1;
 
     const values = await SafePromiseAll(watchedExpressions, inspectValue);

lib/internal/fs/cp/cp.js

@@ -6,11 +6,9 @@ const {
   ArrayPrototypeEvery,
   ArrayPrototypeFilter,
   Boolean,
-  PromiseAll,
-  PromisePrototypeCatch,
   PromisePrototypeThen,
   PromiseReject,
-  SafeArrayIterator,
+  SafePromiseAll,
   StringPrototypeSplit,
 } = primordials;
 const {
@@ -128,13 +126,13 @@ function getStats(src, dest, opts) {
   const statFunc = opts.dereference ?
     (file) => stat(file, { bigint: true }) :
     (file) => lstat(file, { bigint: true });
-  return PromiseAll(new SafeArrayIterator([
+  return SafePromiseAll([
     statFunc(src),
-    PromisePrototypeCatch(statFunc(dest), (err) => {
+    PromisePrototypeThen(statFunc(dest), undefined, (err) => {
       if (err.code === 'ENOENT') return null;
       throw err;
     }),
-  ]));
+  ]);
 }
 
 async function checkParentDir(destStat, src, dest, opts) {

lib/internal/modules/esm/loader.js

@@ -13,9 +13,8 @@ const {
   ObjectCreate,
   ObjectDefineProperty,
   ObjectSetPrototypeOf,
-  PromiseAll,
   RegExpPrototypeExec,
-  SafeArrayIterator,
+  SafePromiseAll,
   SafeWeakMap,
   StringPrototypeSlice,
   StringPrototypeToUpperCase,
@@ -525,7 +524,7 @@ class ESMLoader {
         .then(({ module }) => module.getNamespace());
     }
 
-    const namespaces = await PromiseAll(new SafeArrayIterator(jobs));
+    const namespaces = await SafePromiseAll(jobs);
 
     if (!wasArr) { return namespaces[0]; } // We can skip the pairing below
 

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);
 }
 

lib/internal/webstreams/adapters.js

@@ -1,11 +1,10 @@
 'use strict';
 
 const {
-  ArrayPrototypeMap,
-  PromiseAll,
   PromisePrototypeThen,
-  PromisePrototypeFinally,
   PromiseResolve,
+  SafePromiseAll,
+  SafePromisePrototypeFinally,
   Uint8Array,
 } = primordials;
 
@@ -165,7 +164,7 @@ function newWritableStreamFromStreamWritable(streamWritable) {
     async write(chunk) {
       if (streamWritable.writableNeedDrain || !streamWritable.write(chunk)) {
         backpressurePromise = createDeferredPromise();
-        return PromisePrototypeFinally(
+        return SafePromisePrototypeFinally(
           backpressurePromise.promise, () => {
             backpressurePromise = undefined;
           });
@@ -246,10 +245,9 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj
         writer.ready,
         () => {
           return PromisePrototypeThen(
-            PromiseAll(
-              ArrayPrototypeMap(
-                chunks,
-                (data) => writer.write(data.chunk))),
+            SafePromiseAll(
+              chunks,
+              (data) => writer.write(data.chunk)),
             done,
             done);
         },
@@ -668,10 +666,9 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options =
         writer.ready,
         () => {
           return PromisePrototypeThen(
-            PromiseAll(
-              ArrayPrototypeMap(
-                chunks,
-                (data) => writer.write(data.chunk))),
+            SafePromiseAll(
+              chunks,
+              (data) => writer.write(data.chunk)),
             done,
             done);
         },
@@ -767,7 +764,7 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options =
 
       if (!writableClosed || !readableClosed) {
         PromisePrototypeThen(
-          PromiseAll([
+          SafePromiseAll([
             closeWriter(),
             closeReader(),
           ]),

lib/internal/webstreams/readablestream.js

@@ -19,8 +19,8 @@ const {
   PromisePrototypeThen,
   PromiseResolve,
   PromiseReject,
-  PromiseAll,
   ReflectConstruct,
+  SafePromiseAll,
   Symbol,
   SymbolAsyncIterator,
   SymbolToStringTag,
@@ -1334,7 +1334,7 @@ function readableStreamPipeTo(
     }
 
     shutdownWithAnAction(
-      async () => PromiseAll(actions.map((action) => action())),
+      () => SafePromiseAll(actions, (action) => action()),
       true,
       error);
   }

lib/internal/webstreams/transformstream.js

@@ -4,7 +4,6 @@ const {
   FunctionPrototypeBind,
   FunctionPrototypeCall,
   ObjectDefineProperties,
-  PromisePrototypeCatch,
   PromisePrototypeThen,
   PromiseResolve,
   ReflectConstruct,
@@ -496,19 +495,17 @@ function transformStreamDefaultControllerError(controller, error) {
   transformStreamError(controller[kState].stream, error);
 }
 
-function transformStreamDefaultControllerPerformTransform(controller, chunk) {
-  const transformPromise =
-    ensureIsPromise(
+async function transformStreamDefaultControllerPerformTransform(controller, chunk) {
+  try {
+    return await ensureIsPromise(
       controller[kState].transformAlgorithm,
       controller,
       chunk,
       controller);
-  return PromisePrototypeCatch(
-    transformPromise,
-    (error) => {
-      transformStreamError(controller[kState].stream, error);
-      throw error;
-    });
+  } catch (error) {
+    transformStreamError(controller[kState].stream, error);
+    throw error;
+  }
 }
 
 function transformStreamDefaultControllerTerminate(controller) {

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/ }],

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,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}`,
         });
       },
     };