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
15 changed files with 636 additions and 39 deletions
+17
View File
@@ -0,0 +1,17 @@
import { useRouter } from '@tanstack/react-router'
import { Button } from '@/components/ui/Button'
import { errorMessage } from '@/lib/api'
// Root error boundary: if a route's beforeLoad/loader throws something other
// than a redirect (e.g. /auth/me fails with a network error or a 500), show a
// recoverable message instead of a blank screen. Retrying re-runs the guards.
export function RouteError({ error }: { error: Error }) {
const router = useRouter()
return (
<div className="mx-auto flex min-h-[60vh] w-full max-w-sm flex-col items-center justify-center gap-4 text-center">
<h1 className="text-lg font-semibold text-fg">Something went wrong</h1>
<p className="text-sm text-muted">{errorMessage(error, 'An unexpected error occurred.')}</p>
<Button onClick={() => router.invalidate()}>Try again</Button>
</div>
)
}
+22
View File
@@ -0,0 +1,22 @@
import type { ReactNode } from 'react'
/** Centered card layout shared by the login and register pages. */
export function AuthCard({
title,
subtitle,
children,
}: {
title: string
subtitle?: string
children: ReactNode
}) {
return (
<div className="mx-auto flex min-h-[70vh] w-full max-w-sm flex-col justify-center">
<div className="rounded-xl border border-border bg-surface p-6 shadow-sm">
<h1 className="text-xl font-semibold tracking-tight text-fg">{title}</h1>
{subtitle && <p className="mt-1 text-sm text-muted">{subtitle}</p>}
<div className="mt-5">{children}</div>
</div>
</div>
)
}
+53 -18
View File
@@ -1,4 +1,5 @@
import { Link, Outlet } from '@tanstack/react-router'
import { Link, Outlet, useNavigate } from '@tanstack/react-router'
import { useLogout, useMe } from '@/lib/auth'
const navLinks = [
{ to: '/gardens', label: 'Gardens' },
@@ -14,6 +15,22 @@ 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()
Review

🟡 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>
const logout = useLogout()
const navigate = useNavigate()
const user = me.data
async function onLogout() {
Review

🔴 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>
try {
await logout.mutateAsync()
await navigate({ to: '/login' })
} catch {
// The logout request failed, so the session is still valid server-side:
// leave the user where they are (the button re-enables for a retry) rather
// than pretending they're signed out. logout.isError drives the title below.
}
}
return (
<div className="flex min-h-full flex-col">
<header className="sticky top-0 z-10 border-b border-border bg-surface/90 backdrop-blur">
@@ -21,25 +38,43 @@ export function AppShell() {
<Link to="/gardens" className="text-lg font-semibold text-accent-strong">
🌱 pansy
</Link>
<div className="flex flex-1 items-center gap-1">
{navLinks.map((l) => (
<Link
key={l.to}
to={l.to}
className={navLinkBase}
activeProps={{ className: navLinkActive }}
inactiveProps={{ className: navLinkInactive }}
>
{l.label}
</Link>
))}
{user &&
navLinks.map((l) => (
<Link
key={l.to}
to={l.to}
className={navLinkBase}
activeProps={{ className: navLinkActive }}
inactiveProps={{ className: navLinkInactive }}
>
{l.label}
</Link>
))}
</div>
<Link
to="/login"
className="rounded-md px-3 py-1.5 text-sm font-medium text-muted transition-colors hover:text-fg"
>
Sign in
</Link>
{user ? (
<div className="flex items-center gap-2">
<span className="hidden text-sm text-muted sm:inline">{user.displayName}</span>
<button
type="button"
onClick={onLogout}
disabled={logout.isPending}
title={logout.isError ? 'Sign out failed — try again' : undefined}
className="rounded-md px-3 py-1.5 text-sm font-medium text-muted transition-colors hover:text-fg disabled:opacity-60"
>
{logout.isPending ? 'Signing out…' : logout.isError ? 'Retry sign out' : 'Sign out'}
</button>
</div>
) : (
<Link
to="/login"
className="rounded-md px-3 py-1.5 text-sm font-medium text-muted transition-colors hover:text-fg"
>
Sign in
</Link>
)}
</nav>
</header>
+20
View File
@@ -0,0 +1,20 @@
import type { ReactNode } from 'react'
import { cn } from '@/lib/cn'
type AlertTone = 'error' | 'info'
/** A small inline notice for form errors and status messages. */
export function Alert({ tone = 'error', children }: { tone?: AlertTone; children: ReactNode }) {
return (
<div
role={tone === 'error' ? 'alert' : 'status'}
className={cn(
'rounded-md border px-3 py-2 text-sm',
tone === 'error' && 'border-red-500/40 bg-red-500/10 text-red-700 dark:text-red-300',
tone === 'info' && 'border-border bg-border/30 text-muted',
)}
>
{children}
</div>
)
}
+24
View File
@@ -0,0 +1,24 @@
import type { ButtonHTMLAttributes } from 'react'
import { cn } from '@/lib/cn'
export type ButtonVariant = 'primary' | 'ghost'
/** Shared button styling, exported so anchor "buttons" (e.g. the OIDC link) match. */
export function buttonClasses(variant: ButtonVariant = 'primary', className?: string) {
return cn(
'inline-flex items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors',
'outline-none focus-visible:ring-2 focus-visible:ring-accent/40',
'disabled:cursor-not-allowed disabled:opacity-60',
variant === 'primary' && 'bg-accent text-accent-contrast hover:bg-accent-strong',
variant === 'ghost' && 'border border-border text-fg hover:bg-border/50',
className,
)
}
interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
variant?: ButtonVariant
}
export function Button({ variant = 'primary', className, type = 'button', ...props }: ButtonProps) {
return <button type={type} className={buttonClasses(variant, className)} {...props} />
}
+12
View File
@@ -0,0 +1,12 @@
import type { ReactNode } from 'react'
/** A horizontal rule with centered label text (e.g. "or" between auth options). */
export function Divider({ children }: { children: ReactNode }) {
return (
<div className="flex items-center gap-3 text-xs uppercase tracking-wide text-muted">
<span className="h-px flex-1 bg-border" />
{children}
<span className="h-px flex-1 bg-border" />
</div>
)
}
+45
View File
@@ -0,0 +1,45 @@
import { forwardRef, useId, type InputHTMLAttributes } from 'react'
import { cn } from '@/lib/cn'
interface TextFieldProps extends InputHTMLAttributes<HTMLInputElement> {
label: string
hint?: string
}
// A labelled text input. text-base (16px) is deliberate: smaller fonts make iOS
// Safari zoom on focus. The field id falls back to the name, then to a generated
// id, so the label/hint associations always hold.
export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(function TextField(
{ label, hint, id, name, className, ...props },
ref,
Review

🟠 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>
) {
const generatedId = useId()
const inputId = id ?? name ?? generatedId
const hintId = hint ? `${inputId}-hint` : undefined
return (
<div className="flex flex-col gap-1.5">
<label htmlFor={inputId} className="text-sm font-medium text-fg">
{label}
</label>
<input
ref={ref}
id={inputId}
name={name}
aria-describedby={hintId}
className={cn(
'w-full rounded-md border border-border bg-surface px-3 py-2 text-base text-fg',
'placeholder:text-muted/70 outline-none transition-colors',
'focus:border-accent focus:ring-2 focus:ring-accent/30',
'disabled:opacity-60',
className,
)}
{...props}
/>
{hint && (
<p id={hintId} className="text-xs text-muted">
{hint}
</p>
)}
</div>
)
})
+22 -2
View File
@@ -5,7 +5,8 @@
// editor (per DESIGN.md § Sync) sends each PATCH/DELETE with a row `version` and,
// on a 409, reads the *current* row back out of `ApiError.body` to reconcile.
const BASE = '/api/v1'
/** Base path for the versioned JSON API; also used to build server-nav links. */
export const API_BASE = '/api/v1'
/** Error thrown for any non-2xx response (or a network failure, status 0). */
export class ApiError extends Error {
@@ -44,7 +45,7 @@ export interface RequestOptions {
}
function buildUrl(path: string, params?: Params): string {
const url = BASE + path
const url = API_BASE + path
if (!params) return url
const qs = new URLSearchParams()
for (const [k, v] of Object.entries(params)) {
@@ -112,6 +113,25 @@ export async function apiFetch<T>(path: string, opts: RequestOptions = {}): Prom
return payload as T
}
/** The server error code from an ApiError body ({error:{code,message}}), if any. */
export function apiErrorCode(err: unknown): string | null {
if (err instanceof ApiError && err.body && typeof err.body === 'object') {
const e = (err.body as { error?: { code?: unknown } }).error
if (e && typeof e.code === 'string') return e.code
}
return null
}
/**
* A user-facing message for a caught request error. An ApiError carries the
* server's own message (already user-friendly), so that wins; `fallback` covers
* everything else (unexpected throws).
*/
export function errorMessage(err: unknown, fallback = 'Something went wrong.'): string {
if (err instanceof ApiError) return err.message
return fallback
}
/** Convenience verbs over apiFetch. */
export const api = {
get: <T>(path: string, opts?: Omit<RequestOptions, 'method' | 'body'>) =>
+97
View File
@@ -0,0 +1,97 @@
// Auth data layer: zod-validated shapes for the /api/v1/auth/* endpoints plus
// the react-query hooks and shared query options the router guard reuses.
import { queryOptions, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
import { z } from 'zod'
import { ApiError, api } from './api'
export const userSchema = z.object({
id: z.number(),
email: z.string(),
displayName: z.string(),
isAdmin: z.boolean(),
version: z.number(),
createdAt: z.string(),
updatedAt: z.string(),
})
export type User = z.infer<typeof userSchema>
export const providersSchema = z.object({
local: z.boolean(),
oidc: z.boolean(),
oidcLabel: z.string(),
})
export type Providers = z.infer<typeof providersSchema>
// meQueryOptions is shared by useMe and the router's beforeLoad guard so both
// hit one cache entry. A 401 is the unauthenticated state, not an error, so it
// resolves to null rather than throwing (which would surface as a query error).
export const meQueryOptions = queryOptions({
queryKey: ['auth', 'me'] as const,
queryFn: async (): Promise<User | null> => {
try {
return userSchema.parse(await api.get('/auth/me'))
} catch (err) {
Review

🔴 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>
if (err instanceof ApiError && err.isUnauthorized) return null
throw err
}
},
staleTime: 60_000,
Review

🟠 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>
})
export function useMe() {
return useQuery(meQueryOptions)
}
export const providersQueryOptions = queryOptions({
queryKey: ['auth', 'providers'] as const,
queryFn: async (): Promise<Providers> => providersSchema.parse(await api.get('/auth/providers')),
staleTime: Infinity, // the enabled methods don't change within a session
})
export function useProviders() {
return useQuery(providersQueryOptions)
}
export interface RegisterInput {
email: string
displayName: string
password: string
}
export interface LoginInput {
email: string
password: string
}
export function useRegister() {
const qc = useQueryClient()
return useMutation({
mutationFn: async (input: RegisterInput): Promise<User> =>
userSchema.parse(await api.post('/auth/register', input)),
// Registration auto-logs-in (the server sets the cookie), so prime the me
// cache with the returned user instead of a refetch.
onSuccess: (user) => qc.setQueryData(meQueryOptions.queryKey, user),
})
}
export function useLogin() {
const qc = useQueryClient()
return useMutation({
mutationFn: async (input: LoginInput): Promise<User> =>
userSchema.parse(await api.post('/auth/login', input)),
onSuccess: (user) => qc.setQueryData(meQueryOptions.queryKey, user),
})
}
export function useLogout() {
const qc = useQueryClient()
return useMutation({
mutationFn: () => api.post('/auth/logout'),
Review

🟡 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>
onSuccess: () => {
qc.setQueryData(meQueryOptions.queryKey, null)
// Drop any other cached data so the next user never sees the previous
// user's gardens/plants. The me/providers entries are re-fetched as needed.
qc.removeQueries({ predicate: (q) => q.queryKey[0] !== 'auth' })
},
})
}
+10
View File
@@ -0,0 +1,10 @@
import { QueryClient } from '@tanstack/react-query'
// A single QueryClient shared by the React tree (QueryClientProvider) and the
// router (as beforeLoad context), so route guards and components read the same
// cache — the auth guard's ensureQueryData populates exactly what useMe reads.
export const queryClient = new QueryClient({
defaultOptions: {
queries: { refetchOnWindowFocus: false, staleTime: 30_000 },
},
})
+14
View File
@@ -0,0 +1,14 @@
// Where to land after auth. The router guard stashes the intended destination
// in ?redirect=, and the login page reads it back; both go through this so a
// caller-controlled value can never send the user to an external origin.
const DEFAULT_DESTINATION = '/gardens'
/**
* Return dest only if it is a same-origin absolute path (starts with a single
* '/'); otherwise fall back to the gardens list. Rejects protocol-relative
* ('//evil.com') and absolute ('https://…') URLs.
*/
export function safeRedirectPath(dest: string | undefined, fallback = DEFAULT_DESTINATION): string {
return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : fallback
}
+2 -7
View File
@@ -1,16 +1,11 @@
import { StrictMode } from 'react'
import { createRoot } from 'react-dom/client'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
import { QueryClientProvider } from '@tanstack/react-query'
import { RouterProvider } from '@tanstack/react-router'
import { router } from './router'
import { queryClient } from './lib/queryClient'
import './styles/index.css'
const queryClient = new QueryClient({
defaultOptions: {
queries: { refetchOnWindowFocus: false, staleTime: 30_000 },
},
})
const rootEl = document.getElementById('root')
if (!rootEl) throw new Error('Missing #root element')
+107 -2
View File
@@ -1,5 +1,110 @@
import { PageStub } from '@/components/PageStub'
import { useState, type FormEvent } from 'react'
import { Link, useNavigate, useSearch } from '@tanstack/react-router'
import { AuthCard } from '@/components/auth/AuthCard'
import { Alert } from '@/components/ui/Alert'
import { Button, buttonClasses } from '@/components/ui/Button'
import { Divider } from '@/components/ui/Divider'
import { TextField } from '@/components/ui/TextField'
import { API_BASE, errorMessage } from '@/lib/api'
import { useLogin, useProviders } from '@/lib/auth'
import { safeRedirectPath } from '@/lib/redirect'
Review

🟡 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>
// Server-initiated redirect (not a fetch): the browser navigates to the Go
// handler, which 302s to the IdP.
const OIDC_LOGIN_URL = `${API_BASE}/auth/oidc/login`
// Messages for the ?error= codes the OIDC callback redirects back with.
const callbackErrors: Record<string, string> = {
oidc: 'Single sign-on did not complete. Please try again.',
oidc_unavailable: 'The single sign-on provider is currently unavailable.',
state: 'Your sign-in attempt expired. Please start again.',
no_email: 'Your identity provider did not share an email address, which pansy needs.',
email_unverified: 'Your email address is not verified with the identity provider.',
oidc_conflict: 'That single sign-on identity conflicts with an existing account.',
}
Review

🟠 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>
export function LoginPage() {
return <PageStub title="Sign in">Login form and SSO button arrive in issue #6.</PageStub>
const search = useSearch({ from: '/login' })
const navigate = useNavigate()
const providers = useProviders()
const login = useLogin()
const [email, setEmail] = useState('')
const [password, setPassword] = useState('')
const dest = safeRedirectPath(search.redirect)
const callbackError = search.error ? (callbackErrors[search.error] ?? callbackErrors.oidc) : null
async function onSubmit(e: FormEvent) {
Review

🟡 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>
e.preventDefault()
try {
await login.mutateAsync({ email, password })
await navigate({ to: dest })
} catch {
Review

🟡 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>
// Shown below via login.error.
}
}
const local = providers.data?.local ?? false
const oidc = providers.data?.oidc ?? false
return (
<AuthCard title="Sign in to pansy" subtitle="Plan your garden, bed by bed.">
<div className="flex flex-col gap-4">
{callbackError && <Alert>{callbackError}</Alert>}
{providers.isPending && <p className="text-sm text-muted">Loading</p>}
{providers.isError && <Alert>Could not load sign-in options. Please refresh.</Alert>}
{oidc && (
<a href={OIDC_LOGIN_URL} className={buttonClasses('primary', 'w-full')}>
{providers.data?.oidcLabel || 'Sign in with SSO'}
Review

🟠 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>
</a>
)}
{oidc && local && <Divider>or</Divider>}
{local && (
<form onSubmit={onSubmit} className="flex flex-col gap-3">
<TextField
label="Email"
Review

🟡 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>
name="email"
type="email"
autoComplete="email"
autoCapitalize="none"
spellCheck={false}
required
value={email}
onChange={(e) => setEmail(e.target.value)}
/>
<TextField
label="Password"
name="password"
type="password"
autoComplete="current-password"
required
value={password}
onChange={(e) => setPassword(e.target.value)}
/>
{login.isError && <Alert>{errorMessage(login.error, 'Could not sign in.')}</Alert>}
<Button type="submit" disabled={login.isPending} className="mt-1">
{login.isPending ? 'Signing in…' : 'Sign in'}
</Button>
</form>
)}
{providers.isSuccess && !local && !oidc && (
<Alert tone="info">No sign-in methods are configured on this server.</Alert>
)}
{local && (
<p className="text-center text-sm text-muted">
No account?{' '}
<Link to="/register" className="font-medium text-accent-strong hover:underline">
Create one
</Link>
</p>
)}
</div>
</AuthCard>
)
}
+115 -2
View File
@@ -1,5 +1,118 @@
import { PageStub } from '@/components/PageStub'
import { useState, type FormEvent } from 'react'
import { Link, useNavigate } from '@tanstack/react-router'
import { AuthCard } from '@/components/auth/AuthCard'
import { Alert } from '@/components/ui/Alert'
import { Button } from '@/components/ui/Button'
import { TextField } from '@/components/ui/TextField'
import { apiErrorCode, errorMessage } from '@/lib/api'
import { useProviders, useRegister } from '@/lib/auth'
const MIN_PASSWORD = 8
export function RegisterPage() {
return <PageStub title="Create account">Registration form arrives in issue #6.</PageStub>
const navigate = useNavigate()
const providers = useProviders()
const register = useRegister()
const [email, setEmail] = useState('')
const [displayName, setDisplayName] = useState('')
const [password, setPassword] = useState('')
async function onSubmit(e: FormEvent) {
e.preventDefault()
try {
await register.mutateAsync({ email, displayName, password })
await navigate({ to: '/gardens' }) // registration auto-logs-in
} catch {
// Shown below via register.error.
}
}
// Don't render the form until we know local registration is offered — otherwise
Outdated
Review

🔴 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>
// it briefly appears (and is submittable) on an SSO-only server.
if (providers.isPending) {
return (
<AuthCard title="Create account">
<p className="text-sm text-muted">Loading</p>
</AuthCard>
)
}
if (providers.isError) {
return (
<AuthCard title="Create account">
<Alert>Could not load sign-in options. Please refresh.</Alert>
</AuthCard>
)
}
// Local registration is only meaningful when local auth is enabled.
if (!providers.data.local) {
return (
<AuthCard title="Create account">
<div className="flex flex-col gap-4">
<Alert tone="info">This server uses single sign-on. Local registration is disabled.</Alert>
<Link to="/login" className="text-center text-sm font-medium text-accent-strong hover:underline">
Back to sign in
</Link>
</div>
Review

🟡 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>
</AuthCard>
)
}
const registrationClosed = apiErrorCode(register.error) === 'REGISTRATION_CLOSED'
return (
<AuthCard title="Create your pansy account" subtitle="The first account becomes the admin.">
{registrationClosed ? (
<div className="flex flex-col gap-4">
<Alert>Registration is closed on this server. Ask an administrator for an account.</Alert>
<Link to="/login" className="text-center text-sm font-medium text-accent-strong hover:underline">
Back to sign in
</Link>
</div>
) : (
<form onSubmit={onSubmit} className="flex flex-col gap-3">
<TextField
label="Email"
name="email"
type="email"
autoComplete="email"
autoCapitalize="none"
spellCheck={false}
required
value={email}
onChange={(e) => setEmail(e.target.value)}
/>
<TextField
label="Display name"
name="displayName"
type="text"
autoComplete="name"
required
value={displayName}
onChange={(e) => setDisplayName(e.target.value)}
/>
<TextField
label="Password"
name="password"
type="password"
autoComplete="new-password"
required
minLength={MIN_PASSWORD}
hint={`At least ${MIN_PASSWORD} characters.`}
value={password}
onChange={(e) => setPassword(e.target.value)}
/>
{register.isError && <Alert>{errorMessage(register.error, 'Could not create your account.')}</Alert>}
<Button type="submit" disabled={register.isPending} className="mt-1">
{register.isPending ? 'Creating account…' : 'Create account'}
</Button>
<p className="text-center text-sm text-muted">
Already have an account?{' '}
<Link to="/login" className="font-medium text-accent-strong hover:underline">
Sign in
</Link>
</p>
</form>
)}
</AuthCard>
)
}
+76 -8
View File
@@ -1,20 +1,49 @@
import {
createRootRoute,
createRootRouteWithContext,
createRoute,
createRouter,
redirect,
} from '@tanstack/react-router'
import type { QueryClient } from '@tanstack/react-query'
import { AppShell } from '@/components/layout/AppShell'
import { RouteError } from '@/components/RouteError'
import { LoginPage } from '@/pages/LoginPage'
import { RegisterPage } from '@/pages/RegisterPage'
import { GardensPage } from '@/pages/GardensPage'
import { GardenEditorPage } from '@/pages/GardenEditorPage'
import { PlantsPage } from '@/pages/PlantsPage'
import { meQueryOptions } from '@/lib/auth'
import { queryClient } from '@/lib/queryClient'
import { safeRedirectPath } from '@/lib/redirect'
const rootRoute = createRootRoute({ component: AppShell })
interface RouterContext {
queryClient: QueryClient
}
const rootRoute = createRootRouteWithContext<RouterContext>()({
component: AppShell,
// A beforeLoad/loader failure that isn't a redirect (e.g. /auth/me errors with
Outdated
Review

🔴 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>
// a 500 or the network drops) lands here instead of a blank screen.
errorComponent: RouteError,
})
// requireAuth: resolve the current user (shared cache with useMe); send anyone
// unauthenticated to /login, remembering the path they were headed to.
async function requireAuth(context: RouterContext, path: string) {
const me = await context.queryClient.ensureQueryData(meQueryOptions)
if (!me) {
throw redirect({ to: '/login', search: { redirect: path } })
}
}
// requireGuest: keep already-authenticated users off /login and /register.
async function requireGuest(context: RouterContext, redirectTo: string) {
Review

🟠 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>
const me = await context.queryClient.ensureQueryData(meQueryOptions)
if (me) {
throw redirect({ to: redirectTo })
}
}
// The root path has no page of its own yet; send it to the gardens list. A real
// auth-aware landing/redirect lands with the route guard in issue #6.
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
@@ -23,15 +52,53 @@ const indexRoute = createRoute({
},
})
const loginRoute = createRoute({ getParentRoute: () => rootRoute, path: 'login', component: LoginPage })
const registerRoute = createRoute({ getParentRoute: () => rootRoute, path: 'register', component: RegisterPage })
const gardensRoute = createRoute({ getParentRoute: () => rootRoute, path: 'gardens', component: GardensPage })
interface LoginSearch {
redirect?: string
error?: string
}
const loginRoute = createRoute({
getParentRoute: () => rootRoute,
path: 'login',
// Optional properties (not `key: undefined`) so links to /login don't have to
// pass a search object.
validateSearch: (search: Record<string, unknown>): LoginSearch => {
const out: LoginSearch = {}
if (typeof search.redirect === 'string') out.redirect = search.redirect
if (typeof search.error === 'string') out.error = search.error
return out
},
beforeLoad: ({ context, search }) => requireGuest(context, safeRedirectPath(search.redirect)),
component: LoginPage,
})
const registerRoute = createRoute({
getParentRoute: () => rootRoute,
path: 'register',
beforeLoad: ({ context }) => requireGuest(context, '/gardens'),
component: RegisterPage,
})
const gardensRoute = createRoute({
Review

🟠 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>
getParentRoute: () => rootRoute,
path: 'gardens',
beforeLoad: ({ context, location }) => requireAuth(context, location.href),
component: GardensPage,
})
const gardenEditorRoute = createRoute({
getParentRoute: () => rootRoute,
path: 'gardens/$gardenId',
beforeLoad: ({ context, location }) => requireAuth(context, location.href),
component: GardenEditorPage,
})
const plantsRoute = createRoute({ getParentRoute: () => rootRoute, path: 'plants', component: PlantsPage })
const plantsRoute = createRoute({
getParentRoute: () => rootRoute,
path: 'plants',
beforeLoad: ({ context, location }) => requireAuth(context, location.href),
component: PlantsPage,
})
const routeTree = rootRoute.addChildren([
indexRoute,
@@ -45,6 +112,7 @@ const routeTree = rootRoute.addChildren([
export const router = createRouter({
routeTree,
defaultPreload: 'intent',
context: { queryClient },
})
declare module '@tanstack/react-router' {