From e84835a0fb4ad0a83eae95e3f29f091ad76ebaaa Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 21:41:56 -0500 Subject: [PATCH] Derive uploaded file extension from mimetype, not originalname (#11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The multer filename kept path.extname(file.originalname), while the fileFilter only checked the spoofable client-supplied mimetype. An attacker could send Content-Type: image/png with originalname x.html, landing an .html file in /uploads that express.static serves as text/html — same-origin stored XSS. - Store the extension from a whitelist keyed by the accepted mimetype (MIME_EXT), never from originalname. The fileFilter uses the same map as its single source of truth, so only mimetypes with a safe mapped extension pass. - Use crypto.randomBytes for the random filename component. - Serve /uploads with an explicit X-Content-Type-Options: nosniff (defense in depth alongside helmet's global setting). Co-Authored-By: Claude Opus 4.8 --- server/src/app.js | 12 ++++++++++-- server/src/router/v1/admin/admin.routes.js | 20 +++++++++++++++++--- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/server/src/app.js b/server/src/app.js index 8f70ef0..beecc3b 100644 --- a/server/src/app.js +++ b/server/src/app.js @@ -51,8 +51,16 @@ const UPLOAD_DIR = process.env.UPLOAD_DIR || path.join(SERVER_ROOT, 'uploads') const CLIENT_DIST = path.join(REPO_ROOT, 'client', 'dist') fs.mkdirSync(UPLOAD_DIR, { recursive: true }) -// Uploaded images — always served, even during maintenance. -app.use('/uploads', express.static(UPLOAD_DIR)) +// Uploaded images — always served, even during maintenance. Force nosniff so a +// stored file is never interpreted as anything other than its declared type +// (defense in depth alongside helmet's global X-Content-Type-Options, and in +// case that global config is ever changed). +app.use( + '/uploads', + express.static(UPLOAD_DIR, { + setHeaders: (res) => res.set('X-Content-Type-Options', 'nosniff'), + }), +) // ── API ─────────────────────────────────────────────────────────────── app.get('/api/health', (req, res) => res.json({ status: 'ok' })) diff --git a/server/src/router/v1/admin/admin.routes.js b/server/src/router/v1/admin/admin.routes.js index 83de1d6..f53c0c1 100644 --- a/server/src/router/v1/admin/admin.routes.js +++ b/server/src/router/v1/admin/admin.routes.js @@ -1,6 +1,7 @@ const express = require('express') const path = require('path') const fs = require('fs') +const crypto = require('crypto') const multer = require('multer') const { body, param } = require('express-validator') @@ -23,18 +24,31 @@ const UPLOAD_DIR = process.env.UPLOAD_DIR || path.join(__dirname, '..', '..', '..', '..', 'uploads') fs.mkdirSync(UPLOAD_DIR, { recursive: true }) +// Whitelisted image mimetypes → the extension we store the file under. The +// stored extension is derived from this map (keyed by the accepted mimetype), +// never from originalname — so a spoofed `Content-Type: image/png` paired with +// `originalname: x.html` can never land an executable .html file in /uploads. +const MIME_EXT = { + 'image/png': '.png', + 'image/jpeg': '.jpg', + 'image/gif': '.gif', + 'image/webp': '.webp', + 'image/avif': '.avif', +} + const storage = multer.diskStorage({ destination: (req, file, cb) => cb(null, UPLOAD_DIR), filename: (req, file, cb) => { - const ext = path.extname(file.originalname).toLowerCase() - cb(null, `${Date.now()}-${Math.round(Math.random() * 1e9)}${ext}`) + const ext = MIME_EXT[file.mimetype] || '' + cb(null, `${Date.now()}-${crypto.randomBytes(8).toString('hex')}${ext}`) }, }) const upload = multer({ storage, limits: { fileSize: 8 * 1024 * 1024 }, fileFilter: (req, file, cb) => { - if (/^image\/(png|jpe?g|gif|webp|avif)$/.test(file.mimetype)) cb(null, true) + // Single source of truth: only mimetypes we can map to a safe extension pass. + if (MIME_EXT[file.mimetype]) cb(null, true) else cb(new Error('Only image uploads are allowed')) }, })