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