Auth UI: login/register pages, OIDC button, route guard (#6) #25
@@ -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>
|
||||
)
|
||||
}
|
||||
@@ -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>
|
||||
)
|
||||
}
|
||||
@@ -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()
|
||||
|
|
||||
const logout = useLogout()
|
||||
const navigate = useNavigate()
|
||||
const user = me.data
|
||||
|
||||
async function onLogout() {
|
||||
|
gitea-actions
commented
🔴 Logout failure is unhandled, leaving stale auth state and no user feedback correctness, error-handling · flagged by 5 models
🪰 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>
|
||||
|
||||
|
||||
@@ -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>
|
||||
)
|
||||
}
|
||||
@@ -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} />
|
||||
}
|
||||
@@ -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>
|
||||
)
|
||||
}
|
||||
@@ -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,
|
||||
|
gitea-actions
commented
🟠 hintId becomes literal 'undefined-hint' when id and name are both omitted correctness, error-handling · flagged by 1 model
🪰 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>
|
||||
)
|
||||
})
|
||||
@@ -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'>) =>
|
||||
|
||||
@@ -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) {
|
||||
|
gitea-actions
commented
🔴 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
🪰 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,
|
||||
|
gitea-actions
commented
🟠 meQueryOptions staleTime of 60s allows stale auth data to pass route guards correctness · flagged by 1 model
🪰 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'),
|
||||
|
gitea-actions
commented
🟡 error-handling · flagged by 1 model
🪰 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' })
|
||||
},
|
||||
})
|
||||
}
|
||||
@@ -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 },
|
||||
},
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
@@ -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')
|
||||
|
||||
|
||||
@@ -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'
|
||||
|
||||
|
gitea-actions
commented
🟡 Hardcoded API base path duplicates lib/api.ts constant maintainability · flagged by 1 model
🪰 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.',
|
||||
}
|
||||
|
gitea-actions
commented
🟠 Duplicate redirect sanitization logic with router.tsx maintainability · flagged by 4 models
🪰 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) {
|
||||
|
gitea-actions
commented
🟡 error-handling · flagged by 1 model
🪰 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 {
|
||||
|
gitea-actions
commented
🟡 error-handling · flagged by 1 model
🪰 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'}
|
||||
|
gitea-actions
commented
🟠 OIDC login drops the redirect target; user lands on /gardens instead of the remembered destination correctness · flagged by 1 model
🪰 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"
|
||||
|
gitea-actions
commented
🟡 noValidate disables required/minLength guards; empty/invalid input is sent to the server with no client-side check error-handling · flagged by 2 models
🪰 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>
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
gitea-actions
commented
🔴 RegisterPage renders form while providers query is pending or errored correctness, error-handling, maintainability · flagged by 4 models
🪰 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>
|
||||
|
gitea-actions
commented
🟡 Form with noValidate lacks client-side validation, allowing empty submissions error-handling · flagged by 1 model
🪰 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>
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
gitea-actions
commented
🔴 Redirect-after-login broken because location.href (full URL) is passed but safeRedirect expects a path error-handling · flagged by 2 models
🪰 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) {
|
||||
|
gitea-actions
commented
🟠 Duplicate redirect sanitization logic with LoginPage.tsx maintainability · flagged by 2 models
🪰 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({
|
||||
|
gitea-actions
commented
🟠 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' {
|
||||
|
||||
🟡 Auth nav flickers unauthenticated state during me query loading
error-handling · flagged by 1 model
web/src/components/layout/AppShell.tsx:18-21—useMe()’s loading state is ignored (const user = me.data). On a hard refresh,dataisundefinedwhile the query is pending, so authenticated users see a brief flash of the “Sign in” link and the unauthenticated nav layout. Fix: Checkme.isPendingand render a neutral loading state (e.g., a skeleton or simply omit the auth-specific buttons until resolved).🪰 Gadfly · advisory