fix(auth): redirect /login to dashboard if already logged in#368
fix(auth): redirect /login to dashboard if already logged in#368sneumannb5 wants to merge 2 commits into
Conversation
ConnysCode
left a comment
There was a problem hiding this comment.
Review: Comment — no blockers. The redirect is correct and safe; everything below is non-blocking.
What's solid (verified against head 13df95b)
- Uses
getSessionStatus()rather thangetAuthMe/getJson, so a 401 on this page returns{authenticated:false}instead of firingmaybeNavigateToLogin— that's the right call, it avoids a redirect loop on/login. (api.ts:1033,api.ts:108) - The existing
returnPathsanitizer (must start with/, rejects//) already guards the newrouter.replace(returnPath)— no open-redirect. (login/page.tsx:63-68) cancelledguard kept andstatestaysloadingthrough the probe, so the form does not flash before the redirect (covers the flash item in your test plan).
Non-blocking suggestions
- The session probe is sequential, on the hot path.
getAuthProviders()now waits forgetSessionStatus(), so the sign-in form renders one round-trip later for every visitor — and most people hitting/loginare logged-out. Optional parallelization in the inline note. - Loop if
returnPath === '/login'(reachable only via a hand-crafted URL — nothing in the app emits?return=/login). Optional hardening, inline.
PR description — factcheck
| Claim | Reality |
|---|---|
Test plan: /login?returnPath=/some/page → /some/page |
The query param is return, not returnPath (login/page.tsx:64; matches maybeNavigateToLogin / redirectIfUnauthorized, which build ?return=). As written, that manual step would land on /. Please fix the wording (or you may not have actually run that step). |
"Adds one getSessionStatus() call … negligible impact" |
Accurate on request count, but it runs before getAuthProviders() (sequential), so it adds latency to the logged-out path rather than zero. |
"redirect … to returnPath (default dashboard)" |
Accurate — default is / (login/page.tsx:65), which is the chat home. |
| "No schema, API, env-var, or CI changes" | Accurate — single file, +7/-0. |
npm run lint && npm run typecheck (unchecked) |
Not run on my side either (no node_modules; a full Next install was too heavy to be worth it). The change is type-trivial (correctly-typed, correctly-sorted import of an existing export). Please run both and tick the boxes before merge. |
Minimum to merge
Nothing blocking. Worth doing before merge: tick lint/typecheck, and fix the returnPath → return wording in the test plan. Optional: parallelize the two probes; guard return=/login; add a small redirect test.
Nits (non-blocking)
app/login/has onlypage.tsx— no test for the new authenticated→redirect branch. A small test mirroringapp/_components/__tests__/SessionWatcher.test.tsxwould lock the behavior in.login/page.tsx:143— during the redirect the page briefly shows theloadingProviderscopy ("loading providers"), which isn't quite what's happening. Cosmetic and near-instant; ignore unless trivial.
(Reviewed against head 13df95b. lint/typecheck not executed locally — deps not installed.)
| let cancelled = false; | ||
| (async () => { | ||
| try { | ||
| const session = await getSessionStatus(); |
There was a problem hiding this comment.
Sequential probe on the hot path (non-blocking). This makes getAuthProviders() wait for the session probe, so the sign-in form renders one round-trip later for every visitor — and the majority who reach /login are logged-out. Consider running both in parallel and only using providers when not authenticated:
const [session, providers] = await Promise.all([
getSessionStatus(),
getAuthProviders(),
]);
if (cancelled) return;
if (session.authenticated) {
router.replace(returnPath);
return;
}
// …use `providers` as todayThe only cost is one wasted providers fetch in the rare already-authenticated case. Optional.
| const session = await getSessionStatus(); | ||
| if (cancelled) return; | ||
| if (session.authenticated) { | ||
| router.replace(returnPath); |
There was a problem hiding this comment.
Optional: guard returnPath === '/login'. With ?return=/login, this redirects back to /login, which re-probes and redirects again — a loop. Nothing in the app currently emits ?return=/login (the builders at api.ts:114 and authRedirect.ts:37 use the current path and skip /login), so today this is only reachable via a hand-crafted URL — non-blocking. If you want to harden it: router.replace(returnPath === '/login' ? '/' : returnPath).
What
Redirect authenticated users away from
/loginto theirreturnPath(default dashboard) instead of rendering the sign-in form.Why
Visiting
/loginwhile already signed in showed the sign-in form, which was confusing and let users re-authenticate unnecessarily. Checking session status on mount short-circuits this and matches expected behavior.Test plan
npm run lint && npm run typecheckinweb-ui/login→ redirected to dashboard/login?returnPath=/some/page→ redirected to/some/page/login→ sign-in form renders as before/loginduring in-flight session check does not flash form before redirectRisk / blast radius
getSessionStatus()call on/loginmount — extra request only on login page load, negligible impact.