lib: add lint rule to protect against Object.prototype.then pollution

PR-URL: #45061 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
nodejs · Nov 10, 2022 · c7b7f2b · c7b7f2b
1 parent 93a34fa
commit c7b7f2b
Show file tree

Hide file tree

Showing 10 changed files with 98 additions and 33 deletions.
diff --git a/lib/internal/crypto/cfrg.js b/lib/internal/crypto/cfrg.js
@@ -188,7 +188,7 @@ async function cfrgGenerateKey(algorithm, extractable, keyUsages) {
       privateUsages,
       extractable);
 
-  return { privateKey, publicKey };
+  return { __proto__: null, privateKey, publicKey };
 }
 
 function cfrgExportKey(key, format) {

diff --git a/lib/internal/crypto/ec.js b/lib/internal/crypto/ec.js
@@ -146,7 +146,7 @@ async function ecGenerateKey(algorithm, extractable, keyUsages) {
       privateUsages,
       extractable);
 
-  return { publicKey, privateKey };
+  return { __proto__: null, publicKey, privateKey };
 }
 
 function ecExportKey(key, format) {

diff --git a/lib/internal/crypto/rsa.js b/lib/internal/crypto/rsa.js
@@ -219,7 +219,7 @@ async function rsaKeyGenerate(
       privateUsages,
       extractable);
 
-  return { publicKey, privateKey };
+  return { __proto__: null, publicKey, privateKey };
 }
 
 function rsaExportKey(key, format) {

diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js
@@ -115,7 +115,7 @@ async function checkPaths(src, dest, opts) {
       code: 'EINVAL',
     });
   }
