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

Set-Cookie headers are merged when using fetch #536

Closed
edenstrom opened this issue Aug 17, 2023 · 6 comments
Closed

Set-Cookie headers are merged when using fetch #536

edenstrom opened this issue Aug 17, 2023 · 6 comments

Comments

@edenstrom
Copy link

Bug Report

Current behavior

Forwarding Set-Cookie headers from fetch concatenates multiple ones into a single comma-separated Set-Cookie header. This happens only when deployed as en edge route.

The main issue is that if the Set-Cookie header exceeds 4096 bytes, everything after gets discarded by the browser. (Related issue: nextauthjs/next-auth#7443 (comment))

Expected behavior/code

The Set-Cookie headers should be preserved as-is and not concatenated.

Additional context/screenshots

Reproduction: https://vercel-set-cookie-header.vercel.app/
Repo: https://github.com/edenstrom/vercel-set-cookie-header

Example contains four routes.

Test using the buttons. Check the cookie panel in devtools. Notice that only the giant cookie updates with /api/edge.

Works Route Description
/api/cookie Returns 3 Set-Cookie headers
/api/node Node route, fetches /api/cookie and forwards headers
/api/edge Edge route, fetches /api/cookie and forwards headers
/api/parseEdge Edge route, fetches /api/cookie, parses using @edge-runtime/cookies and forwards headers
@balazsorban44
Copy link
Member

balazsorban44 commented Aug 17, 2023

Hi, from the looks of it, I believe that this works as-per spec:

https://github.com/edenstrom/vercel-set-cookie-header/blob/c91a21a0e90c282b2144b84f7524902a2c9ea853/src/app/api/edge/route.ts#L11

Here, .get() will always return a string, see https://fetch.spec.whatwg.org/#dom-headers-get

.getSetCookie() will return an array instead, and should be used when trying to access a list of set-cookie headers. 🤔

splitCookiesString is essentially doing the same as .getSetCookie(), but is not built into the platform, while .getSetCookie() is a web standard now, and should be preferred. See https://fetch.spec.whatwg.org/#dom-headers-getsetcookie

@balazsorban44
Copy link
Member

balazsorban44 commented Aug 17, 2023

The spec also says that .set():

Replaces the value of the first header whose name is name with value and removes any remaining headers whose name is name.

So you might need to do the for-each loop as in https://github.com/edenstrom/vercel-set-cookie-header/blob/c91a21a0e90c282b2144b84f7524902a2c9ea853/src/app/api/parseEdge/route.ts#L21 instead of https://github.com/edenstrom/vercel-set-cookie-header/blob/c91a21a0e90c282b2144b84f7524902a2c9ea853/src/app/api/edge/route.ts#L18

Combining this info, I think the correct way to write this would be:

import { NextRequest, NextResponse } from "next/server"

export const runtime = "edge"

export const GET = async (req: NextRequest) => {
  const url = `${req.nextUrl.origin}/api/cookie`
  const sessionResponse = await fetch(url, { credentials: "include" })
  const setCookies = sessionResponse.headers.getSetCookie() ?? []
  const response = NextResponse.json({
    route: "success. Returns multiple Set-Cookie headers"
  })

  for (const cookie in setCookies) {
    response.cookies.set("set-cookie", cookie)
  }

  return response
}

@balazsorban44
Copy link
Member

Related microsoft/TypeScript#55270

@edenstrom
Copy link
Author

The spec also says that .set():

Replaces the value of the first header whose name is name with value and removes any remaining headers whose name is name.

So you might need to do the for-each loop as in edenstrom/vercel-set-cookie-header@c91a21a/src/app/api/parseEdge/route.ts#L21 instead of edenstrom/vercel-set-cookie-header@c91a21a/src/app/api/edge/route.ts#L18

Combining this info, I think the correct way to write this would be:

import { NextRequest, NextResponse } from "next/server"

export const runtime = "edge"

export const GET = async (req: NextRequest) => {
  const url = `${req.nextUrl.origin}/api/cookie`
  const sessionResponse = await fetch(url, { credentials: "include" })
  const setCookies = sessionResponse.headers.getSetCookie() ?? []
  const response = NextResponse.json({
    route: "success. Returns multiple Set-Cookie headers"
  })

  for (const cookie in setCookies) {
    response.cookies.set("set-cookie", cookie)
  }

  return response
}

Hey! Yeah, you're right that it's according to spec. Just a weird use-case when it's server-to-server. 😅

Sadly there's no support for .getSetCookie in Vercel Edge Runtime yet. I've added a new route + button for that in the reproduction above. Returns 500 with the error TypeError: K.headers.getSetCookie is not a function. Works locally, but not deployed. (I guess the local version is a Node-based wrapper?)


Related:

The code in the edge-compatible next-auth should probably handle this use-case as well then? Wait for .getSetCookie or go with @edge-runtime/cookies?

https://github.com/nextauthjs/next-auth/blob/ffd055339b408475ef9acd780a71d85762e06311/packages/next-auth/src/lib/index.ts#L207-L208

@balazsorban44
Copy link
Member

Correct, this is now confirmed not to be a bug in edge-runtime. I forwarded this to the responsible Vercel team. (Vercel itself does not use the edge-runtime package, but the intention is that the implementation is identical).

next-auth will have to update to getSetCookie as well, and once it's fixed on Vercel, everything should work as expected.

I'm closing this issue then. Thanks!

@Pieparker
Copy link

@balazsorban44 Is there anywhere that interested parties can follow along with the progress of the bug fix (a public issue, or a commitment that an update will be shared somewhere)?

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

No branches or pull requests

3 participants