Skip to content

Commit 29299cc

Browse files
guybedfordtargos
guybedford
authored and
targos
committed
esm: loader hook URL validation and error messages
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>
1 parent 91384bf commit 29299cc

File tree

8 files changed

+219
-75
lines changed

8 files changed

+219
-75
lines changed

‎doc/api/errors.md

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1219,10 +1219,23 @@ An invalid `options.protocol` was passed.
12191219
Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which
12201220
is not supported.
12211221

1222+
<a id="ERR_INVALID_RETURN_PROPERTY"></a>
1223+
### ERR_INVALID_RETURN_PROPERTY
1224+
1225+
Thrown in case a function option does not provide a valid value for one of its
1226+
returned object properties on execution.
1227+
1228+
<a id="ERR_INVALID_RETURN_PROPERTY_VALUE"></a>
1229+
### ERR_INVALID_RETURN_PROPERTY_VALUE
1230+
1231+
Thrown in case a function option does not provide an expected value
1232+
type for one of its returned object properties on execution.
1233+
12221234
<a id="ERR_INVALID_RETURN_VALUE"></a>
12231235
### ERR_INVALID_RETURN_VALUE
12241236

1225-
Thrown in case a function option does not return an expected value on execution.
1237+
Thrown in case a function option does not return an expected value
1238+
type on execution.
12261239
For example when a function is expected to return a promise.
12271240

12281241
<a id="ERR_INVALID_SYNC_FORK_INPUT"></a>

