module: fix sync resolve hooks for require with node: prefixes

lib/internal/modules/cjs/loader.js

@@ -1111,7 +1111,9 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
     filename = convertURLToCJSFilename(url);
   }
 
-  return { __proto__: null, url, format, filename, parentURL };
+  const result = { __proto__: null, url, format, filename, parentURL };
+  debug('resolveForCJSWithHooks', specifier, parent?.id, '->', result);
+  return result;
 }
 
 /**
@@ -1168,24 +1170,29 @@ function getDefaultLoad(url, filename) {
  * @param {string} id The module ID (without the node: prefix)
  * @param {string} url The module URL (with the node: prefix)
  * @param {string} format Format from resolution.
- * @returns {any} If there are no load hooks or the load hooks do not override the format of the
- *   builtin, load and return the exports of the builtin. Otherwise, return undefined.
+ * @returns {{builtinExports: any, resultFromHook: undefined|ModuleLoadResult}} If there are no load
+ *   hooks or the load hooks do not override the format of the builtin, load and return the exports
+ *   of the builtin module. Otherwise, return the loadResult for the caller to continue loading.
  */
 function loadBuiltinWithHooks(id, url, format) {
+  let resultFromHook;
   if (loadHooks.length) {
     url ??= `node:${id}`;
+    debug('loadBuiltinWithHooks ', loadHooks.length, id, url, format);
     // TODO(joyeecheung): do we really want to invoke the load hook for the builtins?
-    const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined,
-                                     getCjsConditionsArray(), getDefaultLoad(url, id), validateLoadStrict);
-    if (loadResult.format && loadResult.format !== 'builtin') {
-      return undefined;  // Format has been overridden, return undefined for the caller to continue loading.
+    resultFromHook = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined,
+                                   getCjsConditionsArray(), getDefaultLoad(url, id), validateLoadStrict);
+    if (resultFromHook.format && resultFromHook.format !== 'builtin') {
+      debug('loadBuiltinWithHooks overriding module', id, url, resultFromHook);
+      // Format has been overridden, return result for the caller to continue loading.
+      return { builtinExports: undefined, resultFromHook };
     }
   }
 
   // No hooks or the hooks have not overridden the format. Load it as a builtin module and return the
   // exports.
   const mod = loadBuiltinModule(id);
