fix(auth): re-auth expired session at runtime - #182
Conversation
|
Warning Review limit reached
More reviews will be available in 23 minutes and 43 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAuth status now uses initializing, disabled, renewing, and reauth states with reason codes. OIDC redirect handling deduplicates sign-in and validates return URLs. Fetch retry, auth-gated UI, callback failure handling, tests, and translation copy were updated to use the new reauth flow. ChangesAuth recovery flow
Sequence Diagram(s)sequenceDiagram
participant Client
participant fetchWithAuth
participant OidcManager
participant HTTPFetch as "fetch"
Client->>fetchWithAuth: request
fetchWithAuth->>HTTPFetch: initial request
alt response is 401
fetchWithAuth->>OidcManager: refresh()
alt refresh returns token
fetchWithAuth->>HTTPFetch: retry request
alt retry response is 401
fetchWithAuth->>OidcManager: requireReauth("token_rejected")
end
else refresh returns null
fetchWithAuth->>OidcManager: requireReauth("refresh_failed")
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/components/auth-gate.tsx`:
- Around line 26-31: The auth gate’s redirect path drops failures from OIDC
sign-in, which can leave the app stuck on the terminal overlay. Update the
`useEffect` in `AuthGate` that calls `OidcManager.signIn()` so it catches
rejected promises and transitions to a recoverable retry/error state instead of
continuing to render `FullScreenLoading` forever. Use the existing `terminal`
flow and `AuthGate`/`OidcManager.signIn` symbols to add a fallback path that
allows retrying or unblocking the UI after a failed redirect attempt.
🪄 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
Run ID: fc224b9d-a378-4d04-8d3c-90fdb6fe2d54
📒 Files selected for processing (10)
src/auth/auth-policy.test.tssrc/auth/auth-policy.tssrc/auth/index.tssrc/auth/oidc-manager.test.tssrc/auth/oidc-manager.tssrc/components/auth-gate.test.tsxsrc/components/auth-gate.tsxsrc/components/full-screen-loading.tsxsrc/locales/en/translation.jsonsrc/routes/__root.tsx
| status, | ||
| error, | ||
| }: Pick<AuthSnapshot, "status" | "error">): boolean { | ||
| if (status === "unauthorized") return error !== "missing_oidc_config"; |
There was a problem hiding this comment.
"unauthorized" here already basically means 401, the raw response is just not exposed here
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
f8bc326 to
1dafd94
Compare
Replace the flat status + free-form error with a typed AuthStatus where "re-auth needed" is a status tag, not a predicate derived from magic strings. Drop isTerminalAuthFailure. Centralize the redirect in OidcManager.requireReauth(), called from the 401 path and the silent-renew failure, so the 401 check stays at the fetch boundary and the auth layer never references HTTP status. The gate is now pure presentation. Add a reauth_failed state with a retry so a redirect that fails to start can no longer pin the app behind a permanent overlay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
From a full-flow review of the auth/reauth path: - safeReturnUrl: resolve against our origin and echo back only a same-origin path+query+hash. The prefix checks let backslash network paths (/\host, folded to //host by browsers) through as open redirects. - signIn: never persist /callback as the return target — a fresh visit has no code and loops back into the failure screen. - fetchWithAuth: only fall back to a dev/impersonation bearer when OIDC is disabled, so a URL override can't mint an unsigned alg:none bearer while a real OIDC token is briefly null mid-renew. - root beforeLoad: route first-load through requireReauth so a redirect that can't start lands in reauth_failed (retry UI) instead of throwing an unhandled rejection out of beforeLoad. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/routes/callback.tsx (1)
61-68: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCatch
signIn()failures on retry.
OidcManager.signIn()can reject, and this handler drops that promise. A failed retry will surface as an unhandled rejection instead of staying fully contained on the existing error screen.Proposed fix
<AuthError title={t("auth.callback.failed_title")} message={ data.error === "missing_code" ? t("auth.callback.missing_code") : t("auth.callback.exchange_failed") } - onRetry={() => void OidcManager.signIn()} + onRetry={() => { + void OidcManager.signIn().catch(() => { + // Keep the current error UI mounted if redirect startup fails again. + }); + }} />🤖 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/routes/callback.tsx` around lines 61 - 68, The retry handler in AuthError currently calls OidcManager.signIn() without handling rejections, which can leave failed retries as unhandled promise rejections. Update the onRetry callback in the callback flow to catch errors from OidcManager.signIn() and keep the failure contained on the existing error screen, using the same callback/AuthError path for reference.
🧹 Nitpick comments (3)
src/api/fetch-with-auth.test.ts (1)
22-63: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for the disabled-only dev bearer path.
These tests only exercise the token-present retry flow. The security-sensitive part of this change is that
devBearer()is allowed only whenauthStore.status === "disabled"; if that regresses and starts minting unsigned bearers duringinitializing/renewing, this suite still passes. Please add one case fordisabled + dev emailand one forrenewing + dev email.🤖 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/api/fetch-with-auth.test.ts` around lines 22 - 63, The fetchWithAuth test suite is missing coverage for the dev-bearer-only path guarded by authStore.status, so add cases that exercise devBearer with a dev email in both the disabled and renewing states. Update the tests around fetchWithAuth, OidcManager, and any authStore/devBearer setup so one case verifies a bearer is minted only when authStore.status is "disabled", and another verifies no unsigned bearer is minted when status is "renewing". Keep the existing retry/reauth assertions intact while extending coverage to this status-based branch.src/auth/oidc-manager.test.ts (1)
8-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture event handlers so the expiry escalation path is tested.
The PR’s core runtime fix depends on
addAccessTokenExpiredandaddSilentRenewError, but the mock only records registration. Store and invoke those callbacks in tests to assertrenewingandreauth_requiredtransitions directly.🤖 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/auth/oidc-manager.test.ts` around lines 8 - 15, The oidc-client-ts mock in oidc-manager.test.ts only records event registration and never exercises the expiry callbacks, so the renewal escalation path is untested. Update the UserManager mock to capture the handlers passed to addAccessTokenExpired and addSilentRenewError, then invoke them in the relevant tests to assert the renewing and reauth_required state transitions in the OidcManager flow.src/auth/types.ts (1)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the
reauth_requiredowner contract.These lines say the setter also kicks
signIn(), butOidcManager.doInit()setsreauth_requiredfor the first-load path wherebeforeLoadowns the redirect. Please document that exception so future callers don’t accidentally depend on the wrong invariant.Suggested wording
- * reauth_required— renewal is no longer possible; an interactive redirect is - * needed. Whoever sets this also kicks `signIn()`. + * reauth_required— renewal is no longer possible, or no live session exists + * after init; an interactive redirect is needed. The setter + * must either start `signIn()` or run in the first-load path + * where the route guard owns the redirect.🤖 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/auth/types.ts` around lines 22 - 23, Clarify the ownership contract for reauth_required in the auth types documentation: the current comment implies the setter always triggers signIn(), but OidcManager.doInit() also sets reauth_required during the first-load path where beforeLoad owns the redirect. Update the wording near reauth_required to explicitly note this exception so callers do not assume signIn() is always the trigger.
🤖 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/auth/oidc-manager.ts`:
- Around line 68-76: The `addUserUnloaded` handler in `oidc-manager.ts` is using
the wrong status and can leave the app in a non-terminal `"renewing"` state
after logout. Update the `um.events.addUserUnloaded` callback to clear the token
and transition `authStore` to a terminal state such as `"requireReauth"` or
`"disabled"` instead of `"renewing"`, while keeping `"renewing"` only for actual
renewal paths like `addAccessTokenExpired` or `addSilentRenewError`.
- Around line 91-104: The `devBearer()` fallback is too broad because
`fetch-with-auth` currently treats any `authStore.status === "disabled"` as
eligible for impersonation. Update the fallback logic to also check the auth
reason exposed by `authStore.getSnapshot()` so `devBearer()` is only used when
the reason is `dev_bypass`, and not when `OidcManager` has set
`missing_oidc_config`. This keeps the intended development bypass while failing
closed in unconfigured production-like cases.
---
Duplicate comments:
In `@src/routes/callback.tsx`:
- Around line 61-68: The retry handler in AuthError currently calls
OidcManager.signIn() without handling rejections, which can leave failed retries
as unhandled promise rejections. Update the onRetry callback in the callback
flow to catch errors from OidcManager.signIn() and keep the failure contained on
the existing error screen, using the same callback/AuthError path for reference.
---
Nitpick comments:
In `@src/api/fetch-with-auth.test.ts`:
- Around line 22-63: The fetchWithAuth test suite is missing coverage for the
dev-bearer-only path guarded by authStore.status, so add cases that exercise
devBearer with a dev email in both the disabled and renewing states. Update the
tests around fetchWithAuth, OidcManager, and any authStore/devBearer setup so
one case verifies a bearer is minted only when authStore.status is "disabled",
and another verifies no unsigned bearer is minted when status is "renewing".
Keep the existing retry/reauth assertions intact while extending coverage to
this status-based branch.
In `@src/auth/oidc-manager.test.ts`:
- Around line 8-15: The oidc-client-ts mock in oidc-manager.test.ts only records
event registration and never exercises the expiry callbacks, so the renewal
escalation path is untested. Update the UserManager mock to capture the handlers
passed to addAccessTokenExpired and addSilentRenewError, then invoke them in the
relevant tests to assert the renewing and reauth_required state transitions in
the OidcManager flow.
In `@src/auth/types.ts`:
- Around line 22-23: Clarify the ownership contract for reauth_required in the
auth types documentation: the current comment implies the setter always triggers
signIn(), but OidcManager.doInit() also sets reauth_required during the
first-load path where beforeLoad owns the redirect. Update the wording near
reauth_required to explicitly note this exception so callers do not assume
signIn() is always the trigger.
🪄 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
Run ID: 221b5aaf-b050-4cdf-8a2a-889a16e85f4a
📒 Files selected for processing (14)
src/api/fetch-with-auth.test.tssrc/api/fetch-with-auth.tssrc/auth/auth-store.tssrc/auth/index.tssrc/auth/oidc-manager.test.tssrc/auth/oidc-manager.tssrc/auth/types.tssrc/components/auth-error.tsxsrc/components/auth-gate.test.tsxsrc/components/auth-gate.tsxsrc/components/full-screen-loading.tsxsrc/locales/en/translation.jsonsrc/routes/__root.tsxsrc/routes/callback.tsx
✅ Files skipped from review due to trivial changes (2)
- src/components/auth-error.tsx
- src/locales/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/full-screen-loading.tsx
| um.events.addUserUnloaded(() => { | ||
| authStore.setToken(null); | ||
| authStore.setStatus("expired"); | ||
| authStore.setStatus("renewing"); | ||
| }); | ||
|
|
||
| um.events.addAccessTokenExpired(() => { | ||
| authStore.setToken(null); | ||
| authStore.setStatus("expired"); | ||
| authStore.setStatus("renewing"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
oidc-client-ts UserManagerEvents addUserUnloaded when is it raised removeUser access token expired silent renew
💡 Result:
In oidc-client-ts, the addUserUnloaded event is raised when a user session has been terminated or explicitly removed [1][2]. This event is triggered internally by the library when the userManager.unload() method is called [2][3]. Crucially, the addUserUnloaded event is not automatically raised simply because an access token has expired [4]. Access token expiration triggers the addAccessTokenExpired event [1][4], and silent renew errors trigger the addSilentRenewError event [2][3]. If you want to perform a logout or clear the user state upon token expiration or renewal failure, you must explicitly implement that logic by registering callbacks for those events and calling userManager.removeUser() [4][5][6]. Regarding userManager.removeUser(): Calling userManager.removeUser() explicitly invokes the library's unload() process, which subsequently raises the addUserUnloaded event [2][7]. Key behaviors to note: 1. Automatic Silent Renew: If automaticSilentRenew is enabled, the library attempts to renew tokens automatically before expiration [8]. Failure during this process triggers addSilentRenewError, not addUserUnloaded [2][5]. 2. Sign-out flows: Calling userManager.signoutRedirect() or similar sign-out methods typically triggers the unload() process (and thus addUserUnloaded), though behavior regarding the exact timing of this event relative to the sign-out request can be influenced by configuration settings [9][10]. To ensure a user is logged out when a token expires or cannot be renewed, a common pattern is to register a callback for both addAccessTokenExpired and addSilentRenewError that calls userManager.removeUser() [5].
Citations:
- 1: https://authts.github.io/oidc-client-ts/interfaces/UserManagerEvents.html
- 2: https://github.com/authts/oidc-client-ts/blob/9bea2d897123bbc7ca656dfbb9547eb66d0114fb/src/UserManagerEvents.ts
- 3: https://cdn.jsdelivr.net/npm/oidc-client-ts@2.2.4/dist/types/oidc-client-ts.d.ts
- 4: Access token not renewed if expired authts/oidc-client-ts#1601
- 5: Expired refresh and access token does not log out user authts/oidc-client-ts#1435
- 6: Network error after waking up from sleep authts/oidc-client-ts#1343
- 7: addUserSignedOut event being fired when user is signed in authts/react-oidc-context#982
- 8: https://authts.github.io/oidc-client-ts/interfaces/UserManagerSettings.html
- 9:
UserManager.events().unload()event is triggered too early onUserManager.signoutRedirect()authts/oidc-client-ts#1341 - 10: add setting to configure when to raise the user unload event and remove user always before requesting sign-out authts/oidc-client-ts#1391
Correct state transition on addUserUnloaded to avoid stranding app in renewing.
The addUserUnloaded event in oidc-client-ts signals that the user session has been explicitly removed or terminated (e.g., via removeUser() or signoutRedirect), not that a silent renewal is in progress. Setting the status to "renewing" is semantically incorrect here because:
- The user context is gone; silent renewal cannot proceed.
AuthGatetreats"renewing"as a non-terminal state allowing access, which exposes protected content after the user has been logged out.
This should transition to a terminal state like "requireReauth" or "disabled" to force a full login flow. Use addAccessTokenExpired or addSilentRenewError only for actual renewal attempts.
Relevant Event Semantics
addAccessTokenExpired: Fires on token expiry (renewal candidate).addSilentRenewError: Fires when silent renewal fails (retry or fallback logic).addUserUnloaded: Fires when the user is explicitly unloaded/removed (terminal, requires re-login).
Reference: oidc-client-ts documentation and source logic.
🤖 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/auth/oidc-manager.ts` around lines 68 - 76, The `addUserUnloaded` handler
in `oidc-manager.ts` is using the wrong status and can leave the app in a
non-terminal `"renewing"` state after logout. Update the
`um.events.addUserUnloaded` callback to clear the token and transition
`authStore` to a terminal state such as `"requireReauth"` or `"disabled"`
instead of `"renewing"`, while keeping `"renewing"` only for actual renewal
paths like `addAccessTokenExpired` or `addSilentRenewError`.
Second-pass review follow-ups: - fetchWithAuth: mint a dev bearer only when status is "disabled" AND reason is "dev_bypass". An unconfigured prod deploy (missing_oidc_config) now fails closed instead of letting a URL override forge a bearer. - callback retry: catch signIn() rejection so a redirect that can't start stays contained on the error screen instead of surfacing as an unhandled promise rejection. - types: note that reauth_required is also set by doInit on first load, where beforeLoad (not the component tree) performs the redirect. - tests: cover the status/reason-gated dev-bearer branch and the renewal escalation path (access-token expiry -> renewing, silent-renew failure -> reauth_required) by exercising the captured oidc-client handlers. Skipped the addUserUnloaded -> renewing suggestion: with monitorSession off it only fires via signOut's removeUser, which immediately resets and redirects; a terminal/trigger state there would either be overwritten or race the signout redirect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
2 participants
Problem
Auth was enforced once, in the root route's
beforeLoad. On a long-open tab anexpired session never re-authenticated: after silent renew failed, every request
returned
401and widgets rendered error cells until a manual reload. Nothing inthe React tree reacted to the terminal auth state.
Fix
Add a reactive
<AuthGate>that watches auth status and triggers a single fullOIDC sign-in redirect on a terminal failure, rendering a redirect overlay instead
of the route content so no
401error cells paint. Plainexpiredis still leftto silent renew. Return-URL preservation and
refresh()de-duplication alreadyexisted and are unchanged.
Closes #1497
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes