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-471] Calling cookies.set twice sets only one value, in API route with Edge runtime #38302

Closed
1 task done
eric-burel opened this issue Jul 4, 2022 · 23 comments · Fixed by #42736
Closed
1 task done
Assignees
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. Runtime Related to Node.js or Edge Runtime with Next.js.

Comments

@eric-burel
Copy link
Contributor

eric-burel commented Jul 4, 2022

Verify canary release

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

Provide environment information

Not applicable, see reproduction here: https://codesandbox.io/s/condescending-torvalds-e3il3k?file=/package.json

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

Calling "cookies.set" twice on an API route with edge handler runtime will set only one cookie.

Expected Behavior

You'll see one "Set-Cookie" with 2 values, however it should be instead 2 "Set-Cookie" with only one value.

Set-Cookie can be repeated as many time as needed in an HTTP response to set multiple cookies. Edge middlewares do not suffer from this issue, it affects only API routes.

Link to reproduction

https://codesandbox.io/s/condescending-torvalds-e3il3k

To Reproduce

https://codesandbox.io/s/condescending-torvalds-e3il3k?file=/pages/index.tsx
Open index, check the Network tab for the "cookie" request.

NEXT-471

@eric-burel eric-burel added the bug Issue was opened via the bug report template. label Jul 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2022

Please verify your issue reproduces with next@canary. The canary version of Next.js ships daily and includes all features and fixes that have not been released to the stable version yet. Think of canary as a public beta. Some issues may already be fixed in the canary version, so please verify that your issue reproduces by running npm install next@canary. If the issue does not reproduce with the canary version, then it has already been fixed and this issue can be closed.

@github-actions github-actions bot added the please verify canary The issue should be verified against next@canary. It will be closed after 30 days of inactivity label Jul 4, 2022
@eric-burel eric-burel changed the title Calling cookies.set twice doesn't work as expected in API route with Edge runtime Calling cookies.set twice sets only one value, in API route with Edge runtime Jul 4, 2022
@controversial
Copy link
Contributor

Note this issue still exists even when returning a Response, even when avoiding the NextResponse cookies API entirely:

Starting from npx create-next-app -e reproduction-template, I added only one file, pages/api/test.js:

export default function handler() {
  const res = new Response();
  res.headers.append('Set-Cookie', 'a=b');
  res.headers.append('Set-Cookie', 'c=d');
  return res;
}

export const config = {
  runtime: 'experimental-edge',
};

Now, running npm run dev and curl -v http://localhost:3000/api/test:

*   Trying 127.0.0.1:3000...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET /api/test HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.79.1
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< set-cookie: a=b, c=d
< Date: Fri, 15 Jul 2022 18:47:49 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Transfer-Encoding: chunked
< 
* Connection #0 to host localhost left intact

The expected behavior, when using append with set-cookie, is that multiple set-cookie headers are included in the response [ref].

@controversial
Copy link
Contributor

controversial commented Jul 15, 2022

I’ve verified that this bug still exists in the current next@canary release, so the “please verify canary” label should be removed.

@controversial
Copy link
Contributor

Also worth noting that there’s a test [a], [b] for this edge case for middleware. A similar test case is missing for edge functions

@eric-burel
Copy link
Contributor Author

eric-burel commented Jul 28, 2022

Also worth noting that there’s a test [a], [b] for this edge case for middleware. A similar test case is missing for edge functions

Thanks for the tip, I've started to write a test case: #39132
I am stuck on running the test locally right now ^^ but I hope it will help getting some progress on the issue

@ijjk ijjk added Runtime Related to Node.js or Edge Runtime with Next.js. and removed please verify canary The issue should be verified against next@canary. It will be closed after 30 days of inactivity labels Jul 28, 2022
@eric-burel
Copy link
Contributor Author

Btw this seems vaguely related to whatwg/fetch#1346, it discusses the handling of list of set-header values in fetch.

@controversial
Copy link
Contributor

This issue still exists in Next.js 13.

@controversial
Copy link
Contributor

This issue still exists in Next.js 13.1, where edge API routes have been marked “stable.”

@alvarlagerlof
Copy link

This issue promoted me to store a stringified json object in a cookie. Not a great experience.

@sannajammeh
Copy link
Contributor

Still a blocking problem here for introducing edge authentication... Not sure why the API is marked as stable when this issue persists into 2023.

