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: 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 3 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
18 changes: 15 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
@@ -1,5 +1,6 @@
'use strict';
const {
ArrayPrototypeIncludes,
ObjectPrototypeHasOwnProperty,
PromisePrototypeThen,
SafeMap,
Expand Down Expand Up @@ -89,6 +90,18 @@ 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) {
return ArrayPrototypeIncludes([300, 301, 302, 303, 307, 308], statusCode);
}

/**
* @param {URL} parsed
* @returns {Promise<CacheEntry> | CacheEntry}
Expand All @@ -107,9 +120,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 +139,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