Harden session sync against false logouts and refresh races - #172
Merged
Conversation
Follow-up to the JWT session-sync work (#171), addressing review findings: - Checkout email input is keyed off the account email rather than the live input value, so an authenticated user without an email on file can still type instead of being disabled after the first keystroke. - Transient customer-fetch failures no longer collapse a valid session to logged-out: syncSession flags them as `stale` so the client keeps the current user, clearing only on a confirmed 401/403. - Concurrent token refreshes sharing the same rotated refresh token are coalesced into a single in-flight request, so a losing race can no longer wipe the freshly persisted session. - Deduplicate best-effort cookie clearing behind an exported clearAuthCookies helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9CjDU8j8MvtR2Wtxpobfa
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughAuthentication handling now coalesces refreshes, distinguishes transient failures from confirmed logout, preserves stale sessions, and updates checkout email behavior for authenticated users. ChangesAuthentication and session resilience
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Checkout
participant AuthContext
participant syncSession
participant fetchCustomer
participant SpreeAuth
Checkout->>AuthContext: refresh user
AuthContext->>syncSession: synchronize session
syncSession->>fetchCustomer: fetch customer
fetchCustomer->>SpreeAuth: use coalesced auth refresh
SpreeAuth-->>fetchCustomer: customer or auth/transient error
fetchCustomer-->>syncSession: result
syncSession-->>AuthContext: customer, refreshed, stale
AuthContext-->>Checkout: updated or preserved user state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/data/customer.ts (1)
1-1: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winToken rotation signal (
refreshed) is lost on a transiently-stale follow-up fetch.syncSession()hardcodesrefreshed: falsein its stale branch even when the preceding refresh actually rotated tokens, andAuthContext.refreshUser()checksstalebeforerefreshed. Together, a successful rotation immediately followed by a transient customer-fetch failure never triggersrouter.refresh(), so Server Components keep rendering under the pre-rotation session until the next throttled resync.
src/lib/data/customer.ts#L93-117: returnrefreshed: state === "refreshed"in the stale branch instead of hardcodingfalse(and update the matching assertion insrc/lib/data/__tests__/customer.test.tslines 172-190).src/contexts/AuthContext.tsx#L70-79: checkrefreshed(and callrouter.refresh()) before the earlystalereturn, e.g.if (refreshed) router.refresh(); if (stale) return;.🔧 Proposed fix
--- a/src/lib/data/customer.ts @@ // Transient failure — signal the client to preserve its current session. - return { customer: null, refreshed: false, stale: true }; + return { customer: null, refreshed: state === "refreshed", stale: true };--- a/src/contexts/AuthContext.tsx @@ const { customer, refreshed, stale } = await syncSession(); - // A transient fetch failure returns `stale` — keep the current session - // rather than flashing the user to logged-out on a blip. - if (stale) return; - setUser(customer ? toUser(customer) : null); - if (refreshed) { - router.refresh(); - } + if (refreshed) { + router.refresh(); + } + // A transient fetch failure returns `stale` — keep the current session + // rather than flashing the user to logged-out on a blip. + if (stale) return; + setUser(customer ? toUser(customer) : null);🤖 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/lib/data/customer.ts` at line 1, Preserve the token-rotation signal in syncSession’s stale branch by returning refreshed based on state === "refreshed" instead of false, and update the corresponding customer test assertion. In AuthContext.refreshUser, handle refreshed and call router.refresh() before returning early for stale results.
🧹 Nitpick comments (3)
src/lib/spree/auth-helpers.ts (2)
111-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a regression test for the coalescing behavior.
This is the PR's central resilience fix; a test asserting two concurrent
tryRefresh()/withAuthRefresh()calls for the same refresh token result in a single call to the underlyingauth.refresh()would give confidence this doesn't regress.🤖 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/lib/spree/auth-helpers.ts` around lines 111 - 140, The refresh coalescing behavior in tryRefresh should be covered by a regression test. Add a test that invokes two concurrent tryRefresh or withAuthRefresh calls with the same refresh token, mocks auth.refresh, and asserts both callers share the result while auth.refresh is called exactly once.
95-140: 🩺 Stability & Availability | 🔵 TrivialIn-memory refresh coalescing is process-local. That still leaves the same rotated-token race across Vercel/serverless invocations or multiple replicas; if that runtime is in scope, use shared coordination instead of relying on this map.
🤖 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/lib/spree/auth-helpers.ts` around lines 95 - 140, Replace the process-local refreshInFlight map used by tryRefresh with shared coordination keyed by refreshToken, so concurrent refreshes across serverless invocations or replicas coalesce around one refresh operation. Preserve the existing cookie-persistence guard, token rotation persistence, failure cleanup, and returned-token behavior.src/components/checkout/AddressSection.tsx (1)
277-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect fix for the checkout email lockout.
Keying
disabled/the hint offuser?.email(account email) rather than the liveOptional: the predicate
isAuthenticated && !!user?.emailis duplicated at lines 281 and 284; extracting it to a localconst hasAccountEmail = isAuthenticated && !!user?.email;would avoid the repetition.🤖 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/components/checkout/AddressSection.tsx` around lines 277 - 284, In AddressSection, extract the repeated isAuthenticated && !!user?.email predicate into a local hasAccountEmail constant and reuse it for the email field’s disabled prop and conditional hint rendering, preserving the current account-email-based behavior.
🤖 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 `@src/contexts/AuthContext.tsx`:
- Around line 70-79: Update refreshUser in AuthContext so stale results preserve
the current user while still calling router.refresh() when refreshed is true;
avoid returning before the refresh check. Update syncSession’s stale branch to
propagate whether token rotation occurred instead of hardcoding refreshed:
false, preserving the existing stale-session behavior.
In `@src/lib/data/customer.ts`:
- Around line 93-117: Update the transient-failure return in syncSession so
refreshed reflects whether ensureFreshSession returned "refreshed", even when
fetchCustomer fails non-auth transiently. Preserve stale: true and the existing
auth-error handling, while ensuring AuthContext.refreshUser can observe the
successful rotation.
In `@src/lib/spree/auth-helpers.ts`:
- Around line 118-140: The catch in tryRefresh must distinguish confirmed
invalid refresh tokens from transient failures. Inspect the caught error and
call clearAuthCookies only for a SpreeError with code invalid_refresh_token or
status 401; return null for other refresh errors while preserving the existing
finally cleanup and in-flight deduplication.
---
Outside diff comments:
In `@src/lib/data/customer.ts`:
- Line 1: Preserve the token-rotation signal in syncSession’s stale branch by
returning refreshed based on state === "refreshed" instead of false, and update
the corresponding customer test assertion. In AuthContext.refreshUser, handle
refreshed and call router.refresh() before returning early for stale results.
---
Nitpick comments:
In `@src/components/checkout/AddressSection.tsx`:
- Around line 277-284: In AddressSection, extract the repeated isAuthenticated
&& !!user?.email predicate into a local hasAccountEmail constant and reuse it
for the email field’s disabled prop and conditional hint rendering, preserving
the current account-email-based behavior.
In `@src/lib/spree/auth-helpers.ts`:
- Around line 111-140: The refresh coalescing behavior in tryRefresh should be
covered by a regression test. Add a test that invokes two concurrent tryRefresh
or withAuthRefresh calls with the same refresh token, mocks auth.refresh, and
asserts both callers share the result while auth.refresh is called exactly once.
- Around line 95-140: Replace the process-local refreshInFlight map used by
tryRefresh with shared coordination keyed by refreshToken, so concurrent
refreshes across serverless invocations or replicas coalesce around one refresh
operation. Preserve the existing cookie-persistence guard, token rotation
persistence, failure cleanup, and returned-token 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d275a8f8-e8dc-4fa3-a119-a3d69870feed
📒 Files selected for processing (6)
src/components/checkout/AddressSection.tsxsrc/contexts/AuthContext.tsxsrc/lib/data/__tests__/customer.test.tssrc/lib/data/customer.tssrc/lib/spree/auth-helpers.tssrc/lib/spree/index.ts
The session-sync work made the customer-fetch path resilient to transient errors but left the token-refresh path clearing auth cookies on any failure, so a network or 5xx blip during refresh logged the user out — the exact failure this change set out to prevent. Only drop the session on a confirmed-invalid refresh token; treat other refresh failures as stale and preserve the session. Carry the rotation signal through a stale follow-up fetch so server components still re-render under the renewed session. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9CjDU8j8MvtR2Wtxpobfa
The comment justified the `!!user?.email` guard with an impossible state (a saved customer always has an email). The guard actually protects the window where the server-derived `isAuthenticated` is true while the AuthContext `user` is still null — during hydration or after a transient sync preserved as stale — where keying off `isAuthenticated` alone would leave the email field disabled and blank, blocking checkout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X9CjDU8j8MvtR2Wtxpobfa
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/spree/__tests__/auth-helpers.test.ts`:
- Around line 22-32: Update the ../cookies mock factory in auth-helpers.test.ts
to include the exported clearAuthCookies function, using a dedicated
mockClearAuthCookies spy or equivalent delegation to the existing clear-token
mocks. Review the affected auth error test assertions to verify clearAuthCookies
is invoked, while preserving the existing cookie mock 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4360b01f-66d7-46ad-a4c5-68b85fd30ac4
📒 Files selected for processing (7)
src/components/checkout/AddressSection.tsxsrc/contexts/AuthContext.tsxsrc/lib/data/__tests__/customer.test.tssrc/lib/data/customer.tssrc/lib/spree/__tests__/auth-helpers.test.tssrc/lib/spree/auth-helpers.tssrc/lib/spree/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/spree/index.ts
- src/components/checkout/AddressSection.tsx
- src/contexts/AuthContext.tsx
- src/lib/data/customer.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #171 (session sync with JWT refresh on tab focus), addressing review findings from that PR that landed alongside the merge.
Changes
AddressSection.tsx) — the field'sdisabled/helper condition now keys off the account email (user?.email) instead of the live input state. Previously an authenticated user with no email on file would have the field disable itself after the first keystroke, blocking checkout.customer.ts,AuthContext.tsx) —syncSession()now flags a transient customer-fetch failure asstaleso the client keeps the current user rather than flashing to logged-out on a network blip / 5xx. The session is only cleared on a confirmed401/403.auth-helpers.ts) —tryRefresh()now shares a single in-flight request per rotated refresh token, so a losing race can no longer rotate a stale token and wipe the freshly persisted session. Keyed by token value so distinct sessions never share a refresh.clearAuthCookies()is exported fromauth-helpersand reused incustomer.tsin place of the duplicateclearAuthTokens()helper.Testing
vitest run— 106 tests pass, including newsyncSessioncases covering the transient-failure (stale) and auth-failure paths.tsc --noEmitclean; Biome check clean.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Note
Medium Risk
Touches authentication, cookie clearing, and token refresh—security-sensitive paths—but behavior is narrowed (fewer logouts) with added tests; checkout UX change is low scope.
Overview
Hardens JWT session handling so network blips and concurrent refreshes no longer sign users out or break checkout.
Session sync —
syncSession()can returnstalewhen refresh or customer fetch fails transiently;AuthContextkeeps the in-memory user instead of clearing it. Cookies are cleared only on confirmed 401/403 via sharedisAuthError/clearAuthCookies.ensureFreshSession()adds astalestate when refresh fails but the refresh token remains;tryRefresh()no longer wipes cookies on generic errors.Refresh races — Concurrent refreshes for the same refresh token are coalesced so a losing race cannot rotate an already-invalidated token and wipe a good session.
Checkout — The email field locks only when
user?.emailis present (hasAccountEmail), not whenisAuthenticatedis true whileuseris still null (hydration or stale sync), avoiding a disabled blank email field.Tests cover stale vs auth-failure paths and refresh coalescing.
Reviewed by Cursor Bugbot for commit 699feba. Bugbot is set up for automated code reviews on this repo. Configure here.