From 00a5c3a0752bb30446f4cb1ed24dd95191a284c5 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:21:54 +0200 Subject: [PATCH] fix(backend): validate business-profile logo uploads by content, not filename (stable) (#1395) * fix(backend): validate business-profile logo uploads by content, not filename The upload route skipped the shared validateFileType() helper every sibling upload route uses, and derived the stored extension from the client-supplied filename. A file could declare an image MIME type while carrying an executable/HTML extension and arbitrary content, then be served same-origin via the mass-assignable logoPath field. * fix(backend): content-sniff business-profile logo uploads too fileFilter paired the claimed MIME type against the extension but never verified the actual bytes matched, unlike other upload routes that already call validateFileContent(). Defense-in-depth: the extension-confusion XSS itself was already closed (stored extension is derived from the validated MIME, not client input), this closes the remaining gap where declared-vs-actual content can still diverge. --------- Co-authored-by: Paul Nothaft --- .../businessProfileLogoUpload.test.js | 195 ++++++++++++++++++ backend/src/routes/adminBusinessProfile.js | 55 ++++- 2 files changed, 245 insertions(+), 5 deletions(-) create mode 100644 backend/__tests__/integration/businessProfileLogoUpload.test.js diff --git a/backend/__tests__/integration/businessProfileLogoUpload.test.js b/backend/__tests__/integration/businessProfileLogoUpload.test.js new file mode 100644 index 00000000..bf1c93e3 --- /dev/null +++ b/backend/__tests__/integration/businessProfileLogoUpload.test.js @@ -0,0 +1,195 @@ +/** + * POST /api/admin/business-profile/logo and PUT /api/admin/business-profile + * — GHSA-6wrv-9pr4-hhmw regression coverage. + * + * The upload route used to take the stored file extension straight from + * the client-supplied filename and only checked `file.mimetype` against an + * allowlist — a file could declare an image MIME type while carrying a + * `.html`/`.js` extension and arbitrary content, land in the same-origin + * `/uploads/logos` static mount, and execute as script. The mass-assignable + * `logoPath` field on PUT compounded it: an attacker could point the + * "logo" at any other uploaded file. + * + * These tests pin: + * (a) a MIME/extension mismatch is rejected at upload, + * (b) the extension actually written to disk always matches the + * validated MIME type, never the client-supplied filename, + * (c) legitimate PNG/JPEG/SVG uploads still succeed, + * (d) `logoPath` on PUT cannot be set to an arbitrary string pointing at + * another file, only to a path the upload route itself produced. + * + * Defense-in-depth (not a re-opening of the above): fileFilter only pairs + * the claimed MIME type against the extension — it can't see the bytes, + * since it runs before multer finishes writing the stream to disk. A file + * whose declared MIME/extension pair is valid but whose actual content + * doesn't match (e.g. a PNG-declared upload that isn't really a PNG) is + * now caught by validateFileContent() (magic-number check) after multer + * writes it, closing the gap where declared-vs-actual content diverges. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-bplogo-test-')); +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'db.sqlite'); +process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); +fs.mkdirSync(process.env.STORAGE_PATH, { recursive: true }); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'bplogo-route-test-secret'; + +const request = require('supertest'); +const { + bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken, buildRouteApp, +} = require('./helpers/crmDb'); + +// Real magic-number-prefixed payloads, for content-sniffing to accept. +const REAL_PNG_BYTES = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]), + Buffer.from('not a real png body, but the header is real'), +]); +const REAL_JPEG_BYTES = Buffer.concat([ + Buffer.from([0xFF, 0xD8, 0xFF]), + Buffer.from('not a real jpeg body, but the header is real'), +]); + +describe('business profile — logo upload content/extension validation', () => { + let db; + let cleanup; + let app; + let token; + + const uploadLogo = (buffer, filename, mimetype) => request(app) + .post('/api/admin/business-profile/logo') + .set('Authorization', `Bearer ${token}`) + .attach('logo', buffer, { filename, contentType: mimetype }); + + const put = (payload) => request(app) + .put('/api/admin/business-profile') + .set('Authorization', `Bearer ${token}`) + .send(payload); + + const get = () => request(app) + .get('/api/admin/business-profile') + .set('Authorization', `Bearer ${token}`); + + const profileOf = (res) => (res.body.data || res.body).profile; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + const { adminId } = await seedMinimal(db); + await assignAdminRole(db, adminId, 'super_admin'); + token = mintAdminToken(adminId); + app = buildRouteApp('/api/admin/business-profile', require('../../src/routes/adminBusinessProfile')); + }, 120000); + + afterAll(async () => { + if (cleanup) await cleanup(); + }); + + // fileFilter rejections surface via Express's generic error handler + // (the pre-existing behaviour of every sibling logo/favicon upload + // route in this codebase — none of them special-case multer's + // fileFilter `Error` into a 400 either), so the status code itself + // can be 400 or 500 depending on environment. What actually matters + // for GHSA-6wrv-9pr4-hhmw is that the request never succeeds and + // nothing with the dangerous extension is ever written to disk. + const logosDirFiles = () => { + const logosDir = path.join(process.env.STORAGE_PATH, 'uploads', 'logos'); + return fs.existsSync(logosDir) ? fs.readdirSync(logosDir) : []; + }; + + it('rejects an HTML/script payload disguised as an image via mismatched extension', async () => { + const evil = Buffer.from(''); + const res = await uploadLogo(evil, 'evil.html', 'image/svg+xml'); + expect(res.status).not.toBe(200); + expect(logosDirFiles().some((f) => f.endsWith('.html'))).toBe(false); + }); + + it('rejects a .js file disguised with an image MIME type', async () => { + const evil = Buffer.from('alert(1)'); + const res = await uploadLogo(evil, 'evil.js', 'image/png'); + expect(res.status).not.toBe(200); + expect(logosDirFiles().some((f) => f.endsWith('.js'))).toBe(false); + }); + + it('rejects a disallowed MIME type outright', async () => { + const res = await uploadLogo(Buffer.from('whatever'), 'file.pdf', 'application/pdf'); + expect(res.status).not.toBe(200); + expect(logosDirFiles().some((f) => f.endsWith('.pdf'))).toBe(false); + }); + + it('accepts a legitimate PNG upload and stores it with a .png extension', async () => { + const res = await uploadLogo(REAL_PNG_BYTES, 'logo.png', 'image/png'); + expect(res.status).toBe(200); + const logoPath = (res.body.data || res.body).logoPath; + expect(logoPath).toMatch(/^\/uploads\/logos\/pdf-logo-\d+\.png$/); + + const onDisk = path.join(process.env.STORAGE_PATH, logoPath.replace(/^\//, '')); + expect(fs.existsSync(onDisk)).toBe(true); + + expect(profileOf(await get()).logoPath).toBe(logoPath); + }); + + it('accepts a legitimate JPEG upload and stores it with a .jpg extension', async () => { + const res = await uploadLogo(REAL_JPEG_BYTES, 'logo.jpg', 'image/jpeg'); + expect(res.status).toBe(200); + const logoPath = (res.body.data || res.body).logoPath; + expect(logoPath).toMatch(/^\/uploads\/logos\/pdf-logo-\d+\.jpg$/); + }); + + it('rejects a PNG-declared upload whose bytes are not actually a PNG, and leaves nothing on disk', async () => { + const before = logosDirFiles(); + const res = await uploadLogo(Buffer.from('totally not a png'), 'logo.png', 'image/png'); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/content does not match/i); + + // No new file left behind: the rejected upload's own file was cleaned + // up, and every other file on disk (if any) is unchanged. + expect(logosDirFiles()).toEqual(before); + }); + + it('accepts a legitimate SVG upload and always stores it with a .svg extension, even under a spoofed filename', async () => { + const svg = Buffer.from(''); + // Client-declared filename ext is .svg here to pass validateFileType + // (mismatched ext is covered by the rejection tests above); the point + // of this test is that the ON-DISK extension comes from the MIME type + // lookup table, not path.extname(originalname). + const res = await uploadLogo(svg, 'vector-logo.svg', 'image/svg+xml'); + expect(res.status).toBe(200); + const logoPath = (res.body.data || res.body).logoPath; + expect(logoPath).toMatch(/^\/uploads\/logos\/pdf-logo-\d+\.svg$/); + }); + + it('rejects logoPath on PUT set to an arbitrary string pointing at another file', async () => { + const before = profileOf(await get()).logoPath; + + const res = await put({ logoPath: '/uploads/logos/cms-somepage-1234.png' }); + expect(res.status).toBe(400); + + expect(profileOf(await get()).logoPath).toBe(before); + }); + + it('rejects logoPath on PUT with a path-traversal payload', async () => { + const res = await put({ logoPath: '/uploads/logos/../../../../etc/passwd' }); + expect(res.status).toBe(400); + }); + + it('accepts logoPath on PUT when it matches the pattern this route itself writes', async () => { + const upload = await uploadLogo(REAL_PNG_BYTES, 'logo2.png', 'image/png'); + const uploadedPath = (upload.body.data || upload.body).logoPath; + + // Round-trip: PUT-ing back the exact value the upload endpoint + // returned (what the frontend's generic profile save does) must + // keep working. + const res = await put({ logoPath: uploadedPath }); + expect(res.status).toBe(200); + expect(profileOf(await get()).logoPath).toBe(uploadedPath); + }); + + it('still allows clearing logoPath with an empty string', async () => { + const res = await put({ logoPath: '' }); + expect(res.status).toBe(200); + expect(profileOf(await get()).logoPath).toBe(''); + }); +}); diff --git a/backend/src/routes/adminBusinessProfile.js b/backend/src/routes/adminBusinessProfile.js index 889f35d9..ef31a06e 100644 --- a/backend/src/routes/adminBusinessProfile.js +++ b/backend/src/routes/adminBusinessProfile.js @@ -23,6 +23,7 @@ const { requirePermission } = require('../middleware/permissions'); const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers'); const { getStoragePath } = require('../config/storage'); const { uploadedPdfLogoPath } = require('../utils/safePath'); +const { validateFileType, validateFileContent, ALLOWED_MEDIA_TYPES } = require('../utils/fileSecurityUtils'); const businessProfileService = require('../services/businessProfileService'); const { db } = require('../database/db'); const { validateIban } = require('../utils/iban'); @@ -95,6 +96,19 @@ const router = express.Router(); // but accepts SVG in addition to PNG / JPEG — the PDF renderer // rasterises SVGs to PNG on the fly via resolveLogoFile() so the // admin can drop a vector logo here and have it work in print. +// +// GHSA-6wrv-9pr4-hhmw: this route used to take the stored extension +// straight from `file.originalname` and only checked `file.mimetype` +// against an allowlist — a file could declare an image MIME type +// while carrying a `.html`/`.js` extension and arbitrary content, get +// served same-origin from /uploads/logos with that extension, and +// execute as script in the browser. Fixed the same way every sibling +// upload route (adminSettings.js, adminCMS.js) already does it: +// `validateFileType()` pairs the claimed MIME type against the +// extension, and the extension actually written to disk is looked up +// from the validated MIME type — never taken from client input. +const PDF_LOGO_ALLOWED_MIME_TYPES = ['image/png', 'image/jpeg', 'image/svg+xml']; + const pdfLogoStorage = multer.diskStorage({ destination: async (_req, _file, cb) => { const dir = path.join(getStoragePath(), 'uploads/logos'); @@ -102,7 +116,11 @@ const pdfLogoStorage = multer.diskStorage({ cb(null, dir); }, filename: (_req, file, cb) => { - const ext = path.extname(file.originalname) || '.png'; + // fileFilter (below) runs before this and already rejected any + // mimetype outside PDF_LOGO_ALLOWED_MIME_TYPES, so the lookup below + // always hits. The extension is derived from the validated MIME + // type, never from file.originalname. + const ext = ALLOWED_MEDIA_TYPES[file.mimetype]?.extensions[0] || '.png'; cb(null, `pdf-logo-${Date.now()}${ext}`); }, }); @@ -113,9 +131,11 @@ const pdfLogoUpload = multer({ // array-indexed field names, so reject any bracket-index field name. limits: { fileSize: 5 * 1024 * 1024, fieldArrayIndexLimit: 0 }, fileFilter: (_req, file, cb) => { - const allowed = ['image/png', 'image/jpeg', 'image/svg+xml']; - if (allowed.includes(file.mimetype)) cb(null, true); - else cb(new Error('Only PNG, JPEG and SVG logos are allowed')); + if (validateFileType(file.originalname, file.mimetype, PDF_LOGO_ALLOWED_MIME_TYPES)) { + cb(null, true); + } else { + cb(new Error('Only PNG, JPEG and SVG logos are allowed')); + } }, }); @@ -355,6 +375,19 @@ router.post( return res.status(400).json({ error: 'No logo file uploaded' }); } + // fileFilter above only pairs the claimed MIME type against the + // extension — it runs on the in-flight stream, before any bytes are + // written, so it can't inspect content. Content-sniff the bytes multer + // just wrote to disk (magic numbers) before trusting them; SVG has no + // magic-number check (validateFileContent returns true for it), it's + // protected by the CSP header instead. Matches the cleanup-then-reject + // pattern createFileUploadValidator() uses for other upload routes. + const contentIsValid = await validateFileContent(req.file.path, req.file.mimetype); + if (!contentIsValid) { + try { await fs.unlink(req.file.path); } catch (_) { /* ignore */ } + return res.status(400).json({ error: 'File content does not match its declared type' }); + } + // Clean up the previous PDF logo on disk if it was uploaded via // this same endpoint (matches the pdf-logo-* prefix). We leave // anything else untouched — the admin may have set logo_path to @@ -428,7 +461,19 @@ router.put( body('defaultLocale').optional({ values: 'falsy' }).isString().isLength({ max: 8 }), body('defaultQrFormat').optional({ values: 'falsy' }).isIn(['swiss', 'epc', 'none']), body('footerLine').optional({ values: 'falsy' }).isString().isLength({ max: 255 }), - body('logoPath').optional({ values: 'falsy' }).isString().isLength({ max: 512 }), + // GHSA-6wrv-9pr4-hhmw: logoPath is mass-assignable here, so it must + // only ever be settable to a path the POST /logo upload route itself + // produced (or '' to clear it, allowed by `values: 'falsy'` above) — + // not an arbitrary string chaining in a file uploaded elsewhere. + // uploadedPdfLogoPath() is the same pattern check the delete/replace + // cleanup path already trusts to name a file this route wrote. + body('logoPath').optional({ values: 'falsy' }).isString().isLength({ max: 512 }) + .custom((value) => { + if (!uploadedPdfLogoPath(value, getStoragePath())) { + throw new Error('logoPath must be a path produced by the logo upload endpoint'); + } + return true; + }), // Bundled-fonts dropdown (migration 121). Free-text upload field // (pdfFontTtfPath, migration 103) was retired from the UI in // favour of this dropdown; the column stays in the DB so any