lib: add Promise methods to avoid-prototype-pollution lint rule

PR-URL: nodejs#43849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Fyko · Sep 15, 2022 · d5d16ac · d5d16ac
1 parent 9a544c9
commit d5d16ac
Show file tree

Hide file tree

Showing 10 changed files with 91 additions and 45 deletions.
diff --git a/lib/internal/debugger/inspect.js b/lib/internal/debugger/inspect.js
@@ -11,7 +11,6 @@ const {
   FunctionPrototypeBind,
   Number,
   Promise,
-  PromisePrototypeCatch,
   PromisePrototypeThen,
   PromiseResolve,
   Proxy,
@@ -169,12 +168,17 @@ class NodeInspector {
     process.once('SIGTERM', exitCodeZero);
     process.once('SIGHUP', exitCodeZero);
 
-    PromisePrototypeCatch(PromisePrototypeThen(this.run(), async () => {
-      const repl = await startRepl();
-      this.repl = repl;
-      this.repl.on('exit', exitCodeZero);
-      this.paused = false;
-    }), (error) => process.nextTick(() => { throw error; }));
+    (async () => {
+      try {
+        await this.run();
+        const repl = await startRepl();
+        this.repl = repl;
+        this.repl.on('exit', exitCodeZero);
+        this.paused = false;
+      } catch (error) {
+        process.nextTick(() => { throw error; });
+      }
+    })();
   }
 
   suspendReplWhile(fn) {
@@ -183,16 +187,19 @@ class NodeInspector {
     }
     this.stdin.pause();
     this.paused = true;
-    return PromisePrototypeCatch(PromisePrototypeThen(new Promise((resolve) => {
-      resolve(fn());
-    }), () => {
-      this.paused = false;
-      if (this.repl) {
-        this.repl.resume();
-        this.repl.displayPrompt();
+    return (async () => {
+      try {
+        await fn();
+        this.paused = false;
+        if (this.repl) {
+          this.repl.resume();
+          this.repl.displayPrompt();
+        }
+        this.stdin.resume();
+      } catch (error) {
+        process.nextTick(() => { throw error; });
       }
-      this.stdin.resume();
-    }), (error) => process.nextTick(() => { throw error; }));
+    })();
   }
 
   killChild() {

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -17,7 +17,7 @@ const {
   ObjectDefineProperty,
   ObjectPrototypeHasOwnProperty,
   Promise,
-  PromisePrototypeCatch,
+  PromisePrototypeThen,
   Proxy,
   ReflectApply,
   ReflectGet,
@@ -2456,8 +2456,8 @@ function processHeaders(oldHeaders, options) {
 function onFileUnpipe() {
   const stream = this.sink[kOwner];
   if (stream.ownsFd)
-    PromisePrototypeCatch(this.source.close(),
-                          FunctionPrototypeBind(stream.destroy, stream));
+    PromisePrototypeThen(this.source.close(), undefined,
+                         FunctionPrototypeBind(stream.destroy, stream));
   else
     this.source.releaseFD();
 }

diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js
@@ -8,7 +8,7 @@ const {
   ArrayPrototypePushApply,
   ArrayPrototypeSplice,
   ObjectDefineProperty,
-  PromisePrototypeCatch,
+  PromisePrototypeThen,
   globalThis: { Atomics },
 } = primordials;
 
@@ -185,7 +185,7 @@ port.on('message', (message) => {
       evalScript(name, filename);
     } else if (doEval === 'module') {
       const { evalModule } = require('internal/process/execution');
-      PromisePrototypeCatch(evalModule(filename), (e) => {
+      PromisePrototypeThen(evalModule(filename), undefined, (e) => {
         workerOnGlobalUncaughtException(e, true);
       });
     } else {

diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -8,7 +8,7 @@ const {
   ObjectCreate,
   ObjectSetPrototypeOf,
   PromiseResolve,
-  PromisePrototypeCatch,
+  PromisePrototypeThen,
   ReflectApply,
   RegExpPrototypeExec,
   RegExpPrototypeSymbolReplace,
@@ -88,7 +88,7 @@ class ModuleJob {
     this.linked = link();
     // This promise is awaited later anyway, so silence
     // 'unhandled rejection' warnings.
-    PromisePrototypeCatch(this.linked, noop);
+    PromisePrototypeThen(this.linked, undefined, noop);
 
     // instantiated == deep dependency jobs wrappers are instantiated,
     // and module wrapper is instantiated.

diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js
@@ -409,9 +409,6 @@ const SafePromise = makeSafe(
   }
 );
 
-primordials.PromisePrototypeCatch = (thisPromise, onRejected) =>
-  PromisePrototypeThen(thisPromise, undefined, onRejected);
-
 /**
  * Attaches a callback that is invoked when the Promise is settled (fulfilled or
  * rejected). The resolved value cannot be modified from the callback.

diff --git a/lib/internal/streams/operators.js b/lib/internal/streams/operators.js
@@ -25,7 +25,7 @@ const {
   NumberIsNaN,
   Promise,
   PromiseReject,
-  PromisePrototypeCatch,
+  PromisePrototypeThen,
   Symbol,
 } = primordials;
 
@@ -113,7 +113,7 @@ function map(fn, options) {
         queue.push(kEof);
       } catch (err) {
         const val = PromiseReject(err);
-        PromisePrototypeCatch(val, onDone);
+        PromisePrototypeThen(val, undefined, onDone);
         queue.push(val);
       } finally {
         done = true;

diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js
@@ -16,7 +16,6 @@ const {
   ObjectDefineProperties,
   ObjectSetPrototypeOf,
   Promise,
-  PromisePrototypeCatch,
   PromisePrototypeThen,
   PromiseResolve,
   PromiseReject,
@@ -1334,7 +1333,7 @@ function readableStreamPipeTo(
     if (stream[kState].state === 'errored')
       action(stream[kState].storedError);
     else
-      PromisePrototypeCatch(promise, action);
+      PromisePrototypeThen(promise, undefined, action);
   }
 
   function watchClosed(stream, promise, action) {
@@ -1503,8 +1502,9 @@ function readableStreamTee(stream, cloneForBranch2) {
   branch2 =
     createTeeReadableStream(nonOpStart, pullAlgorithm, cancel2Algorithm);
 
-  PromisePrototypeCatch(
+  PromisePrototypeThen(
     reader[kState].close.promise,
+    undefined,
     (error) => {
       readableStreamDefaultControllerError(branch1[kState].controller, error);
       readableStreamDefaultControllerError(branch2[kState].controller, error);

diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js
@@ -203,5 +203,25 @@ new RuleTester({
         code: 'new Proxy({}, { ...{ __proto__: null } })',
         errors: [{ message: /null-prototype/ }]
       },
+      {
+        code: 'PromisePrototypeCatch(promise, ()=>{})',
+        errors: [{ message: /\bPromisePrototypeThen\b/ }]
+      },
+      {
+        code: 'PromiseAll([])',
+        errors: [{ message: /\bSafePromiseAll\b/ }]
+      },
+      {
+        code: 'PromiseAllSettled([])',
+        errors: [{ message: /\bSafePromiseAllSettled\b/ }]
+      },
+      {
+        code: 'PromiseAny([])',
+        errors: [{ message: /\bSafePromiseAny\b/ }]
+      },
+      {
+        code: 'PromiseRace([])',
+        errors: [{ message: /\bSafePromiseRace\b/ }]
+      },
     ]
   });
diff --git a/test/parallel/test-primordials-promise.js b/test/parallel/test-primordials-promise.js
@@ -5,7 +5,6 @@ const common = require('../common');
 const assert = require('assert');
 
 const {
-  PromisePrototypeCatch,
   PromisePrototypeThen,
   SafePromiseAll,
   SafePromiseAllSettled,
@@ -14,16 +13,15 @@ const {
   SafePromiseRace,
 } = require('internal/test/binding').primordials;
 
-Array.prototype[Symbol.iterator] = common.mustNotCall();
-Promise.all = common.mustNotCall();
-Promise.allSettled = common.mustNotCall();
-Promise.any = common.mustNotCall();
-Promise.race = common.mustNotCall();
-Promise.prototype.catch = common.mustNotCall();
-Promise.prototype.finally = common.mustNotCall();
-Promise.prototype.then = common.mustNotCall();
+Array.prototype[Symbol.iterator] = common.mustNotCall('%Array.prototype%[@@iterator]');
+Promise.all = common.mustNotCall('%Promise%.all');
+Promise.allSettled = common.mustNotCall('%Promise%.allSettled');
+Promise.any = common.mustNotCall('%Promise%.any');
+Promise.race = common.mustNotCall('%Promise%.race');
+Promise.prototype.catch = common.mustNotCall('%Promise.prototype%.catch');
+Promise.prototype.finally = common.mustNotCall('%Promise.prototype%.finally');
+Promise.prototype.then = common.mustNotCall('%Promise.prototype%.then');
 
-assertIsPromise(PromisePrototypeCatch(Promise.reject(), common.mustCall()));
 assertIsPromise(PromisePrototypeThen(test(), common.mustCall()));
 assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall()));
 

diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js
@@ -109,11 +109,11 @@ module.exports = {
               testRange.start = testRange.start + 'RegexpPrototype'.length;
               testRange.end = testRange.start + 'Test'.length;
               return [
-                fixer.replaceTextRange(node.range, 'Exec'),
+                fixer.replaceTextRange(testRange, 'Exec'),
                 fixer.insertTextAfter(node, ' !== null'),
               ];
             }
-          }]
+          }],
         });
       },
       [`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) {
@@ -142,9 +142,33 @@ module.exports = {
         }
         context.report({
           node,
-          message: 'Proxy handler must be a null-prototype object'
+          message: 'Proxy handler must be a null-prototype object',
         });
-      }
+      },
+
+      [`${CallExpression}[expression.callee.name=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) {
+        context.report({
+          node,
+          message: '%Promise.prototype.finally% look up the `then` property of ' +
+                   'the `this` argument, use SafePromisePrototypeFinally or ' +
+                   'try/finally instead',
+        });
+      },
+
+      [`${CallExpression}[expression.callee.name=${/^Promise(All(Settled)?|Any|Race)/}]`](node) {
+        context.report({
+          node,
+          message: `Use Safe${node.expression.callee.name} instead of ${node.expression.callee.name}`,
+        });
+      },
     };
   },
 };