docs(security): put the SQL injection invariant where it is enforced (#202)

#180 cleared the three `typescript:S2077` hotspots and marked them Reviewed/Safe on the dashboard, but the repository half never reached main — the branch carrying it was deleted before merge, so the markers are cleared, the issue is closed, and nothing in the code said why. That is the exact state #180 set out to avoid: "the justification has to live in the repository, not only in SonarQube's UI".

The three call-site comments are restored, with two corrections a review of the original found.

They were in the wrong file. `buildItemFilterSql` is where the rule actually lives: both callers splice its clauses straight into query text, so only a placeholder index may ever be interpolated into one and every value must go onto `params`. That function's header said nothing about it, and it is where a seventh clause would be added.

This matters more than ordinary comment placement because of how a cleared hotspot behaves. Reviewed/Safe stays marked and does not re-raise when a *different* file changes, so the one edit that would break this — interpolating a filter value in `itemFilters.ts` — was the one edit that would have got neither a warning nor a fresh marker.

`items.ts` gets the same note. It builds `${PUBLIC_ITEM_SELECT} WHERE ${where}` from the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one. It also records why joining with AND cannot weaken `EXCLUDE_PENDING`: no fragment carries a top-level OR for the join to re-associate against.

The wording was slightly false. "The single interpolation is `$${next}`" — the tags clause also interpolates `$${next + 1}`. Same category, so the argument is untouched, but a reader checking it literally finds a counter-example immediately, and a comment asserting safety cannot afford that.

Two tests make the invariant fail a build rather than depend on being read. One feeds values built by hand rather than parsed — `"1); DROP TABLE items; --"` in every field — and asserts none of it reaches the clause text, which states directly that these literals are safe with no parser at all. The other asserts two disjoint filter sets produce byte-identical SQL, which catches a value that happens not to look hostile.

Both were mutation-tested rather than assumed: interpolating `filters.minPriceCents` into the price clause — the precise edit the comment forbids — fails both, and one pre-existing test besides. Reverted, and the diff against main for `itemFilters.ts` is comment-only.

Verified: backend build clean, 280 unit tests pass.

Closes #202
Refs #180

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/src/itemFilters.ts

@@ -239,6 +239,22 @@ export function parseItemFilters(query: Record<string, unknown>): 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

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<AdminItemRow>(`${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<AdminItemRow>(`${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<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]);
     res.json(full[0]);
   } catch (err) {

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<PublicItemRow>(
     `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`,

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<typeof buildItemFilterSql>[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);
+  });
+});