Auth UI: login/register pages, OIDC button, route guard (#6) #25

Merged
steve merged 2 commits from phase-1-auth-ui into main 2026-07-18 22:11:21 +00:00
Owner

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.tsxcreateRootRouteWithContext 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

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)
steve added 1 commit 2026-07-18 21:50:15 +00:00
Add auth UI: login/register pages, OIDC button, route guard (#6)
Build image / build-and-push (push) Successful in 7s
Gadfly review (reusable) / review (pull_request) Successful in 9m4s
Adversarial Review (Gadfly) / review (pull_request) Successful in 9m4s
622010cd71
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

🪰 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

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>
gitea-actions bot reviewed 2026-07-18 21:59:20 +00:00
gitea-actions bot left a comment

🪰 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>
@@ -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-21useMe()’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>
@@ -17,0 +20,4 @@
const navigate = useNavigate()
const user = me.data
async 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-26onLogout 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…

🪰 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>
@@ -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:15inputId 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>
@@ -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-37meQueryOptions.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>
@@ -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:39meQueryOptions 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>
@@ -0,0 +86,4 @@
export function useLogout() {
const qc = useQueryClient()
return useMutation({
mutationFn: () => api.post('/auth/logout'),

🟡 web/src/lib/auth.ts:89-95

error-handling · flagged by 1 model

  • web/src/lib/auth.ts:89-95useLogout'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>
@@ -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

🟡 **Generic API error helpers misplaced in auth module** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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:109errorMessage 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…

🪰 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>
@@ -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:11OIDC_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>
@@ -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:24 and web/src/router.tsx:40safeRedirect 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>
@@ -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:37

error-handling · flagged by 1 model

  • web/src/pages/LoginPage.tsx:37callbackErrors[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>
@@ -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-23

error-handling · flagged by 1 model

  • web/src/pages/LoginPage.tsx:42-43 & RegisterPage.tsx:22-23await 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>
@@ -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

  • OIDC login drops the redirect targetweb/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>
@@ -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: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>
@@ -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:114Divider 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>
@@ -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 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>
@@ -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: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>
@@ -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 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>
@@ -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:24 and web/src/router.tsx:40safeRedirect 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>
@@ -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

🟠 **requireAuth passes full href to path-only redirect sanitizer** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>

🪰 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
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 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
Per-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:

  • 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.tsmeQueryOptions (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.tsxrequireAuth/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-38meQueryOptions 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-26onLogout 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.

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:

  • 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 })requireAuthensureQueryData finds fresh data and does not re-fetch. useLogout sets me to null (fresh), so the following navigate({ to: '/login' })requireGuestensureQueryData 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.

🧯 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 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.)

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:24safeInternalPath 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.tsmeQueryOptions 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 filesLoginPage.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 moduleerrorMessage 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 sanitizerrouter.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-26onLogout 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-21useMe()’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.

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).

  • 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 requireAuthsearch.redirectsafeRedirectnavigate.
  • 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 targetweb/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:114Divider 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:109errorMessage 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.

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-26onLogout 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-37meQueryOptions.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:37callbackErrors[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-23await 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-95useLogout'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

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:

  • 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:15inputId 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:39meQueryOptions 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-26onLogout 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:40safeRedirect 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:11OIDC_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:30RegisterPage 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>
steve added 1 commit 2026-07-18 22:10:02 +00:00
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 main 2026-07-18 22:11:21 +00:00
steve deleted branch phase-1-auth-ui 2026-07-18 22:11:21 +00:00
Sign in to join this conversation.