From 31bc01cb4bbf65b48b3a5c3c94ad35e487df9fcc Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 16 Jul 2026 10:55:10 +0200 Subject: [PATCH] fix(security): sanitize chunked-upload filename (GHSA-pc72-jf53-w28j) The chunked video upload stored req.body.filename unmodified and later built the merged path as path.join(tempDir, uploadMeta.filename). path.join does not neutralise '../', so a filename like '../../uploads/logos/evil.svg' escaped the temp dir on merge and overwrote arbitrary files. Requires admin with photos.upload. Fix: path.basename() the client filename in initializeUpload() and reject names that collapse to nothing. Adds a regression test. --- .../services/chunkedUploadFilename.test.js | 52 +++++++++++++++++++ backend/src/services/chunkedUploadService.js | 14 ++++- 2 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 backend/__tests__/services/chunkedUploadFilename.test.js diff --git a/backend/__tests__/services/chunkedUploadFilename.test.js b/backend/__tests__/services/chunkedUploadFilename.test.js new file mode 100644 index 00000000..fd9fd34f --- /dev/null +++ b/backend/__tests__/services/chunkedUploadFilename.test.js @@ -0,0 +1,52 @@ +const path = require('path'); +const os = require('os'); +const fs = require('fs').promises; + +// Point storage at a throwaway temp dir before requiring the service so the +// module-level getStoragePath() picks it up if evaluated. +process.env.STORAGE_PATH = path.join(os.tmpdir(), `picpeak-chunk-test-${process.pid}`); + +const chunkedUpload = require('../../src/services/chunkedUploadService'); + +describe('chunkedUploadService.initializeUpload filename sanitisation (GHSA-pc72-jf53-w28j)', () => { + afterAll(async () => { + await fs.rm(process.env.STORAGE_PATH, { recursive: true, force: true }).catch(() => {}); + }); + + it('strips directory-traversal components from the stored filename', async () => { + const { uploadId } = await chunkedUpload.initializeUpload({ + filename: '../../uploads/logos/evil.svg', + fileSize: 10, + mimeType: 'video/mp4', + eventId: 1, + totalChunks: 1, + }); + const meta = chunkedUpload.getUploadStatus(uploadId); + // basename('../../uploads/logos/evil.svg') === 'evil.svg' — the traversal + // is gone, so path.join(tempDir, filename) can no longer escape tempDir. + expect(meta.filename).toBe('evil.svg'); + }); + + it('keeps a normal filename intact', async () => { + const { uploadId } = await chunkedUpload.initializeUpload({ + filename: 'clip.mp4', + fileSize: 10, + mimeType: 'video/mp4', + eventId: 1, + totalChunks: 1, + }); + expect(uploadId).toBeTruthy(); + }); + + it('rejects a filename that collapses to nothing', async () => { + await expect( + chunkedUpload.initializeUpload({ + filename: '../', + fileSize: 10, + mimeType: 'video/mp4', + eventId: 1, + totalChunks: 1, + }) + ).rejects.toThrow(/Invalid filename/); + }); +}); diff --git a/backend/src/services/chunkedUploadService.js b/backend/src/services/chunkedUploadService.js index 17cc8bb7..b3fe43ff 100644 --- a/backend/src/services/chunkedUploadService.js +++ b/backend/src/services/chunkedUploadService.js @@ -30,6 +30,16 @@ async function initializeUpload(options) { totalChunks } = options; + // Strip any directory components from the client-supplied filename. It is + // later joined onto the temp merge dir (path.join(tempDir, filename)), and + // path.join does NOT neutralise `../` — a filename like `../../uploads/ + // logos/evil.svg` would escape the temp dir and overwrite arbitrary files + // (GHSA-pc72-jf53-w28j). basename() collapses it to the leaf name only. + const safeFilename = path.basename(String(filename || '')); + if (!safeFilename || safeFilename === '.' || safeFilename === '..') { + throw new Error('Invalid filename'); + } + // Generate unique upload ID const uploadId = crypto.randomUUID(); @@ -43,7 +53,7 @@ async function initializeUpload(options) { // Store upload metadata const uploadMeta = { uploadId, - filename, + filename: safeFilename, fileSize, mimeType, eventId, @@ -59,7 +69,7 @@ async function initializeUpload(options) { logger.info('Initialized chunked upload', { uploadId, - filename, + filename: safeFilename, fileSize, expectedChunks, eventId