fix(security): contain logo, favicon and PDF-logo unlinks to their upload directories

Settings > Branding persisted logo_url / favicon_url verbatim and on clear
unlinked path.join(storage, url) behind a startsWith('/uploads/logos/')
check, which '..' segments pass. The business-profile PDF logo did the same
behind a /pdf-logo-\d+\./ marker test, and used absolute values as given.
Either let a settings.edit or settings.banking holder delete any file the
process can reach.

Both now resolve through helpers in utils/safePath that only ever name a
flat leaf inside the fixed directory. The /favicon.ico streamer is narrowed
the same way: it contained to the whole uploads/ root, which also holds
signed contracts and transfer files.

(cherry picked from commit 3e46530072)

backend/__tests__/utils/safePathUploadedAssets.test.js

@@ -0,0 +1,59 @@
+/**
+ * Containment for the two admin-writable "delete the old file" paths.
+ *
+ * Settings → Branding persists logo_url / favicon_url verbatim and, on
+ * clear, unlinked `path.join(storage, url)` after a mere prefix check.
+ * Business profile did the same for logo_path behind a `/pdf-logo-\d+\./`
+ * marker. Both let an admin delete any file the process can reach. The
+ * helpers below only ever name a flat leaf inside the fixed directory.
+ */
+const path = require('path');
+const { uploadedAssetPath, uploadedPdfLogoPath } = require('../../src/utils/safePath');
+
+const root = '/srv/picpeak/storage';
+
+describe('uploadedAssetPath', () => {
+  it('resolves a flat leaf inside the named upload directory', () => {
+    expect(uploadedAssetPath('/uploads/logos/logo-1.png', 'logos', root))
+      .toBe(path.join(root, 'uploads', 'logos', 'logo-1.png'));
+    expect(uploadedAssetPath('/uploads/favicons/fav.ico', 'favicons', root))
+      .toBe(path.join(root, 'uploads', 'favicons', 'fav.ico'));
+  });
+
+  it.each([
+    '/uploads/logos/../../../data/picpeak.db',
+    '/uploads/logos/..',
+    '/uploads/logos/',
+    '/uploads/logos/sub/dir.png',
+    '/uploads/favicons/x.ico', // wrong kind
+    'uploads/logos/logo.png', // not /-rooted
+    'https://example.com/uploads/logos/logo.png',
+    '',
+    null,
+    42,
+  ])('refuses %p', (value) => {
+    expect(uploadedAssetPath(value, 'logos', root)).toBeNull();
+  });
+});
+
+describe('uploadedPdfLogoPath', () => {
+  it('resolves the file the upload route writes', () => {
+    expect(uploadedPdfLogoPath('/uploads/logos/pdf-logo-1700000000000.png', root))
+      .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1700000000000.png'));
+    expect(uploadedPdfLogoPath('uploads/logos/pdf-logo-1.svg', root))
+      .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1.svg'));
+  });
+
+  it.each([
+    'pdf-logo-1./../../../../etc/target',
+    '/uploads/logos/pdf-logo-1./../../secret',
+    '/etc/pdf-logo-1.x',
+    '/uploads/logos/pdf-logo-1.png/../other',
+    '/uploads/logos/other-logo.png',
+    '/uploads/contracts/signed/pdf-logo-1.pdf',
+    '',
+    null,
+  ])('refuses %p', (value) => {
+    expect(uploadedPdfLogoPath(value, root)).toBeNull();
+  });
+});

backend/server.js

