esm: loader hook URL validation and error messages

PR-URL: https://github.com/nodejs/node/pull/21352
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

doc/api/errors.md

@@ -1209,10 +1209,23 @@ An invalid `options.protocol` was passed.
 Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which
 is not supported.
 
+<a id="ERR_INVALID_RETURN_PROPERTY"></a>
+### ERR_INVALID_RETURN_PROPERTY
+
+Thrown in case a function option does not provide a valid value for one of its
+returned object properties on execution.
+
+<a id="ERR_INVALID_RETURN_PROPERTY_VALUE"></a>
+### ERR_INVALID_RETURN_PROPERTY_VALUE
+
+Thrown in case a function option does not provide an expected value
+type for one of its returned object properties on execution.
+
 <a id="ERR_INVALID_RETURN_VALUE"></a>
 ### ERR_INVALID_RETURN_VALUE
 
-Thrown in case a function option does not return an expected value on execution.
+Thrown in case a function option does not return an expected value
+type on execution.
 For example when a function is expected to return a promise.
 
 <a id="ERR_INVALID_SYNC_FORK_INPUT"></a>

lib/internal/errors.js

@@ -681,6 +681,20 @@ E('ERR_INVALID_PROTOCOL',
   TypeError);
 E('ERR_INVALID_REPL_EVAL_CONFIG',
   'Cannot specify both "breakEvalOnSigint" and "eval" for REPL', TypeError);
