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

backend/src/routes/admin.ts

@@ -71,6 +71,9 @@ const storage = multer.diskStorage({
   }
 });
 
+// Reviewed for #180. Bounding one request is only half the problem — see
+// discardUnlessAccepted below for the other half, which is bounding what the
+// volume accumulates across requests that were refused.
 const upload = multer({
   storage,
   limits: {
@@ -93,6 +96,12 @@ const upload = multer({
 // Reads only the leading bytes — enough to identify a format, not enough to
 // care how large the file is. The handle is closed before anything is unlinked,
 // because an open handle makes the unlink fail on Windows.
+//
+// Reviewed for #180. The path is not caller-controlled despite arriving from a
+// request: multer composes it from `destination`, which is a server constant,
+// and `filename`, which the storage above sets to `randomUUID()` plus an
+// extension looked up from the validated content type. The caller's
+// `originalname` is never consulted, so no part of the path traverses anywhere.
 async function readHead(filePath: string): Promise<Buffer> {
   const handle = await fs.open(filePath, 'r');
   try {
@@ -116,14 +125,43 @@ async function discardUploads(files: Express.Multer.File[]): Promise<void> {
   );
 }
 
+/**
+ * Removes a request's uploaded files unless the request actually succeeded.
+ *
+ * multer writes to disk before any route logic runs, and its own cleanup only
+ * covers errors it raised itself. Everything after that — a failed signature
+ * check, a malformed `category_id`, a database error, a dropped connection —
+ * previously left the bytes on the volume with nothing referencing them: no row
+ * to find them by, and no bound on how many could accumulate. Bounding the size
+ * of one upload does not help if every refused upload is kept forever (#180).
+ *
+ * Registered as soon as multer succeeds rather than at each `return`, so a
+ * route added later inherits it instead of having to remember it. That is the
+ * whole reason it is a hook and not a call: the failure it prevents is someone
+ * adding a fourth early return.
+ *
+ * `close` rather than `finish`, so an aborted connection is covered too, and
+ * `writableEnded` distinguishes a response that completed from one that never
+ * did — the latter is not a success however its status code reads.
+ */
+function discardUnlessAccepted(req: Request, res: Response): void {
+  res.on('close', () => {
+    if (res.writableEnded && res.statusCode < 400) return;
+    void discardUploads((req.files as Express.Multer.File[]) || []);
+  });
+}
+
 /**
  * Confirms each stored file actually is what it was declared to be.
  *
  * This cannot happen in multer's fileFilter, which runs before the stream has
  * been read — there are no bytes to look at yet. So the check runs after the
- * write, and a failure removes *every* file from the request rather than only
- * the offending one: a half-accepted upload would leave files on the volume
- * that the request was refused for.
+ * write.
+ *
+ * Checking only: removing the files is discardUnlessAccepted's job, and doing
+ * it here as well would unlink twice and log an ENOENT for every refused
+ * upload. That also covers the case this function used to miss — `readHead`
+ * itself throwing, which returned no message and so cleaned up nothing.
  *
  * Returns the message to refuse with, or null when everything checks out.
  */
@@ -133,7 +171,6 @@ async function verifyUploadedImages(req: Request): Promise<string | null> {
   for (const file of files) {
     const head = await readHead(file.path);
     if (!signatureMatches(file.mimetype, head)) {
-      await discardUploads(files);
       return `${file.originalname} does not contain ${file.mimetype} data`;
     }
   }
@@ -156,6 +193,11 @@ const uploadImages = (req: Request, res: Response, next: NextFunction) => {
       return next(err);
     }
 
+    // Every file is on disk by this point and multer will not clean up after
+    // itself again, so the bytes become this request's responsibility before
+    // anything else is allowed to fail.
+    discardUnlessAccepted(req, res);
+
     verifyUploadedImages(req)
       .then((problem) => {
         if (problem) {

backend/tests/integration/uploadValidation.integration.test.ts

@@ -153,3 +153,59 @@ describe('the uploads directory', () => {
     }
   });
 });
+
+/**
+ * Bounding what one request may write is only half of bounding what the volume
+ * accumulates. multer writes to disk before any route logic runs, so a request
+ * refused *after* the write leaves its bytes behind with nothing referencing
+ * them — no database row, no way to find them again, and no upper bound on how
+ * many an attacker with admin access can pile up. Found reviewing the security
+ * hotspots in this file (#180).
+ */
+describe('what it leaves on disk', () => {
+  it('removes the upload when the request is refused for its other fields', async () => {
+    const before = await storedFiles();
+
+    const res = await request(app)
+      .post('/api/admin/items')
+      .field('name', 'Photographed thing')
+      .field('description', '')
+      .field('price', '30')
+      // Valid image, invalid sibling field: the file is already written by the
+      // time the route reads this and returns 400.
+      .field('category_id', 'not-a-number')
+      .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' });
+
+    expect(res.status).toBe(400);
+    expect(await storedFiles()).toEqual(before);
+  });
+
+  it('removes the upload when the tags field is refused', async () => {
+    const before = await storedFiles();
+
+    const res = await request(app)
+      .post('/api/admin/items')
+      .field('name', 'Photographed thing')
+      .field('description', '')
+      .field('price', '30')
+      .field('tags', '[not json')
+      .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' });
+
+    expect(res.status).toBe(400);
+    expect(await storedFiles()).toEqual(before);
+  });
+
+  // The accepted case must not be swept up by the same cleanup: these files are
+  // the ones the item now points at.
+  it('keeps the upload when the request succeeds', async () => {
+    const before = await storedFiles();
+
+    const res = await createItem().attach('images', REAL_PNG, {
+      filename: 'photo.png',
+      contentType: 'image/png'
+    });
+
+    expect(res.status).toBe(200);
+    expect((await storedFiles()).length).toBe(before.length + 1);
+  });
+});