diff --git a/client/src/admin/Admin.tsx b/client/src/admin/Admin.tsx index 9c9e8d1..093e3fe 100644 --- a/client/src/admin/Admin.tsx +++ b/client/src/admin/Admin.tsx @@ -7,10 +7,14 @@ async function j(res: Response): Promise { return res.json(); } -// Best-effort error message from a failed response ({ error } JSON or status text). +// Best-effort error message from a failed response. Prefers RFC 7807 +// (application/problem+json) `detail`/`title` — what the admin routes send +// — falls back to the older ad hoc `{ error }` shape (still used by a few +// not-yet-converted endpoints), then plain status text. async function errText(res: Response): Promise { try { - return (await res.json()).error ?? res.statusText; + const body = await res.json(); + return body.detail ?? body.title ?? body.error ?? res.statusText; } catch { return res.statusText; } diff --git a/server/src/index.ts b/server/src/index.ts index b1747e2..81f0925 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -8,6 +8,7 @@ import { existsSync } from 'node:fs'; import { openDb } from './db.js'; import { registerPublicRoutes } from './routes/public.js'; import { registerAdminRoutes } from './routes/admin.js'; +import { sendProblem } from './problem-details.js'; const __dirname = dirname(fileURLToPath(import.meta.url)); @@ -46,6 +47,38 @@ const app = Fastify({ trustProxy: process.env.WUTZ_TRUST_PROXY === '1', }); +// RFC 7807 title for Fastify's own sub-500 errors (bad JSON body, schema +// validation) — those never go through our own `sendProblem` call sites, +// so this is the only place their status code needs a stable `title`. +const STATUS_TITLES: Record = { + 400: 'Bad Request', + 401: 'Unauthorized', + 403: 'Forbidden', + 404: 'Not Found', + 409: 'Conflict', + 413: 'Payload Too Large', + 415: 'Unsupported Media Type', + 429: 'Too Many Requests', +}; + +// Fastify's default error handler serialises a thrown error's `code` and +// `message` straight into the response body — fine for our own explicit +// `reply.code(4xx).send(...)` calls (those never reach this handler at +// all), but any *unhandled* throw (a raw SQLite constraint error, for +// instance) would otherwise hand the client a raw SQL message. Keep +// Fastify's own sub-500 errors (bad JSON body, schema validation) as-is — +// those messages are meant for the client — and generalise anything ≥ 500. +// All bodies are RFC 7807 (application/problem+json) — see problem-details.ts. +app.setErrorHandler((err, req, reply) => { + const statusCode = err.statusCode ?? 500; + if (statusCode < 500) { + sendProblem(req, reply, statusCode, STATUS_TITLES[statusCode] ?? 'Bad Request', err.message); + return; + } + req.log.error(err); + sendProblem(req, reply, 500, 'Internal Server Error', 'internal error'); +}); + await app.register(fastifyCookie); const db = openDb(DB_PATH); @@ -65,7 +98,7 @@ if (existsSync(clientDist)) { }); app.setNotFoundHandler((req, reply) => { if (req.url.startsWith('/api') || req.url.startsWith('/admin/api')) { - reply.code(404).send({ error: 'not found' }); + sendProblem(req, reply, 404, 'Not Found', 'not found'); return; } if (req.url.startsWith('/admin')) { diff --git a/server/src/problem-details.ts b/server/src/problem-details.ts new file mode 100644 index 0000000..3aa1b5f --- /dev/null +++ b/server/src/problem-details.ts @@ -0,0 +1,31 @@ +import type { FastifyReply, FastifyRequest } from 'fastify'; + +// RFC 7807 (application/problem+json) error body. `title` is the general +// class of problem — stable for a given status/situation, safe to key UI +// copy off of — while `detail` is specific to this occurrence (the old ad +// hoc `{ error: string }` body's message lives on here so existing client +// error-text handling degrades gracefully). +export interface ProblemDetails { + type: string; + title: string; + status: number; + detail?: string; + instance?: string; +} + +export function sendProblem( + req: FastifyRequest, + reply: FastifyReply, + status: number, + title: string, + detail?: string +): void { + const body: ProblemDetails = { + type: 'about:blank', + title, + status, + ...(detail !== undefined ? { detail } : {}), + instance: req.url, + }; + reply.code(status).header('Content-Type', 'application/problem+json').send(body); +} diff --git a/server/src/routes/admin.ts b/server/src/routes/admin.ts index f2c7de2..21c0374 100644 --- a/server/src/routes/admin.ts +++ b/server/src/routes/admin.ts @@ -2,6 +2,7 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; import type { DB } from '../db.js'; import { businessDay, formatLocal, parseDbTime } from '../time.js'; import { createHash, randomBytes, timingSafeEqual } from 'node:crypto'; +import { sendProblem } from '../problem-details.js'; const SESSION_COOKIE = 'wutz_admin'; @@ -35,16 +36,29 @@ function safeEqual(a: string, b: string): boolean { function requireAuth(req: FastifyRequest, reply: FastifyReply): boolean { if (isAuthed(req)) return true; - reply.code(401).send({ error: 'unauthorized' }); + sendProblem(req, reply, 401, 'Unauthorized', 'unauthorized'); return false; } +// `Number(nonNumericString)` is NaN, and NaN binds as SQL NULL — an +// `UPDATE ... WHERE id = NULL` silently matches zero rows instead of +// throwing, so a bad :id param used to "succeed" the same way a genuinely +// missing id does. Reject it explicitly instead. +function parseId(raw: string, req: FastifyRequest, reply: FastifyReply): number | null { + const id = Number(raw); + if (!Number.isInteger(id)) { + sendProblem(req, reply, 400, 'Bad Request', 'invalid id'); + return null; + } + return id; +} + export function registerAdminRoutes(app: FastifyInstance, db: DB) { app.post<{ Body: { password?: string } }>('/admin/login', async (req, reply) => { const expected = process.env.ADMIN_PASSWORD; - if (!expected) return reply.code(500).send({ error: 'ADMIN_PASSWORD not set' }); + if (!expected) return sendProblem(req, reply, 500, 'Internal Server Error', 'ADMIN_PASSWORD not set'); if (!safeEqual(req.body?.password ?? '', expected)) { - return reply.code(401).send({ error: 'wrong password' }); + return sendProblem(req, reply, 401, 'Unauthorized', 'wrong password'); } const token = randomBytes(32).toString('hex'); validSessions.add(token); @@ -79,11 +93,18 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { async (req, reply) => { if (!requireAuth(req, reply)) return; const { name, price_cents } = req.body ?? ({} as any); - if (!name || !isValidCents(price_cents)) return reply.code(400).send({ error: 'invalid' }); - const info = db - .prepare('INSERT INTO drinks (name, price_cents) VALUES (?, ?)') - .run(name, price_cents); - return { id: Number(info.lastInsertRowid) }; + if (!name || !isValidCents(price_cents)) return sendProblem(req, reply, 400, 'Bad Request', 'invalid'); + try { + const info = db + .prepare('INSERT INTO drinks (name, price_cents) VALUES (?, ?)') + .run(name, price_cents); + return { id: Number(info.lastInsertRowid) }; + } catch (e: any) { + if (String(e.message).includes('UNIQUE') && String(e.message).includes('drinks.name')) { + return sendProblem(req, reply, 409, 'Conflict', 'name already exists'); + } + throw e; + } } ); @@ -91,10 +112,11 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { '/admin/api/drinks/:id', async (req, reply) => { if (!requireAuth(req, reply)) return; - const id = Number(req.params.id); + const id = parseId(req.params.id, req, reply); + if (id === null) return; const { name, price_cents, archived } = req.body ?? {}; if (price_cents !== undefined && !isValidCents(price_cents)) { - return reply.code(400).send({ error: 'invalid price_cents' }); + return sendProblem(req, reply, 400, 'Bad Request', 'invalid price_cents'); } const sets: string[] = []; const args: any[] = []; @@ -103,26 +125,33 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { if (archived !== undefined) { sets.push('archived = ?'); args.push(archived ? 1 : 0); } if (sets.length === 0) return { ok: true }; args.push(id); - db.prepare(`UPDATE drinks SET ${sets.join(', ')} WHERE id = ?`).run(...args); + const info = db.prepare(`UPDATE drinks SET ${sets.join(', ')} WHERE id = ?`).run(...args); + if (info.changes === 0) return sendProblem(req, reply, 404, 'Not Found', 'drink not found'); return { ok: true }; } ); app.delete<{ Params: { id: string } }>('/admin/api/drinks/:id', async (req, reply) => { if (!requireAuth(req, reply)) return; - const id = Number(req.params.id); + const id = parseId(req.params.id, req, reply); + if (id === null) return; const sold = db .prepare('SELECT 1 FROM transaction_items WHERE drink_id = ? LIMIT 1') .get(id); if (sold) { - return reply - .code(409) - .send({ error: 'Getränk wurde bereits verkauft — bitte archivieren statt löschen.' }); + return sendProblem( + req, + reply, + 409, + 'Conflict', + 'Getränk wurde bereits verkauft — bitte archivieren statt löschen.' + ); } - db.transaction(() => { + const changes = db.transaction(() => { db.prepare('DELETE FROM bar_drinks WHERE drink_id = ?').run(id); - db.prepare('DELETE FROM drinks WHERE id = ?').run(id); + return db.prepare('DELETE FROM drinks WHERE id = ?').run(id).changes; })(); + if (changes === 0) return sendProblem(req, reply, 404, 'Not Found', 'drink not found'); return { ok: true }; }); @@ -145,9 +174,9 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { if (!requireAuth(req, reply)) return; const name = req.body?.name?.trim(); const pfand_cents = req.body?.pfand_cents ?? 0; - if (!name) return reply.code(400).send({ error: 'name required' }); + if (!name) return sendProblem(req, reply, 400, 'Bad Request', 'name required'); if (!isValidCents(pfand_cents)) { - return reply.code(400).send({ error: 'invalid pfand_cents' }); + return sendProblem(req, reply, 400, 'Bad Request', 'invalid pfand_cents'); } try { const info = db @@ -156,7 +185,7 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { return { id: Number(info.lastInsertRowid) }; } catch (e: any) { if (String(e.message).includes('UNIQUE')) { - return reply.code(409).send({ error: 'name already exists' }); + return sendProblem(req, reply, 409, 'Conflict', 'name already exists'); } throw e; } @@ -167,10 +196,16 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { '/admin/api/bars/:id', async (req, reply) => { if (!requireAuth(req, reply)) return; - const id = Number(req.params.id); + const id = parseId(req.params.id, req, reply); + if (id === null) return; const { name, pfand_cents, drink_ids } = req.body ?? {}; if (pfand_cents !== undefined && !isValidCents(pfand_cents)) { - return reply.code(400).send({ error: 'invalid pfand_cents' }); + return sendProblem(req, reply, 400, 'Bad Request', 'invalid pfand_cents'); + } + if (drink_ids !== undefined) { + if (!Array.isArray(drink_ids) || !drink_ids.every(did => Number.isInteger(did))) { + return sendProblem(req, reply, 400, 'Bad Request', 'invalid drink_ids'); + } } try { db.transaction(() => { @@ -187,14 +222,18 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { const ins = db.prepare( 'INSERT INTO bar_drinks (bar_id, drink_id, sort_order) VALUES (?, ?, ?)' ); - drink_ids.forEach((did, idx) => ins.run(id, did, idx)); + // Dedupe: a repeated id would otherwise trip bar_drinks' own + // (bar_id, drink_id) primary key and get mis-reported below as + // "name already exists" via the UNIQUE substring match. + [...new Set(drink_ids)].forEach((did, idx) => ins.run(id, did, idx)); } })(); } catch (e: any) { - if (String(e.message).includes('UNIQUE')) { - return reply.code(409).send({ error: 'name already exists' }); + const msg = String(e.message); + if (msg.includes('UNIQUE') && msg.includes('bars.name')) { + return sendProblem(req, reply, 409, 'Conflict', 'name already exists'); } - if (e.message === 'name empty') return reply.code(400).send({ error: 'name empty' }); + if (msg === 'name empty') return sendProblem(req, reply, 400, 'Bad Request', 'name empty'); throw e; } return { ok: true }; @@ -203,19 +242,25 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { app.delete<{ Params: { id: string } }>('/admin/api/bars/:id', async (req, reply) => { if (!requireAuth(req, reply)) return; - const id = Number(req.params.id); + const id = parseId(req.params.id, req, reply); + if (id === null) return; const used = db .prepare('SELECT 1 FROM transactions WHERE bar_id = ? LIMIT 1') .get(id); if (used) { - return reply - .code(409) - .send({ error: 'Tresen hat Transaktionen — Löschen würde die Statistik verfälschen.' }); + return sendProblem( + req, + reply, + 409, + 'Conflict', + 'Tresen hat Transaktionen — Löschen würde die Statistik verfälschen.' + ); } - db.transaction(() => { + const changes = db.transaction(() => { db.prepare('DELETE FROM bar_drinks WHERE bar_id = ?').run(id); - db.prepare('DELETE FROM bars WHERE id = ?').run(id); + return db.prepare('DELETE FROM bars WHERE id = ?').run(id).changes; })(); + if (changes === 0) return sendProblem(req, reply, 404, 'Not Found', 'bar not found'); return { ok: true }; });