From d89401810f177aff77ba7cc341abae08e8b1a732 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Fri, 11 Sep 2026 08:30:19 +0200 Subject: [PATCH] fix(upload): stop buffering a chunk body before anything checks its size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chunk route drained the whole request into an array and concatenated it before calling uploadChunk — which is where every check lives. So a 300MB body sent against an unknown upload id was read in full, cost ~300MB of heap, and was only then answered with an error. The per-file cap was real but applied after the damage, and nothing looked at Content-Length at all. uploadChunk now takes the request stream itself and consumes nothing until the upload id, the chunk index and the declared Content-Length have all been checked against the remaining allowance. A body that clears those is streamed straight to the chunk file under a hard byte cap, so a sender that lies about its length — or sends none, the Transfer-Encoding: chunked case — is cut off at the allowance instead of being read to the end. A Buffer is still accepted, so the existing callers and chunkedUploadSizeCap tests are untouched. Worth noting the old code silently did nothing when handed a stream: fs.promises.writeFile accepts an async iterable, so the body was written while `chunkData.length` was undefined and the cap comparison was NaN > max, i.e. always false. Reachable only for an authenticated admin holding photos.upload on an event they own, and only once the CSRF gate lets application/octet-stream through. Relates to issue 1403 --- .../services/chunkedUploadStreaming.test.js | 150 ++++++++++++++++++ backend/src/routes/adminPhotos.js | 18 ++- backend/src/services/chunkedUploadService.js | 79 ++++++++- 3 files changed, 232 insertions(+), 15 deletions(-) create mode 100644 backend/__tests__/services/chunkedUploadStreaming.test.js diff --git a/backend/__tests__/services/chunkedUploadStreaming.test.js b/backend/__tests__/services/chunkedUploadStreaming.test.js new file mode 100644 index 00000000..8051ccc9 --- /dev/null +++ b/backend/__tests__/services/chunkedUploadStreaming.test.js @@ -0,0 +1,150 @@ +/** + * A rejected chunk must not cost its own size in memory (#1403). + * + * The route used to drain the whole request into an array and `Buffer.concat` + * it before calling uploadChunk, which is where every check lives — the + * per-file cap, the chunk index, and even "does this upload id exist". So a + * 300MB body against an unknown upload id was read in full, added ~300MB to + * RSS, and was then answered with an error. The size cap was real but only + * applied after the damage. + * + * The contract these tests pin: uploadChunk consumes NOTHING until every check + * has passed, and once it does start reading it stops at the remaining + * allowance rather than trusting the sender. + */ +const path = require('path'); +const os = require('os'); +const fs = require('fs').promises; +const { Readable } = require('stream'); + +process.env.STORAGE_PATH = path.join(os.tmpdir(), `picpeak-chunk-stream-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, +}); + +/** + * A readable that reports how much of it was actually pulled. Bytes are + * generated lazily, so "never read" really means the body never materialized. + */ +function countingSource(totalBytes, sliceSize = 64 * 1024) { + let remaining = totalBytes; + const source = new Readable({ + read() { + if (remaining <= 0) return this.push(null); + const n = Math.min(sliceSize, remaining); + remaining -= n; + source.bytesRead += n; + this.push(Buffer.alloc(n)); + }, + }); + source.bytesRead = 0; + return source; +} + +describe('chunked upload streams the body under a cap (#1403)', () => { + afterAll(async () => { + await fs.rm(process.env.STORAGE_PATH, { recursive: true, force: true }).catch(() => {}); + }); + + describe('refused before the body is read', () => { + it('reads nothing for an unknown upload id', async () => { + const source = countingSource(8 * MB); + await expect(chunkedUpload.uploadChunk('does-not-exist', 0, source, { declaredBytes: 8 * MB })) + .rejects.toThrow('Upload not found or expired'); + expect(source.bytesRead).toBe(0); + }); + + it('reads nothing for an out-of-range chunk index', async () => { + const { uploadId } = await init(); + const source = countingSource(8 * MB); + await expect(chunkedUpload.uploadChunk(uploadId, 99, source, { declaredBytes: 8 * MB })) + .rejects.toMatchObject({ code: 'INVALID_CHUNK', statusCode: 400 }); + expect(source.bytesRead).toBe(0); + }); + + it('reads nothing when Content-Length already exceeds the cap', async () => { + const { uploadId } = await init(); + const source = countingSource(8 * MB); + await expect(chunkedUpload.uploadChunk(uploadId, 0, source, { declaredBytes: 8 * MB })) + .rejects.toMatchObject({ code: 'FILE_TOO_LARGE', statusCode: 413 }); + expect(source.bytesRead).toBe(0); + expect(chunkedUpload.getUploadStatus(uploadId)).toBeNull(); + }); + + it('counts what earlier chunks already banked when checking Content-Length', async () => { + const { uploadId } = await init(); + await chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(0.75 * MB)); + const source = countingSource(0.5 * MB); + // 0.75MB banked + 0.5MB declared > the 1MB cap. + await expect(chunkedUpload.uploadChunk(uploadId, 1, source, { declaredBytes: 0.5 * MB })) + .rejects.toMatchObject({ code: 'FILE_TOO_LARGE', statusCode: 413 }); + expect(source.bytesRead).toBe(0); + }); + }); + + describe('a sender that lies, or says nothing', () => { + it('stops at the allowance instead of reading the whole body', async () => { + const { uploadId } = await init(); + // No declaredBytes at all — the Transfer-Encoding: chunked case. + const source = countingSource(8 * MB); + await expect(chunkedUpload.uploadChunk(uploadId, 0, source)) + .rejects.toMatchObject({ code: 'FILE_TOO_LARGE', statusCode: 413 }); + // The overshoot is whatever the readable had already buffered ahead when + // the cap tripped — a small constant tied to highWaterMark, NOT a + // function of the body size. That is the whole claim: 8MB offered, ~1MB + // read. The slack is deliberately loose so this doesn't turn into a + // Node-version canary. + expect(source.bytesRead).toBeLessThan(2 * MB); + }); + + it('leaves no partial chunk file behind when it cuts a body off', async () => { + const { uploadId } = await init(); + const meta = chunkedUpload.getUploadStatus(uploadId); + await expect(chunkedUpload.uploadChunk(uploadId, 0, countingSource(8 * MB))) + .rejects.toMatchObject({ statusCode: 413 }); + // abortUpload removes the whole directory; assert nothing survived it. + await expect(fs.readdir(path.join(process.env.STORAGE_PATH, 'chunks', uploadId))) + .rejects.toMatchObject({ code: 'ENOENT' }); + expect(meta).not.toBeNull(); + }); + }); + + describe('the happy path still works', () => { + it('writes a streamed chunk and reports progress', async () => { + const { uploadId } = await init(); + const result = await chunkedUpload.uploadChunk(uploadId, 0, countingSource(0.25 * MB), { + declaredBytes: 0.25 * MB, + }); + expect(result).toMatchObject({ chunkIndex: 0, received: 1, expected: 2, complete: false }); + + const chunkPath = path.join(process.env.STORAGE_PATH, 'chunks', uploadId, 'chunk_000000'); + expect((await fs.stat(chunkPath)).size).toBe(0.25 * MB); + }); + + it('still accepts a Buffer, the shape the service was written for', async () => { + const { uploadId } = await init(); + const result = await chunkedUpload.uploadChunk(uploadId, 0, Buffer.alloc(0.25 * MB)); + expect(result).toMatchObject({ chunkIndex: 0, received: 1 }); + }); + + it('lets a re-sent chunk replace itself without double-counting', async () => { + const { uploadId } = await init(); + await chunkedUpload.uploadChunk(uploadId, 0, countingSource(0.6 * MB), { declaredBytes: 0.6 * MB }); + // Same index again: the first copy's 0.6MB must not count toward the cap. + await expect( + chunkedUpload.uploadChunk(uploadId, 0, countingSource(0.6 * MB), { declaredBytes: 0.6 * MB }), + ).resolves.toBeTruthy(); + }); + }); +}); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index bab076d2..09f46c81 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -1701,14 +1701,16 @@ router.post('/:eventId/chunked-upload/:uploadId/chunk/:chunkIndex', adminAuth, r try { const { uploadId, chunkIndex } = req.params; - // Get chunk data from request body - const chunks = []; - for await (const chunk of req) { - chunks.push(chunk); - } - const chunkData = Buffer.concat(chunks); - - const result = await chunkedUpload.uploadChunk(uploadId, parseInt(chunkIndex), chunkData); + // The request stream is handed over unread (#1403). Every check — unknown + // upload id, bad index, the per-file cap against Content-Length — runs + // inside uploadChunk before a byte is consumed, and the body is then + // streamed to the chunk file under a hard cap rather than concatenated in + // memory. Buffering it first meant a rejected 300MB request still cost + // 300MB of heap. + const declaredBytes = Number(req.headers['content-length']); + const result = await chunkedUpload.uploadChunk(uploadId, parseInt(chunkIndex), req, { + declaredBytes: Number.isFinite(declaredBytes) ? declaredBytes : undefined, + }); res.json(result); } catch (error) { diff --git a/backend/src/services/chunkedUploadService.js b/backend/src/services/chunkedUploadService.js index 43ff475b..b67eb48e 100644 --- a/backend/src/services/chunkedUploadService.js +++ b/backend/src/services/chunkedUploadService.js @@ -115,14 +115,54 @@ async function initializeUpload(options) { }; } +/** + * Stream `source` into `chunkPath`, refusing to write more than `allowance` + * bytes (#1403). The cap is the backstop for a request that lies about its + * Content-Length or omits it: the moment the running total passes the + * allowance the source is destroyed and the partial file removed, so an + * oversized body costs the allowance rather than its own size. + */ +function writeChunkStream(source, chunkPath, allowance) { + const fsSync = require('fs'); + return new Promise((resolve, reject) => { + const out = fsSync.createWriteStream(chunkPath); + let written = 0; + let failed = null; + + const fail = (err) => { + if (failed) return; + failed = err; + source.destroy(); + out.destroy(); + fsSync.unlink(chunkPath, () => reject(err)); + }; + + source.on('data', (buf) => { + written += buf.length; + if (written > allowance) { + fail(Object.assign(new Error('CHUNK_OVER_ALLOWANCE'), { overAllowance: true })); + } + }); + source.on('error', fail); + out.on('error', fail); + out.on('finish', () => { if (!failed) resolve(written); }); + source.pipe(out); + }); +} + /** * Upload a single chunk * @param {string} uploadId - Upload ID * @param {number} chunkIndex - Chunk index (0-based) - * @param {Buffer} chunkData - Chunk data + * @param {Buffer|import('stream').Readable} source - Chunk bytes, or a stream + * of them (the request). A stream is never read until every check below has + * passed, so a rejected request costs nothing (#1403). + * @param {Object} [options] + * @param {number} [options.declaredBytes] - Content-Length, when the caller + * has one. Checked against the remaining allowance before the body is read. * @returns {Promise} - Chunk upload result */ -async function uploadChunk(uploadId, chunkIndex, chunkData) { +async function uploadChunk(uploadId, chunkIndex, source, { declaredBytes } = {}) { const uploadMeta = activeUploads.get(uploadId); if (!uploadMeta) { @@ -148,19 +188,44 @@ async function uploadChunk(uploadId, chunkIndex, chunkData) { // 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) { + // + // What this chunk may still contribute — everything already banked, minus a + // re-sent copy of this same index. Computed before the body is touched so a + // Content-Length that already blows the budget is refused having read zero + // bytes (#1403). + const bankedBytes = totalReceivedBytes(uploadMeta) - (uploadMeta.chunkSizes.get(chunkIndex) || 0); + const allowance = uploadMeta.maxFileSizeBytes - bankedBytes; + + if (Number.isFinite(declaredBytes) && declaredBytes > allowance) { 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); + let chunkLength; + + if (Buffer.isBuffer(source)) { + if (bankedBytes + source.length > uploadMeta.maxFileSizeBytes) { + await abortUpload(uploadId); + throw fileTooLargeError(uploadMeta.maxFileSizeBytes); + } + await fs.writeFile(chunkPath, source); + chunkLength = source.length; + } else { + try { + chunkLength = await writeChunkStream(source, chunkPath, allowance); + } catch (err) { + if (err.overAllowance) { + await abortUpload(uploadId); + throw fileTooLargeError(uploadMeta.maxFileSizeBytes); + } + throw err; + } + } // Mark chunk as received uploadMeta.receivedChunks.add(chunkIndex); - uploadMeta.chunkSizes.set(chunkIndex, chunkData.length); + uploadMeta.chunkSizes.set(chunkIndex, chunkLength); const progress = (uploadMeta.receivedChunks.size / uploadMeta.expectedChunks) * 100;