From 169bf7a9cf50f745ce4f01505c6c4c66e0c8d381 Mon Sep 17 00:00:00 2001 From: Sebastian Danielsson Date: Sat, 31 Oct 2026 22:44:45 +0100 Subject: [PATCH] module: fix imported CJS customized by sync hooks When module.register() hooks are registered, they run in the default step of the module.registerHooks() resolve chain, but reaching that step was only tracked when there were no asynchronous hooks. Imported CommonJS resolved by a synchronous hook that calls nextResolve() was then treated as short-circuited, and the format from the hooks made the CommonJS loader bypass require.extensions. Since #62920, the CommonJS loader also resolved and loaded imported CommonJS again when the resolution was not short-circuited, even when synchronous load hooks provided the source, ignoring their format and failing for modules that are not on disk. With asynchronous hooks, this only worked because of the wrong tracking above. Track reaching the default resolve step in both cases. Pass the resolution, format and source on to the CommonJS loader when the synchronous load hooks provide a source, unless it's the format and source from the default step passed on as-is. Leave builtins alone, as Module._load() runs the load hooks again for them. Setups that only worked because of the wrong tracking, such as an asynchronous resolve hook overriding the format while synchronous hooks are also registered, behave as before #62920 again. Fixes: https://github.com/nodejs/node/issues/66998 Refs: https://github.com/nodejs/node/pull/62920 Signed-off-by: Sebastian Danielsson Assisted-by: Claude Code --- lib/internal/modules/esm/loader.js | 56 ++++++++++++--- lib/internal/modules/esm/translators.js | 13 ++-- .../import-cjs-custom-format/data.json | 1 + .../import-cjs-custom-format/format-only.mjs | 1 + .../import-cjs-custom-format/main.js | 70 +++++++++++++++++++ .../import-cjs-custom-format/module.mjs | 1 + .../import-cjs-custom-format/mutated.mjs | 1 + .../import-cjs-require-extensions/main.js | 45 ++++++++++++ .../import-cjs-require-extensions/target.ts | 1 + ...s-load-builtin-override-commonjs-import.js | 30 ++++++++ ...ule-hooks-load-import-cjs-custom-format.js | 30 ++++++++ ...-import-cjs-custom-source-async-resolve.js | 55 +++++++++++++++ ...s-resolve-import-cjs-require-extensions.js | 24 +++++++ 13 files changed, 313 insertions(+), 15 deletions(-) create mode 100644 test/fixtures/module-hooks/import-cjs-custom-format/data.json create mode 100644 test/fixtures/module-hooks/import-cjs-custom-format/format-only.mjs create mode 100644 test/fixtures/module-hooks/import-cjs-custom-format/main.js create mode 100644 test/fixtures/module-hooks/import-cjs-custom-format/module.mjs create mode 100644 test/fixtures/module-hooks/import-cjs-custom-format/mutated.mjs create mode 100644 test/fixtures/module-hooks/import-cjs-require-extensions/main.js create mode 100644 test/fixtures/module-hooks/import-cjs-require-extensions/target.ts create mode 100644 test/module-hooks/test-module-hooks-load-builtin-override-commonjs-import.js create mode 100644 test/module-hooks/test-module-hooks-load-import-cjs-custom-format.js create mode 100644 test/module-hooks/test-module-hooks-load-import-cjs-custom-source-async-resolve.js create mode 100644 test/module-hooks/test-module-hooks-resolve-import-cjs-require-extensions.js diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 2e0b0fef8e9d..0f5096d05dae 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -3,7 +3,9 @@ const { ArrayPrototypeJoin, ArrayPrototypeMap, + ArrayPrototypePush, ArrayPrototypeReduce, + ArrayPrototypeSome, FunctionPrototypeCall, JSONStringify, ObjectSetPrototypeOf, @@ -123,6 +125,7 @@ const { defaultLoadSync, throwUnknownModuleFormat } = require('internal/modules/ * responseURL?: string, * translatorKey: string, * isResolvedBySyncHooks: boolean, + * isLoadedBySyncHooks: boolean, * isSourceLoadedSynchronously: boolean, * }} TranslateContext */ @@ -761,7 +764,7 @@ class ModuleLoader { /** * This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks * from module.register() which are run in a blocking fashion for it to be synchronous. - * @param {{isResolvedByDefaultResolve: boolean}} out Output object to track whether the default resolve was used + * @param {{isResolvedByDefaultStep: boolean}} out Output object to track whether the default step was used * without polluting the user-visible resolve result. * @param {string|URL} specifier See {@link resolveSync}. * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context @@ -769,10 +772,12 @@ class ModuleLoader { * @returns {{ format: string, url: string }} */ #resolveAndMaybeBlockOnLoaderThread(out, specifier, context) { + // Track that the synchronous hooks reached the default step, whether the + // asynchronous hooks or the default resolver handle it. + out.isResolvedByDefaultStep = true; if (this.#asyncLoaderHooks?.resolveSync) { return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes); } - out.isResolvedByDefaultResolve = true; return this.#cachedDefaultResolve(specifier, context); } @@ -793,7 +798,7 @@ class ModuleLoader { const specifier = `${request.specifier}`; const importAttributes = request.attributes ?? kEmptyObject; // Use an output parameter to track the state and avoid polluting the user-visible resolve results. - const out = { isResolvedByDefaultResolve: false, __proto__: null }; + const out = { isResolvedByDefaultStep: false, __proto__: null }; let result; let isResolvedBySyncHooks = false; @@ -802,7 +807,7 @@ class ModuleLoader { result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions, this.#resolveAndMaybeBlockOnLoaderThread.bind(this, out)); // If the default step ran, sync hooks did not short-circuit the resolution. - isResolvedBySyncHooks = !out.isResolvedByDefaultResolve; + isResolvedBySyncHooks = !out.isResolvedByDefaultStep; } else { const context = { ...request, @@ -824,6 +829,7 @@ class ModuleLoader { * format: ModuleFormat, * source: ModuleSource, * responseURL?: string, + * isLoadedBySyncHooks: boolean, * isSourceLoadedSynchronously: boolean, * }} LoadResult * @param {string} url The URL of the module to be loaded. @@ -841,20 +847,31 @@ class ModuleLoader { /** * This is the default load step for module.registerHooks(), which incorporates asynchronous hooks * from module.register() which are run in a blocking fashion for it to be synchronous. - * @param {{isSourceLoadedSynchronously: boolean}} out - * Output object to track whether the source was loaded synchronously without polluting - * the user-visible load result. + * @param {{isSourceLoadedSynchronously: boolean, defaultStepResults?: object[]}} out + * Output object to track whether the source was loaded synchronously, and the results + * of this step when there are synchronous hooks, without polluting the user-visible load result. * @param {string} url See {@link load} * @param {object} context See {@link load} * @returns {{ format: ModuleFormat, source: ModuleSource }} */ #loadAndMaybeBlockOnLoaderThread(out, url, context) { + let result; if (this.#asyncLoaderHooks?.loadSync) { out.isSourceLoadedSynchronously = false; - return this.#asyncLoaderHooks.loadSync(url, context); + result = this.#asyncLoaderHooks.loadSync(url, context); + } else { + out.isSourceLoadedSynchronously = true; + result = defaultLoadSync(url, context); + } + if (out.defaultStepResults !== undefined) { + // Copy the format and source, as the hooks may modify the result in place. + ArrayPrototypePush(out.defaultStepResults, { + __proto__: null, + format: result.format, + source: result.source, + }); } - out.isSourceLoadedSynchronously = true; - return defaultLoadSync(url, context); + return result; } /** @@ -870,10 +887,15 @@ class ModuleLoader { // Use an output parameter to track the state and avoid polluting the user-visible resolve results. const out = { isSourceLoadedSynchronously: true, + defaultStepResults: undefined, __proto__: null, }; let result; + let isLoadedBySyncHooks = false; if (syncLoadHooks.length) { + out.defaultStepResults = []; + let finalFormat; + let finalSource; // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step. // TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead // of converting them from plain objects in the hooks. @@ -883,11 +905,23 @@ class ModuleLoader { context.importAttributes, this.#defaultConditions, this.#loadAndMaybeBlockOnLoaderThread.bind(this, out), - validateLoadSloppy, + (url, context, result) => { + // The last result validated is the final one, before its source gets decoded. + ({ format: finalFormat, source: finalSource } = result); + return validateLoadSloppy(url, context, result); + }, + ); + // The sync hooks customized the loading if they provided a source, other than passing + // on the format and source from the default step. A nullish source for CommonJS means + // that the CommonJS loader should load it. + isLoadedBySyncHooks = finalSource != null && !ArrayPrototypeSome( + out.defaultStepResults, + (step) => step.format === finalFormat && step.source === finalSource, ); } else { result = this.#loadAndMaybeBlockOnLoaderThread(out, url, context); } + result.isLoadedBySyncHooks = isLoadedBySyncHooks; result.isSourceLoadedSynchronously = out.isSourceLoadedSynchronously; return result; } diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 583b6d1fe91d..575eb1e2a828 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -351,9 +351,9 @@ translators.set('require-commonjs-typescript', (url, translateContext, parentURL // This goes through Module._load to accommodate monkey-patchers. /** * Loads a CommonJS module through Module._load to accommodate monkey-patchers. - * If the module was resolved by synchronous hooks (i.e. not by the default resolver), - * passes the pre-resolved information and source to Module._load to avoid - * re-resolving and re-loading. + * If the module was resolved by synchronous hooks (i.e. without reaching the default step), + * or its format or source was provided by synchronous hooks, passes the pre-resolved + * information and source to Module._load to avoid re-resolving and re-loading. * @param {import('internal/modules/cjs/loader').Module} module - The module to load. * @param {string} source - The source code of the module. * @param {string} url - The URL of the module. @@ -365,7 +365,12 @@ function loadCJSModuleWithModuleLoad(module, source, url, filename, isMain, tran assert(module === CJSModule._cache[filename]); debug(`loadCJSModuleWithModuleLoad ${url}`); let exports; - if (translateContext.isResolvedBySyncHooks) { + // Otherwise the CommonJS loader would resolve the module again, which may not find it, + // and pick the extension handler by the file name instead of the format from the hooks. + // Leave builtins alone, as Module._load() runs the load hooks again for them and + // expects the builtin format. + if (translateContext.isResolvedBySyncHooks || + (translateContext.isLoadedBySyncHooks && !StringPrototypeStartsWith(url, 'node:'))) { exports = wrapModuleLoad(filename, undefined, isMain, { __proto__: null, resolved: { diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/data.json b/test/fixtures/module-hooks/import-cjs-custom-format/data.json new file mode 100644 index 000000000000..0967ef424bce --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/data.json @@ -0,0 +1 @@ +{} diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/format-only.mjs b/test/fixtures/module-hooks/import-cjs-custom-format/format-only.mjs new file mode 100644 index 000000000000..6d0da5fe8797 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/format-only.mjs @@ -0,0 +1 @@ +module.exports = { name: "format-only.mjs" }; diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/main.js b/test/fixtures/module-hooks/import-cjs-custom-format/main.js new file mode 100644 index 000000000000..4f61ce38d14b --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/main.js @@ -0,0 +1,70 @@ +'use strict'; +// Imports modules that synchronous load hooks provide as CommonJS, while the +// resolve hook passes on the default resolution when there is one. + +const { registerHooks } = require('module'); +const path = require('path'); +const { pathToFileURL } = require('url'); + +const missingURL = pathToFileURL(path.join(__dirname, 'missing.cjs')).href; +const mutatedURL = pathToFileURL(path.join(__dirname, 'mutated.mjs')).href; +const formatOnlyURL = pathToFileURL(path.join(__dirname, 'format-only.mjs')).href; +const dataURL = 'data:text/javascript,export default "from data URL";'; + +const names = new Map([ + [pathToFileURL(path.join(__dirname, 'data.json')).href, 'data.json'], + [pathToFileURL(path.join(__dirname, 'module.mjs')).href, 'module.mjs'], + [missingURL, 'missing.cjs'], + [dataURL, 'data URL'], +]); + +registerHooks({ + resolve(specifier, context, nextResolve) { + try { + return nextResolve(specifier, context); + } catch (err) { + if (specifier === './missing.cjs') { + return { url: missingURL, shortCircuit: true }; + } + throw err; + } + }, + load(url, context, nextLoad) { + // The two cases below customize the result from the default step. With + // asynchronous hooks, they still use the re-invented require(), so they don't + // report require.cache. + // TODO: load them with the CommonJS loader in that case too. + if (url === mutatedURL) { + // Modify the result in place. + const result = nextLoad(url, context); + result.format = 'commonjs'; + result.source = 'module.exports = { name: "mutated.mjs" };'; + return result; + } + if (url === formatOnlyURL) { + // Only override the format of the CommonJS source in a .mjs file. + return { ...nextLoad(url, context), format: 'commonjs' }; + } + const name = names.get(url); + if (name !== undefined) { + return { + format: 'commonjs', + source: `module.exports = { name: ${JSON.stringify(name)}, ` + + 'hasRequireCache: require.cache !== undefined };', + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, +}); + +// Import the modules one by one to keep the output in order. +let imported = Promise.resolve(); +const specifiers = [ + './data.json', './module.mjs', './missing.cjs', dataURL, './mutated.mjs', './format-only.mjs', +]; +for (const specifier of specifiers) { + imported = imported + .then(() => import(specifier)) + .then(({ default: result }) => console.log(JSON.stringify(result))); +} diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/module.mjs b/test/fixtures/module-hooks/import-cjs-custom-format/module.mjs new file mode 100644 index 000000000000..0b5728904626 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/module.mjs @@ -0,0 +1 @@ +export default 'from disk'; diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/mutated.mjs b/test/fixtures/module-hooks/import-cjs-custom-format/mutated.mjs new file mode 100644 index 000000000000..0b5728904626 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/mutated.mjs @@ -0,0 +1 @@ +export default 'from disk'; diff --git a/test/fixtures/module-hooks/import-cjs-require-extensions/main.js b/test/fixtures/module-hooks/import-cjs-require-extensions/main.js new file mode 100644 index 000000000000..12da05e7c1d1 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-require-extensions/main.js @@ -0,0 +1,45 @@ +'use strict'; +// Imports a .ts file as CommonJS, with a resolve hook that only overrides the +// format, and a load hook that doesn't provide its own source in the way named +// by the first argument. The require.extensions handler for .ts should load it. + +const fs = require('fs'); +const Module = require('module'); +const path = require('path'); +const { pathToFileURL } = require('url'); + +const targetURL = pathToFileURL(path.join(__dirname, 'target.ts')).href; + +Module._extensions['.ts'] = (mod, filename) => { + const source = fs.readFileSync(filename, 'utf8'); + mod._compile(source.replace('original', 'transformed'), filename); +}; + +const loadHooks = { + 'pass-through': (url, context, nextLoad) => nextLoad(url, context), + 'spread': (url, context, nextLoad) => ({ ...nextLoad(url, context) }), + 'next-twice': (url, context, nextLoad) => { + const result = nextLoad(url, context); + nextLoad(url, context); + return result; + }, + 'nullish-source': (url, context, nextLoad) => { + if (url === targetURL) { + return { format: 'commonjs', source: null, shortCircuit: true }; + } + return nextLoad(url, context); + }, +}; + +Module.registerHooks({ + resolve(specifier, context, nextResolve) { + const result = nextResolve(specifier, context); + if (result.url === targetURL) { + return { ...result, format: 'commonjs' }; + } + return result; + }, + load: loadHooks[process.argv[2]], +}); + +import(targetURL).then(({ default: result }) => console.log(result)); diff --git a/test/fixtures/module-hooks/import-cjs-require-extensions/target.ts b/test/fixtures/module-hooks/import-cjs-require-extensions/target.ts new file mode 100644 index 000000000000..b0a7d69e8b40 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-require-extensions/target.ts @@ -0,0 +1 @@ +module.exports = 'original'; diff --git a/test/module-hooks/test-module-hooks-load-builtin-override-commonjs-import.js b/test/module-hooks/test-module-hooks-load-builtin-override-commonjs-import.js new file mode 100644 index 000000000000..d3cd55eb2e0c --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-builtin-override-commonjs-import.js @@ -0,0 +1,30 @@ +'use strict'; + +// This tests that load hooks can override the format of imported builtin +// modules to 'commonjs' after calling nextLoad(). +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const hook = registerHooks({ + load: common.mustCall(function load(url, context, nextLoad) { + const result = nextLoad(url, context); + if (url === 'node:zlib' && result.format === 'builtin') { + return { + source: 'exports.custom_zlib = "overridden by load hook";', + format: 'commonjs', + shortCircuit: true, + }; + } + return result; + }, 2), // Called by the ESM loader, then again by Module._load() for the builtin. +}); + +import('node:zlib').then(common.mustCall((ns) => { + assert.strictEqual(ns.default.custom_zlib, 'overridden by load hook'); + assert.strictEqual(ns.default.createGzip, undefined); + hook.deregister(); +})); diff --git a/test/module-hooks/test-module-hooks-load-import-cjs-custom-format.js b/test/module-hooks/test-module-hooks-load-import-cjs-custom-format.js new file mode 100644 index 000000000000..7694db228cbc --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-import-cjs-custom-format.js @@ -0,0 +1,30 @@ +'use strict'; +// Test that imported CJS uses the format and source from synchronous load hooks, +// with and without asynchronous hooks from module.register(), when the resolve +// hooks don't short-circuit, or the module is not on disk. When the load hooks +// don't call nextLoad(), the module also gets the full CommonJS require(). +// Refs: https://github.com/nodejs/node/issues/63060 +// Refs: https://github.com/nodejs/node/issues/66998 + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const main = fixtures.path('module-hooks', 'import-cjs-custom-format', 'main.js'); +const asyncForward = fixtures.fileURL('module-hooks', 'sync-and-async', 'async-forward.js').href; + +const stdout = [ + '{"name":"data.json","hasRequireCache":true}', + '{"name":"module.mjs","hasRequireCache":true}', + '{"name":"missing.cjs","hasRequireCache":true}', + '{"name":"data URL","hasRequireCache":true}', + '{"name":"mutated.mjs"}', + '{"name":"format-only.mjs"}', +].join('\n'); + +for (const preload of [[], ['--import', asyncForward]]) { + spawnSyncAndAssert(process.execPath, [...preload, main], { + stdout, + trim: true, + }); +} diff --git a/test/module-hooks/test-module-hooks-load-import-cjs-custom-source-async-resolve.js b/test/module-hooks/test-module-hooks-load-import-cjs-custom-source-async-resolve.js new file mode 100644 index 000000000000..58872f5a6371 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-import-cjs-custom-source-async-resolve.js @@ -0,0 +1,55 @@ +'use strict'; +// Test that imported CJS resolved to a non-file URL, or to a file that is not on +// disk, by asynchronous hooks from module.register(), with the source provided by +// synchronous hooks from module.registerHooks(), won't be resolved again by the +// CJS loader (in which case it would throw because the module is not on disk). + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const { register, registerHooks } = require('module'); +const { pathToFileURL } = require('url'); + +const missingURL = pathToFileURL(path.join(__dirname, 'missing.cjs')).href; +const sources = new Map([ + ['virtual:entry', 'module.exports = "from virtual URL";'], + [missingURL, 'module.exports = "from missing file";'], +]); + +const asyncHooks = ` +export async function resolve(specifier, context, nextResolve) { + if (specifier === 'virtual:entry') { + return { url: specifier, format: 'commonjs', shortCircuit: true }; + } + if (specifier === 'missing') { + return { url: ${JSON.stringify(missingURL)}, format: 'commonjs', shortCircuit: true }; + } + return nextResolve(specifier, context); +} +`; +register(`data:text/javascript,${encodeURIComponent(asyncHooks)}`); + +const hook = registerHooks({ + resolve: common.mustCall((specifier, context, nextResolve) => { + return nextResolve(specifier, context); + }, 2), + load: common.mustCall((url, context, nextLoad) => { + if (sources.has(url)) { + return { + format: 'commonjs', + source: sources.get(url), + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, 2), +}); + +Promise.all([ + import('virtual:entry'), + import('missing'), +]).then(common.mustCall(([virtual, missing]) => { + assert.strictEqual(virtual.default, 'from virtual URL'); + assert.strictEqual(missing.default, 'from missing file'); + hook.deregister(); +})); diff --git a/test/module-hooks/test-module-hooks-resolve-import-cjs-require-extensions.js b/test/module-hooks/test-module-hooks-resolve-import-cjs-require-extensions.js new file mode 100644 index 000000000000..0a83416c2f54 --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-import-cjs-require-extensions.js @@ -0,0 +1,24 @@ +'use strict'; +// Test that when module.registerHooks() resolve hooks call nextResolve() and only +// override the format to 'commonjs', and the load hooks don't provide their own +// source, the imported CJS module is still loaded through require.extensions, +// with or without asynchronous hooks from module.register(). The asynchronous +// hooks run in the default step of the synchronous hooks, so passing through +// them should not count as a short-circuit. +// Refs: https://github.com/nodejs/node/issues/66998 + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const main = fixtures.path('module-hooks', 'import-cjs-require-extensions', 'main.js'); +const asyncForward = fixtures.fileURL('module-hooks', 'sync-and-async', 'async-forward.js').href; + +for (const preload of [[], ['--import', asyncForward]]) { + for (const load of ['pass-through', 'spread', 'next-twice', 'nullish-source']) { + spawnSyncAndAssert(process.execPath, [...preload, main, load], { + stdout: 'transformed', + trim: true, + }); + } +}