Drop ID token from Nuxt session cookie to fix oversized-cookie login failure - #69
Drop ID token from Nuxt session cookie to fix oversized-cookie login failure#69janithjay wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Nuxt session token no longer accepts, stores, or receives the raw ChangesNuxt session token handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/nuxt/tests/unit/session-manager.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nuxt/tests/unit/session-manager.test.ts (1)
200-200: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that new session tokens omit
idToken.The PR removes the raw ID token to reduce session-cookie size. The updated combined test checks only
accessTokenExpiresAtandrefreshToken, so a regression could re-addidTokenwithout failing this suite.Add an explicit absence assertion.
Proposed test assertion
+ expect(payload).not.toHaveProperty('idToken');🤖 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 `@packages/nuxt/tests/unit/session-manager.test.ts` at line 200, Add an explicit assertion in the combined new-session token test that the returned payload does not contain idToken, alongside the existing accessTokenExpiresAt and refreshToken checks. Use the same payload produced by the session-token flow so regressions that reintroduce the raw ID token fail the test.
🔇 Additional comments (2)
packages/nuxt/tests/unit/session-manager.test.ts (2)
179-179: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Remove
idTokenfrom the refresh call.
createSessionTokeninpackages/nuxt/src/runtime/server/utils/session.ts:39-70no longer declaresidToken. The call inpackages/nuxt/src/runtime/server/utils/token-refresh.ts:51-126still passes it. TypeScript excess-property checking will reject this object literal.Remove the stale property before merge.
Proposed fix
- idToken: refreshed.id_token ?? session.idToken,Verify all call sites:
144-144: LGTM!Also applies to: 160-160, 189-189, 212-212
🤖 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 `@packages/nuxt/tests/unit/session-manager.test.ts`:
- Line 200: Add an explicit assertion in the combined new-session token test
that the returned payload does not contain idToken, alongside the existing
accessTokenExpiresAt and refreshToken checks. Use the same payload produced by
the session-token flow so regressions that reintroduce the raw ID token fail the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fba35ba-c85c-4e2d-a2d3-bb36b89b5e2f
📒 Files selected for processing (2)
packages/nuxt/src/runtime/server/utils/session.tspackages/nuxt/tests/unit/session-manager.test.ts
💤 Files with no reviewable changes (1)
- packages/nuxt/src/runtime/server/utils/session.ts
|
IMO, the ideal fix for this is to implement session cookie chunking. |
By PR #74, Supersedes this PR by fixing this issue with session cookie chunking as above suggested approach. |
Purpose
The Nuxt quickstart's redirect-flow sign-in silently fails: the OAuth exchange completes successfully end-to-end (confirmed via backend logs - authorize, credential submission, code exchange, and JWKS fetch all return 200/302 as expected), but the user is bounced back to a signed-out state instead of landing on the authenticated page.
Root cause:
createSessionToken(packages/nuxt/src/runtime/server/utils/session.ts) embedded the full access token, ID token, and refresh token - all JWTs themselves - as fields inside the session cookie's own JWT payload. With ThunderID's token claim set, the resulting cookie value routinely exceeds the browser's hard ~4096-byte per-cookie limit. The browser silently drops anySet-Cookieover that limit (confirmed in Chrome DevTools under "Malformed Response Cookies"), so the session is never actually established despite the server doing everything correctly.Approach
Stop embedding
idTokenin the session cookie payload. It's only used transiently at issuance time (getDecodedIdTokenforsub/user_org) and doesn't need to be persisted.This matches
@thunderid/nextjs'sSessionManager.createSessionToken, which already excludesidTokenfrom its session cookie and only storesaccessToken+refreshToken. Nuxt was the only SDK in the monorepo embedding the full ID token client-side. This brings it in line with the Next.js precedent without a larger architectural change.After the fix
__thunderid__sessioncookie was acceptedRelated Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit