From d6e99794396b15b7f70df892a5217e7c3e0c7f2f Mon Sep 17 00:00:00 2001 From: iris Date: Fri, 31 Jul 2026 11:07:49 +0200 Subject: [PATCH] admin: close six 500-instead-of-4xx gaps on drink/bar CRUD endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six defensive-check gaps in server/src/routes/admin.ts, all the same theme (missing the same check a sibling endpoint already has): 1. PATCH /admin/api/drinks/:id with a taken name 500'd instead of 409 — the bar-rename equivalent (and drink POST) already catch the UNIQUE constraint, this endpoint didn't. 2. PATCH /admin/api/bars/:id with a nonexistent drink_id 500'd instead of 400 — drink_ids was only checked for integer-ness, not existence, so a bogus id hit an uncaught FK-constraint error. Now validated against the drinks table up front (rollback was already correct, this only fixes the status code). 3. PATCH /admin/api/bars/:id on a nonexistent bar silently returned 200 {ok:true} — the drinks-PATCH equivalent checks info.changes === 0 and 404s, this endpoint checked nothing. Now 404s up front. 4. Non-string name in any of the four drink/bar POST/PATCH endpoints 500'd (object/numeric name hit an uncaught TypeError calling .trim() on a non-string, or an uncaught SQLite type error). All four now typeof-guard before use. 5. Drink names weren't trimmed or checked for emptiness, unlike bars — an empty/whitespace name showed as a blank tablet tile, and a trailing-space variant of an existing name bypassed the UNIQUE index and split that drink's stats across two rows. Drink POST/PATCH now trim + reject blank, matching bars. Verified all six against a scratch DB with the real server running end-to-end (login, each failure case, plus a same-request-shape sanity check that valid updates still succeed). tsc --noEmit and the server build both clean. --- server/src/routes/admin.ts | 83 +++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 15 deletions(-) diff --git a/server/src/routes/admin.ts b/server/src/routes/admin.ts index d6b52e7..133902e 100644 --- a/server/src/routes/admin.ts +++ b/server/src/routes/admin.ts @@ -322,7 +322,12 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { '/admin/api/drinks', async (req, reply) => { if (!requireAuth(req, reply)) return; - const { name, price_cents } = req.body ?? ({} as any); + const { name: rawName, price_cents } = req.body ?? ({} as any); + // Trim + reject non-strings/blank, same as bars already do — an + // untrimmed or empty name shows as a blank tile on the tablet grid, + // and a trailing-space variant of an existing name bypasses the + // UNIQUE index and splits that drink's stats across two rows. + const name = typeof rawName === 'string' ? rawName.trim() : ''; if (!name || !isValidCents(price_cents)) return sendProblem(req, reply, 400, 'Bad Request', 'invalid'); try { const info = db @@ -344,10 +349,18 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { if (!requireAuth(req, reply)) return; const id = parseId(req.params.id, req, reply); if (id === null) return; - const { name, price_cents, archived } = req.body ?? {}; + const { name: rawName, price_cents, archived } = req.body ?? {}; if (price_cents !== undefined && !isValidCents(price_cents)) { return sendProblem(req, reply, 400, 'Bad Request', 'invalid price_cents'); } + let name: string | undefined; + if (rawName !== undefined) { + if (typeof rawName !== 'string') { + return sendProblem(req, reply, 400, 'Bad Request', 'invalid name'); + } + name = rawName.trim(); + if (!name) return sendProblem(req, reply, 400, 'Bad Request', 'name empty'); + } const sets: string[] = []; const args: any[] = []; if (name !== undefined) { sets.push('name = ?'); args.push(name); } @@ -355,9 +368,18 @@ 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); - 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 }; + try { + 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 }; + } catch (e: any) { + // Matches the POST /admin/api/drinks UNIQUE-name handling below — + // this endpoint was the one place that gap hadn't been closed yet. + if (String(e.message).includes('UNIQUE') && String(e.message).includes('drinks.name')) { + return sendProblem(req, reply, 409, 'Conflict', 'name already exists'); + } + throw e; + } } ); @@ -402,7 +424,14 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { '/admin/api/bars', async (req, reply) => { if (!requireAuth(req, reply)) return; - const name = req.body?.name?.trim(); + const rawName = req.body?.name; + // typeof-guard before .trim() — a non-string name (number/object) + // used to reach `.trim()` and throw an uncaught TypeError, surfacing + // as a raw 500 instead of a 400. + if (typeof rawName !== 'string') { + return sendProblem(req, reply, 400, 'Bad Request', 'name required'); + } + const name = rawName.trim(); const pfand_cents = req.body?.pfand_cents ?? 0; if (!name) return sendProblem(req, reply, 400, 'Bad Request', 'name required'); if (!isValidCents(pfand_cents)) { @@ -428,7 +457,22 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { if (!requireAuth(req, reply)) return; const id = parseId(req.params.id, req, reply); if (id === null) return; - const { name, pfand_cents, drink_ids } = req.body ?? {}; + // The drinks-PATCH equivalent checks info.changes === 0 and 404s; a + // PATCH to this multi-statement transaction had no such check at all, + // so a stale admin tab "fixing" a Pfand rate on an already-deleted bar + // got a silent 200 {ok:true} with nothing actually changed. + const bar = db.prepare('SELECT 1 FROM bars WHERE id = ?').get(id); + if (!bar) return sendProblem(req, reply, 404, 'Not Found', 'bar not found'); + + const { name: rawName, pfand_cents, drink_ids } = req.body ?? {}; + let name: string | undefined; + if (rawName !== undefined) { + if (typeof rawName !== 'string') { + return sendProblem(req, reply, 400, 'Bad Request', 'invalid name'); + } + name = rawName.trim(); + if (!name) return sendProblem(req, reply, 400, 'Bad Request', 'name empty'); + } if (pfand_cents !== undefined && !isValidCents(pfand_cents)) { return sendProblem(req, reply, 400, 'Bad Request', 'invalid pfand_cents'); } @@ -436,13 +480,25 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { if (!Array.isArray(drink_ids) || !drink_ids.every(did => Number.isInteger(did))) { return sendProblem(req, reply, 400, 'Bad Request', 'invalid drink_ids'); } + // drink_ids was only checked for integer-ness, not existence — a + // bogus id threw an uncaught FK-constraint error inside the + // transaction below (rollback held correctly, but surfaced as a + // raw 500 instead of a 400). + const uniqueIds = [...new Set(drink_ids)]; + if (uniqueIds.length > 0) { + const placeholders = uniqueIds.map(() => '?').join(','); + const found = db + .prepare(`SELECT id FROM drinks WHERE id IN (${placeholders})`) + .all(...uniqueIds) as { id: number }[]; + if (found.length !== uniqueIds.length) { + return sendProblem(req, reply, 400, 'Bad Request', 'unknown drink_id'); + } + } } try { db.transaction(() => { if (name !== undefined) { - const trimmed = name.trim(); - if (!trimmed) throw new Error('name empty'); - db.prepare('UPDATE bars SET name = ? WHERE id = ?').run(trimmed, id); + db.prepare('UPDATE bars SET name = ? WHERE id = ?').run(name, id); } if (pfand_cents !== undefined) { db.prepare('UPDATE bars SET pfand_cents = ? WHERE id = ?').run(pfand_cents, id); @@ -453,17 +509,14 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) { 'INSERT INTO bar_drinks (bar_id, drink_id, sort_order) VALUES (?, ?, ?)' ); // 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. + // (bar_id, drink_id) primary key. [...new Set(drink_ids)].forEach((did, idx) => ins.run(id, did, idx)); } })(); } catch (e: any) { - const msg = String(e.message); - if (msg.includes('UNIQUE') && msg.includes('bars.name')) { + if (String(e.message).includes('UNIQUE') && String(e.message).includes('bars.name')) { return sendProblem(req, reply, 409, 'Conflict', 'name already exists'); } - if (msg === 'name empty') return sendProblem(req, reply, 400, 'Bad Request', 'name empty'); throw e; } return { ok: true };