Auto-redirect dashboard to login on 401 instead of a dead-end error#75
Merged
Conversation
When a dashboard session expired, the SPA's /api/v1 calls came back 401
and the UI painted an "Authentication required" card with no way to
re-auth — the only escape was manually visiting /dashboard, signing out,
then signing back in.
- web/src/api/client.ts: on a 401 from apiGet/apiSend, bounce the whole
page to /dashboard/auth/login?next=<current page> (which 302s to GitHub
OAuth). Centralized in the fetch wrapper so every API call is covered.
Skips demo mode and public /public/* share reads (no-auth, served before
the auth gate); a one-shot flag redirects once when a screen fires many
parallel queries.
- src/dashboard/auth.ts: broaden the OAuth `next` guard from
startsWith("/dashboard") to isSafeReturnPath() so SPA routes (served at
/) round-trip back to where the user was, while still rejecting
open-redirects (//evil.com, /\evil.com, absolute URLs).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughFixed the dashboard auth flow so SPA API requests now redirected users to login on 401 instead of leaving them on a dead-end error state. Expanded post-login return path handling to safely restore both SPA and legacy dashboard routes. Changes
Sequence Diagram(s)sequenceDiagram
participant SPA
participant API as /api/v1
participant Auth as /dashboard/auth/login
participant OAuth as GitHub OAuth
SPA->>API: fetch protected resource
API-->>SPA: 401 unauthorized
SPA->>Auth: navigate with next=current route
Auth-->>OAuth: redirect to provider
OAuth-->>Auth: callback with auth code
Auth-->>SPA: redirect back to saved route
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes Suggested Labels
Risk AssessmentScore: 17/100 — 🟡 Moderate
Test Coverage Signal🔴 47 line(s) of production code added with no test changes.
🧭 Description Drift
Compares PR description claims to the actual diff. Update the description (or the code) so they tell the same story. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in your ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Generate docstrings (beta)
🧹 Simplify (beta)
🪄 Autofix unresolved comments (beta)
Comment |
![diffsentry[bot]](https://avatars.githubusercontent.com/in/3360270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
This change cleanly fixes the expired-session dead end by centralizing 401 handling in the SPA client and broadening post-login return paths without opening obvious external redirects. The server-side isSafeReturnPath() guard is a good improvement, but the client-side redirect currently builds the login URL via string concatenation against an unvalidated window.location value, which can be tripped up by unusual path forms and is safer to construct with URL/URLSearchParams.
🧹 Nitpick comments (1)
web/src/api/client.ts (1)
Line 56: Build the login redirect URL with URL APIs instead of manual string concatenation.🛠️ Refactor suggestion | 🟡 Minor
Build the login redirect URL with URL APIs instead of manual string concatenation.
redirectToLogin()currently interpolatesreturnTodirectly into a path string. WhileencodeURIComponent()protects the query value, constructing navigation targets by concatenation is brittle around edge-case path forms and makes the intended same-origin redirect semantics less explicit.Using
new URL()plussearchParams.set()keeps the redirect target normalized and avoids accidental malformed URLs if the login route ornexthandling changes later. It also makes the code consistent with the rest of this file, which already relies on URL primitives for request construction.🔧 Proposed fix
function redirectToLogin(path: string): void { // Nothing to redirect in SSR/tests or demo mode. Public reads (/public/*) are // served BEFORE the auth gate, so a 401 there isn't a session problem — never // hijack the no-auth share viewer into a GitHub login. if (typeof window === "undefined" || DEMO || path.startsWith("/public/")) return; // A screen fires many queries at once; the first 401 wins the redirect and the // rest are no-ops, so we never stack navigations. if (redirectingToLogin) return; redirectingToLogin = true; const returnTo = window.location.pathname + window.location.search + window.location.hash; const loginUrl = new URL("/dashboard/auth/login", window.location.origin); loginUrl.searchParams.set("next", returnTo); window.location.assign(loginUrl.toString()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/api/client.ts around line 56, change `redirectToLogin()` to construct the `/dashboard/auth/login` navigation target with `new URL()` and `searchParams.set("next", returnTo)` instead of manual string concatenation; keep the existing `redirectingToLogin` guard and `/public/*` bypass logic intact. Preserve the exact `returnTo` composition from `window.location.pathname + window.location.search + window.location.hash`.🧠 Prior discussions on this file
♻️ Suggested fix
function redirectToLogin(path: string): void { // Nothing to redirect in SSR/tests or demo mode. Public reads (/public/*) are // served BEFORE the auth gate, so a 401 there isn't a session problem — never // hijack the no-auth share viewer into a GitHub login. if (typeof window === "undefined" || DEMO || path.startsWith("/public/")) return; // A screen fires many queries at once; the first 401 wins the redirect and the // rest are no-ops, so we never stack navigations. if (redirectingToLogin) return; redirectingToLogin = true; const returnTo = window.location.pathname + window.location.search + window.location.hash; const loginUrl = new URL("/dashboard/auth/login", window.location.origin); loginUrl.searchParams.set("next", returnTo); window.location.assign(loginUrl.toString()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/api/client.ts around line 56, change `redirectToLogin()` to construct the `/dashboard/auth/login` navigation target with `new URL()` and `searchParams.set("next", returnTo)` instead of manual string concatenation; keep the existing `redirectingToLogin` guard and `/public/*` bypass logic intact. Preserve the exact `returnTo` composition from `window.location.pathname + window.location.search + window.location.hash`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/api/client.ts`:
- Line 56: In web/src/api/client.ts around line 56, change `redirectToLogin()` to construct the `/dashboard/auth/login` navigation target with `new URL()` and `searchParams.set("next", returnTo)` instead of manual string concatenation; keep the existing `redirectingToLogin` guard and `/public/*` bypass logic intact. Preserve the exact `returnTo` composition from `window.location.pathname + window.location.search + window.location.hash`.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 41980526-699e-4502-a905-0cc58a9dd040
📥 Commits
Reviewing files at 537a01a (base SHA unavailable).
📒 Files selected for processing (2)
src/dashboard/auth.tsweb/src/api/client.ts
📌 Status — last updated
|
| Risk score | 17/100 (Moderate) ▁ |
| Unresolved threads | 1 |
| Failing checks | 0 |
| Pending checks | 1 |
| Files reviewed | 2 |
| Updated | 2026-07-07 18:18Z |
Live-updated by DiffSentry on every push. Use @diffsentry ship for a verdict, @diffsentry timeline for full history.
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
1 participant
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.
Problem
When a dashboard session expired, the web SPA's
/api/v1/*calls came back401and the UI just painted an "Authentication required" error card with no way to re-auth. The server only redirected to login for the legacy/dashboardSSR pages — never for the SPA at/— so the only escape was manually visiting/dashboard, signing out, then signing back in. Terrible UX.Fix
1.
web/src/api/client.ts— auto-redirect on 401. Centralized in theapiGet/apiSendfetch wrapper so every API call is covered: a401now bounces the whole page to/dashboard/auth/login?next=<current page>, which 302s to GitHub OAuth. Guards:/public/*share reads (no-auth, served before the auth gate — a public viewer is never hijacked into a GitHub login).401on a cookie-session request unambiguously means "session expired" (the server only 401s when OAuth is enabled), so this never fires spuriously in open mode.2.
src/dashboard/auth.ts— return you to where you were. The OAuthnextparam was hard-restricted tostartsWith("/dashboard"), so post-login you'd land on the legacy dashboard instead of the SPA page you left. Replaced with anisSafeReturnPath()guard that accepts any same-origin absolute path (SPA routes and legacy) while rejecting open-redirects (//evil.com,/\evil.com, absolute URLs). Applied at both the login entry and the OAuth callback.Verification
smoke:dashboard(real auth flow): passed, including the unauthenticated→login redirect./overviewand/repos/acme/checkout-api/pr/142now round-trip,/dashboard/settingsstill works, and//evil.com//\evil.com/https://evil.comall fall back to/dashboard.Note: the client-side redirect was verified via typecheck + logic review; it needs a real 401 from an OAuth-enabled instance to observe live in a browser.
🤖 Generated with Claude Code
Summary
Fixed the dashboard auth flow so SPA API requests now redirected users to login on 401 instead of leaving them on a dead-end error state. Expanded post-login return path handling to safely restore both SPA and legacy dashboard routes.
Changes
src/dashboard/auth.tsweb/src/api/client.ts