Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 29299cc

Browse filesBrowse 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
Copy full SHA for 29299cc

File tree

Expand file treeCollapse file tree

8 files changed

+219
-75
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

8 files changed

+219
-75
lines changed
Open diff view settings
Collapse file

‎doc/api/errors.md‎

Copy file name to clipboardExpand all lines: doc/api/errors.md
+14-1Lines changed: 14 additions & 1 deletion
  • Display the source diff
  • Display the rich diff
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>
Collapse file

‎lib/internal/errors.js‎

Copy file name to clipboardExpand all lines: lib/internal/errors.js
+15-1Lines 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',
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js
+31-6Lines 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
}
Collapse file

‎test/common/index.mjs‎

Copy file name to clipboard
+119-67Lines 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+
};
Collapse file

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

Copy file name to clipboard
+11Lines 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());
Collapse file

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

Copy file name to clipboard
+12Lines 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());
Collapse file

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

Copy file name to clipboard
+8Lines 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+
}
Collapse file

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

Copy file name to clipboard
+9Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.