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

kidonng · 2022-07-05T14:17:54Z

Per RFC 7231 and 7238, HTTP 307 and 308 status code are also for
redirect responses.

Fixes: #43679
Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7
Refs: https://datatracker.ietf.org/doc/html/rfc7238

Per RFC 7231 and 7238, HTTP `307` and `308` status code are also for redirect responses. Fixes: #43679 Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7 Refs: https://datatracker.ietf.org/doc/html/rfc7238

nodejs-github-bot · 2022-07-05T14:18:01Z

Review requested:

@nodejs/modules

lib/internal/modules/esm/fetch_module.js

LiviaMedeiros · 2022-07-06T08:17:27Z

lib/internal/modules/esm/fetch_module.js

-      if (res.statusCode > 303 || res.statusCode < 200) {
+      if (
+        (
+          res.statusCode > 303 &&
+          res.statusCode !== 307 &&
+          res.statusCode !== 308
+        ) || res.statusCode < 200) {


This part makes me wonder if original flow is correct. If the response isRedirect && !hasLocation, shouldn't we throw? Especially if we deal with 300 Multiple Choices.
This concern is not blocking and can be addressed separately, the change itself LGTM.

I think if the server returns 300 Multiple Choices and no Location, it should be treated as unresolvable/fail (or follow the Location if provided).

Perhaps a custom loader should be used deal with Multiple Choices.

To be fair, I'm not even sure if there are reasonable use cases to import a body of response with any 2xx code other than 200.

I often get the body from a 201 as "a representation of the thing that was created".

From a GET request? May I ask in which scenario it's required or has high probability of happening?

oh true, i wouldn't expect a 201 from a GET request :-) a 204 (no body) or 206 (partial body) maybe, though

206 probably shouldn't happen there (i.e. we don't send any Range so partial body is not what should be expected), 204 restricts having a body at all... Although yes, technically we can import {} from 'https://example.com/204-no-content.mjs'; and there's nothing really wrong about that. 😄

lib/internal/modules/esm/fetch_module.js

GeoffreyBooth · 2022-07-06T16:11:50Z

lib/internal/modules/esm/fetch_module.js

+      if ((res.statusCode > 303 && !isRedirect(res.statusCode)) ||
+          res.statusCode < 200) {


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) {

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

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

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?

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.

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)

hemanth · 2022-07-06T22:56:58Z

Maybe we need some tests ?

kidonng · 2022-07-07T03:14:21Z

Maybe we need some tests ?

I didn't add tests because this is simply expanding existing check and should be covered by existing tests like:

node/test/es-module/test-http-imports.mjs

Lines 104 to 117 in b985ad5

    
           // Redirects have same import.meta.url but different cache 
        
           // entry on Web 
        
           const redirect = new URL(url.href); 
        
           redirect.searchParams.set('redirect', JSON.stringify({ 
        
             status: 302, 
        
             location: url.href 
        
           })); 
        
           const redirectedNS = await import(redirect.href); 
        
           assert.strict.deepStrictEqual( 
        
             Object.keys(redirectedNS), 
        
             ['default', 'url'] 
        
           ); 
        
           assert.strict.notEqual(redirectedNS.default, ns.default); 
        
           assert.strict.equal(redirectedNS.url, url.href);

Probably don't need a test for every response code.

ljharb · 2022-07-07T03:49:14Z

boo on the switch :-) an array can be made multiline with comments just as easily.

GeoffreyBooth · 2022-07-07T04:28:49Z

lib/internal/modules/esm/fetch_module.js

+ * https://datatracker.ietf.org/doc/html/rfc7231#section-6.4
+ * and RFC 7238:
+ * https://datatracker.ietf.org/doc/html/rfc7238
+ * @param {number} statusCode


Suggested change

* @param {number} statusCode

* See also https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

* @param {number} statusCode

Suggested change

* @param {number} statusCode

* @param {number} statusCode

* @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

