set-cookie folding
acarioni opened this issue · 6 comments
The library folds multiple set-cookie headers using commas to separate them, but according to RFC 6265 section 3 this behavior is deprecated.
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.
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.
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.
Here's the issue I've opened: salesforce/tough-cookie#88
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.
salesforce/tough-cookie#88 was closed as wontfix. I was wondering whether you had any feedback on my remarks above, @acarioni?