@@ -716,10 +716,15 @@ app.get(
         // whereas Firefox/Chrome do — so a 302 worked everywhere except
         // Safari. sendFile sets the right content-type from the extension.
         const rel = String(url).replace(/^\/+/, '').replace(/^uploads\//, '');
+        // Containment is the two public asset trees, not the whole uploads/
+        // root: that root also holds signed contracts and client transfer
+        // files, and the favicon URL is an admin-writable setting, so the
+        // wider check let `/uploads/contracts/signed/<file>` be served here
+        // unauthenticated with a day of cache.
         const uploadsRoot = path.resolve(path.join(storagePath, 'uploads'));
         const resolved = path.resolve(path.join(uploadsRoot, rel));
-        // Path containment — never serve outside the uploads dir.
+        const servableRoots = ['favicons', 'logos'].map((d) => path.join(uploadsRoot, d) + path.sep);
-        if (resolved.startsWith(uploadsRoot + path.sep) && fs.existsSync(resolved)) {
+        if (servableRoots.some((root) => resolved.startsWith(root)) && fs.existsSync(resolved)) {
           // This route streams the file directly, bypassing the secureStatic
           // middleware — so re-apply its SVG hardening here. An admin-uploaded
           // SVG favicon could contain <script>; served at the top-level

backend/src/routes/adminBusinessProfile.js

@@ -22,6 +22,7 @@ const { adminAuth } = require('../middleware/auth');
 const { requirePermission } = require('../middleware/permissions');
 const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers');
 const { getStoragePath } = require('../config/storage');
+const { uploadedPdfLogoPath } = require('../utils/safePath');
 const businessProfileService = require('../services/businessProfileService');
 const { db } = require('../database/db');
 const { validateIban } = require('../utils/iban');
@@ -358,12 +359,8 @@ router.post(
     // a path managed by a different system.
     try {
       const previous = await db('business_profile').where({ id: 1 }).first();
-      const prev = previous?.logo_path;
+      const prevDisk = uploadedPdfLogoPath(previous?.logo_path, getStoragePath());
-      if (prev && typeof prev === 'string' && /pdf-logo-\d+\./.test(prev)) {
+      if (prevDisk) {
-        const stripped = prev.replace(/^\/+/, '');
-        const prevDisk = path.isAbsolute(prev)
-          ? prev
-          : path.join(getStoragePath(), stripped);
         try { await fs.unlink(prevDisk); } catch (_) { /* ignore */ }
       }
     } catch (_) { /* ignore */ }
@@ -383,12 +380,8 @@ router.delete(
   requirePermission('settings.edit'),
   handleAsync(async (req, res) => {
     const existing = await db('business_profile').where({ id: 1 }).first();
-    const prev = existing?.logo_path;
+    const prevDisk = uploadedPdfLogoPath(existing?.logo_path, getStoragePath());
-    if (prev && typeof prev === 'string' && /pdf-logo-\d+\./.test(prev)) {
+    if (prevDisk) {
-      const stripped = prev.replace(/^\/+/, '');
-      const prevDisk = path.isAbsolute(prev)
-        ? prev
-        : path.join(getStoragePath(), stripped);
       try { await fs.unlink(prevDisk); } catch (_) { /* ignore */ }
     }
     await businessProfileService.updateProfile(

backend/src/routes/adminSettings.js

@@ -1,6 +1,7 @@
 const express = require('express');
 const multer = require('multer');
 const path = require('path');
+const { uploadedAssetPath } = require('../utils/safePath');
 const fs = require('fs').promises;
 const { body, validationResult } = require('express-validator');
 const { db, logActivity } = require('../database/db');
@@ -567,10 +568,13 @@ router.put('/branding', adminAuth, requirePermission('settings.edit'), async (re
           currentFaviconUrl = currentFaviconSetting.setting_value;
         }
         
-        if (currentFaviconUrl && typeof currentFaviconUrl === 'string' && currentFaviconUrl.startsWith('/uploads/favicons/')) {
+        // Containment: the stored URL is admin-writable, so only the leaf
-          // Delete the file from filesystem
+        // name is used and it is joined onto the fixed favicon directory. A
-          const relativePath = currentFaviconUrl.replace(/^\//, '');
+        // prefix test alone let `/uploads/favicons/../../<anything>` pass
-          const faviconPath = path.join(getStoragePath(), relativePath);
+        // and path.join collapse it -- an arbitrary-file delete for any
+        // holder of settings.edit.
+        const faviconPath = uploadedAssetPath(currentFaviconUrl, 'favicons', getStoragePath());
+        if (faviconPath) {
           try {
             await fs.unlink(faviconPath);
             logger.info('Deleted favicon file:', faviconPath);
@@ -598,10 +602,9 @@ router.put('/branding', adminAuth, requirePermission('settings.edit'), async (re
           currentLogoUrl = currentLogoSetting.setting_value;
         }
         
-        if (currentLogoUrl && typeof currentLogoUrl === 'string' && currentLogoUrl.startsWith('/uploads/logos/')) {
+        // Same containment as the favicon branch above.
-          // Delete the file from filesystem
+        const logoPath = uploadedAssetPath(currentLogoUrl, 'logos', getStoragePath());
-          const relativePath = currentLogoUrl.replace(/^\//, '');
+        if (logoPath) {
-          const logoPath = path.join(getStoragePath(), relativePath);
           try {
             await fs.unlink(logoPath);
             logger.info('Deleted logo file:', logoPath);

backend/src/utils/safePath.js

@@ -171,8 +171,50 @@ function assertZipEntriesWithin(entries, extractRoot) {
   }
 }
 
+/**
+ * Resolve a stored `/uploads/<kind>/<file>` URL to the file it names inside
+ * that upload directory, or null when the value is not one of ours.
+ *
+ * Only the basename is trusted: the URL comes from an admin-writable
+ * setting, and `path.join(storage, url)` after a `startsWith('/uploads/…')`
+ * check still collapses `..` segments, so it could name any file the process
+ * can delete. Restricting to a flat leaf inside the fixed directory is the
+ * whole control -- the upload routes only ever write flat filenames there.
+ *
+ * @param {string} url          stored value, e.g. "/uploads/logos/logo-1.png"
+ * @param {string} kind         "logos" | "favicons"
+ * @param {string} storageRoot  the root the writer used (callers differ)
+ */
+function uploadedAssetPath(url, kind, storageRoot) {
+  if (!url || typeof url !== 'string') return null;
+  const prefix = `/uploads/${kind}/`;
+  if (!url.startsWith(prefix)) return null;
+  const leaf = url.slice(prefix.length);
+  if (!leaf || leaf === '.' || leaf === '..' || path.basename(leaf) !== leaf) return null;
+  return path.join(storageRoot, 'uploads', kind, leaf);
+}
+
+/**
+ * Resolve business_profile.logo_path to the file the PDF-logo upload route
+ * wrote, or null. logo_path is a free-text field on the profile PUT (an
+ * admin may point it at a file managed elsewhere), so it must never be
+ * unlinked as given: a `/pdf-logo-\d+\./` marker test plus path.join let
+ * `pdf-logo-1./../../../<anything>` -- or any absolute path containing the
+ * marker -- delete arbitrary files. Only a flat `pdf-logo-<n>.<ext>` leaf
+ * inside uploads/logos is ever named.
+ */
+function uploadedPdfLogoPath(logoPath, storageRoot) {
+  if (!logoPath || typeof logoPath !== 'string') return null;
+  const normalized = logoPath.replace(/^\/+/, '');
+  const match = /^uploads\/logos\/(pdf-logo-\d+\.[A-Za-z0-9]+)$/.exec(normalized);
+  if (!match) return null;
+  return path.join(storageRoot, 'uploads', 'logos', match[1]);
+}
+
 module.exports = {
   assertPathInside,
   assertContractPdfPath,
   assertZipEntriesWithin,
+  uploadedAssetPath,
+  uploadedPdfLogoPath,
 };