fix(backend): validate event id before using it in the logo storage filename (stable) (#1397)

* fix(backend): validate event id before using it in the logo storage filename

The multer filename callback built the stored path directly from
req.params.id with no integer validation, letting a traversal payload
in the route param escape the intended uploads/logos/events/
directory — most directly reachable via a super_admin session, since
requireEventOwnership short-circuits with no DB lookup for that role.

* fix(backend): validate contract id before using it in the signed-PDF storage filename

Backport of the same fix on main: same pattern as the event-logo fix
(GHSA-9q5j-vqfw-32hr) in a different file this branch never touched —
multer's filename callback ran before express-validator's :id check,
letting a traversal payload escape uploads/contracts/signed/.

---------

Co-authored-by: Paul Nothaft <[email protected]>

backend/__tests__/routes/adminContractsSignedPdfPathTraversal.test.js

@@ -0,0 +1,126 @@
+/**
+ * Same bug class as GHSA-9q5j-vqfw-32hr (fixed in adminEvents/logo.js) —
+ * the signed-PDF upload's multer `filename` callback built the stored
+ * path directly from `req.params.id` with no integer validation:
+ *
+ *   filename: (req, file, cb) => {
+ *     cb(null, `contract-${req.params.id}-${Date.now()}${ext}`);
+ *   }
+ *
+ * `POST /:id/upload-signed-pdf` declares `param('id').isInt({ min: 1 })`,
+ * but express-validator's check only runs inside the route handler via
+ * validateRequest(req) — AFTER multer has already parsed the multipart
+ * body and invoked the filename callback. A traversal payload in the raw
+ * `:id` URL segment reaches multer completely unvalidated.
+ *
+ * Fixed by rejecting any non-positive-integer id before it is used to
+ * build the filename, independent of the declared-but-too-late
+ * express-validator check.
+ */
+const path = require('path');
+const fs = require('fs');
+const os = require('os');
+
+// ALLOWED_MEDIA_TYPES in fileSecurityUtils.js only defines image/video
+// entries, so the route's real fileFilter (validateFileType(..., ['application/pdf']))
+// rejects every PDF upload with "Only PDF files are allowed" — a
+// separate, pre-existing bug unrelated to the path-traversal fix under
+// test here (also present in publicContracts.js, which is why neither
+// suite exercises a successful upload). Stub validateFileType so this
+// suite can drive the full route, including the filename-callback fix,
+// end-to-end.
+jest.mock('../../src/utils/fileSecurityUtils', () => {
+  const actual = jest.requireActual('../../src/utils/fileSecurityUtils');
+  return {
+    ...actual,
+    validateFileType: (filename, mimetype, allowedTypes) => allowedTypes.includes(mimetype),
+  };
+});
+
+process.env.NODE_ENV = 'test';
+process.env.TEST_DATABASE_PATH = path.join(
+  fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-contracts-signed-pdf-')), 'db.sqlite'
+);
+process.env.JWT_SECRET = process.env.JWT_SECRET || 'admin-contracts-signed-pdf-test-secret';
+
+const request = require('supertest');
+const {
+  bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken, buildRouteApp,
+} = require('../integration/helpers/crmDb');
+
+describe('POST /api/admin/contracts/:id/upload-signed-pdf — path traversal guard', () => {
+  let db; let cleanup; let app; let adminId; let customerId; let token;
+
+  beforeAll(async () => {
+    ({ db, cleanup } = await bootCrmDb());
+    ({ adminId, customerId } = await seedMinimal(db));
+    await assignAdminRole(db, adminId, 'super_admin');
+    token = mintAdminToken(adminId);
+
+    // Feature flag defaults OFF on a fresh install — the contracts
+    // router 403s every route until it's on.
+    await db('feature_flags').where({ key: 'contracts' }).update({ value: true });
+
+    app = buildRouteApp('/api/admin/contracts', require('../../src/routes/adminContracts'));
+  }, 120000);
+
+  afterAll(async () => { await cleanup(); });
+
+  const auth = (req) => req.set('Authorization', `Bearer ${token}`);
+  const signedDir = () => path.join(process.env.STORAGE_PATH, 'uploads/contracts/signed');
+
+  async function insertContract(over = {}) {
+    const base = {
+      contract_number: `K-TEST-${Math.random().toString(16).slice(2, 8)}`,
+      customer_account_id: customerId,
+      title: 'Test Contract',
+      issue_date: new Date().toISOString().slice(0, 10),
+      status: 'sent',
+      language: 'de',
+      created_at: new Date().toISOString(),
+      ...over,
+    };
+    const inserted = await db('contracts').insert(base).returning('id');
+    return inserted[0]?.id ?? inserted[0];
+  }
+
+  it('rejects a traversal payload in the id param instead of writing outside uploads/contracts/signed', async () => {
+    // '../../../../tmp/pwned' URL-encoded so the raw request path still
+    // has a single segment (matches Express's `:id`), but Express
+    // decodes the param back into literal '../' sequences before the
+    // route sees it.
+    const traversalId = encodeURIComponent('../../../../tmp/pwned');
+
+    const res = await auth(
+      request(app).post(`/api/admin/contracts/${traversalId}/upload-signed-pdf`)
+    ).attach('file', Buffer.from('%PDF-1.4 fake'), 'signed.pdf');
+
+    expect(res.status).toBeGreaterThanOrEqual(400);
+    expect(res.body.error).toMatch(/invalid contract id/i);
+
+    // No file should have been written anywhere — the filename callback
+    // must error out before multer opens a write stream.
+    const escapedFile = path.join(os.tmpdir(), 'pwned');
+    expect(fs.existsSync(escapedFile)).toBe(false);
+    if (fs.existsSync(signedDir())) {
+      expect(fs.readdirSync(signedDir())).toHaveLength(0);
+    }
+  });
+
+  it('still accepts a normal numeric contract id', async () => {
+    const id = await insertContract();
+
+    const res = await auth(
+      request(app).post(`/api/admin/contracts/${id}/upload-signed-pdf`)
+    ).attach('file', Buffer.from('%PDF-1.4 fake'), 'signed.pdf');
+
+    expect(res.status).toBe(200);
+
+    const files = fs.readdirSync(signedDir());
+    expect(files.some((f) => f.startsWith(`contract-${id}-`))).toBe(true);
+
+    const row = await db('contracts').where({ id }).first();
+    expect(row.status).toBe('fully_signed');
+    expect(row.signed_pdf_path).toMatch(new RegExp(`contract-${id}-`));
+  });
+});

backend/__tests__/routes/adminEventsLogoPathTraversal.test.js

@@ -0,0 +1,122 @@
+/**
+ * GHSA-9q5j-vqfw-32hr — the event-logo upload's multer `filename` callback
+ * built the stored path directly from `req.params.id` with no integer
+ * validation:
+ *
+ *   filename: (req, file, cb) => {
+ *     cb(null, `event-${req.params.id}-logo-${Date.now()}${ext}`);
+ *   }
+ *
+ * A traversal payload in the `:id` route param (URL-encoded so it still
+ * matches a single Express path segment, then decoded back into literal
+ * `../` sequences by Express before handlers see it) could escape the
+ * intended uploads/logos/events/ directory. Most directly reachable via a
+ * super_admin session: requireEventOwnership short-circuits with next() and
+ * zero DB lookup for that role (src/middleware/ownership.js), so nothing
+ * upstream of multer validates the id first.
+ *
+ * Fixed by rejecting any non-positive-integer id before it is used to build
+ * the filename, regardless of role or ownership-check ordering.
+ */
+const path = require('path');
+const fs = require('fs');
+const os = require('os');
+
+process.env.NODE_ENV = 'test';
+process.env.TEST_DATABASE_PATH = path.join(
+  fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-events-logo-')), 'db.sqlite'
+);
+process.env.JWT_SECRET = process.env.JWT_SECRET || 'admin-events-logo-test-secret';
+
+const express = require('express');
+const cookieParser = require('cookie-parser');
+const request = require('supertest');
+const { bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken } = require('../integration/helpers/crmDb');
+
+async function insertEvent(db, adminId, over = {}) {
+  const base = {
+    slug: `ev-${Math.random().toString(16).slice(2)}`,
+    event_type: 'wedding',
+    event_name: 'Test Wedding',
+    event_date: '2026-05-29',
+    host_email: '[email protected]',
+    admin_email: '[email protected]',
+    password_hash: 'x',
+    share_link: `/gallery/share-${Math.random().toString(16).slice(2)}`,
+    share_token: `st-${Math.random().toString(16).slice(2)}`,
+    expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(),
+    is_active: 1, is_archived: 0, is_draft: 0,
+    created_by: adminId,
+    created_at: new Date().toISOString(),
+    ...over,
+  };
+  const r = await db('events').insert(base).returning('id');
+  return r[0]?.id ?? r[0];
+}
+
+describe('POST /api/admin/events/:id/logo — path traversal guard', () => {
+  let db; let cleanup; let app; let adminId; let token;
+
+  beforeAll(async () => {
+    ({ db, cleanup } = await bootCrmDb());
+    ({ adminId } = await seedMinimal(db));
+    // super_admin: requireEventOwnership short-circuits with no DB lookup
+    // for this role, so it reaches multer with nothing upstream having
+    // validated the id — the exact path GHSA-9q5j-vqfw-32hr exploited.
+    await assignAdminRole(db, adminId, 'super_admin');
+    token = mintAdminToken(adminId);
+
+    app = express();
+    app.use(express.json());
+    app.use(cookieParser());
+    app.use('/api/admin/events', require('../../src/routes/adminEvents'));
+    // eslint-disable-next-line no-unused-vars
+    app.use((err, req, res, next) => {
+      res.status(err.statusCode || err.status || 500).json({ error: err.message, code: err.code });
+    });
+  }, 120000);
+
+  afterAll(async () => { await cleanup(); });
+
+  const auth = (req) => req.set('Authorization', `Bearer ${token}`);
+  const logoDir = () => path.join(process.env.STORAGE_PATH, 'uploads/logos/events');
+
+  it('rejects a traversal payload in the id param instead of writing outside uploads/logos/events', async () => {
+    // '../../../../tmp/pwned' URL-encoded so the raw request path still has
+    // a single segment (matches Express's `:id`), but Express decodes the
+    // param back into literal '../' sequences before the route sees it.
+    const traversalId = encodeURIComponent('../../../../tmp/pwned');
+
+    const res = await auth(
+      request(app).post(`/api/admin/events/${traversalId}/logo`)
+    ).attach('logo', Buffer.from('fake image data'), 'logo.png');
+
+    expect(res.status).toBeGreaterThanOrEqual(400);
+    expect(res.body.error).toMatch(/invalid event id/i);
+
+    // No file should have been written anywhere — the filename callback
+    // must error out before multer opens a write stream.
+    const escapedFile = path.join(os.tmpdir(), 'pwned');
+    expect(fs.existsSync(escapedFile)).toBe(false);
+    if (fs.existsSync(logoDir())) {
+      expect(fs.readdirSync(logoDir())).toHaveLength(0);
+    }
+  });
+
+  it('still accepts a normal numeric event id', async () => {
+    const id = await insertEvent(db, adminId, { event_name: 'Logo Event' });
+
+    const res = await auth(
+      request(app).post(`/api/admin/events/${id}/logo`)
+    ).attach('logo', Buffer.from('fake image data'), 'logo.png');
+
+    expect(res.status).toBe(200);
+    expect(res.body.hero_logo_url).toMatch(new RegExp(`^/uploads/logos/events/event-${id}-logo-`));
+
+    const files = fs.readdirSync(logoDir());
+    expect(files.some((f) => f.startsWith(`event-${id}-logo-`))).toBe(true);
+
+    const row = await db('events').where({ id }).first();
+    expect(row.hero_logo_url).toBe(res.body.hero_logo_url);
+  });
+});

backend/src/routes/adminContracts.js

@@ -65,8 +65,12 @@ const signedPdfStorage = multer.diskStorage({
     cb(null, uploadDir);
   },
   filename: (req, file, cb) => {
+    const contractId = Number(req.params.id);
+    if (!Number.isInteger(contractId) || contractId <= 0) {
+      return cb(new Error('Invalid contract id'));
+    }
     const ext = path.extname(file.originalname) || '.pdf';
-    cb(null, `contract-${req.params.id}-${Date.now()}${ext}`);
+    cb(null, `contract-${contractId}-${Date.now()}${ext}`);
   },
 });
 

backend/src/routes/adminEvents/logo.js

@@ -23,8 +23,12 @@ const eventLogoStorage = multer.diskStorage({
     cb(null, uploadDir);
   },
   filename: (req, file, cb) => {
+    const eventId = Number(req.params.id);
+    if (!Number.isInteger(eventId) || eventId <= 0) {
+      return cb(new Error('Invalid event id'));
+    }
     const ext = path.extname(file.originalname);
-    cb(null, `event-${req.params.id}-logo-${Date.now()}${ext}`);
+    cb(null, `event-${eventId}-logo-${Date.now()}${ext}`);
   }
 });