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

set-cookie folding #62

Open
acarioni opened this issue May 4, 2017 · 6 comments
Open

set-cookie folding #62

acarioni opened this issue May 4, 2017 · 6 comments

Comments

@acarioni
Copy link

acarioni commented May 4, 2017

The library folds multiple set-cookie headers using commas to separate them, but according to RFC 6265 section 3 this behavior is deprecated.

@jcoglan
Copy link
Collaborator

jcoglan commented May 4, 2017

Can you clarify exactly what you're referring to, maybe giving an example of a program that doesn't do what you expect?

This library doesn't do anything specific with regard to cookie headers so I just wasn't sure what you're getting at.

@acarioni
Copy link
Author

acarioni commented May 5, 2017

Hi,

for example suppose that the server sent the following headers:

Set-Cookie: id=a3fWa; Expires=Wed, 21 Oct 2015 07:28:00 GMT
Set-Cookie: qwerty=219ffwef9w0f

Then your library folds the two headers into this one (as returned by the headers property):

Set-Cookie: id=a3fWa; Expires=Wed, 21 Oct 2015 07:28:00 GMT, qwerty=219ffwef9w0f

Note that I can't simply split the cookies using the comma as separator because I obtains this result

['id=a3fWa; Expires=Wed', '21 Oct 2015 07:28:00 GMT', 'qwerty=219ffwef9w0f'] which doesn't represent valid cookies.

I suppose this is one of the reasons why RFC 6265 advises against the cookie folding.

@jcoglan
Copy link
Collaborator

jcoglan commented May 20, 2017

I've spent a while looking into this, refreshing my understanding and looking at possible solutions. Here's what I have so far.

The behaviour you're observing is coming from websocket-driver, in its wrapper for Node's HTTP parser. I could change that behaviour, in one of several ways:

  • If a header appears multiple times, assign an array value for it, rather than a string
  • Assign an array for any header that's allowed to appear multiple times, regardless of whether it does or not
  • Assign an array for all cookies, i.e. don't assume anything about the semantics of particular field names

All of these feel problematic to me and will break some existing code, and will also mean downstream code must become more complex. Rather than each header value definitely being a string if it exists, it might also be an array. That means client code would need to check the type or do something like [].concat(value) before iterating. If we make the types consistent by making everything an array, then all current code relying on websocket-driver will definitely break. The second option also raises the question of whether a header value should be an empty array or absent if the field does not appear at all. All these options feel ergonomically awkward to me.

A different option would be to bake cookie handling into this library, or into websocket-driver. Either this library picks a cookie jar to use, or you pass one in -- the latter would probably be necessary to maintain state between client instances or if you wanted to share a cookie jar with other HTTP stuff you're doing. That is more appealing to me since it removes the burden of handling strings from the user, but it introduces another problem, namely that this library now either needs to know how to parse cookie headers or delegate that to a cookie jar library.

However, the bigger problem is that none of these will solve the actual problem. RFC 2616 says field-names can appear multiple times if and only if their value is equivalent to a comma-separated list; if a field appears multiple times clients and proxies are allowed to fold them as done here and expect the same result. So what this client is doing is legal, and also it might not even be the one responsible for the folding: the server or a proxy might fold the headers in this way before the client sees them.

So the actual problem is that any code parsing a set-cookie header needs to take into account that multiple cookies may appear in the same header line. Parsing by splitting on commas is a bad strategy, just as splitting on semicolons is bad because the cookie's value may be quoted and contain a semicolon itself. You actually need a more robust parser that understands how to deal with value quoting, date formatting, and multiple field values.

There exist cookie libraries that handle this cleanly, for example the cookiejar library that I use in the Ruby version of Faye does this correctly. Apparently, tough-cookie (which I use for the Node version of Faye) does not; it parses the id cookie correctly including the expiry date, but then ignores the qwerty cookie. I will open an issue on tough-cookie enquiring about this.

What I recommend you do is use a cookie library instead of parsing these things yourself, and use one that supports multiple field values.

@jcoglan
Copy link
Collaborator

jcoglan commented May 20, 2017

Here's the issue I've opened: salesforce/tough-cookie#88

@jcoglan
Copy link
Collaborator

jcoglan commented Aug 1, 2017

Just an update that I recently changed the HTTP parser in websocket-driver to use http-parser-js rather than Node's built-in HTTP parser, but the above all holds true. I'd appreciate any feedback that you have.

@jcoglan
Copy link
Collaborator

jcoglan commented May 14, 2020

salesforce/tough-cookie#88 was closed as wontfix. I was wondering whether you had any feedback on my remarks above, @acarioni?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants