Review and disposition the 3 SQL-injection hotspots in admin.ts — they block the quality gate #74

New Issue

2026-08-20T10:56:26-05:00

bermudalamb commented

2026-08-20 10:56:26 -05:00

SonarQube reports 3 open security hotspots, all in backend/src/routes/admin.ts, all category sql-injection with vulnerability probability HIGH, all TO_REVIEW:

Line	Message	Key
139	Make sure that executing SQL queries is safe here	`AaAR8Abx4ZoWEMGvzW6M`
174	Make sure that executing SQL queries is safe here	`AaAQKzce4ZoWEMGvzSeZ`
227	Make sure that executing SQL queries is safe here	`AaAQKzce4ZoWEMGvzSea`

A fourth hotspot at line 28 (dos, content-length limit) is already Reviewed / Safe and needs nothing.

They are blocking the gate

The quality gate condition new_security_hotspots_reviewed requires 100% and currently reads 0%. That is one of only two conditions failing — the other, new_coverage, was addressed by #61. So the gate cannot go green until these three are dispositioned, whatever the disposition turns out to be.

Worth being precise about what SonarQube means here: a hotspot is not a reported vulnerability. It is a location the analyser cannot prove safe on its own and wants a human to look at. "Review" is the required action; a code change is only required if the review finds a real problem.

I read all three, and my analysis is that they are false positives

Recorded here so whoever reviews them is checking my reasoning rather than starting cold. Please verify rather than take this at face value — a second reader is the entire point of the hotspot mechanism, and marking something safe on someone else's say-so defeats it.

Lines 174 and 227 are the same shape:

const { rows: full } = await pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]);

The only interpolation is ADMIN_ITEM_SELECT, a module constant in src/itemSelect.ts built from other module constants. The user-supplied value is bound through $1. The analyser flags it purely because the query text is assembled with a template literal.

Line 139 is the one that deserved real scrutiny, because its interpolation is not constant:

const { clauses, params } = buildItemFilterSql(filters, 1, null);
const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : '';
const { rows } = await pool.query(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params);

clauses is derived from user-controlled filters, so if it carried values this would be injectable. Reading buildItemFilterSql in src/itemFilters.ts, every branch does the same thing: push the value onto params, and emit a clause containing only a numbered placeholder, for example

params.push(filters.minPriceCents);
clauses.push(`i.price_cents >= $${next}`);

The three things interpolated into the final SQL are the module constant, literal clause fragments, and next — an integer counter starting from a hardcoded 1 and incremented internally. No user value reaches the query text on any path, including the recursive-CTE category clause and the tag-count clause.

What to do

Open each hotspot in SonarQube, check the reasoning above against the code, and mark it Reviewed → Safe with a short comment recording why.
Confirm new_security_hotspots_reviewed moves to 100% and the gate condition clears.

If any review disagrees with the analysis above, that turns into a real fix and this issue changes shape — say so on the issue rather than marking it safe.

Worth considering separately

These will keep coming back. Any new pool.query built with a template literal gets flagged, regardless of parameterisation, so the same review cost recurs on every route added this way. Two options if it becomes tiresome: extract a small helper that makes the parameterised shape explicit, or add a SonarQube issue-suppression rule scoped to this pattern. Neither belongs in this issue — the immediate need is to clear the gate.

Priority

Highest of the currently open issues, per the request — not because a vulnerability is believed to exist, but because these three are one of the two things holding the quality gate red, and the fix is a review rather than a change.

Note this repo has no labels defined, so the priority is not expressed as one. Say the word and I will add a label set.

