esm: throw on any non-2xx response

[LiviaMedeiros]

Refs: #43689

Restrict accepting responses with 3xx status code as to-be-imported code and proceeding.
There are three categories in 3xx codes:

• Redirects (300, 301, 302, 303, 307, 308). If a redirecting response has Location header, it is already handled by code above. If it doesn't, we can't follow it and should throw. We shouldn't attempt to read the response body (usually, there will be human-readable redirect, e.g. text or html).
• Known non-redirects (304, 305, 306). Any of these should be treated as error here.
• Unknown codes (e.g. future RFC). If it's something that we'll want to handle, it should be implemented explicitly. By default all unknown non-2xx responses are errors.

Arguably, we can move further and restrict all 2xx responses other than 200 and maybe 204. I think, formally it would contradict with the purpose of 2xx codes.

---

/cc @GeoffreyBooth @JakobJingleheimer

I expect to see other follow-ups from #43689. Depending on them, this PR might be made obsolete by a better alternative, applied separately, or improved.

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[GeoffreyBooth]

Is this rebased off the latest main since we merged in the redirect PR?

[LiviaMedeiros]

Yep, it's based on 3d575a4

[aduh95]

LGTM. It'd be nice to have tests for all possible HTTP response codes but I might be asking too much here.

[LiviaMedeiros]

From a quick glance on current plans of Loaders project, I'd prefer if tests for various HTTP codes will eventually be a part of tests for more specific conditions (e.g. currently this subsystem is sensitive to Content-Type header, there is a possibility of caching with ETag in the future, etc. etc.)

[JakobJingleheimer]

Are you thinking correcting the error be handled separately/subsequently?

I think it would be better to create helpers for these, like Kidonng just added with isRedirect(). Something like

if (!isStatusOk(res.statsuCode)) { // keeping with fetch's response.ok
  throw new ERR_NETWORK_IMPORT_BAD_RESPONSE(…);

I think response.ok is a bad prop name though because 200 = OK, and response.ok is any 2xx. If we're okay deviating, I think isStatusGood is a better name.

If you'd rather that also be a follow-up, sure, I can do.

A little troubling that no test needed to be updated for this 😨

[aduh95]

I think response.ok is a bad prop name though because 200 = OK, and response.ok is any 2xx. If we're okay deviating, I think isStatusGood is a better name.

Why deviate? AFAICT web browsers would accept any response with a 2xx status code as valid (as long as it has the correct Content-Type header), I think Node.js should too.

Repro

// server.cjs
const http = require('node:http');
http.createServer((req, res) => {
  if (/^\/\d+$/.test(req.url))
    res.writeHead(Number(req.url.slice(1)), { 'Content-Type': 'text/javascript' });
  res.end();
}).listen(8000);

Then open http://localhost:8000 on the web browser you want to test and paste the following in the DevTools JS console:

Promise.allSettled(Array.from({length: 99}, (_, i) => import(`/${i+200}`))).then(console.log)

[JakobJingleheimer]

Do you mean what reason would we have for deviating, or why would using a different name be deviating?

I'm not proposing different status codes, merely word-smithing the name of the potential helper (isStatusOk vs isStatusGood).

[aduh95]

I'm sorry, I don't understand what you mean by "because 200 = OK, and response.ok is any 2xx", or how that's a justification for preferring isStatusGood – if anything that seems like a reason for keeping ok in the helper name.
Anyway, I don't have any strong feeling regarding the name of the helper, I interpreted your comment as you wanted to limit valid response to code 200, as long as it's not the case I'm happy either way.

[JakobJingleheimer]

I interpreted your comment as you wanted to limit valid response to code 200

Noooo ;)

I don't understand what you mean by "because 200 = OK, and response.ok is any 2xx"

Fetch's response.ok is true when the status code is 2xx. However, status code 200's textual name is OK. I'm wondering if people might misunderstand isStatusOk to mean "is the status specifically 200 OK". isStatusGood removes the ambiguity but deviates from fetch's poorly named response.ok.

[LiviaMedeiros]

I'll be happy with any outcome that will be most appropriate according to "big picture" of this subsystem.

My personal preference for this part is to see it rewritten using fetch (i.e. undici). In that case, you can use response.ok directly and handle redirects without deviations from common ways. Additionally, you won't need to maintain this logic separately, e.g. if there will be new RFC with new codes.

As for naming and helper functions, my personal preference for 2xx codes is to have it inlined with hardcoded numbers as long as there is only one place where it's checked; or relying on response.ok if possible.
Same for 3xx if manual redirection handling will be kept: I think, having a function named isRedirect() or variable named isRedirect is exactly as self-explanatory in the code as needed.

Regarding isStatus* name, I agree that response.ok is poorly named. But if it's used on actual Response, it's ok. :)
isStatusGood sounds worse to me as it implies that there was some specific validation.
I'd suggest isStatusSuccess or isStatusSuccessful because it's canonical term for 2xx codes.

[JakobJingleheimer]

Oow, actually, that's an excellent point. Is there any reason we can't port this to actual fetch? Would we run into any cache contamination? 🤔 CORS would suddenly come into the picture.

Fetch didn't exist yet in Node when this was written, so that discussion hasn't already happened AFAIK.

[LiviaMedeiros]

I think, porting this to fetch is feasible and desirable. CORS shouldn't be an issue for client-side Node.js.
Cache shouldn't be a problem as well: I don't know if ignoring cache-related HTTP headers was intentional, but Request.cache supports different modes, so you can either rely on it or handle manually.

[JakobJingleheimer]

I don't know if ignoring cache-related HTTP headers was intentional

I vaguely remember that it was but I couldn't tell you the rationale at the time.

[GeoffreyBooth]

I think an explanation for this if condition would be helpful. Is this the correct reason?

Suggested change

	if (res.statusCode < 200 || res.statusCode >= 300) {
	// The 3xx codes that we support were handled above in isRedirect
	// This condition catches all unsupported status codes
	if (res.statusCode < 200 || res.statusCode >= 300) {

[LiviaMedeiros]

Added a comment via 640b85d.
I think, this condition can be read properly and unambiguously as "we need 2xx code", so there is no difference between 1xx, 3xx, 5xx or any invalid number.
What we might need to point out is that isRedirect check above handled only machine-readable redirects with Location, so we want to filter them again. If this part will be refactored into some validateStatusCode that throws meaningful errors for different coderanges, we don't want these to be omitted.

[nodejs-github-bot]

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

[LiviaMedeiros]

Landed in b3f2af9