GeoffreyBooth · 2022-07-07T04:29:34Z

lib/internal/modules/esm/fetch_module.js

+    case 301: // Moved Permanently
+    case 302: // Found
+    case 303: // See Other
+    case 307: // Temporary Redirect


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.

+1 for the comment about explicitly skipping 304 🙂

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

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.

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).

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

GeoffreyBooth · 2022-07-07T04:32:59Z

lib/internal/modules/esm/fetch_module.js

@@ -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) {


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.

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?

Sorry, where are you seeing a 500?

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

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.

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).

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.

LiviaMedeiros

LGTM before it's derailed too much. 😄
The fix is correct; issues of original code (both functional and stylistic) can be addressed by follow-up PRs.

nodejs-github-bot · 2022-07-07T11:01:54Z

Commit Queue failed

- Loading data for nodejs/node/pull/43689
✔  Done loading data for nodejs/node/pull/43689
----------------------------------- PR info ------------------------------------
Title      esm: treat `307` and `308` as redirects in HTTPS imports (#43689)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     kidonng:fetch_module-3xx -> nodejs:main
Labels     esm, author ready, needs-ci, commit-queue-squash
Commits    4
 - esm: treat `307` and `308` as redirects in HTTPS imports
 - Split `isRedirect` into its own function per review
 - Apply review suggestions
 - Use a switch
Committers 1
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/43689
Fixes: https://github.com/nodejs/node/issues/43679
Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7
Refs: https://datatracker.ietf.org/doc/html/rfc7238
Reviewed-By: Geoffrey Booth 
Reviewed-By: Zijian Liu 
Reviewed-By: LiviaMedeiros 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Jacob Smith 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43689
Fixes: https://github.com/nodejs/node/issues/43679
Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7
Refs: https://datatracker.ietf.org/doc/html/rfc7238
Reviewed-By: Geoffrey Booth 
Reviewed-By: Zijian Liu 
Reviewed-By: LiviaMedeiros 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Jacob Smith 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 05 Jul 2022 14:17:54 GMT
   ✔  Approvals: 5
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/43689#pullrequestreview-1030971473
   ✔  - Zijian Liu (@Lxxyx): https://github.com/nodejs/node/pull/43689#pullrequestreview-1030853737
   ✔  - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/43689#pullrequestreview-1031160705
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43689#pullrequestreview-1031177455
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/43689#pullrequestreview-1031313255
   ✖  This PR needs to wait 3 more hours to land
   ✔  Last GitHub CI successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2629054923

JakobJingleheimer · 2022-07-07T11:06:03Z

Ah, whoops. The PR says 2d, but it's only 45 hours (not 48).

LiviaMedeiros · 2022-07-07T11:10:39Z

We also need a CI run (Jenkins, not GitHub), but it's on lockdown right now.

JakobJingleheimer · 2022-07-07T12:53:47Z

Oh, I thought it was only locked til yesterday. Sit tight. Rodger 👍

nodejs-github-bot · 2022-07-08T19:53:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45189/

nodejs-github-bot · 2022-07-09T00:01:29Z

Landed in 3d575a4

Per RFC 7231 and 7238, HTTP `307` and `308` status code are also for redirect responses. Fixes: #43679 Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7 Refs: https://datatracker.ietf.org/doc/html/rfc7238 PR-URL: #43689 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Treat redirects without Location and other 3xx responses as errors PR-URL: nodejs#43742 Refs: nodejs#43689 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Per RFC 7231 and 7238, HTTP `307` and `308` status code are also for redirect responses. Fixes: #43679 Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7 Refs: https://datatracker.ietf.org/doc/html/rfc7238 PR-URL: #43689 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Treat redirects without Location and other 3xx responses as errors PR-URL: #43742 Refs: #43689 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Per RFC 7231 and 7238, HTTP `307` and `308` status code are also for redirect responses. Fixes: #43679 Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7 Refs: https://datatracker.ietf.org/doc/html/rfc7238 PR-URL: #43689 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Treat redirects without Location and other 3xx responses as errors PR-URL: #43742 Refs: #43689 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Per RFC 7231 and 7238, HTTP `307` and `308` status code are also for redirect responses. Fixes: nodejs/node#43679 Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7 Refs: https://datatracker.ietf.org/doc/html/rfc7238 PR-URL: nodejs/node#43689 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

Treat redirects without Location and other 3xx responses as errors PR-URL: nodejs/node#43742 Refs: nodejs/node#43689 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

esm: treat 307 and 308 as redirects in HTTPS imports

52aaed3

Per RFC 7231 and 7238, HTTP `307` and `308` status code are also for redirect responses. Fixes: #43679 Refs: https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.7 Refs: https://datatracker.ietf.org/doc/html/rfc7238

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Jul 5, 2022

GeoffreyBooth reviewed Jul 5, 2022

View reviewed changes

lib/internal/modules/esm/fetch_module.js Outdated Show resolved Hide resolved

LiviaMedeiros reviewed Jul 6, 2022

View reviewed changes

Split isRedirect into its own function per review

54ee622

GeoffreyBooth reviewed Jul 6, 2022

View reviewed changes

Apply review suggestions

542b5cf

GeoffreyBooth approved these changes Jul 6, 2022

View reviewed changes

GeoffreyBooth added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2022

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 6, 2022

Lxxyx approved these changes Jul 7, 2022

View reviewed changes

kidonng requested a review from GeoffreyBooth July 7, 2022 03:18

Use a switch

9ba00b1

GeoffreyBooth approved these changes Jul 7, 2022

View reviewed changes

LiviaMedeiros approved these changes Jul 7, 2022

View reviewed changes

aduh95 approved these changes Jul 7, 2022

View reviewed changes

JakobJingleheimer approved these changes Jul 7, 2022

View reviewed changes

JakobJingleheimer added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 7, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 7, 2022

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 7, 2022

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2022

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2022

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Jul 8, 2022

CI Reliability 2022-07-08 nodejs/reliability#318

Open

16 tasks

This comment was marked as outdated.

Sign in to view

LiviaMedeiros added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 8, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 9, 2022

nodejs-github-bot merged commit 3d575a4 into nodejs:main Jul 9, 2022

github-actions bot mentioned this pull request Jul 9, 2022

CI Reliability 2022-07-09 nodejs/reliability#319

Open

21 tasks

LiviaMedeiros mentioned this pull request Jul 9, 2022

esm: throw on any non-2xx response #43742

Merged

github-actions bot mentioned this pull request Jul 10, 2022

CI Reliability 2022-07-10 nodejs/reliability#320

Open

20 tasks

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

		if ((res.statusCode > 303 && !isRedirect(res.statusCode)) \|\|
		res.statusCode < 200) {

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

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

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

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

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

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

Conversation

kidonng commented Jul 5, 2022

nodejs-github-bot commented Jul 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiviaMedeiros Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

hemanth commented Jul 6, 2022

kidonng commented Jul 7, 2022

ljharb commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

JakobJingleheimer Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiviaMedeiros left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 7, 2022

JakobJingleheimer commented Jul 7, 2022 • edited Loading

LiviaMedeiros commented Jul 7, 2022

JakobJingleheimer commented Jul 7, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jul 8, 2022

nodejs-github-bot commented Jul 9, 2022

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

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

LiviaMedeiros Jul 6, 2022 •

edited

Loading

kidonng Jul 7, 2022 •

edited

Loading

GeoffreyBooth Jul 7, 2022 •

edited

Loading

GeoffreyBooth Jul 7, 2022 •

edited

Loading

JakobJingleheimer Jul 7, 2022 •

edited

Loading

JakobJingleheimer Jul 7, 2022 •

edited

Loading

JakobJingleheimer Jul 7, 2022 •

edited

Loading

JakobJingleheimer commented Jul 7, 2022 •

edited

Loading