SonarQube reports **3 open security hotspots**, all in `backend/src/routes/admin.ts`, all category `sql-injection` with vulnerability probability **HIGH**, all `TO_REVIEW`: | Line | Message | Key | | --- | --- | --- | | 139 | Make sure that executing SQL queries is safe here | `AaAR8Abx4ZoWEMGvzW6M` | | 174 | Make sure that executing SQL queries is safe here | `AaAQKzce4ZoWEMGvzSeZ` | | 227 | Make sure that executing SQL queries is safe here | `AaAQKzce4ZoWEMGvzSea` | A fourth hotspot at line 28 (`dos`, content-length limit) is already **Reviewed / Safe** and needs nothing. ## They are blocking the gate The quality gate condition `new_security_hotspots_reviewed` requires **100%** and currently reads **0%**. That is one of only two conditions failing — the other, `new_coverage`, was addressed by #61. So the gate cannot go green until these three are dispositioned, whatever the disposition turns out to be. Worth being precise about what SonarQube means here: a hotspot is **not** a reported vulnerability. It is a location the analyser cannot prove safe on its own and wants a human to look at. "Review" is the required action; a code change is only required if the review finds a real problem. ## I read all three, and my analysis is that they are false positives Recorded here so whoever reviews them is checking my reasoning rather than starting cold. **Please verify rather than take this at face value** — a second reader is the entire point of the hotspot mechanism, and marking something safe on someone else's say-so defeats it. **Lines 174 and 227** are the same shape: ```ts const { rows: full } = await pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]); ``` The only interpolation is `ADMIN_ITEM_SELECT`, a module constant in `src/itemSelect.ts` built from other module constants. The user-supplied value is bound through `$1`. The analyser flags it purely because the query text is assembled with a template literal. **Line 139** is the one that deserved real scrutiny, because its interpolation is not constant: ```ts const { clauses, params } = buildItemFilterSql(filters, 1, null); const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : ''; const { rows } = await pool.query(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params); ``` `clauses` is derived from user-controlled filters, so if it carried values this would be injectable. Reading `buildItemFilterSql` in `src/itemFilters.ts`, every branch does the same thing: push the value onto `params`, and emit a clause containing only a **numbered placeholder**, for example ```ts params.push(filters.minPriceCents); clauses.push(`i.price_cents >= $${next}`); ``` The three things interpolated into the final SQL are the module constant, literal clause fragments, and `next` — an integer counter starting from a hardcoded `1` and incremented internally. No user value reaches the query text on any path, including the recursive-CTE category clause and the tag-count clause. ## What to do 1. Open each hotspot in SonarQube, check the reasoning above against the code, and mark it **Reviewed → Safe** with a short comment recording why. 2. Confirm `new_security_hotspots_reviewed` moves to 100% and the gate condition clears. If any review disagrees with the analysis above, that turns into a real fix and this issue changes shape — say so on the issue rather than marking it safe. ## Worth considering separately These will keep coming back. Any new `pool.query` built with a template literal gets flagged, regardless of parameterisation, so the same review cost recurs on every route added this way. Two options if it becomes tiresome: extract a small helper that makes the parameterised shape explicit, or add a SonarQube issue-suppression rule scoped to this pattern. Neither belongs in this issue — the immediate need is to clear the gate. ## Priority Highest of the currently open issues, per the request — not because a vulnerability is believed to exist, but because these three are one of the two things holding the quality gate red, and the fix is a review rather than a change. Note this repo has **no labels defined**, so the priority is not expressed as one. Say the word and I will add a label set.

bermudalamb added this to the Code Quality and Hardening project 2026-08-20 11:00:52 -05:00

bermudalamb commented

2026-08-20 11:20:53 -05:00

Reviewed independently, and I agree — all three marked Reviewed → Safe

Taking the issue's instruction seriously that a second reader is the point, I read the code before reading the conclusion rather than checking the conclusion against the code.

Lines 174 and 227. Confirmed. The only interpolation is ADMIN_ITEM_SELECT, and src/itemSelect.ts builds it from three module-level string constants (IMAGES_SUBQUERY, TAGS_SUBQUERY, FROM_CLAUSE) with no user input anywhere in its construction. The id is bound through $1 on both.

Line 139. Confirmed, and it did deserve the scrutiny. I traced all six branches of buildItemFilterSql: category, tags, min price, max price, status, favorites. Every one pushes its value onto params and emits clause text containing only SQL literals plus $${next}. The recursive-CTE category clause interpolates next and nothing else; the tag-count clause interpolates next and next + 1 into ANY($n::int[]) and the equality, with the tag id array itself passed as a parameter. next starts from the hardcoded 1 at the call site and only ever increments internally. No user-controlled string reaches the query text on any path.

Worth adding to the record as defence in depth, since the issue's analysis stops at the SQL construction: by the time buildItemFilterSql runs, parseItemFilters has already forced every value through /^\d+$/ or an enum whitelist, and refuses anything else with a FilterError → 400. So even the bound parameters are integers and known status strings. Two independent reasons this is not injectable, and only one of them has to hold.

What was done

All three marked Reviewed → Safe via api/hotspots/change_status, each carrying a comment recording the reasoning above so the next reader is not starting cold either. Verified afterwards against api/hotspots/search: all three now read REVIEWED / SAFE, alongside the line-28 dos hotspot that was already dispositioned.

new_security_hotspots_reviewed now reads 100.0% and its gate condition reads OK. Overall security_hotspots_reviewed is 100% too.

One cosmetic blemish in the record: the em dash in two of the three SonarQube comments came through the API as a replacement character. The text is otherwise intact and readable, and SonarQube 9.9 has no working endpoint to edit a hotspot comment after the fact (api/issues/edit_comment 404s on hotspot comment keys), so it stays as is rather than being fixed by posting duplicate comments.

The gate is still red, and not for the reason this issue predicted

This issue says new_coverage "was addressed by #61". It was not — or at least not sufficiently. The gate reads:

Condition	Threshold	Actual	Status
`new_coverage`	≥ 80	71.2	ERROR
`new_security_hotspots_reviewed`	100	100.0	OK
`new_reliability_rating`	1	1	OK
`new_security_rating`	1	1	OK
`new_maintainability_rating`	1	1	OK
`new_duplicated_lines_density`	≤ 3	1.10	OK