-  return { srcStat, destStat, skipped: false };
+  return { __proto__: null, srcStat, destStat, skipped: false };
 }
 
 function areIdentical(srcStat, destStat) {

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -580,7 +580,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
   length |= 0;
 
   if (length === 0)
-    return { bytesRead: length, buffer };
+    return { __proto__: null, bytesRead: length, buffer };
 
   if (buffer.byteLength === 0) {
     throw new ERR_INVALID_ARG_VALUE('buffer', buffer,
@@ -595,7 +595,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
   const bytesRead = (await binding.read(handle.fd, buffer, offset, length,
                                         position, kUsePromises)) || 0;
 
-  return { bytesRead, buffer };
+  return { __proto__: null, bytesRead, buffer };
 }
 
 async function readv(handle, buffers, position) {
@@ -606,12 +606,12 @@ async function readv(handle, buffers, position) {
 
   const bytesRead = (await binding.readBuffers(handle.fd, buffers, position,
                                                kUsePromises)) || 0;
-  return { bytesRead, buffers };
+  return { __proto__: null, bytesRead, buffers };
 }
 
 async function write(handle, buffer, offsetOrOptions, length, position) {
   if (buffer?.byteLength === 0)
-    return { bytesWritten: 0, buffer };
+    return { __proto__: null, bytesWritten: 0, buffer };
 
   let offset = offsetOrOptions;
   if (isArrayBufferView(buffer)) {
@@ -636,14 +636,14 @@ async function write(handle, buffer, offsetOrOptions, length, position) {
     const bytesWritten =
       (await binding.writeBuffer(handle.fd, buffer, offset,
                                  length, position, kUsePromises)) || 0;
-    return { bytesWritten, buffer };
+    return { __proto__: null, bytesWritten, buffer };
   }
 
   validateStringAfterArrayBufferView(buffer, 'buffer');
   validateEncoding(buffer, length);
   const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
                                                   length, kUsePromises)) || 0;
-  return { bytesWritten, buffer };
+  return { __proto__: null, bytesWritten, buffer };
 }
 
 async function writev(handle, buffers, position) {
@@ -653,12 +653,12 @@ async function writev(handle, buffers, position) {
     position = null;
 
   if (buffers.length === 0) {
-    return { bytesWritten: 0, buffers };
+    return { __proto__: null, bytesWritten: 0, buffers };
   }
 
   const bytesWritten = (await binding.writeBuffers(handle.fd, buffers, position,
                                                    kUsePromises)) || 0;
-  return { bytesWritten, buffers };
+  return { __proto__: null, bytesWritten, buffers };
 }
 
 async function rename(oldPath, newPath) {

diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -378,6 +378,7 @@ class ESMLoader {
     const { module } = await job.run();
 
     return {
+      __proto__: null,
       namespace: module.getNamespace(),
     };
   }

diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -977,7 +977,7 @@ async function defaultResolve(specifier, context = {}) {
         missing = false;
       } else if (destination) {
         const href = destination.href;
-        return { url: href };
+        return { __proto__: null, url: href };
       }
       if (missing) {
         // Prevent network requests from firing if resolution would be banned.
@@ -1035,7 +1035,7 @@ async function defaultResolve(specifier, context = {}) {
   if (maybeReturn) return maybeReturn;
 
   // This must come after checkIfDisallowedImport
-  if (parsed && parsed.protocol === 'node:') return { url: specifier };
+  if (parsed && parsed.protocol === 'node:') return { __proto__: null, url: specifier };
 
   throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports);
 
@@ -1087,6 +1087,7 @@ async function defaultResolve(specifier, context = {}) {
   throwIfUnsupportedURLProtocol(url);
 
   return {
+    __proto__: null,
     // Do NOT cast `url` to a string: that will work even when there are real
     // problems, silencing them
     url: url.href,

diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js
@@ -476,7 +476,7 @@ class ReadableStream {
 
     async function returnSteps(value) {
       if (done)
-        return { done: true, value };
+        return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution
       done = true;
 
       if (reader[kState].stream === undefined) {
@@ -488,11 +488,11 @@ class ReadableStream {
         const result = readableStreamReaderGenericCancel(reader, value);
         readableStreamReaderGenericRelease(reader);
         await result;
-        return { done: true, value };
+        return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution
       }
 
       readableStreamReaderGenericRelease(reader);
-      return { done: true, value };
+      return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution
     }
 
     // TODO(@jasnell): Explore whether an async generator

diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js
@@ -45,6 +45,17 @@ new RuleTester({
       'ReflectDefineProperty({}, "key", { "__proto__": null })',
       'ObjectDefineProperty({}, "key", { \'__proto__\': null })',
       'ReflectDefineProperty({}, "key", { \'__proto__\': null })',
+      'async function myFn() { return { __proto__: null } }',
+      'async function myFn() { function myFn() { return {} } return { __proto__: null } }',
+      'const myFn = async function myFn() { return { __proto__: null } }',
+      'const myFn = async function () { return { __proto__: null } }',
+      'const myFn = async () => { return { __proto__: null } }',
+      'const myFn = async () => ({ __proto__: null })',
+      'function myFn() { return {} }',
+      'const myFn = function myFn() { return {} }',
+      'const myFn = function () { return {} }',
+      'const myFn = () => { return {} }',
+      'const myFn = () => ({})',
       'StringPrototypeReplace("some string", "some string", "some replacement")',
       'StringPrototypeReplaceAll("some string", "some string", "some replacement")',
       'StringPrototypeSplit("some string", "some string")',
@@ -150,6 +161,34 @@ new RuleTester({
         code: 'ReflectDefineProperty({}, "key", { enumerable: true })',
         errors: [{ message: /null-prototype/ }],
       },
+      {
+        code: 'async function myFn(){ return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'async function myFn(){ async function someOtherFn() { return { __proto__: null } } return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'async function myFn(){ if (true) { return {} } return { __proto__: null } }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async function myFn(){ return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async function (){ return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async () => { return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async () => ({})',
+        errors: [{ message: /null-prototype/ }],
+      },
       {
         code: 'RegExpPrototypeTest(/some regex/, "some string")',
         errors: [{ message: /looks up the "exec" property/ }],

diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const CallExpression = (fnName) => `CallExpression[callee.name=${fnName}]`;
+const AnyFunction = 'FunctionDeclaration, FunctionExpression, ArrowFunctionExpression';
 
 function checkProperties(context, node) {
   if (
@@ -25,6 +26,22 @@ function checkProperties(context, node) {
   }
 }
 
+function isNullPrototypeObjectExpression(node) {
+  if (node.type !== 'ObjectExpression') return;
+
+  for (const { key, value } of node.properties) {
+    if (
+      key != null && value != null &&
+      ((key.type === 'Identifier' && key.name === '__proto__') ||
+        (key.type === 'Literal' && key.value === '__proto__')) &&
+      value.type === 'Literal' && value.value === null
+    ) {
+      return true;
+    }
+  }
+  return false;
+}
+
 function checkPropertyDescriptor(context, node) {
   if (
     node.type === 'CallExpression' &&
@@ -46,23 +63,12 @@ function checkPropertyDescriptor(context, node) {
       }],
     });
   }
-  if (node.type !== 'ObjectExpression') return;
-
-  for (const { key, value } of node.properties) {
-    if (
-      key != null && value != null &&
-      ((key.type === 'Identifier' && key.name === '__proto__') ||
-        (key.type === 'Literal' && key.value === '__proto__')) &&
-      value.type === 'Literal' && value.value === null
-    ) {
-      return true;
-    }
+  if (isNullPrototypeObjectExpression(node) === false) {
+    context.report({
+      node,
+      message: 'Must use null-prototype object for property descriptors',
+    });
   }
-
-  context.report({
-    node,
-    message: 'Must use null-prototype object for property descriptors',
-  });
 }
 
 function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
@@ -117,6 +123,24 @@ module.exports = {
       [`${CallExpression('ObjectCreate')}[arguments.length=2]`](node) {
         checkProperties(context, node.arguments[1]);
       },
+
+      [`:matches(${AnyFunction})[async=true]>BlockStatement ReturnStatement>ObjectExpression, :matches(${AnyFunction})[async=true]>ObjectExpression`](node) {
+        if (node.parent.type === 'ReturnStatement') {
+          let { parent } = node;
+          do {
+            ({ parent } = parent);
+          } while (!parent.type.includes('Function'));
+
+          if (!parent.async) return;
+        }
+        if (isNullPrototypeObjectExpression(node) === false) {
+          context.report({
+            node,
+            message: 'Use null-prototype when returning from async function',
+          });
+        }
+      },
+
       [CallExpression('RegExpPrototypeTest')](node) {
         context.report({
           node,