-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
combine multiple headers using comma #1489
Conversation
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.
Thanks a lot for the PR. I've put a couple of comments but we're definitely on the way to fix this!
@@ -19,7 +19,7 @@ export const parseResponseBody = ( | |||
); | |||
|
|||
export const parseResponseHeaders = (headers: Dictionary<string[]>): Dictionary<string> => | |||
mapValues(headers, hValue => hValue.join(' ')); | |||
mapValues(headers, hValue => hValue.join(',')); |
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.
This is probably the good change — but I'd say we only need to apply this on the Set-Cookie
header instead of doing it for all of them. mapValues
is likely offering an overload to also get the key and react accordingly
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.
The reason I didn't initially only special case set-cookie
was because of the wording in rfc2616, as far as I can tell joining with a comma is right thing to do by default - a space would be the exception to the general rule.
I could be wrong, do you have a link to more/newer info about the correct way to combine multiple header fields into one?
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 need to take a deeper look into; the reason is that it seems like such rule applies only for "state" headers that is Cookies, Caching and some other that now I do not remember.
You can leave it as it is. I'll check it out later or tomorrow and then get back to you.
@@ -78,7 +78,7 @@ describe('parseResponseHeaders()', () => { | |||
expect(parseResponseHeaders({ h1: ['a b'], h2: ['c'], h3: ['a', 'b'] })).toEqual({ | |||
h1: 'a b', | |||
h2: 'c', | |||
h3: 'a b', | |||
h3: 'a,b', |
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.
We'll also have to write a more specific test to make sure it only happens only under specific conditions.
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.
Ok I've reviewed the entire thing now. While it won't fix a probable issue with Set-Cookie
with commas, it should still do the job for now. Thanks
This addresses issue #1190 (Multiple Set-Cookie headers get aggregated)
https://tools.ietf.org/html/rfc2616#section-4.2
I searched newer RFCs and as far as I can tell the above still holds true
One thing to note, is this:
https://tools.ietf.org/html/rfc6265#section-3
With that in mind (and some individual header spesific things outlined in other RFCs) it may be better to rework how these headers flow through the application so that multiple headers do not need to be serialized.
For now joining with a comma fixes the
set-cookie
issue and seems to be more correct than joining with a space