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

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

[hexchain]

I've pushed a different fix as discussed in https://github.com/nodejs/undici/pull/4390#discussion_r2259309353. Please take a look, thanks!

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

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