-  return mod.exports;
+  return { builtinExports: mod.exports, resultFromHook: undefined };
 }
 
 /**
@@ -1223,47 +1230,64 @@ Module._load = function(request, parent, isMain) {
     }
   }
 
-  const { url, format, filename } = resolveForCJSWithHooks(request, parent, isMain);
+  const resolveResult = resolveForCJSWithHooks(request, parent, isMain);
+  let { format } = resolveResult;
+  const { url, filename } = resolveResult;
 
+  let resultFromLoadHook;
   // For backwards compatibility, if the request itself starts with node:, load it before checking
   // Module._cache. Otherwise, load it after the check.
-  if (StringPrototypeStartsWith(request, 'node:')) {
-    const result = loadBuiltinWithHooks(filename, url, format);
-    if (result) {
-      return result;
+  // TODO(joyeecheung): a more sensible handling is probably, if there are hooks, always go through the hooks
+  // first before checking the cache. Otherwise, check the cache first, then proceed to default loading.
+  if (request === url && StringPrototypeStartsWith(request, 'node:')) {
+    const normalized = BuiltinModule.normalizeRequirableId(request);
+    if (normalized) {  // It's a builtin module.
+      const { resultFromHook, builtinExports } = loadBuiltinWithHooks(normalized, url, format);
+      if (builtinExports) {
+        return builtinExports;
+      }
+      // The format of the builtin has been overridden by user hooks. Continue loading.
+      resultFromLoadHook = resultFromHook;
+      format = resultFromLoadHook.format;
     }
-    // The format of the builtin has been overridden by user hooks. Continue loading.
   }
 
-  const cachedModule = Module._cache[filename];
-  if (cachedModule !== undefined) {
-    updateChildren(parent, cachedModule, true);
-    if (cachedModule.loaded) {
-      return cachedModule.exports;
-    }
-    // If it's not cached by the ESM loader, the loading request
-    // comes from required CJS, and we can consider it a circular
-    // dependency when it's cached.
-    if (!cachedModule[kIsCachedByESMLoader]) {
-      return getExportsForCircularRequire(cachedModule);
-    }
-    // If it's cached by the ESM loader as a way to indirectly pass
-    // the module in to avoid creating it twice, the loading request
-    // came from imported CJS. In that case use the kModuleCircularVisited
-    // to determine if it's loading or not.
-    if (cachedModule[kModuleCircularVisited]) {
-      return getExportsForCircularRequire(cachedModule);
+  // If load hooks overrides the format for a built-in, bypass the cache.
+  let cachedModule;
+  if (resultFromLoadHook === undefined) {
+    cachedModule = Module._cache[filename];
+    debug('Module._load checking cache for', filename, !!cachedModule);
+    if (cachedModule !== undefined) {
+      updateChildren(parent, cachedModule, true);
+      if (cachedModule.loaded) {
+        return cachedModule.exports;
+      }
+      // If it's not cached by the ESM loader, the loading request
+      // comes from required CJS, and we can consider it a circular
+      // dependency when it's cached.
+      if (!cachedModule[kIsCachedByESMLoader]) {
+        return getExportsForCircularRequire(cachedModule);
+      }
+      // If it's cached by the ESM loader as a way to indirectly pass
+      // the module in to avoid creating it twice, the loading request
+      // came from imported CJS. In that case use the kModuleCircularVisited
+      // to determine if it's loading or not.
+      if (cachedModule[kModuleCircularVisited]) {
+        return getExportsForCircularRequire(cachedModule);
+      }
+      // This is an ESM loader created cache entry, mark it as visited and fallthrough to loading the module.
+      cachedModule[kModuleCircularVisited] = true;
     }
-    // This is an ESM loader created cache entry, mark it as visited and fallthrough to loading the module.
-    cachedModule[kModuleCircularVisited] = true;
   }
 
-  if (BuiltinModule.canBeRequiredWithoutScheme(filename)) {
-    const result = loadBuiltinWithHooks(filename, url, format);
-    if (result) {
-      return result;
+  if (resultFromLoadHook === undefined && BuiltinModule.canBeRequiredWithoutScheme(filename)) {
+    const { resultFromHook, builtinExports } = loadBuiltinWithHooks(filename, url, format);
+    if (builtinExports) {
+      return builtinExports;
     }
     // The format of the builtin has been overridden by user hooks. Continue loading.
+    resultFromLoadHook = resultFromHook;
+    format = resultFromLoadHook.format;
   }
 
   // Don't call updateChildren(), Module constructor already does.
@@ -1278,6 +1302,9 @@ Module._load = function(request, parent, isMain) {
     } else {
       module[kIsMainSymbol] = false;
     }
+    if (resultFromLoadHook !== undefined) {
+      module[kModuleSource] = resultFromLoadHook.source;
+    }
 
     reportModuleToWatchMode(filename);
     Module._cache[filename] = module;
@@ -1463,6 +1490,17 @@ function createEsmNotFoundErr(request, path) {
   return err;
 }
 
+function getExtensionForFormat(format) {
+  switch (format) {
+    case 'addon':
+      return '.node';
+    case 'json':
+      return '.json';
+    default:
+      return '.js';
+  }
+}
+
 /**
  * Given a file name, pass it to the proper extension handler.
  * @param {string} filename The `require` specifier
@@ -1475,7 +1513,13 @@ Module.prototype.load = function(filename) {
   this.filename ??= filename;
   this.paths ??= Module._nodeModulePaths(path.dirname(filename));
 
-  const extension = findLongestRegisteredExtension(filename);
+  // If the format is already overridden by hooks, convert that back to extension.
+  let extension;
+  if (this[kFormat] !== undefined) {
+    extension = getExtensionForFormat(this[kFormat]);
+  } else {
+    extension = findLongestRegisteredExtension(filename);
+  }
 
   Module._extensions[extension](this, filename);
   this.loaded = true;

test/fixtures/module-hooks/redirected-zlib.mjs

@@ -0,0 +1 @@
+export const url = import.meta.url;

test/module-hooks/test-module-hooks-load-builtin-override-commonjs.js

@@ -0,0 +1,37 @@
+'use strict';
+
+// This tests that load hooks can override the format of builtin modules
+// to 'commonjs' format.
+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) {
+    // Only intercept zlib builtin
+    if (url === 'node:zlib') {
+      // Return a different format to override the builtin
+      return {
+        source: 'exports.custom_zlib = "overridden by load hook";',
+        format: 'commonjs',
+        shortCircuit: true,
+      };
+    }
+    return nextLoad(url, context);
+  }, 2), // Called twice: once for 'zlib', once for 'node:zlib'
+});
+
+// Test: Load hook overrides builtin format to commonjs
+const zlib = require('zlib');
+assert.strictEqual(zlib.custom_zlib, 'overridden by load hook');
+assert.strictEqual(typeof zlib.createGzip, 'undefined'); // Original zlib API should not be available
+
+// Test with node: prefix
+const zlib2 = require('node:zlib');
+assert.strictEqual(zlib2.custom_zlib, 'overridden by load hook');
+assert.strictEqual(typeof zlib2.createGzip, 'undefined');
+
+hook.deregister();

test/module-hooks/test-module-hooks-load-builtin-override-json.js

@@ -0,0 +1,37 @@
+'use strict';
+
+// This tests that load hooks can override the format of builtin modules
+// to 'json' format.
+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) {
+    // Only intercept zlib builtin
+    if (url === 'node:zlib') {
+      // Return JSON format to override the builtin
+      return {
+        source: JSON.stringify({ custom_zlib: 'JSON overridden zlib' }),
+        format: 'json',
+        shortCircuit: true,
+      };
+    }
+    return nextLoad(url, context);
+  }, 2), // Called twice: once for 'zlib', once for 'node:zlib'
+});
+
+// Test: Load hook overrides builtin format to json
+const zlib = require('zlib');
+assert.strictEqual(zlib.custom_zlib, 'JSON overridden zlib');
+assert.strictEqual(typeof zlib.createGzip, 'undefined'); // Original zlib API should not be available
+
+// Test with node: prefix
+const zlib2 = require('node:zlib');
+assert.strictEqual(zlib2.custom_zlib, 'JSON overridden zlib');
+assert.strictEqual(typeof zlib2.createGzip, 'undefined');
+
+hook.deregister();

test/module-hooks/test-module-hooks-load-builtin-override-module.js

@@ -0,0 +1,41 @@
+'use strict';
+
+// This tests that load hooks can override the format of builtin modules
+// to 'module', and require() can load them.
+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) {
+    // Only intercept zlib builtin
+    if (url === 'node:zlib') {
+      // Return ES module format to override the builtin
+      // Note: For require() to work with ESM, we need to export 'module.exports'
+      return {
+        source: `const exports = { custom_zlib: "ESM overridden zlib" };
+                 export default exports;
+                 export { exports as 'module.exports' };`,
+        format: 'module',
+        shortCircuit: true,
+      };
+    }
+    return nextLoad(url, context);
+  }, 2), // Called twice: once for 'zlib', once for 'node:zlib'
+});
+
+// Test: Load hook overrides builtin format to module.
+// With the 'module.exports' export, require() should work
+const zlib = require('zlib');
+assert.strictEqual(zlib.custom_zlib, 'ESM overridden zlib');
+assert.strictEqual(typeof zlib.createGzip, 'undefined'); // Original zlib API should not be available
+
+// Test with node: prefix
+const zlib2 = require('node:zlib');
+assert.strictEqual(zlib2.custom_zlib, 'ESM overridden zlib');
+assert.strictEqual(typeof zlib2.createGzip, 'undefined');
+
+hook.deregister();

test/module-hooks/test-module-hooks-resolve-builtin-on-disk-require-with-prefix.js

@@ -0,0 +1,33 @@
+'use strict';
+
+// This tests that builtins can be redirected to a local file when they are prefixed
+// with `node:`.
+require('../common');
+
+const assert = require('assert');
+const { registerHooks } = require('module');
+const fixtures = require('../common/fixtures');
+
+// This tests that builtins can be redirected to a local file.
+// Pick a builtin that's unlikely to be loaded already - like zlib.
+assert(!process.moduleLoadList.includes('NativeModule zlib'));
+
+const hook = registerHooks({
+  resolve(specifier, context, nextLoad) {
+    specifier = specifier.replaceAll('node:', '');
+    return {
+      url: fixtures.fileURL('module-hooks', `redirected-${specifier}.js`).href,
+      shortCircuit: true,
+    };
+  },
+});
+
+// Check assert, which is already loaded.
+// eslint-disable-next-line node-core/must-call-assert
+assert.strictEqual(require('node:assert').exports_for_test, 'redirected assert');
+// Check zlib, which is not yet loaded.
+assert.strictEqual(require('node:zlib').exports_for_test, 'redirected zlib');
+// Check fs, which is redirected to an ESM
+assert.strictEqual(require('node:fs').exports_for_test, 'redirected fs');
+
+hook.deregister();

test/module-hooks/test-module-hooks-resolve-load-builtin-override-both-prefix.js

@@ -0,0 +1,36 @@
+'use strict';
+
+// This tests the interaction between resolve and load hooks for builtins with the
+// `node:` prefix.
+const common = require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+const fixtures = require('../common/fixtures');
+
+// Pick a builtin that's unlikely to be loaded already - like zlib.
+assert(!process.moduleLoadList.includes('NativeModule zlib'));
+
+const redirectedURL = fixtures.fileURL('module-hooks/redirected-zlib.js').href;
+
+registerHooks({
+  resolve: common.mustCall(function resolve(specifier, context, nextResolve) {
+    assert.strictEqual(specifier, 'node:zlib');
+    return {
+      url: redirectedURL,
+      format: 'module',
+      shortCircuit: true,
+    };
+  }),
+
+  load: common.mustCall(function load(url, context, nextLoad) {
+    assert.strictEqual(url, redirectedURL);
+    return {
+      source: 'export const loadURL = import.meta.url;',
+      format: 'module',
+      shortCircuit: true,
+    };
+  }),
+});
+
+const zlib = require('node:zlib');
+assert.strictEqual(zlib.loadURL, redirectedURL);