Allow multiple set-cookie headers #4840

marcosc90 · 2020-04-21T21:56:51Z

Multiple Set-Cookie headers should be allowed without allowing multiple headers with same cookie name

Servers SHOULD NOT include more than one Set-Cookie header field in
the same response with the same cookie-name. (See Section 5.2 for
how user agents handle this case.)

See RFC 6265

This was also being worked here #4829, although with a different approach

Fixes #4826

I exposed a new method cookies to iterate through the cookies Map, I know this is not in the spec but the spec was built for client-side JavaScript. (Alternatives are welcome though)

Unlike a header list, a Headers object cannot represent more than one Set-Cookie header. In a way this is problematic as unlike all other headers Set-Cookie headers cannot be combined, but since Set-Cookie headers are not exposed to client-side JavaScript this is deemed an acceptable compromise. Implementations could chose the more efficient Headers object representation even for a header list, as long as they also support an associated data structure for Set-Cookie headers.

CLAassistant · 2020-04-21T21:56:57Z

All committers have signed the CLA.

marcosc90 · 2020-04-21T22:00:08Z

cli/js/web/headers.ts

-            name,
-            existingValue ? `${existingValue}, ${value}` : value
-          );
+          this.append(tuple[0], tuple[1]);


I refactored the constructor a bit, to keep the code DRY and to reuse Set-Cookie logic in append/set methods.

Caesar2011 · 2020-04-21T22:05:51Z

Better appoach then mine. What about custom headers which are allowed to be send multiple times?

marcosc90 · 2020-04-21T22:13:03Z

Better appoach then mine. What about custom headers which are allowed to be send multiple times?

Thanks! Set-Cookie is a major issue since browsers and clients expect multiple values.
I've tested setting multiple custom headers in Node and it's not allowed (Maybe there's a way, didn't spend much time testing it), and pretty sure most clients do not support it.

marcosc90 · 2020-04-21T22:21:37Z

I'm working on adding some more tests.

Caesar2011 · 2020-04-21T22:34:11Z

Could you add a test for the constructor taking an string[][] as a parameter?

marcosc90 · 2020-04-21T22:46:44Z

Could you add a test for the constructor taking an string[][] as a parameter?

Sure, can I grab yours with some minor editions?

https://github.com/Caesar2011/deno/blob/aa5c311cbe0225110c460aa788fd61d5589b50ad/std/http/io_test.ts#L346-L361

Caesar2011 · 2020-04-21T22:49:26Z

Sure! Just check the expected output which is wrong as you can see in the automated testing routine for macos-release

ry · 2020-04-23T05:54:10Z

cli/js/web/headers.ts

@@ -97,12 +96,22 @@ class HeadersBase {
    return `Headers {${output}}`;
  }

+  cookies(): IterableIterator<string> {
+    return this[cookieMap].values();
+  }


This method isn't in the web API
https://developer.mozilla.org/en-US/docs/Web/API/Headers

I know, I mentioned that but it was hard to implement Set-Cookie behaviour it without extending it a bit since the web API did a compromise that the server cannot do. Although a better alternative could be to use a symbol, maybe in the Deno namespace.

[Deno.cookiesIteratorSymbol](): IterableIterator<string> { return this[cookieMap].values(); }

That way is web compatible & can be iterated in std/http/io.ts. I'm not yet very familiar with the codebase, an alternative to expose a Symbol that can be used in std and cli/web will work too.

What do you think?

ry · 2020-04-23T05:55:41Z

cli/js/web/headers.ts

+  // Servers SHOULD NOT include more than one Set-Cookie header field in
+  // the same response with the same cookie-name
+  [cookieMap]: Map<string, string>;
+


In general Headers are not a Key/Value pair - I think headerMap should be replaced with a list of key, value pairs.

Implemented a Map<cookieName, cookieHeaderValue> so we only kept the last value for the same cookie name, which is what the spec says. If I use a list of key/value pairs, I'll need to add additional logic to remove duplicates and keep the last one.

We are not storing strictly headers, but cookieName/SetCookieValue

ry

Thanks - this is much better than before with the regex, but it's made the Headers object not web compatible.

marcosc90 · 2020-04-23T09:08:04Z

Thanks - this is much better than before with the regex, but it's made the Headers object not web compatible.

Thank you. It should be web compatible now, by using a Symbol exposed in Deno.symbols.cookiesIterator

ry

Hi @marcosc90 sorry for the delay on this. I've wanted to check this out and get my hands dirty with it - because maybe I'm just missing something - but I think this can be done in a simpler more general way by changing the structure of headers from Map<string, string> to Array<[string, string]>. But I haven't had the time to actually try myself.

cli/js/lib.deno.ns.d.ts

ry · 2020-04-30T17:20:06Z

cli/js/web/headers.ts

