diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index 6cb826c..da96f5f 100755 --- a/backend/src/routes/items.ts +++ b/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' }); diff --git a/backend/src/utils.ts b/backend/src/utils.ts index 7f7c833..b397d7a 100755 --- a/backend/src/utils.ts +++ b/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; } diff --git a/backend/tests/integration/errorHandling.integration.test.ts b/backend/tests/integration/errorHandling.integration.test.ts index 98a67e5..36e8fa5 100644 --- a/backend/tests/integration/errorHandling.integration.test.ts +++ b/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); }); }); diff --git a/backend/tests/unit/readId.test.ts b/backend/tests/unit/readId.test.ts index 8b498b5..542a4d2 100644 --- a/backend/tests/unit/readId.test.ts +++ b/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(); });