docs(security): the S2077 justification never reached main, and belongs beside the invariant it describes #202

New Issue

2026-08-28T11:39:23-05:00

bermudalamb commented

2026-08-28 11:39:23 -05:00

#180 is closed and the three typescript:S2077 hotspots are marked Reviewed → Safe on the dashboard, but the repository half never landed. main contains no S2077 comment: the branch carrying it was deleted before merge and the commit (66ff354) survives only as an unreferenced object.

That is precisely the state #180 set out to avoid. Its own text: "The justification has to live in the repository, not only in SonarQube's UI, so that the next person to see the marker does not have to re-derive the reasoning." Right now the marker is cleared, the issue is closed, and nothing in the code says why.

Restore it, but not where it was

A code review of 66ff354 confirmed the Safe verdict independently — it swept all 33 files in backend/src and found the only interpolations reaching SQL text anywhere are module constants and numeric placeholder indices. It also found the comments were in the wrong file, which is worth fixing while restoring rather than after.

The invariant is asserted in admin.ts but enforced in itemFilters.ts. buildItemFilterSql is where the rule actually lives: every string pushed onto clauses is spliced into query text by its callers, so only placeholder indices may ever be interpolated there and every value must go on params. That function's header comment says nothing about it.

This matters more than ordinary comment placement because of how hotspots behave. A hotspot marked Reviewed → Safe stays marked; it is not re-raised when a different file changes. So the one edit that would break the invariant — adding a seventh clause in itemFilters.ts that interpolates a filter value — gets neither a warning comment nor a fresh hotspot. It is the single blind spot the marking creates.

items.ts:76-78 is the same construct and is undocumented. The public storefront does [EXCLUDE_PENDING, ...clauses].join(' AND ') into ${PUBLIC_ITEM_SELECT} WHERE ${where} — structurally identical to the flagged admin.ts site but reachable without authentication. SonarQube did not flag it, so it got no comment, leaving the higher-exposure copy as the undocumented one.

Also worth correcting while restoring

The comment says "the single interpolation inside any of them is $${next}". The tags clause at itemFilters.ts:286 also interpolates $${next + 1}. Same category — a placeholder index — so the argument is unaffected, but a reader checking the claim literally finds a counter-example on the first pass.

Scope

Restore the three call-site comments in admin.ts.
Put the load-bearing invariant on buildItemFilterSql, where a future editor will actually see it.
Note the same reasoning at items.ts:76.
Correct the $${next} wording to cover $${next + 1}.
Consider a unit test asserting no clause text ever contains a filter value, which turns the invariant from advisory into enforced. itemFilters.test.ts asserts clause content but nothing asserts the absence of values.

No behaviour change; comments and possibly one test.

Refs

#180 for the review and the dashboard state. Commit 66ff354 holds the original text and is preserved on feature/180-sql-justification-restore.

#180 is closed and the three `typescript:S2077` hotspots are marked Reviewed → Safe on the dashboard, but the repository half never landed. `main` contains no `S2077` comment: the branch carrying it was deleted before merge and the commit (`66ff354`) survives only as an unreferenced object. That is precisely the state #180 set out to avoid. Its own text: *"The justification has to live in the repository, not only in SonarQube's UI, so that the next person to see the marker does not have to re-derive the reasoning."* Right now the marker is cleared, the issue is closed, and nothing in the code says why. ## Restore it, but not where it was A code review of `66ff354` confirmed the Safe verdict independently — it swept all 33 files in `backend/src` and found the only interpolations reaching SQL text anywhere are module constants and numeric placeholder indices. It also found the comments were in the wrong file, which is worth fixing while restoring rather than after. **The invariant is asserted in `admin.ts` but enforced in `itemFilters.ts`.** `buildItemFilterSql` is where the rule actually lives: every string pushed onto `clauses` is spliced into query text by its callers, so only placeholder indices may ever be interpolated there and every value must go on `params`. That function's header comment says nothing about it. This matters more than ordinary comment placement because of how hotspots behave. A hotspot marked Reviewed → Safe stays marked; it is not re-raised when a *different* file changes. So the one edit that would break the invariant — adding a seventh clause in `itemFilters.ts` that interpolates a filter value — gets neither a warning comment nor a fresh hotspot. It is the single blind spot the marking creates. **`items.ts:76-78` is the same construct and is undocumented.** The public storefront does `[EXCLUDE_PENDING, ...clauses].join(' AND ')` into `${PUBLIC_ITEM_SELECT} WHERE ${where}` — structurally identical to the flagged `admin.ts` site but reachable **without authentication**. SonarQube did not flag it, so it got no comment, leaving the higher-exposure copy as the undocumented one. ## Also worth correcting while restoring The comment says "the single interpolation inside any of them is `$${next}`". The tags clause at `itemFilters.ts:286` also interpolates `$${next + 1}`. Same category — a placeholder index — so the argument is unaffected, but a reader checking the claim literally finds a counter-example on the first pass. ## Scope - Restore the three call-site comments in `admin.ts`. - Put the load-bearing invariant on `buildItemFilterSql`, where a future editor will actually see it. - Note the same reasoning at `items.ts:76`. - Correct the `$${next}` wording to cover `$${next + 1}`. - Consider a unit test asserting no clause text ever contains a filter value, which turns the invariant from advisory into enforced. `itemFilters.test.ts` asserts clause content but nothing asserts the absence of values. No behaviour change; comments and possibly one test. ## Refs #180 for the review and the dashboard state. Commit `66ff354` holds the original text and is preserved on `feature/180-sql-justification-restore`.

bermudalamb referenced this issue from a commit

2026-08-28 13:59:53 -05:00

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

bermudalamb referenced a pull request that will close this issue

2026-08-28 14:04:05 -05:00

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

bermudalamb closed this issue

2026-08-28 14:04:22 -05:00

bermudalamb referenced this issue from a commit

2026-08-28 14:04:24 -05:00

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

bermudalamb referenced this issue from a commit