Jetty.project: Cookies parsing in RFC2965 should allow deprecated comma separators

[jborik5]

When multiple Cookies are sent in one Cookie header (separated by comma), they are not parsed correctly.

Example:

Cookie: $Version="1"; COOKIE1="ABC", $Version="1"; COOKIE2="123"

A real use case for this header is:

• Java client using commons-httpclient sends request which encodes each cookie as a separate header.
• The request goes through the Apache Httpd server which merges multiple cookie headers into the one as it is seen above.

I wrote a reproducer which you can find in the repo - https://github.com/jborik5/jetty-cookies-issue-reproducer

Originally I thought that this is an issue in CookieCutter implementation but I was told in #3039 that splitting by comma character should be done in HttpParser.

[sbordet]

Sending cookies in separate Cookie headers is against the specification:

When the user agent generates an HTTP request, the user agent MUST NOT attach more than one Cookie header field.

[jborik5]

I see. I have no more arguments for defending this as issue. My only argument is that it works on Weblogic :)

If you don't plan to fix it, feel free to close the issue.

[sbordet]

@jborik5 well since you have a reproducer from common libraries, we may change our mind - seems like a case that may not be that rare.

[gregw]

From rfc2965:

Note: For backward compatibility, the separator in the Cookie header
   is semi-colon (;) everywhere.  A server SHOULD also accept comma (,)
   as the separator between cookie-values for future compatibility

So we should support commas!

[jborik5]

The RFC 2965 says:

cookie = "Cookie:" cookie-version 1*((";" | ",") cookie-value)
cookie-value = NAME "=" VALUE [";" path] [";" domain] [";" port]
cookie-version = "$Version" "=" value
NAME = attr
VALUE = value
path = "$Path" "=" value
domain = "$Domain" "=" value
port = "$Port" [ "=" <"> value <"> ]

So the parameters like path, domain or port can be separated only by semi-colon (;).

Just side note: I was looking at Wildfly source code to see how they handle it and they have configuration parameter which enables comma as separator. If it is enabled, the comma has very same meaning as semi-colon.

[gregw]

The bnf in the rfc also only had semicolon between value pairs, so it is tempting to take the simple solution as wildfly has and make comma and semicolon interchangeable.

But i think it more correct to only accept commas between different cookies.

[jborik5]

Have you seen my PR #3039? The comma is conditioned by CookieCompliance.RFC2965, but it can be easily changed to support it always.

We could also reconsider the compliance parameter. I think it would be better if the parser figured out automatically which cookie format it is parsing.

[joakime]

commons-httpclient version 3.x series is the only one that has this behavior.
That is a very OLD series of clients that by default only supported Netscape original and RFC2109.
There are ways with that old client to force RFC2965 or "Browser" behavior.
See http://hc.apache.org/httpclient-3.x/cookies.html
and http://httpcomponents.10934.n7.nabble.com/Cookies-with-a-comma-in-the-value-td12676.html

If we do an option to allow comma, it should be limited to RFC2965 CookieCompliance support.

https://tools.ietf.org/html/rfc2965#page-13

   Note: For backward compatibility, the separator in the Cookie header
   is semi-colon (;) everywhere.  A server SHOULD also accept comma (,)
   as the separator between cookie-values for future compatibility.

The above language was removed in RFC6265 and the comma was marked as excluded in the value.

https://tools.ietf.org/html/rfc6265#page-9

 cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash

[jborik5]

If we do an option to allow comma, it should be limited to RFC2965 CookieCompliance support.

This behavior is implemented in my PR #3039. Can I reopen it?

[joakime]

PR #3039 doesn't work, as the standard HTTP 1.1 field parsing behaviors will split the Cookie Field Value by the commas for separate Cookies.
We need a solution that either works with the raw Cookie Field Value (unsplit), or a feature in the HTTP parser that ignores commas in a Field Value for Cookies?

[jborik5]

But the Cookie header is not split by comma. The CookieCutter receives the raw string with comma. At least on Jetty version which is used in my test.

If the header was split as you describe, it would solve the reported case. The question is why it is not split. Whether it is bug or feature.

My implementation of CookieCutter handles also raw Cookie Field Value (unsplit).

[gregw]

I'm working on this. I'm not using #3039 because I'm very cautious about large behaviour changes as we have lots of leniency (when allowed by 6265) that we allow to try to work out if we have been given an old cookie.... which is kind of what you have suggested. But we can't be too lenient without opening ourselves up for attacks.

[gregw]

I have created a PR that I think fixes this for RFC2965 compliance mode. The code is indeed overly complex and feels very fragile.... but the whole Cookie standards thing has been a slow motion train wreck over the decades, so it is complex to have a balance between compliance and leniency.

@joakime @sbordet and @jborik5 give me an extra tough review on this one... it's 3am here and I'm only working because I'm jet lagged, so I've probably just made fragile code even more brittle

[jborik5]

@gregw many thanks :)

[gregw]

I have merged the support for commas in 2965, but I will leave this open as I think we need separate compliance modes for generation and parsing.

[jborik5]

Will you port these changes also into the 10.0.x branch?

[gregw]

yes, most work in 9.4 is merged forward to 10

[joakime]

Closing as merged.