Created attachment 37299 [details] test code (Tested in both 9.0.33 and 9.0.36, but 9.0.36 wasn't available in the drop-down when I opened this bug report.) Rfc6265CookieProcessor tries to accept a comma as a cookie pair separator for $Version=1 cookie headers, but it gets it wrong. It also behave differently from the legacy parser. For this Cookie: header value $Version=1;first=1,second=2;third=3;case=justCOMMA the new parser loses cookies "first" and "second" (logs an invalid cookie warning). The legacy parser does not. Here is a small test program (also attached) that parses headers with both processors. The test values include both "$Version=1" cookie headers and the same with no version attribute. I believe the RFC-6265 parsing is not trying to honor the comma, which is fair enough. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> package aside; import org.apache.tomcat.util.http.LegacyCookieProcessor; import org.apache.tomcat.util.http.MimeHeaders; import org.apache.tomcat.util.http.Rfc6265CookieProcessor; import org.apache.tomcat.util.http.ServerCookie; import org.apache.tomcat.util.http.ServerCookies; public class TC9CookieParsingTest { private static final String[] cookieHeaderValues = { "$Version=1;first=1;second=2;third=3;case=justSEMI", "first=1;second=2;third=3;case=justSEMI", "$Version=1;first=1,second=2;third=3;case=justCOMMA", "first=1,second=2;third=3;case=justCOMMA", "$Version=1;first=1,;second=2;third=3;case=COMMAthenSEMI", "first=1,;second=2;third=3;case=COMMAthenSEMI", "$Version=1;first=1;,second=2;third=3;case=SEMIthenCOMMA", "first=1;,second=2;third=3;case=SEMIthenCOMMA", }; public static void main(String[] args) { for (final String cookieHeaderValue: TC9CookieParsingTest.cookieHeaderValues) { TC9CookieParsingTest.parseWithRfc6265Parser(cookieHeaderValue); TC9CookieParsingTest.parseWithLegacyParser(cookieHeaderValue); } } private static void parseWithRfc6265Parser(final String cookieHeaderValue) { final MimeHeaders headers = new MimeHeaders(); headers.addValue("Cookie").setString(cookieHeaderValue); final ServerCookies serverCookies = new ServerCookies(10); final Rfc6265CookieProcessor processor = new Rfc6265CookieProcessor(); processor.parseCookieHeader(headers, serverCookies); final int howMany = serverCookies.getCookieCount(); System.out.println("\n====================\nRfc6265CookieProcessor 'Cookie: " + cookieHeaderValue + "'"); for (int ii=0; ii<howMany; ++ii) { final ServerCookie sc = serverCookies.getCookie(ii); System.out.println(ii + " cookie: '" + sc.getName() + "' = '" + sc.getValue() + "'"); } } private static void parseWithLegacyParser(final String cookieHeaderValue) { final MimeHeaders headers = new MimeHeaders(); headers.addValue("Cookie").setString(cookieHeaderValue); final ServerCookies serverCookies = new ServerCookies(10); final LegacyCookieProcessor processor = new LegacyCookieProcessor(); processor.parseCookieHeader(headers, serverCookies); final int howMany = serverCookies.getCookieCount(); System.out.println("--------------------\nLegacyCookieProcessor 'Cookie: " + cookieHeaderValue + "'"); for (int ii=0; ii<howMany; ++ii) { final ServerCookie sc = serverCookies.getCookie(ii); System.out.println(ii + " cookie: '" + sc.getName() + "' = '" + sc.getValue() + "'"); } } } >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Here is the output from a run of the test program: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ==================== Rfc6265CookieProcessor 'Cookie: $Version=1;first=1;second=2;third=3;case=justSEMI' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'justSEMI' -------------------- LegacyCookieProcessor 'Cookie: $Version=1;first=1;second=2;third=3;case=justSEMI' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'justSEMI' ==================== Rfc6265CookieProcessor 'Cookie: first=1;second=2;third=3;case=justSEMI' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'justSEMI' -------------------- LegacyCookieProcessor 'Cookie: first=1;second=2;third=3;case=justSEMI' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'justSEMI' ==================== Rfc6265CookieProcessor 'Cookie: $Version=1;first=1,second=2;third=3;case=justCOMMA' 0 cookie: 'third' = '3' 1 cookie: 'case' = 'justCOMMA' -------------------- LegacyCookieProcessor 'Cookie: $Version=1;first=1,second=2;third=3;case=justCOMMA' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'justCOMMA' ==================== Rfc6265CookieProcessor 'Cookie: first=1,second=2;third=3;case=justCOMMA' 0 cookie: 'third' = '3' 1 cookie: 'case' = 'justCOMMA' -------------------- LegacyCookieProcessor 'Cookie: first=1,second=2;third=3;case=justCOMMA' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'justCOMMA' ==================== Rfc6265CookieProcessor 'Cookie: $Version=1;first=1,;second=2;third=3;case=COMMAthenSEMI' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'COMMAthenSEMI' -------------------- LegacyCookieProcessor 'Cookie: $Version=1;first=1,;second=2;third=3;case=COMMAthenSEMI' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'COMMAthenSEMI' ==================== Rfc6265CookieProcessor 'Cookie: first=1,;second=2;third=3;case=COMMAthenSEMI' 0 cookie: 'second' = '2' 1 cookie: 'third' = '3' 2 cookie: 'case' = 'COMMAthenSEMI' -------------------- LegacyCookieProcessor 'Cookie: first=1,;second=2;third=3;case=COMMAthenSEMI' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'COMMAthenSEMI' ==================== Rfc6265CookieProcessor 'Cookie: $Version=1;first=1;,second=2;third=3;case=SEMIthenCOMMA' 0 cookie: 'first' = '1' 1 cookie: 'third' = '3' 2 cookie: 'case' = 'SEMIthenCOMMA' -------------------- LegacyCookieProcessor 'Cookie: $Version=1;first=1;,second=2;third=3;case=SEMIthenCOMMA' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'SEMIthenCOMMA' ==================== Rfc6265CookieProcessor 'Cookie: first=1;,second=2;third=3;case=SEMIthenCOMMA' 0 cookie: 'first' = '1' 1 cookie: 'third' = '3' 2 cookie: 'case' = 'SEMIthenCOMMA' -------------------- LegacyCookieProcessor 'Cookie: first=1;,second=2;third=3;case=SEMIthenCOMMA' 0 cookie: 'first' = '1' 1 cookie: 'second' = '2' 2 cookie: 'third' = '3' 3 cookie: 'case' = 'SEMIthenCOMMA' >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I've had a quick look at the parser code. I suspect the problem lines in org.apache.tomcat.util.http.parser.Cookie, near line 274: skipResult = skipByte(bb, COMMA_BYTE); if (skipResult == SkipResult.FOUND) { parseAttributes = false; } skipResult = skipByte(bb, SEMICOLON_BYTE); if (skipResult == SkipResult.EOF) { parseAttributes = false; moreToProcess = false; } else if (skipResult == SkipResult.NOT_FOUND) { skipInvalidCookie(bb); continue; } Even though we've just found the COMMA_BYTE, it's still a failure if we don't then fint the SEMICOLON_BYTE. I suspect that wanted to be an "else if", but since the parser is kind of complicated I hesitate to claim that's a definitive fix. It might break some other edge case.
Thanks for the reminder. I've added 9.0.36 to the list of versions and updated the version for this issue. I'll look at the detail of the report next.
Fixed in: - master for 10.0.0-M7 onwards - 9.0.x for 9.0.37 onwards - 8.5.x for 8.5.57 onwards 7.0.x is not affected. Thanks for the report. You were right about the location of the bug. There were a couple of other places the same bug was present. I've fixed them an added a parameterised test case that should test all combinations.
Thanks for the quick action on this.