tools,doc: add guards against prototype pollution when creating proxies

PR-URL: nodejs/node#43391 Backport-PR-URL: nodejs/node#44081 Reviewed-By: James M Snell <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
noslate-project · Oct 10, 2022 · 10e7b9a · 10e7b9a
1 parent 549bdb2
commit 10e7b9a
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 0 deletions.
diff --git a/doc/contributing/primordials.md b/doc/contributing/primordials.md
@@ -738,3 +738,27 @@ class SomeClass {
 ObjectDefineProperty(SomeClass.prototype, 'readOnlyProperty', kEnumerableProperty);
 console.log(new SomeClass().readOnlyProperty); // genuine data
 ```
+
+### Defining a `Proxy` handler
+
+When defining a `Proxy`, the handler object could be at risk of prototype
+pollution when using a plain object literal:
+
+```js
+// User-land
+Object.prototype.get = () => 'Unrelated user-provided data';
+
+// Core
+const objectToProxy = { someProperty: 'genuine value' };
+
+const proxyWithPlainObjectLiteral = new Proxy(objectToProxy, {
+  has() { return false; },
+});
+console.log(proxyWithPlainObjectLiteral.someProperty); // Unrelated user-provided data
+
+const proxyWithNullPrototypeObject = new Proxy(objectToProxy, {
+  __proto__: null,
+  has() { return false; },
+});
+console.log(proxyWithNullPrototypeObject.someProperty); // genuine value
+```
diff --git a/lib/internal/debugger/inspect.js b/lib/internal/debugger/inspect.js
@@ -117,6 +117,7 @@ function createAgentProxy(domain, client) {
   };
 
   return new Proxy(agent, {
+    __proto__: null,
     get(target, name) {
       if (name in target) return target[name];
       return function callVirtualMethod(params) {

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -982,6 +982,7 @@ function trackAssignmentsTypedArray(typedArray) {
   }
 
   return new Proxy(typedArray, {
+    __proto__: null,
     get(obj, prop, receiver) {
       if (prop === 'copyAssigned') {
         return copyAssigned;

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -217,6 +217,8 @@ const wrapper = [
 ];
 
 let wrapperProxy = new Proxy(wrapper, {
+  __proto__: null,
+
   set(target, property, value, receiver) {
     patched = true;
     return ReflectSet(target, property, value, receiver);
@@ -725,6 +727,8 @@ function emitCircularRequireWarning(prop) {
 // A Proxy that can be used as the prototype of a module.exports object and
 // warns when non-existent properties are accessed.
 const CircularRequirePrototypeWarningProxy = new Proxy({}, {
+  __proto__: null,
+
   get(target, prop) {
     // Allow __esModule access in any case because it is used in the output
     // of transpiled code to determine whether something comes from an

diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js
@@ -45,6 +45,10 @@ new RuleTester({
       'ReflectDefineProperty({}, "key", { "__proto__": null })',
       'ObjectDefineProperty({}, "key", { \'__proto__\': null })',
       'ReflectDefineProperty({}, "key", { \'__proto__\': null })',
+      'new Proxy({}, otherObject)',
+      'new Proxy({}, someFactory())',
+      'new Proxy({}, { __proto__: null })',
+      'new Proxy({}, { __proto__: null, ...{} })',
     ],
     invalid: [
       {
@@ -183,5 +187,21 @@ new RuleTester({
         code: 'StringPrototypeSplit("some string", /some regex/)',
         errors: [{ message: /looks up the Symbol\.split property/ }],
       },
+      {
+        code: 'new Proxy({}, {})',
+        errors: [{ message: /null-prototype/ }]
+      },
+      {
+        code: 'new Proxy({}, { [`__proto__`]: null })',
+        errors: [{ message: /null-prototype/ }]
+      },
+      {
+        code: 'new Proxy({}, { __proto__: Object.prototype })',
+        errors: [{ message: /null-prototype/ }]
+      },
+      {
+        code: 'new Proxy({}, { ...{ __proto__: null } })',
+        errors: [{ message: /null-prototype/ }]
+      },
     ]
   });
diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js
@@ -128,6 +128,23 @@ module.exports = {
       ...createUnsafeStringMethodReport(context, 'StringPrototypeReplaceAll', 'Symbol.replace'),
       ...createUnsafeStringMethodReport(context, 'StringPrototypeSearch', 'Symbol.search'),
       ...createUnsafeStringMethodReport(context, 'StringPrototypeSplit', 'Symbol.split'),
+
+      'NewExpression[callee.name="Proxy"][arguments.1.type="ObjectExpression"]'(node) {
+        for (const { key, value } of node.arguments[1].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;
+          }
+        }
+        context.report({
+          node,
+          message: 'Proxy handler must be a null-prototype object'
+        });
+      }
     };
   },
 };