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

[NEXT-735] Using headers.append twice with the Set-Cookie header only adds a single header #46579

Closed
1 task done
controversial opened this issue Feb 28, 2023 · 8 comments · Fixed by #47718
Closed
1 task done
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team.

Comments

@controversial
Copy link
Contributor

controversial commented Feb 28, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 22.4.0: Fri Feb 10 08:08:36 PST 2023; root:xnu-8796.100.721.505.3~4/RELEASE_ARM64_T6000
Binaries:
  Node: 19.6.0
  npm: 9.4.0
  Yarn: 3.3.1
  pnpm: N/A
Relevant packages:
  next: 13.2.2-canary.3
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

Middleware / Edge (API routes, runtime)

Link to the code that reproduces this issue

https://codesandbox.io/p/sandbox/mystifying-babbage-ruhtyx?file=%2Fpages%2Fapi%2Ftest.ts

To Reproduce

Visit /api/test with devtools open to observe the cookies sent with the request.

Describe the Bug

Only a single Set-Cookie header is sent and only a single cookie is set:
Screenshot 2023-02-28 at 3 50 42 PM

Expected Behavior

Two Set-Cookie headers should be added, as the fetch standard specifies.

This issue is very similar to #38302; @balazsorban44 asked that I open this issue to track this separate reproduction!

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-735

@controversial controversial added the bug Issue was opened via the bug report template. label Feb 28, 2023
@timneutkens timneutkens added the linear: next Confirmed issue that is tracked by the Next.js team. label Mar 1, 2023
@timneutkens timneutkens changed the title Using headers.append twice with the Set-Cookie header only adds a single header [NEXT-735] Using headers.append twice with the Set-Cookie header only adds a single header Mar 1, 2023
@xriter
Copy link

xriter commented Mar 1, 2023

Even when doing it manually, it only sets one header:

return new Response(null, {
	headers: [
		[
			"Set-Cookie",
			cookie.serialize("cookie1", "myval1", {
				maxAge: 3600,
				path: "/",
			}),
		],
		[
			"Set-Cookie",
			cookie.serialize("cookie2", "myval2", {
				maxAge: 3600,
				path: "/",
			}),
		],
	],
});

gives

< HTTP/1.1 200 OK
< vary: RSC, Next-Router-State-Tree, Next-Router-Prefetch
< set-cookie: cookie1=myval1; Max-Age=3600; Path=/, cookie2=myval2; Max-Age=3600; Path=/
< Date: Wed, 01 Mar 2023 16:32:00 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Transfer-Encoding: chunked

They are being serialised with a comma, instead of adding two separate headers.

It might be a bug in the Fetch api (see https://www.ryadel.com/en/fetch-api-bug-get-set-multiple-set-cookie-headers/)
I have not been able to find a fix yet...

@qq99
Copy link

qq99 commented Mar 7, 2023

Just ran into this too, following

@raphaelbadia
Copy link
Contributor

Related to vercel/edge-runtime#283

@dillondotzip
Copy link

Apparently there's been a fix for this that's merged into canary, but still getting the same issue. Even on localhost.

@xriter
Copy link

xriter commented Mar 17, 2023

@dillonraphael Could you share the link of the pull request that's supposed to fix this you are referring to?

@artemavrin
Copy link

In middleware setting 2 cookies works as it should, but in app/route, it sets only the first value

@abuinitski
Copy link

IMO Fetch API not to be blamed here, this must be very specific to Next API Routes implementation (or whatever underlying http rendering mechanism that it's using).

It all seems like API Routes do not handle an easy-to-miss edge case that is specific to Set-Cookie. This is a special header, the only one that has this funny behavior when multiple values are to be rendered into repeating header (vs. one header instance with values joined by comma).

Writing all this to ping that issue still exists in today's canary, and it hurts.

timneutkens pushed a commit that referenced this issue Mar 31, 2023
## What

This fix serves to address issues where multiple `Set-Cookie` headers
were combined in some runtimes.

## Why

This is because `set-cookie` behaves differently than other headers in
some cases.

Eg. when iterating on a `Headers` instance, multiple set-cookie headers
are folded. To set them correctly, we need to split them. But it'd not
be enough to naively split on the first occurrence, because `,` is a
valid cookie value when for example it's used in `Expires` in a date
string.

So we use a method to correctly detect where to split the cookie.

This should fix all runtimes.

Note, the spec now has `Headers#getSetCookie` which should be preferred
if it's present. whatwg/fetch#1346. We are using
the [`edge-runtime`](https://github.com/vercel/edge-runtime), so this
should be fixed upstream and then reused in Next.js in the future.

## How

Wherever we can, we reuse the `fromNodeHeaders` and `toNodeHeaders`
methods that have the correct implementation. This should be preferred
in the future in other parts of the codebase. We fixed some related TS
issues as well.

Fixes #46579, supersedes #40579
fix NEXT-735 ([link](https://linear.app/vercel/issue/NEXT-735))

---------

Co-authored-by: Balázs Orbán <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants