Ignore invalid cookies

[dorfsmay]

In the spirit of Postel's law, ignore invalid cookies rather than
completely discard the entire Cookie header, which is what the current
code does, and which will lead to confusion when dealing with headers
with invalid cookies injected by proxies and intermediate apps servers.

Cookies are injected at multiple different layers between the browser and the end server, proxies, load balancers, security appliances, A/B testing appliances, etc... some of them inject invalid cookies, it's fine to discard those, but in I'm opinion it is wrong to discard every cookie because one field is bad. The separation of cookies between semi-colon makes it easy enough to cleanly separate cookies, ignore the invalid ones, and keep the good ones.

• The commit messages match the guidelines in https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md#git-commit-guidelines

[seanmonstar]

Hm, I can look later, but I'm curious what other libraries do. I'd probably want to keep to whatever others do: either drop the whole thing on the floor, or accept all valid cookies.

[dorfsmay]

Three different results out of three direfferent libraries so far!

https://github.com/dorfsmay/Invalid_cookies_in_Cookie_header

Thinking more about this, if ignoring bad cookies while accepting good ones from a given header is not acceptable by virtue of being rigorous with our inputs, shouldn't we return an http 400 or 422 then? If we're being rigorous to the extreme, isn't discarding the entire header as bad as just ignore what we assume are bad cookies?

Again, genuinely interested, not trying to push my opinions/PR here.

[seanmonstar]

I'm thinking that rejecting the request outright should be up to the user of hyper. They can optionally compare headers.get_raw and headers.get if they want to be sure it parses entirely.

Looking at your linked document (which is awesome, by the way!), I'm kind of scared at how some libraries just accept broken cookies as if they were legit. In reality, though, any application that is used cookies for security purposes should be signing and encrypting them, or they're doomed anyways.

It looks like hyper should:

• Be lenient about a trailing ;
• Either drop the whole header on parse errors, or drop the current ; section. I'm not certain which is better, still.

[dorfsmay]

Yes, very interesting how different libraries behaves fundamentally differently. Sadly, RFC6265, leaves a lot of space for interpretation...

One thing I found out since I have started this document is that it seems that in the early days of the web sites used singular cookie without name, so a simple string without equal sign was interpreted as a value, which Django still supports. I'm surprised by golang behaviour given Google inc involvement in that language and their prominent presence on the web, they use strings with missing equal sign as a name. I'm wondering if this is due to a naive parsing with no consideration for this case, or the result of a conscious choice.

As far as current support for hyper, my thoughts are:

• I agree with being lenient with the trailing semi-colon. I am yet to see a server which does not.
• discarding all cookies because some of them are invalid seems like an optimization gone bad, especially given how cookies can be injected by third parties. Let say your session starts failing, silently, because of this, how do you debug this‽
• I understand how discarding bad cookies only and keeping the good ones could be considered an optimization, or an interpretation, which, although I think is the right thing to do, could lead to unforeseen consequences, but, should I system rely on invalid cookies (or anything) to start with? Again considering how cookies are regularly inject by third parties, which can sometime be difficult, or even impossible to fix, I think this not doing this would be unfair to a client and a server with are using the right format. This is how "express" and "ring", two widely used libraries are doing it.
• Please note how "hapi" (a javascript framework used by some large companies) behaves: It has a very strict interpretation of the RFC, and returns http 400 on anything not following the RFC, including a space between the value and the semi-colon! Although I think this is too strict, I think if good cookies are going to be discarded because of bad ones present, this is how it should be done, a clear failure that forces the issue to be dealt with.

A compromise?
What about having one behaviour by default and an option for the other one. For example, discard bad cookies and keep the good one by default, but have a "strict_rfc_6265 option returning http 400 on malformed Cookie header. If anything, having that option in the docs will highlight the fact that this can be an issue.

[seanmonstar]

It would seem the behavior from Go is just treating a cookie-string as a list of cookie-av; * (cookie-av) from the Set-Cookie behavior. That is what allows Set-Cookie: foo=bar; HttpOnly. Understanding that, I am warming up to that way, actually.

As for application behavior, I don't think hyper should try to be like Hapi. Hapi is a large server framework, trying to do a lot of things for you automatically. hyper should just give the user the information, and let them decide how to react. (Besides, since typed headers are parsed lazily, the time the typed Cookie would know of invalid strings is once the user tries to access it, and generation of the Response is entirely in their hands.)

[dorfsmay]

I don't understand the concern about being able to parse a Set-Cookie header when receiving a Cookie header... Regardless, this would be a much better option than the current behaviour (accepting the request but discard the entire Cookie header).

[seanmonstar]

I think I've found in discussing this that the exact behavior isn't all that important to me. As long as it is documented, and not wrong, any of the options seem fine to me. Anyone who wants a different behavior can create their own type and implement Header.

[dorfsmay]

Do you consider the behaviour from this PR, discarding all the invalid invalid cookies and keep all the valid cookies, wrong?

Is it worth that I add/change the documentation to reflect this behaviour?

[seanmonstar]

No, by wrong I meant that if the parsing were too permissive to the point that a value were to contain other name-values. The behavior in this PR could work.

[seanmonstar]

Thanks for this!