diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index ad98c98..ab43eee 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -239,6 +239,22 @@ export function parseItemFilters(query: Record): ItemFilters { // Returns WHERE fragments plus their parameters, with placeholders numbered // from `startIndex` so the caller can splice these in after its own params. // +// SECURITY INVARIANT, and it is load-bearing. Both callers splice these clauses +// straight into query text — admin.ts as `${ADMIN_ITEM_SELECT} ${where}`, and +// items.ts as `${PUBLIC_ITEM_SELECT} WHERE ${where}`, which is reachable +// without signing in. So the only thing that may ever be interpolated into a +// string pushed onto `clauses` is a placeholder index: `$${next}`, or +// `$${next + 1}` in the tags clause. Every value goes onto `params` and is +// bound by the driver. Interpolating a filter value here would be SQL injection +// at both call sites, and `parseItemFilters` refusing malformed input is not +// what prevents it — these literals would be safe with no parser at all. +// +// Stated here rather than only at the call sites because this is where the rule +// is enforced and where a seventh clause would be added. SonarQube raised S2077 +// on the call sites and they are marked Reviewed/Safe (#180); that marking does +// not re-raise when this file changes, so this comment and the two tests over +// it are what stand between that edit and a live injection. See #202. +// // `favoritesCustomerId` is required rather than optional so a caller has to say // whose favorites it means, even when it means nobody's. Both routes already // reject a favorites filter they cannot satisfy, so reaching the throw below is diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index dd3cfd8..34a66ce 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -340,6 +340,18 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => { return res.status(400).json({ error: 'favorites is not a valid inventory filter' }); } + // S2077 flags every query below that assembles its SQL as a template literal, + // and this is the one where that is more than a formality: `where` really is + // built at run time. What makes it safe is that buildItemFilterSql composes + // only string literals written in itemFilters.ts. The only interpolations + // inside any of them are placeholder indices — `$${next}`, and `$${next + 1}` + // in the tags clause — numbers, seeded from the startIndex argument and + // incremented locally. Neither is ever derived from a filter value. + // + // So a caller chooses which of six fixed fragments are joined, and supplies + // every value in `params`, and neither of those becomes SQL. parseItemFilters + // rejects malformed input above, but that is defence in depth rather than the + // reason this holds — the clause literals would be safe without it. const { clauses, params } = buildItemFilterSql(filters, 1, null); const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : ''; const { rows } = await pool.query(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params); @@ -367,6 +379,11 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons await setItemTags(client, item.id, await resolveTagIds(client, tagNames)); } await client.query('COMMIT'); + // S2077 again, and here the template is a module constant plus a literal: + // ADMIN_ITEM_SELECT interpolates nothing of its own, and the id is bound as + // $1 rather than formatted in. Same shape as the update route below, where + // the bound value is caller-supplied — which is precisely why it is a + // parameter. const { rows: full } = await pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]); res.json(requireRow(full, 'the item just inserted')); } catch (err) { @@ -415,6 +432,9 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp await insertItemImages(client, Number(req.params.id), files, nextSort); } await client.query('COMMIT'); + // 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]); } catch (err) { diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index 290c9ff..ee54a29 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -73,6 +73,14 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { }; const { clauses, params } = buildItemFilterSql(effectiveFilters, 1, req.customerId ?? null); + // The same construct SonarQube flagged as S2077 in admin.ts and which is + // marked Reviewed/Safe there (#180) — and this is the copy reachable without + // signing in, so it is worth saying here too rather than relying on the + // reader having seen the other one. It holds for the same reason: the clauses + // are literals from buildItemFilterSql carrying only placeholder indices, and + // EXCLUDE_PENDING is a module constant. Joining with AND cannot weaken + // EXCLUDE_PENDING either, because no fragment contains a top-level OR for the + // join to re-associate against. const where = [EXCLUDE_PENDING, ...clauses].join(' AND '); const { rows } = await pool.query( `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`, diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 845aaf6..5dce722 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -256,3 +256,52 @@ describe('buildItemFilterSql', () => { expect(sql).toContain('$3'); }); }); + +// Both callers splice these clauses straight into query text, so a value +// reaching the clause string is SQL injection rather than a style problem. The +// comment on buildItemFilterSql says so; these two make it fail a build instead +// of relying on someone reading it. See #202, and #180 for the S2077 review. +describe('buildItemFilterSql keeps every value out of the SQL text', () => { + // Deliberately built by hand rather than through parseItemFilters, because + // the claim is that the clause literals are safe with no parser at all. These + // values could never survive parsing, which is the point: the parser is + // defence in depth, not the reason this holds. + const HOSTILE = "1); DROP TABLE items; --"; + const hostileFilters = { + categoryIds: [HOSTILE], + tagIds: [HOSTILE], + minPriceCents: HOSTILE, + maxPriceCents: HOSTILE, + status: [HOSTILE], + favoritesOnly: true + } as unknown as Parameters[0]; + + it('never lets a filter value reach a clause, even one the parser would reject', () => { + const built = buildItemFilterSql(hostileFilters, 1, HOSTILE as unknown as number); + const sql = built.clauses.join(' AND '); + + expect(sql).not.toContain(HOSTILE); + expect(sql).not.toContain('DROP TABLE'); + // Every value still arrives, bound, where it can do nothing. + expect(built.params).toContain(HOSTILE); + }); + + // The structural version of the same claim, and the one that catches a value + // which happens not to look hostile: the SQL text must not depend on the + // values at all. Two disjoint sets of inputs, byte-identical clauses. + it('produces byte-identical SQL for two completely different filter sets', () => { + const a = buildItemFilterSql( + parseItemFilters({ category: '4', tags: '7,8', min_price: '100', max_price: '900', status: 'sold' }), + 1, + 42 + ); + const b = buildItemFilterSql( + parseItemFilters({ category: '99', tags: '11,12', min_price: '5', max_price: '6', status: 'available' }), + 1, + 7 + ); + + expect(a.clauses).toEqual(b.clauses); + expect(a.params).not.toEqual(b.params); + }); +});