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 headers.entries/values/forEach handling of setCookie #39

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

brophdawg11
Copy link

@brophdawg11 brophdawg11 commented Aug 14, 2023

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

@changeset-bot
Copy link

changeset-bot bot commented Aug 14, 2023

🦋 Changeset detected

Latest commit: 24aac45

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@remix-run/web-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 merged commit f6ac353 into main Aug 16, 2023
11 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/headers branch August 16, 2023 17:03
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