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 2 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
Expand Up @@ -89,6 +89,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 [300, 301, 302, 303, 307, 308].includes(statusCode);
kidonng marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @param {URL} parsed
* @returns {Promise<CacheEntry> | CacheEntry}
Expand All @@ -107,9 +119,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 +138,8 @@ function fetchWithRedirects(parsed) {
err.message = `Cannot find module '${parsed.href}', HTTP 404`;
throw err;
}
if (res.statusCode > 303 || res.statusCode < 200) {
if ((res.statusCode > 303 && !isRedirect(res.statusCode)) ||
res.statusCode < 200) {
Copy link
Member

Choose a reason for hiding this comment

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

The original code was intending to catch all status codes in the 1xx, 4xx and 5xx ranges. The author didn’t realize that there were valid redirect codes higher than 303; a better version would just start at 400:

Suggested change
if ((res.statusCode > 303 && !isRedirect(res.statusCode)) ||
res.statusCode < 200) {
if (res.statusCode < 200 || res.statusCode >= 400) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for explaining, this code always looks awkward to me, but I did not change it in order to be cautious.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to point out that 3xx codes are not only redirects: for example, there is 304 Not Modified.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should correct myself: that was my assumption as to what the original code was intending. @JakobJingleheimer perhaps you can confirm? Should this condition catch 304 and/or any of the other 3xx codes?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why Bradley did not include them (he wrote this part). Perhaps it has to do with the subtle differences/specifics in 307 and 308?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#redirection_messages

307
This has the same semantics as the 302 Found HTTP response code, with the exception that the user agent must not change the HTTP method used: if a POST was used in the first request, a POST must be used in the second request.

(308 is the same, but relative to 301)

I'd need to check through the other code to see if it even supports changing the method during a redirect—I don't recall it supporting that. If it can't, I see no reason to not include 307 and 308.

Copy link
Contributor Author

@kidonng kidonng 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.

The nuances between 301/302 and 307/308 is unrelated here since it is only doing GET.

But upon taking a close look I feel this is throwing an inappropriate error. If the status code is < 200 or >= 400, why would the code expect it to have a location header? It probably should be something like ERR_NETWORK_STATUS_NOT_ALLOWED. oops, I think I almost said the same thing as #43689 (comment)

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