From 1bf42f4777bfc7ce61873dbd17660b9e265357e9 Mon Sep 17 00:00:00 2001 From: guybedford Date: Mon, 18 Jun 2018 13:59:22 +0200 Subject: [PATCH] esm: loader hook URL validation and error messages PR-URL: https://github.com/nodejs/node/pull/21352 Reviewed-By: Gus Caplan Reviewed-By: James M Snell Reviewed-By: Tiancheng "Timothy" Gu --- doc/api/errors.md | 15 +- lib/internal/errors.js | 16 +- lib/internal/modules/esm/loader.js | 37 +++- test/common/index.mjs | 186 +++++++++++------- .../test-esm-loader-invalid-format.mjs | 11 ++ .../es-module/test-esm-loader-invalid-url.mjs | 12 ++ .../loader-invalid-format.mjs | 8 + .../es-module-loaders/loader-invalid-url.mjs | 9 + 8 files changed, 219 insertions(+), 75 deletions(-) create mode 100644 test/es-module/test-esm-loader-invalid-format.mjs create mode 100644 test/es-module/test-esm-loader-invalid-url.mjs create mode 100644 test/fixtures/es-module-loaders/loader-invalid-format.mjs create mode 100644 test/fixtures/es-module-loaders/loader-invalid-url.mjs diff --git a/doc/api/errors.md b/doc/api/errors.md index c0b2564e2b0c6e..57f64367c1d3eb 100644 --- a/doc/api/errors.md +++ b/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. + +### 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. + + +### 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. + ### 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. diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 24e1f34b25d96a..be37c96d018704 100644 --- a/lib/internal/errors.js +++ b/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) { @@ -689,7 +703,7 @@ E('ERR_INVALID_RETURN_VALUE', (input, name, value) => { type = `type ${typeof value}`; } return `Expected ${input} to be returned from the "${name}"` + - ` function but got ${type}.`; + ` function but got ${type}.`; }, TypeError); E('ERR_INVALID_SYNC_FORK_INPUT', 'Asynchronous forks do not support Buffer, Uint8Array or string input: %s', diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index b3023fcd4f8e36..2fa10c7eab9b44 100644 --- a/lib/internal/modules/esm/loader.js +++ b/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 } = - await this._resolve(specifier, parentURL, defaultResolve); + const resolved = 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 }; } diff --git a/test/common/index.mjs b/test/common/index.mjs index b7ac556bca52c3..f73a9d9be5c08e 100644 --- a/test/common/index.mjs +++ b/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 = [ - Buffer, - clearImmediate, - clearInterval, - clearTimeout, - global, - process, - setImmediate, - setInterval, - setTimeout -]; - -if (process.env.NODE_TEST_KNOWN_GLOBALS) { - const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(','); - allowGlobals(...knownFromEnv); -} - -export function allowGlobals(...whitelist) { - knownGlobals = knownGlobals.concat(whitelist); -} - -export function leakedGlobals() { - // Add possible expected globals - if (global.gc) { - knownGlobals.push(global.gc); - } - - if (global.DTRACE_HTTP_SERVER_RESPONSE) { - knownGlobals.push(DTRACE_HTTP_SERVER_RESPONSE); - knownGlobals.push(DTRACE_HTTP_SERVER_REQUEST); - knownGlobals.push(DTRACE_HTTP_CLIENT_RESPONSE); - knownGlobals.push(DTRACE_HTTP_CLIENT_REQUEST); - knownGlobals.push(DTRACE_NET_STREAM_END); - knownGlobals.push(DTRACE_NET_SERVER_CONNECTION); - } - - if (global.COUNTER_NET_SERVER_CONNECTION) { - knownGlobals.push(COUNTER_NET_SERVER_CONNECTION); - knownGlobals.push(COUNTER_NET_SERVER_CONNECTION_CLOSE); - knownGlobals.push(COUNTER_HTTP_SERVER_REQUEST); - knownGlobals.push(COUNTER_HTTP_SERVER_RESPONSE); - knownGlobals.push(COUNTER_HTTP_CLIENT_REQUEST); - knownGlobals.push(COUNTER_HTTP_CLIENT_RESPONSE); - } - - const leaked = []; - - for (const val in global) { - if (!knownGlobals.includes(global[val])) { - leaked.push(val); - } - } - - if (global.__coverage__) { - return leaked.filter((varname) => !/^(?:cov_|__cov)/.test(varname)); - } else { - return leaked; - } -} - -process.on('exit', function() { - const leaked = leakedGlobals(); - if (leaked.length > 0) { - assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`); - } -}); +export { + 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 +}; diff --git a/test/es-module/test-esm-loader-invalid-format.mjs b/test/es-module/test-esm-loader-invalid-format.mjs new file mode 100644 index 00000000000000..88b128affd528f --- /dev/null +++ b/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()); diff --git a/test/es-module/test-esm-loader-invalid-url.mjs b/test/es-module/test-esm-loader-invalid-url.mjs new file mode 100644 index 00000000000000..43971a2e6e3b71 --- /dev/null +++ b/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()); diff --git a/test/fixtures/es-module-loaders/loader-invalid-format.mjs b/test/fixtures/es-module-loaders/loader-invalid-format.mjs new file mode 100644 index 00000000000000..17a0dcd04daad9 --- /dev/null +++ b/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); +} diff --git a/test/fixtures/es-module-loaders/loader-invalid-url.mjs b/test/fixtures/es-module-loaders/loader-invalid-url.mjs new file mode 100644 index 00000000000000..12efbb5021a4bc --- /dev/null +++ b/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); +}