Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The comma-delimited combined value definition does not support Set-Cookie headers #345

Closed
malisas opened this issue Jul 29, 2016 · 11 comments

Comments

@malisas
Copy link

commented Jul 29, 2016

The spec says:

A combined value, given a name (name) and header list (list), is the values of all headers in list whose name is name, separated from each other by ,, in order.

This properly follows the HTTP/1.1 Message Syntax and Routing Standards, which says:

A recipient MAY combine multiple header fields with the same field
name into one "field-name: field-value" pair, without changing the
semantics of the message, by appending each subsequent field value to
the combined field value in order, separated by a comma.

However, headers with the name Set-Cookie are discussed as a special case:

Note: In practice, the "Set-Cookie" header field ([RFC6265]) often
appears multiple times in a response message and does not use the
list syntax, violating the above requirements on multiple header
fields with the same name. Since it cannot be combined into a
single field-value, recipients ought to handle "Set-Cookie" as a
special case while processing header fields. (See Appendix A.2.3
of [Kri2001] for details.)

The Set-Cookie header field is defined here. A couple of its sub-rules, specifically path-value and extension-av, allow commas as part of the elements. This means that header-field values for Set-Cookie names should not be comma-delimited.

Also to note from "HTTP State Management Mechanism" from the latter link above:

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

Conclusion: The Fetch spec should be updated with an exception for handling the Set-Cookie header name case

(please double-check my interpretation, I could be wrong!)

@malisas malisas referenced this issue Jul 29, 2016
3 of 5 tasks complete
@annevk

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

We don't expose Set-Cookie so this isn't really a problem for the API, but there's probably a few things that could be clarified here, agreed.

@malisas

This comment has been minimized.

Copy link
Author

commented Aug 4, 2016

Ah ok, thank you!

@malisas malisas closed this Aug 4, 2016
@annevk

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

I do think we should clarify this so I'm going to reopen. Hope you don't mind.

@annevk annevk reopened this Aug 4, 2016
annevk added a commit that referenced this issue Apr 19, 2018
@annevk

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

I finally posted a clarification in #714.

@kentonv

This comment has been minimized.

Copy link

commented Jun 12, 2018

FWIW, Cloudflare Workers needs to break spec here, because in our environment Set-Cookie very much is accessible and intended to be accessed from JavaScript. Our plan for now is to special-case Set-Cookie in calls to append().

@annevk

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

That would still result in a broken get() and enumeration.

@kentonv

This comment has been minimized.

Copy link

commented Jun 12, 2018

@annevk Yes. It's less obvious what to do about those, so we haven't come up with anything. Looking at practical use cases, fixing append() seems to be the most pressing need -- lots of people want to add cookies in Workers, but we haven't seen anyone who cares to inspect cookies coming from upstream.

@annevk

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

It seems somewhat inevitable that eventually code gets complex enough that it needs inspection. I suppose when returning instead of "combined value" you could use something else that uses \n to combine the headers. (I'm not sure why you need to special case append() btw other than allowing this header. It doesn't eagerly combine at least as currently defined.)

@kentonv

This comment has been minimized.

Copy link

commented Jun 13, 2018

Oh hmm... @harrishancock did we misread this? It looks like append() actually doesn't specify that same-named headers should be combined. It's only on get() that this happens. https://fetch.spec.whatwg.org/#concept-header-list-append

@harrishancock

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

@kentonv This is embarrassing but I think we didn't realize we fixed it while fixing the passthrough case. D:

@kentonv

This comment has been minimized.

Copy link

commented Jun 15, 2018

Hah. Indeed, it turns out that our implementation is correct and append('Set-Cookie', ...) works just fine, but somehow we developed the urban legend that it wouldn't work.

@annevk annevk closed this in #714 Jul 23, 2018
annevk added a commit that referenced this issue Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.