Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: Ensure custom loader resolved "url" is properly validated #21352

Closed
wants to merge 8 commits into from
Closed

esm: Ensure custom loader resolved "url" is properly validated #21352

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bc83382
refactor common
guybedford Jun 18, 2018
cf1e32f
esm: validate custom loader url
guybedford Jun 15, 2018
6594462
braces nit
guybedford Jun 15, 2018
1776edf
remove unnecessary try wrapper
guybedford Jun 15, 2018
888d186
beautiful error messages
guybedford Jun 17, 2018
233cf2a
error handling feedback
guybedford Jun 18, 2018
d434368
test case refinements
guybedford Jun 18, 2018
a235f08
format case test and fix
guybedford Jun 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1204,6 +1204,17 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name here might be better as: ERR_INVALID_RETURN_PROPERTY_TYPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the consistent name here would be ERR_INVALID_RETURN_PROPERTY_VALUE actually :)


Thrown in case a function option does not return an expected property type.

<a id="ERR_INVALID_RETURN_PROPERTY_STRING"></a>
### ERR_INVALID_RETURN_PROPERTY_STRING

Thrown in case a function option does not return an expected string property
type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why strings got special handled while other types not. I recommend to change this to: ERR_INVALID_RETURN_PROPERTY_VALUE.
The current description is definitely wrong as the usage is not about the returned type but about the returned value (at least when looking at the actual implementation).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the semantics, but the thing is ideally the current ERR_INVALID_RETURN_VALUE error should then be ERR_INVALID_RETURN_TYPE as that is what it is really checking. So I was trying to remain consistent with that. I think ERR_INVALID_RETURN_PROPERTY_VALUE isn't suitable for this reason. type in the description is referring to "types of strings" as in categories / enums.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the ERR_INVALID_RETURN_VALUE could be improved. However the return checks are not common and could be consolidated to always be a ERR_INVALID_RETURN_VALUE, just with a more specific error message. IMHO differentiating the type error from the value error is not useful for users.

That all aside: I am definitely against having a error code that is specific for property strings. So at least that has to be more generalized out of my perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO differentiating the type error from the value error is not useful for users.

See my example response in the last comment for why I definitely feel this is a useful distinction in the error message.

In terms of naming, I've switched them around to use ERR_INVALID_RETURN_PROPERTY_VALUE to be consistent with return value, then made the new one just ERR_INVALID_RETURN_PROPERTY.


<a id="ERR_INVALID_RETURN_VALUE"></a>
### ERR_INVALID_RETURN_VALUE

Expand Down
16 changes: 15 additions & 1 deletion lib/internal/errors.js
View file
Open in desktop
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) => {
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_PROPERTY_STRING', (input, name, prop, value) => {
return `Expected a valid ${input} to be returned for the ${prop} from the ` +
`"${name}" function but got ${value}.`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: If this error code should still be kept, I suggest to change it to a string by reordering the arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is quite a complex arguments case, I think it is important to have the labelled arguments and also the order is important to the message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already thought so but all other errors work like that and it should still be relatively clear what it is about.

}, 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
View file
Open in desktop
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_STRING,
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(
'string', 'loader resolve', 'url', url
);

if (typeof format !== 'string')
throw new ERR_INVALID_ARG_TYPE('format', 'string', format);
throw new ERR_INVALID_RETURN_PROPERTY_STRING(
'string', 'loader resolve', 'format', format
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error should likely be ERR_INVALID_RETURN_PROPERTY just as the error above. Or I fail to see why this is handled differently than the url error case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first error checks that it is a string, the second error checks that it is the right type of string.

The messages are:

  1. Expected string to be returned for the url from the "loader resolve" function but got ${type}.
  2. Expected a valid url to be returned for the url from the "loader resolve" function but got ${value}.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to see the second case being related to this error. It is about format?

Copy link
Contributor Author

@guybedford guybedford Jun 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I missed this was specifically the format case, but the same argument applied - format must be one of a specific set of string values, so the error message will say:

Expected a valid format to be returned for the format from the "loader resolve" function but got ${value}

instead of

Expected string to be returned for the format from the "loader resolve" function but got string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not certain why typeof format !== 'string' is an ERR_INVALID_RETURN_PROPERTY while typeof url !== 'string' is an ERR_INVALID_RETURN_PROPERTY_VALUE. The validation step seems the same so it is somewhat weird for me that it results in different errors.


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

if (this._resolve !== defaultResolve) {
try {
new URL(url);
} catch (e) {
throw new ERR_INVALID_RETURN_PROPERTY_STRING(
'url', 'loader resolve', 'url', url
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... I'm not sure what the benefit of the try/catch and additional throw here. If the url is invalid, new URL(url) will throw and accomplish the same thing, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well caught, yes that's simpler. Updated.

}

if (format !== 'dynamic' && !url.startsWith('file:'))
throw new ERR_INVALID_PROTOCOL(url, 'file:');
throw new ERR_INVALID_RETURN_PROPERTY_STRING(
'file: url', 'loader resolve', 'url', url
);

return { url, format };
}
Expand Down
186 changes: 119 additions & 67 deletions test/common/index.mjs
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be sufficient to just export common, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can only do this once we have named exports support for CommonJS modules.


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-url.mjs
View file
Open in desktop
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-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_STRING',
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());
9 changes: 9 additions & 0 deletions test/fixtures/es-module-loaders/loader-invalid-url.mjs
View file
Open in desktop
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);
}