Skip to content

Stop the dashboard reloading itself when it cannot authenticate - #378

Merged
Babissimo merged 1 commit into
mainfrom
fix/dashboard-401-redirect-loop
Sep 14, 2026
Merged

Babissimo merged 1 commit into
mainfrom
fix/dashboard-401-redirect-loop

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

Prerequisite for #376. Found by auditing what depends on the anonymous-admin
bypass, not by the diff review on that PR, because nothing in the diff is wrong:
the bug is in code #376 never touches and has simply never been reached.

The loop

dashboard/src/api/client.ts answered every 401 with
window.location.href = "/login". AuthProvider wraps the whole router,
including the /login route (dashboard/src/main.tsx:13), so on the login page
the sequence is: mount, GET /api/auth/me, 401, assign /login while already on
/login, which reloads, which remounts, which calls again. Where nothing can
mint a session it does not terminate, and the card is visible only between
reloads.

Why it has never happened

AUTH_BYPASS is true only when no OAuth client is configured, so the bypass
being set in every deployed environment is also proof that none of them can issue
a session. While it was set, /api/auth/me answered 200 to anyone and this path
was unreachable. #376 removes the flag, and dash.retina.fm has no Cloudflare
Access application in front of it by design, so that vhost lands here on every
load.

It is also why no test caught it: the oauth branch of
frontend/e2e/dashboard.spec.ts has never once executed against a deployed
environment, every one of them having reported bypass.

The change

Navigate only when the caller is elsewhere in the app. A 401 also stops being a
bare Error: AuthProvider could not distinguish it from the network and
timeout failures its four-attempt backoff exists for, so a settled answer sat
behind a loading state for about nine seconds before the card appeared.

Scope is deliberately just the loop. Whether there should be a user-facing auth
story at all is open, and this does not presume one — it only makes the
unauthenticated state settle instead of thrash.

Verification

  • Three tests added, all failing first, and both guards mutation-checked
    individually: removing the path check fails the already-on-login test, removing
    the break fails the settles-without-retrying test, one each.
  • Full dashboard suite 56 passed, npm run typecheck clean, npm run build
    clean, pre-commit run --all-files clean.
  • Nothing else in dashboard/src keyed off the old error message.

Touches only dashboard/, so it is disjoint from #376 and the two can land in
either order.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

The API client answered every 401 by assigning window.location.href = "/login".
On the login page that is a same-URL assignment, which reloads; the reload
remounts AuthProvider, which calls /api/auth/me, which 401s, which assigns it
again. Where nothing can mint a session the loop does not terminate, and the
login card is only visible between reloads.

Nothing has hit this yet because no deployed environment has ever reported the
enforced auth mode: AUTH_BYPASS is only true when no OAuth client is configured,
and while it was set /api/auth/me answered 200 to everyone. Closing the
anonymous-admin bypass is what makes dash.retina.fm reach this path, and that
vhost has no Cloudflare Access application in front of it by design.

So: navigate only when the caller is somewhere else in the app. The comparison
trims trailing slashes, because the router matches /login/ to the same route and
a raw string compare would let that spelling through for one more reload.

A 401 also stops being a bare Error, because AuthProvider could not tell it from
the network and timeout failures its four-attempt backoff exists for, and
spending those on a settled answer held the login card behind a loading state for
around nine seconds.

Every guard is mutation-checked one at a time: removing the path check fails the
already-on-login test, removing the slash trim fails its /login/ case, and
removing the break fails the settles-without-retrying test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the fix/dashboard-401-redirect-loop branch from 1af189b to cfbf601 Compare September 14, 2026 14:47
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 1m 54s —— View job


Review: fix/dashboard-401-redirect-loop (PR #378)

  • Read PR description and diff (single commit cfbf601, diff vs its parent since the PR is one commit)
  • Re-reviewed dashboard/src/api/client.ts, dashboard/src/context/AuthContext.tsx, dashboard/src/test/authUnauthorized.test.tsx
  • Compared against the earlier review on this PR to see what changed
  • Posted review

This is an update to the PR since the last review — it addresses both concrete findings from that pass.

Previously flagged, now fixed:

  1. Trailing-slash gap (/login/)onLoginPage() (dashboard/src/api/client.ts:19-21) now trims trailing slashes with pathname.replace(/\/+$/, "") before comparing, and it.each(["/login", "/login/"]) (dashboard/src/test/authUnauthorized.test.tsx:43-53) covers both spellings. This closes the one-extra-reload gap from before.
  2. Route coupling called out — the check now lives behind a named LOGIN_PATH constant with a /** Must match the route in App.tsx. */ comment (dashboard/src/api/client.ts:13-14), which is a reasonable amount of structure for the stated scope; it does verifiably match App.tsx:63's <Route path="/login" .../>.

Still open, as noted last time (not blocking, out of the PR's stated scope):

  1. Parallel 401s in the same tick each still reassign location.href redundantly before the first navigation lands — harmless (same target URL, page is unloading) but unchanged.
  2. AuthContext.logout() (dashboard/src/context/AuthContext.tsx:36-39) still has no UnauthorizedError handling. If POST /api/auth/logout 401s, client.ts's redirect fires but the thrown UnauthorizedError propagates uncaught through logout()Header.handleLogout() (dashboard/src/components/Header.tsx:19-21), so setUser(null) and the SPA's own navigate("/login") never run — cleanup still relies entirely on the hard redirect. Low impact given the redirect masks it, and reasonable to leave as a follow-up given the PR's explicit scope note, but flagging again since it's the one remaining place UnauthorizedError isn't used where it now could be: Fix this →

Correctness check on the new logic: AuthContext.tsx:23's if (e instanceof UnauthorizedError) break; correctly falls through to the existing setUser(null); setLoading(false) after the loop (line 29), so the settle-as-signed-out path is exercised as claimed, and the three added tests map 1:1 to the guards described in the commit message.

Not run: dashboard/node_modules isn't installed in this environment and installing/fetching would need approval I don't have here, so I read the tests and traced the logic statically rather than executing npm test/typecheck/build. The PR description states these were run and passed; nothing in the diff gives me reason to doubt that.

Nothing here blocks merge — #3 and #4 are the same pre-existing, low-impact, explicitly-out-of-scope items from the prior pass, now just narrower in scope since #1 and #2 are resolved.

@Babissimo
Babissimo merged commit 389e815 into main Sep 14, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant