diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index a259678..dbe87bf 100755 --- a/backend/src/routes/admin.ts +++ b/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 { const handle = await fs.open(filePath, 'r'); try { @@ -116,14 +125,43 @@ async function discardUploads(files: Express.Multer.File[]): Promise { ); } +/** + * 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 { 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) { diff --git a/backend/tests/integration/uploadValidation.integration.test.ts b/backend/tests/integration/uploadValidation.integration.test.ts index d5b9b86..e54af36 100644 --- a/backend/tests/integration/uploadValidation.integration.test.ts +++ b/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); + }); +});