-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Support for concatenation of headers #4864
Conversation
If the server returns several headers of the same key (e.g Set-Cookie) only the last one is returned, causing issues in communicating with some servers where cookies are required. This change concatenates the headers of the same key separated by ";" to alleviate this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine with the one comment. Do you have a public site that has this behavior to test against?
@@ -1026,7 +1026,7 @@ int HTTPClient::handleHeaderResponse() | |||
|
|||
for(size_t i = 0; i < _headerKeysCount; i++) { | |||
if(_currentHeaders[i].key.equalsIgnoreCase(headerName)) { | |||
_currentHeaders[i].value = headerValue; | |||
_currentHeaders[i].value += headerValue + ";"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we follow the HTTP spec and use a ,
instead for concatenation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on the ,
A comma is definitely the expected concatenation character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also looks like this is going to mess up the default, single-header case by adding an illegal/unneeded ";" (or "," once fixed to match RFC 2616) to all values.
Change ";" to "," to match the HTTP specs. Add ",<nextval>" on each additional value instead of adding a comma automatically after every value, even single ones.
Verified multiple |
I'll let this go as-is, but it doesn't actually help for cookies proper. To do it right and support multiple cookies, we will need to have this call return an iterator and potentially a whole series of individual strings for parsing. It's doable, but not something I can fix in the online editor. For example, see the following header set from GOOG:
|
If the server returns several headers of the same key (e.g Set-Cookie) only the last one is returned, causing issues in communicating with some servers where cookies are required.
This change concatenates the headers of the same key separated by ";" to alleviate this issue