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: treat 307 and 308 as redirects in HTTPS imports #43689

Merged
merged 4 commits into from
Jul 9, 2022
Merged

esm: treat 307 and 308 as redirects in HTTPS imports #43689

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52aaed3
esm: treat `307` and `308` as redirects in HTTPS imports
kidonng Jul 4, 2022
54ee622
Split `isRedirect` into its own function per review
kidonng Jul 6, 2022
542b5cf
Apply review suggestions
kidonng Jul 6, 2022
9ba00b1
Use a switch
kidonng Jul 7, 2022
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
27 changes: 24 additions & 3 deletions lib/internal/modules/esm/fetch_module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,28 @@ function createUnzip() {
return createUnzip();
}

/**
* Redirection status code as per section 6.4 of RFC 7231:
* https://datatracker.ietf.org/doc/html/rfc7231#section-6.4
* and RFC 7238:
* https://datatracker.ietf.org/doc/html/rfc7238
* @param {number} statusCode
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {number} statusCode
* See also https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
* @param {number} statusCode

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {number} statusCode
* @param {number} statusCode
* @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

* @returns {boolean}
*/
function isRedirect(statusCode) {
switch (statusCode) {
case 300: // Multiple Choices
case 301: // Moved Permanently
case 302: // Found
case 303: // See Other
case 307: // Temporary Redirect
Copy link
Member

@GeoffreyBooth GeoffreyBooth Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should include a comment noting that leaving out 304 is a deliberate choice.

Suggested change
case 307: // Temporary Redirect
// Skipping 304: Not Modified
case 307: // Temporary Redirect

I’m also fine with this being a SafeSet defined outside of the function if that seems more readable to everyone. I don’t have a preference between one or the other.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for the comment about explicitly skipping 304 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

-1 for the comment about 304 because the next steps are commenting 305 and 306.

I agree with author's #43689 (comment) that http codes are well-known and there already is a link. I don't think extra verbosity is useful here.

Copy link
Member

@GeoffreyBooth GeoffreyBooth Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

304 is commonly used whereas 305 and 306 basically don't exist. I think it's worth calling out that we intended to treat 304 differently, so that no one comes by later and opens a PR to add it here—like this PR is doing. It's not about explaining what 304 is, but that this code intentionally excludes it from the "treat as redirect" set.

Copy link
Member

@JakobJingleheimer JakobJingleheimer Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @GeoffreyBooth that it's worth noting. I am the primary maintainer and I know my future self would appreciate the comment.

I'll add it in a follow-up PR and we can debate it there (along with a bugfix for the wrong error type he found in the original implementation).

Copy link
Member

Choose a reason for hiding this comment

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

CI hasn’t run yet for this PR, I think there’s no reason to wait for a follow-up to add a comment.

case 308: // Permanent Redirect
return true;
default:
return false;
}
}

/**
* @param {URL} parsed
* @returns {Promise<CacheEntry> | CacheEntry}
Expand All @@ -107,9 +129,8 @@ function fetchWithRedirects(parsed) {
// `finally` on network error/timeout.
const { 0: res } = await once(req, 'response');
try {
const isRedirect = res.statusCode >= 300 && res.statusCode <= 303;
const hasLocation = ObjectPrototypeHasOwnProperty(res.headers, 'location');
if (isRedirect && hasLocation) {
if (isRedirect(res.statusCode) && hasLocation) {
const location = new URL(res.headers.location, parsed);
if (location.protocol !== 'http:' && location.protocol !== 'https:') {
throw new ERR_NETWORK_IMPORT_DISALLOWED(
Expand All @@ -127,7 +148,7 @@ function fetchWithRedirects(parsed) {
err.message = `Cannot find module '${parsed.href}', HTTP 404`;
throw err;
}
if (res.statusCode > 303 || res.statusCode < 200) {
if (res.statusCode < 200 || res.statusCode >= 400) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this also include 304, as in (res.statusCode < 200 || res.statusCode === 304 || res.statusCode >= 400)? @JakobJingleheimer? I feel like we should never get a 304 in Node because we’re not maintaining a cache in the way browsers are?

Also what is this error 'cannot redirect to non-network location' referring to? If the status code is 500, this seems a weird error to throw.

Copy link
Member

@JakobJingleheimer JakobJingleheimer Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would just switch this to the new isRedirect() helper

I feel like we should never get a 304 in Node because we’re not maintaining a cache in the way browsers are?

We maintain a cache, and I think there are designs (perhaps only in people's heads at the moment?) for a write-to-disk cache in future (at which point a 304 would be very valid). I'd say account for it now whilst we're thinking of it (especially because it's trivial) rather than get bitten by it later.

Also what is this error 'cannot redirect to non-network location' referring to?

This is when a remote module tries to access a local module (eg fs), which is forbidden.

If the status code is 500, this seems a weird error to throw.

Sorry, where are you seeing a 500?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, where are you seeing a 500?

500 >= 400 so it would throw here if the server responds with this code.

Copy link
Member

@JakobJingleheimer JakobJingleheimer Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh! Yes, indeed it should not throw disallowed network import for a 500.

@aduh95 I see you just approved; I'm thinking this should be consider a blocker (it introduces a bug). The rest can be addressed in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong error is not introduced in this PR, it's a part of original code.
What this PR in its current state changes here is behaviour for 303 < statusCode < 400 (for 307 and 308, only if no Location provided), importing body instead of throwing "non-network location" error. Which can be addressed in a follow-up, but is still in line with logic of old code (it did the same for 300~303).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you just approved; I'm thinking this should be consider a blocker

Livia said it all, if there's a bug let's fix it in its own PR. This PR does a great job at adding support for 307 and 308, it wouldn't be fait to block it on a bug (or maybe not bug?) of the existing implementation.

throw new ERR_NETWORK_IMPORT_DISALLOWED(
res.headers.location,
parsed.href,
Expand Down