feat(auth): restart login on auth_error callback bounces, loop-guarded - #239
Conversation
A failed OIDC callback used to dead-end the browser on the authenticator's problem+json — nothing was loaded at /auth/callback, so an expired login state (the 300 s Redis TTL, insight#2032) simply stopped the login. The authenticator now 302s such failures back to the SPA with a fixed auth_error=<reason> query parameter. Boot consumes the parameter before the session probe: a stale bounce on a live session is ignored, retryable reasons (state_expired, idp_error, invalid_callback, exchange_failed) restart the login once, and a sessionStorage attempt counter halts persistent failures on a new full-page error screen with a manual retry instead of looping browser -> IdP. access_denied never auto-retries — a silent SSO hop would just reproduce it. Safe to deploy ahead of the authenticator change: without the new parameter the boot path is unchanged. Part of insight#2032 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
- signIn() with no arg on auto-retry and the error screen's button: return to the current URL (already stripped of auth_error) instead of hardcoding "/", so a non-root default_return_to survives the bounce. - Strip an empty ?auth_error= without counting it against the retry budget (the authenticator always sends a reason; empty means a hand-crafted URL) so it can't ride into return_to and stick. - Reset the signIn redirect guard on bfcache restores (pageshow with persisted) — browser Back from the IdP used to leave the module's redirecting flag true, turning the "Try again" button into a silent no-op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds bounded OIDC callback-error handling, session-backed retry tracking, auth-aware bootstrap routing, localized login-error UI, and bfcache redirect recovery. ChangesAuth error recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant bootstrap
participant AuthError
participant Session
participant LoginError
Browser->>bootstrap: start application
bootstrap->>AuthError: consume auth_error
bootstrap->>Session: load session
Session-->>bootstrap: session state
bootstrap->>LoginError: render failure when needed
LoginError->>Session: signIn on retry
Possibly related PRs
Suggested reviewers: 🚥 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: 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 `@src/auth/auth-error.ts`:
- Around line 36-40: Update writeAttempts and its callers in auth-error.ts so it
returns whether persisting the retry counter succeeded, and require that result
when computing autoRetry in the returned object; if setItem fails, autoRetry
must be false even when the attempt count is otherwise within MAX_AUTO_RETRIES.
Add coverage for the setItem-only failure path.
In `@src/components/login-error.tsx`:
- Around line 29-33: Update the shared default return-URL calculation in useAuth
to append window.location.hash to the existing pathname-and-search value, so
signIn() preserves hash-routed destinations during manual retry from the login
error flow. Do not duplicate URL construction in the Button handler.
- Around line 20-40: Update the failure screen markup in the login error
component to use a main landmark instead of the outer div, promote the
loginFailedTitle text to an h1, and wrap the failure message in an assertive
live status region so screen readers announce it when the root is replaced.
Preserve the existing translations, styling, and retry behavior.
In `@src/main.tsx`:
- Around line 54-59: Update the retryable auth-error branch in the main
component to call signIn with the full current URL target, including
window.location.hash, so consumeAuthErrorParam()’s preserved fragment survives
login. Apply the same return-target change to the manual retry handler in the
LoginError component, preserving existing retry 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: ee35236c-11b9-4d4d-ba71-00e15478b521
📒 Files selected for processing (7)
src/auth/auth-error.test.tssrc/auth/auth-error.tssrc/auth/index.tssrc/auth/use-auth.tssrc/components/login-error.tsxsrc/locales/en/translation.jsonsrc/main.tsx
| if (authError?.autoRetry) { | ||
| // A fresh login fixes the retryable reasons (expired state after a | ||
| // slow IdP round-trip, IdP hiccup); the attempt counter halts a | ||
| // persistent failure on the error screen instead of looping. No-arg | ||
| // signIn: return to the current URL, already stripped of auth_error. | ||
| signIn(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the fragment as the login return target.
Line 59 uses no-argument signIn(), whose default return target excludes window.location.hash. The retry therefore loses the fragment that consumeAuthErrorParam() preserved. Pass the full current target here; apply the same change to the manual retry in src/components/login-error.tsx.
Proposed fix
- signIn();
+ signIn(
+ window.location.pathname +
+ window.location.search +
+ window.location.hash,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (authError?.autoRetry) { | |
| // A fresh login fixes the retryable reasons (expired state after a | |
| // slow IdP round-trip, IdP hiccup); the attempt counter halts a | |
| // persistent failure on the error screen instead of looping. No-arg | |
| // signIn: return to the current URL, already stripped of auth_error. | |
| signIn(); | |
| if (authError?.autoRetry) { | |
| // A fresh login fixes the retryable reasons (expired state after a | |
| // slow IdP round-trip, IdP hiccup); the attempt counter halts a | |
| // persistent failure on the error screen instead of looping. No-arg | |
| // signIn: return to the current URL, already stripped of auth_error. | |
| signIn( | |
| window.location.pathname + | |
| window.location.search + | |
| window.location.hash, | |
| ); |
🤖 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/main.tsx` around lines 54 - 59, Update the retryable auth-error branch in
the main component to call signIn with the full current URL target, including
window.location.hash, so consumeAuthErrorParam()’s preserved fragment survives
login. Apply the same return-target change to the manual retry handler in the
LoginError component, preserving existing retry behavior.
- Do not auto-retry an attempt that could not be persisted (getItem works, setItem throws): the next bounce would read zero again and the loop guard would never trip. writeAttempts now reports success and autoRetry requires it. - Preserve the URL hash in signIn's default return_to (covers both the auto-retry and the error screen's Try-again path). - Semantic markup on the login-error screen: main landmark, h1 title, role=alert on the message so screen readers announce it on mount. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
2 participants
Summary
FE half of constructorfabric/insight#2032. A failed OIDC callback used to dead-end the browser on the authenticator's problem+json — nothing is loaded at
/auth/callback, so an expired login state (the 300 s Redis TTL) simply stopped the login on a raw JSON page. The authenticator now 302s such failures back to the SPA with a fixedauth_error=<reason>query parameter (backend PR to follow); this PR makes the SPA consume it and go through the whole login machinery from scratch.Behavior
main.tsx) consumes?auth_error=before the session probe and strips it from the URL (reloads, copied links, and the next login'sreturn_tonever carry it).auth_erroris ignored, app renders normally.state_expired,idp_error,invalid_callback,exchange_failed) → one automaticsignIn()restart; a sessionStorage attempt counter halts a persistent failure on a new full-page error screen with a manual "Try again" instead of looping browser → IdP.access_denied(unknown person / no tenant / unknown view-as target) → error screen immediately; a silent SSO hop would just reproduce it.redirectingguard stucktrue, turning every latersignIn()into a silent no-op; it now resets onpageshow.Deploy order
Safe to deploy ahead of the authenticator change — without the new parameter the boot path is unchanged. The authenticator PR should not be deployed first: its
access_deniedbounce relies on this guard to avoid an SPA↔IdP redirect loop for unprovisioned users.Test plan
npm run test— 701 passed (8 new tests insrc/auth/auth-error.test.ts: consume/strip, hash preservation, empty value, retry budget, access_denied, storage-off fail-closed, budget reset).npm run typecheck,eslint src/— clean.Part of constructorfabric/insight#2032
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes