Auth UI: login/register pages, OIDC button, route guard (#6) #25
@@ -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,16 @@ const navLinkInactive = 'text-muted hover:bg-border/60 hover:text-fg'
|
||||
|
||||
/** Top-level chrome: a sticky nav bar plus the routed page in an <Outlet>. */
|
||||
export function AppShell() {
|
||||
const me = useMe()
|
||||
|
|
||||
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>
|
||||
await logout.mutateAsync()
|
||||
await navigate({ to: '/login' })
|
||||
}
|
||||
|
||||
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 +32,42 @@ 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}
|
||||
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'}
|
||||
</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,43 @@
|
||||
import { forwardRef, 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 label's htmlFor falls back to the field name.
|
||||
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 inputId = id ?? name
|
||||
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>
|
||||
)
|
||||
})
|
||||
@@ -0,0 +1,112 @@
|
||||
// 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' })
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
/** The server error code from an ApiError body ({error:{code,message}}), if any. */
|
||||
|
gitea-actions
commented
🟡 Generic API error helpers misplaced in auth module maintainability · flagged by 1 model 🪰 Gadfly · advisory 🟡 **Generic API error helpers misplaced in auth module**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
|
||||
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 {
|
||||
|
gitea-actions
commented
🟡 errorMessage ignores the caller's fallback for any ApiError, contradicting its doc-comment and surprising the login/register callers maintainability · flagged by 1 model
🪰 Gadfly · advisory 🟡 **errorMessage ignores the caller's fallback for any ApiError, contradicting its doc-comment and surprising the login/register callers**
_maintainability · flagged by 1 model_
- **`web/src/lib/auth.ts:109` — `errorMessage` silently ignores its `fallback` for `ApiError`.** The signature reads as "use this fallback message", but the implementation returns `err.message` for *any* `ApiError`, including ones whose server message is generic/technical (e.g. `Request failed (NNN)` from `messageFrom` in `api.ts`). Callers in `LoginPage` (line 90) and `RegisterPage` (line 87) pass tailored fallbacks (`'Could not sign in.'`, `'Could not create your account.'`) that never win for…
<sub>🪰 Gadfly · advisory</sub>
|
||||
if (err instanceof ApiError) return err.message
|
||||
return fallback
|
||||
}
|
||||
@@ -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 },
|
||||
},
|
||||
})
|
||||
@@ -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,122 @@
|
||||
import { PageStub } from '@/components/PageStub'
|
||||
import { useState, type FormEvent, type ReactNode } 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 { TextField } from '@/components/ui/TextField'
|
||||
import { errorMessage, useLogin, useProviders } from '@/lib/auth'
|
||||
|
||||
// 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'
|
||||
|
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>
|
||||
|
||||
// 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.',
|
||||
}
|
||||
|
||||
// Only follow same-origin absolute paths, never a caller-controlled external URL.
|
||||
function safeRedirect(dest: string | undefined): string {
|
||||
|
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>
|
||||
return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens'
|
||||
}
|
||||
|
||||
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 = safeRedirect(search.redirect)
|
||||
const callbackError = search.error ? (callbackErrors[search.error] ?? callbackErrors.oidc) : null
|
||||
|
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>
|
||||
|
||||
async function onSubmit(e: FormEvent) {
|
||||
e.preventDefault()
|
||||
try {
|
||||
await login.mutateAsync({ email, password })
|
||||
|
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>
|
||||
await navigate({ to: dest })
|
||||
} catch {
|
||||
// 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 && (
|
||||
|
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 href={OIDC_LOGIN_URL} className={buttonClasses('primary', 'w-full')}>
|
||||
{providers.data?.oidcLabel || 'Sign in with SSO'}
|
||||
</a>
|
||||
)}
|
||||
|
||||
{oidc && local && <Divider>or</Divider>}
|
||||
|
||||
{local && (
|
||||
<form onSubmit={onSubmit} className="flex flex-col gap-3" noValidate>
|
||||
|
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>
|
||||
<TextField
|
||||
label="Email"
|
||||
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>
|
||||
)
|
||||
}
|
||||
|
||||
function Divider({ children }: { children: ReactNode }) {
|
||||
|
gitea-actions
commented
⚪ Divider is a generic UI primitive but lives unexported in a page file, breaking the components/ui/ pattern used by the rest of the PR* maintainability · flagged by 1 model
🪰 Gadfly · advisory ⚪ **Divider is a generic UI primitive but lives unexported in a page file, breaking the components/ui/* pattern used by the rest of the PR**
_maintainability · flagged by 1 model_
- **`web/src/pages/LoginPage.tsx:114` — `Divider` is a generic UI primitive parked in a page file.** It's a stateless, reusable "or" divider with no login-specific logic, yet it lives inside `LoginPage.tsx` and is not exported. The rest of the PR deliberately promotes shared visuals to `components/ui/*` (`Alert`, `Button`, `TextField`) and `components/auth/*` (`AuthCard`). `Divider` breaks that pattern for no reason. Suggest moving it to `components/ui/Divider.tsx` (or folding into `AuthCard` if…
<sub>🪰 Gadfly · advisory</sub>
|
||||
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>
|
||||
)
|
||||
}
|
||||
|
||||
@@ -1,5 +1,101 @@
|
||||
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, 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.
|
||||
}
|
||||
}
|
||||
|
||||
// Local registration is only meaningful when local auth is enabled.
|
||||
if (providers.isSuccess && !providers.data.local) {
|
||||
|
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>
|
||||
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>
|
||||
</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" noValidate>
|
||||
|
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>
|
||||
<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,46 @@
|
||||
import {
|
||||
createRootRoute,
|
||||
createRootRouteWithContext,
|
||||
createRoute,
|
||||
createRouter,
|
||||
redirect,
|
||||
} from '@tanstack/react-router'
|
||||
import type { QueryClient } from '@tanstack/react-query'
|
||||
import { AppShell } from '@/components/layout/AppShell'
|
||||
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'
|
||||
|
||||
const rootRoute = createRootRoute({ component: AppShell })
|
||||
interface RouterContext {
|
||||
queryClient: QueryClient
|
||||
}
|
||||
|
||||
const rootRoute = createRootRouteWithContext<RouterContext>()({ component: AppShell })
|
||||
|
||||
// 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) {
|
||||
|
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>
|
||||
const me = await context.queryClient.ensureQueryData(meQueryOptions)
|
||||
if (!me) {
|
||||
throw redirect({ to: '/login', search: { redirect: href } })
|
||||
}
|
||||
}
|
||||
|
||||
// requireGuest: keep already-authenticated users off /login and /register.
|
||||
async function requireGuest(context: RouterContext, redirectTo: string) {
|
||||
const me = await context.queryClient.ensureQueryData(meQueryOptions)
|
||||
if (me) {
|
||||
throw redirect({ to: redirectTo })
|
||||
}
|
||||
}
|
||||
|
||||
function safeInternalPath(dest: string | undefined): 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>
|
||||
return dest && dest.startsWith('/') && !dest.startsWith('//') ? dest : '/gardens'
|
||||
}
|
||||
|
||||
// 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 +49,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, safeInternalPath(search.redirect)),
|
||||
component: LoginPage,
|
||||
})
|
||||
|
||||
const registerRoute = createRoute({
|
||||
getParentRoute: () => rootRoute,
|
||||
path: 'register',
|
||||
beforeLoad: ({ context }) => requireGuest(context, '/gardens'),
|
||||
component: RegisterPage,
|
||||
})
|
||||
|
||||
const gardensRoute = createRoute({
|
||||
getParentRoute: () => rootRoute,
|
||||
path: 'gardens',
|
||||
beforeLoad: ({ context, location }) => requireAuth(context, location.href),
|
||||
|
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>
|
||||
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 +109,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