Merge pull request 'fix(admin): answer 404 for an unreadable id instead of 500 (#207)' (#289) from fix/207-guard-every-admin-id into main

Reviewed-on: #289

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);
+  });
 });