@@ -43,15 +46,23 @@ function validateValue(value: string): void {
 class HeadersBase {
  [headerMap]: Map<string, string>;


This is the wrong structure to store HTTP headers. It should be Array<[string, string]>.

Using Array<[string, string]> would make it browser incompatible. If use call Headers.prototype.append with the same key in the browser you expect to override the previous header. But not with Set-Cookie. In this case is has to be incompatible with the standardized Header implementation.

IMHO I would prefer HeaderMap to be an Array as well to make it work without exceptions. But the user has to be warned, that append is not a Map anymore.

I agree with you, didn't want to change that because of the premise of trying to be fully browser compatible.

I can do a rework, store headers in Array<[string, string]> and change all the methods. But we'll need to define what we'll expect when the user does:

header.set('Set-Cooke', 'some-value'); header.set('Set-Cooke', 'other-value'); header.get('Set-Cookie'); // ['some-value', 'other-value'] header.get('Set-Cookie'); // 'some-value, other-value'

If we return an array, it's not respecting the specification. Although Set-Cookie is not used by users on the client side.

If we use an Array I can drop the cookies Map, although I'll have to loop every time we try to set a cookie, because we can't store cookies with same name, we should only keep the latest one.

And what happens when using an Array and header.set('Content-Type'); twice!

And what happens when using an Array and header.set('Content-Type'); twice!

We can still make it browser compatible by using Array<[string, string]> and doing extra validation and looping through the array on every method. The implementation will be less clean and a bit slower since we'll be iterating constantly through the array.

bartlomieju · 2020-05-09T17:19:18Z

After offline discussion we decided to go with #5100 as it doesn't introduce new symbols.

Thank you @marcosc90 for your work but I'm gonna close this PR without a merge.

Allow multiple set-cookie headers

f0b6b48

marcosc90 mentioned this pull request Apr 21, 2020

WIP: Allow multiple set-cookie headers #4829

Closed

marcosc90 commented Apr 21, 2020

View reviewed changes

Added return type

b09dedd

Format code

3576a78

marcosc90 added 2 commits April 22, 2020 01:22

Tests added

dd70ec6

Fix io_test

055f023

ry reviewed Apr 23, 2020

View reviewed changes

marcosc90 added 2 commits April 23, 2020 10:58

Replace cookies method with [cookiesIteratorSymbol] method

bd8645b

Fix headers test

fba2726

marcosc90 added 2 commits April 30, 2020 10:47

Merge branch 'master' into multiple-set-cookie

100b588

Code changes after breaking changes

e80793c

ry reviewed Apr 30, 2020

View reviewed changes

Remove symbol from public types

bc738f6

This was referenced May 6, 2020

Sending multiple Set-Cookie headers get merged #4826

Closed

All multiple Set-Cookie headers #5100

Merged

bartlomieju closed this May 9, 2020

marcosc90 deleted the multiple-set-cookie branch May 24, 2020 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple set-cookie headers #4840

Allow multiple set-cookie headers #4840

marcosc90 commented Apr 21, 2020

CLAassistant commented Apr 21, 2020 •

edited

Loading

marcosc90 Apr 21, 2020

Caesar2011 commented Apr 21, 2020

marcosc90 commented Apr 21, 2020 •

edited

Loading

marcosc90 commented Apr 21, 2020

Caesar2011 commented Apr 21, 2020

marcosc90 commented Apr 21, 2020

Caesar2011 commented Apr 21, 2020

ry Apr 23, 2020

marcosc90 Apr 23, 2020 •

edited

Loading

ry Apr 23, 2020

marcosc90 Apr 23, 2020 •

edited

Loading

ry left a comment

marcosc90 commented Apr 23, 2020

ry left a comment

ry Apr 30, 2020

Caesar2011 Apr 30, 2020

marcosc90 Apr 30, 2020

Caesar2011 Apr 30, 2020

marcosc90 Apr 30, 2020

bartlomieju commented May 9, 2020

		@@ -43,15 +46,23 @@ function validateValue(value: string): void {
		class HeadersBase {
		[headerMap]: Map<string, string>;

Allow multiple set-cookie headers #4840

Allow multiple set-cookie headers #4840

Conversation

marcosc90 commented Apr 21, 2020

CLAassistant commented Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Caesar2011 commented Apr 21, 2020

marcosc90 commented Apr 21, 2020 • edited Loading

marcosc90 commented Apr 21, 2020

Caesar2011 commented Apr 21, 2020

marcosc90 commented Apr 21, 2020

Caesar2011 commented Apr 21, 2020

Choose a reason for hiding this comment

marcosc90 Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosc90 Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

marcosc90 commented Apr 23, 2020

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented May 9, 2020

CLAassistant commented Apr 21, 2020 •

edited

Loading

marcosc90 commented Apr 21, 2020 •

edited

Loading

marcosc90 Apr 23, 2020 •

edited

Loading

marcosc90 Apr 23, 2020 •

edited

Loading