fix: correctly handle multi-value headers in fetch when rawHeaders contains array

[hexchain]

This relates to...

#4389

Changes

In httpNetworkFetch->dispatch->onHeaders, append multi-value header values one by one and let HeadersList handle concatenation.

Features

N/A

Bug Fixes

#4389

Breaking Changes and Deprecations

N/A

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[hexchain]

I've pushed a different fix as discussed in #4390 (comment). Please take a look, thanks!

[metcoder95]

code wise, lgtm; but defer to @KhafraDev

[metcoder95]

As this only breaks given an interceptor, I'd prefer that we fix it within undici's rather than fetch

[hexchain]

My idea is that fetch should properly support arrays of header values in rawHeaders, if such case is valid.

[metcoder95]

Sure, but I'd prefer to cross check with the spec to see what it has to say regarding duplicated headers, if the implementation already aligns, most likely is an undici issue.

[hexchain]

IIUC, RFC 7230 Section 3.2.2 means that:

• A sender must not send multiple headers with the same name unless it is Set-Cookie,
• A receiver may combine values of multiple headers with the same name into a comma-separated list, except for Set-Cookie.

I'd say it is fine to have them separated in Undici/interceptor APIs, and only join them in fetch.

[Uzlopak]

You refer to RFC7230 but not the fetch spec

[hexchain]

It seems to me that HTTP-network fetch doesn't really specify how headers should be treated while being received? It merely says:

Follow the relevant requirements from HTTP. [HTTP] [HTTP-CACHING]

Wait until all the HTTP response headers are transmitted.

in which "[HTTP]" refers to RFC9110. In that document, section 5.2 and 5.3 are basically a rephrase of RFC7230 section 3.2.2.

In undici, the relevant code is in httpNetworkFetch->dispatch->onHeaders, and I believe is an implementation detail. IIUC, changes to this part of the code should count as fixing the issue in undici.

[hexchain]

Gentle ping. May I have some reviews again?

The latest change only touches fetch and not (Un)wrapHandlers, so no changes to existing interceptor/handler behaviors. Duplicate headers are processed mostly in the same way as before, except when interceptors exist, in which case it is more correct than before - it uses HeadersList.append now instead of Array<Buffer>.toString.

[mcollina]

lgtm

[metcoder95]

I'd still suggest to alter the interceptor rather than fetch itself

[hexchain]

@metcoder95 By "interceptor" do you mean Wrap/UnwrapHandler? There is (almost) no interceptor involved in the original reproducer.

[metcoder95]

Yes, exactly; sorry for the confusion, was referring to the wrapper

[mcollina]

@metcoder95 can you do an alternative PR with that fix? I'm not sure it'd be possible to actually do that.

[hexchain]

I believe that was also the very first iteration of this PR.

I'll try to summarize the situation and hopefully uncover a clear path forward.

The issue happens because the original dispatch handler (which is "old"-style, according to #3878) gets wrapped and unwrapped, so the headers argument of onHeaders/onResponseStart changed, from a Buffer[] to a Record<string, string | string[]> and then (incorrectly) to an Array<Buffer | Buffer[]>, which ultimately breaks the dispatch handler in fetch. So far, I see two approaches:

1. The first iteration: fix the header conversion in the "wrapping" process: expand header values of string[] back to multiple key-value Buffer pairs - basically undo UnwrapHandler::onHeaders.
   This was the first iteration, however in this review I was asked to add a check to see whether the header allows multi-value before expanding. It feels a bit awkward to do that because:
   • The same kind of check also happens somewhere in node:http and (to some extent) HeadersList but neither is reusable.
   • WrapHandler::onResponseStart isn't the best place to modify/sanitize the data, when it is only supposed to restore the headers to what it looked like before "unwrapping". Even if ensuring the header being spec-compliant is a general concern, it should still happen somewhere else, either in the dispatcher or higher-level APIs.
2. Current: fix the dispatch handler in fetch to also accept Array<Buffer | Buffer[]>, in addition to Buffer[]. For now, the type of headers in the signature of (the deprecated) onHeaders is still Buffer[], so this also feels like a hack.

Please enlighten me if there are better ways. Personally, I think the first idea is more sound but I'm really unsure about adding the header name check.