From ae1906e169acc47f4ac25022e24aa5ff95c2938e Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 18:10:00 -0400 Subject: [PATCH] Address Gadfly review on #6: logout errors, register gating, guard errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- web/src/components/RouteError.tsx | 17 ++++++++++++++++ web/src/components/layout/AppShell.tsx | 13 +++++++++--- web/src/components/ui/Divider.tsx | 12 +++++++++++ web/src/components/ui/TextField.tsx | 8 +++++--- web/src/lib/api.ts | 24 ++++++++++++++++++++-- web/src/lib/auth.ts | 15 -------------- web/src/lib/redirect.ts | 14 +++++++++++++ web/src/pages/LoginPage.tsx | 28 ++++++++------------------ web/src/pages/RegisterPage.tsx | 23 ++++++++++++++++++--- web/src/router.tsx | 21 ++++++++++--------- 10 files changed, 120 insertions(+), 55 deletions(-) create mode 100644 web/src/components/RouteError.tsx create mode 100644 web/src/components/ui/Divider.tsx create mode 100644 web/src/lib/redirect.ts diff --git a/web/src/components/RouteError.tsx b/web/src/components/RouteError.tsx new file mode 100644 index 0000000..095900e --- /dev/null +++ b/web/src/components/RouteError.tsx @@ -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 ( +
+

Something went wrong

+

{errorMessage(error, 'An unexpected error occurred.')}

