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.
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>
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>
Closing unmerged: main already carries this, in a better form than the branch holds.
This branch is 66ff354, the commit rescued after the original #180 branch was deleted. It was superseded by #202 (c704c07), which restored the same three call-site comments and acted on the code review of them:
The load-bearing invariant moved to buildItemFilterSql in itemFilters.ts, which is where the rule is actually enforced and where a seventh clause would be added. A hotspot marked Reviewed/Safe does not re-raise when a different file changes, so that was the one edit that would have got neither a warning nor a fresh marker.
items.ts:76 gained the same note. It builds the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one.
The wording was corrected: this branch says "the single interpolation inside any of them is $${next}", and the tags clause also interpolates $${next + 1}.
Two tests were added that fail the build if a filter value ever reaches clause text, mutation-tested by interpolating filters.minPriceCents into the price clause.
Verified against current main: admin.ts carries all three S2077 comments and itemFilters.ts carries the invariant.
The conflict here is exactly that overlap — both sides edited the same region of admin.ts. Resolving it in this branch's favour would replace the reviewed text with the earlier draft and drop the two tests, so merging is the one outcome worth avoiding. Nothing is lost by closing.
Closing unmerged: `main` already carries this, in a better form than the branch holds.
This branch is `66ff354`, the commit rescued after the original #180 branch was deleted. It was superseded by #202 (`c704c07`), which restored the same three call-site comments **and** acted on the code review of them:
- The load-bearing invariant moved to `buildItemFilterSql` in `itemFilters.ts`, which is where the rule is actually enforced and where a seventh clause would be added. A hotspot marked Reviewed/Safe does not re-raise when a different file changes, so that was the one edit that would have got neither a warning nor a fresh marker.
- `items.ts:76` gained the same note. It builds the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one.
- The wording was corrected: this branch says "the single interpolation inside any of them is `$${next}`", and the tags clause also interpolates `$${next + 1}`.
- Two tests were added that fail the build if a filter value ever reaches clause text, mutation-tested by interpolating `filters.minPriceCents` into the price clause.
Verified against current `main`: `admin.ts` carries all three S2077 comments and `itemFilters.ts` carries the invariant.
The conflict here is exactly that overlap — both sides edited the same region of `admin.ts`. Resolving it in this branch's favour would replace the reviewed text with the earlier draft and drop the two tests, so merging is the one outcome worth avoiding. Nothing is lost by closing.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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_SELECTinterpolates 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 becausebuildItemFilterSqlcomposes only string literals written initemFilters.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 inparams, and neither becomes SQL.parseItemFiltersrejecting 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
Closing unmerged:
mainalready carries this, in a better form than the branch holds.This branch is
66ff354, the commit rescued after the original #180 branch was deleted. It was superseded by #202 (c704c07), which restored the same three call-site comments and acted on the code review of them:buildItemFilterSqlinitemFilters.ts, which is where the rule is actually enforced and where a seventh clause would be added. A hotspot marked Reviewed/Safe does not re-raise when a different file changes, so that was the one edit that would have got neither a warning nor a fresh marker.items.ts:76gained the same note. It builds the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one.$${next}", and the tags clause also interpolates$${next + 1}.filters.minPriceCentsinto the price clause.Verified against current
main:admin.tscarries all three S2077 comments anditemFilters.tscarries the invariant.The conflict here is exactly that overlap — both sides edited the same region of
admin.ts. Resolving it in this branch's favour would replace the reviewed text with the earlier draft and drop the two tests, so merging is the one outcome worth avoiding. Nothing is lost by closing.Pull request closed