module: use CJS conditions for hook-loaded require

Pass CommonJS package export conditions through synchronous resolution
that blocks on async loader hooks when the request originates from
require() in a CommonJS module loaded by module.register().

This keeps async resolve hooks from seeing ESM import conditions for
require.resolve().

Fixes: #51327
Signed-off-by: Bob Put <bobu.work@gmail.com>

Author: bobu-putheeckal

lib/internal/modules/esm/hooks.js

@@ -115,11 +115,11 @@ function defineImportAssertionAlias(context) {
  *   Calling the asynchronous `load` hook asynchronously.
  * @property {(url: string, context: object, defaultLoad: Function) => LoadResult} [loadSync]
  *   Calling the asynchronous `load` hook synchronously.
- * @property {(originalSpecifier: string, parentURL: string,
- *             importAttributes: Record<string, string>) => Promise<ResolveResult>} resolve
+ * @property {(originalSpecifier: string, parentURL: string, importAttributes:
+ *   Record<string, string>, conditions?: string[]) => Promise<ResolveResult>} resolve
  *   Calling the asynchronous `resolve` hook asynchronously.
- * @property {(originalSpecifier: string, parentURL: string,
- *            importAttributes: Record<string, string>) => ResolveResult} [resolveSync]
+ * @property {(originalSpecifier: string, parentURL: string, importAttributes:
+ *   Record<string, string>, conditions?: string[]) => ResolveResult} [resolveSync]
  *   Calling the asynchronous `resolve` hook synchronously.
  * @property {(specifier: string, parentURL: string) => any} register Register asynchronous loader hooks
  * @property {() => void} waitForLoaderHookInitialization Force loading of hooks.
@@ -219,18 +219,20 @@ class AsyncLoaderHooksOnLoaderHookWorker {
    * @param {string} [parentURL] The URL path of the module's parent.
    * @param {ImportAttributes} [importAttributes] Attributes from the import
    *   statement or expression.
+   * @param {string[]} [conditions] Package export conditions for this request.
    * @returns {Promise<{ format: string, url: URL['href'] }>}
    */
   async resolve(
     originalSpecifier,
     parentURL,
     importAttributes = { __proto__: null },
+    conditions = getDefaultConditions(),
   ) {
     throwIfInvalidParentURL(parentURL);
 
     const chain = this.#chains.resolve;
     const context = {
-      conditions: getDefaultConditions(),
+      conditions,
       importAttributes,
       parentURL,
     };
@@ -834,15 +836,30 @@ class AsyncLoaderHooksProxiedToLoaderHookWorker {
    * @param {string} [parentURL] The URL path of the module's parent.
    * @param {ImportAttributes} importAttributes Attributes from the import
    *   statement or expression.
+   * @param {string[]} [conditions] Package export conditions for this request.
    * @returns {{ format: string, url: URL['href'] }}
    */
-  resolve(originalSpecifier, parentURL, importAttributes) {
-    return asyncLoaderHookWorker.makeAsyncRequest('resolve', undefined, originalSpecifier, parentURL, importAttributes);
+  resolve(originalSpecifier, parentURL, importAttributes, conditions) {
+    return asyncLoaderHookWorker.makeAsyncRequest(
+      'resolve',
+      undefined,
+      originalSpecifier,
+      parentURL,
+      importAttributes,
+      conditions,
+    );
   }
 
-  resolveSync(originalSpecifier, parentURL, importAttributes) {
+  resolveSync(originalSpecifier, parentURL, importAttributes, conditions) {
     // This happens only as a result of `import.meta.resolve` calls, which must be sync per spec.
-    return asyncLoaderHookWorker.makeSyncRequest('resolve', undefined, originalSpecifier, parentURL, importAttributes);
+    return asyncLoaderHookWorker.makeSyncRequest(
+      'resolve',
+      undefined,
+      originalSpecifier,
+      parentURL,
+      importAttributes,
+      conditions,
+    );
   }
 
   /**

lib/internal/modules/esm/loader.js

@@ -61,6 +61,7 @@ const {
   setImportMetaResolveInitializer,
 } = internalBinding('module_wrap');
 const {
+  getCjsConditionsArray,
   urlToFilename,
 } = require('internal/modules/helpers');
 const {
@@ -600,7 +601,9 @@ class ModuleLoader {
     let maybePromise;
     if (requestType === kRequireInImportedCJS || requestType === kImportInRequiredESM) {
       // In these two cases, resolution must be synchronous.
-      maybePromise = this.resolveSync(parentURL, request);
+      const conditions = requestType === kRequireInImportedCJS ?
+        getCjsConditionsArray() : this.#defaultConditions;
+      maybePromise = this.resolveSync(parentURL, request, false, conditions);
       assert(!isPromise(maybePromise));
     } else {
       maybePromise = this.#resolve(parentURL, request);
@@ -680,7 +683,7 @@ class ModuleLoader {
       const specifier = `${request.specifier}`;
       const importAttributes = request.attributes ?? kEmptyObject;
       // TODO(joyeecheung): invoke the synchronous hooks in the default step on loader thread.
-      return this.#asyncLoaderHooks.resolve(specifier, parentURL, importAttributes);
+      return this.#asyncLoaderHooks.resolve(specifier, parentURL, importAttributes, request.conditions);
     }
 
     return this.resolveSync(parentURL, request);
@@ -718,7 +721,12 @@ class ModuleLoader {
    */
   #resolveAndMaybeBlockOnLoaderThread(out, specifier, context) {
     if (this.#asyncLoaderHooks?.resolveSync) {
-      return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes);
+      return this.#asyncLoaderHooks.resolveSync(
+        specifier,
+        context.parentURL,
+        context.importAttributes,
+        context.conditions,
+      );
     }
     out.isResolvedByDefaultResolve = true;
     return this.#cachedDefaultResolve(specifier, context);
@@ -735,9 +743,15 @@ class ModuleLoader {
    * @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.
+   * @param {string[]} [conditions] Package export conditions for this request.
    * @returns {ResolveResult}
    */
-  resolveSync(parentURL, request, shouldSkipSyncHooks = false) {
+  resolveSync(
+    parentURL,
+    request,
+    shouldSkipSyncHooks = false,
+    conditions = request.conditions ?? this.#defaultConditions,
+  ) {
     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.
@@ -747,14 +761,14 @@ class ModuleLoader {
     let isResolvedBySyncHooks = false;
     if (!shouldSkipSyncHooks && syncResolveHooks.length) {
       // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step.
-      result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
+      result = resolveWithSyncHooks(specifier, parentURL, importAttributes, conditions,
                                     this.#resolveAndMaybeBlockOnLoaderThread.bind(this, out));
       // If the default step ran, sync hooks did not short-circuit the resolution.
       isResolvedBySyncHooks = !out.isResolvedByDefaultResolve;
     } else {
       const context = {
         ...request,
-        conditions: this.#defaultConditions,
+        conditions,
         parentURL,
         importAttributes,
         __proto__: null,

lib/internal/modules/esm/translators.js

@@ -25,6 +25,7 @@ const assert = require('internal/assert');
 const { dirname, extname } = require('path');
 const {
   assertBufferSource,
+  getCjsConditionsArray,
   loadBuiltinModule,
   stringify,
   stripBOM,
@@ -178,7 +179,12 @@ function loadCJSModuleWithSpecialRequire(module, source, url, filename, isMain,
       }
     }
 
-    const request = { specifier, __proto__: null, attributes: kEmptyObject };
+    const request = {
+      specifier,
+      __proto__: null,
+      attributes: kEmptyObject,
+      conditions: getCjsConditionsArray(),
+    };
     // Skip sync hooks in resolveSync since resolveForCJSWithHooks already ran them above.
     const { url: resolvedURL } = cascadedLoader.resolveSync(url, request, /* shouldSkipSyncHooks */ true);
     return urlToFilename(resolvedURL);

test/module-hooks/test-async-loader-hooks-cjs-require-conditions.mjs

@@ -0,0 +1,50 @@
+import '../common/index.mjs';
+import assert from 'node:assert';
+import { register } from 'node:module';
+
+const loader = `
+  import assert from 'node:assert';
+
+  function assertCjsConditions(context) {
+    assert.ok(
+      context.conditions.includes('require'),
+      \`Conditions should include "require": \${JSON.stringify(context.conditions)}\`,
+    );
+    assert.ok(
+      !context.conditions.includes('import'),
+      \`Conditions should not include "import": \${JSON.stringify(context.conditions)}\`,
+    );
+  }
+
+  export async function resolve(specifier, context, nextResolve) {
+    if (specifier === 'custom:cjs') {
+      return { url: 'custom:cjs', format: 'commonjs', shortCircuit: true };
+    }
+
+    if (specifier === 'node:target') {
+      assertCjsConditions(context);
+      return { url: 'node:fs', shortCircuit: true };
+    }
+
+    return nextResolve(specifier, context);
+  }
+
+  export async function load(url, context, nextLoad) {
+    if (url === 'custom:cjs') {
+      return {
+        format: 'commonjs',
+        source: \`
+          module.exports = require.resolve('node:target');
+        \`,
+        shortCircuit: true,
+      };
+    }
+
+    return nextLoad(url, context);
+  }
+`;
+
+register(`data:text/javascript,${encodeURIComponent(loader)}`);
+
+const ns = await import('custom:cjs');
+assert.strictEqual(ns.default, 'node:fs');