module: protect against prototype mutation

Ensures that mutating the `Object` prototype does not influence the parsing of `package.json` files. Backport-PR-URL: nodejs-private/node-private#373 PR-URL: #44007 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
nodejs · Feb 15, 2023 · fa115ee · fa115ee
1 parent 97a0443
commit fa115ee
Show file tree

Hide file tree

Showing 15 changed files with 77 additions and 23 deletions.
diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js
@@ -23,6 +23,7 @@ const path = require('path');
 const { pathToFileURL, fileURLToPath, URL } = require('internal/url');
 
 const { getOptionValue } = require('internal/options');
+const { setOwnProperty } = require('internal/util');
 const userConditions = getOptionValue('--conditions');
 
 let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
@@ -116,7 +117,7 @@ function makeRequireFunction(mod, redirects) {
 
   resolve.paths = paths;
 
-  require.main = process.mainModule;
+  setOwnProperty(require, 'main', process.mainModule);
 
   // Enable support to add extra extension types.
   require.extensions = Module._extensions;

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -76,7 +76,7 @@ const {
   maybeCacheSourceMap,
 } = require('internal/source_map/source_map_cache');
 const { pathToFileURL, fileURLToPath, isURLInstance } = require('internal/url');
-const { deprecate } = require('internal/util');
+const { deprecate, filterOwnProperties, setOwnProperty } = require('internal/util');
 const vm = require('vm');
 const assert = require('internal/assert');
 const fs = require('fs');
@@ -163,7 +163,7 @@ function updateChildren(parent, child, scan) {
 function Module(id = '', parent) {
   this.id = id;
   this.path = path.dirname(id);
-  this.exports = {};
+  setOwnProperty(this, 'exports', {});
   this.parent = parent;
   updateChildren(parent, this, false);
   this.filename = null;
@@ -269,14 +269,13 @@ function readPackage(requestPath) {
   }
 
   try {
-    const parsed = JSONParse(json);
-    const filtered = {
-      name: parsed.name,
-      main: parsed.main,
-      exports: parsed.exports,
-      imports: parsed.imports,
-      type: parsed.type
-    };
+    const filtered = filterOwnProperties(JSONParse(json), [
+      'name',
+      'main',
+      'exports',
+      'imports',
+      'type',
+    ]);
     packageJsonCache.set(jsonPath, filtered);
     return filtered;
   } catch (e) {
@@ -1125,7 +1124,7 @@ Module._extensions['.json'] = function(module, filename) {
   }
 
   try {
-    module.exports = JSONParse(stripBOM(content));
+    setOwnProperty(module, 'exports', JSONParse(stripBOM(content)));
   } catch (err) {
     err.message = filename + ': ' + err.message;
     throw err;

diff --git a/lib/internal/util.js b/lib/internal/util.js
@@ -11,6 +11,7 @@ const {
   ObjectGetOwnPropertyDescriptor,
   ObjectGetOwnPropertyDescriptors,
   ObjectGetPrototypeOf,
+  ObjectPrototypeHasOwnProperty,
   ObjectSetPrototypeOf,
   Promise,
   ReflectConstruct,
@@ -458,6 +459,35 @@ function createDeferredPromise() {
   return { promise, resolve, reject };
 }
 
+function filterOwnProperties(source, keys) {
+  const filtered = ObjectCreate(null);
+  for (let i = 0; i < keys.length; i++) {
+    const key = keys[i];
+    if (ObjectPrototypeHasOwnProperty(source, key)) {
+      filtered[key] = source[key];
+    }
+  }
+
+  return filtered;
+}
+
+/**
+ * Mimics `obj[key] = value` but ignoring potential prototype inheritance.
+ * @param {any} obj
+ * @param {string} key
+ * @param {any} value
+ * @returns {any}
+ */
+function setOwnProperty(obj, key, value) {
+  return ObjectDefineProperty(obj, key, {
+    __proto__: null,
+    configurable: true,
+    enumerable: true,
+    value,
+    writable: true,
+  });
+}
+
 module.exports = {
   assertCrypto,
   cachedResult,
@@ -468,6 +498,7 @@ module.exports = {
   deprecate,
   emitExperimentalWarning,
   filterDuplicateStrings,
+  filterOwnProperties,
   getConstructorOf,
   getSystemErrorMap,
   getSystemErrorName,
@@ -492,5 +523,6 @@ module.exports = {
   // Used by the buffer module to capture an internal reference to the
   // default isEncoding implementation, just in case userland overrides it.
   kIsEncodingSymbol: Symbol('kIsEncodingSymbol'),
-  kVmBreakFirstLineSymbol: Symbol('kVmBreakFirstLineSymbol')
+  kVmBreakFirstLineSymbol: Symbol('kVmBreakFirstLineSymbol'),
+  setOwnProperty,
 };
diff --git a/test/common/fixtures.js b/test/common/fixtures.js
@@ -2,13 +2,18 @@
 
 const path = require('path');
 const fs = require('fs');
+const { pathToFileURL } = require('url');
 
 const fixturesDir = path.join(__dirname, '..', 'fixtures');
 
 function fixturesPath(...args) {
   return path.join(fixturesDir, ...args);
 }
 
+function fixturesFileURL(...args) {
+  return pathToFileURL(fixturesPath(...args));
+}
+
 function readFixtureSync(args, enc) {
   if (Array.isArray(args))
     return fs.readFileSync(fixturesPath(...args), enc);
@@ -22,6 +27,7 @@ function readFixtureKey(name, enc) {
 module.exports = {
   fixturesDir,
   path: fixturesPath,
+  fileURL: fixturesFileURL,
   readSync: readFixtureSync,
   readKey: readFixtureKey
 };
diff --git a/test/fixtures/es-module-specifiers/index.mjs b/test/fixtures/es-module-specifiers/index.mjs
@@ -1,10 +1,11 @@
 import explicit from 'explicit-main';
 import implicit from 'implicit-main';
 import implicitModule from 'implicit-main-type-module';
+import noMain from 'no-main-field';
 
 function getImplicitCommonjs () {
   return import('implicit-main-type-commonjs');
 }
 
-export {explicit, implicit, implicitModule, getImplicitCommonjs};
+export {explicit, implicit, implicitModule, getImplicitCommonjs, noMain};
 export default 'success';
diff --git a/test/fixtures/es-module-specifiers/node_modules/no-main-field/index.js b/test/fixtures/es-module-specifiers/node_modules/no-main-field/index.js
diff --git a/test/fixtures/es-module-specifiers/node_modules/no-main-field/package.json b/test/fixtures/es-module-specifiers/node_modules/no-main-field/package.json
diff --git a/test/message/source_map_disabled_by_api.out b/test/message/source_map_disabled_by_api.out
@@ -8,7 +8,7 @@ Error: an error!
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*)
     at Module.load (internal/modules/cjs/loader.js:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*)
-    at Module.require (internal/modules/cjs/loader.js:*)
+    at Module.internalRequire (internal/modules/cjs/loader.js:*)
 *enclosing-call-site.js:16
       throw new Error('an error!')
                 ^
@@ -23,4 +23,4 @@ Error: an error!
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*)
     at Module.load (internal/modules/cjs/loader.js:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*)
-    at Module.require (internal/modules/cjs/loader.js:*)
+    at Module.internalRequire (internal/modules/cjs/loader.js:*)
diff --git a/test/message/source_map_enabled_by_api.out b/test/message/source_map_enabled_by_api.out
@@ -12,7 +12,7 @@ Error: an error!
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*)
     at Module.load (internal/modules/cjs/loader.js:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*)
-    at Module.require (internal/modules/cjs/loader.js:*)
+    at Module.internalRequire (internal/modules/cjs/loader.js:*)
 *enclosing-call-site-min.js:1
 var functionA=function(){functionB()};function functionB(){functionC()}var functionC=function(){functionD()},functionD=function(){if(0<Math.random())throw Error("an error!");},thrower=functionA;try{functionA()}catch(a){throw a;};
                                                                                                                                                                                                                            ^
@@ -27,4 +27,4 @@ Error: an error!
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*)
     at Module.load (internal/modules/cjs/loader.js:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*)
-    at Module.require (internal/modules/cjs/loader.js:*)
+    at Module.internalRequire (internal/modules/cjs/loader.js:*)
diff --git a/test/message/source_map_enclosing_function.out b/test/message/source_map_enclosing_function.out
@@ -12,4 +12,4 @@ Error: an error!
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*)
     at Module.load (internal/modules/cjs/loader.js:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*)
-    at Module.require (internal/modules/cjs/loader.js:*)
+    at Module.internalRequire (internal/modules/cjs/loader.js:*)
diff --git a/test/message/source_map_reference_error_tabs.out b/test/message/source_map_reference_error_tabs.out
@@ -9,7 +9,7 @@ ReferenceError: alert is not defined
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*
     at Module.load (internal/modules/cjs/loader.js:*
     at Function.Module._load (internal/modules/cjs/loader.js:*
-    at Module.require (internal/modules/cjs/loader.js:*
+    at Module.internalRequire (internal/modules/cjs/loader.js:*
     at require (internal/modules/cjs/helpers.js:*
     at Object.<anonymous> (*source_map_reference_error_tabs.js:*
     at Module._compile (internal/modules/cjs/loader.js:*
diff --git a/test/message/source_map_throw_catch.out b/test/message/source_map_throw_catch.out
@@ -9,7 +9,7 @@ Error: an exception
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*)
     at Module.load (internal/modules/cjs/loader.js:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*)
-    at Module.require (internal/modules/cjs/loader.js:*)
+    at Module.internalRequire (internal/modules/cjs/loader.js:*)
     at require (internal/modules/cjs/helpers.js:*)
     at Object.<anonymous> (*source_map_throw_catch.js:6:3)
     at Module._compile (internal/modules/cjs/loader.js:*)
diff --git a/test/message/source_map_throw_first_tick.out b/test/message/source_map_throw_first_tick.out
@@ -9,7 +9,7 @@ Error: an exception
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*)
     at Module.load (internal/modules/cjs/loader.js:*)
     at Function.Module._load (internal/modules/cjs/loader.js:*)
-    at Module.require (internal/modules/cjs/loader.js:*)
+    at Module.internalRequire (internal/modules/cjs/loader.js:*)
     at require (internal/modules/cjs/helpers.js:*)
     at Object.<anonymous> (*source_map_throw_first_tick.js:5:1)
     at Module._compile (internal/modules/cjs/loader.js:*)
diff --git a/test/message/source_map_throw_icu.out b/test/message/source_map_throw_icu.out
@@ -9,7 +9,7 @@ Error: an error
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:*
     at Module.load (internal/modules/cjs/loader.js:*
     at Function.Module._load (internal/modules/cjs/loader.js:*
-    at Module.require (internal/modules/cjs/loader.js:*
+    at Module.internalRequire (internal/modules/cjs/loader.js:*
     at require (internal/modules/cjs/helpers.js:*
     at Object.<anonymous> (*source_map_throw_icu.js:*
     at Module._compile (internal/modules/cjs/loader.js:*
diff --git a/test/parallel/test-module-prototype-mutation.js b/test/parallel/test-module-prototype-mutation.js
@@ -0,0 +1,12 @@
+'use strict';
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+
+assert.strictEqual(
+  require(fixtures.path('es-module-specifiers', 'node_modules', 'no-main-field')),
+  'no main field'
+);
+
+import(fixtures.fileURL('es-module-specifiers', 'index.mjs'))
+  .then(common.mustCall((module) => assert.strictEqual(module.noMain, 'no main field')));