docs(security): record why the three SQL hotspots in admin.ts are safe (#180)
Linting / lint (pull_request) Successful in 2m4s
SonarQube Analysis / sonarqube (pull_request) Failing after 17m31s

The three open hotspots on the dashboard are `typescript:S2077`, "Formatting SQL queries is security-sensitive" — not the upload-path rules #180's review worked from. That review did reach the multer limits hotspot, which is marked Reviewed/Safe, and the refused-upload accumulation it uncovered was a real defect; it simply was not reviewing these three. The issue's scope note is where it went wrong. It says there is no string-built SQL, and that is true of the binding but not of the assembly — S2077 fires on the formatting.

All three are safe, and the reasoning now sits beside the code rather than only in SonarQube's UI, following the S5693 comment already in this file.

Two of them are a module constant plus a literal holding `$1`. `ADMIN_ITEM_SELECT` interpolates nothing of its own and the id is bound rather than formatted in — including on the update route, where the bound value is caller-supplied, which is exactly why it is a parameter.

The third is the one the rule is for, because `WHERE ${clauses.join(' AND ')}` really is assembled at run time. It holds because `buildItemFilterSql` composes only string literals written in `itemFilters.ts`, and the single interpolation inside any of them is `$${next}` — a placeholder index, a number, seeded from the startIndex argument and incremented locally, never derived from a filter value. A caller chooses which of six fixed fragments are joined and supplies every value in `params`, and neither becomes SQL. `parseItemFilters` rejecting malformed input above is defence in depth rather than the reason this holds.

Comments only, so nothing changes at run time. Backend build clean and 278 unit tests pass.

The dashboard half — marking the three Reviewed → Safe so the count reflects decisions made rather than pending — is still outstanding, which is why this refs rather than closes.

Refs #180

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-27 11:18:39 -05:00
co-authored by Claude Opus 5
parent fdb5b7a0be
commit 66ff354631
+20
View File
@@ -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 single interpolation
// inside any of them is `$${next}` — a placeholder index, a number, seeded
// from the startIndex argument and incremented locally. It is never 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) {