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
|
totalChunks
|
||||||
} = options;
|
} = 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
|
// Generate unique upload ID
|
||||||
const uploadId = crypto.randomUUID();
|
const uploadId = crypto.randomUUID();
|
||||||
|
|
||||||
@@ -43,7 +53,7 @@ async function initializeUpload(options) {
|
|||||||
// Store upload metadata
|
// Store upload metadata
|
||||||
const uploadMeta = {
|
const uploadMeta = {
|
||||||
uploadId,
|
uploadId,
|
||||||
filename,
|
filename: safeFilename,
|
||||||
fileSize,
|
fileSize,
|
||||||
mimeType,
|
mimeType,
|
||||||
eventId,
|
eventId,
|
||||||
@@ -59,7 +69,7 @@ async function initializeUpload(options) {
|
|||||||
|
|
||||||
logger.info('Initialized chunked upload', {
|
logger.info('Initialized chunked upload', {
|
||||||
uploadId,
|
uploadId,
|
||||||
filename,
|
filename: safeFilename,
|
||||||
fileSize,
|
fileSize,
|
||||||
expectedChunks,
|
expectedChunks,
|
||||||
eventId
|
eventId
|
||||||
|
|||||||
Reference in New Issue
Block a user