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>
This commit is contained in:
2026-08-28 14:04:21 -05:00
co-authored by Claude Opus 5
parent 7421676568
commit c704c07b89
4 changed files with 93 additions and 0 deletions
+49
View File
@@ -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);
});
});