Closes#6. Phase 1, step 3 (final) of the tracking epic #20. Consumes the backend from #4/#5.
What's here
lib/auth.ts — zod-validated User/Providers; a shared meQueryOptions (a 401 resolves to null, not an error) reused by useMe() and the router guard; useProviders(); and useLogin/useRegister/useLogout mutations that prime/clear the me cache (logout also drops non-auth cached data so the next user never sees the previous one's).
lib/queryClient.ts — one QueryClient shared by the React tree and the router context, so guards and components read the same cache.
router.tsx — createRootRouteWithContext with the queryClient; requireAuth on gardens/plants/editor (→ /login?redirect=<dest>), requireGuest on login/register. Login search params (redirect, error) validated as optional; redirect targets restricted to same-origin paths.
LoginPage — OIDC button (label from providers) → /auth/oidc/login, an "or" divider, and the local email/password form; friendly messages for the OIDC callback ?error= codes (no_email, email_unverified, oidc_conflict, …).
RegisterPage — email + display name + password (min 8); registration-closed and SSO-only handling; auto-login lands on /gardens.
AppShell — auth-aware nav: user's display name + Sign out when logged in, Sign in otherwise; nav links only when authed.
ui/ — TextField (16px text to avoid iOS zoom, autocomplete + a11y), Button (+ shared buttonClasses for the OIDC anchor), Alert; AuthCard.
Verification
npx tsc --noEmit clean; vite build succeeds.
Real-browser E2E against the embedded single binary (Playwright):
register → auto-login → /gardens; full-page refresh stays signed in;
Sign out → /login, session cleared;
logged-out /gardens/1 → /login?redirect=%2Fgardens%2F1, and logging in returns you to the destination;
with OIDC configured, the login page shows the "Sign in with Authentik" button + local form (screenshot shared in-session).
Out of scope
Account settings / password change (post-v1); the Gardens list/create flow (#8). The pages here are the auth surface only.
Closes #6. Phase 1, step 3 (final) of the tracking epic #20. Consumes the backend from #4/#5.
## What's here
- **`lib/auth.ts`** — zod-validated `User`/`Providers`; a shared `meQueryOptions` (a **401 resolves to `null`, not an error**) reused by `useMe()` and the router guard; `useProviders()`; and `useLogin`/`useRegister`/`useLogout` mutations that prime/clear the `me` cache (logout also drops non-auth cached data so the next user never sees the previous one's).
- **`lib/queryClient.ts`** — one `QueryClient` shared by the React tree and the router context, so guards and components read the same cache.
- **`router.tsx`** — `createRootRouteWithContext` with the queryClient; `requireAuth` on gardens/plants/editor (→ `/login?redirect=<dest>`), `requireGuest` on login/register. Login search params (`redirect`, `error`) validated as optional; redirect targets restricted to same-origin paths.
- **`LoginPage`** — OIDC button (label from providers) → `/auth/oidc/login`, an "or" divider, and the local email/password form; friendly messages for the OIDC callback `?error=` codes (`no_email`, `email_unverified`, `oidc_conflict`, …).
- **`RegisterPage`** — email + display name + password (min 8); registration-closed and SSO-only handling; auto-login lands on `/gardens`.
- **`AppShell`** — auth-aware nav: user's display name + Sign out when logged in, Sign in otherwise; nav links only when authed.
- **`ui/`** — `TextField` (16px text to avoid iOS zoom, autocomplete + a11y), `Button` (+ shared `buttonClasses` for the OIDC anchor), `Alert`; `AuthCard`.
## Verification
- `npx tsc --noEmit` clean; `vite build` succeeds.
- **Real-browser E2E** against the embedded single binary (Playwright):
- register → auto-login → `/gardens`; full-page refresh stays signed in;
- Sign out → `/login`, session cleared;
- logged-out `/gardens/1` → `/login?redirect=%2Fgardens%2F1`, and logging in returns you to the destination;
- with OIDC configured, the login page shows the **"Sign in with Authentik"** button + local form (screenshot shared in-session).
## Out of scope
- Account settings / password change (post-v1); the Gardens list/create flow (#8). The pages here are the auth surface only.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Frontend for pansy auth, rendering whatever /auth/providers reports.
- lib/auth.ts: zod-validated User/Providers, a shared meQueryOptions
(a 401 resolves to null, not an error) reused by useMe() and the router
guard, useProviders(), and useLogin/useRegister/useLogout mutations that
prime/clear the me cache (logout also drops non-auth cached data).
- lib/queryClient.ts: one QueryClient shared by the React tree and the
router context so guard and components hit the same cache.
- router.tsx: createRootRouteWithContext with the queryClient; requireAuth
(redirects to /login?redirect=<dest>) on gardens/plants/editor, and
requireGuest on login/register (bounces authed users away). Login search
params (redirect, error) validated as optional; redirect targets are
restricted to same-origin paths.
- pages/LoginPage: OIDC button (label from providers) linking to
/auth/oidc/login, "or" divider, local email/password form, and friendly
messages for the OIDC callback ?error= codes. RegisterPage: email +
display name + password (min 8), registration-closed and SSO-only
handling; auto-login lands on /gardens.
- AppShell: auth-aware nav — shows the user's display name + Sign out when
logged in, Sign in otherwise; nav links only when authed.
- ui/: TextField (16px text to avoid iOS zoom, autocomplete + a11y),
Button (+ shared buttonClasses for the OIDC anchor), Alert; AuthCard.
Verified in a real browser against the embedded binary: register →
auto-login → /gardens; refresh stays in; Sign out → /login; a logged-out
/gardens/1 redirects to /login and returns after login; OIDC button renders
with the Authentik label when configured. tsc --noEmit clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
4/5 reviewers finished · updated 2026-07-18 21:59:13Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ⚠️ **maintainability** — could not complete
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ⏳ 4/5 lenses
- ✅ **security** — No material issues found
- 🔄 **correctness** — running
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 20 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 20 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡Auth nav flickers unauthenticated state during me query loading
error-handling · flagged by 1 model
web/src/components/layout/AppShell.tsx:18-21 — useMe()’s loading state is ignored (const user = me.data). On a hard refresh, data is undefined while the query is pending, so authenticated users see a brief flash of the “Sign in” link and the unauthenticated nav layout. Fix: Check me.isPending and render a neutral loading state (e.g., a skeleton or simply omit the auth-specific buttons until resolved).
🪰 Gadfly · advisory
🟡 **Auth nav flickers unauthenticated state during me query loading**
_error-handling · flagged by 1 model_
- **`web/src/components/layout/AppShell.tsx:18-21`** — `useMe()`’s loading state is ignored (`const user = me.data`). On a hard refresh, `data` is `undefined` while the query is pending, so authenticated users see a brief flash of the “Sign in” link and the unauthenticated nav layout. **Fix:** Check `me.isPending` and render a neutral loading state (e.g., a skeleton or simply omit the auth-specific buttons until resolved).
<sub>🪰 Gadfly · advisory</sub>
🔴Logout failure is unhandled, leaving stale auth state and no user feedback
correctness, error-handling · flagged by 5 models
web/src/components/layout/AppShell.tsx:23-26 — onLogout awaits logout.mutateAsync() without a try/catch. If the logout request fails (network error, server 5xx), the error becomes an unhandled promise rejection, the me cache is never cleared (the useLogoutonSuccess only runs on success), and the UI continues to show the user as signed in with zero feedback. Fix: Wrap logout.mutateAsync() in try/catch; on error, surface it in the UI (e.g., an inline alert) or at l…
🪰 Gadfly · advisory
🔴 **Logout failure is unhandled, leaving stale auth state and no user feedback**
_correctness, error-handling · flagged by 5 models_
- **`web/src/components/layout/AppShell.tsx:23-26`** — `onLogout` awaits `logout.mutateAsync()` without a `try/catch`. If the logout request fails (network error, server 5xx), the error becomes an unhandled promise rejection, the `me` cache is **never** cleared (the `useLogout` `onSuccess` only runs on success), and the UI continues to show the user as signed in with zero feedback. **Fix:** Wrap `logout.mutateAsync()` in `try/catch`; on error, surface it in the UI (e.g., an inline alert) or at l…
<sub>🪰 Gadfly · advisory</sub>
🟠hintId becomes literal 'undefined-hint' when id and name are both omitted
correctness, error-handling · flagged by 1 model
web/src/components/ui/TextField.tsx:15 — inputId is computed as id ?? name. Both are optional on InputHTMLAttributes, so a caller that omits both (easy to do in a shared UI primitive) yields inputId === undefined. The <label htmlFor> and <input id> are then omitted, breaking the programmatic label–input association, and if hint is present aria-describedby becomes "undefined-hint". Fix: fall back to React.useId() when neither id nor name is supplied.
🪰 Gadfly · advisory
🟠 **hintId becomes literal 'undefined-hint' when id and name are both omitted**
_correctness, error-handling · flagged by 1 model_
- **`web/src/components/ui/TextField.tsx:15`** — `inputId` is computed as `id ?? name`. Both are optional on `InputHTMLAttributes`, so a caller that omits both (easy to do in a shared UI primitive) yields `inputId === undefined`. The `<label htmlFor>` and `<input id>` are then omitted, breaking the programmatic label–input association, and if `hint` is present `aria-describedby` becomes `"undefined-hint"`. **Fix:** fall back to `React.useId()` when neither `id` nor `name` is supplied.
<sub>🪰 Gadfly · advisory</sub>
🔴meQueryOptions only treats 401 as unauthenticated; other failures (network, 5xx, zod parse) propagate uncaught into router guards with no error boundary
error-handling · flagged by 2 models
web/src/lib/auth.ts:34-37 — meQueryOptions.queryFn catches ApiError 401 → null, but a zod ZodError from a malformed /auth/me body (e.g., server returns 200 with unexpected shape) is rethrown. In requireAuth (router.tsx:26) this surfaces via ensureQueryData as a route beforeLoad error, and there is no errorComponent/ErrorBoundary defined anywhere in the route tree (grep confirmed none in the repo). Result: the user lands on TanStack's default raw-error render rather…
🪰 Gadfly · advisory
🔴 **meQueryOptions only treats 401 as unauthenticated; other failures (network, 5xx, zod parse) propagate uncaught into router guards with no error boundary**
_error-handling · flagged by 2 models_
- **`web/src/lib/auth.ts:34-37`** — `meQueryOptions.queryFn` catches `ApiError` 401 → `null`, but a zod `ZodError` from a malformed `/auth/me` body (e.g., server returns 200 with unexpected shape) is rethrown. In `requireAuth` (`router.tsx:26`) this surfaces via `ensureQueryData` as a route `beforeLoad` error, and there is no `errorComponent`/`ErrorBoundary` defined anywhere in the route tree (grep confirmed none in the repo). Result: the user lands on TanStack's default raw-error render rather…
<sub>🪰 Gadfly · advisory</sub>
🟠meQueryOptions staleTime of 60s allows stale auth data to pass route guards
correctness · flagged by 1 model
web/src/lib/auth.ts:39 — meQueryOptions sets staleTime: 60_000. With refetchOnWindowFocus: false (global default in queryClient.ts), a cached User object is treated as fresh for a full minute. If the session expires, or the user logs out in another tab, requireAuth in beforeLoad can still pass the guard and render protected routes using stale authenticated state. Fix: reduce staleTime to 0 for auth queries so ensureQueryData always verifies the session with the se…
🪰 Gadfly · advisory
🟠 **meQueryOptions staleTime of 60s allows stale auth data to pass route guards**
_correctness · flagged by 1 model_
- **`web/src/lib/auth.ts:39`** — `meQueryOptions` sets `staleTime: 60_000`. With `refetchOnWindowFocus: false` (global default in `queryClient.ts`), a cached `User` object is treated as fresh for a full minute. If the session expires, or the user logs out in another tab, `requireAuth` in `beforeLoad` can still pass the guard and render protected routes using stale authenticated state. **Fix:** reduce `staleTime` to `0` for auth queries so `ensureQueryData` always verifies the session with the se…
<sub>🪰 Gadfly · advisory</sub>
web/src/lib/auth.ts:89-95 — useLogout's cache cleanup (removeQueries) runs in onSuccess, which fires only on a successful logout. If the logout request fails (network/5xx), stale non-auth cache from the previous session remains, and since the cookie may still be valid the user stays logged in but with no cache cleanup. Consistent with "still logged in" semantics, but worth noting the asymmetry: the only cleanup path is gated on the success that, on failure, leaves the cache inconsi…
🪰 Gadfly · advisory
🟡 **`web/src/lib/auth.ts:89-95`**
_error-handling · flagged by 1 model_
- **`web/src/lib/auth.ts:89-95`** — `useLogout`'s cache cleanup (`removeQueries`) runs in `onSuccess`, which fires only on a successful logout. If the logout request fails (network/5xx), stale non-auth cache from the previous session remains, and since the cookie may still be valid the user stays logged in but with no cache cleanup. Consistent with "still logged in" semantics, but worth noting the asymmetry: the only cleanup path is gated on the success that, on failure, leaves the cache inconsi…
<sub>🪰 Gadfly · advisory</sub>
🟡errorMessage ignores the caller's fallback for any ApiError, contradicting its doc-comment and surprising the login/register callers
maintainability · flagged by 1 model
web/src/lib/auth.ts:109 — errorMessage silently ignores its fallback for ApiError. The signature reads as "use this fallback message", but the implementation returns err.message for anyApiError, including ones whose server message is generic/technical (e.g. Request failed (NNN) from messageFrom in api.ts). Callers in LoginPage (line 90) and RegisterPage (line 87) pass tailored fallbacks ('Could not sign in.', 'Could not create your account.') that never win for…
🪰 Gadfly · advisory
🟡 **errorMessage ignores the caller's fallback for any ApiError, contradicting its doc-comment and surprising the login/register callers**
_maintainability · flagged by 1 model_
- **`web/src/lib/auth.ts:109` — `errorMessage` silently ignores its `fallback` for `ApiError`.** The signature reads as "use this fallback message", but the implementation returns `err.message` for *any* `ApiError`, including ones whose server message is generic/technical (e.g. `Request failed (NNN)` from `messageFrom` in `api.ts`). Callers in `LoginPage` (line 90) and `RegisterPage` (line 87) pass tailored fallbacks (`'Could not sign in.'`, `'Could not create your account.'`) that never win for…
<sub>🪰 Gadfly · advisory</sub>
🟡Hardcoded API base path duplicates lib/api.ts constant
maintainability · flagged by 1 model
web/src/pages/LoginPage.tsx:11 — OIDC_LOGIN_URL hardcodes /api/v1, duplicating the base path already defined in lib/api.ts. If the API base ever changes, this link will break silently. Export BASE from lib/api.ts (or a derived helper) and construct the OIDC URL from it so the prefix lives in one place.
🪰 Gadfly · advisory
🟡 **Hardcoded API base path duplicates lib/api.ts constant**
_maintainability · flagged by 1 model_
- **`web/src/pages/LoginPage.tsx:11`** — `OIDC_LOGIN_URL` hardcodes `/api/v1`, duplicating the base path already defined in `lib/api.ts`. If the API base ever changes, this link will break silently. Export `BASE` from `lib/api.ts` (or a derived helper) and construct the OIDC URL from it so the prefix lives in one place.
<sub>🪰 Gadfly · advisory</sub>
🟠Duplicate redirect sanitization logic with router.tsx
maintainability · flagged by 4 models
web/src/pages/LoginPage.tsx:24 and web/src/router.tsx:40 — safeRedirect and safeInternalPath are identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g. lib/url.ts or lib/auth.ts) and import it in both files.
🪰 Gadfly · advisory
🟠 **Duplicate redirect sanitization logic with router.tsx**
_maintainability · flagged by 4 models_
- **`web/src/pages/LoginPage.tsx:24`** and **`web/src/router.tsx:40`** — `safeRedirect` and `safeInternalPath` are identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g. `lib/url.ts` or `lib/auth.ts`) and import it in both files.
<sub>🪰 Gadfly · advisory</sub>
web/src/pages/LoginPage.tsx:37 — callbackErrors[search.error] ?? callbackErrors.oidc falls back to the generic oidc ("Single sign-on did not complete") message for any unknown ?error= code. validateSearch (router.tsx:62-67) accepts any string for error, so an attacker-supplied/typo'd code yields a misleading SSO message even on a non-OIDC flow. Minor UX/edge: better to show a generic fallback or restrict allowed codes. Verified by reading both files.
🪰 Gadfly · advisory
🟡 **`web/src/pages/LoginPage.tsx:37`**
_error-handling · flagged by 1 model_
- **`web/src/pages/LoginPage.tsx:37`** — `callbackErrors[search.error] ?? callbackErrors.oidc` falls back to the generic `oidc` ("Single sign-on did not complete") message for any unknown `?error=` code. `validateSearch` (`router.tsx:62-67`) accepts any string for `error`, so an attacker-supplied/typo'd code yields a misleading SSO message even on a non-OIDC flow. Minor UX/edge: better to show a generic fallback or restrict allowed codes. **Verified** by reading both files.
<sub>🪰 Gadfly · advisory</sub>
web/src/pages/LoginPage.tsx:42-43 & RegisterPage.tsx:22-23 — await navigate({ to: dest }) after a successful login/register is inside the same try block as the mutation. If navigation throws (e.g., redirect target's own beforeLoad redirects again, or an invalid path), the rejection is caught by the form's catch {} and silently swallowed alongside any mutation error display logic; the navigation failure is lost with no UI feedback. Minor, but it's the one path here where a navig…
🪰 Gadfly · advisory
🟡 **`web/src/pages/LoginPage.tsx:42-43` & `RegisterPage.tsx:22-23`**
_error-handling · flagged by 1 model_
- **`web/src/pages/LoginPage.tsx:42-43` & `RegisterPage.tsx:22-23`** — `await navigate({ to: dest })` after a successful login/register is inside the same `try` block as the mutation. If navigation throws (e.g., redirect target's own `beforeLoad` redirects again, or an invalid path), the rejection is caught by the form's `catch {}` and silently swallowed alongside any mutation error display logic; the navigation failure is lost with no UI feedback. Minor, but it's the one path here where a navig…
<sub>🪰 Gadfly · advisory</sub>
🟠OIDC login drops the redirect target; user lands on /gardens instead of the remembered destination
correctness · flagged by 1 model
OIDC login drops the redirect target — web/src/pages/LoginPage.tsx:60-64. The local form threads the remembered destination (dest = safeRedirect(search.redirect) → navigate({ to: dest })), but the OIDC anchor is a plain <a href={OIDC_LOGIN_URL}> with no redirect query param forwarded, and the backend oidcLogin handler reads no redirect param (internal/api/oidc.go:123-150) while oidcCallback's success path hardcodes c.Redirect(http.StatusFound, "/gardens") (`internal/api…
🪰 Gadfly · advisory
🟠 **OIDC login drops the redirect target; user lands on /gardens instead of the remembered destination**
_correctness · flagged by 1 model_
- **OIDC login drops the `redirect` target** — `web/src/pages/LoginPage.tsx:60-64`. The local form threads the remembered destination (`dest = safeRedirect(search.redirect)` → `navigate({ to: dest })`), but the OIDC anchor is a plain `<a href={OIDC_LOGIN_URL}>` with no `redirect` query param forwarded, and the backend `oidcLogin` handler reads no redirect param (`internal/api/oidc.go:123-150`) while `oidcCallback`'s success path hardcodes `c.Redirect(http.StatusFound, "/gardens")` (`internal/api…
<sub>🪰 Gadfly · advisory</sub>
🟡noValidate disables required/minLength guards; empty/invalid input is sent to the server with no client-side check
error-handling · flagged by 2 models
web/src/pages/LoginPage.tsx:69 and web/src/pages/RegisterPage.tsx:55 (register form) — noValidate silently disables the declared required/minLength guards. Both forms set noValidate but perform no client-side validation before mutateAsync(...), so empty email/displayName and sub-8-char passwords are submitted straight to the network. The server does reject them (shown via errorMessage), so it isn't a crash, but the required/minLength={8} attributes are decorative. Eithe…
🪰 Gadfly · advisory
🟡 **noValidate disables required/minLength guards; empty/invalid input is sent to the server with no client-side check**
_error-handling · flagged by 2 models_
- **`web/src/pages/LoginPage.tsx:69` and `web/src/pages/RegisterPage.tsx:55` (register form) — `noValidate` silently disables the declared `required`/`minLength` guards.** Both forms set `noValidate` but perform no client-side validation before `mutateAsync(...)`, so empty email/displayName and sub-8-char passwords are submitted straight to the network. The server does reject them (shown via `errorMessage`), so it isn't a crash, but the `required`/`minLength={8}` attributes are decorative. Eithe…
<sub>🪰 Gadfly · advisory</sub>
⚪Divider is a generic UI primitive but lives unexported in a page file, breaking the components/ui/ pattern used by the rest of the PR*
maintainability · flagged by 1 model
web/src/pages/LoginPage.tsx:114 — Divider is a generic UI primitive parked in a page file. It's a stateless, reusable "or" divider with no login-specific logic, yet it lives inside LoginPage.tsx and is not exported. The rest of the PR deliberately promotes shared visuals to components/ui/* (Alert, Button, TextField) and components/auth/* (AuthCard). Divider breaks that pattern for no reason. Suggest moving it to components/ui/Divider.tsx (or folding into AuthCard if…
🪰 Gadfly · advisory
⚪ **Divider is a generic UI primitive but lives unexported in a page file, breaking the components/ui/* pattern used by the rest of the PR**
_maintainability · flagged by 1 model_
- **`web/src/pages/LoginPage.tsx:114` — `Divider` is a generic UI primitive parked in a page file.** It's a stateless, reusable "or" divider with no login-specific logic, yet it lives inside `LoginPage.tsx` and is not exported. The rest of the PR deliberately promotes shared visuals to `components/ui/*` (`Alert`, `Button`, `TextField`) and `components/auth/*` (`AuthCard`). `Divider` breaks that pattern for no reason. Suggest moving it to `components/ui/Divider.tsx` (or folding into `AuthCard` if…
<sub>🪰 Gadfly · advisory</sub>
🔴RegisterPage renders form while providers query is pending or errored
correctness, error-handling, maintainability · flagged by 4 models
web/src/pages/RegisterPage.tsx:30 — The page immediately falls through to render the full registration form when providers is still loading (isPending) or has errored (isError). Unlike LoginPage, there is no loading spinner or error alert for the providers query, so the form is interactive before the app even knows whether local registration is enabled. Fix: Add early returns for providers.isPending and providers.isError, mirroring the guard pattern in `LoginPage.tsx:57-5…
🪰 Gadfly · advisory
🔴 **RegisterPage renders form while providers query is pending or errored**
_correctness, error-handling, maintainability · flagged by 4 models_
- **`web/src/pages/RegisterPage.tsx:30`** — The page immediately falls through to render the full registration form when `providers` is still loading (`isPending`) or has errored (`isError`). Unlike `LoginPage`, there is no loading spinner or error alert for the providers query, so the form is interactive before the app even knows whether local registration is enabled. **Fix:** Add early returns for `providers.isPending` and `providers.isError`, mirroring the guard pattern in `LoginPage.tsx:57-5…
<sub>🪰 Gadfly · advisory</sub>
🟡Form with noValidate lacks client-side validation, allowing empty submissions
error-handling · flagged by 1 model
web/src/pages/LoginPage.tsx:69 and web/src/pages/RegisterPage.tsx:55 — Both <form> elements set noValidate, yet there is no compensating client-side validation. Empty strings or a password shorter than 8 chars are submitted to the server unnecessarily. Fix: Add minimal validation (e.g., email.trim() and password.length >= MIN_PASSWORD) before calling mutateAsync, or remove noValidate if native browser validation is acceptable.
🪰 Gadfly · advisory
🟡 **Form with noValidate lacks client-side validation, allowing empty submissions**
_error-handling · flagged by 1 model_
- **`web/src/pages/LoginPage.tsx:69` and `web/src/pages/RegisterPage.tsx:55`** — Both `<form>` elements set `noValidate`, yet there is no compensating client-side validation. Empty strings or a password shorter than 8 chars are submitted to the server unnecessarily. **Fix:** Add minimal validation (e.g., `email.trim()` and `password.length >= MIN_PASSWORD`) before calling `mutateAsync`, or remove `noValidate` if native browser validation is acceptable.
<sub>🪰 Gadfly · advisory</sub>
🔴Redirect-after-login broken because location.href (full URL) is passed but safeRedirect expects a path
error-handling · flagged by 2 models
web/src/router.tsx:25-30 / :33-38 — a non-401 error in ensureQueryData throws inside beforeLoad with no errorComponent configured.meQueryOptions.queryFn rethrows anything that isn't a 401 (e.g. a network failure, status: 0). The query default retry (3) softens transient blips, but a persistent outage on /gardens makes requireAuth throw a real (non-redirect) error in beforeLoad. rootRoute is created with only { component: AppShell } — no errorComponent/`notFound…
🪰 Gadfly · advisory
🔴 **Redirect-after-login broken because location.href (full URL) is passed but safeRedirect expects a path**
_error-handling · flagged by 2 models_
- **`web/src/router.tsx:25-30` / `:33-38` — a non-401 error in `ensureQueryData` throws inside `beforeLoad` with no `errorComponent` configured.** `meQueryOptions.queryFn` rethrows anything that isn't a 401 (e.g. a network failure, `status: 0`). The query default `retry` (3) softens transient blips, but a persistent outage on `/gardens` makes `requireAuth` throw a real (non-redirect) error in `beforeLoad`. `rootRoute` is created with only `{ component: AppShell }` — no `errorComponent`/`notFound…
<sub>🪰 Gadfly · advisory</sub>
🟠Duplicate redirect sanitization logic with LoginPage.tsx
maintainability · flagged by 2 models
web/src/pages/LoginPage.tsx:24 and web/src/router.tsx:40 — safeRedirect and safeInternalPath are identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g. lib/url.ts or lib/auth.ts) and import it in both files.
🪰 Gadfly · advisory
🟠 **Duplicate redirect sanitization logic with LoginPage.tsx**
_maintainability · flagged by 2 models_
- **`web/src/pages/LoginPage.tsx:24`** and **`web/src/router.tsx:40`** — `safeRedirect` and `safeInternalPath` are identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g. `lib/url.ts` or `lib/auth.ts`) and import it in both files.
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 23 findings (7 with multi-model agreement)
Finding
Where
Models
Lens
🔴
Logout failure is unhandled, leaving stale auth state and no user feedback
web/src/components/layout/AppShell.tsx:23
5/5
correctness, error-handling
🔴
RegisterPage renders form while providers query is pending or errored
web/src/pages/RegisterPage.tsx:30
4/5
correctness, error-handling, maintainability
🟠
Duplicate redirect sanitization logic with router.tsx
web/src/pages/LoginPage.tsx:24
4/5
maintainability
🔴
meQueryOptions only treats 401 as unauthenticated; other failures (network, 5xx, zod parse) propagate uncaught into router guards with no error boundary
web/src/lib/auth.ts:34
2/5
error-handling
🔴
Redirect-after-login broken because location.href (full URL) is passed but safeRedirect expects a path
web/src/router.tsx:25
2/5
error-handling
🟠
Duplicate redirect sanitization logic with LoginPage.tsx
web/src/router.tsx:40
2/5
maintainability
🟡
noValidate disables required/minLength guards; empty/invalid input is sent to the server with no client-side check
web/src/pages/LoginPage.tsx:69
2/5
error-handling
16 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
hintId becomes literal 'undefined-hint' when id and name are both omitted
web/src/components/ui/TextField.tsx:14
opencode/kimi-k2.6:cloud
correctness, error-handling
🟠
meQueryOptions staleTime of 60s allows stale auth data to pass route guards
web/src/lib/auth.ts:39
opencode/kimi-k2.6:cloud
correctness
🟠
OIDC login drops the redirect target; user lands on /gardens instead of the remembered destination
web/src/pages/LoginPage.tsx:60
opencode/glm-5.2:cloud
correctness
🟠
requireAuth passes full href to path-only redirect sanitizer
No credential logging or client-side storage of secrets. That closes out the security lens investigation.
VERDICT: No material issues found
Reviewed the auth data layer (lib/auth.ts), the fetch wrapper (lib/api.ts), the router guards (router.tsx), and the login/register pages through a security lens. Key things I verified rather than assumed:
Open-redirect guard is real and correctly restrictive.safeRedirect/safeInternalPath (web/src/pages/LoginPage.tsx:26, web/src/router.tsx) reject anything not starting with /, and explicitly reject // (protocol-relative bypass). Verified the redirect destination only ever flows into TanStack Router's navigate({to})/redirect({to}) — never into window.location or a raw <a href> — so even a scheme-like string can't cause an actual cross-origin navigation.
CSRF is already handled server-side, independent of this PR: internal/api/auth.go:131 (csrfGuard, Origin-header check) plus SameSite=Lax + HttpOnly + conditionally-Secure cookies (internal/api/auth.go:186-205), and the OIDC state cookie carries its own CSRF-state check (internal/api/oidc.go:167). The new fetch wrapper (web/src/lib/api.ts:96) uses credentials: 'same-origin', so it won't leak the session cookie cross-origin.
No XSS surface. All server-derived/user-derived strings (user.displayName, providers.data.oidcLabel, errorMessage(err), the ?error= callback text) are rendered as JSX children, never via dangerouslySetInnerHTML. The ?error= code from the OIDC callback is used only as a lookup key into a fixed callbackErrors map (LoginPage.tsx:12) — the raw query value is never reflected into the DOM, confirmed the server only ever sets one of a fixed enum of codes (internal/api/oidc.go:115-118).
401-as-null pattern is sound.meQueryOptions (lib/auth.ts) only swallows 401 into null; any other error (network failure, 5xx) propagates and ensureQueryData in requireAuth/requireGuest throws rather than treating the failure as "authenticated" — i.e., it fails closed, not open.
Server independently re-validates what the client only soft-checks. Password min-length is enforced server-side too (internal/api/auth.go:29, binding:"required,min=8,max=1024"), so the client's minLength={8} isn't a bypassable control.
Logout hygiene.useLogout clears the me cache and purges all non-auth query cache (lib/auth.ts), preventing a shared-device scenario where the next signed-in user could see stale cached data from the previous session.
I considered whether the client-side prefix check for redirects could be defeated by control-character tricks (e.g. a tab between slashes that some URL parsers normalize away), which is a known weakness in naive startsWith('/') guards — but since the destination is only ever consumed by the router's internal navigate/redirect (not assigned to location.href or an anchor), I could not establish an actual exploitation path, and didn't have node_modules available to inspect TanStack Router's exact to-resolution internals to rule it in or out. Flagging as considered-but-not-confirmed rather than reporting it as a finding.
🎯 Correctness — No material issues found
Verified Review
VERDICT: No material issues found
The one flagged finding does not survive verification. requireGuest's throw redirect({ to: redirectTo }) at web/src/router.tsx:36 omits a search field — and TanStack Router does not auto-forward the previous location's search params on a redirect/navigate unless explicitly passed (this codebase's own pattern confirms it: requireAuth at router.tsx:28 explicitly sets search: { redirect: href } when it wants to forward a param, while requireGuest and the / redirect deliberately don't). So for the crafted case /login?redirect=%2Flogin: requireGuest computes redirectTo = '/login' and throws redirect({ to: '/login' }) with no search — the resulting navigation lands on plain /login with no redirect param. beforeLoad reruns, search.redirect is now undefined, safeInternalPath(undefined) falls back to /gardens, and since the user is still authenticated, requireGuest redirects them to /gardens. That's a single harmless extra bounce through /login, not the infinite loop / browser-hang the draft describes — the router self-heals on the second pass. The draft's core impact claim is incorrect, so the finding is dropped.
Everything else in the draft (schema/tag parity, REGISTRATION_CLOSED code match, OIDC callback's hardcoded /gardens redirect, meQueryOptions/useLogout cache wiring, and the client-side password minimum being a UX-only stricter-than-server constraint) was independently spot-checked and holds up.
🧹 Code cleanliness & maintainability — Minor issues
Confirmed: both router.tsx:40 (safeInternalPath) and LoginPage.tsx:24 (safeRedirect) have byte-identical bodies and same-origin logic, just different names, and no shared import between them. The finding holds.
VERDICT: Minor issues
web/src/pages/LoginPage.tsx:24 (safeRedirect) duplicates web/src/router.tsx:40 (safeInternalPath) — identical body (dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens') implementing the same same-origin redirect check under two different names, with no shared import. router.tsx's copy guards beforeLoad (via requireGuest), while LoginPage.tsx's copy guards the post-login navigate destination. A future change to the redirect-safety rule (tightening or loosening the same-origin check) would need to be applied in both places, and there's nothing forcing that — a fix to one silently leaves the other stale/inconsistent. Should be a single shared helper (e.g. exported from lib/auth.ts, which already owns auth/redirect concerns and is imported by both files).
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed this PR strictly through the performance lens (auth data layer, query client config, router guards, and the new auth pages/components).
What I checked:
web/src/lib/auth.ts — meQueryOptions (staleTime 60s) and providersQueryOptions (staleTime Infinity) are shared singletons keyed by queryOptions(), so useMe(), the router guards, and the mutations' onSuccess all hit the same cache entry — no duplicate fetching introduced.
web/src/router.tsx — requireAuth/requireGuest call ensureQueryData(meQueryOptions) in beforeLoad; because of the 60s staleTime this is a cache hit on repeat navigations within the window, not a fetch-per-route-change.
useLogout's qc.removeQueries({ predicate: ... }) (web/src/lib/auth.ts) is O(number of cached queries), but it only runs once per logout, not a hot path — fine at this scale.
web/src/lib/queryClient.ts — single QueryClient instance shared between the router context and React tree (confirmed via main.tsx now importing it instead of constructing a second one), so no duplicate client/cache.
web/src/lib/api.ts — default fetch wrapper, no added retry/backoff changes; 401 on /auth/me is caught inside meQueryOptions and resolved to null rather than thrown, so it won't trigger react-query's default retry loop.
Components (TextField, Button, Alert, AuthCard) are simple, no loops, no unbounded lists, no unmemoized expensive computation.
Nothing here shows N+1 requests, unbounded growth, blocking hot-path calls, or quadratic behavior. This is a small, standard React Query-based auth UI; scope doesn't offer much surface for performance regressions, and I didn't find any.
🧯 Error handling & edge cases — Minor issues
All three findings check out exactly against the current source. The line references, code paths, and reasoning all match: meQueryOptions (auth.ts:29-38) only special-cases 401 and rethrows everything else (including network failures per api.ts:103-106 and zod parse errors), which then flows uncaught through ensureQueryData in requireAuth/requireGuest (router.tsx:26,34) with no error boundary anywhere in web/src (grep confirmed empty). AppShell.tsx:23-26's onLogout has no try/catch, unlike Login/RegisterPage's submit handlers which do. RegisterPage.tsx (confirmed no isPending/isError checks anywhere in the file) falls through to the local form on a providers fetch failure, unlike LoginPage.tsx:57-58 which explicitly handles both states.
Review — 🧯 Error handling & edge cases
VERDICT: Minor issues
web/src/lib/auth.ts:29-38 + web/src/router.tsx:25-38 — meQueryOptions only special-cases HTTP 401 as "logged out"; every other failure mode is rethrown unmodified: a network outage (api.ts:103-106 throws ApiError(..., status: 0, ...) for fetch failures), a 5xx from the server, or a response that fails userSchema.parse (a ZodError, not an ApiError, so the err instanceof ApiError guard doesn't catch it) all propagate straight out of the query function. That rejection flows uncaught through context.queryClient.ensureQueryData(meQueryOptions) in both requireAuth and requireGuest (router.tsx:26,34), which have no try/catch around it. There is no errorComponent/ErrorBoundary anywhere in the app (confirmed via repo-wide grep), so any transient failure of /auth/me breaks navigation into every guarded route (/gardens, /gardens/$gardenId, /plants, and the guest-guarded /login//register) instead of degrading gracefully.
web/src/components/layout/AppShell.tsx:23-26 — onLogout awaits logout.mutateAsync() with no try/catch, unlike LoginPage.tsx and RegisterPage.tsx, which both wrap their mutateAsync calls. If /auth/logout fails (network drop or 5xx — same ApiError path as above), the rejection becomes an unhandled promise rejection inside the click handler: navigate never runs, logout.isPending resets to false so the button silently re-enables, and there is no Alert/message shown anywhere — the user is left looking "signed in" with no indication the sign-out failed.
web/src/pages/RegisterPage.tsx — unlike LoginPage.tsx:57-58 (which renders providers.isPending/providers.isError states), RegisterPage never checks providers.isPending or providers.isError. If the /auth/providers fetch fails, the providers.isSuccess && !providers.data.local guard (line 30) is simply false, so the component falls through to rendering the full local-registration form even on servers configured for SSO-only — an inconsistent, unhandled edge case compared to how LoginPage handles the same fetch failure.
Through the security lens, I traced the tainted, user-controlled inputs to their sinks:
redirect search param — used in two places: safeRedirect(search.redirect) in LoginPage and safeInternalPath(search.redirect) in router.tsx:40-42. Both sinks call TanStack Router's navigate()/redirect() (SPA-internal navigation), not window.location/<a href>. The startsWith('/') && !startsWith('//') guard blocks protocol-relative (//evil.com) and absolute-URL (http://...) values. The one gap — a leading backslash (/\evil.com) passes the check — is not exploitable here because the router resolves to against its own route tree and renders notFound rather than performing a cross-origin browser navigation. So no open-redirect vulnerability. (Defense-in-depth: a new URL(dest, location.origin).origin === location.origin check would be more robust, but it's not required for safety.)
error search param — never reflected raw; it's only a lookup key into the fixed callbackErrors map (LoginPage.tsx:11-19), and the rendered string is always a predefined constant. No reflected XSS.
Server-supplied strings (err.message via errorMessage, providers.oidcLabel, user.displayName) are rendered as React text children in Alert/spans, so React escapes them. No dangerouslySetInnerHTML anywhere in the diff.
OIDC_LOGIN_URL is a hardcoded same-origin path (/api/v1/auth/oidc/login), not attacker-influenced.
location.href passed to requireAuth is TanStack Router's location.href (pathname+search+hash, always same-origin-relative), not the browser's full URL.
Verified by reading web/src/lib/api.ts (ApiError contract, credentials: 'same-origin', no raw HTML handling) and web/src/router.tsx (guard logic + search validation).
🎯 Correctness — Minor issues
VERDICT: Minor issues
web/src/pages/RegisterPage.tsx:30 — The "local registration disabled" branch only fires once providers.isSuccess. While the providers query is still loading (isPending), the condition providers.isSuccess && !providers.data.local is false, so the page falls through and renders the full registration form on a server where local auth is actually disabled. A user on an SSO-only server can fill out and submit the form before the providers response arrives, only to be rejected by the server. LoginPage handles this symmetrically: it gates the local form behind const local = providers.data?.local ?? false (which defaults to false during load) and renders a providers.isPending && <p>Loading…</p> state, so nothing actionable appears until the response arrives. RegisterPage has no such gating and shows the form by default. Suggested fix: render a "Loading…" state (or nothing) until providers resolves, mirroring LoginPage's providers.isPending handling, so the form never appears on a local-disabled server.
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: all chain targets failed
ollama-cloud/glm-5.2☁️ model returned an empty response
⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed the change through the performance lens, verifying the query/cache strategy against the actual code rather than the diff alone.
meQueryOptions + ensureQueryData guard sharing one cache (lib/auth.ts:29, router.tsx:26,34):useMe() in AppShell and the requireAuth/requireGuest guards read the same ['auth','me'] entry, so there is no duplicate /auth/me fetch between the guard resolving and the component rendering. Verified the cache key and queryOptions shape match across both call sites.
Login/logout cache priming avoids refetches (lib/auth.ts:73,82,91):useLogin/useRegister prime me via setQueryData, so the subsequent navigate({ to: dest }) → requireAuth → ensureQueryData finds fresh data and does not re-fetch. useLogout sets me to null (fresh), so the following navigate({ to: '/login' }) → requireGuest → ensureQueryData resolves to null without another /auth/me round-trip. I confirmed ensureQueryData treats the null set by setQueryData as present-and-fresh, so this is a genuine save, not a hidden double-fetch.
removeQueries on logout (lib/auth.ts:94): Iterates all cached queries with a predicate, but this runs once per logout (a cold, user-initiated path), not on any hot loop. Not material.
providersQueryOptionsstaleTime: Infinity (lib/auth.ts:49): Fetches providers once per session; the logout cleanup intentionally preserves auth-keyed queries, so no refetch storm on the next sign-in. Fine.
ensureQueryData on every protected navigation (router.tsx:26): Bounded by the 60s staleTime, with react-query deduping concurrent calls against the same key, so there is no N+1 or per-navigation refetch regression. defaultPreload: 'intent' can cause a /auth/me fetch on hover only once the entry is stale, which is the intended preload behavior.
No efficiency regressions, unbounded growth, hot-loop work, or duplicate-fetch issues found in this lens.
🧯 Error handling & edge cases — Minor issues
All three findings verified against the actual source.
Finding 1 (AppShell logout): Confirmed. onLogout (lines 23-26) awaits logout.mutateAsync() then navigate with no try/catch; the header renders only "Sign out" / "Signing out…", no logout.isError/logout.error UI. Login/Register pages show Alert on error; logout does not. The useLogoutonSuccess clears the me cache, so on rejection me stays non-null and the navigate is skipped — confirmed in auth.ts.
Finding 2 (noValidate): Confirmed for both forms. LoginPage.tsx:69 has noValidate with required fields and no explicit guard; RegisterPage.tsx:55 has noValidate with required/minLength and no explicit guard. The mutation onSubmit calls mutateAsync directly with raw state.
Finding 3 (router error): Confirmed. rootRoute (router.tsx:21) is created with only { component: AppShell }; requireAuth/requireGuest throw the raw error (non-redirect) from ensureQueryData when meQueryOptions.queryFn rethrows a non-401. queryClient.ts sets no retry override, so the framework default applies.
Corrected review
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:23-26 — logout failures are swallowed with no user feedback.onLogout does await logout.mutateAsync() then await navigate({ to: '/login' }) with no try/catch. If the /auth/logout request fails (e.g. transient network failure → ApiError status 0, or the server 5xxs), mutateAsync rejects: the navigate never runs, the rejection becomes an unhandled promise rejection, and onSuccess never fires so the me cache stays non-null — the user appears still logged in. Unlike login/register (which surface login.error/register.error via an Alert), logout has no UI for logout.isError; the button just flips back to "Sign out" silently. Suggest wrapping in try/catch and showing logout.error (e.g. an Alert in the header or a toast), and only navigating on success.
web/src/pages/LoginPage.tsx:69 and web/src/pages/RegisterPage.tsx:55 (register form) — noValidate silently disables the declared required/minLength guards. Both forms set noValidate but perform no client-side validation before mutateAsync(...), so empty email/displayName and sub-8-char passwords are submitted straight to the network. The server does reject them (shown via errorMessage), so it isn't a crash, but the required/minLength={8} attributes are decorative. Either drop noValidate (let the browser enforce them) or add an explicit guard before the mutation to avoid pointless failed requests and to give immediate field-level feedback.
web/src/router.tsx:25-30 / :33-38 — a non-401 error in ensureQueryData throws inside beforeLoad with no errorComponent configured.meQueryOptions.queryFn rethrows anything that isn't a 401 (e.g. a network failure, status: 0). The query default retry (3) softens transient blips, but a persistent outage on /gardens makes requireAuth throw a real (non-redirect) error in beforeLoad. rootRoute is created with only { component: AppShell } — no errorComponent/notFoundComponent — so the user gets TanStack's default raw error screen instead of being sent to login or offered a retry. Consider adding a root errorComponent (with a "try again" affordance) or treating a network error in the guard distinctly from "unauthenticated." (Verified rootRoute at router.tsx:21 and the queryClient defaults at web/src/lib/queryClient.ts:7 — no retry override, so the framework default retry applies.)
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
I examined the auth surface carefully for open redirects, XSS via URL parameters, injection paths, and insecure defaults. The code holds up:
router.tsx:40 / LoginPage.tsx:24 — safeInternalPath and safeRedirect both restrict redirect to same-origin absolute paths (startsWith('/') && !startsWith('//')). Verified callers: requireAuth (router guard) and LoginPage post-login navigation. No open redirect.
LoginPage.tsx:37 — The ?error= OIDC callback parameter is mapped through a whitelist (callbackErrors) before rendering. Arbitrary user-supplied values fall back to a safe default string. No XSS via error codes.
lib/auth.ts — meQueryOptions swallows 401 as null, avoiding query-error surfacing. useLogout clears the me cache and drops non-auth queries so stale data doesn't leak across sessions.
lib/api.ts:96 — Fetch uses credentials: 'same-origin', appropriate for the embedded-single-binary architecture.
Nothing in this diff crosses into a genuine security defect from the frontend side.
🎯 Correctness — No material issues found
No material issues found
I scrutinized the auth layer, routing guards, form validation, cache priming/clearing, redirect handling, and OIDC callback error mapping against the backend contract. The implementation is semantically correct:
meQueryOptions correctly treats 401 as null (unauthenticated state), shared between useMe and the router guard, with ensureQueryData keeping them in sync.
useLogout atomically nulls the me cache and scrubs non-auth queries, preventing cross-user data leakage.
useLogin/useRegister prime the me cache from the mutation response, avoiding a redundant fetch.
callbackErrors maps exactly to the backend OIDC error codes (oidc_unavailable, state, no_email, email_unverified, oidc_conflict, oidc).
The backend LOCAL_AUTH_DISABLED / REGISTRATION_CLOSED codes are consumed appropriately; the frontend also gates the register form on the providers query so the common case is handled before submission.
safeRedirect/safeInternalPath correctly restrict redirects to path-absolute URLs; tanstack router’s navigate only performs in-app routing, so the // check is sufficient for the current architecture.
TextField generates proper id/htmlFor/aria-describedby linkage in all actual call-sites (every usage supplies name).
Nothing in the diff violates domain correctness or introduces a logic bug.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
safeRedirect duplicated in two files — LoginPage.tsx:24 defines safeRedirect and router.tsx:40 defines an almost identical safeInternalPath. Keeping two copies of the same origin-check in sync is brittle. Fix: move one shared helper to lib/auth.ts (or lib/api.ts) and import it in both places.
Generic API helpers live in the auth module — errorMessage and apiErrorCode (lib/auth.ts:99-112) are general-purpose utilities that depend only on ApiError, not on auth concepts. Leaving them in lib/auth.ts couples unrelated modules to auth when they need error formatting. Fix: move them next to ApiError in lib/api.ts.
requireAuth passes a full href to a path-only sanitizer — router.tsx:82,89,96 pass location.href (a full URL including origin) into requireAuth, which stores it in search.redirect. Later, LoginPage.tsx:36 runs safeRedirect(search.redirect), which expects a path and checks dest.startsWith('/'). A full https://… href fails that check and silently falls back to /gardens, so the “remember where they were” feature is broken. The mismatch between what the helper claims to handle (paths) and what the caller supplies (hrefs) is confusing. Fix: pass location.pathname + location.search (or a dedicated path field) instead of location.href, or rename the helper and make it handle both.
⚡ Performance — No material issues found
No material issues found
The auth layer is well-designed from a performance standpoint. Shared queryOptions (meQueryOptions, providersQueryOptions) ensure a single cache entry is read by both route guards (ensureQueryData in beforeLoad) and components (useMe, useProviders). staleTime is set sensibly (60_000 for the user query, Infinity for providers), so protected-route navigation and page switches won't trigger redundant fetches within a session. Logout’s removeQueries predicate is bounded and only runs on an infrequent user action. No hot loops, N+1 patterns, unbounded growth, or blocking work on hot paths were introduced.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/components/layout/AppShell.tsx:23-26 — onLogout awaits logout.mutateAsync() without a try/catch. If the logout request fails (network error, server 5xx), the error becomes an unhandled promise rejection, the me cache is never cleared (the useLogoutonSuccess only runs on success), and the UI continues to show the user as signed in with zero feedback. Fix: Wrap logout.mutateAsync() in try/catch; on error, surface it in the UI (e.g., an inline alert) or at least ensure the cache is invalidated so the client doesn’t pretend the session is still valid.
web/src/pages/RegisterPage.tsx:30 — The page immediately falls through to render the full registration form when providers is still loading (isPending) or has errored (isError). Unlike LoginPage, there is no loading spinner or error alert for the providers query, so the form is interactive before the app even knows whether local registration is enabled. Fix: Add early returns for providers.isPending and providers.isError, mirroring the guard pattern in LoginPage.tsx:57-58.
Minor issues
web/src/components/layout/AppShell.tsx:18-21 — useMe()’s loading state is ignored (const user = me.data). On a hard refresh, data is undefined while the query is pending, so authenticated users see a brief flash of the “Sign in” link and the unauthenticated nav layout. Fix: Check me.isPending and render a neutral loading state (e.g., a skeleton or simply omit the auth-specific buttons until resolved).
web/src/pages/LoginPage.tsx:69 and web/src/pages/RegisterPage.tsx:55 — Both <form> elements set noValidate, yet there is no compensating client-side validation. Empty strings or a password shorter than 8 chars are submitted to the server unnecessarily. Fix: Add minimal validation (e.g., email.trim() and password.length >= MIN_PASSWORD) before calling mutateAsync, or remove noValidate if native browser validation is acceptable.
I reviewed the change through the security lens, verifying against the checked-out code (web/src/router.tsx, web/src/pages/LoginPage.tsx, web/src/lib/auth.ts, web/src/lib/api.ts).
Open-redirect protection (verified).requireAuth puts location.href (TanStack Router's in-app path/search/hash, not a full URL) into search.redirect, and LoginPage.safeRedirect (LoginPage.tsx:24-26) plus safeInternalPath (router.tsx:40-42) both reject anything not starting with / and reject // (protocol-relative). Known bypasses like /\evil.com are not exploitable here because navigate({ to }) performs client-side routing only — TanStack won't issue a document-level navigation to an external URL — so a slipped-through value just produces a broken in-app route, not an open redirect. I traced the full path from requireAuth → search.redirect → safeRedirect → navigate.
OIDC callback ?error= codes.search.error is only used as a lookup key into the static callbackErrors map (LoginPage.tsx:14-21, 37); it is never rendered directly, so no reflected XSS. Unknown codes fall back to callbackErrors.oidc. Verified at LoginPage.tsx:37.
OIDC button.<a href="/api/v1/auth/oidc/login"> (LoginPage.tsx:11, 60-64) is a same-origin navigation with no target="_blank", so no rel="noopener" concern. The oidcLabel is rendered as escaped text content.
Cookie/credentials.apiFetch uses credentials: 'same-origin' (api.ts:96) — no credentialed cross-origin requests, no CSRF surface introduced by the frontend. SameSite/CSRF on the auth POSTs is a backend concern and not visible here; nothing in this PR weakens it.
Cache hygiene on logout.useLogout sets me to null and removes all non-auth cached queries (auth.ts:90-95), so a subsequent user on a shared browser session does not see the previous user's gardens/plants. Verified the predicate keeps ['auth','me']/['auth','providers'] and drops everything else.
No injection / SSRF / unsafe deserialization. All server responses are zod-parsed (userSchema, providersSchema) before use; error bodies are read via narrow casts (apiErrorCode, auth.ts:100-106); no dangerouslySetInnerHTML, eval, template-string SQL, or dynamic href from untrusted input anywhere in the diff.
No security findings to report for this lens.
🎯 Correctness — Minor issues
Corrected review
VERDICT: Minor issues
OIDC login drops the redirect target — web/src/pages/LoginPage.tsx:60-64. The local form threads the remembered destination (dest = safeRedirect(search.redirect) → navigate({ to: dest })), but the OIDC anchor is a plain <a href={OIDC_LOGIN_URL}> with no redirect query param forwarded, and the backend oidcLogin handler reads no redirect param (internal/api/oidc.go:123-150) while oidcCallback's success path hardcodes c.Redirect(http.StatusFound, "/gardens") (internal/api/oidc.go:255). So a user bounced from /gardens/1 → /login?redirect=%2Fgardens%2F1 who chooses "Sign in with SSO" lands on /gardens, not /gardens/1 — the destination is silently lost. The frontend makes no attempt to thread the target through; fixing it would require backend support (carrying redirect through the OIDC tx cookie and honoring it on success). The redirect-roundtrip behavior therefore does not hold for OIDC, which is the primary sign-in path when configured.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/pages/LoginPage.tsx:24 and web/src/router.tsx:40 — duplicated redirect-sanitization helper.safeRedirect (LoginPage) and safeInternalPath (router) are byte-for-byte identical logic (dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens') living in two files under two names. This is exactly the copy-paste the PR's "shared" philosophy is meant to avoid: if the allow-rule tightens, one site will drift. Suggested fix: export one helper (e.g. safeInternalPath from @/lib/auth or a new @/lib/redirect) and have LoginPage import it. Verified by reading both files.
web/src/pages/LoginPage.tsx:114 — Divider is a generic UI primitive parked in a page file. It's a stateless, reusable "or" divider with no login-specific logic, yet it lives inside LoginPage.tsx and is not exported. The rest of the PR deliberately promotes shared visuals to components/ui/* (Alert, Button, TextField) and components/auth/* (AuthCard). Divider breaks that pattern for no reason. Suggest moving it to components/ui/Divider.tsx (or folding into AuthCard if it's meant to be auth-only). Verified by grep — Divider is referenced only in LoginPage.tsx.
web/src/lib/auth.ts:109 — errorMessage silently ignores its fallback for ApiError. The signature reads as "use this fallback message", but the implementation returns err.message for anyApiError, including ones whose server message is generic/technical (e.g. Request failed (NNN) from messageFrom in api.ts). Callers in LoginPage (line 90) and RegisterPage (line 87) pass tailored fallbacks ('Could not sign in.', 'Could not create your account.') that never win for an ApiError. Not a bug, but the API is misleading vs. its doc-comment. Either rename to clarify ("surfaces the server message if present") or prefer the caller's fallback when the server message is empty/generic. Verified at auth.ts:108-112, api.ts:73-81 (messageFrom), and the two call sites.
⚡ Performance — No material issues found
I read web/src/lib/auth.ts, web/src/router.tsx, and web/src/lib/api.ts to verify behavior. Reviewing through the performance lens only.
VERDICT: No material issues found
A few things I explicitly checked and confirmed are fine:
No N+1 / duplicate fetches on the auth probe.meQueryOptions is shared by useMe() (AppShell, mounted on every route) and the requireAuth/requireGuest guards via context.queryClient.ensureQueryData. Because both use the same ['auth','me'] queryKey against the single shared queryClient (lib/queryClient.ts), a logged-out user landing on /login produces exactly one /auth/me round-trip (401 → null), not one per guard + one per useMe. The shared client is what makes this work; the diff's separation of queryClient into its own module is the right call.
defaultPreload: 'intent' + beforeLoad guard. Hovering a guarded link fires ensureQueryData(meQueryOptions). With staleTime: 60_000 on meQueryOptions (overriding the 30s default), repeated intent-hovers within a minute do not refetch — no per-hover request storm. The providers query uses staleTime: Infinity, so it's fetched once per session.
useLogout cache teardown.qc.removeQueries({ predicate: (q) => q.queryKey[0] !== 'auth' }) is an O(n) pass over the (small) query cache on logout only — not a hot path — and correctly preserves the me/providers entries while dropping user-scoped data. Not a concern.
No avoidable allocations/copies on hot render paths.callbackErrors is a module-level constant; safeRedirect/safeInternalPath do trivial string checks; nothing allocates per-render in LoginPage/RegisterPage beyond React's own state.
🧯 Error handling & edge cases — Minor issues
All findings verified. Draft review confirmed.
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:23-26 — onLogout awaits logout.mutateAsync() with no try/catch. If the logout request fails (network, 5xx), the rejection is unhandled at the onClick handler, navigate({ to: '/login' }) never runs, and there's no Alert/feedback in AppShell — the button just re-enables and the user is left on the page with no indication anything went wrong. Verified by reading the file (no error UI in AppShell).
web/src/lib/auth.ts:34-37 — meQueryOptions.queryFn catches ApiError 401 → null, but a zod ZodError from a malformed /auth/me body (e.g., server returns 200 with unexpected shape) is rethrown. In requireAuth (router.tsx:26) this surfaces via ensureQueryData as a route beforeLoad error, and there is no errorComponent/ErrorBoundary defined anywhere in the route tree (grep confirmed none in the repo). Result: the user lands on TanStack's default raw-error render rather than any friendly handling. Same applies to requireGuest.
web/src/pages/LoginPage.tsx:37 — callbackErrors[search.error] ?? callbackErrors.oidc falls back to the generic oidc ("Single sign-on did not complete") message for any unknown ?error= code. validateSearch (router.tsx:62-67) accepts any string for error, so an attacker-supplied/typo'd code yields a misleading SSO message even on a non-OIDC flow. Minor UX/edge: better to show a generic fallback or restrict allowed codes. Verified by reading both files.
web/src/pages/LoginPage.tsx:42-43 & RegisterPage.tsx:22-23 — await navigate({ to: dest }) after a successful login/register is inside the same try block as the mutation. If navigation throws (e.g., redirect target's own beforeLoad redirects again, or an invalid path), the rejection is caught by the form's catch {} and silently swallowed alongside any mutation error display logic; the navigation failure is lost with no UI feedback. Minor, but it's the one path here where a navigation error is silently absorbed. Verified by reading both pages.
web/src/lib/auth.ts:89-95 — useLogout's cache cleanup (removeQueries) runs in onSuccess, which fires only on a successful logout. If the logout request fails (network/5xx), stale non-auth cache from the previous session remains, and since the cookie may still be valid the user stays logged in but with no cache cleanup. Consistent with "still logged in" semantics, but worth noting the asymmetry: the only cleanup path is gated on the success that, on failure, leaves the cache inconsistent with the (failed) intent. Verified by reading the hook.
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
I examined the auth surface carefully through the security lens:
CSRF: State-changing endpoints (/login, /register, /logout) are protected by the backend csrfGuard() middleware that validates the Origin header against PANSY_BASE_URL. The logout mutation POSTs to /auth/logout and is therefore covered.
Session fixation / cookie flags: Session cookie is HttpOnly, Secure (when served over HTTPS), SameSite=Lax, and scoped to path /. Correct.
OIDC flow security: State, PKCE verifier, and nonce are generated with 32 bytes of CSPRNG, stored in an HttpOnly cookie, and verified with constant-time comparison in the callback. The callback hardcodes redirect targets (/gardens on success, /login?error=... on failure) — no open redirect.
Redirect validation: Both safeRedirect (login page) and safeInternalPath (router) restrict redirect to same-origin absolute paths (startsWith('/') && !startsWith('//')). Protocol-relative URLs and external origins are blocked.
XSS: User-controlled inputs (email, display name, password, OIDC error codes) are rendered via React JSX, which escapes text content. Server error messages are hardcoded strings, not derived from user input.
Cache poisoning: me cache is primed with userSchema.parse(...) output, so unvalidated server responses cannot poison auth state.
No secret leakage: No API keys, tokens, or credentials are embedded in the frontend code.
The auth implementation is sound from a security perspective.
🎯 Correctness — Minor issues
Minor issues
web/src/components/ui/TextField.tsx:15 — inputId is computed as id ?? name. Both are optional on InputHTMLAttributes, so a caller that omits both (easy to do in a shared UI primitive) yields inputId === undefined. The <label htmlFor> and <input id> are then omitted, breaking the programmatic label–input association, and if hint is present aria-describedby becomes "undefined-hint". Fix: fall back to React.useId() when neither id nor name is supplied.
web/src/lib/auth.ts:39 — meQueryOptions sets staleTime: 60_000. With refetchOnWindowFocus: false (global default in queryClient.ts), a cached User object is treated as fresh for a full minute. If the session expires, or the user logs out in another tab, requireAuth in beforeLoad can still pass the guard and render protected routes using stale authenticated state. Fix: reduce staleTime to 0 for auth queries so ensureQueryData always verifies the session with the server before deciding access.
web/src/components/layout/AppShell.tsx:23-26 — onLogout awaits logout.mutateAsync() without a try/catch. If the network is down or the server returns an error, the rejection is unhandled and the user receives no feedback (the button simply flips from “Signing out…” back to “Sign out” with no alert). Fix: wrap the call in try/catch and surface logout.error (e.g. with an <Alert>) or switch to logout.mutate with an onError callback.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/pages/LoginPage.tsx:24 and web/src/router.tsx:40 — safeRedirect and safeInternalPath are identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g. lib/url.ts or lib/auth.ts) and import it in both files.
web/src/pages/LoginPage.tsx:11 — OIDC_LOGIN_URL hardcodes /api/v1, duplicating the base path already defined in lib/api.ts. If the API base ever changes, this link will break silently. Export BASE from lib/api.ts (or a derived helper) and construct the OIDC URL from it so the prefix lives in one place.
web/src/pages/RegisterPage.tsx:30 — RegisterPage only checks providers.isSuccess before deciding whether to show the SSO-only message, while LoginPage explicitly renders loading and error states for the same query (providers.isPending, providers.isError). This pattern divergence makes the auth surface harder to maintain; future changes to provider-state UX need to be remembered for both pages. Mirror the explicit pending/error handling from LoginPage in RegisterPage.
⚡ Performance — No material issues found
No material issues found
Through the performance lens, this diff is clean:
The shared QueryClient (queryClient.ts) eliminates duplicate caches between the React tree and router context.
ensureQueryData in the beforeLoad guards (router.tsx:26,34) only triggers a fetch when the me cache is missing or stale (staleTime: 60_000 in meQueryOptions), so repeated navigation within that window is instant and non-blocking.
providersQueryOptions correctly uses staleTime: Infinity because auth methods are session-static.
Logout’s removeQueries predicate (auth.ts:99) is scoped and appropriate—prevents cross-user data leakage without perf harm.
No N+1 patterns, unbounded growth, hot-loop allocations, or missing pagination are introduced.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
Broken redirect-after-login (router.tsx:28, router.tsx:82/89/96): requireAuth passes location.href (a full URL, e.g. https://example.com/gardens/1) into the login redirect search param. LoginPage.safeRedirect (LoginPage.tsx:24) only allows values starting with /, so the destination is always coerced to /gardens. Users are never returned to the protected page they originally requested. Fix: pass location.pathname + location.search instead of location.href in the three requireAuth call sites.
Unhandled logout failure (AppShell.tsx:23): onLogout awaits logout.mutateAsync() without a try/catch. If the logout request fails (network error, server 5xx), the rejected promise becomes an unhandled rejection and navigation to /login is skipped. Contrast with LoginPage and RegisterPage, which correctly wrap mutateAsync in try/catch. Fix: wrap the mutation call in try { … } catch { /* mutation error is already surfaced by React Query */ }.
TextField generates broken aria-describedby when id/name omitted (TextField.tsx:14): hintId is built as `${inputId}-hint`. If both id and name are omitted (possible because neither is required in TextFieldProps), inputId is undefined and hintId becomes the literal string "undefined-hint", producing a dangling aria-describedby. Fix: generate a fallback id when id ?? name is missing (e.g. useId() or require one of them).
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 23 findings (7 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | Logout failure is unhandled, leaving stale auth state and no user feedback | `web/src/components/layout/AppShell.tsx:23` | 5/5 | correctness, error-handling |
| 🔴 | RegisterPage renders form while providers query is pending or errored | `web/src/pages/RegisterPage.tsx:30` | 4/5 | correctness, error-handling, maintainability |
| 🟠 | Duplicate redirect sanitization logic with router.tsx | `web/src/pages/LoginPage.tsx:24` | 4/5 | maintainability |
| 🔴 | meQueryOptions only treats 401 as unauthenticated; other failures (network, 5xx, zod parse) propagate uncaught into router guards with no error boundary | `web/src/lib/auth.ts:34` | 2/5 | error-handling |
| 🔴 | Redirect-after-login broken because location.href (full URL) is passed but safeRedirect expects a path | `web/src/router.tsx:25` | 2/5 | error-handling |
| 🟠 | Duplicate redirect sanitization logic with LoginPage.tsx | `web/src/router.tsx:40` | 2/5 | maintainability |
| 🟡 | noValidate disables required/minLength guards; empty/invalid input is sent to the server with no client-side check | `web/src/pages/LoginPage.tsx:69` | 2/5 | error-handling |
<details><summary>16 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | hintId becomes literal 'undefined-hint' when id and name are both omitted | `web/src/components/ui/TextField.tsx:14` | opencode/kimi-k2.6:cloud | correctness, error-handling |
| 🟠 | meQueryOptions staleTime of 60s allows stale auth data to pass route guards | `web/src/lib/auth.ts:39` | opencode/kimi-k2.6:cloud | correctness |
| 🟠 | OIDC login drops the redirect target; user lands on /gardens instead of the remembered destination | `web/src/pages/LoginPage.tsx:60` | opencode/glm-5.2:cloud | correctness |
| 🟠 | requireAuth passes full href to path-only redirect sanitizer | `web/src/router.tsx:82` | kimi-k2.6:cloud | maintainability |
| 🟡 | 'web/src/pages/LoginPage.tsx:42-43' & 'RegisterPage.tsx:22-23' | `RegisterPage.tsx:22` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | 'web/src/lib/auth.ts:34-37' | `router.tsx:26` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | 'web/src/pages/LoginPage.tsx:37' | `router.tsx:62` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | Auth nav flickers unauthenticated state during me query loading | `web/src/components/layout/AppShell.tsx:18` | kimi-k2.6:cloud | error-handling |
| 🟡 | 'web/src/lib/auth.ts:89-95' | `web/src/lib/auth.ts:89` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | Generic API error helpers misplaced in auth module | `web/src/lib/auth.ts:99` | kimi-k2.6:cloud | maintainability |
| 🟡 | errorMessage ignores the caller's fallback for any ApiError, contradicting its doc-comment and surprising the login/register callers | `web/src/lib/auth.ts:109` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | Hardcoded API base path duplicates lib/api.ts constant | `web/src/pages/LoginPage.tsx:11` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | 'web/src/pages/LoginPage.tsx:37' | `web/src/pages/LoginPage.tsx:37` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | 'web/src/pages/LoginPage.tsx:42-43' & 'RegisterPage.tsx:22-23' | `web/src/pages/LoginPage.tsx:42` | opencode/glm-5.2:cloud | error-handling |
| 🟡 | Form with noValidate lacks client-side validation, allowing empty submissions | `web/src/pages/RegisterPage.tsx:55` | kimi-k2.6:cloud | error-handling |
| ⚪ | Divider is a generic UI primitive but lives unexported in a page file, breaking the components/ui/* pattern used by the rest of the PR | `web/src/pages/LoginPage.tsx:114` | opencode/glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No credential logging or client-side storage of secrets. That closes out the security lens investigation.
## VERDICT: No material issues found
Reviewed the auth data layer (`lib/auth.ts`), the fetch wrapper (`lib/api.ts`), the router guards (`router.tsx`), and the login/register pages through a security lens. Key things I verified rather than assumed:
- **Open-redirect guard is real and correctly restrictive.** `safeRedirect`/`safeInternalPath` (`web/src/pages/LoginPage.tsx:26`, `web/src/router.tsx`) reject anything not starting with `/`, and explicitly reject `//` (protocol-relative bypass). Verified the redirect destination only ever flows into TanStack Router's `navigate({to})`/`redirect({to})` — never into `window.location` or a raw `<a href>` — so even a scheme-like string can't cause an actual cross-origin navigation.
- **CSRF is already handled server-side**, independent of this PR: `internal/api/auth.go:131` (`csrfGuard`, Origin-header check) plus `SameSite=Lax` + `HttpOnly` + conditionally-`Secure` cookies (`internal/api/auth.go:186-205`), and the OIDC state cookie carries its own CSRF-state check (`internal/api/oidc.go:167`). The new `fetch` wrapper (`web/src/lib/api.ts:96`) uses `credentials: 'same-origin'`, so it won't leak the session cookie cross-origin.
- **No XSS surface.** All server-derived/user-derived strings (`user.displayName`, `providers.data.oidcLabel`, `errorMessage(err)`, the `?error=` callback text) are rendered as JSX children, never via `dangerouslySetInnerHTML`. The `?error=` code from the OIDC callback is used only as a lookup key into a fixed `callbackErrors` map (`LoginPage.tsx:12`) — the raw query value is never reflected into the DOM, confirmed the server only ever sets one of a fixed enum of codes (`internal/api/oidc.go:115-118`).
- **401-as-null pattern is sound.** `meQueryOptions` (`lib/auth.ts`) only swallows `401` into `null`; any other error (network failure, 5xx) propagates and `ensureQueryData` in `requireAuth`/`requireGuest` throws rather than treating the failure as "authenticated" — i.e., it fails closed, not open.
- **Server independently re-validates what the client only soft-checks.** Password min-length is enforced server-side too (`internal/api/auth.go:29`, `binding:"required,min=8,max=1024"`), so the client's `minLength={8}` isn't a bypassable control.
- **Logout hygiene.** `useLogout` clears the `me` cache and purges all non-`auth` query cache (`lib/auth.ts`), preventing a shared-device scenario where the next signed-in user could see stale cached data from the previous session.
I considered whether the client-side prefix check for redirects could be defeated by control-character tricks (e.g. a tab between slashes that some URL parsers normalize away), which is a known weakness in naive `startsWith('/')` guards — but since the destination is only ever consumed by the router's internal `navigate`/`redirect` (not assigned to `location.href` or an anchor), I could not establish an actual exploitation path, and didn't have `node_modules` available to inspect TanStack Router's exact `to`-resolution internals to rule it in or out. Flagging as considered-but-not-confirmed rather than reporting it as a finding.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
## Verified Review
**VERDICT: No material issues found**
The one flagged finding does not survive verification. `requireGuest`'s `throw redirect({ to: redirectTo })` at `web/src/router.tsx:36` omits a `search` field — and TanStack Router does not auto-forward the previous location's search params on a `redirect`/`navigate` unless explicitly passed (this codebase's own pattern confirms it: `requireAuth` at `router.tsx:28` explicitly sets `search: { redirect: href }` when it *wants* to forward a param, while `requireGuest` and the `/` redirect deliberately don't). So for the crafted case `/login?redirect=%2Flogin`: `requireGuest` computes `redirectTo = '/login'` and throws `redirect({ to: '/login' })` with no search — the resulting navigation lands on plain `/login` with no `redirect` param. `beforeLoad` reruns, `search.redirect` is now `undefined`, `safeInternalPath(undefined)` falls back to `/gardens`, and since the user is still authenticated, `requireGuest` redirects them to `/gardens`. That's a single harmless extra bounce through `/login`, not the infinite loop / browser-hang the draft describes — the router self-heals on the second pass. The draft's core impact claim is incorrect, so the finding is dropped.
Everything else in the draft (schema/tag parity, `REGISTRATION_CLOSED` code match, OIDC callback's hardcoded `/gardens` redirect, `meQueryOptions`/`useLogout` cache wiring, and the client-side password minimum being a UX-only stricter-than-server constraint) was independently spot-checked and holds up.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Confirmed: both `router.tsx:40` (`safeInternalPath`) and `LoginPage.tsx:24` (`safeRedirect`) have byte-identical bodies and same-origin logic, just different names, and no shared import between them. The finding holds.
**VERDICT: Minor issues**
- `web/src/pages/LoginPage.tsx:24` (`safeRedirect`) duplicates `web/src/router.tsx:40` (`safeInternalPath`) — identical body (`dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens'`) implementing the same same-origin redirect check under two different names, with no shared import. `router.tsx`'s copy guards `beforeLoad` (via `requireGuest`), while `LoginPage.tsx`'s copy guards the post-login `navigate` destination. A future change to the redirect-safety rule (tightening or loosening the same-origin check) would need to be applied in both places, and there's nothing forcing that — a fix to one silently leaves the other stale/inconsistent. Should be a single shared helper (e.g. exported from `lib/auth.ts`, which already owns auth/redirect concerns and is imported by both files).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed this PR strictly through the performance lens (auth data layer, query client config, router guards, and the new auth pages/components).
What I checked:
- `web/src/lib/auth.ts` — `meQueryOptions` (staleTime 60s) and `providersQueryOptions` (staleTime `Infinity`) are shared singletons keyed by `queryOptions()`, so `useMe()`, the router guards, and the mutations' `onSuccess` all hit the same cache entry — no duplicate fetching introduced.
- `web/src/router.tsx` — `requireAuth`/`requireGuest` call `ensureQueryData(meQueryOptions)` in `beforeLoad`; because of the 60s staleTime this is a cache hit on repeat navigations within the window, not a fetch-per-route-change.
- `useLogout`'s `qc.removeQueries({ predicate: ... })` (`web/src/lib/auth.ts`) is O(number of cached queries), but it only runs once per logout, not a hot path — fine at this scale.
- `web/src/lib/queryClient.ts` — single `QueryClient` instance shared between the router context and React tree (confirmed via `main.tsx` now importing it instead of constructing a second one), so no duplicate client/cache.
- `web/src/lib/api.ts` — default fetch wrapper, no added retry/backoff changes; 401 on `/auth/me` is caught inside `meQueryOptions` and resolved to `null` rather than thrown, so it won't trigger react-query's default retry loop.
- Components (`TextField`, `Button`, `Alert`, `AuthCard`) are simple, no loops, no unbounded lists, no unmemoized expensive computation.
Nothing here shows N+1 requests, unbounded growth, blocking hot-path calls, or quadratic behavior. This is a small, standard React Query-based auth UI; scope doesn't offer much surface for performance regressions, and I didn't find any.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All three findings check out exactly against the current source. The line references, code paths, and reasoning all match: `meQueryOptions` (auth.ts:29-38) only special-cases 401 and rethrows everything else (including network failures per api.ts:103-106 and zod parse errors), which then flows uncaught through `ensureQueryData` in `requireAuth`/`requireGuest` (router.tsx:26,34) with no error boundary anywhere in `web/src` (grep confirmed empty). `AppShell.tsx:23-26`'s `onLogout` has no try/catch, unlike Login/RegisterPage's submit handlers which do. `RegisterPage.tsx` (confirmed no `isPending`/`isError` checks anywhere in the file) falls through to the local form on a `providers` fetch failure, unlike `LoginPage.tsx:57-58` which explicitly handles both states.
## Review — 🧯 Error handling & edge cases
**VERDICT: Minor issues**
- **`web/src/lib/auth.ts:29-38` + `web/src/router.tsx:25-38`** — `meQueryOptions` only special-cases HTTP 401 as "logged out"; every other failure mode is rethrown unmodified: a network outage (`api.ts:103-106` throws `ApiError(..., status: 0, ...)` for fetch failures), a 5xx from the server, or a response that fails `userSchema.parse` (a `ZodError`, not an `ApiError`, so the `err instanceof ApiError` guard doesn't catch it) all propagate straight out of the query function. That rejection flows uncaught through `context.queryClient.ensureQueryData(meQueryOptions)` in both `requireAuth` and `requireGuest` (`router.tsx:26,34`), which have no try/catch around it. There is no `errorComponent`/`ErrorBoundary` anywhere in the app (confirmed via repo-wide grep), so any transient failure of `/auth/me` breaks navigation into every guarded route (`/gardens`, `/gardens/$gardenId`, `/plants`, and the guest-guarded `/login`/`/register`) instead of degrading gracefully.
- **`web/src/components/layout/AppShell.tsx:23-26`** — `onLogout` awaits `logout.mutateAsync()` with no try/catch, unlike `LoginPage.tsx` and `RegisterPage.tsx`, which both wrap their `mutateAsync` calls. If `/auth/logout` fails (network drop or 5xx — same `ApiError` path as above), the rejection becomes an unhandled promise rejection inside the click handler: `navigate` never runs, `logout.isPending` resets to `false` so the button silently re-enables, and there is no `Alert`/message shown anywhere — the user is left looking "signed in" with no indication the sign-out failed.
- **`web/src/pages/RegisterPage.tsx`** — unlike `LoginPage.tsx:57-58` (which renders `providers.isPending`/`providers.isError` states), `RegisterPage` never checks `providers.isPending` or `providers.isError`. If the `/auth/providers` fetch fails, the `providers.isSuccess && !providers.data.local` guard (line 30) is simply false, so the component falls through to rendering the full local-registration form even on servers configured for SSO-only — an inconsistent, unhandled edge case compared to how `LoginPage` handles the same fetch failure.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## VERDICT: No material issues found
Through the security lens, I traced the tainted, user-controlled inputs to their sinks:
- **`redirect` search param** — used in two places: `safeRedirect(search.redirect)` in `LoginPage` and `safeInternalPath(search.redirect)` in `router.tsx:40-42`. Both sinks call TanStack Router's `navigate()`/`redirect()` (SPA-internal navigation), not `window.location`/`<a href>`. The `startsWith('/') && !startsWith('//')` guard blocks protocol-relative (`//evil.com`) and absolute-URL (`http://...`) values. The one gap — a leading backslash (`/\evil.com`) passes the check — is **not exploitable** here because the router resolves `to` against its own route tree and renders `notFound` rather than performing a cross-origin browser navigation. So no open-redirect vulnerability. (Defense-in-depth: a `new URL(dest, location.origin).origin === location.origin` check would be more robust, but it's not required for safety.)
- **`error` search param** — never reflected raw; it's only a lookup key into the fixed `callbackErrors` map (`LoginPage.tsx:11-19`), and the rendered string is always a predefined constant. No reflected XSS.
- **Server-supplied strings** (`err.message` via `errorMessage`, `providers.oidcLabel`, `user.displayName`) are rendered as React text children in `Alert`/spans, so React escapes them. No `dangerouslySetInnerHTML` anywhere in the diff.
- **`OIDC_LOGIN_URL`** is a hardcoded same-origin path (`/api/v1/auth/oidc/login`), not attacker-influenced.
- **`location.href`** passed to `requireAuth` is TanStack Router's `location.href` (pathname+search+hash, always same-origin-relative), not the browser's full URL.
Verified by reading `web/src/lib/api.ts` (ApiError contract, `credentials: 'same-origin'`, no raw HTML handling) and `web/src/router.tsx` (guard logic + search validation).
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**VERDICT: Minor issues**
- **`web/src/pages/RegisterPage.tsx:30`** — The "local registration disabled" branch only fires once `providers.isSuccess`. While the providers query is still loading (`isPending`), the condition `providers.isSuccess && !providers.data.local` is false, so the page falls through and renders the full registration form on a server where local auth is actually disabled. A user on an SSO-only server can fill out and submit the form before the providers response arrives, only to be rejected by the server. `LoginPage` handles this symmetrically: it gates the local form behind `const local = providers.data?.local ?? false` (which defaults to `false` during load) and renders a `providers.isPending && <p>Loading…</p>` state, so nothing actionable appears until the response arrives. `RegisterPage` has no such gating and shows the form by default. Suggested fix: render a "Loading…" state (or nothing) until `providers` resolves, mirroring `LoginPage`'s `providers.isPending` handling, so the form never appears on a local-disabled server.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 8: all chain targets failed
ollama-cloud/glm-5.2:cloud: model returned an empty response
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
I reviewed the change through the performance lens, verifying the query/cache strategy against the actual code rather than the diff alone.
- **`meQueryOptions` + `ensureQueryData` guard sharing one cache (`lib/auth.ts:29`, `router.tsx:26,34`):** `useMe()` in `AppShell` and the `requireAuth`/`requireGuest` guards read the same `['auth','me']` entry, so there is no duplicate `/auth/me` fetch between the guard resolving and the component rendering. Verified the cache key and `queryOptions` shape match across both call sites.
- **Login/logout cache priming avoids refetches (`lib/auth.ts:73,82,91`):** `useLogin`/`useRegister` prime `me` via `setQueryData`, so the subsequent `navigate({ to: dest })` → `requireAuth` → `ensureQueryData` finds fresh data and does not re-fetch. `useLogout` sets `me` to `null` (fresh), so the following `navigate({ to: '/login' })` → `requireGuest` → `ensureQueryData` resolves to `null` without another `/auth/me` round-trip. I confirmed `ensureQueryData` treats the `null` set by `setQueryData` as present-and-fresh, so this is a genuine save, not a hidden double-fetch.
- **`removeQueries` on logout (`lib/auth.ts:94`):** Iterates all cached queries with a predicate, but this runs once per logout (a cold, user-initiated path), not on any hot loop. Not material.
- **`providersQueryOptions` `staleTime: Infinity` (`lib/auth.ts:49`):** Fetches providers once per session; the logout cleanup intentionally preserves `auth`-keyed queries, so no refetch storm on the next sign-in. Fine.
- **`ensureQueryData` on every protected navigation (`router.tsx:26`):** Bounded by the 60s `staleTime`, with react-query deduping concurrent calls against the same key, so there is no N+1 or per-navigation refetch regression. `defaultPreload: 'intent'` can cause a `/auth/me` fetch on hover only once the entry is stale, which is the intended preload behavior.
No efficiency regressions, unbounded growth, hot-loop work, or duplicate-fetch issues found in this lens.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All three findings verified against the actual source.
**Finding 1 (AppShell logout):** Confirmed. `onLogout` (lines 23-26) awaits `logout.mutateAsync()` then `navigate` with no try/catch; the header renders only "Sign out" / "Signing out…", no `logout.isError`/`logout.error` UI. Login/Register pages show `Alert` on error; logout does not. The `useLogout` `onSuccess` clears the `me` cache, so on rejection `me` stays non-null and the navigate is skipped — confirmed in `auth.ts`.
**Finding 2 (noValidate):** Confirmed for both forms. `LoginPage.tsx:69` has `noValidate` with `required` fields and no explicit guard; `RegisterPage.tsx:55` has `noValidate` with `required`/`minLength` and no explicit guard. The mutation `onSubmit` calls `mutateAsync` directly with raw state.
**Finding 3 (router error):** Confirmed. `rootRoute` (router.tsx:21) is created with only `{ component: AppShell }`; `requireAuth`/`requireGuest` throw the raw error (non-redirect) from `ensureQueryData` when `meQueryOptions.queryFn` rethrows a non-401. `queryClient.ts` sets no `retry` override, so the framework default applies.
## Corrected review
VERDICT: Minor issues
- **`web/src/components/layout/AppShell.tsx:23-26` — logout failures are swallowed with no user feedback.** `onLogout` does `await logout.mutateAsync()` then `await navigate({ to: '/login' })` with no `try/catch`. If the `/auth/logout` request fails (e.g. transient network failure → `ApiError` status 0, or the server 5xxs), `mutateAsync` rejects: the navigate never runs, the rejection becomes an unhandled promise rejection, and `onSuccess` never fires so the `me` cache stays non-null — the user appears still logged in. Unlike login/register (which surface `login.error`/`register.error` via an `Alert`), logout has **no UI for `logout.isError`**; the button just flips back to "Sign out" silently. Suggest wrapping in `try/catch` and showing `logout.error` (e.g. an `Alert` in the header or a toast), and only navigating on success.
- **`web/src/pages/LoginPage.tsx:69` and `web/src/pages/RegisterPage.tsx:55` (register form) — `noValidate` silently disables the declared `required`/`minLength` guards.** Both forms set `noValidate` but perform no client-side validation before `mutateAsync(...)`, so empty email/displayName and sub-8-char passwords are submitted straight to the network. The server does reject them (shown via `errorMessage`), so it isn't a crash, but the `required`/`minLength={8}` attributes are decorative. Either drop `noValidate` (let the browser enforce them) or add an explicit guard before the mutation to avoid pointless failed requests and to give immediate field-level feedback.
- **`web/src/router.tsx:25-30` / `:33-38` — a non-401 error in `ensureQueryData` throws inside `beforeLoad` with no `errorComponent` configured.** `meQueryOptions.queryFn` rethrows anything that isn't a 401 (e.g. a network failure, `status: 0`). The query default `retry` (3) softens transient blips, but a persistent outage on `/gardens` makes `requireAuth` throw a real (non-redirect) error in `beforeLoad`. `rootRoute` is created with only `{ component: AppShell }` — no `errorComponent`/`notFoundComponent` — so the user gets TanStack's default raw error screen instead of being sent to login or offered a retry. Consider adding a root `errorComponent` (with a "try again" affordance) or treating a network error in the guard distinctly from "unauthenticated." (Verified `rootRoute` at `router.tsx:21` and the queryClient defaults at `web/src/lib/queryClient.ts:7` — no `retry` override, so the framework default retry applies.)
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
I examined the auth surface carefully for open redirects, XSS via URL parameters, injection paths, and insecure defaults. The code holds up:
- **`router.tsx:40`** / **`LoginPage.tsx:24`** — `safeInternalPath` and `safeRedirect` both restrict `redirect` to same-origin absolute paths (`startsWith('/') && !startsWith('//')`). Verified callers: `requireAuth` (router guard) and `LoginPage` post-login navigation. No open redirect.
- **`LoginPage.tsx:37`** — The `?error=` OIDC callback parameter is mapped through a whitelist (`callbackErrors`) before rendering. Arbitrary user-supplied values fall back to a safe default string. No XSS via error codes.
- **`lib/auth.ts`** — `meQueryOptions` swallows 401 as `null`, avoiding query-error surfacing. `useLogout` clears the `me` cache and drops non-auth queries so stale data doesn't leak across sessions.
- **`lib/api.ts:96`** — Fetch uses `credentials: 'same-origin'`, appropriate for the embedded-single-binary architecture.
Nothing in this diff crosses into a genuine security defect from the frontend side.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I scrutinized the auth layer, routing guards, form validation, cache priming/clearing, redirect handling, and OIDC callback error mapping against the backend contract. The implementation is semantically correct:
- `meQueryOptions` correctly treats 401 as `null` (unauthenticated state), shared between `useMe` and the router guard, with `ensureQueryData` keeping them in sync.
- `useLogout` atomically nulls the `me` cache and scrubs non-auth queries, preventing cross-user data leakage.
- `useLogin`/`useRegister` prime the `me` cache from the mutation response, avoiding a redundant fetch.
- `callbackErrors` maps exactly to the backend OIDC error codes (`oidc_unavailable`, `state`, `no_email`, `email_unverified`, `oidc_conflict`, `oidc`).
- The backend `LOCAL_AUTH_DISABLED` / `REGISTRATION_CLOSED` codes are consumed appropriately; the frontend also gates the register form on the `providers` query so the common case is handled before submission.
- `safeRedirect`/`safeInternalPath` correctly restrict redirects to path-absolute URLs; tanstack router’s `navigate` only performs in-app routing, so the `//` check is sufficient for the current architecture.
- `TextField` generates proper `id`/`htmlFor`/`aria-describedby` linkage in all actual call-sites (every usage supplies `name`).
Nothing in the diff violates domain correctness or introduces a logic bug.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues
- **`safeRedirect` duplicated in two files** — `LoginPage.tsx:24` defines `safeRedirect` and `router.tsx:40` defines an almost identical `safeInternalPath`. Keeping two copies of the same origin-check in sync is brittle. **Fix:** move one shared helper to `lib/auth.ts` (or `lib/api.ts`) and import it in both places.
- **Generic API helpers live in the auth module** — `errorMessage` and `apiErrorCode` (`lib/auth.ts:99-112`) are general-purpose utilities that depend only on `ApiError`, not on auth concepts. Leaving them in `lib/auth.ts` couples unrelated modules to auth when they need error formatting. **Fix:** move them next to `ApiError` in `lib/api.ts`.
- **`requireAuth` passes a full `href` to a path-only sanitizer** — `router.tsx:82,89,96` pass `location.href` (a full URL including origin) into `requireAuth`, which stores it in `search.redirect`. Later, `LoginPage.tsx:36` runs `safeRedirect(search.redirect)`, which expects a path and checks `dest.startsWith('/')`. A full `https://…` href fails that check and silently falls back to `/gardens`, so the “remember where they were” feature is broken. The mismatch between what the helper claims to handle (paths) and what the caller supplies (hrefs) is confusing. **Fix:** pass `location.pathname + location.search` (or a dedicated path field) instead of `location.href`, or rename the helper and make it handle both.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The auth layer is well-designed from a performance standpoint. Shared `queryOptions` (`meQueryOptions`, `providersQueryOptions`) ensure a single cache entry is read by both route guards (`ensureQueryData` in `beforeLoad`) and components (`useMe`, `useProviders`). `staleTime` is set sensibly (`60_000` for the user query, `Infinity` for providers), so protected-route navigation and page switches won't trigger redundant fetches within a session. Logout’s `removeQueries` predicate is bounded and only runs on an infrequent user action. No hot loops, N+1 patterns, unbounded growth, or blocking work on hot paths were introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/components/layout/AppShell.tsx:23-26`** — `onLogout` awaits `logout.mutateAsync()` without a `try/catch`. If the logout request fails (network error, server 5xx), the error becomes an unhandled promise rejection, the `me` cache is **never** cleared (the `useLogout` `onSuccess` only runs on success), and the UI continues to show the user as signed in with zero feedback.
**Fix:** Wrap `logout.mutateAsync()` in `try/catch`; on error, surface it in the UI (e.g., an inline alert) or at least ensure the cache is invalidated so the client doesn’t pretend the session is still valid.
- **`web/src/pages/RegisterPage.tsx:30`** — The page immediately falls through to render the full registration form when `providers` is still loading (`isPending`) or has errored (`isError`). Unlike `LoginPage`, there is no loading spinner or error alert for the providers query, so the form is interactive before the app even knows whether local registration is enabled.
**Fix:** Add early returns for `providers.isPending` and `providers.isError`, mirroring the guard pattern in `LoginPage.tsx:57-58`.
**Minor issues**
- **`web/src/components/layout/AppShell.tsx:18-21`** — `useMe()`’s loading state is ignored (`const user = me.data`). On a hard refresh, `data` is `undefined` while the query is pending, so authenticated users see a brief flash of the “Sign in” link and the unauthenticated nav layout.
**Fix:** Check `me.isPending` and render a neutral loading state (e.g., a skeleton or simply omit the auth-specific buttons until resolved).
- **`web/src/pages/LoginPage.tsx:69` and `web/src/pages/RegisterPage.tsx:55`** — Both `<form>` elements set `noValidate`, yet there is no compensating client-side validation. Empty strings or a password shorter than 8 chars are submitted to the server unnecessarily.
**Fix:** Add minimal validation (e.g., `email.trim()` and `password.length >= MIN_PASSWORD`) before calling `mutateAsync`, or remove `noValidate` if native browser validation is acceptable.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Verdict: No material issues found
I reviewed the change through the security lens, verifying against the checked-out code (`web/src/router.tsx`, `web/src/pages/LoginPage.tsx`, `web/src/lib/auth.ts`, `web/src/lib/api.ts`).
- **Open-redirect protection (verified).** `requireAuth` puts `location.href` (TanStack Router's in-app path/search/hash, not a full URL) into `search.redirect`, and `LoginPage.safeRedirect` (`LoginPage.tsx:24-26`) plus `safeInternalPath` (`router.tsx:40-42`) both reject anything not starting with `/` and reject `//` (protocol-relative). Known bypasses like `/\evil.com` are not exploitable here because `navigate({ to })` performs client-side routing only — TanStack won't issue a document-level navigation to an external URL — so a slipped-through value just produces a broken in-app route, not an open redirect. I traced the full path from `requireAuth` → `search.redirect` → `safeRedirect` → `navigate`.
- **OIDC callback `?error=` codes.** `search.error` is only used as a lookup key into the static `callbackErrors` map (`LoginPage.tsx:14-21, 37`); it is never rendered directly, so no reflected XSS. Unknown codes fall back to `callbackErrors.oidc`. Verified at `LoginPage.tsx:37`.
- **OIDC button.** `<a href="/api/v1/auth/oidc/login">` (`LoginPage.tsx:11, 60-64`) is a same-origin navigation with no `target="_blank"`, so no `rel="noopener"` concern. The `oidcLabel` is rendered as escaped text content.
- **Cookie/credentials.** `apiFetch` uses `credentials: 'same-origin'` (`api.ts:96`) — no credentialed cross-origin requests, no CSRF surface introduced by the frontend. SameSite/CSRF on the auth POSTs is a backend concern and not visible here; nothing in this PR weakens it.
- **Cache hygiene on logout.** `useLogout` sets `me` to `null` and removes all non-`auth` cached queries (`auth.ts:90-95`), so a subsequent user on a shared browser session does not see the previous user's gardens/plants. Verified the predicate keeps `['auth','me']`/`['auth','providers']` and drops everything else.
- **No injection / SSRF / unsafe deserialization.** All server responses are zod-parsed (`userSchema`, `providersSchema`) before use; error bodies are read via narrow casts (`apiErrorCode`, `auth.ts:100-106`); no `dangerouslySetInnerHTML`, `eval`, template-string SQL, or dynamic `href` from untrusted input anywhere in the diff.
No security findings to report for this lens.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## Corrected review
**VERDICT: Minor issues**
- **OIDC login drops the `redirect` target** — `web/src/pages/LoginPage.tsx:60-64`. The local form threads the remembered destination (`dest = safeRedirect(search.redirect)` → `navigate({ to: dest })`), but the OIDC anchor is a plain `<a href={OIDC_LOGIN_URL}>` with no `redirect` query param forwarded, and the backend `oidcLogin` handler reads no redirect param (`internal/api/oidc.go:123-150`) while `oidcCallback`'s success path hardcodes `c.Redirect(http.StatusFound, "/gardens")` (`internal/api/oidc.go:255`). So a user bounced from `/gardens/1` → `/login?redirect=%2Fgardens%2F1` who chooses "Sign in with SSO" lands on `/gardens`, not `/gardens/1` — the destination is silently lost. The frontend makes no attempt to thread the target through; fixing it would require backend support (carrying `redirect` through the OIDC tx cookie and honoring it on success). The redirect-roundtrip behavior therefore does not hold for OIDC, which is the primary sign-in path when configured.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**VERDICT: Minor issues**
- **`web/src/pages/LoginPage.tsx:24` and `web/src/router.tsx:40` — duplicated redirect-sanitization helper.** `safeRedirect` (LoginPage) and `safeInternalPath` (router) are byte-for-byte identical logic (`dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens'`) living in two files under two names. This is exactly the copy-paste the PR's "shared" philosophy is meant to avoid: if the allow-rule tightens, one site will drift. Suggested fix: export one helper (e.g. `safeInternalPath` from `@/lib/auth` or a new `@/lib/redirect`) and have `LoginPage` import it. Verified by reading both files.
- **`web/src/pages/LoginPage.tsx:114` — `Divider` is a generic UI primitive parked in a page file.** It's a stateless, reusable "or" divider with no login-specific logic, yet it lives inside `LoginPage.tsx` and is not exported. The rest of the PR deliberately promotes shared visuals to `components/ui/*` (`Alert`, `Button`, `TextField`) and `components/auth/*` (`AuthCard`). `Divider` breaks that pattern for no reason. Suggest moving it to `components/ui/Divider.tsx` (or folding into `AuthCard` if it's meant to be auth-only). Verified by grep — `Divider` is referenced only in `LoginPage.tsx`.
- **`web/src/lib/auth.ts:109` — `errorMessage` silently ignores its `fallback` for `ApiError`.** The signature reads as "use this fallback message", but the implementation returns `err.message` for *any* `ApiError`, including ones whose server message is generic/technical (e.g. `Request failed (NNN)` from `messageFrom` in `api.ts`). Callers in `LoginPage` (line 90) and `RegisterPage` (line 87) pass tailored fallbacks (`'Could not sign in.'`, `'Could not create your account.'`) that never win for an `ApiError`. Not a bug, but the API is misleading vs. its doc-comment. Either rename to clarify ("surfaces the server message if present") or prefer the caller's fallback when the server message is empty/generic. Verified at `auth.ts:108-112`, `api.ts:73-81` (`messageFrom`), and the two call sites.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I read `web/src/lib/auth.ts`, `web/src/router.tsx`, and `web/src/lib/api.ts` to verify behavior. Reviewing through the performance lens only.
**VERDICT: No material issues found**
A few things I explicitly checked and confirmed are fine:
- **No N+1 / duplicate fetches on the auth probe.** `meQueryOptions` is shared by `useMe()` (AppShell, mounted on every route) and the `requireAuth`/`requireGuest` guards via `context.queryClient.ensureQueryData`. Because both use the same `['auth','me']` queryKey against the single shared `queryClient` (`lib/queryClient.ts`), a logged-out user landing on `/login` produces exactly one `/auth/me` round-trip (401 → `null`), not one per guard + one per `useMe`. The shared client is what makes this work; the diff's separation of `queryClient` into its own module is the right call.
- **`defaultPreload: 'intent'` + `beforeLoad` guard.** Hovering a guarded link fires `ensureQueryData(meQueryOptions)`. With `staleTime: 60_000` on `meQueryOptions` (overriding the 30s default), repeated intent-hovers within a minute do not refetch — no per-hover request storm. The providers query uses `staleTime: Infinity`, so it's fetched once per session.
- **`useLogout` cache teardown.** `qc.removeQueries({ predicate: (q) => q.queryKey[0] !== 'auth' })` is an O(n) pass over the (small) query cache on logout only — not a hot path — and correctly preserves the `me`/`providers` entries while dropping user-scoped data. Not a concern.
- **No avoidable allocations/copies on hot render paths.** `callbackErrors` is a module-level constant; `safeRedirect`/`safeInternalPath` do trivial string checks; nothing allocates per-render in `LoginPage`/`RegisterPage` beyond React's own state.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All findings verified. Draft review confirmed.
## VERDICT: Minor issues
- **`web/src/components/layout/AppShell.tsx:23-26`** — `onLogout` awaits `logout.mutateAsync()` with no try/catch. If the logout request fails (network, 5xx), the rejection is unhandled at the onClick handler, `navigate({ to: '/login' })` never runs, and there's no `Alert`/feedback in `AppShell` — the button just re-enables and the user is left on the page with no indication anything went wrong. **Verified** by reading the file (no error UI in AppShell).
- **`web/src/lib/auth.ts:34-37`** — `meQueryOptions.queryFn` catches `ApiError` 401 → `null`, but a zod `ZodError` from a malformed `/auth/me` body (e.g., server returns 200 with unexpected shape) is rethrown. In `requireAuth` (`router.tsx:26`) this surfaces via `ensureQueryData` as a route `beforeLoad` error, and there is no `errorComponent`/`ErrorBoundary` defined anywhere in the route tree (grep confirmed none in the repo). Result: the user lands on TanStack's default raw-error render rather than any friendly handling. Same applies to `requireGuest`.
- **`web/src/pages/LoginPage.tsx:37`** — `callbackErrors[search.error] ?? callbackErrors.oidc` falls back to the generic `oidc` ("Single sign-on did not complete") message for any unknown `?error=` code. `validateSearch` (`router.tsx:62-67`) accepts any string for `error`, so an attacker-supplied/typo'd code yields a misleading SSO message even on a non-OIDC flow. Minor UX/edge: better to show a generic fallback or restrict allowed codes. **Verified** by reading both files.
- **`web/src/pages/LoginPage.tsx:42-43` & `RegisterPage.tsx:22-23`** — `await navigate({ to: dest })` after a successful login/register is inside the same `try` block as the mutation. If navigation throws (e.g., redirect target's own `beforeLoad` redirects again, or an invalid path), the rejection is caught by the form's `catch {}` and silently swallowed alongside any mutation error display logic; the navigation failure is lost with no UI feedback. Minor, but it's the one path here where a navigation error is silently absorbed. **Verified** by reading both pages.
- **`web/src/lib/auth.ts:89-95`** — `useLogout`'s cache cleanup (`removeQueries`) runs in `onSuccess`, which fires only on a successful logout. If the logout request fails (network/5xx), stale non-auth cache from the previous session remains, and since the cookie may still be valid the user stays logged in but with no cache cleanup. Consistent with "still logged in" semantics, but worth noting the asymmetry: the only cleanup path is gated on the success that, on failure, leaves the cache inconsistent with the (failed) intent. **Verified** by reading the hook.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
I examined the auth surface carefully through the security lens:
- **CSRF**: State-changing endpoints (`/login`, `/register`, `/logout`) are protected by the backend `csrfGuard()` middleware that validates the `Origin` header against `PANSY_BASE_URL`. The logout mutation POSTs to `/auth/logout` and is therefore covered.
- **Session fixation / cookie flags**: Session cookie is `HttpOnly`, `Secure` (when served over HTTPS), `SameSite=Lax`, and scoped to path `/`. Correct.
- **OIDC flow security**: State, PKCE verifier, and nonce are generated with 32 bytes of CSPRNG, stored in an `HttpOnly` cookie, and verified with constant-time comparison in the callback. The callback hardcodes redirect targets (`/gardens` on success, `/login?error=...` on failure) — no open redirect.
- **Redirect validation**: Both `safeRedirect` (login page) and `safeInternalPath` (router) restrict `redirect` to same-origin absolute paths (`startsWith('/') && !startsWith('//')`). Protocol-relative URLs and external origins are blocked.
- **XSS**: User-controlled inputs (email, display name, password, OIDC error codes) are rendered via React JSX, which escapes text content. Server error messages are hardcoded strings, not derived from user input.
- **Cache poisoning**: `me` cache is primed with `userSchema.parse(...)` output, so unvalidated server responses cannot poison auth state.
- **No secret leakage**: No API keys, tokens, or credentials are embedded in the frontend code.
The auth implementation is sound from a security perspective.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Minor issues**
- **`web/src/components/ui/TextField.tsx:15`** — `inputId` is computed as `id ?? name`. Both are optional on `InputHTMLAttributes`, so a caller that omits both (easy to do in a shared UI primitive) yields `inputId === undefined`. The `<label htmlFor>` and `<input id>` are then omitted, breaking the programmatic label–input association, and if `hint` is present `aria-describedby` becomes `"undefined-hint"`.
**Fix:** fall back to `React.useId()` when neither `id` nor `name` is supplied.
- **`web/src/lib/auth.ts:39`** — `meQueryOptions` sets `staleTime: 60_000`. With `refetchOnWindowFocus: false` (global default in `queryClient.ts`), a cached `User` object is treated as fresh for a full minute. If the session expires, or the user logs out in another tab, `requireAuth` in `beforeLoad` can still pass the guard and render protected routes using stale authenticated state.
**Fix:** reduce `staleTime` to `0` for auth queries so `ensureQueryData` always verifies the session with the server before deciding access.
- **`web/src/components/layout/AppShell.tsx:23-26`** — `onLogout` awaits `logout.mutateAsync()` without a `try/catch`. If the network is down or the server returns an error, the rejection is unhandled and the user receives no feedback (the button simply flips from “Signing out…” back to “Sign out” with no alert).
**Fix:** wrap the call in `try/catch` and surface `logout.error` (e.g. with an `<Alert>`) or switch to `logout.mutate` with an `onError` callback.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`web/src/pages/LoginPage.tsx:24`** and **`web/src/router.tsx:40`** — `safeRedirect` and `safeInternalPath` are identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g. `lib/url.ts` or `lib/auth.ts`) and import it in both files.
- **`web/src/pages/LoginPage.tsx:11`** — `OIDC_LOGIN_URL` hardcodes `/api/v1`, duplicating the base path already defined in `lib/api.ts`. If the API base ever changes, this link will break silently. Export `BASE` from `lib/api.ts` (or a derived helper) and construct the OIDC URL from it so the prefix lives in one place.
- **`web/src/pages/RegisterPage.tsx:30`** — `RegisterPage` only checks `providers.isSuccess` before deciding whether to show the SSO-only message, while `LoginPage` explicitly renders loading and error states for the same query (`providers.isPending`, `providers.isError`). This pattern divergence makes the auth surface harder to maintain; future changes to provider-state UX need to be remembered for both pages. Mirror the explicit pending/error handling from `LoginPage` in `RegisterPage`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Through the performance lens, this diff is clean:
- The shared `QueryClient` (`queryClient.ts`) eliminates duplicate caches between the React tree and router context.
- `ensureQueryData` in the `beforeLoad` guards (`router.tsx:26,34`) only triggers a fetch when the `me` cache is missing or stale (`staleTime: 60_000` in `meQueryOptions`), so repeated navigation within that window is instant and non-blocking.
- `providersQueryOptions` correctly uses `staleTime: Infinity` because auth methods are session-static.
- Logout’s `removeQueries` predicate (`auth.ts:99`) is scoped and appropriate—prevents cross-user data leakage without perf harm.
- No N+1 patterns, unbounded growth, hot-loop allocations, or missing pagination are introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **Broken redirect-after-login (`router.tsx:28`, `router.tsx:82/89/96`)**: `requireAuth` passes `location.href` (a full URL, e.g. `https://example.com/gardens/1`) into the login `redirect` search param. `LoginPage.safeRedirect` (`LoginPage.tsx:24`) only allows values starting with `/`, so the destination is always coerced to `/gardens`. Users are never returned to the protected page they originally requested. **Fix:** pass `location.pathname + location.search` instead of `location.href` in the three `requireAuth` call sites.
- **Unhandled logout failure (`AppShell.tsx:23`)**: `onLogout` awaits `logout.mutateAsync()` without a `try/catch`. If the logout request fails (network error, server 5xx), the rejected promise becomes an unhandled rejection and navigation to `/login` is skipped. Contrast with `LoginPage` and `RegisterPage`, which correctly wrap `mutateAsync` in `try/catch`. **Fix:** wrap the mutation call in `try { … } catch { /* mutation error is already surfaced by React Query */ }`.
- **TextField generates broken `aria-describedby` when `id`/`name` omitted (`TextField.tsx:14`)**: `hintId` is built as `` `${inputId}-hint` ``. If both `id` and `name` are omitted (possible because neither is required in `TextFieldProps`), `inputId` is `undefined` and `hintId` becomes the literal string `"undefined-hint"`, producing a dangling `aria-describedby`. **Fix:** generate a fallback id when `id ?? name` is missing (e.g. `useId()` or require one of them).
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Fixes from the PR #25 adversarial review (graded 23 real / 5 false positive):
Error handling
- onLogout wraps mutateAsync in try/catch: a failed logout no longer
becomes an unhandled rejection; the user stays put (session still valid)
and the button offers "Retry sign out" (5 models flagged this).
- Root errorComponent (RouteError): a non-401 /auth/me failure in a
beforeLoad guard now shows a recoverable "Try again" screen instead of
blanking.
- Forms drop noValidate, restoring native required/type=email/minLength
checks before hitting the server.
Correctness
- RegisterPage gates on providers.isPending/isError before rendering, so
the form no longer briefly appears (submittable) on an SSO-only server.
- TextField id falls back to name then a useId() value, so the label/hint
associations hold even if a caller omits both.
Maintainability
- Single safeRedirectPath (lib/redirect.ts) replaces the duplicated
safeRedirect/safeInternalPath (4 models).
- Generic ApiError helpers (apiErrorCode, errorMessage) moved to lib/api.ts;
LoginPage builds the OIDC URL from the exported API_BASE.
- Divider moved to components/ui/. errorMessage doc clarified.
Verified again in a real browser: register -> /gardens; Sign out -> /login.
tsc --noEmit and vite build clean.
Not changed (graded, with rationale): OIDC deep-link redirect isn't
preserved (needs backend state threading — follow-up); 60s me staleTime in
guards (server is authoritative); the login/register onSubmit catches are
the intended react-query pattern (errors render via mutation state).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve
merged commit 03f2eb2d5c into main2026-07-18 22:11:21 +00:00
steve
deleted branch phase-1-auth-ui2026-07-18 22:11:21 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #6. Phase 1, step 3 (final) of the tracking epic #20. Consumes the backend from #4/#5.
What's here
lib/auth.ts— zod-validatedUser/Providers; a sharedmeQueryOptions(a 401 resolves tonull, not an error) reused byuseMe()and the router guard;useProviders(); anduseLogin/useRegister/useLogoutmutations that prime/clear themecache (logout also drops non-auth cached data so the next user never sees the previous one's).lib/queryClient.ts— oneQueryClientshared by the React tree and the router context, so guards and components read the same cache.router.tsx—createRootRouteWithContextwith the queryClient;requireAuthon gardens/plants/editor (→/login?redirect=<dest>),requireGueston login/register. Login search params (redirect,error) validated as optional; redirect targets restricted to same-origin paths.LoginPage— OIDC button (label from providers) →/auth/oidc/login, an "or" divider, and the local email/password form; friendly messages for the OIDC callback?error=codes (no_email,email_unverified,oidc_conflict, …).RegisterPage— email + display name + password (min 8); registration-closed and SSO-only handling; auto-login lands on/gardens.AppShell— auth-aware nav: user's display name + Sign out when logged in, Sign in otherwise; nav links only when authed.ui/—TextField(16px text to avoid iOS zoom, autocomplete + a11y),Button(+ sharedbuttonClassesfor the OIDC anchor),Alert;AuthCard.Verification
npx tsc --noEmitclean;vite buildsucceeds./gardens; full-page refresh stays signed in;/login, session cleared;/gardens/1→/login?redirect=%2Fgardens%2F1, and logging in returns you to the destination;Out of scope
🤖 Generated with Claude Code
🪰 Gadfly — live review status
4/5 reviewers finished · updated 2026-07-18 21:59:13Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ⏳ 4/5 lensesLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 20 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -14,6 +15,16 @@ const navLinkInactive = 'text-muted hover:bg-border/60 hover:text-fg'/** Top-level chrome: a sticky nav bar plus the routed page in an <Outlet>. */export function AppShell() {const me = useMe()🟡 Auth nav flickers unauthenticated state during me query loading
error-handling · flagged by 1 model
web/src/components/layout/AppShell.tsx:18-21—useMe()’s loading state is ignored (const user = me.data). On a hard refresh,dataisundefinedwhile the query is pending, so authenticated users see a brief flash of the “Sign in” link and the unauthenticated nav layout. Fix: Checkme.isPendingand render a neutral loading state (e.g., a skeleton or simply omit the auth-specific buttons until resolved).🪰 Gadfly · advisory
@@ -17,0 +20,4 @@const navigate = useNavigate()const user = me.dataasync function onLogout() {🔴 Logout failure is unhandled, leaving stale auth state and no user feedback
correctness, error-handling · flagged by 5 models
web/src/components/layout/AppShell.tsx:23-26—onLogoutawaitslogout.mutateAsync()without atry/catch. If the logout request fails (network error, server 5xx), the error becomes an unhandled promise rejection, themecache is never cleared (theuseLogoutonSuccessonly runs on success), and the UI continues to show the user as signed in with zero feedback. Fix: Wraplogout.mutateAsync()intry/catch; on error, surface it in the UI (e.g., an inline alert) or at l…🪰 Gadfly · advisory
@@ -0,0 +11,4 @@export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(function TextField({ label, hint, id, name, className, ...props },ref,) {🟠 hintId becomes literal 'undefined-hint' when id and name are both omitted
correctness, error-handling · flagged by 1 model
web/src/components/ui/TextField.tsx:15—inputIdis computed asid ?? name. Both are optional onInputHTMLAttributes, so a caller that omits both (easy to do in a shared UI primitive) yieldsinputId === undefined. The<label htmlFor>and<input id>are then omitted, breaking the programmatic label–input association, and ifhintis presentaria-describedbybecomes"undefined-hint". Fix: fall back toReact.useId()when neitheridnornameis supplied.🪰 Gadfly · advisory
@@ -0,0 +31,4 @@queryFn: async (): Promise<User | null> => {try {return userSchema.parse(await api.get('/auth/me'))} catch (err) {🔴 meQueryOptions only treats 401 as unauthenticated; other failures (network, 5xx, zod parse) propagate uncaught into router guards with no error boundary
error-handling · flagged by 2 models
web/src/lib/auth.ts:34-37—meQueryOptions.queryFncatchesApiError401 →null, but a zodZodErrorfrom a malformed/auth/mebody (e.g., server returns 200 with unexpected shape) is rethrown. InrequireAuth(router.tsx:26) this surfaces viaensureQueryDataas a routebeforeLoaderror, and there is noerrorComponent/ErrorBoundarydefined anywhere in the route tree (grep confirmed none in the repo). Result: the user lands on TanStack's default raw-error render rather…🪰 Gadfly · advisory
@@ -0,0 +36,4 @@throw err}},staleTime: 60_000,🟠 meQueryOptions staleTime of 60s allows stale auth data to pass route guards
correctness · flagged by 1 model
web/src/lib/auth.ts:39—meQueryOptionssetsstaleTime: 60_000. WithrefetchOnWindowFocus: false(global default inqueryClient.ts), a cachedUserobject is treated as fresh for a full minute. If the session expires, or the user logs out in another tab,requireAuthinbeforeLoadcan still pass the guard and render protected routes using stale authenticated state. Fix: reducestaleTimeto0for auth queries soensureQueryDataalways verifies the session with the se…🪰 Gadfly · advisory
@@ -0,0 +86,4 @@export function useLogout() {const qc = useQueryClient()return useMutation({mutationFn: () => api.post('/auth/logout'),🟡
web/src/lib/auth.ts:89-95error-handling · flagged by 1 model
web/src/lib/auth.ts:89-95—useLogout's cache cleanup (removeQueries) runs inonSuccess, which fires only on a successful logout. If the logout request fails (network/5xx), stale non-auth cache from the previous session remains, and since the cookie may still be valid the user stays logged in but with no cache cleanup. Consistent with "still logged in" semantics, but worth noting the asymmetry: the only cleanup path is gated on the success that, on failure, leaves the cache inconsi…🪰 Gadfly · advisory
@@ -0,0 +96,4 @@})}/** The server error code from an ApiError body ({error:{code,message}}), if any. */🟡 Generic API error helpers misplaced in auth module
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +106,4 @@}/** A user-facing message for a caught request error. */export function errorMessage(err: unknown, fallback = 'Something went wrong.'): string {🟡 errorMessage ignores the caller's fallback for any ApiError, contradicting its doc-comment and surprising the login/register callers
maintainability · flagged by 1 model
web/src/lib/auth.ts:109—errorMessagesilently ignores itsfallbackforApiError. The signature reads as "use this fallback message", but the implementation returnserr.messagefor anyApiError, including ones whose server message is generic/technical (e.g.Request failed (NNN)frommessageFrominapi.ts). Callers inLoginPage(line 90) andRegisterPage(line 87) pass tailored fallbacks ('Could not sign in.','Could not create your account.') that never win for…🪰 Gadfly · advisory
@@ -2,0 +8,4 @@// Server-initiated redirect (not a fetch): the browser navigates to the Go// handler, which 302s to the IdP.const OIDC_LOGIN_URL = '/api/v1/auth/oidc/login'🟡 Hardcoded API base path duplicates lib/api.ts constant
maintainability · flagged by 1 model
web/src/pages/LoginPage.tsx:11—OIDC_LOGIN_URLhardcodes/api/v1, duplicating the base path already defined inlib/api.ts. If the API base ever changes, this link will break silently. ExportBASEfromlib/api.ts(or a derived helper) and construct the OIDC URL from it so the prefix lives in one place.🪰 Gadfly · advisory
@@ -2,0 +21,4 @@}// Only follow same-origin absolute paths, never a caller-controlled external URL.function safeRedirect(dest: string | undefined): string {🟠 Duplicate redirect sanitization logic with router.tsx
maintainability · flagged by 4 models
web/src/pages/LoginPage.tsx:24andweb/src/router.tsx:40—safeRedirectandsafeInternalPathare identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g.lib/url.tsorlib/auth.ts) and import it in both files.🪰 Gadfly · advisory
@@ -5,0 +34,4 @@const [password, setPassword] = useState('')const dest = safeRedirect(search.redirect)const callbackError = search.error ? (callbackErrors[search.error] ?? callbackErrors.oidc) : null🟡
web/src/pages/LoginPage.tsx:37error-handling · flagged by 1 model
web/src/pages/LoginPage.tsx:37—callbackErrors[search.error] ?? callbackErrors.oidcfalls back to the genericoidc("Single sign-on did not complete") message for any unknown?error=code.validateSearch(router.tsx:62-67) accepts any string forerror, so an attacker-supplied/typo'd code yields a misleading SSO message even on a non-OIDC flow. Minor UX/edge: better to show a generic fallback or restrict allowed codes. Verified by reading both files.🪰 Gadfly · advisory
@@ -5,0 +39,4 @@async function onSubmit(e: FormEvent) {e.preventDefault()try {await login.mutateAsync({ email, password })🟡
web/src/pages/LoginPage.tsx:42-43&RegisterPage.tsx:22-23error-handling · flagged by 1 model
web/src/pages/LoginPage.tsx:42-43&RegisterPage.tsx:22-23—await navigate({ to: dest })after a successful login/register is inside the sametryblock as the mutation. If navigation throws (e.g., redirect target's ownbeforeLoadredirects again, or an invalid path), the rejection is caught by the form'scatch {}and silently swallowed alongside any mutation error display logic; the navigation failure is lost with no UI feedback. Minor, but it's the one path here where a navig…🪰 Gadfly · advisory
@@ -5,0 +57,4 @@{providers.isPending && <p className="text-sm text-muted">Loading…</p>}{providers.isError && <Alert>Could not load sign-in options. Please refresh.</Alert>}{oidc && (🟠 OIDC login drops the redirect target; user lands on /gardens instead of the remembered destination
correctness · flagged by 1 model
redirecttarget —web/src/pages/LoginPage.tsx:60-64. The local form threads the remembered destination (dest = safeRedirect(search.redirect)→navigate({ to: dest })), but the OIDC anchor is a plain<a href={OIDC_LOGIN_URL}>with noredirectquery param forwarded, and the backendoidcLoginhandler reads no redirect param (internal/api/oidc.go:123-150) whileoidcCallback's success path hardcodesc.Redirect(http.StatusFound, "/gardens")(`internal/api…🪰 Gadfly · advisory
@@ -5,0 +66,4 @@{oidc && local && <Divider>or</Divider>}{local && (<form onSubmit={onSubmit} className="flex flex-col gap-3" noValidate>🟡 noValidate disables required/minLength guards; empty/invalid input is sent to the server with no client-side check
error-handling · flagged by 2 models
web/src/pages/LoginPage.tsx:69andweb/src/pages/RegisterPage.tsx:55(register form) —noValidatesilently disables the declaredrequired/minLengthguards. Both forms setnoValidatebut perform no client-side validation beforemutateAsync(...), so empty email/displayName and sub-8-char passwords are submitted straight to the network. The server does reject them (shown viaerrorMessage), so it isn't a crash, but therequired/minLength={8}attributes are decorative. Eithe…🪰 Gadfly · advisory
@@ -5,0 +111,4 @@)}function Divider({ children }: { children: ReactNode }) {⚪ Divider is a generic UI primitive but lives unexported in a page file, breaking the components/ui/ pattern used by the rest of the PR*
maintainability · flagged by 1 model
web/src/pages/LoginPage.tsx:114—Divideris a generic UI primitive parked in a page file. It's a stateless, reusable "or" divider with no login-specific logic, yet it lives insideLoginPage.tsxand is not exported. The rest of the PR deliberately promotes shared visuals tocomponents/ui/*(Alert,Button,TextField) andcomponents/auth/*(AuthCard).Dividerbreaks that pattern for no reason. Suggest moving it tocomponents/ui/Divider.tsx(or folding intoAuthCardif…🪰 Gadfly · advisory
@@ -5,0 +27,4 @@}// Local registration is only meaningful when local auth is enabled.if (providers.isSuccess && !providers.data.local) {🔴 RegisterPage renders form while providers query is pending or errored
correctness, error-handling, maintainability · flagged by 4 models
web/src/pages/RegisterPage.tsx:30— The page immediately falls through to render the full registration form whenprovidersis still loading (isPending) or has errored (isError). UnlikeLoginPage, there is no loading spinner or error alert for the providers query, so the form is interactive before the app even knows whether local registration is enabled. Fix: Add early returns forproviders.isPendingandproviders.isError, mirroring the guard pattern in `LoginPage.tsx:57-5…🪰 Gadfly · advisory
@@ -5,0 +52,4 @@</Link></div>) : (<form onSubmit={onSubmit} className="flex flex-col gap-3" noValidate>🟡 Form with noValidate lacks client-side validation, allowing empty submissions
error-handling · flagged by 1 model
web/src/pages/LoginPage.tsx:69andweb/src/pages/RegisterPage.tsx:55— Both<form>elements setnoValidate, yet there is no compensating client-side validation. Empty strings or a password shorter than 8 chars are submitted to the server unnecessarily. Fix: Add minimal validation (e.g.,email.trim()andpassword.length >= MIN_PASSWORD) before callingmutateAsync, or removenoValidateif native browser validation is acceptable.🪰 Gadfly · advisory
@@ -15,0 +22,4 @@// requireAuth: resolve the current user (shared cache with useMe); send anyone// unauthenticated to /login, remembering where they were headed.async function requireAuth(context: RouterContext, href: string) {🔴 Redirect-after-login broken because location.href (full URL) is passed but safeRedirect expects a path
error-handling · flagged by 2 models
web/src/router.tsx:25-30/:33-38— a non-401 error inensureQueryDatathrows insidebeforeLoadwith noerrorComponentconfigured.meQueryOptions.queryFnrethrows anything that isn't a 401 (e.g. a network failure,status: 0). The query defaultretry(3) softens transient blips, but a persistent outage on/gardensmakesrequireAuththrow a real (non-redirect) error inbeforeLoad.rootRouteis created with only{ component: AppShell }— noerrorComponent/`notFound…🪰 Gadfly · advisory
@@ -15,0 +37,4 @@}}function safeInternalPath(dest: string | undefined): string {🟠 Duplicate redirect sanitization logic with LoginPage.tsx
maintainability · flagged by 2 models
web/src/pages/LoginPage.tsx:24andweb/src/router.tsx:40—safeRedirectandsafeInternalPathare identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g.lib/url.tsorlib/auth.ts) and import it in both files.🪰 Gadfly · advisory
@@ -29,0 +79,4 @@const gardensRoute = createRoute({getParentRoute: () => rootRoute,path: 'gardens',beforeLoad: ({ context, location }) => requireAuth(context, location.href),🟠 requireAuth passes full href to path-only redirect sanitizer
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 23 findings (7 with multi-model agreement)
web/src/components/layout/AppShell.tsx:23web/src/pages/RegisterPage.tsx:30web/src/pages/LoginPage.tsx:24web/src/lib/auth.ts:34web/src/router.tsx:25web/src/router.tsx:40web/src/pages/LoginPage.tsx:6916 single-model findings (lower confidence)
web/src/components/ui/TextField.tsx:14web/src/lib/auth.ts:39web/src/pages/LoginPage.tsx:60web/src/router.tsx:82RegisterPage.tsx:22router.tsx:26router.tsx:62web/src/components/layout/AppShell.tsx:18web/src/lib/auth.ts:89web/src/lib/auth.ts:99web/src/lib/auth.ts:109web/src/pages/LoginPage.tsx:11web/src/pages/LoginPage.tsx:37web/src/pages/LoginPage.tsx:42web/src/pages/RegisterPage.tsx:55web/src/pages/LoginPage.tsx:114Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No credential logging or client-side storage of secrets. That closes out the security lens investigation.
VERDICT: No material issues found
Reviewed the auth data layer (
lib/auth.ts), the fetch wrapper (lib/api.ts), the router guards (router.tsx), and the login/register pages through a security lens. Key things I verified rather than assumed:safeRedirect/safeInternalPath(web/src/pages/LoginPage.tsx:26,web/src/router.tsx) reject anything not starting with/, and explicitly reject//(protocol-relative bypass). Verified the redirect destination only ever flows into TanStack Router'snavigate({to})/redirect({to})— never intowindow.locationor a raw<a href>— so even a scheme-like string can't cause an actual cross-origin navigation.internal/api/auth.go:131(csrfGuard, Origin-header check) plusSameSite=Lax+HttpOnly+ conditionally-Securecookies (internal/api/auth.go:186-205), and the OIDC state cookie carries its own CSRF-state check (internal/api/oidc.go:167). The newfetchwrapper (web/src/lib/api.ts:96) usescredentials: 'same-origin', so it won't leak the session cookie cross-origin.user.displayName,providers.data.oidcLabel,errorMessage(err), the?error=callback text) are rendered as JSX children, never viadangerouslySetInnerHTML. The?error=code from the OIDC callback is used only as a lookup key into a fixedcallbackErrorsmap (LoginPage.tsx:12) — the raw query value is never reflected into the DOM, confirmed the server only ever sets one of a fixed enum of codes (internal/api/oidc.go:115-118).meQueryOptions(lib/auth.ts) only swallows401intonull; any other error (network failure, 5xx) propagates andensureQueryDatainrequireAuth/requireGuestthrows rather than treating the failure as "authenticated" — i.e., it fails closed, not open.internal/api/auth.go:29,binding:"required,min=8,max=1024"), so the client'sminLength={8}isn't a bypassable control.useLogoutclears themecache and purges all non-authquery cache (lib/auth.ts), preventing a shared-device scenario where the next signed-in user could see stale cached data from the previous session.I considered whether the client-side prefix check for redirects could be defeated by control-character tricks (e.g. a tab between slashes that some URL parsers normalize away), which is a known weakness in naive
startsWith('/')guards — but since the destination is only ever consumed by the router's internalnavigate/redirect(not assigned tolocation.hrefor an anchor), I could not establish an actual exploitation path, and didn't havenode_modulesavailable to inspect TanStack Router's exactto-resolution internals to rule it in or out. Flagging as considered-but-not-confirmed rather than reporting it as a finding.🎯 Correctness — No material issues found
Verified Review
VERDICT: No material issues found
The one flagged finding does not survive verification.
requireGuest'sthrow redirect({ to: redirectTo })atweb/src/router.tsx:36omits asearchfield — and TanStack Router does not auto-forward the previous location's search params on aredirect/navigateunless explicitly passed (this codebase's own pattern confirms it:requireAuthatrouter.tsx:28explicitly setssearch: { redirect: href }when it wants to forward a param, whilerequireGuestand the/redirect deliberately don't). So for the crafted case/login?redirect=%2Flogin:requireGuestcomputesredirectTo = '/login'and throwsredirect({ to: '/login' })with no search — the resulting navigation lands on plain/loginwith noredirectparam.beforeLoadreruns,search.redirectis nowundefined,safeInternalPath(undefined)falls back to/gardens, and since the user is still authenticated,requireGuestredirects them to/gardens. That's a single harmless extra bounce through/login, not the infinite loop / browser-hang the draft describes — the router self-heals on the second pass. The draft's core impact claim is incorrect, so the finding is dropped.Everything else in the draft (schema/tag parity,
REGISTRATION_CLOSEDcode match, OIDC callback's hardcoded/gardensredirect,meQueryOptions/useLogoutcache wiring, and the client-side password minimum being a UX-only stricter-than-server constraint) was independently spot-checked and holds up.🧹 Code cleanliness & maintainability — Minor issues
Confirmed: both
router.tsx:40(safeInternalPath) andLoginPage.tsx:24(safeRedirect) have byte-identical bodies and same-origin logic, just different names, and no shared import between them. The finding holds.VERDICT: Minor issues
web/src/pages/LoginPage.tsx:24(safeRedirect) duplicatesweb/src/router.tsx:40(safeInternalPath) — identical body (dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens') implementing the same same-origin redirect check under two different names, with no shared import.router.tsx's copy guardsbeforeLoad(viarequireGuest), whileLoginPage.tsx's copy guards the post-loginnavigatedestination. A future change to the redirect-safety rule (tightening or loosening the same-origin check) would need to be applied in both places, and there's nothing forcing that — a fix to one silently leaves the other stale/inconsistent. Should be a single shared helper (e.g. exported fromlib/auth.ts, which already owns auth/redirect concerns and is imported by both files).⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed this PR strictly through the performance lens (auth data layer, query client config, router guards, and the new auth pages/components).
What I checked:
web/src/lib/auth.ts—meQueryOptions(staleTime 60s) andprovidersQueryOptions(staleTimeInfinity) are shared singletons keyed byqueryOptions(), souseMe(), the router guards, and the mutations'onSuccessall hit the same cache entry — no duplicate fetching introduced.web/src/router.tsx—requireAuth/requireGuestcallensureQueryData(meQueryOptions)inbeforeLoad; because of the 60s staleTime this is a cache hit on repeat navigations within the window, not a fetch-per-route-change.useLogout'sqc.removeQueries({ predicate: ... })(web/src/lib/auth.ts) is O(number of cached queries), but it only runs once per logout, not a hot path — fine at this scale.web/src/lib/queryClient.ts— singleQueryClientinstance shared between the router context and React tree (confirmed viamain.tsxnow importing it instead of constructing a second one), so no duplicate client/cache.web/src/lib/api.ts— default fetch wrapper, no added retry/backoff changes; 401 on/auth/meis caught insidemeQueryOptionsand resolved tonullrather than thrown, so it won't trigger react-query's default retry loop.TextField,Button,Alert,AuthCard) are simple, no loops, no unbounded lists, no unmemoized expensive computation.Nothing here shows N+1 requests, unbounded growth, blocking hot-path calls, or quadratic behavior. This is a small, standard React Query-based auth UI; scope doesn't offer much surface for performance regressions, and I didn't find any.
🧯 Error handling & edge cases — Minor issues
All three findings check out exactly against the current source. The line references, code paths, and reasoning all match:
meQueryOptions(auth.ts:29-38) only special-cases 401 and rethrows everything else (including network failures per api.ts:103-106 and zod parse errors), which then flows uncaught throughensureQueryDatainrequireAuth/requireGuest(router.tsx:26,34) with no error boundary anywhere inweb/src(grep confirmed empty).AppShell.tsx:23-26'sonLogouthas no try/catch, unlike Login/RegisterPage's submit handlers which do.RegisterPage.tsx(confirmed noisPending/isErrorchecks anywhere in the file) falls through to the local form on aprovidersfetch failure, unlikeLoginPage.tsx:57-58which explicitly handles both states.Review — 🧯 Error handling & edge cases
VERDICT: Minor issues
web/src/lib/auth.ts:29-38+web/src/router.tsx:25-38—meQueryOptionsonly special-cases HTTP 401 as "logged out"; every other failure mode is rethrown unmodified: a network outage (api.ts:103-106throwsApiError(..., status: 0, ...)for fetch failures), a 5xx from the server, or a response that failsuserSchema.parse(aZodError, not anApiError, so theerr instanceof ApiErrorguard doesn't catch it) all propagate straight out of the query function. That rejection flows uncaught throughcontext.queryClient.ensureQueryData(meQueryOptions)in bothrequireAuthandrequireGuest(router.tsx:26,34), which have no try/catch around it. There is noerrorComponent/ErrorBoundaryanywhere in the app (confirmed via repo-wide grep), so any transient failure of/auth/mebreaks navigation into every guarded route (/gardens,/gardens/$gardenId,/plants, and the guest-guarded/login//register) instead of degrading gracefully.web/src/components/layout/AppShell.tsx:23-26—onLogoutawaitslogout.mutateAsync()with no try/catch, unlikeLoginPage.tsxandRegisterPage.tsx, which both wrap theirmutateAsynccalls. If/auth/logoutfails (network drop or 5xx — sameApiErrorpath as above), the rejection becomes an unhandled promise rejection inside the click handler:navigatenever runs,logout.isPendingresets tofalseso the button silently re-enables, and there is noAlert/message shown anywhere — the user is left looking "signed in" with no indication the sign-out failed.web/src/pages/RegisterPage.tsx— unlikeLoginPage.tsx:57-58(which rendersproviders.isPending/providers.isErrorstates),RegisterPagenever checksproviders.isPendingorproviders.isError. If the/auth/providersfetch fails, theproviders.isSuccess && !providers.data.localguard (line 30) is simply false, so the component falls through to rendering the full local-registration form even on servers configured for SSO-only — an inconsistent, unhandled edge case compared to howLoginPagehandles the same fetch failure.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Through the security lens, I traced the tainted, user-controlled inputs to their sinks:
redirectsearch param — used in two places:safeRedirect(search.redirect)inLoginPageandsafeInternalPath(search.redirect)inrouter.tsx:40-42. Both sinks call TanStack Router'snavigate()/redirect()(SPA-internal navigation), notwindow.location/<a href>. ThestartsWith('/') && !startsWith('//')guard blocks protocol-relative (//evil.com) and absolute-URL (http://...) values. The one gap — a leading backslash (/\evil.com) passes the check — is not exploitable here because the router resolvestoagainst its own route tree and rendersnotFoundrather than performing a cross-origin browser navigation. So no open-redirect vulnerability. (Defense-in-depth: anew URL(dest, location.origin).origin === location.origincheck would be more robust, but it's not required for safety.)errorsearch param — never reflected raw; it's only a lookup key into the fixedcallbackErrorsmap (LoginPage.tsx:11-19), and the rendered string is always a predefined constant. No reflected XSS.err.messageviaerrorMessage,providers.oidcLabel,user.displayName) are rendered as React text children inAlert/spans, so React escapes them. NodangerouslySetInnerHTMLanywhere in the diff.OIDC_LOGIN_URLis a hardcoded same-origin path (/api/v1/auth/oidc/login), not attacker-influenced.location.hrefpassed torequireAuthis TanStack Router'slocation.href(pathname+search+hash, always same-origin-relative), not the browser's full URL.Verified by reading
web/src/lib/api.ts(ApiError contract,credentials: 'same-origin', no raw HTML handling) andweb/src/router.tsx(guard logic + search validation).🎯 Correctness — Minor issues
VERDICT: Minor issues
web/src/pages/RegisterPage.tsx:30— The "local registration disabled" branch only fires onceproviders.isSuccess. While the providers query is still loading (isPending), the conditionproviders.isSuccess && !providers.data.localis false, so the page falls through and renders the full registration form on a server where local auth is actually disabled. A user on an SSO-only server can fill out and submit the form before the providers response arrives, only to be rejected by the server.LoginPagehandles this symmetrically: it gates the local form behindconst local = providers.data?.local ?? false(which defaults tofalseduring load) and renders aproviders.isPending && <p>Loading…</p>state, so nothing actionable appears until the response arrives.RegisterPagehas no such gating and shows the form by default. Suggested fix: render a "Loading…" state (or nothing) untilprovidersresolves, mirroringLoginPage'sproviders.isPendinghandling, so the form never appears on a local-disabled server.🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: all chain targets failed
ollama-cloud/glm-5.2☁️ model returned an empty response
⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed the change through the performance lens, verifying the query/cache strategy against the actual code rather than the diff alone.
meQueryOptions+ensureQueryDataguard sharing one cache (lib/auth.ts:29,router.tsx:26,34):useMe()inAppShelland therequireAuth/requireGuestguards read the same['auth','me']entry, so there is no duplicate/auth/mefetch between the guard resolving and the component rendering. Verified the cache key andqueryOptionsshape match across both call sites.lib/auth.ts:73,82,91):useLogin/useRegisterprimemeviasetQueryData, so the subsequentnavigate({ to: dest })→requireAuth→ensureQueryDatafinds fresh data and does not re-fetch.useLogoutsetsmetonull(fresh), so the followingnavigate({ to: '/login' })→requireGuest→ensureQueryDataresolves tonullwithout another/auth/meround-trip. I confirmedensureQueryDatatreats thenullset bysetQueryDataas present-and-fresh, so this is a genuine save, not a hidden double-fetch.removeQuerieson logout (lib/auth.ts:94): Iterates all cached queries with a predicate, but this runs once per logout (a cold, user-initiated path), not on any hot loop. Not material.providersQueryOptionsstaleTime: Infinity(lib/auth.ts:49): Fetches providers once per session; the logout cleanup intentionally preservesauth-keyed queries, so no refetch storm on the next sign-in. Fine.ensureQueryDataon every protected navigation (router.tsx:26): Bounded by the 60sstaleTime, with react-query deduping concurrent calls against the same key, so there is no N+1 or per-navigation refetch regression.defaultPreload: 'intent'can cause a/auth/mefetch on hover only once the entry is stale, which is the intended preload behavior.No efficiency regressions, unbounded growth, hot-loop work, or duplicate-fetch issues found in this lens.
🧯 Error handling & edge cases — Minor issues
All three findings verified against the actual source.
Finding 1 (AppShell logout): Confirmed.
onLogout(lines 23-26) awaitslogout.mutateAsync()thennavigatewith no try/catch; the header renders only "Sign out" / "Signing out…", nologout.isError/logout.errorUI. Login/Register pages showAlerton error; logout does not. TheuseLogoutonSuccessclears themecache, so on rejectionmestays non-null and the navigate is skipped — confirmed inauth.ts.Finding 2 (noValidate): Confirmed for both forms.
LoginPage.tsx:69hasnoValidatewithrequiredfields and no explicit guard;RegisterPage.tsx:55hasnoValidatewithrequired/minLengthand no explicit guard. The mutationonSubmitcallsmutateAsyncdirectly with raw state.Finding 3 (router error): Confirmed.
rootRoute(router.tsx:21) is created with only{ component: AppShell };requireAuth/requireGuestthrow the raw error (non-redirect) fromensureQueryDatawhenmeQueryOptions.queryFnrethrows a non-401.queryClient.tssets noretryoverride, so the framework default applies.Corrected review
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:23-26— logout failures are swallowed with no user feedback.onLogoutdoesawait logout.mutateAsync()thenawait navigate({ to: '/login' })with notry/catch. If the/auth/logoutrequest fails (e.g. transient network failure →ApiErrorstatus 0, or the server 5xxs),mutateAsyncrejects: the navigate never runs, the rejection becomes an unhandled promise rejection, andonSuccessnever fires so themecache stays non-null — the user appears still logged in. Unlike login/register (which surfacelogin.error/register.errorvia anAlert), logout has no UI forlogout.isError; the button just flips back to "Sign out" silently. Suggest wrapping intry/catchand showinglogout.error(e.g. anAlertin the header or a toast), and only navigating on success.web/src/pages/LoginPage.tsx:69andweb/src/pages/RegisterPage.tsx:55(register form) —noValidatesilently disables the declaredrequired/minLengthguards. Both forms setnoValidatebut perform no client-side validation beforemutateAsync(...), so empty email/displayName and sub-8-char passwords are submitted straight to the network. The server does reject them (shown viaerrorMessage), so it isn't a crash, but therequired/minLength={8}attributes are decorative. Either dropnoValidate(let the browser enforce them) or add an explicit guard before the mutation to avoid pointless failed requests and to give immediate field-level feedback.web/src/router.tsx:25-30/:33-38— a non-401 error inensureQueryDatathrows insidebeforeLoadwith noerrorComponentconfigured.meQueryOptions.queryFnrethrows anything that isn't a 401 (e.g. a network failure,status: 0). The query defaultretry(3) softens transient blips, but a persistent outage on/gardensmakesrequireAuththrow a real (non-redirect) error inbeforeLoad.rootRouteis created with only{ component: AppShell }— noerrorComponent/notFoundComponent— so the user gets TanStack's default raw error screen instead of being sent to login or offered a retry. Consider adding a rooterrorComponent(with a "try again" affordance) or treating a network error in the guard distinctly from "unauthenticated." (VerifiedrootRouteatrouter.tsx:21and the queryClient defaults atweb/src/lib/queryClient.ts:7— noretryoverride, so the framework default retry applies.)kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
I examined the auth surface carefully for open redirects, XSS via URL parameters, injection paths, and insecure defaults. The code holds up:
router.tsx:40/LoginPage.tsx:24—safeInternalPathandsafeRedirectboth restrictredirectto same-origin absolute paths (startsWith('/') && !startsWith('//')). Verified callers:requireAuth(router guard) andLoginPagepost-login navigation. No open redirect.LoginPage.tsx:37— The?error=OIDC callback parameter is mapped through a whitelist (callbackErrors) before rendering. Arbitrary user-supplied values fall back to a safe default string. No XSS via error codes.lib/auth.ts—meQueryOptionsswallows 401 asnull, avoiding query-error surfacing.useLogoutclears themecache and drops non-auth queries so stale data doesn't leak across sessions.lib/api.ts:96— Fetch usescredentials: 'same-origin', appropriate for the embedded-single-binary architecture.Nothing in this diff crosses into a genuine security defect from the frontend side.
🎯 Correctness — No material issues found
No material issues found
I scrutinized the auth layer, routing guards, form validation, cache priming/clearing, redirect handling, and OIDC callback error mapping against the backend contract. The implementation is semantically correct:
meQueryOptionscorrectly treats 401 asnull(unauthenticated state), shared betweenuseMeand the router guard, withensureQueryDatakeeping them in sync.useLogoutatomically nulls themecache and scrubs non-auth queries, preventing cross-user data leakage.useLogin/useRegisterprime themecache from the mutation response, avoiding a redundant fetch.callbackErrorsmaps exactly to the backend OIDC error codes (oidc_unavailable,state,no_email,email_unverified,oidc_conflict,oidc).LOCAL_AUTH_DISABLED/REGISTRATION_CLOSEDcodes are consumed appropriately; the frontend also gates the register form on theprovidersquery so the common case is handled before submission.safeRedirect/safeInternalPathcorrectly restrict redirects to path-absolute URLs; tanstack router’snavigateonly performs in-app routing, so the//check is sufficient for the current architecture.TextFieldgenerates properid/htmlFor/aria-describedbylinkage in all actual call-sites (every usage suppliesname).Nothing in the diff violates domain correctness or introduces a logic bug.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
safeRedirectduplicated in two files —LoginPage.tsx:24definessafeRedirectandrouter.tsx:40defines an almost identicalsafeInternalPath. Keeping two copies of the same origin-check in sync is brittle. Fix: move one shared helper tolib/auth.ts(orlib/api.ts) and import it in both places.Generic API helpers live in the auth module —
errorMessageandapiErrorCode(lib/auth.ts:99-112) are general-purpose utilities that depend only onApiError, not on auth concepts. Leaving them inlib/auth.tscouples unrelated modules to auth when they need error formatting. Fix: move them next toApiErrorinlib/api.ts.requireAuthpasses a fullhrefto a path-only sanitizer —router.tsx:82,89,96passlocation.href(a full URL including origin) intorequireAuth, which stores it insearch.redirect. Later,LoginPage.tsx:36runssafeRedirect(search.redirect), which expects a path and checksdest.startsWith('/'). A fullhttps://…href fails that check and silently falls back to/gardens, so the “remember where they were” feature is broken. The mismatch between what the helper claims to handle (paths) and what the caller supplies (hrefs) is confusing. Fix: passlocation.pathname + location.search(or a dedicated path field) instead oflocation.href, or rename the helper and make it handle both.⚡ Performance — No material issues found
No material issues found
The auth layer is well-designed from a performance standpoint. Shared
queryOptions(meQueryOptions,providersQueryOptions) ensure a single cache entry is read by both route guards (ensureQueryDatainbeforeLoad) and components (useMe,useProviders).staleTimeis set sensibly (60_000for the user query,Infinityfor providers), so protected-route navigation and page switches won't trigger redundant fetches within a session. Logout’sremoveQueriespredicate is bounded and only runs on an infrequent user action. No hot loops, N+1 patterns, unbounded growth, or blocking work on hot paths were introduced.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/components/layout/AppShell.tsx:23-26—onLogoutawaitslogout.mutateAsync()without atry/catch. If the logout request fails (network error, server 5xx), the error becomes an unhandled promise rejection, themecache is never cleared (theuseLogoutonSuccessonly runs on success), and the UI continues to show the user as signed in with zero feedback.Fix: Wrap
logout.mutateAsync()intry/catch; on error, surface it in the UI (e.g., an inline alert) or at least ensure the cache is invalidated so the client doesn’t pretend the session is still valid.web/src/pages/RegisterPage.tsx:30— The page immediately falls through to render the full registration form whenprovidersis still loading (isPending) or has errored (isError). UnlikeLoginPage, there is no loading spinner or error alert for the providers query, so the form is interactive before the app even knows whether local registration is enabled.Fix: Add early returns for
providers.isPendingandproviders.isError, mirroring the guard pattern inLoginPage.tsx:57-58.Minor issues
web/src/components/layout/AppShell.tsx:18-21—useMe()’s loading state is ignored (const user = me.data). On a hard refresh,dataisundefinedwhile the query is pending, so authenticated users see a brief flash of the “Sign in” link and the unauthenticated nav layout.Fix: Check
me.isPendingand render a neutral loading state (e.g., a skeleton or simply omit the auth-specific buttons until resolved).web/src/pages/LoginPage.tsx:69andweb/src/pages/RegisterPage.tsx:55— Both<form>elements setnoValidate, yet there is no compensating client-side validation. Empty strings or a password shorter than 8 chars are submitted to the server unnecessarily.Fix: Add minimal validation (e.g.,
email.trim()andpassword.length >= MIN_PASSWORD) before callingmutateAsync, or removenoValidateif native browser validation is acceptable.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I reviewed the change through the security lens, verifying against the checked-out code (
web/src/router.tsx,web/src/pages/LoginPage.tsx,web/src/lib/auth.ts,web/src/lib/api.ts).requireAuthputslocation.href(TanStack Router's in-app path/search/hash, not a full URL) intosearch.redirect, andLoginPage.safeRedirect(LoginPage.tsx:24-26) plussafeInternalPath(router.tsx:40-42) both reject anything not starting with/and reject//(protocol-relative). Known bypasses like/\evil.comare not exploitable here becausenavigate({ to })performs client-side routing only — TanStack won't issue a document-level navigation to an external URL — so a slipped-through value just produces a broken in-app route, not an open redirect. I traced the full path fromrequireAuth→search.redirect→safeRedirect→navigate.?error=codes.search.erroris only used as a lookup key into the staticcallbackErrorsmap (LoginPage.tsx:14-21, 37); it is never rendered directly, so no reflected XSS. Unknown codes fall back tocallbackErrors.oidc. Verified atLoginPage.tsx:37.<a href="/api/v1/auth/oidc/login">(LoginPage.tsx:11, 60-64) is a same-origin navigation with notarget="_blank", so norel="noopener"concern. TheoidcLabelis rendered as escaped text content.apiFetchusescredentials: 'same-origin'(api.ts:96) — no credentialed cross-origin requests, no CSRF surface introduced by the frontend. SameSite/CSRF on the auth POSTs is a backend concern and not visible here; nothing in this PR weakens it.useLogoutsetsmetonulland removes all non-authcached queries (auth.ts:90-95), so a subsequent user on a shared browser session does not see the previous user's gardens/plants. Verified the predicate keeps['auth','me']/['auth','providers']and drops everything else.userSchema,providersSchema) before use; error bodies are read via narrow casts (apiErrorCode,auth.ts:100-106); nodangerouslySetInnerHTML,eval, template-string SQL, or dynamichreffrom untrusted input anywhere in the diff.No security findings to report for this lens.
🎯 Correctness — Minor issues
Corrected review
VERDICT: Minor issues
redirecttarget —web/src/pages/LoginPage.tsx:60-64. The local form threads the remembered destination (dest = safeRedirect(search.redirect)→navigate({ to: dest })), but the OIDC anchor is a plain<a href={OIDC_LOGIN_URL}>with noredirectquery param forwarded, and the backendoidcLoginhandler reads no redirect param (internal/api/oidc.go:123-150) whileoidcCallback's success path hardcodesc.Redirect(http.StatusFound, "/gardens")(internal/api/oidc.go:255). So a user bounced from/gardens/1→/login?redirect=%2Fgardens%2F1who chooses "Sign in with SSO" lands on/gardens, not/gardens/1— the destination is silently lost. The frontend makes no attempt to thread the target through; fixing it would require backend support (carryingredirectthrough the OIDC tx cookie and honoring it on success). The redirect-roundtrip behavior therefore does not hold for OIDC, which is the primary sign-in path when configured.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/pages/LoginPage.tsx:24andweb/src/router.tsx:40— duplicated redirect-sanitization helper.safeRedirect(LoginPage) andsafeInternalPath(router) are byte-for-byte identical logic (dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens') living in two files under two names. This is exactly the copy-paste the PR's "shared" philosophy is meant to avoid: if the allow-rule tightens, one site will drift. Suggested fix: export one helper (e.g.safeInternalPathfrom@/lib/author a new@/lib/redirect) and haveLoginPageimport it. Verified by reading both files.web/src/pages/LoginPage.tsx:114—Divideris a generic UI primitive parked in a page file. It's a stateless, reusable "or" divider with no login-specific logic, yet it lives insideLoginPage.tsxand is not exported. The rest of the PR deliberately promotes shared visuals tocomponents/ui/*(Alert,Button,TextField) andcomponents/auth/*(AuthCard).Dividerbreaks that pattern for no reason. Suggest moving it tocomponents/ui/Divider.tsx(or folding intoAuthCardif it's meant to be auth-only). Verified by grep —Divideris referenced only inLoginPage.tsx.web/src/lib/auth.ts:109—errorMessagesilently ignores itsfallbackforApiError. The signature reads as "use this fallback message", but the implementation returnserr.messagefor anyApiError, including ones whose server message is generic/technical (e.g.Request failed (NNN)frommessageFrominapi.ts). Callers inLoginPage(line 90) andRegisterPage(line 87) pass tailored fallbacks ('Could not sign in.','Could not create your account.') that never win for anApiError. Not a bug, but the API is misleading vs. its doc-comment. Either rename to clarify ("surfaces the server message if present") or prefer the caller's fallback when the server message is empty/generic. Verified atauth.ts:108-112,api.ts:73-81(messageFrom), and the two call sites.⚡ Performance — No material issues found
I read
web/src/lib/auth.ts,web/src/router.tsx, andweb/src/lib/api.tsto verify behavior. Reviewing through the performance lens only.VERDICT: No material issues found
A few things I explicitly checked and confirmed are fine:
meQueryOptionsis shared byuseMe()(AppShell, mounted on every route) and therequireAuth/requireGuestguards viacontext.queryClient.ensureQueryData. Because both use the same['auth','me']queryKey against the single sharedqueryClient(lib/queryClient.ts), a logged-out user landing on/loginproduces exactly one/auth/meround-trip (401 →null), not one per guard + one peruseMe. The shared client is what makes this work; the diff's separation ofqueryClientinto its own module is the right call.defaultPreload: 'intent'+beforeLoadguard. Hovering a guarded link firesensureQueryData(meQueryOptions). WithstaleTime: 60_000onmeQueryOptions(overriding the 30s default), repeated intent-hovers within a minute do not refetch — no per-hover request storm. The providers query usesstaleTime: Infinity, so it's fetched once per session.useLogoutcache teardown.qc.removeQueries({ predicate: (q) => q.queryKey[0] !== 'auth' })is an O(n) pass over the (small) query cache on logout only — not a hot path — and correctly preserves theme/providersentries while dropping user-scoped data. Not a concern.callbackErrorsis a module-level constant;safeRedirect/safeInternalPathdo trivial string checks; nothing allocates per-render inLoginPage/RegisterPagebeyond React's own state.🧯 Error handling & edge cases — Minor issues
All findings verified. Draft review confirmed.
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:23-26—onLogoutawaitslogout.mutateAsync()with no try/catch. If the logout request fails (network, 5xx), the rejection is unhandled at the onClick handler,navigate({ to: '/login' })never runs, and there's noAlert/feedback inAppShell— the button just re-enables and the user is left on the page with no indication anything went wrong. Verified by reading the file (no error UI in AppShell).web/src/lib/auth.ts:34-37—meQueryOptions.queryFncatchesApiError401 →null, but a zodZodErrorfrom a malformed/auth/mebody (e.g., server returns 200 with unexpected shape) is rethrown. InrequireAuth(router.tsx:26) this surfaces viaensureQueryDataas a routebeforeLoaderror, and there is noerrorComponent/ErrorBoundarydefined anywhere in the route tree (grep confirmed none in the repo). Result: the user lands on TanStack's default raw-error render rather than any friendly handling. Same applies torequireGuest.web/src/pages/LoginPage.tsx:37—callbackErrors[search.error] ?? callbackErrors.oidcfalls back to the genericoidc("Single sign-on did not complete") message for any unknown?error=code.validateSearch(router.tsx:62-67) accepts any string forerror, so an attacker-supplied/typo'd code yields a misleading SSO message even on a non-OIDC flow. Minor UX/edge: better to show a generic fallback or restrict allowed codes. Verified by reading both files.web/src/pages/LoginPage.tsx:42-43&RegisterPage.tsx:22-23—await navigate({ to: dest })after a successful login/register is inside the sametryblock as the mutation. If navigation throws (e.g., redirect target's ownbeforeLoadredirects again, or an invalid path), the rejection is caught by the form'scatch {}and silently swallowed alongside any mutation error display logic; the navigation failure is lost with no UI feedback. Minor, but it's the one path here where a navigation error is silently absorbed. Verified by reading both pages.web/src/lib/auth.ts:89-95—useLogout's cache cleanup (removeQueries) runs inonSuccess, which fires only on a successful logout. If the logout request fails (network/5xx), stale non-auth cache from the previous session remains, and since the cookie may still be valid the user stays logged in but with no cache cleanup. Consistent with "still logged in" semantics, but worth noting the asymmetry: the only cleanup path is gated on the success that, on failure, leaves the cache inconsistent with the (failed) intent. Verified by reading the hook.opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
I examined the auth surface carefully through the security lens:
/login,/register,/logout) are protected by the backendcsrfGuard()middleware that validates theOriginheader againstPANSY_BASE_URL. The logout mutation POSTs to/auth/logoutand is therefore covered.HttpOnly,Secure(when served over HTTPS),SameSite=Lax, and scoped to path/. Correct.HttpOnlycookie, and verified with constant-time comparison in the callback. The callback hardcodes redirect targets (/gardenson success,/login?error=...on failure) — no open redirect.safeRedirect(login page) andsafeInternalPath(router) restrictredirectto same-origin absolute paths (startsWith('/') && !startsWith('//')). Protocol-relative URLs and external origins are blocked.mecache is primed withuserSchema.parse(...)output, so unvalidated server responses cannot poison auth state.The auth implementation is sound from a security perspective.
🎯 Correctness — Minor issues
Minor issues
web/src/components/ui/TextField.tsx:15—inputIdis computed asid ?? name. Both are optional onInputHTMLAttributes, so a caller that omits both (easy to do in a shared UI primitive) yieldsinputId === undefined. The<label htmlFor>and<input id>are then omitted, breaking the programmatic label–input association, and ifhintis presentaria-describedbybecomes"undefined-hint".Fix: fall back to
React.useId()when neitheridnornameis supplied.web/src/lib/auth.ts:39—meQueryOptionssetsstaleTime: 60_000. WithrefetchOnWindowFocus: false(global default inqueryClient.ts), a cachedUserobject is treated as fresh for a full minute. If the session expires, or the user logs out in another tab,requireAuthinbeforeLoadcan still pass the guard and render protected routes using stale authenticated state.Fix: reduce
staleTimeto0for auth queries soensureQueryDataalways verifies the session with the server before deciding access.web/src/components/layout/AppShell.tsx:23-26—onLogoutawaitslogout.mutateAsync()without atry/catch. If the network is down or the server returns an error, the rejection is unhandled and the user receives no feedback (the button simply flips from “Signing out…” back to “Sign out” with no alert).Fix: wrap the call in
try/catchand surfacelogout.error(e.g. with an<Alert>) or switch tologout.mutatewith anonErrorcallback.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/pages/LoginPage.tsx:24andweb/src/router.tsx:40—safeRedirectandsafeInternalPathare identical redirect-sanitization functions. Keeping two copies means any future change (e.g., adding an allow-list of paths, or changing the default fallback) has to be made in both places. Extract one shared utility (e.g.lib/url.tsorlib/auth.ts) and import it in both files.web/src/pages/LoginPage.tsx:11—OIDC_LOGIN_URLhardcodes/api/v1, duplicating the base path already defined inlib/api.ts. If the API base ever changes, this link will break silently. ExportBASEfromlib/api.ts(or a derived helper) and construct the OIDC URL from it so the prefix lives in one place.web/src/pages/RegisterPage.tsx:30—RegisterPageonly checksproviders.isSuccessbefore deciding whether to show the SSO-only message, whileLoginPageexplicitly renders loading and error states for the same query (providers.isPending,providers.isError). This pattern divergence makes the auth surface harder to maintain; future changes to provider-state UX need to be remembered for both pages. Mirror the explicit pending/error handling fromLoginPageinRegisterPage.⚡ Performance — No material issues found
No material issues found
Through the performance lens, this diff is clean:
QueryClient(queryClient.ts) eliminates duplicate caches between the React tree and router context.ensureQueryDatain thebeforeLoadguards (router.tsx:26,34) only triggers a fetch when themecache is missing or stale (staleTime: 60_000inmeQueryOptions), so repeated navigation within that window is instant and non-blocking.providersQueryOptionscorrectly usesstaleTime: Infinitybecause auth methods are session-static.removeQueriespredicate (auth.ts:99) is scoped and appropriate—prevents cross-user data leakage without perf harm.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
Broken redirect-after-login (
router.tsx:28,router.tsx:82/89/96):requireAuthpasseslocation.href(a full URL, e.g.https://example.com/gardens/1) into the loginredirectsearch param.LoginPage.safeRedirect(LoginPage.tsx:24) only allows values starting with/, so the destination is always coerced to/gardens. Users are never returned to the protected page they originally requested. Fix: passlocation.pathname + location.searchinstead oflocation.hrefin the threerequireAuthcall sites.Unhandled logout failure (
AppShell.tsx:23):onLogoutawaitslogout.mutateAsync()without atry/catch. If the logout request fails (network error, server 5xx), the rejected promise becomes an unhandled rejection and navigation to/loginis skipped. Contrast withLoginPageandRegisterPage, which correctly wrapmutateAsyncintry/catch. Fix: wrap the mutation call intry { … } catch { /* mutation error is already surfaced by React Query */ }.TextField generates broken
aria-describedbywhenid/nameomitted (TextField.tsx:14):hintIdis built as`${inputId}-hint`. If bothidandnameare omitted (possible because neither is required inTextFieldProps),inputIdisundefinedandhintIdbecomes the literal string"undefined-hint", producing a danglingaria-describedby. Fix: generate a fallback id whenid ?? nameis missing (e.g.useId()or require one of them).Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.