fix(items): read the id strictly, and stop the error test needing a route that does not (#307)

Item 5. GET /api/items/:id was the last route reading its id with a bare Number(), so an unreadable id reached Postgres and came back to the caller as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207.

It could not be fixed on its own, which is why it stayed. errorHandling.integration.test.ts used this route's looseness as its way of making a handler reject: tightening the parse would have left that test green while removing the thing it tests. So the test now fails a database call directly, with a spy on pool.query against a route that makes one. That is the failure the error middleware actually exists for, and it does not depend on any route declining to validate — the previous comment's own conclusion, that moving the trigger to cart.ts would only move the wart.

Two things fixed along the way that the issue asked about but that switching to readId would not have delivered on its own.

readId was not as strict as its name suggests. Number reads 5.0, 1e2, 0x10 and +5 as 5, 100, 16 and 5 — every one a positive integer, so every check readId made passed and the route fetched a real row for a URL nobody wrote. /items/5.0 answered with item 5. This never raised an error and so never announced itself; the issue noticed it only because #308 converted the comparison to a real integer. An id is a string of digits, so it is matched against digits before being parsed.

readId is also now bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing, which is the same wrong answer to the caller as the 22P02 the function was written to prevent — a 500 for an id that identifies nothing.

The leak assertion was passing for the wrong reason. It checks the response does not contain "syntax" or "items", and the error it was checking against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against as well, so a future error format cannot slip through by wording itself differently.

Verified: tsc clean, lint 0 errors with no new warnings, 485 unit tests passing across 33 suites — seven of them new, covering the inputs above. The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say.

Item 4, coverage, is not closed by this and cannot be closed yet: the SonarQube scan step has been skipped on every recent run because it is gated on the earlier steps succeeding, and those steps were failing. The dashboard is therefore stale. Reported on the issue rather than guessed at.

Refs #307

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

backend/src/routes/items.ts

@@ -1,6 +1,7 @@
 import { Router, Request, Response } from 'express';
 import { asyncRoute } from '../asyncRoute';
 import { publicItemQuery, PublicItemRow, ItemContext } from '../itemSelect';
+import { readId } from '../utils';
 import {
   parseItemFilters,
   itemFilterExpressions,
@@ -93,16 +94,20 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => {
 }));
 
 router.get('/:id', asyncRoute(async (req: Request, res: Response) => {
-  // Number() rather than readId(), and that is deliberate rather than an
-  // oversight. readId would be stricter and would match every other id-taking
-  // route (#207) — but errorHandling.integration.test.ts drives this exact
-  // route with a non-numeric id to prove that asyncRoute plus the error
-  // middleware answer 500 rather than leaving the request hanging, and a
-  // stricter parse here would leave that test green while removing the thing
-  // it tests. Switching this over means giving that test another trigger in
-  // the same change. See #307.
+  // readId, like every other id-taking route since #207. This was the last one
+  // reading its id with a bare Number(), which meant an unreadable id reached
+  // Postgres and came back to the caller as a 500 for an item that cannot
+  // exist. 404 is what "/items/abc" actually means.
+  //
+  // It was left on Number() because errorHandling.integration.test.ts used this
+  // route's looseness as its way of making a handler reject. That test now
+  // fails a database call directly instead, so it no longer depends on a route
+  // declining to validate — which is what allowed this to be fixed (#307).
+  const id = readId(req.params.id);
+  if (id === null) return res.status(404).json({ error: 'not found' });
+
   const rows = await publicItemQuery()
-    .where('i.id', '=', Number(req.params.id))
+    .where('i.id', '=', id)
     .where((eb) => notPending(eb))
     .execute();
   if (!rows.length) return res.status(404).json({ error: 'not found' });

backend/src/utils.ts

@@ -133,9 +133,26 @@ export function trimTrailingSlashes(value: string): string {
  *
  * Rejects 0 and negatives as well as fractions: every id in this schema is a
  * positive serial, so anything else identifies nothing.
+ *
+ * Matched against decimal digits before parsing, because `Number` on its own is
+ * far more permissive than "is this an id" wants. It reads `5.0`, `1e2`, `0x10`
+ * and `+5` as 5, 100, 16 and 5 — each a positive integer, each passing the
+ * checks below, and each therefore fetching a real row for a URL nobody wrote.
+ * That is not a crash and so it never announced itself; #307 noticed it only
+ * because #308 converted the comparison to a real integer. An id is a string of
+ * digits, and anything else is a different request.
+ *
+ * Bounded at the top for the reason the whole function exists: the column is a
+ * 32-bit serial, so an id above that limit reaches Postgres as an out-of-range
+ * integer and raises 22003 — the same shape of failure as the 22P02 above, and
+ * the same wrong answer to the caller. Below the limit it is a 404.
  */
+const MAX_SERIAL_ID = 2147483647;
+
 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;
+  if (value === undefined) return null;
+  const trimmed = value.trim();
+  if (!/^\d+$/.test(trimmed)) return null;
+  const parsed = Number(trimmed);
+  return Number.isInteger(parsed) && parsed > 0 && parsed <= MAX_SERIAL_ID ? parsed : null;
 }

backend/tests/integration/errorHandling.integration.test.ts

@@ -12,23 +12,80 @@ afterAll(async () => {
   await closeDb();
 });
 
-describe('unexpected route failures', () => {
-  it('answers with 500 instead of leaving the request hanging', async () => {
-    // A non-numeric id reaches Postgres as `WHERE i.id = 'not-a-number'`, which
-    // raises invalid-input-syntax. Express 4 does not forward a rejected async
-    // handler on its own, so without the asyncRoute wrapper plus the error
-    // middleware this request never gets a response at all — and a hung request
-    // renders as an empty storefront rather than a visible failure.
-    const res = await request(app).get('/api/items/not-a-number');
+/**
+ * The message the failing query rejects with. Deliberately shaped like a real
+ * Postgres error and deliberately containing both words the leak test looks
+ * for, so that assertion is checking something rather than passing because the
+ * words happened not to appear.
+ */
+const DB_FAILURE = 'invalid input syntax for type integer while reading items';
 
-    expect(res.status).toBe(500);
-    expect(res.body.error).toBe('internal error');
+/**
+ * Fails the next database call, whichever route makes it.
+ *
+ * This used to be done by asking `/api/items/not-a-number` and relying on that
+ * route sending an unparseable id to Postgres. That worked, but it tied this
+ * test to one route declining to validate its input: fixing that route — which
+ * #307 wanted, and #207 had already done everywhere else — would have left this
+ * test green while removing the thing it tests.
+ *
+ * A rejected query is the failure the error middleware actually exists for, and
+ * it does not depend on any route being wrong. `pool` is a real object, so this
+ * spy is not sensitive to how the module is transpiled.
+ */
+function failNextQuery() {
+  return jest.spyOn(pool, 'query').mockRejectedValueOnce(new Error(DB_FAILURE));
+}
+
+describe('unexpected route failures', () => {
+  // Express 4 does not forward a rejected async handler on its own. Without the
+  // asyncRoute wrapper plus the error middleware this request never gets a
+  // response at all — and a hung request renders as an empty storefront rather
+  // than a visible failure, which is exactly how the 2026-08-17 incident
+  // presented.
+  it('answers with 500 instead of leaving the request hanging', async () => {
+    const spy = failNextQuery();
+    try {
+      const res = await request(app).get('/api/filters');
+
+      expect(res.status).toBe(500);
+      expect(res.body.error).toBe('internal error');
+    } finally {
+      spy.mockRestore();
+    }
   });
 
   it('does not leak the underlying database error to the client', async () => {
+    const spy = failNextQuery();
+    try {
+      const res = await request(app).get('/api/filters');
+
+      const body = JSON.stringify(res.body);
+      expect(body).not.toContain('syntax');
+      expect(body).not.toContain('items');
+      // The whole message, not only the words above, so a future error format
+      // cannot slip through by wording it differently.
+      expect(body).not.toContain(DB_FAILURE);
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
+  // The route that used to be this file's trigger. It answers 404 now rather
+  // than 500, because an id that cannot be read identifies nothing — asserted
+  // here so the behaviour that replaced the trigger is itself covered.
+  it('answers 404, not 500, for an id that cannot be read', async () => {
     const res = await request(app).get('/api/items/not-a-number');
 
-    expect(JSON.stringify(res.body)).not.toContain('syntax');
-    expect(JSON.stringify(res.body)).not.toContain('items');
+    expect(res.status).toBe(404);
+    expect(res.body.error).toBe('not found');
+  });
+
+  // These parse to positive integers and used to fetch real rows — /items/5.0
+  // answered with item 5 (#307). Never a crash, which is why it went unnoticed.
+  it.each(['5.0', '1e2', '0x10'])('answers 404 for %p rather than fetching a row', async (id) => {
+    const res = await request(app).get(`/api/items/${id}`);
+
+    expect(res.status).toBe(404);
   });
 });

backend/tests/unit/readId.test.ts

@@ -15,6 +15,31 @@ describe('readId', () => {
     }
   );
 
+  // These are the dangerous ones, because they were never a 500 and so never
+  // announced themselves (#307). Number reads each as a positive integer, so
+  // every check this function used to make passed and the route fetched a real
+  // row for a URL nobody wrote: /items/5.0 answered with item 5.
+  it.each(['5.0', '1e2', '0x10', '+5', '1_0'])(
+    'refuses %p, which parses to a positive integer but is not an id',
+    (value) => {
+      expect(readId(value)).toBeNull();
+    }
+  );
+
+  // The column is a 32-bit serial. Above that Postgres raises 22003 rather than
+  // returning nothing, which is the same wrong answer to the caller as the
+  // 22P02 this function was written to prevent.
+  it('refuses an id past the top of a 32-bit serial', () => {
+    expect(readId('2147483647')).toBe(2147483647);
+    expect(readId('2147483648')).toBeNull();
+    expect(readId('99999999999999999999')).toBeNull();
+  });
+
+  // Surrounding whitespace is a URL artefact rather than a different id.
+  it('reads an id with surrounding whitespace', () => {
+    expect(readId(' 7 ')).toBe(7);
+  });
+
   it('refuses a missing param', () => {
     expect(readId(undefined)).toBeNull();
   });