9cc82002b82dc27cd57a9badfb5114de2df5134d
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bermudalambandClaude Opus 5
|
0b6cc85c4f |
fix(lint): bring the backend test suites into scope (#298)
The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all. That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see. Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them. What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause. It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing. Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all. Closes #298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
2b8be434c0 |
test(uploads): stop the cleanup assertions racing the cleanup (#228)
discardUnlessAccepted unlinks from a res.on('close') handler, so nothing awaits it and nothing can. `await request()` resolves when the response completes, which is when close fires — so a read taken straight afterwards races the unlink it is meant to observe. The property is an eventual one and the assertions were synchronous.
The issue counted three tests. Injecting a 400ms delay into the unlink to make the race deterministic showed five: "leaves nothing on the volume when it refuses the content" and "discards the valid files from a request that also carried an invalid one" race too, and are not in the describe block the issue named.
It also showed that polling alone is not enough. The race runs in both directions, and the second direction is easy to miss: a deletion still pending from the *previous* test corrupts the next test's baseline before its request is even sent. No amount of waiting fixes a baseline that is already wrong. My first attempt waited for the directory to look quiet, which only works while the unlink is faster than the wait — precisely the assumption this issue is about, and it still failed four tests under the injected delay.
So the baseline is removed as a variable: beforeEach empties the directory, every test starts from empty, and the assertions poll for the expected count. Any orphan from an earlier suite goes with it, which is correct — the directory is temporary and nothing outside these tests owns it. discardUploads already catches per-file errors, so a pending unlink finding its file gone logs and moves on.
Verified by injecting the 400ms delay again: four to five failures before, twelve passing after, with the production file restored untouched.
Closes #228
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalamb
|
b616b9f0ab |
fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)
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 |
||
|
bermudalambandClaude Opus 5
|
35b242a66f |
feat(backend): accept only real images in the inventory upload (#95)
The upload bounded size and count and nothing else: POST /api/admin/items would take a PDF, a zip or an executable and store it as an item image, under an extension copied from whatever the caller named their file. Those files are served by express.static from the application's own origin, so a stored .html came back as text/html and a .svg as image/svg+xml — both able to run script as the site. Three types are accepted: JPEG, PNG and WebP. SVG is excluded deliberately even though it is an image, because it executes script when navigated to directly, which is the exposure #103 describes; a photograph of a one-of-a-kind item is never a vector drawing, so nothing real is lost. GIF is excluded as simply not wanted for product stills. Validation happens twice, because once is not enough. The declared content type is checked in multer's fileFilter, before a byte is written — that catches picking a PDF by accident, which is most of what goes wrong. But file.mimetype is whatever the caller wrote in the multipart headers, so the bytes are checked too: each stored file's leading bytes must match the format it claimed. That is what stops evil.html renamed to photo.jpg and declared image/jpeg, which an allowlist on the declared type alone waves straight through. The byte check cannot live in fileFilter — that runs before multer has read the stream, so there is nothing to look at yet. It runs after the write instead, and a failure removes every file from the request rather than only the offending one: accepting the good half of a refused upload would leave files on the volume that nothing references. Handles are closed before anything is unlinked, because an open handle makes the unlink fail on Windows. The stored name now takes its extension from the validated type rather than from path.extname(file.originalname), so the name on disk cannot disagree with what the file is. The random UUID is unchanged — that was already right, and its comment explains why. The picker offers exactly those three types rather than image/*, so a choice the API will refuse is not on the menu in the first place. That is a convenience, not a control: the operating system's All files option remains, drag-and-drop ignores accept, and anything calling the API directly never sees it. The server is the control. Nine integration tests, and they are the first in this project to upload real file content — which is why none of this was noticed. They cover a genuine PNG accepted, a PDF refused, SVG refused, HTML wearing image/jpeg refused, nothing left on the volume after a refusal, a mixed request discarding its valid file too, and no item created when the upload fails. Plus 21 unit tests on the pure signature checks, including a RIFF container that is not WebP. Verified: 162 unit, 178 integration, 94 end-to-end on a fresh container. Backend lint holds at 4 warnings — it caught the now-unused path import, which is exactly what it is for. Refs #95 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |