fix(admin): answer 404 for an item id that does not exist (#207)

Three routes in admin.ts answered a miss with a success. PUT /items/:id ran an UPDATE that matched nothing, committed happily, selected nothing back and replied 200 with an empty body — a success the admin client could do nothing with, and no record anywhere that the item was not found. mark-sold and mark-available did the same. The create route beside them has always used requireRow for exactly this, which is why this reads as an oversight rather than a decision.

A garbage id was worse in a different direction. Number('abc') is NaN, the driver sends it to Postgres as the text "NaN", Postgres raises 22P02 for an integer column, and the catch turned that into a 500 — so a caller asking for an item that cannot exist was told the server broke. Both now answer 404, because from the caller's side "/items/abc" identifies no item in exactly the way "/items/999999" does.

readId is shared rather than repeated, and rejects zero, negatives and fractions as well as text: every id in this schema is a positive serial, so anything else identifies nothing.

mark-sold now notifies favouriters only after the row is known to exist, so nobody is told about a sale that did not happen.

The issue asked for the same shape to be checked across the other admin routes. It was: unpublish already looks the item up and 404s, and the tags and categories PUT routes both do an existence check before their UPDATE, so their rows[0] is guaranteed. items.ts already guards the public read. These three were the only ones lying about a miss.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/routes/admin.ts

@@ -1,6 +1,7 @@
 import { Router, Request, Response } from 'express';
 import { PoolClient } from 'pg';
 import { pool, requireRow } from '../db';
+import { readId } from '../utils';
 import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect';
 import { ItemStatus } from '../types';
 import { asyncRoute } from '../asyncRoute';
@@ -185,6 +186,9 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons
 }));
 
 router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Response) => {
+  const itemId = readId(req.params.id);
+  if (itemId === null) return res.status(404).json({ error: 'not found' });
+
   const { name, description, price } = req.body;
 
   const parsed = readOptionalItemFields(req.body);
@@ -197,7 +201,7 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
     await client.query('BEGIN');
     await client.query(
       `UPDATE items SET name=$1, description=$2, price_cents=$3 WHERE id=$4`,
-      [name, description, Math.round(parseFloat(price) * 100), req.params.id]
+      [name, description, Math.round(parseFloat(price) * 100), itemId]
     );
     // Only touch the category when the field was actually submitted, so a
     // caller that omits it doesn't silently uncategorize the item.
@@ -224,8 +228,15 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
     // S2077, the same constant-plus-$1 shape as the create route above.
     // req.params.id is caller-controlled and goes through the driver as a bound
     // parameter; it never reaches the query text.
-    const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]);
-    res.json(full[0]);
+    const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [itemId]);
+    // The create route beside this one has always used requireRow here. This
+    // one did not, so an UPDATE matching nothing committed happily, the SELECT
+    // returned nothing, and the caller got 200 with an empty body — a success
+    // it could do nothing with, and no record anywhere that the item was
+    // missing. See #207.
+    const updated = full[0];
+    if (!updated) return res.status(404).json({ error: 'not found' });
+    res.json(updated);
   } catch (err) {
     await client.query('ROLLBACK');
     console.error(err);
@@ -257,14 +268,21 @@ router.delete('/items/:id/images/:imageId', asyncRoute(async (req: Request, res:
 }));
 
 router.post('/items/:id/mark-sold', asyncRoute(async (req: Request, res: Response) => {
+  const itemId = readId(req.params.id);
+  if (itemId === null) return res.status(404).json({ error: 'not found' });
+
   const { rows } = await pool.query<ItemRecord>(
     `UPDATE items SET status='sold', sold_at=now() WHERE id=$1 RETURNING *`,
-    [req.params.id]
+    [itemId]
   );
+  const sold = rows[0];
+  if (!sold) return res.status(404).json({ error: 'not found' });
+
   // No buyer to exclude: an admin marking an item sold has no associated
-  // customer, so everyone watching it hears about it.
-  await notifyFavoritersOfSale([Number(req.params.id)], null);
-  res.json(rows[0]);
+  // customer, so everyone watching it hears about it. Sent only after the row
+  // is known to exist, so nobody is told about a sale that did not happen.
+  await notifyFavoritersOfSale([sold.id], null);
+  res.json(sold);
 }));
 
 // Publishing is the existing mark-available: it already sets status='available'
@@ -302,12 +320,17 @@ router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Respons
 }));
 
 router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Response) => {
+  const itemId = readId(req.params.id);
+  if (itemId === null) return res.status(404).json({ error: 'not found' });
+
   const { rows } = await pool.query<ItemRecord>(
     `UPDATE items SET status='available', sold_at=NULL, reserved_until=NULL, paypal_order_id=NULL
      WHERE id=$1 RETURNING *`,
-    [req.params.id]
+    [itemId]
   );
-  res.json(rows[0]);
+  const available = rows[0];
+  if (!available) return res.status(404).json({ error: 'not found' });
+  res.json(available);
 }));
 
 export default router;