Deno: Sending multiple Set-Cookie headers get merged

[Caesar2011]

When sending multiple Set-Cookie headers, they are combined to one header separated by commas. While this behavior is desired for other headers, such as Accept, the Set-Cookie header is not parsed correctly, at least under Firefox 75.

I set the header as follows, where cookieHeaders is of type [string, string][].

await req.respond({ body: this.body, headers: new Headers([['content-type', this.mimeType], ...cookieHeaders, ...this.headers]), status: this.status });

await req.respond({ body: "Hello World!", headers: new Headers([
    ['content-type', 'text/plain'],
    [ "Set-Cookie", "user.session=qwertz; Max-Age=86400" ],
    [ "Set-Cookie", "a=123; Max-Age=86400" ],
    [ "Set-Cookie", "b=456; Max-Age=86400" ]
  ]), 
  status: 200
});

This results in the following response headers in Firefox's network analysis.

HTTP/1.1 200 OK
content-type: text/plain
set-cookie: user.session=qwertz; Max-Age=86400, a=123; Max-Age=86400, b=456; Max-Age=86400
content-length: 12

When a new request is made to the URL, only the first specified header is actually set. The cookies a and b are ignored. These are the request headers from the 2nd request.

Host: localhost:3001
User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
Accept-Language: de,en-US;q=0.7,en;q=0.3
Accept-Encoding: gzip, deflate
DNT: 1
Connection: keep-alive
Cookie: user.session=qwertz
Upgrade-Insecure-Requests: 1
Cache-Control: max-age=0

In my opinion, the headers should look like this. Sending a header multiple times is totally legitimate and allowed by the HTTP specifications. This is, in my understanding, the reason why headers accept a [string, string][] and not a key-value object, since keys can be used multiple times.

https://tools.ietf.org/html/rfc6265#page-7

HTTP/1.1 200 OK
content-type: text/plain
set-cookie: user.session=qwertz; Max-Age=86400
set-cookie: a=123; Max-Age=86400
set-cookie: b=456; Max-Age=86400
content-length: 12

[Caesar2011]

According to the source code, Headers is an in-build class of the Fetch API. Hard to believe that there is no way to use it in the right way, but it is intended to be used from the client. And as far as I know, Set-Cookie is the only standards-compliant header that can be used multiple times. And clients do not send it.

This is how it is done in node.js and expressJS:

https://stackoverflow.com/questions/39397983/how-do-i-set-multiple-http-header-fields-with-the-same-key-in-node-js

[Caesar2011]

Should be fixed, needs approval

[Caesar2011]

The more I think about this problem, the uglier it gets. What about custom headers? It must be possible to add them multiple times, too. Also in the set-cookie expiration date might be a comma.

The cleanest solution would be to replace or extend the Headers class, which is an build-in javascript class, and the fetch API relies on it. I might be possible to extend the header class, but because it relies on a key-value object structure it wouldn't be consistent in all methods.

IMHO is the Headers class and all derivatives no usable. But I do not suggest to change it. I will later patch a "better" workaround, at least for Set-Cookie.

---

A pretty ugly solution for supporting repeated custom headers could be prefixing them, e.g. with repheader-my-custom-header and the value with %%my, custom; value=1%% and %%my, custom; value=1%%. This would result in the key-value pair repheader-my-custom-header and %%my, custom; value=1%%, %%my, custom; value=2%%, which we are able to split into the 2 header:

my-custom-header: my, custom; value=1
my-custom-header: my, custom; value=2

I don't have to say how "pretty" this solution is.

[kitsonk]

@ry @marcosc90 @Caesar2011 I just ran across this when working on oak. It looks like the intention is that we use .append() when setting multiple headers (see: https://github.com/whatwg/fetch/issues/973). I am going to try to see if that works, it should work. If it does, we should utilise that method. The issue I mention talks about once you set it though, you can't get all of them back, and it is an open issue with the spec.

[Caesar2011]

@kitsonk I already tried append and it doesn't work. All it does is appending to the already existing header by joining the existing one with the new one, seperated by ,.

Using the 2-dimentional table construction is the same as using append. The result is the same as in the initial post at the beginning of the issue. :D

[kitsonk]

so it looks like our current implementation with .append() creates a comma separated value (concatenates them) and we should have custom logic to not do that on our Headers implementation. The specification is a bit vague as I can tell, except for the note found here: https://fetch.spec.whatwg.org/#headers-class which indicates that they can't be concatenated and once appended, they cannot be represented back in JavaScript land (which is the issue mentioned above).

I am going to take another stab at a PR to try to fix this.

[kitsonk]

@Caesar2011 understood... it is clear though there is this one and only special exception to the rule... I am pretty sure I can make the change without leaking anything implementation and making it compliant.

[marcosc90]

@kitsonk the implementation on https://github.com/denoland/deno/pull/4840 is compliant and does not leak anything.

[kitsonk]

No @marcosc90 I don't think it does... it doesn't yield the iterable values right.

[marcosc90]

@kitsonk Care to explain? Because all tests succeeded, cookies work and the implementation does not break previous behaviour.

[kitsonk]

ping @ry @bartlomieju need to come to a decision on this

[ry]

@kitsonk It's on my queue! I will get to it before tomorrow's release. Sorry! :|