+E('ERR_INVALID_RETURN_PROPERTY', (input, name, prop, value) => {
+  return `Expected a valid ${input} to be returned for the "${prop}" from the` +
+         ` "${name}" function but got ${value}.`;
+}, TypeError);
+E('ERR_INVALID_RETURN_PROPERTY_VALUE', (input, name, prop, value) => {
+  let type;
+  if (value && value.constructor && value.constructor.name) {
+    type = `instance of ${value.constructor.name}`;
+  } else {
+    type = `type ${typeof value}`;
+  }
+  return `Expected ${input} to be returned for the "${prop}" from the` +
+         ` "${name}" function but got ${type}.`;
+}, TypeError);
 E('ERR_INVALID_RETURN_VALUE', (input, name, value) => {
   let type;
   if (value && value.constructor && value.constructor.name) {

lib/internal/modules/esm/loader.js

@@ -2,10 +2,13 @@
 
 const {
   ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_PROTOCOL,
+  ERR_INVALID_RETURN_PROPERTY,
+  ERR_INVALID_RETURN_PROPERTY_VALUE,
+  ERR_INVALID_RETURN_VALUE,
   ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK,
   ERR_UNKNOWN_MODULE_FORMAT
 } = require('internal/errors').codes;
+const { URL } = require('url');
 const ModuleMap = require('internal/modules/esm/module_map');
 const ModuleJob = require('internal/modules/esm/module_job');
 const defaultResolve = require('internal/modules/esm/default_resolve');
@@ -52,20 +55,42 @@ class Loader {
     if (!isMain && typeof parentURL !== 'string')
       throw new ERR_INVALID_ARG_TYPE('parentURL', 'string', parentURL);
 
-    const { url, format } =
+    const resolved = await this._resolve(specifier, parentURL, defaultResolve);
-      await this._resolve(specifier, parentURL, defaultResolve);
+
+    if (typeof resolved !== 'object')
+      throw new ERR_INVALID_RETURN_VALUE(
+        'object', 'loader resolve', resolved
+      );
+
+    const { url, format } = resolved;
 
     if (typeof url !== 'string')
-      throw new ERR_INVALID_ARG_TYPE('url', 'string', url);
+      throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
+        'string', 'loader resolve', 'url', url
+      );
 
     if (typeof format !== 'string')
-      throw new ERR_INVALID_ARG_TYPE('format', 'string', format);
+      throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
+        'string', 'loader resolve', 'format', format
+      );
 
     if (format === 'builtin')
       return { url: `node:${url}`, format };
 
+    if (this._resolve !== defaultResolve) {
+      try {
+        new URL(url);
+      } catch (e) {
+        throw new ERR_INVALID_RETURN_PROPERTY(
+          'url', 'loader resolve', 'url', url
+        );
+      }
+    }
+
     if (format !== 'dynamic' && !url.startsWith('file:'))
-      throw new ERR_INVALID_PROTOCOL(url, 'file:');
+      throw new ERR_INVALID_RETURN_PROPERTY(
+        'file: url', 'loader resolve', 'url', url
+      );
 
     return { url, format };
   }

test/common/index.mjs

@@ -1,71 +1,123 @@
 // Flags: --experimental-modules
 /* eslint-disable node-core/required-modules */
+import common from './index.js';
 
-import assert from 'assert';
+const {
+  PORT,
+  isMainThread,
+  isWindows,
+  isWOW64,
+  isAIX,
+  isLinuxPPCBE,
+  isSunOS,
+  isFreeBSD,
+  isOpenBSD,
+  isLinux,
+  isOSX,
+  isGlibc,
+  enoughTestMem,
+  enoughTestCpu,
+  rootDir,
+  buildType,
+  localIPv6Hosts,
+  opensslCli,
+  PIPE,
+  hasIPv6,
+  childShouldThrowAndAbort,
+  ddCommand,
+  spawnPwd,
+  spawnSyncPwd,
+  platformTimeout,
+  allowGlobals,
+  leakedGlobals,
+  mustCall,
+  mustCallAtLeast,
+  mustCallAsync,
+  hasMultiLocalhost,
+  fileExists,
+  skipIfEslintMissing,
+  canCreateSymLink,
+  getCallSite,
+  mustNotCall,
+  printSkipMessage,
+  skip,
+  ArrayStream,
+  nodeProcessAborted,
+  busyLoop,
+  isAlive,
+  noWarnCode,
+  expectWarning,
+  expectsError,
+  skipIfInspectorDisabled,
+  skipIf32Bits,
+  getArrayBufferViews,
+  getBufferSources,
+  crashOnUnhandledRejection,
+  getTTYfd,
+  runWithInvalidFD,
+  hijackStdout,
+  hijackStderr,
+  restoreStdout,
+  restoreStderr,
+  isCPPSymbolsNotMapped
+} = common;
 
-let knownGlobals = [
+export {
-  Buffer,
+  PORT,
-  clearImmediate,
+  isMainThread,
-  clearInterval,
+  isWindows,
-  clearTimeout,
+  isWOW64,
-  global,
+  isAIX,
-  process,
+  isLinuxPPCBE,
-  setImmediate,
+  isSunOS,
-  setInterval,
+  isFreeBSD,
-  setTimeout
+  isOpenBSD,
-];
+  isLinux,
-
+  isOSX,
-if (process.env.NODE_TEST_KNOWN_GLOBALS) {
+  isGlibc,
-  const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
+  enoughTestMem,
-  allowGlobals(...knownFromEnv);
+  enoughTestCpu,
-}
+  rootDir,
-
+  buildType,
-export function allowGlobals(...whitelist) {
+  localIPv6Hosts,
-  knownGlobals = knownGlobals.concat(whitelist);
+  opensslCli,
-}
+  PIPE,
-
+  hasIPv6,
-export function leakedGlobals() {
+  childShouldThrowAndAbort,
-  // Add possible expected globals
+  ddCommand,
-  if (global.gc) {
+  spawnPwd,
-    knownGlobals.push(global.gc);
+  spawnSyncPwd,
-  }
+  platformTimeout,
-
+  allowGlobals,
-  if (global.DTRACE_HTTP_SERVER_RESPONSE) {
+  leakedGlobals,
-    knownGlobals.push(DTRACE_HTTP_SERVER_RESPONSE);
+  mustCall,
-    knownGlobals.push(DTRACE_HTTP_SERVER_REQUEST);
+  mustCallAtLeast,
-    knownGlobals.push(DTRACE_HTTP_CLIENT_RESPONSE);
+  mustCallAsync,
-    knownGlobals.push(DTRACE_HTTP_CLIENT_REQUEST);
+  hasMultiLocalhost,
-    knownGlobals.push(DTRACE_NET_STREAM_END);
+  fileExists,
-    knownGlobals.push(DTRACE_NET_SERVER_CONNECTION);
+  skipIfEslintMissing,
-  }
+  canCreateSymLink,
-
+  getCallSite,
-  if (global.COUNTER_NET_SERVER_CONNECTION) {
+  mustNotCall,
-    knownGlobals.push(COUNTER_NET_SERVER_CONNECTION);
+  printSkipMessage,
-    knownGlobals.push(COUNTER_NET_SERVER_CONNECTION_CLOSE);
+  skip,
-    knownGlobals.push(COUNTER_HTTP_SERVER_REQUEST);
+  ArrayStream,
-    knownGlobals.push(COUNTER_HTTP_SERVER_RESPONSE);
+  nodeProcessAborted,
-    knownGlobals.push(COUNTER_HTTP_CLIENT_REQUEST);
+  busyLoop,
-    knownGlobals.push(COUNTER_HTTP_CLIENT_RESPONSE);
+  isAlive,
-  }
+  noWarnCode,
-
+  expectWarning,
-  const leaked = [];
+  expectsError,
-
+  skipIfInspectorDisabled,
-  for (const val in global) {
+  skipIf32Bits,
-    if (!knownGlobals.includes(global[val])) {
+  getArrayBufferViews,
-      leaked.push(val);
+  getBufferSources,
-    }
+  crashOnUnhandledRejection,
-  }
+  getTTYfd,
-
+  runWithInvalidFD,
-  if (global.__coverage__) {
+  hijackStdout,
-    return leaked.filter((varname) => !/^(?:cov_|__cov)/.test(varname));
+  hijackStderr,
-  } else {
+  restoreStdout,
-    return leaked;
+  restoreStderr,
-  }
+  isCPPSymbolsNotMapped
-}
+};
-
-process.on('exit', function() {
-  const leaked = leakedGlobals();
-  if (leaked.length > 0) {
-    assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
-  }
-});

test/es-module/test-esm-loader-invalid-format.mjs

@@ -0,0 +1,11 @@
+// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-format.mjs
+import { expectsError, mustCall } from '../common';
+import assert from 'assert';
+
+import('../fixtures/es-modules/test-esm-ok.mjs')
+.then(assert.fail, expectsError({
+  code: 'ERR_INVALID_RETURN_PROPERTY',
+  message: 'Expected string to be returned for the "url" from the ' +
+           '"loader resolve" function but got "undefined"'
+}))
+.then(mustCall());

test/es-module/test-esm-loader-invalid-url.mjs

@@ -0,0 +1,12 @@
+// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs
+import { expectsError, mustCall } from '../common';
+import assert from 'assert';
+
+import('../fixtures/es-modules/test-esm-ok.mjs')
+.then(assert.fail, expectsError({
+  code: 'ERR_INVALID_RETURN_PROPERTY',
+  message: 'Expected a valid url to be returned for the "url" from the ' +
+           '"loader resolve" function but got ' +
+           '../fixtures/es-modules/test-esm-ok.mjs.'
+}))
+.then(mustCall());

test/fixtures/es-module-loaders/loader-invalid-format.mjs

@@ -0,0 +1,8 @@
+export async function resolve(specifier, parentModuleURL, defaultResolve) {
+  if (parentModuleURL && specifier === '../fixtures/es-modules/test-esm-ok.mjs') {
+    return {
+      url: 'file:///asdf'
+    };
+  }
+  return defaultResolve(specifier, parentModuleURL);
+}

test/fixtures/es-module-loaders/loader-invalid-url.mjs

@@ -0,0 +1,9 @@
+export async function resolve(specifier, parentModuleURL, defaultResolve) {
+  if (parentModuleURL && specifier === '../fixtures/es-modules/test-esm-ok.mjs') {
+    return {
+      url: specifier,
+      format: 'esm'
+    };
+  }
+  return defaultResolve(specifier, parentModuleURL);
+}