Review and clear the three security hotspots in routes/admin.ts #180
No Assignees
bermudalamb
bermudalamb
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: bermudalamb/redefined-designs#180
Reference in New Issue
Block a user
The first SonarQube analysis since #154 began — recovered by #174 — reports three security hotspots, all in
backend/src/routes/admin.ts.What a hotspot is, because it changes what "fix" means here
A hotspot is not a vulnerability. SonarQube raises one where code touches something security-sensitive and a human has to decide whether the use is safe in context. It stays open until someone reviews it and marks it Safe, Fixed, or Acknowledged. Three open hotspots therefore means three unreviewed decisions, not necessarily three defects.
That matters for this file in particular, because the security-sensitive machinery in it has already been hardened deliberately — #95 added the type allowlist, magic-byte verification and the derived extension, and the size and count limits carry a comment that names S5693 by rule number. The likely outcome is a recorded review confirming the existing mitigations, with a change only where the review finds a genuine gap.
The failure mode to avoid is the opposite of the usual one: changing working, well-reasoned code to silence a marker.
Scope
admin.tshas one concentration of security-sensitive code — the multer upload path — and nothing else that fits a hotspot rule. There are no regular expressions, no dynamic execution, no child processes, no cryptography beyondrandomUUID, and no string-built SQL. The eight routes are all parameterised queries.So the review covers:
fileSize,files,fields,fieldSize(S5693 territory).process.env.UPLOADS_DIRwith an/app/uploadsfallback.randomUUID()plus an extension taken from the validated content type rather than from the caller's filename.fs.openinreadHead,fs.unlinkindiscardUploads.What this should produce
For each hotspot: the rule, what it is pointing at, whether the current code is safe and why, and either a change or a justification. 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 — the existing S5693 comment is the model.
Then mark them reviewed in SonarQube, so the count reflects decisions made rather than decisions pending.
Note
The specific three rule keys are not visible from here —
SONAR_HOST_URLis a CI secret, so the dashboard cannot be queried from the repository. The review below works from the code, covering every hotspot-class rule that can apply to this file. If the three named on the dashboard differ from the ones identified, that is worth knowing, since a hotspot nobody expected is more interesting than one already understood.Reviewed, and one of the three was hiding a real gap
On
feature/180-security-hotspots.The limits — a real finding underneath
The limits block bounds one request properly: 8,000,000 bytes per file (chosen under S5693's threshold, as the existing comment says), six files, eight fields, 64 KB per field. Reviewing why that rule exists turned up the half it does not cover.
multer writes every file to disk before any route logic runs, and multer's own cleanup only fires for errors multer itself raised. Everything after that left the bytes behind:
category_id→ 400 after the write, file kept for evertagsfieldreadHeadthrowing → returned no refusal message, so the existing cleanup never ranNone of these leave a database row, so there is nothing to find the files by afterwards. Bounding one request to 8 MB is not much use if every refused request keeps its bytes permanently, and
POST /api/admin/itemsis reachable by anyone past the admin boundary.Reproduced before fixing — a valid PNG plus
category_id=not-a-numberreturns 400 and the file is still on the volume.Fixed with a hook rather than a call at each
return, registered as soon as multer succeeds, so a route added later inherits it instead of having to remember. The failure being prevented is exactly someone adding a fourth early return. It listens oncloserather thanfinishso an aborted connection is covered, and checkswritableEndedso a response that never completed is not read as a success whatever its status code says.verifyUploadedImagesgoes back to checking only — removing the files there too would unlink twice and log an ENOENT on every refused upload, and the one mechanism covers the case it used to miss.The other two — safe, and now say so in the file
The upload path is not caller-controlled, despite arriving from a request. multer composes it from
destination, a server constant, andfilename, which the storage sets torandomUUID()plus an extension looked up from the validated content type. The caller'soriginalnameis never consulted, so nothing traverses anywhere. That reasoning now sits next to thefs.openthat depends on it rather than only in SonarQube's UI, following the precedent of the comment that already names S5693 by number.The destination directory comes from
process.env.UPLOADS_DIR.UPLOADS_DIRis inALWAYS_REQUIRED, so a deployed process that lacks it refuses to start rather than falling back — the/app/uploadsdefault is reachable only in tests.Verification
Three tests written first and failing first: refused sibling field, refused tags field, and the accepted case, which must not be swept up by the same cleanup. 254 integration and 278 unit tests pass.
Still needs you
The three rule keys on the dashboard.
SONAR_HOST_URLis a CI secret so I cannot query it, and the review above works from the code. If the three named there are the ones I have described, the two safe ones can be marked Reviewed → Safe with the justifications above, and the third as Fixed. If any of them is something else, that one is more interesting than all of this — it would mean a sensitive path nobody has looked at.The three rule keys — and they are not the three that were reviewed
Queried the dashboard directly using
SONARQUBE_URLandSONARQUBE_TOKENfrom the dev machine's environment, which turns out to be reachable after all:GET /api/hotspots/search?projectKey=redefined-designs.The answer is the interesting one the issue was braced for. All three open hotspots are
typescript:S2077— "Formatting SQL queries is security-sensitive" — in the SQL, not the upload path....kvH4typescript:S2077...kvH5typescript:S2077...kvH6typescript:S2077AZ_9K1xD8DfNI3dztoq3So the DoS hotspot over the multer limits is the one that was genuinely closed, and the upload review in the previous comment — the path-traversal and
UPLOADS_DIRreasoning — was reviewing hotspots the dashboard was not raising. That work stands on its own (the refused-upload accumulation it uncovered was a real defect), but it did not touch these three.The scope note in the issue body is where this went wrong. It says "no string-built SQL. The eight routes are all parameterised queries." The queries are parameterised, but three of them are assembled as template literals, and S2077 fires on the formatting, not on the binding.
Where they are
The dashboard's line numbers are against revision
4ff713a, so they do not matchmain. Same three call sites:4ff713a)mainpool.query(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params)pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id])pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id])The review: all three are Safe
admin.ts:370andadmin.ts:418are a module constant concatenated with a literal containing$1.ADMIN_ITEM_SELECTis aconstinitemSelect.tswith no interpolation of its own. The value is bound, not formatted — at:418that value isreq.params.id, which is caller-controlled and goes through the driver as a parameter, which is the whole point. Nothing user-supplied reaches the query text.admin.ts:345is the one worth the rule's attention, becausewherereally is built at runtime:`WHERE ${clauses.join(' AND ')}`. TracingclausesintobuildItemFilterSql(itemFilters.ts:248-325) settles it. Every element pushed ontoclausesis a string literal written in that file. The only interpolation into any of them is$${next}— andnextis a number, seeded from thestartIndexargument and incremented locally. It is never derived from a filter value. Every user-supplied value is pushed ontoparamsand bound.Which clauses appear is caller-influenced; what any clause says is not. An attacker controlling
category_id,tags, price bounds orstatuscan change which of six fixed fragments are joined, and can put anything they like intoparams— and none of it becomes SQL.parseItemFiltersthrowsFilterErrorbefore that point for anything malformed, but that is defence in depth rather than what makes this safe; the clause literals would be safe even without it.Worth noting the same builder backs the storefront (
items.ts:75-76), so this reasoning covers both callers, and the admin route refusesfavoritesOnlyat:339-341rather than passing a null customer id into the clause that would throw.What is left
admin.tsnext to the three call sites, following the precedent of the S5693 comment, so the next person seeing the marker does not re-derive it. That is the repository half the issue asks for.Separately, and worth its own issue
The most recent analysis of the real project key was today at 09:16 against revision
4ff713a— the tip offeature/185-tag-chip-colour, notmain.mainisfdb5b7a. The dashboard is currently describing an old feature branch: itsadmin.tsdiffers frommain's by 67 insertions and 29 deletions, which is why the line numbers do not line up.This is exactly the hazard
scripts/scan-local.shwarns about in its header — Community edition has no branch analysis, so any scan under the real key replaces CI's picture ofmain. The workflow also triggers onpull_request, which has the same effect. So the quality gate and the hotspot list on the dashboard describe whatever was scanned last, notmain, and nothing says so.