Skip to content
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

fix(fetch): fix headers.entries/values/forEach iteration for Set-Cookie headers #89

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

MichaelDeBoey
Copy link
Contributor

See @brophdawg11's remix-run#39

It looks like the headers.entries/headers.values/headers.forEach logic we inherited when we forked isn't quite spec compliant when it comes to the Set-Cookie header. Our current implementation blindly returns this.get(headerName) in these iterators which concatenates multiple values, but Set-Cookie is special and should instead treat each value as a standalone entry (internally via looping over getAll).

Notice how in these examples, the custom header values are combined and comma-delimited, but Set-Cookie is not.

Here's this behavior in Chrome:

Screenshot 2023-08-14 at 4 31 19 PM And in node v18:
Welcome to Node.js v18.17.0.
Type ".help" for more information.
> let headers = new Headers([['X-Custom', '1'],['Set-Cookie', 'a=1']]);
> headers.append('X-Custom', '2');
> headers.append('Set-Cookie', 'b=2');

> JSON.stringify(Array.from(headers.entries()))
'[["set-cookie","a=1"],["set-cookie","b=2"],["x-custom","1, 2"]]'

> JSON.stringify(Array.from(headers.values()))
'["a=1","b=2","1, 2"]'

> let forEachResults = [];
> headers.forEach((value, name) => forEachResults.push([name, value]))
> JSON.stringify(forEachResults)
'[["set-cookie","a=1"],["set-cookie","b=2"],["x-custom","1, 2"]]'

> let forOfResults = [];
> for ([name, value] of headers) { forOfResults.push([name, value]); }
> JSON.stringify(forOfResults)
'[["set-cookie","a=1"],["set-cookie","b=2"],["x-custom","1, 2"]]'

Also, FWIW, there's also a new headers.getSetCookie method which I think was added to help alleviate some of the confusion - but that's not available until node 19 so it didn't feel right to add that and start relying on it since we'd still be incompatible with node 18 Response instances.

Closes remix-run/remix#4354
Relates to remix-run/remix#7150

CC/ @alanshaw @Gozala

brophdawg11 and others added 2 commits August 26, 2023 21:54
* Fix headers.entries/values/forEach handling of setCookie

* Add changeset
@Gozala
Copy link
Contributor

Gozala commented Aug 28, 2023

Thanks for the pr

@Gozala Gozala merged commit 3c20536 into web-std:main Aug 28, 2023
5 checks passed
@MichaelDeBoey MichaelDeBoey deleted the headers branch August 29, 2023 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remix should not rely on Response.headers.raw
3 participants