Merge pull request 'fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)' (#183) from feature/180-security-hotspots into main

Reviewed-on: #183

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);
+  });
+});