esm: protect ESM loader from prototype pollution

In a previous commit, the loader implementation was modified to be
protected against most prototype pollution, but was kept vulnerable to
`Array.prototype` pollution. This commit fixes that, the tradeoff is
that it modifies the `ESMLoader.prototype.import` return type from an
`Array` to an array-like object.

Refs: https://github.com/nodejs/node/pull/45044
PR-URL: https://github.com/nodejs/node/pull/45175
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

doc/contributing/primordials.md

@@ -363,33 +363,52 @@ Object.defineProperty(Object.prototype, Symbol.isConcatSpreadable, {
 // 1. Lookup @@iterator property on `array` (user-mutable if user-provided).
 // 2. Lookup @@iterator property on %Array.prototype% (user-mutable).
 // 3. Lookup `next` property on %ArrayIteratorPrototype% (user-mutable).
+// 4. Lookup `then` property on %Array.Prototype% (user-mutable).
+// 5. Lookup `then` property on %Object.Prototype% (user-mutable).
 PromiseAll([]); // unsafe
 
-PromiseAll(new SafeArrayIterator([])); // safe
+// 1. Lookup `then` property on %Array.Prototype% (user-mutable).
+// 2. Lookup `then` property on %Object.Prototype% (user-mutable).
+PromiseAll(new SafeArrayIterator([])); // still unsafe
+SafePromiseAll([]); // still unsafe
+
+SafePromiseAllReturnVoid([]); // safe
+SafePromiseAllReturnArrayLike([]); // safe
 
 const array = [promise];
 const set = new SafeSet().add(promise);
 // When running one of these functions on a non-empty iterable, it will also:
-// 4. Lookup `then` property on `promise` (user-mutable if user-provided).
-// 5. Lookup `then` property on `%Promise.prototype%` (user-mutable).
+// 1. Lookup `then` property on `promise` (user-mutable if user-provided).
+// 2. Lookup `then` property on `%Promise.prototype%` (user-mutable).
+// 3. Lookup `then` property on %Array.Prototype% (user-mutable).
+// 4. Lookup `then` property on %Object.Prototype% (user-mutable).
 PromiseAll(new SafeArrayIterator(array)); // unsafe
-
 PromiseAll(set); // unsafe
 
-SafePromiseAll(array); // safe
+SafePromiseAllReturnVoid(array); // safe
+SafePromiseAllReturnArrayLike(array); // safe
 
 // Some key differences between `SafePromise[...]` and `Promise[...]` methods:
 
-// 1. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, and SafePromiseRace
-//    support passing a mapperFunction as second argument.
+// 1. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, SafePromiseRace,
+//    SafePromiseAllReturnArrayLike, SafePromiseAllReturnVoid, and
+//    SafePromiseAllSettledReturnVoid support passing a mapperFunction as second
+//    argument.
 SafePromiseAll(ArrayPrototypeMap(array, someFunction));
 SafePromiseAll(array, someFunction); // Same as the above, but more efficient.
 
-// 2. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, and SafePromiseRace
-//    only support arrays, not iterables. Use ArrayFrom to convert an iterable
-//    to an array.
-SafePromiseAll(set); // ignores set content.
-SafePromiseAll(ArrayFrom(set)); // safe
+// 2. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, SafePromiseRace,
+//    SafePromiseAllReturnArrayLike, SafePromiseAllReturnVoid, and
+//    SafePromiseAllSettledReturnVoid only support arrays and array-like
+//    objects, not iterables. Use ArrayFrom to convert an iterable to an array.
+SafePromiseAllReturnVoid(set); // ignores set content.
+SafePromiseAllReturnVoid(ArrayFrom(set)); // works
+
+// 3. SafePromiseAllReturnArrayLike is safer than SafePromiseAll, however you
+//    should not use them when its return value is passed to the user as it can
+//    be surprising for them not to receive a genuine array.
+SafePromiseAllReturnArrayLike(array).then((val) => val instanceof Array); // false
+SafePromiseAll(array).then((val) => val instanceof Array); // true
 ```
 
 </details>

lib/internal/modules/esm/loader.js

@@ -14,7 +14,7 @@ const {
   ObjectDefineProperty,
   ObjectSetPrototypeOf,
   RegExpPrototypeExec,
-  SafePromiseAll,
+  SafePromiseAllReturnArrayLike,
   SafeWeakMap,
   StringPrototypeSlice,
   StringPrototypeToUpperCase,
@@ -516,7 +516,7 @@ class ESMLoader {
         .then(({ module }) => module.getNamespace());
     }
 
-    const namespaces = await SafePromiseAll(jobs);
+    const namespaces = await SafePromiseAllReturnArrayLike(jobs);
 
     if (!wasArr) { return namespaces[0]; } // We can skip the pairing below
 

lib/internal/modules/esm/module_job.js

@@ -12,7 +12,8 @@ const {
   ReflectApply,
   RegExpPrototypeExec,
   RegExpPrototypeSymbolReplace,
-  SafePromiseAll,
+  SafePromiseAllReturnArrayLike,
+  SafePromiseAllReturnVoid,
   SafeSet,
   StringPrototypeIncludes,
   StringPrototypeSplit,
@@ -80,9 +81,9 @@ class ModuleJob {
       });
 
       if (promises !== undefined)
-        await SafePromiseAll(promises);
+        await SafePromiseAllReturnVoid(promises);
 
-      return SafePromiseAll(dependencyJobs);
+      return SafePromiseAllReturnArrayLike(dependencyJobs);
     };
     // Promise for the list of all dependencyJobs.
     this.linked = link();
@@ -110,7 +111,7 @@ class ModuleJob {
       }
       jobsInGraph.add(moduleJob);
       const dependencyJobs = await moduleJob.linked;
-      return SafePromiseAll(dependencyJobs, addJobsToDependencyGraph);
+      return SafePromiseAllReturnVoid(dependencyJobs, addJobsToDependencyGraph);
     };
     await addJobsToDependencyGraph(this);
 

lib/internal/per_context/primordials.js

@@ -261,6 +261,7 @@ function copyPrototype(src, dest, prefix) {
 /* eslint-enable node-core/prefer-primordials */
 
 const {
+  Array: ArrayConstructor,
   ArrayPrototypeForEach,
   ArrayPrototypeMap,
   FinalizationRegistry,
@@ -272,6 +273,7 @@ const {
   ObjectSetPrototypeOf,
   Promise,
   PromisePrototypeThen,
+  PromiseResolve,
   ReflectApply,
   ReflectConstruct,
   ReflectSet,
@@ -466,9 +468,10 @@ const arrayToSafePromiseIterable = (promises, mapFn) =>
   );
 
 /**
- * @param {Promise<any>[]} promises
- * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
- * @returns {Promise<any[]>}
+ * @template T,U
+ * @param {Array<T | PromiseLike<T>>} promises
+ * @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
+ * @returns {Promise<Awaited<U>[]>}
  */
 primordials.SafePromiseAll = (promises, mapFn) =>
   // Wrapping on a new Promise is necessary to not expose the SafePromise
@@ -478,8 +481,56 @@ primordials.SafePromiseAll = (promises, mapFn) =>
   );
 
 /**
- * @param {Promise<any>[]} promises
- * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
+ * Should only be used for internal functions, this would produce similar
+ * results as `Promise.all` but without prototype pollution, and the return
+ * value is not a genuine Array but an array-like object.
+ * @template T,U
+ * @param {ArrayLike<T | PromiseLike<T>>} promises
+ * @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
+ * @returns {Promise<ArrayLike<Awaited<U>>>}
+ */
+primordials.SafePromiseAllReturnArrayLike = (promises, mapFn) =>
+  new Promise((resolve, reject) => {
+    const { length } = promises;
+
+    const returnVal = ArrayConstructor(length);
+    ObjectSetPrototypeOf(returnVal, null);
+    if (length === 0) resolve(returnVal);
+
+    let pendingPromises = length;
+    for (let i = 0; i < length; i++) {
+      const promise = mapFn != null ? mapFn(promises[i], i) : promises[i];
+      PromisePrototypeThen(PromiseResolve(promise), (result) => {
+        returnVal[i] = result;
+        if (--pendingPromises === 0) resolve(returnVal);
+      }, reject);
+    }
+  });
+
+/**
+ * Should only be used when we only care about waiting for all the promises to
+ * resolve, not what value they resolve to.
+ * @template T,U
+ * @param {ArrayLike<T | PromiseLike<T>>} promises
+ * @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
+ * @returns {Promise<void>}
+ */
+primordials.SafePromiseAllReturnVoid = (promises, mapFn) =>
+  new Promise((resolve, reject) => {
+    let pendingPromises = promises.length;
+    if (pendingPromises === 0) resolve();
+    for (let i = 0; i < promises.length; i++) {
+      const promise = mapFn != null ? mapFn(promises[i], i) : promises[i];
+      PromisePrototypeThen(PromiseResolve(promise), () => {
+        if (--pendingPromises === 0) resolve();
+      }, reject);
+    }
+  });
+
+/**
+ * @template T,U
+ * @param {Array<T|PromiseLike<T>>} promises
+ * @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
  * @returns {Promise<PromiseSettledResult<any>[]>}
  */
 primordials.SafePromiseAllSettled = (promises, mapFn) =>
@@ -490,9 +541,28 @@ primordials.SafePromiseAllSettled = (promises, mapFn) =>
   );
 
 /**
- * @param {Promise<any>[]} promises
- * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
- * @returns {Promise<any>}
+ * Should only be used when we only care about waiting for all the promises to
+ * settle, not what value they resolve or reject to.
+ * @template T,U
+ * @param {ArrayLike<T|PromiseLike<T>>} promises
+ * @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
+ * @returns {Promise<void>}
+ */
+primordials.SafePromiseAllSettledReturnVoid = async (promises, mapFn) => {
+  for (let i = 0; i < promises.length; i++) {
+    try {
+      await (mapFn != null ? mapFn(promises[i], i) : promises[i]);
+    } catch {
+      // In all settled, we can ignore errors.
+    }
+  }
+};
+
+/**
+ * @template T,U
+ * @param {Array<T|PromiseLike<T>>} promises
+ * @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
+ * @returns {Promise<Awaited<U>>}
  */
 primordials.SafePromiseAny = (promises, mapFn) =>
   // Wrapping on a new Promise is necessary to not expose the SafePromise
@@ -502,9 +572,10 @@ primordials.SafePromiseAny = (promises, mapFn) =>
   );
 
 /**
- * @param {Promise<any>[]} promises
- * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
- * @returns {Promise<any>}
+ * @template T,U
+ * @param {Array<T|PromiseLike<T>>} promises
+ * @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
+ * @returns {Promise<Awaited<U>>}
  */
 primordials.SafePromiseRace = (promises, mapFn) =>
   // Wrapping on a new Promise is necessary to not expose the SafePromise

lib/internal/vm/module.js

@@ -11,7 +11,7 @@ const {
   ObjectGetPrototypeOf,
   ObjectSetPrototypeOf,
   ReflectApply,
-  SafePromiseAll,
+  SafePromiseAllReturnVoid,
   SafeWeakMap,
   Symbol,
   SymbolToStringTag,
@@ -330,7 +330,7 @@ class SourceTextModule extends Module {
 
       try {
         if (promises !== undefined) {
-          await SafePromiseAll(promises);
+          await SafePromiseAllReturnVoid(promises);
         }
       } catch (e) {
         this.#error = e;

test/es-module/test-cjs-prototype-pollution.js

@@ -2,11 +2,6 @@
 
 const { mustNotCall, mustCall } = require('../common');
 
-Object.defineProperties(Array.prototype, {
-  // %Promise.all% and %Promise.allSettled% are depending on the value of
-  // `%Array.prototype%.then`.
-  then: {},
-});
 Object.defineProperties(Object.prototype, {
   then: {
     set: mustNotCall('set %Object.prototype%.then'),

test/es-module/test-esm-prototype-pollution.mjs

@@ -1,10 +1,5 @@
 import { mustNotCall, mustCall } from '../common/index.mjs';
 
-Object.defineProperties(Array.prototype, {
-  // %Promise.all% and %Promise.allSettled% are depending on the value of
-  // `%Array.prototype%.then`.
-  then: {},
-});
 Object.defineProperties(Object.prototype, {
   then: {
     set: mustNotCall('set %Object.prototype%.then'),

test/parallel/test-eslint-avoid-prototype-pollution.js

@@ -63,6 +63,8 @@ new RuleTester({
       'new Proxy({}, someFactory())',
       'new Proxy({}, { __proto__: null })',
       'new Proxy({}, { __proto__: null, ...{} })',
+      'async function name(){return await SafePromiseAll([])}',
+      'async function name(){const val = await SafePromiseAll([])}',
     ],
     invalid: [
       {
@@ -273,6 +275,14 @@ new RuleTester({
         code: 'PromiseAll([])',
         errors: [{ message: /\bSafePromiseAll\b/ }]
       },
+      {
+        code: 'async function fn(){await SafePromiseAll([])}',
+        errors: [{ message: /\bSafePromiseAllReturnVoid\b/ }]
+      },
+      {
+        code: 'async function fn(){await SafePromiseAllSettled([])}',
+        errors: [{ message: /\bSafePromiseAllSettledReturnVoid\b/ }]
+      },
       {
         code: 'PromiseAllSettled([])',
         errors: [{ message: /\bSafePromiseAllSettled\b/ }]

test/parallel/test-primordials-promise.js

@@ -7,7 +7,10 @@ const assert = require('assert');
 const {
   PromisePrototypeThen,
   SafePromiseAll,
+  SafePromiseAllReturnArrayLike,
+  SafePromiseAllReturnVoid,
   SafePromiseAllSettled,
+  SafePromiseAllSettledReturnVoid,
   SafePromiseAny,
   SafePromisePrototypeFinally,
   SafePromiseRace,
@@ -34,9 +37,11 @@ Object.defineProperties(Promise.prototype, {
   },
 });
 Object.defineProperties(Array.prototype, {
-  // %Promise.all% and %Promise.allSettled% are depending on the value of
-  // `%Array.prototype%.then`.
-  then: {},
+  then: {
+    configurable: true,
+    set: common.mustNotCall('set %Array.prototype%.then'),
+    get: common.mustNotCall('get %Array.prototype%.then'),
+  },
 });
 Object.defineProperties(Object.prototype, {
   then: {
@@ -48,11 +53,65 @@ Object.defineProperties(Object.prototype, {
 assertIsPromise(PromisePrototypeThen(test(), common.mustCall()));
 assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall()));
 
-assertIsPromise(SafePromiseAll([test()]));
-assertIsPromise(SafePromiseAllSettled([test()]));
+assertIsPromise(SafePromiseAllReturnArrayLike([test()]));
+assertIsPromise(SafePromiseAllReturnVoid([test()]));
+assertIsPromise(SafePromiseAllSettledReturnVoid([test()]));
 assertIsPromise(SafePromiseAny([test()]));
 assertIsPromise(SafePromiseRace([test()]));
 
+assertIsPromise(SafePromiseAllReturnArrayLike([]));
+assertIsPromise(SafePromiseAllReturnVoid([]));
+assertIsPromise(SafePromiseAllSettledReturnVoid([]));
+
+{
+  const val1 = Symbol();
+  const val2 = Symbol();
+  PromisePrototypeThen(
+    SafePromiseAllReturnArrayLike([Promise.resolve(val1), { then(resolve) { resolve(val2); } }]),
+    common.mustCall((val) => {
+      assert.strictEqual(Array.isArray(val), true);
+      const expected = [val1, val2];
+      assert.deepStrictEqual(val.length, expected.length);
+      assert.strictEqual(val[0], expected[0]);
+      assert.strictEqual(val[1], expected[1]);
+    })
+  );
+}
+
+{
+  // Never settling promises should not block the resulting promise to be rejected:
+  const error = new Error();
+  PromisePrototypeThen(
+    SafePromiseAllReturnArrayLike([new Promise(() => {}), Promise.reject(error)]),
+    common.mustNotCall('Should have rejected'),
+    common.mustCall((err) => {
+      assert.strictEqual(err, error);
+    })
+  );
+  PromisePrototypeThen(
+    SafePromiseAllReturnVoid([new Promise(() => {}), Promise.reject(error)]),
+    common.mustNotCall('Should have rejected'),
+    common.mustCall((err) => {
+      assert.strictEqual(err, error);
+    })
+  );
+}
+
+Object.defineProperties(Array.prototype, {
+  // %Promise.all% and %Promise.allSettled% are depending on the value of
+  // `%Array.prototype%.then`.
+  then: {
+    __proto__: undefined,
+    value: undefined,
+  },
+});
+
+assertIsPromise(SafePromiseAll([test()]));
+assertIsPromise(SafePromiseAllSettled([test()]));
+
+assertIsPromise(SafePromiseAll([]));
+assertIsPromise(SafePromiseAllSettled([]));
+
 async function test() {
   const catchFn = common.mustCall();
   const finallyFn = common.mustCall();
@@ -70,4 +129,5 @@ function assertIsPromise(promise) {
   // Make sure the returned promise is a genuine %Promise% object and not a
   // subclass instance.
   assert.strictEqual(Object.getPrototypeOf(promise), Promise.prototype);
+  PromisePrototypeThen(promise, common.mustCall());
 }

tools/eslint-rules/avoid-prototype-pollution.js

@@ -194,6 +194,13 @@ module.exports = {
         });
       },
 
+      [`ExpressionStatement>AwaitExpression>${CallExpression(/^(Safe)?PromiseAll(Settled)?$/)}`](node) {
+        context.report({
+          node,
+          message: `Use ${node.callee.name}ReturnVoid`,
+        });
+      },
+
       [CallExpression('PromisePrototypeCatch')](node) {
         context.report({
           node,