Skip to content

Commit

Permalink
esm: loader hook URL validation and error messages
Browse files Browse the repository at this point in the history
PR-URL: #21352
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
  • Loading branch information
guybedford committed Jun 30, 2018
1 parent 81f06ba commit 1bf42f4
Show file tree
Hide file tree
Showing 8 changed files with 219 additions and 75 deletions.
15 changes: 14 additions & 1 deletion doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
16 changes: 15 additions & 1 deletion lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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',
Expand Down
37 changes: 31 additions & 6 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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 };
}
Expand Down
186 changes: 119 additions & 67 deletions test/common/index.mjs
Original file line number Diff line number Diff line change
@@ -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
};
11 changes: 11 additions & 0 deletions test/es-module/test-esm-loader-invalid-format.mjs
Original file line number Diff line number Diff line change
@@ -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());
12 changes: 12 additions & 0 deletions test/es-module/test-esm-loader-invalid-url.mjs
Original file line number Diff line number Diff line change
@@ -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());
8 changes: 8 additions & 0 deletions test/fixtures/es-module-loaders/loader-invalid-format.mjs
Original file line number Diff line number Diff line change
@@ -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);
}
9 changes: 9 additions & 0 deletions test/fixtures/es-module-loaders/loader-invalid-url.mjs
Original file line number Diff line number Diff line change
@@ -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);
}

0 comments on commit 1bf42f4

Please sign in to comment.