From 4e7e478e922333e79bdf0b2e397a441a054d5c9b Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 10 May 2020 13:49:23 +0200 Subject: [PATCH] module: do not check circular dependencies for exported proxies In case the exported module is a proxy that has the `getPrototypeOf` or `setPrototypeOf` trap, skip the circular dependencies check. It would otherwise be triggered by the check itself. Fixes: https://github.com/nodejs/node/issues/33334 Signed-off-by: Ruben Bridgewater --- lib/internal/modules/cjs/loader.js | 18 +++++++++++++----- .../cycles/warning-skip-proxy-traps-a.js | 17 +++++++++++++++++ .../cycles/warning-skip-proxy-traps-b.js | 10 ++++++++++ .../test-module-circular-dependency-warning.js | 5 +++++ 4 files changed, 45 insertions(+), 5 deletions(-) create mode 100644 test/fixtures/cycles/warning-skip-proxy-traps-a.js create mode 100644 test/fixtures/cycles/warning-skip-proxy-traps-b.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index f2715d07cb44fb..af28bc85e1e6b9 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -107,6 +107,11 @@ const { CHAR_COLON } = require('internal/constants'); + +const { + isProxy +} = require('internal/util/types'); + const isWindows = process.platform === 'win32'; const relativeResolveCache = ObjectCreate(null); @@ -821,7 +826,7 @@ function emitCircularRequireWarning(prop) { } // A Proxy that can be used as the prototype of a module.exports object and -// warns when non-existend properties are accessed. +// warns when non-existent properties are accessed. const CircularRequirePrototypeWarningProxy = new Proxy({}, { get(target, prop) { // Allow __esModule access in any case because it is used in the output @@ -840,20 +845,22 @@ const CircularRequirePrototypeWarningProxy = new Proxy({}, { } }); -// Object.prototype and ObjectProtoype refer to our 'primordials' versions +// Object.prototype and ObjectPrototype refer to our 'primordials' versions // and are not identical to the versions on the global object. const PublicObjectPrototype = global.Object.prototype; function getExportsForCircularRequire(module) { if (module.exports && + !isProxy(module.exports) && ObjectGetPrototypeOf(module.exports) === PublicObjectPrototype && // Exclude transpiled ES6 modules / TypeScript code because those may - // employ unusual patterns for accessing 'module.exports'. That should be - // okay because ES6 modules have a different approach to circular + // employ unusual patterns for accessing 'module.exports'. That should + // be okay because ES6 modules have a different approach to circular // dependencies anyway. !module.exports.__esModule) { // This is later unset once the module is done loading. - ObjectSetPrototypeOf(module.exports, CircularRequirePrototypeWarningProxy); + ObjectSetPrototypeOf( + module.exports, CircularRequirePrototypeWarningProxy); } return module.exports; @@ -943,6 +950,7 @@ Module._load = function(request, parent, isMain) { } } } else if (module.exports && + !isProxy(module.exports) && ObjectGetPrototypeOf(module.exports) === CircularRequirePrototypeWarningProxy) { ObjectSetPrototypeOf(module.exports, PublicObjectPrototype); diff --git a/test/fixtures/cycles/warning-skip-proxy-traps-a.js b/test/fixtures/cycles/warning-skip-proxy-traps-a.js new file mode 100644 index 00000000000000..b64f8e633ebfff --- /dev/null +++ b/test/fixtures/cycles/warning-skip-proxy-traps-a.js @@ -0,0 +1,17 @@ +module.exports = new Proxy({}, { + get(_target, prop) { throw new Error(`get: ${String(prop)}`); }, + getPrototypeOf() { throw new Error('getPrototypeOf'); }, + setPrototypeOf() { throw new Error('setPrototypeOf'); }, + isExtensible() { throw new Error('isExtensible'); }, + preventExtensions() { throw new Error('preventExtensions'); }, + getOwnPropertyDescriptor() { throw new Error('getOwnPropertyDescriptor'); }, + defineProperty() { throw new Error('defineProperty'); }, + has() { throw new Error('has'); }, + set() { throw new Error('set'); }, + deleteProperty() { throw new Error('deleteProperty'); }, + ownKeys() { throw new Error('ownKeys'); }, + apply() { throw new Error('apply'); }, + construct() { throw new Error('construct'); } +}); + +require('./warning-skip-proxy-traps-b.js'); diff --git a/test/fixtures/cycles/warning-skip-proxy-traps-b.js b/test/fixtures/cycles/warning-skip-proxy-traps-b.js new file mode 100644 index 00000000000000..e686bd719c136f --- /dev/null +++ b/test/fixtures/cycles/warning-skip-proxy-traps-b.js @@ -0,0 +1,10 @@ +const assert = require('assert'); + +const object = require('./warning-skip-proxy-traps-a.js'); + +assert.throws(() => { + object.missingPropProxyTrap; +}, { + message: 'get: missingPropProxyTrap', + name: 'Error', +}); diff --git a/test/parallel/test-module-circular-dependency-warning.js b/test/parallel/test-module-circular-dependency-warning.js index 1fe82c2b0288d5..6e2924c45eefff 100644 --- a/test/parallel/test-module-circular-dependency-warning.js +++ b/test/parallel/test-module-circular-dependency-warning.js @@ -38,3 +38,8 @@ assert.strictEqual(esmTranspiledExport.__esModule, true); const halfTranspiledExport = require(fixtures.path('cycles', 'warning-esm-half-transpiled-a.js')); assert.strictEqual(halfTranspiledExport.__esModule, undefined); + +// No circular check is done to prevent triggering proxy traps, if +// module.exports is set to a proxy that contains a `getPrototypeOf` or +// `setPrototypeOf` trap. +require(fixtures.path('cycles', 'warning-skip-proxy-traps-a.js'));