fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180) #183

Merged
bermudalamb merged 2 commits from feature/180-security-hotspots into main 2026-08-25 11:50:28 -05:00
2 Commits
Author SHA1 Message Date
bermudalamb d2e5274f8b Merge branch 'main' into feature/180-security-hotspots
Linting / lint (pull_request) Successful in 2m0s
SonarQube Analysis / sonarqube (pull_request) Failing after 21m57s
2026-08-25 11:50:14 -05:00
bermudalamb c07f8c1710 fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)
Linting / lint (pull_request) Successful in 1m59s
SonarQube Analysis / sonarqube (pull_request) Failing after 21m41s
SonarQube reported three security hotspots, all in `routes/admin.ts`. A hotspot is not a defect — it marks code that touches something security-sensitive and needs a human decision — so the work is a recorded review, with a change only where the review finds a real gap. It found one.

The gap: multer writes every file to disk before any route logic runs, and multer's own cleanup only covers errors it raised itself. Everything after that left the bytes behind with nothing referencing them. A request carrying a perfectly valid photograph and a malformed `category_id` is refused with a 400 after the write, and the file stays on the volume permanently — no database row to find it by, and no bound on how many can accumulate. The same held for a malformed `tags` field, for a database error rolling the transaction back, and for `readHead` itself throwing, which returned no message and so cleaned up nothing.

That is the substance of the limits the first hotspot points at. Bounding one request to 8 MB across six files does nothing if every refused request keeps its bytes for ever, and the admin API is the one surface where that is reachable.

The fix is a hook rather than a call at each `return`, registered the moment multer succeeds. A route added later inherits it instead of having to remember it, which matters because the failure being prevented is precisely someone adding a fourth early return. It listens on `close` rather than `finish` so an aborted connection is covered, and checks `writableEnded` so a response that never completed is not mistaken for a success whatever its status code reads.

`verifyUploadedImages` goes back to checking only. Removing the files there as well would unlink twice and log an ENOENT for every refused upload, and the single mechanism covers the case it used to miss.

The other two hotspots are safe, and now say why in the file rather than only in SonarQube's UI — following the precedent of the existing comment that names S5693 by rule number. The upload path is not caller-controlled despite arriving from a request: multer composes it from a server constant and a `randomUUID()` plus an extension looked up from the validated content type, so the caller's `originalname` never reaches the filesystem. That reasoning belongs next to the `fs.open` that depends on it.

Three tests, written first and failing first: a refused sibling field, a refused tags field, and the accepted case, which must not be swept up by the same cleanup. 254 integration and 278 unit tests pass.

Refs #180
2026-08-25 11:39:36 -05:00