Properly copy headers to preserve duplicate set-cookie headers. #872
Conversation
Browser have a lot of bugs in relation to parsing concatenated set-cookie headers. This fixes that.
evanderkoogh
requested review from
GregBrimble,
threepointone and
petebacondarwin
as code owners
|
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2256106421/npm-package-wrangler-872You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/872/npm-package-wrangler-872Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2256106421/npm-package-wrangler-872 dev path/to/script.js |
| @@ -123,7 +161,7 @@ export default { | |||
| // https://fetch.spec.whatwg.org/#null-body-status | |||
| return new Response( | |||
| [101, 204, 205, 304].includes(response.status) ? null : response.body, | |||
| { ...response, headers: new Headers([...response.headers.entries()]) } | |||
| { ...response, headers: copyHeaders(response.headers as Headers) } | |||
There was a problem hiding this comment.
I actually think we can simplify this. I must have been tired when I first wrote this, because apparently just doing new Headers(response.headers) is enough to correctly make a mutable copy of headers.
As explained to me by the ever-helpful champs: https://discord.com/channels/595317990191398933/910978223968518144/970105175924690994
There was a problem hiding this comment.
| { ...response, headers: copyHeaders(response.headers as Headers) } | |
| { ...response, headers: new Headers(response.headers) } |
There was a problem hiding this comment.
the new way:
vs. the old way:
Browser have a lot of bugs in relation to parsing concatenated set-cookie headers. This fixes that.
It used the new
getAll()method which can be used with theset-cookieheader to get an array back. And when using.setand.append, the Worker runtime will emit multiple headers.This fixes the bug with multiple
set-cookieheaders that was introduced by #796.Unfortunately I could not add a test case to the end-to-end tests, because the udici library has no support for
getAll(), so I can't determine if theset-cookieheaders were concatenated or not.