fix(event-handler): handle set-cookie header values with multiple attributes#4990
fix(event-handler): handle set-cookie header values with multiple attributes#4990nateiler wants to merge 3 commits intoaws-powertools:mainfrom
Conversation
boring-cyborg
bot
added
event-handler
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
| // Handle set-cookie headers specially using getSetCookie() | ||
| const cookies = webHeaders.getSetCookie(); | ||
| // Some legacy implementations may concatenate multiple set-cookie values with commas | ||
| // Split them out while preserving the cookie attributes (which use semicolons) | ||
| const allCookies: string[] = []; | ||
| for (const cookie of cookies) { | ||
| allCookies.push(...cookie.split(',').map((v) => v.trimStart())); | ||
| } | ||
|
|
||
| if (allCookies.length > 1) { | ||
| multiValueHeaders['set-cookie'] = allCookies; | ||
| } else if (allCookies.length === 1) { | ||
| headers['set-cookie'] = allCookies[0]; | ||
| } | ||
|
|
||
| for (const [key, value] of webHeaders.entries()) { | ||
| // Skip set-cookie as it's already handled above |
There was a problem hiding this comment.
Let's remove these comments please
| if (headers[key]) { | ||
| multiValueHeaders[key] = [headers[key], ...values]; | ||
| delete headers[key]; | ||
| } else if (values.length > 1) { | ||
| if (values.length > 1) { |
There was a problem hiding this comment.
Why are we removing this logic?
There was a problem hiding this comment.
To my knowledge set-cookie is the only header that can have multiple keys. Other headers, when appended to, will append the value to the end via a comma. This logic would never be evaluated (which was verified through the code coverage)
There was a problem hiding this comment.
There's a lot of diffs that seem to be unrelated to this PR - any chance you could revert?
There was a problem hiding this comment.
Ah, yeah. Looks like my own biome got the best of me.
There was a problem hiding this comment.
Actually, it looks like these changes were automatically made via lint-staged commit hook. After reverting them and attempting to commit, my revert gets stashed and I'm left with an empty commit which is prevented.
| // Prepare | ||
| const headers = new Headers({ | ||
| 'set-cookie': 'session=abc123; theme=dark', | ||
| 'cache-control': 'no-cache; no-store', |
There was a problem hiding this comment.
How come we're changing these?
There was a problem hiding this comment.
set-cookie is a header that shouldn't be separated by a semicolon. Keeping it as it were would result in a single set-cookie header with an unsupported 'theme' attribute which would cause the test to fail and ultimately seems to defeat the purpose of the test. cache-control represents a normal header that can be separated by a semicolon. Other tests cover the nuances of 'set-cookie'.
| headers: { 'content-type': 'application/json' }, | ||
| multiValueHeaders: { 'set-cookie': ['session=abc123', 'theme=dark'] }, | ||
| multiValueHeaders: { | ||
| 'cache-control': ['no-cache', 'no-store'], |
There was a problem hiding this comment.
Where are these cache-control headers coming from?
There was a problem hiding this comment.
I added them to illustrate a 'simple' multi-value header pair along with the set-cookie multi-value header which contains a semicolon.
| continue; | ||
| } | ||
|
|
||
| const values = value.split(/[;,]/).map((v) => v.trimStart()); |
There was a problem hiding this comment.
Ultimately, splitting on a semicolon seems out of place and is the root of this entire issue. If a goal wasn't to keep backwards compatibility I would suggest removing it and using a comma only. Headers by default a separated by a comma.
const headers = new Headers({
'accept': 'application/json'
})
headers.append('accept', 'text/html');
const acceptHeaders = headers.get('accept'); // Results in "application/json, text/html"There was a problem hiding this comment.
So the reason I wrote it this way was because I was under the impression that cookies were an exception in that they could be separated by a semicolon. I got this from the RFC 6265:
The user agent sends stored cookies to the origin server in the
Cookie header. If the server conforms to the requirements in
Section 4.1 (and the user agent conforms to the requirements in
Section 5), the user agent will send a Cookie header that conforms to
the following grammar:cookie-header = "Cookie:" OWS cookie-string OWS
cookie-string = cookie-pair *( ";" SP cookie-pair )
It looks like I made a mistake and thought this referred to the Set-Cookie header rather than the Cookie header as indicated in the quote above.
There was a problem hiding this comment.
What this means though is that to handle the Cookie header case correctly we do need to have special handling for the semicolon. Btw, regarding this:
If a goal wasn't to keep backwards compatibility
We don't need to preserve backward compatibility for what was a bug. If we change this so only Cookie gets this handling and that's fine.
There was a problem hiding this comment.
Thanks for the references to the RFC. In this particular circumstance the webHeadersToApiGatewayV1Headers function is converting from server to user agent (not also user agent to server) so we should be able to omit special handling of the Cookie header as per RFC6265 4.2 the Cookie header is a user agent to server header.
It seems like the semicolon was included to handle the special usage of the 'Cookie' header which we don't actually need here.
We don't need to preserve backward compatibility for what was a bug. If we change this so only Cookie gets this handling and that's fine.
I see your point here; however I think only set-cookie with a semicolon is a bug. Since the current implementation accepts any header with a semicolon separated value (while it may not be popular) removing it might break current implementations.
I'm unsure of the best path here. Ultimately it seems like dropping the semicolon would simplify and unify the usage across the board (all headers are treated the same).
There was a problem hiding this comment.
The issue here is larger though, splitting on ; causes worse problems. The accepted way to inidicate parameters in a header is with a semi colon, e.g., Accept: text/html; q=0.5, text/plain; q=0.2. With the current implementation, this ends up with a multivalue header value which is clearly wrong:
// ...
multiValueHeaders: {
accept: [
"text/html",
"q=0.5",
"text/plain",
"q=0.2",
]
}
// ...It's far more important that we handle this case correctly than headers that aren't Cookie erroneously delimited by ;.
|
3 participants
Summary
This fixes the scenario where a single
set-cookieheader valuefoo=bar; Max-Age=3600is treated as twoset-cookievalues because multiple values may be separated by a;.Changes
When parsing the headers,
set-cookieheaders are handled using the getSetCookie method.Logic to detect multiple header entries has been removed as multiple header values are appended to a single header (via comma).
Issue number: closes #4986
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.