fixup! esm: protect ESM loader from prototype pollution

nodejs · nodejs-github-bot · Oct 27, 2022 · Oct 25, 2022 · Oct 25, 2022 · Oct 25, 2022
commit d82537bec5128eba873dd4cb18cf7219529c1fca
diff --git a/doc/contributing/primordials.md b/doc/contributing/primordials.md
@@ -390,8 +390,10 @@ 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.
 
@@ -405,8 +407,8 @@ 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) => Array.isArray(val)); // false
-SafePromiseAll(array).then((val) => Array.isArray(val)); // true
+SafePromiseAllReturnArrayLike(array).then((val) => val instanceof Array); // false
+SafePromiseAll(array).then((val) => val instanceof Array); // true
 ```
 
 </details>

diff --git a/lib/internal/per_context/primordials.js b/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,
@@ -482,28 +484,45 @@ primordials.SafePromiseAll = (promises, mapFn) =>
  * results as `Promise.all` but without prototype pollution, and the return
  * value is not a genuine Array but an array-like object.
  * @param {Promise<any>[]} promises
+ * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
  * @returns {Promise<any[]>}
  */
-primordials.SafePromiseAllReturnArrayLike = async (promises) => {
-  const { length } = promises;
-  const returnVal = { __proto__: null, length };
-  for (let i = 0; i < length; i++) {
-    returnVal[i] = await promises[i];
-  }
-  return returnVal;
-};
+primordials.SafePromiseAllReturnArrayLike = (promises, mapFn) =>
+  new Promise((resolve, reject) => {
+    const { length } = promises;
+
+    const returnVal = ArrayConstructor(length);
+    ObjectSetPrototypeOf(returnVal, null);
+
+    let pendingPromises = length;
+    if (pendingPromises === 0) resolve(returnVal);
+    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.
  * @param {Promise<any>[]} promises
+ * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
  * @returns {Promise<void>}
  */
-primordials.SafePromiseAllReturnVoid = async (promises) => {
-  for (let i = 0; i < promises.length; i++) {
-    await promises[i];
-  }
-};
+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);
+    }
+  });
 
 /**
  * @param {Promise<any>[]} promises
@@ -521,12 +540,13 @@ primordials.SafePromiseAllSettled = (promises, mapFn) =>
  * Should only be used when we only care about waiting for all the promises to
  * settle, not what value they resolve or reject to.
  * @param {Promise<any>[]} promises
+ * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
  * @returns {Promise<void>}
  */
-primordials.SafePromiseAllSettledReturnVoid = async (promises) => {
+primordials.SafePromiseAllSettledReturnVoid = async (promises, mapFn) => {
   for (let i = 0; i < promises.length; i++) {
     try {
-      await promises[i];
+      await (mapFn != null ? mapFn(promises[i], i) : promises[i]);
     } catch {
       // In all settled, we can ignore errors.
     }

diff --git a/test/es-module/test-esm-prototype-pollution.mjs b/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'),

diff --git a/test/parallel/test-primordials-promise.js b/test/parallel/test-primordials-promise.js
@@ -59,6 +59,44 @@ 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`.
@@ -71,6 +109,9 @@ Object.defineProperties(Array.prototype, {
 assertIsPromise(SafePromiseAll([test()]));
 assertIsPromise(SafePromiseAllSettled([test()]));
 
+assertIsPromise(SafePromiseAll([]));
+assertIsPromise(SafePromiseAllSettled([]));
+
 async function test() {
   const catchFn = common.mustCall();
   const finallyFn = common.mustCall();
@@ -88,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());
 }