-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
WIP: Allow multiple set-cookie headers #4829
Conversation
Just a tiny change, no reason why it should break, and only break on ubuntu... |
Before:
After:
|
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 @Caesar2011 - I wonder if you could include a test?
Sure, could you give me a short instruction how or at least where I can find similar tests? Edit: I think I've got an idea.... Wait.... :D |
} else { | ||
value.split(/(?<!; expires\=\w+), /).forEach((v: string) => { | ||
out += `${key}: ${v}\r\n`; | ||
}); |
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 don't think this regex is the optimal way of achieving this... I don't think any extra parsing should be necessary to write out the response.
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 am with you. No extra parsing should be necessary. But using the Headers construction with string[][]
as parameter multiple headers get concatenated with ,
. You need some way to split it again, but not split within the expire value. (See comment in associated issue)
Do you have any suggestions/inspirations?
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 found the same issue yesterday and started working on it, didn't know there was already work being done. I have a working implementation with a different approach, I'll reference this PR.
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.
Closed due to better approach in #4840 |
Fixes #4826. The separator
,
is only allowed as a separator withinSet-Cookie
but not within other headers. This fix only works forSet-Cookie
, the only standard-complient header witch is allowed to be set multiple times. Should this work for custom headers, too?