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

Jump to bottom

esm: improve fetch_module test coverage and remove hack #41947

Merged
merged 1 commit into from Feb 19, 2022
Merged

esm: improve fetch_module test coverage and remove hack #41947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 1 addition & 8 deletions lib/internal/modules/esm/fetch_module.js
View file
Open in desktop
Expand Up @@ -20,7 +20,6 @@ const {
} = require('internal/errors').codes;
const { URL } = require('internal/url');
const net = require('net');
const path = require('path');

/**
* @typedef CacheEntry
Expand Down Expand Up @@ -263,15 +262,9 @@ function fetchModule(parsed, { parentURL }) {
if (parsed.protocol === 'http:') {
return PromisePrototypeThen(isLocalAddress(parsed.hostname), (is) => {
if (is !== true) {
let parent = parentURL;
const parentName = path.basename(parent.pathname);
if (
parentName === '[eval]' ||
parentName === '[stdin]'
) parent = 'command-line';
throw new ERR_NETWORK_IMPORT_DISALLOWED(
href,
parent,
parentURL,
Comment on lines -266 to +267
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I added this was because the output without it seemed likely to confuse (the file path is irrelevant and makes it took like it's coming from a file, but it's not). Aside from consistency with CJS, I think this is not an improvement (I don't feel strongly though).

Copy link
Contributor Author

@aduh95 aduh95 Feb 13, 2022 •
edited

Choose a reason for hiding this comment

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

I added this was because the output without it seemed likely to confuse (the file path is irrelevant and makes it took like it's coming from a file, but it's not)

If the URL is confusing, we should fix the URL itself, not try to hide it in this specific error message imo. I disagree that command-line is less confusing – what if the user didn't start the process via the command-line?

I think this is not an improvement

Note that the condition can never be true: parentName is never equals to [eval] nor [stdin] (it could be [eval1] though, but we're not testing that).

Copy link
Contributor

Choose a reason for hiding this comment

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

If the URL is confusing, we should fix the URL itself, not try to hide it in this specific error message imo.

Sure! That works for me (and should also better support consistency).

what if the user didn't start the process via the command-line?

Is that possible?

Note that the condition can never be true: parentName is never equals to [eval] nor [stdin] (it could be [eval1] though, but we're not testing that).

If it doesn't now, it did at the time—I specifically tested CLI via -e …, which is how I found this in the first place 🙂 Is there a known related change that would explain why it's not now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that possible?

Sure, why not? I don't have a deep enough knowledge of the various OSs Node.js supports to list them all, but I know that CLI is just an option among others (from the GUI, as a service, from a parent process, etc.).

If it doesn't now, it did at the time—I specifically tested CLI via -e …, which is how I found this in the first place 🙂 Is there a known related change that would explain why it's not now?

Maybe you forgot to add --input-type=module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that possible?

Sure, why not? I don't have a deep enough knowledge of the various OSs Node.js supports to list them all, but I know that CLI is just an option among others (from the GUI, as a service, from a parent process, etc.).

No, I mean, is there a route that exists that leads to [eval*]/[stdin] that did not originate from CLI?

If it doesn't now, it did at the time—I specifically tested CLI via -e …, which is how I found this in the first place 🙂 Is there a known related change that would explain why it's not now?

Maybe you forgot to add --input-type=module?

I did not 🙂

'http can only be used to load local resources (use https instead).'
);
}
Expand Down
48 changes: 48 additions & 0 deletions test/es-module/test-http-imports-cli.mjs
View file
Open in desktop
@@ -0,0 +1,48 @@
import { mustCall } from '../common/index.mjs';
import { match, notStrictEqual } from 'assert';
import { spawn } from 'child_process';
import { execPath } from 'process';

{
const child = spawn(execPath, [
'--experimental-network-imports',
'--input-type=module',
'-e',
'import "http://example.com"',
]);

let stderr = '';
child.stderr.setEncoding('utf8');
child.stderr.on('data', (data) => {
stderr += data;
});
child.on('close', mustCall((code, _signal) => {
notStrictEqual(code, 0);

// [ERR_NETWORK_IMPORT_DISALLOWED]: import of 'http://example.com/' by
// …/[eval1] is not supported: http can only be used to load local
// resources (use https instead).
match(stderr, /[ERR_NETWORK_IMPORT_DISALLOWED]/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I prefer the err.code & err.message.includes() used in other ESM error tests. I think consistency would be favourable.

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 don't have access to err.code not err.message, we're reading the stderr of a subprocess. FYI this test was inspired from

node/test/es-module/test-esm-import-json-named-export.mjs

Lines 7 to 24 in 15f1a45

const child = spawn(execPath, [
path('es-modules', 'import-json-named-export.mjs'),
]);
let stderr = '';
child.stderr.setEncoding('utf8');
child.stderr.on('data', (data) => {
stderr += data;
});
child.on('close', mustCall((code, _signal) => {
notStrictEqual(code, 0);
// SyntaxError: The requested module '../experimental.json'
// does not provide an export named 'ofLife'
match(stderr, /SyntaxError:/);
match(stderr, /'\.\.\/experimental\.json'/);
match(stderr, /'ofLife'/);
}));

Copy link
Contributor

Choose a reason for hiding this comment

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

gah, yes indeed (and aligns with #41352)

}));
}
{
const child = spawn(execPath, [
'--experimental-network-imports',
'--input-type=module',
]);
child.stdin.end('import "http://example.com"');

let stderr = '';
child.stderr.setEncoding('utf8');
child.stderr.on('data', (data) => {
stderr += data;
});
child.on('close', mustCall((code, _signal) => {
notStrictEqual(code, 0);

// [ERR_NETWORK_IMPORT_DISALLOWED]: import of 'http://example.com/' by
// …/[stdin] is not supported: http can only be used to load local
// resources (use https instead).
match(stderr, /[ERR_NETWORK_IMPORT_DISALLOWED]/);
}));
}
11 changes: 6 additions & 5 deletions test/es-module/test-http-imports.mjs
View file
Open in desktop
Expand Up @@ -114,7 +114,7 @@ for (const { protocol, createServer } of [
}));
await assert.rejects(
import(crossProtocolRedirect.href),
'should not be able to redirect across protocols'
{ code: 'ERR_NETWORK_IMPORT_DISALLOWED' }
);

const deps = new URL(url.href);
Expand All @@ -135,7 +135,8 @@ for (const { protocol, createServer } of [
export default 1;`);
await assert.rejects(
import(fileDep.href),
'should not be able to load file: from http:');
{ code: 'ERR_INVALID_URL_SCHEME' }
);

const builtinDep = new URL(url.href);
builtinDep.searchParams.set('body', `
Expand All @@ -144,7 +145,7 @@ for (const { protocol, createServer } of [
`);
await assert.rejects(
import(builtinDep.href),
'should not be able to load node: from http:'
{ code: 'ERR_INVALID_URL_SCHEME' }
);

const unprefixedBuiltinDep = new URL(url.href);
Expand All @@ -154,15 +155,15 @@ for (const { protocol, createServer } of [
`);
await assert.rejects(
import(unprefixedBuiltinDep.href),
'should not be able to load unprefixed builtins from http:'
{ code: 'ERR_INVALID_URL_SCHEME' }
);

const unsupportedMIME = new URL(url.href);
unsupportedMIME.searchParams.set('mime', 'application/node');
unsupportedMIME.searchParams.set('body', '');
await assert.rejects(
import(unsupportedMIME.href),
'should not be able to load unsupported MIMEs from http:'
{ code: 'ERR_UNKNOWN_MODULE_FORMAT' }
);

server.close();
Expand Down