diff --git a/backend/__tests__/routes/adminContractsSignedPdfPathTraversal.test.js b/backend/__tests__/routes/adminContractsSignedPdfPathTraversal.test.js new file mode 100644 index 00000000..ce7c8d29 --- /dev/null +++ b/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}-`)); + }); +}); diff --git a/backend/__tests__/routes/adminEventsLogoPathTraversal.test.js b/backend/__tests__/routes/adminEventsLogoPathTraversal.test.js new file mode 100644 index 00000000..2d64a9bd --- /dev/null +++ b/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: 'host@example.com', + admin_email: 'admin@example.com', + 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); + }); +}); diff --git a/backend/src/routes/adminContracts.js b/backend/src/routes/adminContracts.js index ef44deaa..4d5d4274 100644 --- a/backend/src/routes/adminContracts.js +++ b/backend/src/routes/adminContracts.js @@ -66,8 +66,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}`); }, }); diff --git a/backend/src/routes/adminEvents/logo.js b/backend/src/routes/adminEvents/logo.js index ff239f98..543be340 100644 --- a/backend/src/routes/adminEvents/logo.js +++ b/backend/src/routes/adminEvents/logo.js @@ -24,8 +24,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}`); } });