fix(cloudflare): split `Set-Cookie` header into multiple headers #989

oleghalin · 2023-02-23T17:51:53Z

🔗 Linked issue

#988

❓ Type of change

📖 Documentation (updates to the documentation or readme)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality like performance)
✨ New feature (a non-breaking change that adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Splits Set-Cookie for each upstream header provided.

Resolves #988

📝 Checklist

I have linked an issue or discussion.
I have updated the documentation accordingly.

oleghalin · 2023-02-23T17:53:14Z

I suppose that these changes should be applied also to other presets where .join(',') used. @pi0

codecov · 2023-02-23T18:00:51Z

Codecov Report

Merging #989 (0583c8c) into main (1d0c120) will decrease coverage by 8.98%.
The diff coverage is n/a.

❗ Current head 0583c8c differs from pull request most recent head 1b5a4f6. Consider uploading reports for the commit 1b5a4f6 to get more accurate results

@@            Coverage Diff             @@
##             main     #989      +/-   ##
==========================================
- Coverage   76.64%   67.66%   -8.98%     
==========================================
  Files          71       60      -11     
  Lines        7241     6161    -1080     
  Branches      723      692      -31     
==========================================
- Hits         5550     4169    -1381     
- Misses       1690     1982     +292     
- Partials        1       10       +9

see 51 files with indirect coverage changes

oleghalin · 2023-02-23T18:47:45Z

I checked codebase a little bit deeper and i think that it should be changed on unenv level and response.headers (which is HeadersObject https://github.com/unjs/unenv/blob/920c3869ac5f7e5d1fb0475bb1cccad18f0764e2/src/runtime/_internal/types.ts#L2) should implement native Headers instead of { [key: string]: string | string[] | undefined }; and with this we dont need to make such transformations on preset level.

pi0 · 2023-02-24T10:31:58Z

src/runtime/entries/cloudflare.ts

+
+  for (const [k, v] of Object.entries(headers)) {
+    if (k === "set-cookie") {
+      for (const cookie of splitCookiesString(v)) {


We have recently introduced same util in h3. It might be one less dependency and bundle size if we expose it from h3 package.

Thanks, what is proper way to sync versions of h3 and current nitro? Should we wait for h3 version tag or?

Sure, releasing next version until this weekend. (at the same time thinking about your other comments, we need to really fix header normalization across entries)

As i see normalize function looks same for all other entries, but i think something is different on provider level, i suppose that some of them do cookie splitting themselves. We can drop normalize function to utils and use it every entry until make changes with type change for HadersObject.

Indeed there are slight behavior diferences. Also we never properly tested header behavior on nitro-deploys 😓

Hello, @pi0. Do you have any plans for releasing new versions soon? Gonna continue work on module and project, but this issue blocks me testing any env except local one :)

Hi, dear @oleghalin. Sure. I am (at the moment!) trying workers via pages on nitro-deploys. That would make testing this PR possible.

Thanks, just in case wanna remind that it also waits for unjs/h3 release for splitCookieString helper :)

oleghalin · 2023-03-02T09:24:24Z

Hello, @pi0. I also pushed to this PR change for not caching 404 responses if its asset directory, can you check it too please

which resolves #1001

oleghalin · 2023-03-09T11:12:57Z

Hello, @pi0 any updates on it? Issue with Set-Cookie header and 404 caching its making Nuxt absolutely unusuable with Cloudlare Workers

Stupremee · 2023-03-29T08:50:05Z

Hey @oleghalin,
I just saw this PR because I have the same problems with Nuxt on CF Workers, which is soo annoying.

I noticed that the new h3 version used in nitro now exports the splitCookeString function, so maybe you can remove the set-cookie-parser dependency and this PR can move forward.

oleghalin · 2023-03-30T09:51:43Z

Hello, @pi0. I just just updated my PR and dropped third-party dep

oleghalin · 2023-04-11T07:11:35Z

Hey, @pi0. I suppose that it makes many problems for all users who use CF workers to deploy, can you take a look on it? It solves 2 big problems with caching and cookies

j4tmr · 2023-04-30T13:17:35Z

+1 I had the same bug that bothered me for days，it's not just about nitro.routeRules, proxyRequest has the same bug

…h cookie sent from proxy

Hebilicious · 2023-07-02T03:42:26Z

@oleghalin This is on my radar, apologies for the delay. Can you provide me with a reproduction ? I would like to see if this behaviour affects the other cloudflare presets too.

Hebilicious

This doesn't include any tests or reproduction, so this is hard to test.

oleghalin · 2023-07-08T11:25:11Z

Not sure why you need reproduction, Set-Cookie header should be merged into one because browsers read each Set-Cookie header and set 1 cookie per each Set-Cookie header, as soon as there is only 1 header delimited by coma it will write only first cookie. As I know Vercel and node server parse it on their end and thats why it works, but it doesn't work for CF.

Hebilicious · 2023-07-08T15:02:26Z

Thanks for the additional information and the PR.
A reproduction is useful to test the proposed changes. Without one, it takes more maintainer time to review and get things merged.

oleghalin · 2023-07-17T08:42:24Z

Cannot make repro right now, but referencing RFC for you to understand it better.

https://datatracker.ietf.org/doc/html/rfc6265#section-3

Hebilicious

This is absolutely a necessary change!

The check for publicAsset needs to happen in another PR perf(cloudflare): check for asset url #1236 (feedback welcome)
This affects multiple preset and needs to be applied everywhere

Hebilicious · 2023-07-17T09:05:51Z

src/runtime/entries/cloudflare.ts

+  );
+
+  // Fetch public assets from KV only
+  if (isPublicAssetURL(id)) {


I agree this needs to happen, but it should be handled in a different PR (https://github.com/unjs/nitro/pull/1236/files)

vercel bot had a problem deploying to Preview February 23, 2023 17:52 Failure

vercel bot had a problem deploying to Preview February 23, 2023 17:55 Failure

pi0 reviewed Feb 24, 2023

View reviewed changes

pi0 changed the title ~~fix(cloudflare): split Set-Cookie header into multiple headers~~ fix(cloudflare): split Set-Cookie header into multiple headers Feb 24, 2023

oleghalin mentioned this pull request Feb 24, 2023

feat: expose splitCookiesString utility unjs/h3#343

Merged

vercel bot deployed to Preview March 2, 2023 09:25 View deployment

vercel bot deployed to Preview March 9, 2023 11:23 View deployment

vercel bot deployed to Preview March 30, 2023 09:53 View deployment

vercel bot deployed to Preview April 3, 2023 10:35 View deployment

vercel bot deployed to Preview April 11, 2023 07:12 View deployment

pi0 mentioned this pull request Apr 28, 2023

v2.4.0 #1197

Closed

oleghalin mentioned this pull request May 9, 2023

[nuxt] proxyRequest with multiple 'set-cookie' headers gets merged into one header unjs/h3#384

Closed

Hebilicious self-assigned this Jun 30, 2023

Hebilicious self-requested a review June 30, 2023 19:21

Hebilicious added bug Something isn't working preset:cloudflare labels Jun 30, 2023 — with Volta.net

oleghalin and others added 5 commits July 2, 2023 10:36

fix(cloudflare): split Set-Cookie header into multiple headers foreac…

94c6807

…h cookie sent from proxy

chore: codestyle fix

be09e45

fix(cloudflare): delete cache-control if asset path returned 404

d3d0824

fix(cloudflare): fetch publicAssets from KV only

003f85c

[autofix.ci] apply automated fixes

4712c73

oleghalin and others added 2 commits July 2, 2023 10:37

fix(cloudflare): drop set-cookie-parser

5632f49

[autofix.ci] apply automated fixes

1b5a4f6

Hebilicious force-pushed the main branch from 2aa66e2 to 1b5a4f6 Compare July 2, 2023 03:42

Hebilicious requested changes Jul 2, 2023

View reviewed changes

Hebilicious added pending needs reproduction labels Jul 2, 2023

pi0 requested a review from Hebilicious July 3, 2023 10:18

pi0 assigned pi0 and unassigned Hebilicious Jul 3, 2023

Hebilicious reviewed Jul 17, 2023

View reviewed changes

Hebilicious mentioned this pull request Jul 17, 2023

fix: split cookie headers #1452

Merged

7 tasks

pi0 closed this in #1452 Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudflare): split `Set-Cookie` header into multiple headers #989

fix(cloudflare): split `Set-Cookie` header into multiple headers #989

oleghalin commented Feb 23, 2023 •

edited

Loading

oleghalin commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

oleghalin commented Feb 23, 2023 •

edited

Loading

pi0 Feb 24, 2023

oleghalin Feb 24, 2023

pi0 Feb 24, 2023

oleghalin Feb 24, 2023 •

edited

Loading

pi0 Feb 24, 2023

oleghalin Mar 1, 2023

pi0 Mar 1, 2023

oleghalin Mar 1, 2023

oleghalin commented Mar 2, 2023 •

edited

Loading

oleghalin commented Mar 9, 2023

Stupremee commented Mar 29, 2023

oleghalin commented Mar 30, 2023

oleghalin commented Apr 11, 2023

j4tmr commented Apr 30, 2023

Hebilicious commented Jul 2, 2023

Hebilicious left a comment

oleghalin commented Jul 8, 2023

Hebilicious commented Jul 8, 2023

oleghalin commented Jul 17, 2023

Hebilicious left a comment

Hebilicious Jul 17, 2023

fix(cloudflare): split Set-Cookie header into multiple headers #989

fix(cloudflare): split Set-Cookie header into multiple headers #989

Conversation

oleghalin commented Feb 23, 2023 • edited Loading

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

oleghalin commented Feb 23, 2023 • edited Loading

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

oleghalin commented Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleghalin Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleghalin commented Mar 2, 2023 • edited Loading

oleghalin commented Mar 9, 2023

Stupremee commented Mar 29, 2023

oleghalin commented Mar 30, 2023

oleghalin commented Apr 11, 2023

j4tmr commented Apr 30, 2023

Hebilicious commented Jul 2, 2023

Hebilicious left a comment

Choose a reason for hiding this comment

oleghalin commented Jul 8, 2023

Hebilicious commented Jul 8, 2023

oleghalin commented Jul 17, 2023

Hebilicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix(cloudflare): split `Set-Cookie` header into multiple headers #989

fix(cloudflare): split `Set-Cookie` header into multiple headers #989

oleghalin commented Feb 23, 2023 •

edited

Loading

oleghalin commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

oleghalin commented Feb 23, 2023 •

edited

Loading

oleghalin Feb 24, 2023 •

edited

Loading

oleghalin commented Mar 2, 2023 •

edited

Loading