Implement the requirement of IncomingHttpHeaders["set-cookie"]

[pan93412]

This relates to...

Fixed #1892

Rationale

Currently headers["set-cookie"] does not split, which can't match the expected type of IncomingHttpHeader.

Changes

This commit attempts to split headers["set-cookie"] by ; like what 'node:http' does: https://nodejs.org/api/http.html#class-httpserverresponse

Bug Fixes

See above.

Breaking Changes and Deprecations

• set-cookie is now always an string[].

Status

KEY: S = Skipped, x = complete

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

[pan93412]

Note that there are at least 1 unmatched behavior of IncomingHttpHeaders:

• Duplicates of age, authorization, content-length, content-type, etag, expires, from, host, if-modified-since, if-unmodified-since, last-modified, location, max-forwards, proxy-authorization, referer, retry-after, server, or user-agent are discarded. To allow duplicate values of the headers listed above to be joined, use the option joinDuplicateHeaders in http.request() and http.createServer(). See RFC 9110 Section 5.3 for more information.
• set-cookie is always an array. Duplicates are added to the array.
• For duplicate cookie headers, the values are joined together with ; .
• For all other headers, the values are joined together with , .

https://github.com/nodejs/node/blob/47cd966104a2c959cb76468fc23fd844e338b9f2/lib/_http_incoming.js#L385

Besides, I found another inconsistent part between current's parseHeader() and IncomingHttpHeaders.

According to the IncomingHttpHeaders declaration, some cookies should always be a string (and never become an array:)

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/4ef3d4e49b46e9832fc93208d14ddd249c8360e7/types/node/http.d.ts#L48-L112

However, undici always merges the duplicate part to an array:

undici/lib/core/util.js

Lines 223 to 227 in 196c4da

	if (!Array.isArray(val)) {
	val = [val]
	obj[key] = val
	}
	val.push(headers[i + 1].toString())

…which will break the type declaration of IncomingHttpHeaders when occurring the duplicate header:

Referer: https://www.example.com
Referer: https://www.example.com

{
  Referer: ["https://www.example.com”, "https://www.example.com”]
}

As undici is not the substitute of http, should we maintain our own IncomingHttpHeaders?

[codecov-commenter]

Codecov Report

Base: 90.33% // Head: 90.33% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (4e54ad0) compared to base (196c4da).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1893      +/-   ##
==========================================
- Coverage   90.33%   90.33%   -0.01%     
==========================================
  Files          70       70              
  Lines        6045     6050       +5     
==========================================
+ Hits         5461     5465       +4     
- Misses        584      585       +1     

Impacted Files	Coverage Δ
lib/core/util.js	98.00% <100.00%> (+0.06%)	⬆️
lib/fetch/index.js	84.65% <0.00%> (-0.19%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mcollina]

I'm a bit lost with this PR. The original issue refers a typescript problem. This change alters the current behavior in code to match the types.

Let's do the other way around: the types should follow what the code implements. This change can ship in v5.

A breaking change to match the RFC or the current behavior of node is ok, but it will have to ship on v6.

[pan93412]

I'm a bit lost with this PR. The original issue refers a typescript problem. This change alters the current behavior in code to match the types.

Let's do the other way around: the types should follow what the code implements. This change can ship in v5.

A breaking change to match the RFC or the current behavior of node is ok, but it will have to ship on v6.

Sure, let me fix the type to match what parseCookie current does. I'll currently close it.

[tremby]

I found my way here by way of being confused that IncomingHttpHeaders in undici is not the same as IncomingHttpHeaders from node:http. In particular I was getting broken behaviour with set-cookie headers, which Typescript wasn't catching.

I think the specific problem in my case is a bug in urllib which I reported as node-modules/urllib#467. (I'm using that package only because I need HTTP digest auth, and that was the first working implementation I could find. Open to suggestions.) Urllib makes what I believe is a flawed assumption that the IncomingHttpHeaders undici provides is the same as the node:http IncomingHttpHeaders. This seems like an easy mistake to make; I wonder if other downstream projects have done the same.

So this is just me adding my upward thumb to the idea of changing the behaviour and types to match node's in a future version. Having to check whether it's nullish, or an array, or a string, and handle each appropriately, is a needless pain point.