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

8 files changed

Lines changed: 219 additions & 75 deletions

File tree

‎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+
<aid="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+
<aid="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
<aid="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
<aid="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+
lettype;
698+
if(value&&value.constructor&&value.constructor.name){
699+
type=`instance of ${value.constructor.name}`;
700+
}else{
701+
type=`type ${typeofvalue}`;
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
lettype;
694708
if(value&&value.constructor&&value.constructor.name){
@@ -697,7 +711,7 @@ E('ERR_INVALID_RETURN_VALUE', (input, name, value) => {
697711
type=`type ${typeofvalue}`;
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
constModuleMap=require('internal/modules/esm/module_map');
1013
constModuleJob=require('internal/modules/esm/module_job');
1114
constdefaultResolve=require('internal/modules/esm/default_resolve');
@@ -52,20 +55,42 @@ class Loader {
5255
if(!isMain&&typeofparentURL!=='string')
5356
thrownewERR_INVALID_ARG_TYPE('parentURL','string',parentURL);
5457

55-
const{ url, format }=
56-
awaitthis._resolve(specifier,parentURL,defaultResolve);
58+
constresolved=awaitthis._resolve(specifier,parentURL,defaultResolve);
59+
60+
if(typeofresolved!=='object')
61+
thrownewERR_INVALID_RETURN_VALUE(
62+
'object','loader resolve',resolved
63+
);
64+
65+
const{ url, format }=resolved;
5766

5867
if(typeofurl!=='string')
59-
thrownewERR_INVALID_ARG_TYPE('url','string',url);
68+
thrownewERR_INVALID_RETURN_PROPERTY_VALUE(
69+
'string','loader resolve','url',url
70+
);
6071

6172
if(typeofformat!=='string')
62-
thrownewERR_INVALID_ARG_TYPE('format','string',format);
73+
thrownewERR_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+
newURL(url);
83+
}catch(e){
84+
thrownewERR_INVALID_RETURN_PROPERTY(
85+
'url','loader resolve','url',url
86+
);
87+
}
88+
}
89+
6790
if(format!=='dynamic'&&!url.startsWith('file:'))
68-
thrownewERR_INVALID_PROTOCOL(url,'file:');
91+
thrownewERR_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+
importcommonfrom'./index.js';
34

4-
importassertfrom'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-
letknownGlobals=[
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-
constknownFromEnv=process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
20-
allowGlobals(...knownFromEnv);
21-
}
22-
23-
exportfunctionallowGlobals(...whitelist){
24-
knownGlobals=knownGlobals.concat(whitelist);
25-
}
26-
27-
exportfunctionleakedGlobals(){
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-
constleaked=[];
52-
53-
for(constvalinglobal){
54-
if(!knownGlobals.includes(global[val])){
55-
leaked.push(val);
56-
}
57-
}
58-
59-
if(global.__coverage__){
60-
returnleaked.filter((varname)=>!/^(?:cov_|__cov)/.test(varname));
61-
}else{
62-
returnleaked;
63-
}
64-
}
65-
66-
process.on('exit',function(){
67-
constleaked=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+
importassertfrom'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+
importassertfrom'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+
exportasyncfunctionresolve(specifier,parentModuleURL,defaultResolve){
2+
if(parentModuleURL&&specifier==='../fixtures/es-modules/test-esm-ok.mjs'){
3+
return{
4+
url: 'file:///asdf'
5+
};
6+
}
7+
returndefaultResolve(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+
exportasyncfunctionresolve(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+
returndefaultResolve(specifier,parentModuleURL);
9+
}

0 commit comments

Comments
 (0)