Bug 60876

Summary: Rfc6265CookieProcessor: syntax of Set-Cookie header deviates from spec
Product: Tomcat 8 Reporter: Jim Griswold <jim.griswold>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal CC: jim.griswold, viveka.singh
Priority: P2    
Version: 8.5.11   
Target Milestone: ----   
Hardware: All   
OS: All   
Attachments: Patch with proposed changes

Description Jim Griswold 2017-03-16 14:28:16 UTC
Summary:

After a recent upgrade from Tomcat 8.0.28 to 8.5.11, I've noticed a syntax change in the Set-Cookie header generated by Tomcat on HTTP responses where a cookie is set with additional attributes such as "Path". From my investigation thus far, the syntax change appears to be due to implementation details of Rfc6265CookieProcessor, which was changed to be the default cookie processor in between the Tomcat versions I tested. The syntax change does not appear to be compliant with the syntax in RFC-6265, and this is change is breaking at least one fairly common HTTP client implementation (Apache CXF).

Example:

When I set a cookie named "cookie_name" with value "value" and path "/", the header that's generated by Tomcat 8.0.28 looks like:

    Set-Cookie: cookie_name=value; Path=/

With 8.5.11, it looks like:

    Set-Cookie: cookie_name=value;path=/

Note the missing space after the semicolon and the change from "Path" to "path".

Impact:

This small change appears to break at least the Apache CXF WebClient's implementation of parsing the Set-Cookie header, which is how I noticed the issue.

In my testing, the CXF client discards the fact that a cookie called "cookie_name" has been set. I tried upgrading the CXF client to see if a newer version would be more tolerant, but the issue persisted.

Possible root cause / suggested solution:

After some digging around, I saw that the new Rfc6265CookieProcessor was changed to be the default cookie processor. When I followed instructions [1] to change back to the old processor, the original behavior was restored and my tests passed again.

The RFC 6265 specifies [1] that there must be a space ("SP") between the semicolon and "Path", and that it should be "Path" with the first letter uppercased. Taking a look at the Rfc6265CookieProcessor source (the generateHeader method, specifically), I see the following:

    153: header.append(";path=");

This appears to be a broader issue that impacts elements other than "path", e.g.

    123: header.append(";Max-Age=");  // should be: "; Max-Age="
    131: header.append (";Expires="); // should be: "; Expires="
    146: header.append(";domain=");   // should be: "; Domain="
    158: header.append(";Secure");    // should be: "; Secure"
    162: header.append(";HttpOnly");  // should be: "; HttpOnly"

While I think clients should tolerate the small change, I think it's best for the cookie processor to adhere to the RFC more strictly in this case in order to avoid unexpected issues.

Thank you very much for all of your great work on this project!

[1] https://tools.ietf.org/html/rfc6265#section-4.1.1
[2] http://stackoverflow.com/questions/38696081/how-to-change-cookie-processor-to-legacycookieprocessor-in-tomcat-8

Java version:

java version "1.8.0_91"
Java(TM) SE Runtime Environment (build 1.8.0_91-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.91-b14, mixed mode)
Comment 1 Jim Griswold 2017-03-16 17:46:05 UTC
Created attachment 34836 [details]
Patch with proposed changes

Adding patch.
Comment 2 Mark Thomas 2017-03-16 21:21:57 UTC
Thanks for the report, the analysis and the patch. I hope it is the first of many.

Fixed in:
- trunk for 9.0.0.M19 onwards
- 8.5.x for 8.5.13 onwards
- 8.0.x for 8.0.43 onwards

I also fixed the test cases that started failing once this issue was fixed.