fix: txn cookie accumulation - #2748
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR blocks transaction creation from Next.js prefetches, evicts accumulated transaction cookies, adds cookie-size warnings, updates transaction-cookie encoding and tests, forwards client options, and documents practices for avoiding 431 errors. ChangesTransaction Cookie Controls
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2748 +/- ##
==========================================
+ Coverage 87.98% 88.26% +0.27%
==========================================
Files 80 80
Lines 11514 11611 +97
Branches 2386 2413 +27
==========================================
+ Hits 10131 10248 +117
+ Misses 1338 1318 -20
Partials 45 45 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@EXAMPLES.md`:
- Around line 4048-4051: The transaction-cookie eviction check must include the
incoming cookie size: update the logic calculating txnBytes or the cap condition
to evaluate existing transaction-cookie bytes plus newCookieValue’s serialized
size against the 3500-byte limit. Preserve FIFO eviction and leave
non-transaction cookies untouched, and add a boundary test covering a new cookie
that causes the combined total to reach or exceed the limit.
In `@README.md`:
- Around line 193-194: Align the login-link guidance in README.md with the
corresponding recommendation in EXAMPLES.md. Choose one tested approach—prefer
plain <a> links until the RSC-navigation detector is fixed, or consistently
document Link with prefetch disabled—and update the conflicting documentation so
both files prescribe the same behavior.
In `@src/utils/request.ts`:
- Around line 27-35: The isNonNavigationalRequest function incorrectly treats
the generic Accept header as a prefetch signal. Remove that check or require an
explicit prefetch indicator, then update the login snippets in EXAMPLES.md at
lines 237-238 and 4057-4068 to reflect the corrected navigation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a92bef5-5000-46b6-8039-d5704cdc15da
📒 Files selected for processing (13)
EXAMPLES.mdREADME.mdsrc/server/auth-client.test.tssrc/server/auth-client.tssrc/server/client.tssrc/server/mfa-popup.test.tssrc/server/session/stateless-session-store.test.tssrc/server/session/stateless-session-store.tssrc/server/transaction-store.test.tssrc/server/transaction-store.tssrc/server/txn-cookie-accumulation.test.tssrc/test/utils.tssrc/utils/request.ts
💤 Files with no reviewable changes (1)
- src/server/client.ts
…d targeted cleanup
…nd targeted cleanup
73c819d to
27119c4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server/txn-cookie-accumulation.test.ts (2)
230-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNegative "not deleted" assertions are vacuous. Both sites apply
?.maxAgeto a cookie that is absent from theResponseCookiesjar when the SDK behaves correctly, soundefined !== 0passes even if the surrounding behavior regresses.
src/server/txn-cookie-accumulation.test.ts#L230-L234: replaceexpect(resCookies.get(\_txn${newerState}`)?.maxAge).not.toBe(0)withexpect(resCookies.get(`_txn${newerState}`)).toBeUndefined()`.src/server/txn-cookie-accumulation.test.ts#L644-L646: replaceexpect(callbackRes.cookies.get("__txn_tabB")?.maxAge).not.toBe(0)withexpect(callbackRes.cookies.get("__txn_tabB")).toBeUndefined().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/txn-cookie-accumulation.test.ts` around lines 230 - 234, Replace the vacuous negative cookie assertions with direct absence checks: in src/server/txn-cookie-accumulation.test.ts lines 230-234, assert resCookies.get for newerState is undefined; in lines 644-646, assert callbackRes.cookies.get("__txn_tabB") is undefined. Keep the surrounding eviction and new-cookie assertions unchanged.
237-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate of the preceding eviction test.
This test uses identical fixtures and assertions to "evicts oldest cookie first when the 3500 byte limit is exceeded" (Lines 213-235), minus the newer-cookie check. Either drop it or make it genuinely distinct (e.g., three cookies where two must be evicted, asserting eviction order).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/txn-cookie-accumulation.test.ts` around lines 237 - 256, The test “evicts oldest login cookies first (FIFO by timestamp)” duplicates the preceding eviction coverage. Remove it, or make it distinct by exercising three cookies that exceed the limit and asserting that the two oldest cookies are evicted in timestamp order while the newest remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/server/txn-cookie-accumulation.test.ts`:
- Around line 230-234: Replace the vacuous negative cookie assertions with
direct absence checks: in src/server/txn-cookie-accumulation.test.ts lines
230-234, assert resCookies.get for newerState is undefined; in lines 644-646,
assert callbackRes.cookies.get("__txn_tabB") is undefined. Keep the surrounding
eviction and new-cookie assertions unchanged.
- Around line 237-256: The test “evicts oldest login cookies first (FIFO by
timestamp)” duplicates the preceding eviction coverage. Remove it, or make it
distinct by exercising three cookies that exceed the limit and asserting that
the two oldest cookies are evicted in timestamp order while the newest remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b254c038-2492-4f33-97f1-5e7590de6e67
📒 Files selected for processing (13)
EXAMPLES.mdREADME.mdsrc/server/auth-client.test.tssrc/server/auth-client.tssrc/server/client.tssrc/server/mfa-popup.test.tssrc/server/session/stateless-session-store.test.tssrc/server/session/stateless-session-store.tssrc/server/transaction-store.test.tssrc/server/transaction-store.tssrc/server/txn-cookie-accumulation.test.tssrc/test/utils.tssrc/utils/request.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/server/session/stateless-session-store.test.ts
- src/server/transaction-store.test.ts
- src/server/client.ts
- src/test/utils.ts
- src/utils/request.ts
- README.md
- src/server/session/stateless-session-store.ts
- src/server/mfa-popup.test.ts
- src/server/transaction-store.ts
- src/server/auth-client.ts
- EXAMPLES.md
- src/server/auth-client.test.ts
| const method = req.method; | ||
|
|
||
| if (method === "GET" && sanitizedPathname === this.routes.login) { | ||
| if (isNonNavigationalRequest(req)) { |
There was a problem hiding this comment.
Primary fix for the 431: Next.js prefetches to /auth/login were silently minting a __txn_* cookie each time. We now short-circuit prefetch requests before handleLogin runs, so no cookie is written. Real navigations are unaffected — see isNonNavigationalRequest for why only prefetch-exclusive headers are matched.
| * @param newCookieValue - Value of that cookie; its size is included in the cap | ||
| * so a large new cookie can still trigger eviction. | ||
| */ | ||
| private evictOldestTransactionCookies( |
There was a problem hiding this comment.
This is the safety net for prefetches the header guard can't catch (CDN-stripped, router.prefetch()). Before writing a new txn cookie, we cap the total __txn_* size at 3500 bytes and evict the oldest first (FIFO), so accumulated cookies can never grow the header to 431. Only __txn_* cookies are touched; the one being written is never evicted.
| // "{ts}:{jwe}" — no cookie name change, backward compatible with legacy bare "{jwe}". | ||
| const ts = Math.floor(Date.now() / 1000); | ||
| const newCookieName = this.getTransactionCookieName(transactionState.state); | ||
| const newCookieValue = `${ts}:${jwe}`; |
There was a problem hiding this comment.
Cookie value is now {ts}:{jwe} (name unchanged). The timestamp gives eviction its FIFO order; get() strips the prefix before decrypting and legacy bare {jwe} values still read fine, so this is backward compatible.
| resCookies: cookies.ResponseCookies, | ||
| transactionState: TransactionState, | ||
| reqCookies?: cookies.RequestCookies | ||
| reqCookies?: cookies.RequestCookies | cookies.ReadonlyRequestCookies |
There was a problem hiding this comment.
Intentional removal here: save() used to have a guard that rejected a second login in single-transaction mode. It was dead code on main (nothing ever passed reqCookies, so it never ran). Now that reqCookies is passed for eviction, keeping that guard would have activated it for the first time and broken login retries. Removing it keeps behavior identical to main (single-txn logins overwrite the fixed __txn_ cookie).
2 participants
📋 Changes
Fixes unbounded _txn* transaction-cookie accumulation that produces 431 Request Header Fields Too Large, and adds a warning for the other main cause of oversized request headers - a large session cookie.
Docs (EXAMPLES.md + README.md)
- use
<a>/<Link prefetch={false}>— not<Link href="/auth/login">- prefer withPageAuthRequired over middleware redirects
- recommends lowering transactionCookie.maxAge if in-flight logins are being evicted too aggressively.
📎 References
🎯 Testing
Summary by CodeRabbit