From 77b11ab874b6bdaae005e9699f45d1f24226cb6b Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 2 Sep 2026 09:33:34 +0200 Subject: [PATCH] fix(upload): enforce the chunked-upload cap on bytes received, not declared The init route checked the client-declared fileSize against general_max_file_size_mb, but nothing checked what then came through the chunk route: a client could declare `fileSize: 1` and stream any amount, and completeUpload only logged the size mismatch before handing the merged file on. The cap the earlier commit added at init was therefore a gate with no fence. The service now carries the cap from init and enforces it on the running byte total per chunk (aborting the upload once crossed, since the chunks on disk are already over the limit), rejects chunk indices outside the announced range, and re-checks the merged file as a backstop. Both routes answer 413/400 for these instead of a blanket 500. --- .../routes/adminPhotoUploadSizeLimit.test.js | 16 ++++ .../services/chunkedUploadSizeCap.test.js | 80 +++++++++++++++++++ backend/src/routes/adminPhotos.js | 11 ++- backend/src/services/chunkedUploadService.js | 59 +++++++++++++- 4 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 backend/__tests__/services/chunkedUploadSizeCap.test.js diff --git a/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js b/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js index f6a96bfd..212d1b7e 100644 --- a/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js +++ b/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js @@ -11,6 +11,8 @@ * - a file over the configured cap is rejected with a 400 naming the limit * - the chunked-upload init route honours the same cap (it would otherwise * be a trivial bypass of the multipart route's cap) + * - the chunk route enforces the cap on the bytes actually received, so a + * client can't declare `fileSize: 1` at init and stream past the limit * - a file under the cap still gets past the size gate * - the limit is read per request, so an admin raising it takes effect */ @@ -127,6 +129,20 @@ describe('admin upload per-file size limit (general_max_file_size_mb)', () => { expect(res.body.error).toBe('File too large. Maximum size is 1 MB per file.'); }); + it('rejects chunk bytes over the limit regardless of the declared fileSize', async () => { + await setLimitMb(1); + const initRes = await postChunkedInit(1); + expect(initRes.status).toBe(200); + + const res = await request(app) + .post(`/api/admin/photos/${eventId}/chunked-upload/${initRes.body.uploadId}/chunk/0`) + .set('Authorization', `Bearer ${adminToken}`) + .set('Content-Type', 'application/octet-stream') + .send(Buffer.alloc(2 * 1024 * 1024, 0x41)); + expect(res.status).toBe(413); + expect(res.body.error).toBe('File too large. Maximum size is 1 MB per file.'); + }); + it('lets a file under the limit past the size gate', async () => { await setLimitMb(1); // Junk bytes, so it still fails downstream on the content check — that is diff --git a/backend/__tests__/services/chunkedUploadSizeCap.test.js b/backend/__tests__/services/chunkedUploadSizeCap.test.js new file mode 100644 index 00000000..7b8a32b2 --- /dev/null +++ b/backend/__tests__/services/chunkedUploadSizeCap.test.js @@ -0,0 +1,80 @@ +/** + * The chunked-upload per-file cap has to hold on the bytes actually received, + * not on the client-declared `fileSize` the init route validates. Declaring + * `fileSize: 1` and then streaming 10 GB through the chunk route was a + * complete bypass of general_max_file_size_mb; the merge step only logged a + * size mismatch and processed the file anyway. + */ +const path = require('path'); +const os = require('os'); +const fs = require('fs').promises; + +process.env.STORAGE_PATH = path.join(os.tmpdir(), `picpeak-chunk-cap-test-${process.pid}`); + +const chunkedUpload = require('../../src/services/chunkedUploadService'); + +const MB = 1024 * 1024; + +const init = (overrides = {}) => chunkedUpload.initializeUpload({ + filename: 'clip.mp4', + fileSize: 1, + mimeType: 'video/mp4', + eventId: 1, + totalChunks: 2, + maxFileSizeBytes: 1 * MB, + ...overrides, +}); + +describe('chunkedUploadService per-file size cap', () => { + afterAll(async () => { + await fs.rm(process.env.STORAGE_PATH, { recursive: true, force: true }).catch(() => {}); + }); + + it('rejects a single chunk over the cap even when the declared fileSize is tiny', async () => { + const { uploadId } = await init(); + await expect(chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(2 * MB))) + .rejects.toMatchObject({ code: 'FILE_TOO_LARGE', statusCode: 413 }); + // Aborted, not merely rejected: the upload can no longer be completed. + expect(chunkedUpload.getUploadStatus(uploadId)).toBeNull(); + }); + + it('rejects when the running total across chunks crosses the cap', async () => { + const { uploadId } = await init(); + await chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(0.75 * MB)); + await expect(chunkedUpload.uploadChunk(uploadId, 1, Buffer.alloc(0.5 * MB))) + .rejects.toMatchObject({ code: 'FILE_TOO_LARGE' }); + }); + + it('counts a re-sent chunk once, not twice', async () => { + const { uploadId } = await init(); + await chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(0.6 * MB)); + // Same index again — replaces the earlier bytes, so the total stays 0.6 MB. + await expect(chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(0.6 * MB))).resolves.toBeTruthy(); + await expect(chunkedUpload.uploadChunk(uploadId, 1, Buffer.alloc(0.3 * MB))).resolves.toBeTruthy(); + }); + + it('rejects chunk indices outside the announced range', async () => { + const { uploadId } = await init(); + await expect(chunkedUpload.uploadChunk(uploadId, 2, Buffer.alloc(10))) + .rejects.toMatchObject({ code: 'INVALID_CHUNK', statusCode: 400 }); + await expect(chunkedUpload.uploadChunk(uploadId, -1, Buffer.alloc(10))) + .rejects.toMatchObject({ code: 'INVALID_CHUNK' }); + await expect(chunkedUpload.uploadChunk(uploadId, NaN, Buffer.alloc(10))) + .rejects.toMatchObject({ code: 'INVALID_CHUNK' }); + }); + + it('merges an upload under the cap and reports the real size', async () => { + const { uploadId } = await init(); + await chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(400 * 1024, 0x41)); + await chunkedUpload.uploadChunk(uploadId, 1, Buffer.alloc(400 * 1024, 0x42)); + const merged = await chunkedUpload.completeUpload(uploadId); + expect(merged.size).toBe(800 * 1024); + await fs.rm(merged.tempDir, { recursive: true, force: true }); + }); + + it('applies no cap when none is given', async () => { + const { uploadId } = await init({ maxFileSizeBytes: undefined, totalChunks: 1 }); + await expect(chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(3 * MB))).resolves.toBeTruthy(); + await chunkedUpload.abortUpload(uploadId); + }); +}); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 11d62298..e27451b4 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -1623,7 +1623,10 @@ router.post('/:eventId/chunked-upload/init', adminAuth, requirePermission('photo fileSize, mimeType, eventId: parseInt(eventId), - totalChunks + totalChunks, + // The declared fileSize check above is client-controlled; the service + // enforces this cap on the bytes it actually receives and merges. + maxFileSizeBytes: maxSize }); res.json(result); @@ -1648,6 +1651,9 @@ router.post('/:eventId/chunked-upload/:uploadId/chunk/:chunkIndex', adminAuth, r res.json(result); } catch (error) { + if (error.statusCode === 413 || error.statusCode === 400) { + return res.status(error.statusCode).json({ error: error.message }); + } logger.error('Error uploading chunk:', error); res.status(500).json({ error: error.message || 'Failed to upload chunk' }); } @@ -1690,6 +1696,9 @@ router.post('/:eventId/chunked-upload/:uploadId/complete', adminAuth, requirePer photos: uploadedPhotos }); } catch (error) { + if (error.statusCode === 413) { + return res.status(413).json({ error: error.message }); + } logger.error('Error completing chunked upload:', error); res.status(500).json({ error: error.message || 'Failed to complete upload' }); } diff --git a/backend/src/services/chunkedUploadService.js b/backend/src/services/chunkedUploadService.js index 20959f43..d0fc4711 100644 --- a/backend/src/services/chunkedUploadService.js +++ b/backend/src/services/chunkedUploadService.js @@ -16,6 +16,27 @@ const CHUNK_SIZE = 10 * 1024 * 1024; // Upload expiration: 24 hours const UPLOAD_EXPIRATION_MS = 24 * 60 * 60 * 1000; +function totalReceivedBytes(uploadMeta) { + let total = 0; + for (const size of uploadMeta.chunkSizes.values()) total += size; + return total; +} + +// Tagged errors so the routes can answer 413/400 instead of a blanket 500. +function fileTooLargeError(maxFileSizeBytes) { + const err = new Error(`File too large. Maximum size is ${Math.floor(maxFileSizeBytes / (1024 * 1024))} MB per file.`); + err.code = 'FILE_TOO_LARGE'; + err.statusCode = 413; + return err; +} + +function invalidChunkError(message) { + const err = new Error(message); + err.code = 'INVALID_CHUNK'; + err.statusCode = 400; + return err; +} + /** * Initialize a new chunked upload * @param {Object} options - Upload options @@ -27,7 +48,8 @@ async function initializeUpload(options) { fileSize, mimeType, eventId, - totalChunks + totalChunks, + maxFileSizeBytes } = options; // Strip any directory components from the client-supplied filename. It is @@ -50,6 +72,13 @@ async function initializeUpload(options) { // Calculate expected chunks const expectedChunks = totalChunks || Math.ceil(fileSize / CHUNK_SIZE); + // The per-file cap is enforced on the BYTES ACTUALLY RECEIVED, not on the + // client-declared fileSize the init route checks: a client can declare + // `fileSize: 1` and then stream whatever it likes through the chunk route. + // Missing/invalid cap means "no cap" (callers outside the admin routes). + const cap = Number(maxFileSizeBytes); + const sizeCap = Number.isFinite(cap) && cap > 0 ? cap : Infinity; + // Store upload metadata const uploadMeta = { uploadId, @@ -59,6 +88,9 @@ async function initializeUpload(options) { eventId, expectedChunks, receivedChunks: new Set(), + // Bytes per chunk index, so a re-sent chunk replaces rather than adds. + chunkSizes: new Map(), + maxFileSizeBytes: sizeCap, uploadDir, createdAt: Date.now(), expiresAt: Date.now() + UPLOAD_EXPIRATION_MS, @@ -107,12 +139,28 @@ async function uploadChunk(uploadId, chunkIndex, chunkData) { throw new Error('Upload expired'); } + // Only the announced chunk indices are valid — anything else would merge + // into nothing (a gap) or let more chunks in than the declared file has. + if (!Number.isInteger(chunkIndex) || chunkIndex < 0 || chunkIndex >= uploadMeta.expectedChunks) { + throw invalidChunkError(`Invalid chunk index ${chunkIndex}: expected 0-${uploadMeta.expectedChunks - 1}`); + } + + // Enforce the per-file cap on the running byte total. The upload is + // aborted, not just rejected: the chunks on disk are already over the + // limit and the client can't complete the file any more. + const receivedBytes = totalReceivedBytes(uploadMeta) - (uploadMeta.chunkSizes.get(chunkIndex) || 0) + chunkData.length; + if (receivedBytes > uploadMeta.maxFileSizeBytes) { + await abortUpload(uploadId); + throw fileTooLargeError(uploadMeta.maxFileSizeBytes); + } + // Write chunk to disk const chunkPath = path.join(uploadMeta.uploadDir, `chunk_${String(chunkIndex).padStart(6, '0')}`); await fs.writeFile(chunkPath, chunkData); // Mark chunk as received uploadMeta.receivedChunks.add(chunkIndex); + uploadMeta.chunkSizes.set(chunkIndex, chunkData.length); const progress = (uploadMeta.receivedChunks.size / uploadMeta.expectedChunks) * 100; @@ -184,6 +232,15 @@ async function completeUpload(uploadId) { }); } + // Backstop for the per-chunk running total above: the merged file is + // the number that matters, so it is the number that is checked last. + if (stats.size > uploadMeta.maxFileSizeBytes) { + await fs.rm(tempDir, { recursive: true, force: true }).catch(() => {}); + await fs.rm(uploadMeta.uploadDir, { recursive: true, force: true }).catch(() => {}); + activeUploads.delete(uploadId); + throw fileTooLargeError(uploadMeta.maxFileSizeBytes); + } + // Clean up chunks await fs.rm(uploadMeta.uploadDir, { recursive: true, force: true });