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]);
+    const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [itemId]);
-    res.json(full[0]);
+    // 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.
+  // customer, so everyone watching it hears about it. Sent only after the row
-  await notifyFavoritersOfSale([Number(req.params.id)], null);
+  // is known to exist, so nobody is told about a sale that did not happen.
-  res.json(rows[0]);
+  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;

backend/src/utils.ts

@@ -91,3 +91,21 @@ export function trimTrailingSlashes(value: string): string {
   while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1);
   return trimmed;
 }
+
+/**
+ * A route's `:id` as a positive integer, or null when it is not one.
+ *
+ * Guarding this is not cosmetic. `Number('abc')` is NaN, which the driver sends
+ * to Postgres as the text "NaN"; Postgres raises 22P02 for an integer column,
+ * the route's catch turns that into a 500, and a caller asking for an item that
+ * cannot exist is told the server broke. Returning null lets the route answer
+ * 404, which is what "/items/abc" actually means. See #207.
+ *
+ * Rejects 0 and negatives as well as fractions: every id in this schema is a
+ * positive serial, so anything else identifies nothing.
+ */
+export function readId(value: string | undefined): number | null {
+  if (value === undefined || value.trim() === '') return null;
+  const parsed = Number(value);
+  return Number.isInteger(parsed) && parsed > 0 ? parsed : null;
+}

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

@@ -0,0 +1,110 @@
+import request from 'supertest';
+import app from '../../src/app';
+import { pool } from '../../src/db';
+import { resetDb, closeDb } from './setup/testDb';
+
+beforeEach(async () => {
+  await resetDb();
+});
+
+afterAll(async () => {
+  await pool.end();
+  await closeDb();
+});
+
+const PNG = Buffer.from(
+  'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
+  'base64'
+);
+
+/**
+ * Nothing exists, so every id below is absent. 999999 is well-formed and
+ * missing; 'abc' is not a number at all. Before #207 the first answered 200
+ * with an empty body and the second answered 500, because the raw string
+ * reached Postgres and raised 22P02.
+ */
+const ABSENT = 999999;
+
+describe('admin item routes for an id that does not exist', () => {
+  it('PUT answers 404 rather than 200 with an empty body', async () => {
+    const res = await request(app)
+      .put(`/api/admin/items/${ABSENT}`)
+      .field('name', 'renamed')
+      .field('price', '10.00');
+
+    expect(res.status).toBe(404);
+    expect(res.body).toEqual({ error: 'not found' });
+  });
+
+  it('mark-sold answers 404', async () => {
+    const res = await request(app).post(`/api/admin/items/${ABSENT}/mark-sold`);
+    expect(res.status).toBe(404);
+  });
+
+  it('mark-available answers 404', async () => {
+    const res = await request(app).post(`/api/admin/items/${ABSENT}/mark-available`);
+    expect(res.status).toBe(404);
+  });
+
+  // A garbage id used to reach Postgres and raise 22P02, which the catch turned
+  // into a 500. From the caller's side "/items/abc" identifies no item, exactly
+  // like "/items/999999" does.
+  it.each(['abc', '1.5', '-1', ''])('PUT answers 404 for the id %p', async (id) => {
+    const res = await request(app)
+      .put(`/api/admin/items/${id}`)
+      .field('name', 'renamed')
+      .field('price', '10.00');
+
+    expect(res.status).toBe(404);
+  });
+
+  it('mark-available answers 404 for a non-numeric id', async () => {
+    const res = await request(app).post('/api/admin/items/abc/mark-available');
+    expect(res.status).toBe(404);
+  });
+});
+
+describe('admin item routes for an id that does exist', () => {
+  async function makeItem(): Promise<number> {
+    const res = await request(app)
+      .post('/api/admin/items')
+      .field('name', 'a real item')
+      .field('price', '12.00')
+      .attach('images', PNG, 'a.png');
+    return res.body.id;
+  }
+
+  // The point of the change is to stop lying about misses, not to start
+  // refusing hits.
+  it('PUT still updates and answers with the item', async () => {
+    const id = await makeItem();
+
+    const res = await request(app)
+      .put(`/api/admin/items/${id}`)
+      .field('name', 'renamed')
+      .field('price', '15.00');
+
+    expect(res.status).toBe(200);
+    expect(res.body.name).toBe('renamed');
+    expect(res.body.price_cents).toBe(1500);
+  });
+
+  it('mark-available still publishes', async () => {
+    const id = await makeItem();
+
+    const res = await request(app).post(`/api/admin/items/${id}/mark-available`);
+
+    expect(res.status).toBe(200);
+    expect(res.body.status).toBe('available');
+  });
+});
+
+describe('a non-numeric id on every admin item route', () => {
+  // Each of these used to reach Postgres, raise 22P02 and surface as a 500.
+  it.each([
+    ['mark-sold', '/api/admin/items/abc/mark-sold'],
+    ['mark-available', '/api/admin/items/abc/mark-available']
+  ])('%s answers 404', async (_name, path) => {
+    expect((await request(app).post(path)).status).toBe(404);
+  });
+});

backend/tests/unit/readId.test.ts

@@ -0,0 +1,21 @@
+import { readId } from '../../src/utils';
+
+describe('readId', () => {
+  it('reads a positive integer', () => {
+    expect(readId('7')).toBe(7);
+    expect(readId('999999')).toBe(999999);
+  });
+
+  // Each of these used to be sent to Postgres as text, raising 22P02 for an
+  // integer column and surfacing to the caller as a 500 (#207).
+  it.each(['abc', '', '   ', '1.5', '-1', '0', 'NaN', '1e5abc'])(
+    'refuses %p',
+    (value) => {
+      expect(readId(value)).toBeNull();
+    }
+  );
+
+  it('refuses a missing param', () => {
+    expect(readId(undefined)).toBeNull();
+  });
+});