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.
This commit is contained in:
@@ -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/);
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user