module: run require.resolve through module.registerHooks()

Previously, require.resolve() called Module._resolveFilename() directly,
bypassing any resolve hooks registered via module.registerHooks(). This
patch fixes that.

PR-URL: #62028
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -1035,41 +1035,63 @@ function getExportsForCircularRequire(module) {
   return module.exports;
 }
 
+
 /**
- * Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
- * if necessary.
+ * Wraps result of Module._resolveFilename to include additional fields for hooks.
+ * See resolveForCJSWithHooks.
  * @param {string} specifier
  * @param {Module|undefined} parent
  * @param {boolean} isMain
- * @param {boolean} shouldSkipModuleHooks
+ * @param {ResolveFilenameOptions} options
  * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
  */
-function resolveForCJSWithHooks(specifier, parent, isMain, shouldSkipModuleHooks) {
-  let defaultResolvedURL;
-  let defaultResolvedFilename;
-  let format;
-
-  function defaultResolveImpl(specifier, parent, isMain, options) {
-    // For backwards compatibility, when encountering requests starting with node:,
-    // throw ERR_UNKNOWN_BUILTIN_MODULE on failure or return the normalized ID on success
-    // without going into Module._resolveFilename.
-    let normalized;
-    if (StringPrototypeStartsWith(specifier, 'node:')) {
-      normalized = BuiltinModule.normalizeRequirableId(specifier);
-      if (!normalized) {
-        throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
-      }
-      defaultResolvedURL = specifier;
-      format = 'builtin';
-      return normalized;
+function wrapResolveFilename(specifier, parent, isMain, options) {
+  const filename = Module._resolveFilename(specifier, parent, isMain, options).toString();
+  return { __proto__: null, url: undefined, format: undefined, filename };
+}
+
+/**
+ * See resolveForCJSWithHooks.
+ * @param {string} specifier
+ * @param {Module|undefined} parent
+ * @param {boolean} isMain
+ * @param {ResolveFilenameOptions} options
+ * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
+ */
+function defaultResolveImplForCJSLoading(specifier, parent, isMain, options) {
+  // For backwards compatibility, when encountering requests starting with node:,
+  // throw ERR_UNKNOWN_BUILTIN_MODULE on failure or return the normalized ID on success
+  // without going into Module._resolveFilename.
+  let normalized;
+  if (StringPrototypeStartsWith(specifier, 'node:')) {
+    normalized = BuiltinModule.normalizeRequirableId(specifier);
+    if (!normalized) {
+      throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
     }
-    return Module._resolveFilename(specifier, parent, isMain, options).toString();
+    return { __proto__: null, url: specifier, format: 'builtin', filename: normalized };
   }
+  return wrapResolveFilename(specifier, parent, isMain, options);
+}
 
+/**
+ * Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
+ * if necessary.
+ * @param {string} specifier
+ * @param {Module|undefined} parent
+ * @param {boolean} isMain
+ * @param {object} internalResolveOptions
+ * @param {boolean} internalResolveOptions.shouldSkipModuleHooks Whether to skip module hooks.
+ * @param {ResolveFilenameOptions} internalResolveOptions.requireResolveOptions Options from require.resolve().
+ *   Only used when it comes from require.resolve().
+ * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
+ */
+function resolveForCJSWithHooks(specifier, parent, isMain, internalResolveOptions) {
+  const { requireResolveOptions, shouldSkipModuleHooks } = internalResolveOptions;
+  const defaultResolveImpl = requireResolveOptions ?
+    wrapResolveFilename : defaultResolveImplForCJSLoading;
   // Fast path: no hooks, just return simple results.
   if (!resolveHooks.length || shouldSkipModuleHooks) {
-    const filename = defaultResolveImpl(specifier, parent, isMain);
-    return { __proto__: null, url: defaultResolvedURL, filename, format };
+    return defaultResolveImpl(specifier, parent, isMain, requireResolveOptions);
   }
 
   // Slow path: has hooks, do the URL conversions and invoke hooks with contexts.
@@ -1098,27 +1120,25 @@ function resolveForCJSWithHooks(specifier, parent, isMain, shouldSkipModuleHooks
     } else {
       conditionSet = getCjsConditions();
     }
-    defaultResolvedFilename = defaultResolveImpl(specifier, parent, isMain, {
+
+    const result = defaultResolveImpl(specifier, parent, isMain, {
       __proto__: null,
+      paths: requireResolveOptions?.paths,
       conditions: conditionSet,
     });
+    // If the default resolver does not return a URL, convert it for the public API.
+    result.url ??= convertCJSFilenameToURL(result.filename);
 
-    defaultResolvedURL = convertCJSFilenameToURL(defaultResolvedFilename);
-    return { __proto__: null, url: defaultResolvedURL };
+    // Remove filename because it's not part of the public API.
+    // TODO(joyeecheung): maybe expose it in the public API to avoid re-conversion for users too.
+    return { __proto__: null, url: result.url, format: result.format };
   }
 
   const resolveResult = resolveWithHooks(specifier, parentURL, /* importAttributes */ undefined,
                                          getCjsConditionsArray(), defaultResolve);
-  const { url } = resolveResult;
-  format = resolveResult.format;
-
-  let filename;
-  if (url === defaultResolvedURL) {  // Not overridden, skip the re-conversion.
-    filename = defaultResolvedFilename;
-  } else {
-    filename = convertURLToCJSFilename(url);
-  }
-
+  const { url, format } = resolveResult;
+  // Convert the URL from the hook chain back to a filename for internal use.
+  const filename = convertURLToCJSFilename(url);
   const result = { __proto__: null, url, format, filename, parentURL };
   debug('resolveForCJSWithHooks', specifier, parent?.id, isMain, shouldSkipModuleHooks, '->', result);
   return result;
@@ -1213,10 +1233,10 @@ function loadBuiltinWithHooks(id, url, format) {
  * @param {string} request Specifier of module to load via `require`
  * @param {Module} parent Absolute path of the module importing the child
  * @param {boolean} isMain Whether the module is the main entry point
- * @param {object|undefined} options Additional options for loading the module
+ * @param {object|undefined} internalResolveOptions Additional options for loading the module
  * @returns {object}
  */
-Module._load = function(request, parent, isMain, options = kEmptyObject) {
+Module._load = function(request, parent, isMain, internalResolveOptions = kEmptyObject) {
   let relResolveCacheIdentifier;
   if (parent) {
     debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1239,7 +1259,7 @@ Module._load = function(request, parent, isMain, options = kEmptyObject) {
     }
   }
 
-  const resolveResult = resolveForCJSWithHooks(request, parent, isMain, options.shouldSkipModuleHooks);
+  const resolveResult = resolveForCJSWithHooks(request, parent, isMain, internalResolveOptions);
   let { format } = resolveResult;
   const { url, filename } = resolveResult;
 

lib/internal/modules/esm/loader.js

@@ -722,13 +722,16 @@ class ModuleLoader {
    * `module.registerHooks()` hooks.
    * @param {string} [parentURL] See {@link resolve}.
    * @param {ModuleRequest} request See {@link resolve}.
+   * @param {boolean} [shouldSkipSyncHooks] Whether to skip the synchronous hooks registered by module.registerHooks().
+   *   This is used to maintain compatibility for the re-invented require.resolve (in imported CJS customized
+   *   by module.register()`) which invokes the CJS resolution separately from the hook chain.
    * @returns {{ format: string, url: string }}
    */
-  resolveSync(parentURL, request) {
+  resolveSync(parentURL, request, shouldSkipSyncHooks = false) {
     const specifier = `${request.specifier}`;
     const importAttributes = request.attributes ?? kEmptyObject;
 
-    if (syncResolveHooks.length) {
+    if (!shouldSkipSyncHooks && syncResolveHooks.length) {
       // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
       return resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
                                   this.#resolveAndMaybeBlockOnLoaderThread.bind(this));

lib/internal/modules/esm/translators.js

@@ -93,6 +93,8 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU
 
 const { requestTypes: { kRequireInImportedCJS } } = require('internal/modules/esm/utils');
 const kShouldSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: true };
+const kShouldNotSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: false };
+
 /**
  * Loads a CommonJS module via the ESM Loader sync CommonJS translator.
  * This translator creates its own version of the `require` function passed into CommonJS modules.
@@ -163,14 +165,17 @@ function loadCJSModule(module, source, url, filename, isMain) {
   };
   setOwnProperty(requireFn, 'resolve', function resolve(specifier) {
     if (!StringPrototypeStartsWith(specifier, 'node:')) {
-      const path = CJSModule._resolveFilename(specifier, module);
-      if (specifier !== path) {
-        specifier = `${pathToFileURL(path)}`;
+      const {
+        filename, url: resolvedURL,
+      } = resolveForCJSWithHooks(specifier, module, false, kShouldNotSkipModuleHooks);
+      if (specifier !== filename) {
+        specifier = resolvedURL ?? `${pathToFileURL(filename)}`;
       }
     }
 
     const request = { specifier, __proto__: null, attributes: kEmptyObject };
-    const { url: resolvedURL } = cascadedLoader.resolveSync(url, request);
+    // Skip sync hooks in resolveSync since resolveForCJSWithHooks already ran them above.
+    const { url: resolvedURL } = cascadedLoader.resolveSync(url, request, /* shouldSkipSyncHooks */ true);
     return urlToFilename(resolvedURL);
   });
   setOwnProperty(requireFn, 'main', process.mainModule);
@@ -400,7 +405,7 @@ function cjsPreparseModuleExports(filename, source, format) {
       let resolved;
       let format;
       try {
-        ({ format, filename: resolved } = resolveForCJSWithHooks(reexport, module, false));
+        ({ format, filename: resolved } = resolveForCJSWithHooks(reexport, module, false, kShouldNotSkipModuleHooks));
       } catch (e) {
         debug(`Failed to resolve '${reexport}', skipping`, e);
         continue;

lib/internal/modules/helpers.js

@@ -40,6 +40,7 @@ const {
   flushCompileCache,
 } = internalBinding('modules');
 
+const lazyCJSLoader = getLazy(() => require('internal/modules/cjs/loader'));
 let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
   debug = fn;
 });
@@ -160,7 +161,23 @@ function makeRequireFunction(mod) {
    */
   function resolve(request, options) {
     validateString(request, 'request');
-    return Module._resolveFilename(request, mod, false, options);
+    const { resolveForCJSWithHooks } = lazyCJSLoader();
+    // require.resolve() has different behaviors from the internal resolution used by
+    // Module._load:
+    // 1. When the request resolves to a non-existent built-in, it throws MODULE_NOT_FOUND
+    //   instead of UNKNOWN_BUILTIN_MODULE. This is handled by resolveForCJSWithHooks.
+    // 2. If the request is a prefixed built-in, the returned value is also prefixed. This
+    //   is handled below.
+    const { filename, url } = resolveForCJSWithHooks(
+      request, mod, /* isMain */ false, {
+        __proto__: null,
+        shouldSkipModuleHooks: false,
+        requireResolveOptions: options ?? {},
+      });
+    if (url === request && StringPrototypeStartsWith(request, 'node:')) {
+      return url;
+    }
+    return filename;
   }
 
   require.resolve = resolve;

test/fixtures/module-hooks/require-resolve-caller.js

@@ -0,0 +1,4 @@
+// Fixture CJS file that calls require.resolve and exports the result.
+'use strict';
+const resolved = require.resolve('test-require-resolve-hook-target');
+module.exports = { resolved };

test/fixtures/module-hooks/require-resolve-paths-caller.js

@@ -0,0 +1,13 @@
+// Fixture CJS file that calls require.resolve with the paths option.
+'use strict';
+const path = require('path');
+const fixturesDir = path.resolve(__dirname, '..', '..');
+const nodeModules = path.join(fixturesDir, 'node_modules');
+
+// Use the paths option to resolve 'bar' from the fixtures node_modules.
+const resolved = require.resolve('bar', { paths: [fixturesDir] });
+const expected = path.join(nodeModules, 'bar.js');
+if (resolved !== expected) {
+  throw new Error(`Expected ${expected}, got ${resolved}`);
+}
+module.exports = { resolved };

test/module-hooks/test-module-hooks-resolve-require-resolve-builtin.js

@@ -0,0 +1,25 @@
+'use strict';
+
+// This tests that require.resolve() works with builtin redirection
+// via resolve hooks registered with module.registerHooks().
+
+require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+
+const hook = registerHooks({
+  resolve(specifier, context, nextResolve) {
+    if (specifier === 'assert') {
+      return {
+        url: 'node:zlib',
+        shortCircuit: true,
+      };
+    }
+    return nextResolve(specifier, context);
+  },
+});
+
+const resolved = require.resolve('assert');
+assert.strictEqual(resolved, 'zlib');
+
+hook.deregister();

test/module-hooks/test-module-hooks-resolve-require-resolve-consistency.js

@@ -0,0 +1,38 @@
+'use strict';
+
+// This tests that require.resolve() and require() both go through the same
+// resolve hooks registered via module.registerHooks().
+
+require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+const fixtures = require('../common/fixtures');
+const { pathToFileURL } = require('url');
+
+const redirectedPath = fixtures.path('module-hooks', 'redirected-assert.js');
+
+const resolvedSpecifiers = [];
+const hook = registerHooks({
+  resolve(specifier, context, nextResolve) {
+    if (specifier === 'test-consistency-target') {
+      resolvedSpecifiers.push(specifier);
+      return {
+        url: pathToFileURL(redirectedPath).href,
+        shortCircuit: true,
+      };
+    }
+    return nextResolve(specifier, context);
+  },
+});
+
+const resolveResult = require.resolve('test-consistency-target');
+const requireResult = require('test-consistency-target');
+
+assert.strictEqual(resolveResult, redirectedPath);
+assert.strictEqual(requireResult.exports_for_test, 'redirected assert');
+assert.deepStrictEqual(resolvedSpecifiers, [
+  'test-consistency-target',
+  'test-consistency-target',
+]);
+
+hook.deregister();

test/module-hooks/test-module-hooks-resolve-require-resolve-create-require.js

@@ -0,0 +1,31 @@
+'use strict';
+
+// This tests that require.resolve() from a require function returned by
+// module.createRequire() goes through resolve hooks registered via
+// module.registerHooks().
+
+require('../common');
+const assert = require('assert');
+const { registerHooks, createRequire } = require('module');
+const fixtures = require('../common/fixtures');
+const { pathToFileURL } = require('url');
+
+const redirectedPath = fixtures.path('module-hooks', 'redirected-assert.js');
+
+const hook = registerHooks({
+  resolve(specifier, context, nextResolve) {
+    if (specifier === 'test-create-require-resolve-target') {
+      return {
+        url: pathToFileURL(redirectedPath).href,
+        shortCircuit: true,
+      };
+    }
+    return nextResolve(specifier, context);
+  },
+});
+
+const customRequire = createRequire(__filename);
+const resolved = customRequire.resolve('test-create-require-resolve-target');
+assert.strictEqual(resolved, redirectedPath);
+
+hook.deregister();

test/module-hooks/test-module-hooks-resolve-require-resolve-fallthrough.js

@@ -0,0 +1,19 @@
+'use strict';
+
+// This tests that require.resolve() falls through to default resolution
+// when resolve hooks registered via module.registerHooks() don't override.
+
+require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+
+const hook = registerHooks({
+  resolve(specifier, context, nextResolve) {
+    return nextResolve(specifier, context);
+  },
+});
+
+const resolved = require.resolve('assert');
+assert.strictEqual(resolved, 'assert');
+
+hook.deregister();