fix(auth): refresh expired access tokens and break the stale-session login loop - #3581
Conversation
…login loop The Auth0 session cookie outlives the access token and nothing ever refreshed it, so mid-session users were bounced to /login where the cached client user triggered an infinite login/gated-page redirect loop, and proxied API calls failed with 401. - /login now re-validates the session before trusting the cached user - proxied 401s trigger a session re-check so the client signs out cleanly when the server session is dead - getSession is wrapped server-side to renew expired access tokens with the session's refresh token via the oauth token endpoint Fixes DEPLOY-WEB-2C4
|
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 adds Auth0 refresh-token handling, refresh-aware session retrieval, session-expiry notification through Axios interceptors, client session synchronization, shared token-expiry checks, and mount-time authentication revalidation for ChangesSession lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3581 +/- ##
==========================================
- Coverage 76.43% 75.71% -0.72%
==========================================
Files 1137 1051 -86
Lines 29623 27364 -2259
Branches 7382 6935 -447
==========================================
- Hits 22641 20718 -1923
+ Misses 6153 5846 -307
+ Partials 829 800 -29
*This pull request uses carry forward flags. Click here to find out more.
🚀 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: 4
🤖 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 `@apps/deploy-web/src/components/user/SessionExpirySync/SessionExpirySync.tsx`:
- Around line 27-35: Update the session-expiry subscription callback in
SessionExpirySync to catch rejected checkSession calls and report the error
through LoggerService. Preserve the isReCheckingRef guard and finally-based
reset, ensuring the callback consumes failures so notify() cannot produce
unhandled promise rejections.
In
`@apps/deploy-web/src/lib/auth0/getSessionWithRefresh/getSessionWithRefresh.ts`:
- Around line 52-56: Update the failure branch in getSessionWithRefresh to
return null after clearSessionCookies instead of returning the expired session.
Preserve the existing warning and cookie-clearing behavior so all consumers
treat the current request as unauthenticated and redirect appropriately.
- Around line 33-42: Update refreshOncePerToken to retain the settled refresh
promise or rotated-token result for a short grace period instead of deleting the
inFlightRefreshes entry immediately in finally. Ensure requests arriving shortly
after settlement reuse the successful result rather than submitting the consumed
refresh token, while preserving cleanup after the grace period.
- Line 58: Update the session merge around Object.assign in
getSessionWithRefresh so undefined idToken and accessTokenScope values from
result.val cannot overwrite existing session values. Copy only defined
refreshed-token fields while preserving valid session values when Auth0 omits
them.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10dcbfe8-abac-4003-b9cd-59f294991f30
📒 Files selected for processing (13)
apps/deploy-web/src/components/auth/PasswordlessAuth/PasswordlessAuth.spec.tsxapps/deploy-web/src/components/auth/PasswordlessAuth/PasswordlessAuth.tsxapps/deploy-web/src/components/user/SessionExpirySync/SessionExpirySync.spec.tsxapps/deploy-web/src/components/user/SessionExpirySync/SessionExpirySync.tsxapps/deploy-web/src/components/user/UserProviders/UserProviders.tsxapps/deploy-web/src/lib/auth0/getSessionWithRefresh/getSessionWithRefresh.spec.tsapps/deploy-web/src/lib/auth0/getSessionWithRefresh/getSessionWithRefresh.tsapps/deploy-web/src/services/app-di-container/app-di-container.tsapps/deploy-web/src/services/app-di-container/server-di-container.service.tsapps/deploy-web/src/services/session-expiry-notifier/session-expiry-notifier.service.spec.tsapps/deploy-web/src/services/session-expiry-notifier/session-expiry-notifier.service.tsapps/deploy-web/src/services/session/session.service.spec.tsapps/deploy-web/src/services/session/session.service.ts
Return null (not the expired session) when refresh fails so callers treat the request as unauthenticated instead of proxying a dead access token. Merge only defined refreshed-token fields so Auth0 omitting id_token/scope on a refresh exchange no longer wipes the session's still-valid values. Catch a rejected checkSession in SessionExpirySync so it logs instead of leaking an unhandled rejection. Back the expiry notifier with EventTarget.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, this run also examined and ruled out a few other candidate issues: an unhandled-rejection concern in PasswordlessAuth's revalidateSessionOnMount (Auth0's checkSession() never actually rejects, so nothing escapes .finally()); a broader claim that a failed re-check never breaks the redirect loop (it does for the common clean-401 case — only the narrower network-failure case already covered by an inline nit is affected); and a per-request idempotency concern for getSession (SSR guards share one memoized getCurrentSession() call via once() in defineServerSideProps.ts, and concurrent refreshes for the same token are deduped inside getSessionWithRefresh).
Extended reasoning...
This is a record-only note about candidate issues the bug-hunting system's finders raised and its verifiers refuted this run — it does not restate the inline findings already attached to this PR. I independently checked each: the Auth0 client SDK's checkSession() catches fetch errors internally and never rejects, so there is no unhandled-rejection path in revalidateSessionOnMount; the clean-401 revalidation path does clear a stale cached user (getProfile returns undefined on 401, clearing state.user), so the redirect-loop fix works for the common case — only the narrower non-401 network-failure scenario already flagged inline remains open; and getCurrentSession in defineServerSideProps.ts memoizes the session lookup per request via once(), while getSessionWithRefresh's in-flight map dedupes concurrent refreshes for the same token regardless of caller, so per-request idempotency holds.
A transient refresh failure (Auth0 429 or an unknown 4xx) was clearing the session cookies just like a real invalid_grant, force-logging-out a user whose refresh token is still valid; now only invalid_grant clears them so the next request can retry. refreshAccessToken rejects rather than returning Err on a 5xx or network error (validateStatus is <500), and the un-caught await turned a transient Auth0 blip into a 500 SSR page since requireAuth runs before the handler try/catch; wrap the refresh in try/catch and degrade to null.
revalidateSessionOnMount fired checkSession for every /login visitor, adding a redundant /api/auth/me for anonymous visitors the app already fetched on boot; skip it when no user was cached at mount. Auth0's checkSession keeps the cached user and only sets error when the profile re-fetch fails (network/5xx) rather than a clean 401, so a transient failure left a stale user in place and leaveWhenAuthenticated navigated back onto it, reproducing the redirect loop; gate navigation and the boot loader on the absence of that error so a transient failure falls back to the login form. Expose error from the useUser wrapper.
There was a problem hiding this comment.
I reviewed this PR and found no new bugs. Because it touches security-sensitive session/token-refresh logic (SSR auth guards, /api/auth/me, and the client-side session-expiry convergence flow) and is a large, multi-file change, a human look would still be worthwhile.
What was reviewed: the server-side refresh wrapper's concurrency dedup and failure-mode handling (invalid_grant vs. transient errors), the client revalidate-on-mount flow in PasswordlessAuth for stale-user/error handling, and the 401-driven SessionExpirySync convergence path. I also checked the two specific concerns noted as ruled out this run (raw getSession on the template/[id] page, and SessionExpirySync's ability to clear a stale user for RequireAuth pages) and found them not to be real issues.
Extended reasoning...
Beyond the inline history on this PR (already visible in the thread — prior rounds found and the author fixed the unconditional-cookie-clear-on-transient-failure bug, the uncaught-throw-on-SSR bug, and the stale-user revalidation race), this run's finders raised three additional candidates that verifiers refuted: (1) the template/[id] page allegedly bypassing the refresh wrapper via raw getSession, (2) SessionExpirySync's auto-recheck allegedly being unable to clear a stale user for RequireAuth-gated pages on a transient failure, and (3) the notification-channels page allegedly double-calling the refresh-wrapped getSession with a stale/consumed ref. None of these held up as real bugs in verification.
The remaining two open inline comments from prior rounds (the isAccessTokenExpired predicate duplicated across getSessionWithRefresh.ts/pageGuards.ts/the auth0 profile handler, and the PROXIED_API_BASE_URL literal duplicated across session-expiry-notifier.service.ts/interceptors.ts/browser-di-container.ts) are explicitly filed as non-blocking nits about future drift risk, not correctness defects.
Given the change touches auth guards, token refresh, and session cookie handling directly, and is sized L across 14 files with non-trivial concurrency (in-flight refresh dedup) and failure-mode branching (invalid_grant vs rate_limited vs unknown vs throw), I believe this warrants a human's eyes even though no outstanding correctness bugs remain in the current diff.
Extract isAccessTokenExpired into one helper used by the page guards, the auth0 profile handler, and the refreshing session wrapper, so a rule change like clock-skew leeway lands in one place instead of four. Export PROXY_API_BASE_URL from the withUserToken interceptor and reuse it in the API SDK factory and the session-expiry interceptor's 401 check, so a route rename can't silently drift the copies apart.
Auth0's checkSession resolves and stores a failed profile fetch in the hook's error state rather than rejecting, so SessionExpirySync's catch never fired in production; observe error and log SESSION_RECHECK_FAILED from there while keeping the catch as a defensive guard. In PasswordlessAuth, gate the redirect-to-entry guard on !error to match leaveWhenAuthenticated and the boot loader, so a stale user whose re-check errors on a missing-email verify step still gets redirected instead of seeing a blank screen.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/deploy-web/src/lib/auth0/isAccessTokenExpired/isAccessTokenExpired.spec.ts (1)
5-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
setup()helper for the test cases.The spec creates its fixtures inline and does not use
setup(). Add a local helper that returns independent session fixtures. This keeps the test structure consistent without introducing shared mutable state.Proposed fixture structure
-const NOW_SECONDS = Math.floor(Date.now() / 1_000); +function setup() { + const nowSeconds = Math.floor(Date.now() / 1_000); + return { + futureSession: { accessTokenExpiresAt: nowSeconds + 3_600 }, + expiredSession: { accessTokenExpiresAt: nowSeconds - 60 } + }; +} describe(isAccessTokenExpired.name, () => { it("returns false when the access token expires in the future", () => { - expect(isAccessTokenExpired({ accessTokenExpiresAt: NOW_SECONDS + 3_600 })).toBe(false); + const { futureSession } = setup(); + expect(isAccessTokenExpired(futureSession)).toBe(false); }); it("returns true when the access token has already expired", () => { - expect(isAccessTokenExpired({ accessTokenExpiresAt: NOW_SECONDS - 60 })).toBe(true); + const { expiredSession } = setup(); + expect(isAccessTokenExpired(expiredSession)).toBe(true); });As per path instructions,
**/*.spec.tstests must use asetup()function instead of shared mutable state.🤖 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 `@apps/deploy-web/src/lib/auth0/isAccessTokenExpired/isAccessTokenExpired.spec.ts` around lines 5 - 23, Update the isAccessTokenExpired test suite to add a local setup() helper that returns fresh, independent session fixtures for each case. Refactor the existing tests to obtain their inputs through setup() instead of constructing shared or inline fixtures, while preserving the current expiration and null-session assertions.Source: Path instructions
🤖 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
`@apps/deploy-web/src/lib/auth0/isAccessTokenExpired/isAccessTokenExpired.spec.ts`:
- Around line 5-23: Update the isAccessTokenExpired test suite to add a local
setup() helper that returns fresh, independent session fixtures for each case.
Refactor the existing tests to obtain their inputs through setup() instead of
constructing shared or inline fixtures, while preserving the current expiration
and null-session assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f76a18f-f590-4f7e-b459-4f42bba605c9
📒 Files selected for processing (12)
apps/deploy-web/src/components/auth/PasswordlessAuth/PasswordlessAuth.spec.tsxapps/deploy-web/src/components/auth/PasswordlessAuth/PasswordlessAuth.tsxapps/deploy-web/src/components/user/SessionExpirySync/SessionExpirySync.spec.tsxapps/deploy-web/src/components/user/SessionExpirySync/SessionExpirySync.tsxapps/deploy-web/src/lib/auth0/getSessionWithRefresh/getSessionWithRefresh.tsapps/deploy-web/src/lib/auth0/isAccessTokenExpired/isAccessTokenExpired.spec.tsapps/deploy-web/src/lib/auth0/isAccessTokenExpired/isAccessTokenExpired.tsapps/deploy-web/src/lib/nextjs/pageGuards/pageGuards.tsapps/deploy-web/src/pages/api/auth/[...auth0].tsapps/deploy-web/src/services/app-di-container/browser-di-container.tsapps/deploy-web/src/services/auth/auth/interceptors.tsapps/deploy-web/src/services/session-expiry-notifier/session-expiry-notifier.service.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/deploy-web/src/services/session-expiry-notifier/session-expiry-notifier.service.ts
- apps/deploy-web/src/components/user/SessionExpirySync/SessionExpirySync.spec.tsx
- apps/deploy-web/src/lib/auth0/getSessionWithRefresh/getSessionWithRefresh.ts
- apps/deploy-web/src/components/auth/PasswordlessAuth/PasswordlessAuth.tsx
- apps/deploy-web/src/components/auth/PasswordlessAuth/PasswordlessAuth.spec.tsx
- apps/deploy-web/src/components/user/SessionExpirySync/SessionExpirySync.tsx
The error-observing effect logged on any transition of the shared useUser error to truthy, so a failure unrelated to a notifier-triggered re-check — the app-boot profile fetch or PasswordlessAuth's /login re-check — was mislabeled as a session-expiry re-check failure. Gate the log on a flag set only when this component initiates the re-check (isReCheckingRef resets in finally before the effect commits, so it can't serve as the gate) and clear it once the outcome is observed.
The prior scoping gated the log on a ref cleared only inside the outcome effect, whose deps were [error, logger] — so a re-check that resolved without changing error (a clean refresh or 401) never re-ran the effect, left the flag set, and mislabeled the next unrelated error. Tick a settle counter in finally so the outcome effect runs after every re-check regardless of whether error changed, and report only when that counter advances past the last one seen.
There was a problem hiding this comment.
This automated review ran and found no new bugs this round. Given the history of subtle timing/telemetry bugs already surfaced and fixed across this PR's iterations, and that this touches server-side session/token-refresh logic (auth-sensitive code), a human look is still worthwhile before merging.
What was reviewed: the token-refresh wrapper's in-flight dedupe and error/cookie-clearing paths in getSessionWithRefresh, the PasswordlessAuth revalidation guards (leaveWhenAuthenticated / redirectToEntryWhenEmailMissing / boot-loader gating), and SessionExpirySync's recheck-outcome telemetry gating. A candidate concern that getSession's new statefulness could cause a pre-existing bypass of getCurrentSession() to double-refresh was examined and ruled out.
Extended reasoning...
Overview
This PR adds server-side access-token refresh (createGetSessionWithRefresh wrapping getSession, with a new SessionService.refreshAccessToken and an in-flight-dedupe map keyed by refresh token), client-side session revalidation on /login (PasswordlessAuth now calls checkSession() before trusting a cached user), and a client convergence path for 401s (SessionExpiryNotifier -> SessionExpirySync re-checking the session, with careful gating to avoid mislabeling unrelated auth errors as recheck failures). It touches 20 files across apps/deploy-web, including the DI container wiring for both browser and server, the auth0 catch-all API route, and page guards.
Security risks
The core risk surface is session/token handling: the refresh token exchange (grant_type=refresh_token) against Auth0, cookie clearing on invalid_grant vs. leaving cookies intact on transient failures (rate limit, 5xx, network error), and the in-flight refresh dedupe keyed by refresh token value (bounded by concurrent requests per token, not attacker-controlled). No user input flows into these paths beyond the session's own refresh token; no new endpoints or auth bypass were introduced. I didn't find an injection, auth-bypass, or data-exposure issue in this diff, but this is exactly the kind of surface where a subtle logic error has outsized blast radius (e.g., silently keeping a dead session alive, or logging out active users) — which the PR's own iteration history bears out.
Level of scrutiny
This warrants more than a mechanical-change pass. It's a genuine logic change to production auth flows (not a config tweak), with real concurrency and React-effect-timing subtleties (the SessionExpirySync recheck-outcome gating went through five iterations to correctly avoid both false negatives and false positives in telemetry). The bug-hunting system already found and the author already fixed several real issues in earlier rounds (a stale-user redirect-loop gap, a dead catch block, telemetry misattribution twice). This round found nothing new, and one candidate (getSession's added statefulness enabling a double-refresh via a pre-existing bypass of getCurrentSession()) was investigated and refuted, but given the density of subtle bugs already surfaced in this exact PR, a human's read of the final diff is warranted before merge.
Other factors
Test coverage is substantial (21 new unit tests per the PR description, covering the refresh wrapper, the notifier, and the revalidation guards), and the author has been responsive, fixing each finding with a follow-up commit and a matching test. That responsiveness and coverage lower risk but don't substitute for a human sign-off on security-sensitive auth code per project guidelines.
2 participants
Why
Fixes DEPLOY-WEB-2C4 (Sentry issue, 9.8k events / 306 users).
The Auth0 session cookie outlives the access token, and nothing refreshes the token. Login already requests `offline_access`, so every session carries a refresh token, but no code path used it. Once the token expired, two things went wrong:
What
Refresh tokens on our tenant are non-rotating, which the refresh call handles (it keeps the input token when Auth0 does not return a new one). If refresh token rotation is ever enabled on the Auth0 application, the rotation overlap period should be set to 30 to 60 seconds at the same time so concurrent refreshes across pods do not trip breach detection.
Verified with 21 new unit tests (full deploy-web suite green: 314 files / 3,062 tests), lint, tsc delta, and a local `next dev` smoke test of /login SSR, `/api/auth/me`, and the proxy route. After deploy, `ACCESS_TOKEN_REFRESHED` / `ACCESS_TOKEN_REFRESH_FAILED` log events show the refresh path working in production, and the DEPLOY-WEB-2C4 event rate should drop off.
Summary by CodeRabbit
New Features
Bug Fixes