‎lib/internal/errors.js

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -689,6 +689,20 @@ E('ERR_INVALID_PROTOCOL',
689689
TypeError);
690690
E('ERR_INVALID_REPL_EVAL_CONFIG',
691691
'Cannot specify both "breakEvalOnSigint" and "eval" for REPL', TypeError);
692+
E('ERR_INVALID_RETURN_PROPERTY', (input, name, prop, value) => {
693+
return `Expected a valid ${input} to be returned for the "${prop}" from the` +
694+
` "${name}" function but got ${value}.`;
695+
}, TypeError);
696+
E('ERR_INVALID_RETURN_PROPERTY_VALUE', (input, name, prop, value) => {
697+
let type;
698+
if (value && value.constructor && value.constructor.name) {
699+
type = `instance of ${value.constructor.name}`;
700+
} else {
701+
type = `type ${typeof value}`;
702+
}
703+
return `Expected ${input} to be returned for the "${prop}" from the` +
704+
` "${name}" function but got ${type}.`;
705+
}, TypeError);
692706
E('ERR_INVALID_RETURN_VALUE', (input, name, value) => {
693707
let type;
694708
if (value && value.constructor && value.constructor.name) {
@@ -697,7 +711,7 @@ E('ERR_INVALID_RETURN_VALUE', (input, name, value) => {
697711
type = `type ${typeof value}`;
698712
}
699713
return `Expected ${input} to be returned from the "${name}"` +
700-
` function but got ${type}.`;
714+
` function but got ${type}.`;
701715
}, TypeError);
702716
E('ERR_INVALID_SYNC_FORK_INPUT',
703717
'Asynchronous forks do not support Buffer, Uint8Array or string input: %s',

‎lib/internal/modules/esm/loader.js

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,13 @@
22

33
const {
44
ERR_INVALID_ARG_TYPE,
5-
ERR_INVALID_PROTOCOL,
5+
ERR_INVALID_RETURN_PROPERTY,
6+
ERR_INVALID_RETURN_PROPERTY_VALUE,
7+
ERR_INVALID_RETURN_VALUE,
68
ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK,
79
ERR_UNKNOWN_MODULE_FORMAT
810
} = require('internal/errors').codes;
11+
const { URL } = require('url');
912
const ModuleMap = require('internal/modules/esm/module_map');
1013
const ModuleJob = require('internal/modules/esm/module_job');
1114
const defaultResolve = require('internal/modules/esm/default_resolve');
@@ -52,20 +55,42 @@ class Loader {
5255
if (!isMain && typeof parentURL !== 'string')
5356
throw new ERR_INVALID_ARG_TYPE('parentURL', 'string', parentURL);
5457

55-
const { url, format } =
56-
await this._resolve(specifier, parentURL, defaultResolve);
58+
const resolved = await this._resolve(specifier, parentURL, defaultResolve);
59+
60+
if (typeof resolved !== 'object')
61+
throw new ERR_INVALID_RETURN_VALUE(
62+
'object', 'loader resolve', resolved
63+
);
64+
65+
const { url, format } = resolved;
5766

5867
if (typeof url !== 'string')
59-
throw new ERR_INVALID_ARG_TYPE('url', 'string', url);
68+
throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
69+
'string', 'loader resolve', 'url', url
70+
);
6071

6172
if (typeof format !== 'string')
62-
throw new ERR_INVALID_ARG_TYPE('format', 'string', format);
73+
throw new ERR_INVALID_RETURN_PROPERTY_VALUE(
74+
'string', 'loader resolve', 'format', format
75+
);
6376

6477
if (format === 'builtin')
6578
return { url: `node:${url}`, format };
6679

80+
if (this._resolve !== defaultResolve) {
81+
try {
82+
new URL(url);
83+
} catch (e) {
84+
throw new ERR_INVALID_RETURN_PROPERTY(
85+
'url', 'loader resolve', 'url', url
86+
);
87+
}
88+
}
89+
6790
if (format !== 'dynamic' && !url.startsWith('file:'))
68-
throw new ERR_INVALID_PROTOCOL(url, 'file:');
91+
throw new ERR_INVALID_RETURN_PROPERTY(
92+
'file: url', 'loader resolve', 'url', url
93+
);
6994

7095
return { url, format };
7196
}

‎test/common/index.mjs

Lines changed: 119 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1,71 +1,123 @@
11
// Flags: --experimental-modules
22
/* eslint-disable node-core/required-modules */
3+
import common from './index.js';
34

4-
import assert from 'assert';
5+
const {
6+
PORT,
7+
isMainThread,
8+
isWindows,
9+
isWOW64,
10+
isAIX,
11+
isLinuxPPCBE,
12+
isSunOS,
13+
isFreeBSD,
14+
isOpenBSD,
15+
isLinux,
16+
isOSX,
17+
isGlibc,
18+
enoughTestMem,
19+
enoughTestCpu,
20+
rootDir,
21+
buildType,
22+
localIPv6Hosts,
23+
opensslCli,
24+
PIPE,
25+
hasIPv6,
26+
childShouldThrowAndAbort,
27+
ddCommand,
28+
spawnPwd,
29+
spawnSyncPwd,
30+
platformTimeout,
31+
allowGlobals,
32+
leakedGlobals,
33+
mustCall,
34+
mustCallAtLeast,
35+
mustCallAsync,
36+
hasMultiLocalhost,
37+
fileExists,
38+
skipIfEslintMissing,
39+
canCreateSymLink,
40+
getCallSite,
41+
mustNotCall,
42+
printSkipMessage,
43+
skip,
44+
ArrayStream,
45+
nodeProcessAborted,
46+
busyLoop,
47+
isAlive,
48+
noWarnCode,
49+
expectWarning,
50+
expectsError,
51+
skipIfInspectorDisabled,
52+
skipIf32Bits,
53+
getArrayBufferViews,
54+
getBufferSources,
55+
crashOnUnhandledRejection,
56+
getTTYfd,
57+
runWithInvalidFD,
58+
hijackStdout,
59+
hijackStderr,
60+
restoreStdout,
61+
restoreStderr,
62+
isCPPSymbolsNotMapped
63+
} = common;
564

6-
let knownGlobals = [
7-
Buffer,
8-
clearImmediate,
9-
clearInterval,
10-
clearTimeout,
11-
global,
12-
process,
13-
setImmediate,
14-
setInterval,
15-
setTimeout
16-
];
17-
18-
if (process.env.NODE_TEST_KNOWN_GLOBALS) {
19-
const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
20-
allowGlobals(...knownFromEnv);
21-
}
22-
23-
export function allowGlobals(...whitelist) {
24-
knownGlobals = knownGlobals.concat(whitelist);
25-
}
26-
27-
export function leakedGlobals() {
28-
// Add possible expected globals
29-
if (global.gc) {
30-
knownGlobals.push(global.gc);
31-
}
32-
33-
if (global.DTRACE_HTTP_SERVER_RESPONSE) {
34-
knownGlobals.push(DTRACE_HTTP_SERVER_RESPONSE);
35-
knownGlobals.push(DTRACE_HTTP_SERVER_REQUEST);
36-
knownGlobals.push(DTRACE_HTTP_CLIENT_RESPONSE);
37-
knownGlobals.push(DTRACE_HTTP_CLIENT_REQUEST);
38-
knownGlobals.push(DTRACE_NET_STREAM_END);
39-
knownGlobals.push(DTRACE_NET_SERVER_CONNECTION);
40-
}
41-
42-
if (global.COUNTER_NET_SERVER_CONNECTION) {
43-
knownGlobals.push(COUNTER_NET_SERVER_CONNECTION);
44-
knownGlobals.push(COUNTER_NET_SERVER_CONNECTION_CLOSE);
45-
knownGlobals.push(COUNTER_HTTP_SERVER_REQUEST);
46-
knownGlobals.push(COUNTER_HTTP_SERVER_RESPONSE);
47-
knownGlobals.push(COUNTER_HTTP_CLIENT_REQUEST);
48-
knownGlobals.push(COUNTER_HTTP_CLIENT_RESPONSE);
49-
}
50-
51-
const leaked = [];
52-
53-
for (const val in global) {
54-
if (!knownGlobals.includes(global[val])) {
55-
leaked.push(val);
56-
}
57-
}
58-
59-
if (global.__coverage__) {
60-
return leaked.filter((varname) => !/^(?:cov_|__cov)/.test(varname));
61-
} else {
62-
return leaked;
63-
}
64-
}
65-
66-
process.on('exit', function() {
67-
const leaked = leakedGlobals();
68-
if (leaked.length > 0) {
69-
assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
70-
}
71-
});
65+
export {
66+
PORT,
67+
isMainThread,
68+
isWindows,
69+
isWOW64,
70+
isAIX,
71+
isLinuxPPCBE,
72+
isSunOS,
73+
isFreeBSD,
74+
isOpenBSD,
75+
isLinux,
76+
isOSX,
77+
isGlibc,
78+
enoughTestMem,
79+
enoughTestCpu,
80+
rootDir,
81+
buildType,
82+
localIPv6Hosts,
83+
opensslCli,
84+
PIPE,
85+
hasIPv6,
86+
childShouldThrowAndAbort,
87+
ddCommand,
88+
spawnPwd,
89+
spawnSyncPwd,
90+
platformTimeout,
91+
allowGlobals,
92+
leakedGlobals,
93+
mustCall,
94+
mustCallAtLeast,
95+
mustCallAsync,
96+
hasMultiLocalhost,
97+
fileExists,
98+
skipIfEslintMissing,
99+
canCreateSymLink,
100+
getCallSite,
101+
mustNotCall,
102+
printSkipMessage,
103+
skip,
104+
ArrayStream,
105+
nodeProcessAborted,
106+
busyLoop,
107+
isAlive,
108+
noWarnCode,
109+
expectWarning,
110+
expectsError,
111+
skipIfInspectorDisabled,
112+
skipIf32Bits,
113+
getArrayBufferViews,
114+
getBufferSources,
115+
crashOnUnhandledRejection,
116+
getTTYfd,
117+
runWithInvalidFD,
118+
hijackStdout,
119+
hijackStderr,
120+
restoreStdout,
121+
restoreStderr,
122+
isCPPSymbolsNotMapped
123+
};

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-format.mjs
2+
import { expectsError, mustCall } from '../common';
3+
import assert from 'assert';
4+
5+
import('../fixtures/es-modules/test-esm-ok.mjs')
6+
.then(assert.fail, expectsError({
7+
code: 'ERR_INVALID_RETURN_PROPERTY',
8+
message: 'Expected string to be returned for the "url" from the ' +
9+
'"loader resolve" function but got "undefined"'
10+
}))
11+
.then(mustCall());

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs
2+
import { expectsError, mustCall } from '../common';
3+
import assert from 'assert';
4+
5+
import('../fixtures/es-modules/test-esm-ok.mjs')
6+
.then(assert.fail, expectsError({
7+
code: 'ERR_INVALID_RETURN_PROPERTY',
8+
message: 'Expected a valid url to be returned for the "url" from the ' +
9+
'"loader resolve" function but got ' +
10+
'../fixtures/es-modules/test-esm-ok.mjs.'
11+
}))
12+
.then(mustCall());

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
export async function resolve(specifier, parentModuleURL, defaultResolve) {
2+
if (parentModuleURL && specifier === '../fixtures/es-modules/test-esm-ok.mjs') {
3+
return {
4+
url: 'file:///asdf'
5+
};
6+
}
7+
return defaultResolve(specifier, parentModuleURL);
8+
}

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
export async function resolve(specifier, parentModuleURL, defaultResolve) {
2+
if (parentModuleURL && specifier === '../fixtures/es-modules/test-esm-ok.mjs') {
3+
return {
4+
url: specifier,
5+
format: 'esm'
6+
};
7+
}
8+
return defaultResolve(specifier, parentModuleURL);
9+
}

0 commit comments

Comments
 (0)