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

backport: reading middleware cookies during render #69224

Draft
wants to merge 4 commits into
base: 14-2-1
Choose a base branch
from

Conversation

lubieowoce
Copy link
Member

Backport changes related to reading middleware-set cookies in render/actions.

this required some minor surgery because RequestAsyncStorageWrapper became withRequestStore, but nothing that seems too bad

@lubieowoce lubieowoce changed the title initialize ALS with cookies in middleware (#65008) backport: reading middleware cookies during render Aug 23, 2024
Copy link
Contributor

github-actions bot commented Aug 23, 2024

Hi there 👋

It looks like this PR introduces broken links to the docs, please take a moment to fix them before merging:

Broken link Type File
/docs/app/building-your-application/optimizing/package-bundling#analyzing-javascript-bundles link /docs/02-app/01-building-your-application/06-optimizing/13-memory-usage.mdx

Thank you 🙏

@ijjk
Copy link
Member

ijjk commented Aug 23, 2024

Failing test suites

Commit: 6ff29d2

pnpm test test/integration/app-dir-export/test/dynamicapiroute-prod.test.ts

  • app dir - with output export - dynamic api route prod > production mode > should work in prod with dynamicApiRoute 'force-static'
Expand output

● app dir - with output export - dynamic api route prod › production mode › should work in prod with dynamicApiRoute 'force-static'

thrown: "Exceeded timeout of 60000 ms for a test.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  14 |             'export const dynamic = "force-dynamic" on page "/api/json" cannot be used with "output: export".',
  15 |         },
> 16 |       ])(
     |        ^
  17 |         'should work in prod with dynamicApiRoute $dynamicApiRoute',
  18 |         async ({ dynamicApiRoute, expectedErrMsg }) => {
  19 |           await runTests({ isDev: false, dynamicApiRoute, expectedErrMsg })

  at ../node_modules/.pnpm/jest-each@29.7.0/node_modules/jest-each/build/bind.js:47:15
      at Array.forEach (<anonymous>)
  at integration/app-dir-export/test/dynamicapiroute-prod.test.ts:16:8
  at integration/app-dir-export/test/dynamicapiroute-prod.test.ts:4:56
  at Object.describe (integration/app-dir-export/test/dynamicapiroute-prod.test.ts:3:1)

Read more about building and testing Next.js in contributing.md.

@lubieowoce lubieowoce force-pushed the backport-14-3-0/middleware-cookies branch from c8b36dc to c82d6be Compare August 23, 2024 21:20
@ztanner ztanner changed the base branch from 14-3-0-staging to 14-2-1 August 26, 2024 19:40
@ztanner ztanner changed the base branch from 14-2-1 to 14-3-0-staging August 26, 2024 19:40
@lubieowoce lubieowoce changed the base branch from 14-3-0-staging to 14-2-1 August 28, 2024 11:10
### What
Cookies set/updated/removed in middleware won't be accessible during the
render in which they were set

### Why
Middleware will properly set a `set-cookie` header to inform the client
of the cookie change, but this means the `AsyncLocalStorage` context
containing the cookies value wouldn't be updated until the next time the
request headers were parsed. In other words, on the first request the
cookie would be sent but wouldn't be available in the `cookies()`
context. And then the following request would properly have the cookie
values.

### How
This uses a proxy on the `ResponseCookies` used in middleware to add a
middleware override header with the cookie value. When we instantiate
the cached cookies, we merge in whatever headers would have been set by
middleware, so that they're available in the same render that invoked
middleware.

### Test Plan
This changeset adds a test to confirm cookies set/deleted in middleware
are available in a single pass. Verified with a deployment
[here](https://vtest314-e2e-tests-ldx7olfl1-ztanner.vercel.app/rsc-cookies).

Fixes #49442
Closes NEXT-1126
When we provide the `set-cookie` string in `x-middleware-set-cookie`, we
need to ensure that multiple values are properly delimited.

We also make sure the cookies that get passed into `RequestCookies`
aren't in `ResponseCookie` form, to prevent something like `Path=/` from
being part of `cookies()`.

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

Closes NEXT-
Fixes #

-->
If middleware targets a server action handler and sets or updates a
cookie, the newly updated cookie would not be reflected in the
`cookies()` response of the action handler

In #65008 we fixed a bug where cookies set in middleware were not
reflected in the `cookies()` call in a server component from the same
request. We did this by introducing a `x-middleware-set-cookie` header,
that signaled to downstream handlers that middleware had run on the
request & set a cookie. However this handling was only applied to the
sealed/read-only cookies. Cookies accessed from a server action use
`mutableCookies`, since those aren't frozen as a server action is
allowed to modify cookies.

This pulls the cookie merge handling into a function and applies the
merge to `mutableCookies`.

Fixes #67814
Closes NDX-95
This ensures that cookies merged via `x-middleware-set-cookie` preserve
the options that are passed to them. Currently we're only selectively
merging in `name` and `value` but really we can just copy the
`ResponseCookie` in kind rather than enumerating on select properties.
@lubieowoce lubieowoce force-pushed the backport-14-3-0/middleware-cookies branch from c82d6be to 6ff29d2 Compare August 28, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants