diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 0ad0f70..4797d7a 100755 --- a/backend/src/routes/admin.ts +++ b/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(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]); - res.json(full[0]); + const { rows: full } = await pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [itemId]); + // 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( `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. - await notifyFavoritersOfSale([Number(req.params.id)], null); - res.json(rows[0]); + // customer, so everyone watching it hears about it. Sent only after the row + // is known to exist, so nobody is told about a sale that did not happen. + 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( `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; diff --git a/backend/src/utils.ts b/backend/src/utils.ts index c1425f9..9c64e76 100755 --- a/backend/src/utils.ts +++ b/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; +} diff --git a/backend/tests/integration/adminItemNotFound.integration.test.ts b/backend/tests/integration/adminItemNotFound.integration.test.ts new file mode 100644 index 0000000..eeb5278 --- /dev/null +++ b/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 { + 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); + }); +}); diff --git a/backend/tests/unit/readId.test.ts b/backend/tests/unit/readId.test.ts new file mode 100644 index 0000000..8b498b5 --- /dev/null +++ b/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(); + }); +});