@balazsorban44 balazsorban44 added the linear: next Confirmed issue that is tracked by the Next.js team. label Feb 7, 2023
@balazsorban44 balazsorban44 changed the title Calling cookies.set twice sets only one value, in API route with Edge runtime [NEXT-471] Calling cookies.set twice sets only one value, in API route with Edge runtime Feb 7, 2023
@balazsorban44 balazsorban44 self-assigned this Feb 7, 2023
@balazsorban44
Copy link
Member

vercel/edge-runtime#255 needs to land first, then we can update Next.js as well

@controversial
Copy link
Contributor

Glad to see @edge-runtime/[email protected] land @balazsorban44! What still needs to be done in #42736 to incorporate these changes into Next.js?

@balazsorban44
Copy link
Member

Getting the PR checks to pass. 😁 Working on it today.

@y0zong
Copy link

y0zong commented Feb 26, 2023

this seems still a issue even I upgrade to v13.2.1(which a fix has merged in v13.2.0), what did I miss?

@controversial
Copy link
Contributor

Confirmed ^ On [email protected], the following still only produces only one Set-Cookie header:

export default function handler() {
  const res = new Response('hey');
  res.headers.append('Set-Cookie', 'a=b');
  res.headers.append('Set-Cookie', 'c=d');
  return res;
}

export const config = {
  runtime: 'edge',
};

Screenshot 2023-02-26 at 1 18 46 AM

@balazsorban44 should this issue be reopened?

@balazsorban44
Copy link
Member

@controversial could you open a new one with the above reproduction? 🙏 would be easier to track.

@balazsorban44
Copy link
Member

By the way, it's a different (but related) issue, since you were not using NextResponse as the OP did.

@controversial
Copy link
Contributor

@balazsorban44 Happy to open a new issue; will do that in a moment!

I’m not sure this issue is solved either, though; This example uses the NextResponse API and is almost identical to the reproduction in the original issue:

Screenshot 2023-02-28 at 3 37 35 PM

Only a single Set-Cookie header is returned.

Is there a different test case associated with this issue that #42736 fixed?

@bpossolo
Copy link

bpossolo commented Mar 27, 2023

@balazsorban44 @controversial
im seeing the same problem on NextJS 13.2.4
using NextResponse.cookies.set() to set multiple cookies yields a single Set-Cookie http response header...

const response = NextResponse.redirect(location, 303);
response.cookies.set({
  name: ACCESS_TOKEN,
  value: result.accessToken,
  path: '/',
  secure,
  expires: result.expiresOn ? result.expiresOn : undefined,
});
response.cookies.set({
  name: ACCOUNT_ID,
  value: result.account.homeAccountId,
  path: '/',
  secure,
  expires: expiration('years', 1),
});
return response;
set-cookie: access-token=mytokenvalue; Path=/; Expires=Mon, 27 Mar 2023 04:38:03 GMT, account-id=1c12dd32-abcd-4e0a-9d21-5b480b806b06-b2c_1a_passwordless-authentication.abcdedf-1136-40a6-a376-123456789; Path=/; Expires=Tue, 26 Mar 2024 04:44:43 GMT

from the spec:

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into a single header field. The usual mechanism for folding HTTP headers fields (i.e., as defined in [RFC2616]) might change the semantics of the Set-Cookie header field because the %x2C (",") character is used by Set-Cookie in a way that conflicts with such folding.

comma is used in the Expires attribute value

@controversial
Copy link
Contributor

I think this issue should be reopened

@petersng
Copy link

petersng commented Mar 30, 2023

anyone know of a workaround for this issue that doesn't require turning a cookie into some JSON or an array? I'm trying to set cookies for a library that is pulling the cookies in a standard way. it's currently a blocker for me. thanks for any insights.

@eric-burel
Copy link
Contributor Author

Note that I don't have the option to reopen as issue creator, you might want to open a separate issue to get attention on this issue.
Also, I am pretty sure it can be a nice first contribution for whoever have time to check it out, it's (probably) not difficult since a similar logic exists in other part of the code like middlewares, node.js API routes, but requires digging the codebase.

@github-actions
Copy link
Contributor

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 Apr 30, 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. Runtime Related to Node.js or Edge Runtime with Next.js.
Projects
None yet
9 participants