Missing Set-Cookie headers when more than one is present in the same response

[GoogleCodeExporter]

CefRequestHandler::OnResourceResponse() allows the user to inspect response 
headers. Sometimes, a single HTTP response header can contain multiple header 
lines with the same field-name but different values, Set-Cookie being one 
prominent and often encountered example.

OnResourceResponse() naturally uses the CefResponse::HeaderMap type to convey 
the contents of the response header to the observer. The problem is however, 
that the underlying type for both CefResponse::HeaderMap and 
CefRequest::HeaderMap is std::map<CefString, CefString>. This arrangement means 
that each entry needs to have a unique key (or field-name, in our case). Thus, 
multiple Set-Cookie lines in the same HTTP response result in only the last one 
being preserved and passed on for inspection.

One solution is to combine header lines with the same name into one 
comma-separated string. Indeed, this situation is explicitly enumerated in 
RFC2616 4.2:

  "Multiple message-header fields with the same field-name MAY be
   present in a message if and only if the entire field-value for that
   header field is defined as a comma-separated list [i.e., #(values)].
   It MUST be possible to combine the multiple header fields into one
   "field-name: field-value" pair, without changing the semantics of the
   message, by appending each subsequent field-value to the first, each
   separated by a comma."

We would be justified to assume that since it is quite common to have more than 
one Set-Cookie line in a response header, it should therefore be fine to just 
combine them as instructed in the RFC. And we would be wrong. 

The non-standard but widely endorsed original Netscape cookie specification 
allows the cookie expiry date to be given as "DayOfWeek, DDD-MMM-YY HH:mm::ss 
GMT" which would have been fine had it not been for the comma after DayOfWeek.

Even when combined, multiple Set-Cookie values can be successfully parsed with 
the proper regular expression, but aside from being error-prone, I feel this 
puts unnecessary burden on the user.

My suggestion is to change the underlying type of 
CefRequest/Response::HeaderMap to std::multimap<,> as this is the only 
semantically correct type according to RFC2616. This change will require minor 
modifications to be made to existing client code, mostly because the operator[] 
will no longer be available. I am willing to do the work on this, but I need to 
hear opinions and to gauge the likelihood of the resulting patch making it into 
trunk.

Original issue reported on code.google.com by yyankov on 24 Oct 2011 at 1:11

[GoogleCodeExporter]

Your std::multimap suggestion sounds reasonable. Be aware of the following 
implementation considerations beyond the required C++ changes:

1. Implement a C API for the multimap type. This would be similar in concept to 
the cef_string_map implementation.

2. Add std::multimap parsing support to tools/cef_parser.py.

3. Create a unit test to verify the functionality.

Patches welcome.

Original comment by magreenb...@gmail.com on 24 Oct 2011 at 2:13

[GoogleCodeExporter]

Good advice. I have already considered 1. and 3. Thanks for bringing 2. to my 
attention! I'll start working on a patch ASAP.

Original comment by yyankov on 24 Oct 2011 at 2:19

[GoogleCodeExporter]

Here's the patch against r342. Hopefully not too many people will hate me for 
it, as it  makes dealing with request/response headers a wee bit more involved.

Original comment by yyankov on 27 Oct 2011 at 6:17

Attachments:

• header_multimap_r342.patch

[GoogleCodeExporter]

Great works, thanks! Committed as revision 346 with the following minor changes:

1. Remove the unused cef_string_multimap_iter_t typedef from 
cef_string_multimap.h
2. Fix Mac compile errors in string_unittest.cc
3. Fix minor formatting errors.

Original comment by magreenb...@gmail.com on 28 Oct 2011 at 9:32

• Changed state: Fixed

[GoogleCodeExporter]

Compiling on Linux with GCC 4.6 causes errors:

cef/libcef/cef_string_multimap.cc: In function ‘int 
cef_string_multimap_enumerate(cef_string_multimap_t, const cef_string_t*, int, 
cef_string_t*)’:
cef/libcef/cef_string_multimap.cc:49:12: error: converting to non-pointer type 
‘int’ from NULL [-Werror=conversion-null]
cef/libcef/cef_string_multimap.cc:59:12: error: converting to non-pointer type 
‘int’ from NULL [-Werror=conversion-null]
cef/libcef/cef_string_multimap.cc: In function ‘int 
cef_string_multimap_key(cef_string_multimap_t, int, cef_string_t*)’:
cef/libcef/cef_string_multimap.cc:73:12: error: converting to non-pointer type 
‘int’ from NULL [-Werror=conversion-null]
cef/libcef/cef_string_multimap.cc: In function ‘int 
cef_string_multimap_value(cef_string_multimap_t, int, cef_string_t*)’:
cef/libcef/cef_string_multimap.cc:90:12: error: converting to non-pointer type 
‘int’ from NULL [-Werror=conversion-null]
cc1plus: all warnings being treated as errors

Original comment by fdd...@gmail.com on 28 Oct 2011 at 9:38

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Fix for this problem:

Original comment by fdd...@gmail.com on 28 Oct 2011 at 9:43

Attachments:

• cef_string_multimap_conversion_null_r346.patch

[GoogleCodeExporter]

For the life of me I cannot figure out what I was thinking returning NULL 
instead of 0 :) Sorry about that. That's why nullptr should have been invented 
ages ago.

Original comment by yyankov on 28 Oct 2011 at 9:56

[GoogleCodeExporter]

@comment#7: Thanks, fixed in revision 347.

Original comment by magreenb...@gmail.com on 31 Oct 2011 at 3:27