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: #45044
PR-URL: #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/ }]