diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index ef53078..b7b37dc 100755 --- a/backend/src/routes/admin.ts +++ b/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( `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(`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(`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( `UPDATE items SET status='pending' WHERE id=$1 RETURNING *`, - [req.params.id] + [itemId] ); res.json(updated[0]); })); diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 5fd4c6d..04554c8 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/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 diff --git a/backend/tests/integration/adminItemNotFound.integration.test.ts b/backend/tests/integration/adminItemNotFound.integration.test.ts index eeb5278..d5472b7 100644 --- a/backend/tests/integration/adminItemNotFound.integration.test.ts +++ b/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); + }); });