fix(admin): answer 404 for an unreadable id instead of 500 (#207)

The issue asked for two things and only one had been done. A well-formed but absent id already answered 404 — the PUT route carries a comment saying so. A malformed one still reached Postgres as text, raised 22P02 on an integer column, and surfaced through the route's catch as a 500, telling the admin the server had broken when the truth is that no such item can exist. That half is now closed everywhere rather than on the three routes that happened to have it.

Guarded: DELETE an item, DELETE an image, unpublish, and every route in adminItemDrafts — publish, regenerate, discard, restore, and the two background-removal endpoints added by #281. The last of those were flagged in that feature's own final review as sharing this pre-existing shape, so they are fixed with the rest rather than left to be found again.

Routes carrying two ids guard both. A route can guard the first and forget the second, and the forgotten one fails exactly as loudly, so there is a case each way for both image endpoints and for DELETE image.

DELETE deliberately still answers 204 for a well-formed id that is absent. The method is idempotent and the caller's intent, that the item should not exist, is satisfied either way; what must not happen is a 500. There is a test pinning that so the distinction is a decision rather than an omission.

Also replaced the raw req.params.id and Number(req.params.id) uses that sat inside routes which had already computed a validated id. They were safe, because the guard above them made them safe, but a validated id and a raw one side by side in the same handler is how this bug comes back.

The test block named "a non-numeric id on every admin item route" covered two routes. It now covers every route that takes an id, which is what makes its name true.

Closes #207

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

backend/src/routes/admin.ts

@@ -205,15 +205,15 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
     // Only touch the category when the field was actually submitted, so a
     // caller that omits it doesn't silently uncategorize the item.
     if (categoryId !== undefined) {
-      await client.query(`UPDATE items SET category_id=$1 WHERE id=$2`, [categoryId, req.params.id]);
+      await client.query(`UPDATE items SET category_id=$1 WHERE id=$2`, [categoryId, itemId]);
     }
     if (tagNames) {
-      await setItemTags(client, Number(req.params.id), await resolveTagIds(client, tagNames));
+      await setItemTags(client, itemId, await resolveTagIds(client, tagNames));
     }
     if (files.length) {
       const { rows: existing } = await client.query<MaxSortRow>(
         `SELECT COALESCE(MAX(sort_order), -1) AS max_sort FROM item_images WHERE item_id = $1`,
-        [req.params.id]
+        [itemId]
       );
       // COALESCE'd MAX, so the aggregate always returns exactly one row.
       const nextSort = requireRow(existing, 'the MAX(sort_order) aggregate').max_sort + 1;
@@ -221,7 +221,7 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
       // always has this param, but noUncheckedIndexedAccess cannot know that,
       // and the helper's typed parameter surfaces what the old inline query's
       // unknown[] hid.
-      await insertItemImages(client, Number(req.params.id), files, nextSort);
+      await insertItemImages(client, itemId, files, nextSort);
     }
     await client.query('COMMIT');
     // S2077, the same constant-plus-$1 shape as the create route above.
@@ -246,7 +246,14 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
 }));
 
 router.delete('/items/:id', asyncRoute(async (req: Request, res: Response) => {
-  const itemId = Number(req.params.id);
+  // 404 rather than the 500 a raw Number() produced: 'abc' became NaN, reached
+  // Postgres as the text "NaN", raised 22P02 on an integer column and told the
+  // caller the server had broken. An item that cannot exist is not found (#207).
+  //
+  // A well-formed but absent id still answers 204. DELETE is idempotent and the
+  // caller's intent — that the item should not exist — is satisfied either way.
+  const itemId = readId(req.params.id);
+  if (itemId === null) return res.status(404).json({ error: 'not found' });
 
   // Collected before the delete: favorites cascade with the item, so after it
   // is gone there is no record of who was watching. Restricted to unsold items
@@ -262,7 +269,13 @@ router.delete('/items/:id', asyncRoute(async (req: Request, res: Response) => {
 }));
 
 router.delete('/items/:id/images/:imageId', asyncRoute(async (req: Request, res: Response) => {
-  await pool.query(`DELETE FROM item_images WHERE id = $1 AND item_id = $2`, [req.params.imageId, req.params.id]);
+  // Both ids, not just the first. A route carrying two of them can guard one
+  // and forget the other, and the forgotten one fails exactly as loudly (#207).
+  const itemId = readId(req.params.id);
+  const imageId = readId(req.params.imageId);
+  if (itemId === null || imageId === null) return res.status(404).json({ error: 'not found' });
+
+  await pool.query(`DELETE FROM item_images WHERE id = $1 AND item_id = $2`, [imageId, itemId]);
   res.status(204).end();
 }));
 
@@ -290,7 +303,13 @@ router.post('/items/:id/mark-sold', asyncRoute(async (req: Request, res: Respons
 // duplication, so the admin UI labels that button "Publish" when the item is
 // pending. This is the reverse, and it is not symmetrical — see the guard.
 router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Response) => {
-  const { rows } = await pool.query<ItemStatusRow>(`SELECT status FROM items WHERE id = $1`, [req.params.id]);
+  // Guarded before the lookup, so a malformed id is 404 rather than the 500 the
+  // raw string produced at Postgres. The absent case below was already right;
+  // only the unreadable one was not (#207).
+  const itemId = readId(req.params.id);
+  if (itemId === null) return res.status(404).json({ error: 'not found' });
+
+  const { rows } = await pool.query<ItemStatusRow>(`SELECT status FROM items WHERE id = $1`, [itemId]);
   if (!rows.length) {
     return res.status(404).json({ error: 'not found' });
   }
@@ -313,7 +332,7 @@ router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Respons
 
   const { rows: updated } = await pool.query<ItemRecord>(
     `UPDATE items SET status='pending' WHERE id=$1 RETURNING *`,
-    [req.params.id]
+    [itemId]
   );
   res.json(updated[0]);
 }));

backend/src/routes/adminItemDrafts.ts

@@ -3,6 +3,7 @@ import { pool } from '../db';
 import { asyncRoute } from '../asyncRoute';
 import { draftQueued } from '../intake/draftingWorker';
 import { nextPriceSource, PriceSource } from '../intake/priceSource';
+import { readId } from '../utils';
 import {
   NoOriginalToRestoreError,
   removeImageBackground,
@@ -97,6 +98,13 @@ router.post(
       return res.status(400).json({ error: 'a price in whole cents is required' });
     }
 
+    // Guarded before a connection is taken. A malformed id reached Postgres as
+    // text, raised 22P02 on an integer column and surfaced as a 500 — telling
+    // the admin the server had broken when the truth is that no such draft can
+    // exist (#207).
+    const itemId = readId(req.params.itemId);
+    if (itemId === null) return res.status(404).json({ error: 'no draft for this item' });
+
     const client = await pool.connect();
     try {
       await client.query('BEGIN');
@@ -109,7 +117,7 @@ router.post(
          FROM item_drafts d JOIN items i ON i.id = d.item_id
          WHERE d.item_id = $1
          FOR UPDATE OF d, i`,
-        [req.params.itemId]
+        [itemId]
       );
       const existing = rows[0];
       if (!existing) {
@@ -124,11 +132,11 @@ router.post(
          SET name = $2, description = $3, price_cents = $4,
              status = 'available', sold_at = NULL, reserved_until = NULL, paypal_order_id = NULL
          WHERE id = $1`,
-        [req.params.itemId, name, description === '' ? null : description, priceCents]
+        [itemId, name, description === '' ? null : description, priceCents]
       );
 
       await client.query(`UPDATE item_drafts SET price_source = $2 WHERE item_id = $1`, [
-        req.params.itemId,
+        itemId,
         priceSource
       ]);
 
@@ -155,9 +163,12 @@ router.post(
 router.post(
   '/:itemId/regenerate',
   asyncRoute(async (req: Request, res: Response) => {
+    const itemId = readId(req.params.itemId);
+    if (itemId === null) return res.status(404).json({ error: 'no draft for this item' });
+
     const { rowCount } = await pool.query(
       `UPDATE item_drafts SET state = 'queued', attempts = 0, ai_error = NULL WHERE item_id = $1`,
-      [req.params.itemId]
+      [itemId]
     );
     if (rowCount === 0) return res.status(404).json({ error: 'no draft for this item' });
 
@@ -185,18 +196,21 @@ router.post(
 router.post(
   '/:itemId/discard',
   asyncRoute(async (req: Request, res: Response) => {
+    const itemId = readId(req.params.itemId);
+    if (itemId === null) return res.status(404).json({ error: 'no draft for this item' });
+
     const client = await pool.connect();
     try {
       await client.query('BEGIN');
       const { rowCount } = await client.query(
         `UPDATE item_drafts SET state = 'discarded' WHERE item_id = $1`,
-        [req.params.itemId]
+        [itemId]
       );
       if (rowCount === 0) {
         await client.query('ROLLBACK');
         return res.status(404).json({ error: 'no draft for this item' });
       }
-      await client.query(`UPDATE items SET status = 'pending' WHERE id = $1`, [req.params.itemId]);
+      await client.query(`UPDATE items SET status = 'pending' WHERE id = $1`, [itemId]);
       await client.query('COMMIT');
       res.json({ state: 'discarded' });
     } catch (err) {
@@ -220,11 +234,14 @@ router.post(
 router.post(
   '/:itemId/restore',
   asyncRoute(async (req: Request, res: Response) => {
+    const itemId = readId(req.params.itemId);
+    if (itemId === null) return res.status(404).json({ error: 'no draft for this item' });
+
     const { rowCount } = await pool.query(
       `UPDATE item_drafts
        SET state = CASE WHEN ai_name IS NULL THEN 'failed' ELSE 'ready' END
        WHERE item_id = $1`,
-      [req.params.itemId]
+      [itemId]
     );
     if (rowCount === 0) return res.status(404).json({ error: 'no draft for this item' });
     res.json({ restored: true });
@@ -239,8 +256,8 @@ router.post(
  * so guessing one is not hard.
  */
 async function imageOfItem(
-  itemId: string,
-  imageId: string
+  itemId: number,
+  imageId: number
 ): Promise<{ image_path: string; original_image_path: string | null } | null> {
   const { rows } = await pool.query<{ image_path: string; original_image_path: string | null }>(
     `SELECT image_path, original_image_path
@@ -266,13 +283,20 @@ async function imageOfItem(
 router.post(
   '/:itemId/images/:imageId/remove-background',
   asyncRoute(async (req: Request, res: Response) => {
-    const { itemId = '', imageId = '' } = req.params;
+    // Both ids, before either reaches Postgres. A route carrying two of them
+    // can guard one and forget the other, and the forgotten one is a 500 rather
+    // than the 404 that "no such photo" actually means (#207).
+    const itemId = readId(req.params.itemId);
+    const imageId = readId(req.params.imageId);
+    if (itemId === null || imageId === null) {
+      return res.status(404).json({ error: 'no such photo on this item' });
+    }
     if ((await imageOfItem(itemId, imageId)) === null) {
       return res.status(404).json({ error: 'no such photo on this item' });
     }
 
     try {
-      await removeImageBackground(Number(imageId));
+      await removeImageBackground(imageId);
     } catch (err) {
       console.error(`[drafts] background removal for image ${imageId}:`, err);
 
@@ -321,14 +345,19 @@ router.post(
 router.post(
   '/:itemId/images/:imageId/restore-original',
   asyncRoute(async (req: Request, res: Response) => {
-    const { itemId = '', imageId = '' } = req.params;
+    const itemId = readId(req.params.itemId);
+    const imageId = readId(req.params.imageId);
+    if (itemId === null || imageId === null) {
+      return res.status(404).json({ error: 'this photo has no original to restore' });
+    }
+
     const existing = await imageOfItem(itemId, imageId);
     if (existing === null || existing.original_image_path === null) {
       return res.status(404).json({ error: 'this photo has no original to restore' });
     }
 
     try {
-      await restoreImageOriginal(Number(imageId));
+      await restoreImageOriginal(imageId);
     } catch (err) {
       // Narrow on purpose: only NoOriginalToRestoreError means "another
       // request already did this, the work is done". This precheck and

backend/tests/integration/adminItemNotFound.integration.test.ts

@@ -101,10 +101,63 @@ describe('admin item routes for an id that does exist', () => {
 
 describe('a non-numeric id on every admin item route', () => {
   // Each of these used to reach Postgres, raise 22P02 and surface as a 500.
+  // The block was named "every" while covering two; it now covers every POST
+  // route that takes an id, which is what makes the name true (#207).
   it.each([
     ['mark-sold', '/api/admin/items/abc/mark-sold'],
-    ['mark-available', '/api/admin/items/abc/mark-available']
+    ['mark-available', '/api/admin/items/abc/mark-available'],
+    ['unpublish', '/api/admin/items/abc/unpublish'],
+    ['regenerate', '/api/admin/item-drafts/abc/regenerate'],
+    ['discard', '/api/admin/item-drafts/abc/discard'],
+    ['restore', '/api/admin/item-drafts/abc/restore'],
+    ['remove-background', '/api/admin/item-drafts/abc/images/1/remove-background'],
+    ['restore-original', '/api/admin/item-drafts/abc/images/1/restore-original'],
+    // The image id, not the item id — a route with two of them can guard one
+    // and forget the other, and only a case each way would notice.
+    ['remove-background by image', '/api/admin/item-drafts/1/images/abc/remove-background'],
+    ['restore-original by image', '/api/admin/item-drafts/1/images/abc/restore-original']
   ])('%s answers 404', async (_name, path) => {
     expect((await request(app).post(path)).status).toBe(404);
   });
+
+  it('PUT answers 404', async () => {
+    const res = await request(app)
+      .put('/api/admin/items/abc')
+      .field('name', 'renamed')
+      .field('price', '10.00');
+
+    expect(res.status).toBe(404);
+  });
+
+  // Publish validates its body before it looks at the id, so this sends a
+  // valid one — otherwise the 400 would mask whether the id was ever handled.
+  it('publish answers 404', async () => {
+    const res = await request(app)
+      .post('/api/admin/item-drafts/abc/publish')
+      .send({ name: 'a name', description: '', priceCents: 1000 });
+
+    expect(res.status).toBe(404);
+  });
+
+  it.each([
+    ['DELETE an item', '/api/admin/items/abc'],
+    ['DELETE an image by item id', '/api/admin/items/abc/images/1'],
+    ['DELETE an image by image id', '/api/admin/items/1/images/abc']
+  ])('%s answers 404', async (_name, path) => {
+    expect((await request(app).delete(path)).status).toBe(404);
+  });
+});
+
+/**
+ * A well-formed id that is simply absent.
+ *
+ * Deliberately not the same question as a malformed one. DELETE stays 204 for
+ * an item that is not there: the method is idempotent and the caller's intent —
+ * that the item should not exist — is satisfied either way. What must not
+ * happen is a 500, which is what a malformed id used to produce.
+ */
+describe('a well-formed but absent id on the delete routes', () => {
+  it('DELETE answers 204 rather than failing', async () => {
+    expect((await request(app).delete(`/api/admin/items/${ABSENT}`)).status).toBe(204);
+  });
 });