+ +
+ ) +} diff --git a/web/src/components/layout/AppShell.tsx b/web/src/components/layout/AppShell.tsx index 9054e90..5cf7770 100644 --- a/web/src/components/layout/AppShell.tsx +++ b/web/src/components/layout/AppShell.tsx @@ -21,8 +21,14 @@ export function AppShell() { const user = me.data async function onLogout() { - await logout.mutateAsync() - await navigate({ to: '/login' }) + 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 ( @@ -55,9 +61,10 @@ export function AppShell() { 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…' : 'Sign out'} + {logout.isPending ? 'Signing out…' : logout.isError ? 'Retry sign out' : 'Sign out'} ) : ( diff --git a/web/src/components/ui/Divider.tsx b/web/src/components/ui/Divider.tsx new file mode 100644 index 0000000..5c638f7 --- /dev/null +++ b/web/src/components/ui/Divider.tsx @@ -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 ( +
+ + {children} + +
+ ) +} diff --git a/web/src/components/ui/TextField.tsx b/web/src/components/ui/TextField.tsx index 7015c3a..d277c12 100644 --- a/web/src/components/ui/TextField.tsx +++ b/web/src/components/ui/TextField.tsx @@ -1,4 +1,4 @@ -import { forwardRef, type InputHTMLAttributes } from 'react' +import { forwardRef, useId, type InputHTMLAttributes } from 'react' import { cn } from '@/lib/cn' interface TextFieldProps extends InputHTMLAttributes { @@ -7,12 +7,14 @@ interface TextFieldProps extends InputHTMLAttributes { } // A labelled text input. text-base (16px) is deliberate: smaller fonts make iOS -// Safari zoom on focus. The label's htmlFor falls back to the field name. +// 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(function TextField( { label, hint, id, name, className, ...props }, ref, ) { - const inputId = id ?? name + const generatedId = useId() + const inputId = id ?? name ?? generatedId const hintId = hint ? `${inputId}-hint` : undefined return (
diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 9cefee5..7515f20 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -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(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: (path: string, opts?: Omit) => diff --git a/web/src/lib/auth.ts b/web/src/lib/auth.ts index 85d4996..75ea94e 100644 --- a/web/src/lib/auth.ts +++ b/web/src/lib/auth.ts @@ -95,18 +95,3 @@ export function useLogout() { }, }) } - -/** 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. */ -export function errorMessage(err: unknown, fallback = 'Something went wrong.'): string { - if (err instanceof ApiError) return err.message - return fallback -} diff --git a/web/src/lib/redirect.ts b/web/src/lib/redirect.ts new file mode 100644 index 0000000..bf28804 --- /dev/null +++ b/web/src/lib/redirect.ts @@ -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 +} diff --git a/web/src/pages/LoginPage.tsx b/web/src/pages/LoginPage.tsx index 550f656..884918a 100644 --- a/web/src/pages/LoginPage.tsx +++ b/web/src/pages/LoginPage.tsx @@ -1,14 +1,17 @@ -import { useState, type FormEvent, type ReactNode } from 'react' +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 { errorMessage, useLogin, useProviders } from '@/lib/auth' +import { API_BASE, errorMessage } from '@/lib/api' +import { useLogin, useProviders } from '@/lib/auth' +import { safeRedirectPath } from '@/lib/redirect' // 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' +const OIDC_LOGIN_URL = `${API_BASE}/auth/oidc/login` // Messages for the ?error= codes the OIDC callback redirects back with. const callbackErrors: Record = { @@ -20,11 +23,6 @@ const callbackErrors: Record = { oidc_conflict: 'That single sign-on identity conflicts with an existing account.', } -// Only follow same-origin absolute paths, never a caller-controlled external URL. -function safeRedirect(dest: string | undefined): string { - return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens' -} - export function LoginPage() { const search = useSearch({ from: '/login' }) const navigate = useNavigate() @@ -33,7 +31,7 @@ export function LoginPage() { const [email, setEmail] = useState('') const [password, setPassword] = useState('') - const dest = safeRedirect(search.redirect) + const dest = safeRedirectPath(search.redirect) const callbackError = search.error ? (callbackErrors[search.error] ?? callbackErrors.oidc) : null async function onSubmit(e: FormEvent) { @@ -66,7 +64,7 @@ export function LoginPage() { {oidc && local && or} {local && ( -
+ ) } - -function Divider({ children }: { children: ReactNode }) { - return ( -
- - {children} - -
- ) -} diff --git a/web/src/pages/RegisterPage.tsx b/web/src/pages/RegisterPage.tsx index 9f8cb26..fd3d51e 100644 --- a/web/src/pages/RegisterPage.tsx +++ b/web/src/pages/RegisterPage.tsx @@ -4,7 +4,8 @@ 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, useProviders, useRegister } from '@/lib/auth' +import { apiErrorCode, errorMessage } from '@/lib/api' +import { useProviders, useRegister } from '@/lib/auth' const MIN_PASSWORD = 8 @@ -26,8 +27,24 @@ export function RegisterPage() { } } + // Don't render the form until we know local registration is offered — otherwise + // it briefly appears (and is submittable) on an SSO-only server. + if (providers.isPending) { + return ( + +

Loading…

+ + ) + } + if (providers.isError) { + return ( + + Could not load sign-in options. Please refresh. + + ) + } // Local registration is only meaningful when local auth is enabled. - if (providers.isSuccess && !providers.data.local) { + if (!providers.data.local) { return (
@@ -52,7 +69,7 @@ export function RegisterPage() {
) : ( - + ()({ component: AppShell }) +const rootRoute = createRootRouteWithContext()({ + component: AppShell, + // A beforeLoad/loader failure that isn't a redirect (e.g. /auth/me errors with + // 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 where they were headed. -async function requireAuth(context: RouterContext, href: string) { +// 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: href } }) + throw redirect({ to: '/login', search: { redirect: path } }) } } @@ -37,10 +44,6 @@ async function requireGuest(context: RouterContext, redirectTo: string) { } } -function safeInternalPath(dest: string | undefined): string { - return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens' -} - const indexRoute = createRoute({ getParentRoute: () => rootRoute, path: '/', @@ -65,7 +68,7 @@ const loginRoute = createRoute({ if (typeof search.error === 'string') out.error = search.error return out }, - beforeLoad: ({ context, search }) => requireGuest(context, safeInternalPath(search.redirect)), + beforeLoad: ({ context, search }) => requireGuest(context, safeRedirectPath(search.redirect)), component: LoginPage, })