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 <paul@MacStudio-von-Paul.local>

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('<script>alert(document.domain)</script>');
+    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('<svg xmlns="http://www.w3.org/2000/svg"><rect width="1" height="1"/></svg>');
+    // 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('');
+  });
+});

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