So this issue's half is done and the gate is now one condition from green, but it will not go green until new-code coverage clears 80%. That is a separate piece of work and wants its own issue — flagging it here rather than widening this one. Also worth reading the 71.2% with the caution sonar-project.properties already records about the frontend number, which comes from an istanbul-instrumented Playwright run and marks a line covered merely because a browser executed it.

Q&A

Q: Who signs off on marking the three Reviewed → Safe — me via the API, or you in the UI, given the issue's own point that marking something safe on someone else's say-so defeats the mechanism?
A (Thom): I mark them via the API. Done under the admin token in the local environment, with the reasoning recorded on each hotspot.

Not done here

The issue's "worth considering separately" — a parameterised-query helper, or a scoped suppression rule — is untouched, as intended. Any new pool.query assembled with a template literal will be flagged again and cost another review.

## Reviewed independently, and I agree — all three marked Reviewed → Safe Taking the issue's instruction seriously that a second reader is the point, I read the code before reading the conclusion rather than checking the conclusion against the code. **Lines 174 and 227.** Confirmed. The only interpolation is `ADMIN_ITEM_SELECT`, and `src/itemSelect.ts` builds it from three module-level string constants (`IMAGES_SUBQUERY`, `TAGS_SUBQUERY`, `FROM_CLAUSE`) with no user input anywhere in its construction. The id is bound through `$1` on both. **Line 139.** Confirmed, and it did deserve the scrutiny. I traced all six branches of `buildItemFilterSql`: category, tags, min price, max price, status, favorites. Every one pushes its value onto `params` and emits clause text containing only SQL literals plus `$${next}`. The recursive-CTE category clause interpolates `next` and nothing else; the tag-count clause interpolates `next` and `next + 1` into `ANY($n::int[])` and the equality, with the tag id array itself passed as a parameter. `next` starts from the hardcoded `1` at the call site and only ever increments internally. No user-controlled string reaches the query text on any path. Worth adding to the record as defence in depth, since the issue's analysis stops at the SQL construction: by the time `buildItemFilterSql` runs, `parseItemFilters` has already forced every value through `/^\d+$/` or an enum whitelist, and refuses anything else with a `FilterError` → 400. So even the bound parameters are integers and known status strings. Two independent reasons this is not injectable, and only one of them has to hold. ## What was done All three marked **Reviewed → Safe** via `api/hotspots/change_status`, each carrying a comment recording the reasoning above so the next reader is not starting cold either. Verified afterwards against `api/hotspots/search`: all three now read `REVIEWED` / `SAFE`, alongside the line-28 `dos` hotspot that was already dispositioned. `new_security_hotspots_reviewed` now reads **100.0%** and its gate condition reads **OK**. Overall `security_hotspots_reviewed` is 100% too. One cosmetic blemish in the record: the em dash in two of the three SonarQube comments came through the API as a replacement character. The text is otherwise intact and readable, and SonarQube 9.9 has no working endpoint to edit a hotspot comment after the fact (`api/issues/edit_comment` 404s on hotspot comment keys), so it stays as is rather than being fixed by posting duplicate comments. ## The gate is still red, and not for the reason this issue predicted This issue says `new_coverage` "was addressed by #61". It was not — or at least not sufficiently. The gate reads: | Condition | Threshold | Actual | Status | | --- | --- | --- | --- | | `new_coverage` | ≥ 80 | **71.2** | ERROR | | `new_security_hotspots_reviewed` | 100 | 100.0 | OK | | `new_reliability_rating` | 1 | 1 | OK | | `new_security_rating` | 1 | 1 | OK | | `new_maintainability_rating` | 1 | 1 | OK | | `new_duplicated_lines_density` | ≤ 3 | 1.10 | OK | So this issue's half is done and the gate is now one condition from green, but it will not go green until new-code coverage clears 80%. That is a separate piece of work and wants its own issue — flagging it here rather than widening this one. Also worth reading the 71.2% with the caution `sonar-project.properties` already records about the frontend number, which comes from an istanbul-instrumented Playwright run and marks a line covered merely because a browser executed it. ## Q&A **Q: Who signs off on marking the three Reviewed → Safe — me via the API, or you in the UI, given the issue's own point that marking something safe on someone else's say-so defeats the mechanism?** **A (Thom): I mark them via the API.** Done under the admin token in the local environment, with the reasoning recorded on each hotspot. ## Not done here The issue's "worth considering separately" — a parameterised-query helper, or a scoped suppression rule — is untouched, as intended. Any new `pool.query` assembled with a template literal will be flagged again and cost another review.

bermudalamb closed this issue

2026-08-20 11:21:09 -05:00

bermudalamb referenced this issue

2026-08-20 11:25:14 -05:00

new_coverage is 71.2% against an 80% gate — and "new code" is silently the entire codebase #79

bermudalamb referenced this issue

2026-08-20 13:52:31 -05:00

Clear the 85 minutes of technical debt — four of the fourteen "smells" are real React defects #81

bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-20 17:46:45 -05:00

bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-20 17:46:50 -05:00

bermudalamb moved this to Released in Code Quality and Hardening on 2026-08-21 12:11:54 -05:00

Sign in to join this conversation.