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

Cookies modified then read back again in an RSC return unmodified values #2669

Open
serhalp opened this issue Oct 15, 2024 · 1 comment
Open
Labels
Next.js e2e test failure Errors identified through the Next.js repo e2e tests Next.js

Comments

@serhalp
Copy link
Contributor

serhalp commented Oct 15, 2024

[email protected] unskipped some tests in this commit These tests hadn't been running for some time.

Many of these tests fail on Netlify. For example:

  ● app-dir with middleware › should be possible to modify cookies & read them in an RSC in a single request

    expect(received).toMatch(expected)

    Expected pattern: /Cookie 1: \d+\.\d+/
    Received string:  "Cookie 1:"

      132 |
      133 |     // cookies were set in middleware, assert they are present and match the Math.random() pattern
    > 134 |     expect(initialRandom1).toMatch(/Cookie 1: \d+\.\d+/)
          |                            ^
      135 |     expect(initialRandom2).toMatch(/Cookie 2: \d+\.\d+/)
      136 |     expect(totalCookies).toBe('Total Cookie Length: 2')
      137 |

      at Object.toMatch (e2e/app-dir/app-middleware/app-middleware.test.ts:134:28)

If you run the fixture locally, it appears to work as expected with next dev, ntl dev, and ntl serve, but it does not work as expected when deployed to Netlify (more detail below).

These tests and fix for the issue being tested were originally introduced in next.js in vercel/next.js#65008. My guess is they wouldn't have passed at the time on Netlify. Deploying the fixture exhibits the same behaviour with older canary versions and with next@latest.

I believe this is possibly a confusing intersection of three unrelated things:

  1. there is an inherent flakiness in these tests causing cookie 1 to get unset: test: de-flake rsc-cookies e2e tests vercel/next.js#71319
  2. the above is only triggering because of https://linear.app/netlify/issue/FRB-1359/nextjs-prefetch-priority-tests-are-failing (assuming that is a real issue as opposed to a simple test setup issue)
  3. what initialize ALS with cookies in middleware vercel/next.js#65008 was intending to fix is not fixed on Netlify, presumably due to something in next-runtime

This is largely conjecture, but what I observe is that when reloading the fixture page repeatedly I sometimes see both cookies set on the page and on the response, sometimes one is missing on the page but not headers, and sometimes they are missing on both. It's nondeterministic and confusing. However, if I remove the <Link href="/rsc-cookies-delete"> (or add prefetch={false}), the behaviour is much simpler: the two cookies are always set on the response but the values on the page always reflect the previous values; e.g.

load 1:
  headers: 4 5
  page: [blank] [blank]
load 2:
  headers 6 7
  page: 4 5
load 3:
  headers 8 9
 page 6 7

which is what led me to the above conclusions. This is the same on next@latest and next@canary.

I have no idea why this cannot be reproduced locally. It might be timing related?

You can verify the above with this repo (which is mostly just a copy of the test fixture): https://github.com/serhalp/nextjs-canary-rsc-cookies-repro.

This has never been reported by users AFAIK.

Data

The following is parsed automatically by the Next.js repo e2e test report generator.

test: test/e2e/app-dir/app-middleware/app-middleware.test.ts
reason: Cookies modified then read back again in an RSC return unmodified values

@serhalp serhalp added the Next.js e2e test failure Errors identified through the Next.js repo e2e tests label Oct 15, 2024
@serhalp serhalp added the Next.js label Oct 15, 2024 — with Linear
@orinokai
Copy link
Contributor

I had a look at the repro and it seems the x-middleware-set-cookie header is getting set correctly, so I'd imagine the problem is in the part that reads that header and combines the contents with the actual cookies in the request, i.e. here: https://github.com/vercel/next.js/pull/65008/files#diff-d16b8f6eab3df537748d15ebbf5901a5b4700dff01fbf030c29c7a18d89c5ef6R108

Not sure why this is only a problem with the Next Runtime, perhaps something to do with its use of AsyncLocalStorage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Next.js e2e test failure Errors identified through the Next.js repo e2e tests Next.js
Projects
None yet
Development

No branches or pull requests

2 participants