vercel / next.js

The React Framework
https://nextjs.org
MIT License
124.1k stars 26.44k forks source link

.getSetCookie() and .get('set-cookie') methods are both broken on headers() #54033

Closed controversial closed 1 year ago

controversial commented 1 year ago

Verify canary release

- [X] I verified that the issue exists in the latest Next.js canary release

Provide environment information

```bash Operating System: Platform: darwin Arch: arm64 Version: Darwin Kernel Version 23.0.0: Tue Aug 1 03:25:51 PDT 2023; root:xnu-10002.0.242.0.6~31/RELEASE_ARM64_T6000 Binaries: Node: 20.2.0 npm: 9.8.1 Yarn: N/A pnpm: N/A Relevant Packages: next: 13.4.15 eslint-config-next: 13.4.15 react: 18.2.0 react-dom: 18.2.0 typescript: 5.1.6 Next.js Config: output: N/A ```

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

App Router, Middleware / Edge (API routes, runtime), TypeScript (plugin, built-in types)

Link to the code that reproduces this issue or a replay of the bug

https://codesandbox.io/p/sandbox/fervent-bird-ylrc3x?file=/app/page.tsx

To Reproduce

  1. Set multiple cookies on the response, e.g. in middleware
  2. Read headers().get('set-cookie') in a React Server Component.
  3. Call headers().getSetCookie() (which can be called, even though its TypeScript definition is missing from Next.js)
    • Observe that an empty array is returned, even though headers.get('set-cookie') returns a value (albeit incorrect) for the Set-Cookie header.

Describe the Bug

This blocks applications from reading cookies that are updated by middleware as part of the same request, which is a common flow for use cases such as token refreshing (see discussions like https://github.com/vercel/next.js/discussions/50374, https://github.com/vercel/next.js/discussions/38650)

Expected Behavior

Which browser are you using? (if relevant)

N/A

How are you deploying your application? (if relevant)

N/A

NEXT-1523

viktorlarsson commented 1 year ago

Great find, currently blocking a major release for us. Related: https://github.com/nextauthjs/next-auth/pull/7443#issuecomment-1677261203

balazsorban44 commented 1 year ago

The headers().get('set-cookie') method should not concatenate multiple values into a single comma-separated string

Although the HTTP spec says so, the Headers spec says the opposite for .get():

https://fetch.spec.whatwg.org/#dom-headers-get

Hence the introduction of .getSetCookie() in the first place. The bug here is that the implementation for it might be wrong.

As for TypeScript, it's still missing from the official types https://github.com/microsoft/TypeScript/issues/55270 but we can probably extend it via edge-runtime (what Next.js is using) where we can guarantee the existence of this property.

Related: https://github.com/vercel/edge-runtime/issues/536

Also note, Headers.getAll is non-spec and should not be relied on. It was a platform-specific implementation before the getSetCookie spec existed (https://github.com/whatwg/fetch/issues/973)

To sum it up, it looks like we have to make sure .getSetCookie() works as expected. The rest looks good to me.

balazsorban44 commented 1 year ago

Upon some further investigation, it turns out that the only (new) issue here is the missing typings for getSetCookie. The fact that headers().get('set-cookie') returns something, in this case, is a bug.

You are setting response headers in your middleware, and headers() is supposed to read request headers. There is already an open issue for it at #52564

balazsorban44 commented 1 year ago

So the type is actually out in typescript@5.2.1-rc https://github.com/microsoft/TypeScript/issues/55270#issuecomment-1683706360 just have to wait until it gets to latest. For now, the workaround is @ts-expect-error

controversial commented 1 year ago

Thanks for looking into it @balazsorban44!

github-actions[bot] commented 11 months ago

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