From 6cd546e86ae38819c0fdc24044f86106503fa020 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Fri, 17 Jul 2026 09:16:41 +0200 Subject: [PATCH] fix(security): remove unguarded legacy /api/events router (GHSA-4j34-x562-5vfq) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy gallery router mounted at /api/events exposed create/list/update/ delete/extend guarded by adminAuth ALONE — no requirePermission, no requireEventOwnership. adminAuth only checks the token is a valid type:'admin' session, which every back-office role holds, down to read-only `viewer`. So any non-super-admin account could: - GET /api/events → every gallery's bcrypt password_hash, share_token, and client name/email (the list handler selects * and mapEventForApi keeps those columns), - PUT /api/events/:id → reset any gallery's password (full takeover), - DELETE /api/events/:id → delete any gallery, all bypassing the per-photographer ownership isolation the canonical /api/admin/events router enforces. Affects any instance with more than the single super_admin. Fix: remove the legacy router entirely (mount + require + src/routes/events.js). It was a superseded duplicate of /api/admin/events and unused by the frontend EXCEPT for one live route — POST /:id/extend (the "Extend expiration" UI action, which hit /api/events/:id/extend via the api client's /api base). That route is migrated to the canonical mount as POST /api/admin/events/:id/extend with the same guards as every other gallery mutation (adminAuth + requirePermission ('events.edit') + requireEventOwnership), and the frontend is repointed to it. Behaviour of the extend itself is unchanged (expires_at + reactivate). Verified end-to-end on a booted instance: /api/events (all methods) now 404; /api/admin/events/:id/extend returns 401 unauth, 200 for the owner, 403 for a non-owning editor; the full login→create→extend flow works. Adds a regression test pinning the router removal and the extend ownership check. --- .../routes/legacyEventsRouterRemoved.test.js | 119 +++++ backend/server.js | 4 +- backend/src/routes/adminEvents/crud.js | 47 ++ backend/src/routes/events.js | 443 ------------------ frontend/src/services/events.service.ts | 5 +- 5 files changed, 170 insertions(+), 448 deletions(-) create mode 100644 backend/__tests__/routes/legacyEventsRouterRemoved.test.js delete mode 100644 backend/src/routes/events.js diff --git a/backend/__tests__/routes/legacyEventsRouterRemoved.test.js b/backend/__tests__/routes/legacyEventsRouterRemoved.test.js new file mode 100644 index 00000000..07958f86 --- /dev/null +++ b/backend/__tests__/routes/legacyEventsRouterRemoved.test.js @@ -0,0 +1,119 @@ +/** + * Regression test for GHSA-4j34-x562-5vfq — broken access control in the legacy + * /api/events router. + * + * The legacy router exposed create/list/update/delete/extend guarded by + * adminAuth ALONE (no requirePermission, no requireEventOwnership), so any + * back-office account — down to a read-only viewer — could read every gallery's + * password_hash/share_token and take over any gallery. The fix removes that + * router entirely and migrates its one UI-used route (POST /:id/extend) to the + * canonical /api/admin/events mount, where it inherits the permission + + * ownership guards. + * + * This test pins two invariants: + * 1. The legacy source file is gone (nothing can re-mount it). + * 2. The migrated extend route enforces ownership — a non-owning editor gets + * 403, the owner succeeds. + */ +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-legacy-acl-')), 'db.sqlite' +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'legacy-acl-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, ownerId, over = {}) { + const base = { + slug: `ev-${Math.random().toString(16).slice(2)}`, + event_type: 'wedding', + event_name: 'Owner Gallery', + 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: ownerId, + created_at: new Date().toISOString(), + ...over, + }; + const r = await db('events').insert(base).returning('id'); + return r[0]?.id ?? r[0]; +} + +describe('GHSA-4j34: legacy /api/events router removed + extend guarded', () => { + it('the legacy events router source file no longer exists', () => { + expect(fs.existsSync(path.join(__dirname, '../../src/routes/events.js'))).toBe(false); + }); + + describe('POST /api/admin/events/:id/extend ownership enforcement', () => { + let db; let cleanup; let app; + let ownerId; let ownerToken; + let editorId; let editorToken; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ adminId: ownerId } = await seedMinimal(db)); + await assignAdminRole(db, ownerId, 'super_admin'); + ownerToken = mintAdminToken(ownerId); + + // A second, non-owning account with the low-trust editor role. + [editorId] = await db('admin_users').insert({ + username: 'editor1', email: 'editor1@example.com', + password_hash: 'x', is_active: 1, + }).returning('id'); + editorId = editorId?.id ?? editorId; + await assignAdminRole(db, editorId, 'editor'); + editorToken = mintAdminToken(editorId); + + 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(); }); + + it('lets the owner extend their own gallery', async () => { + const id = await insertEvent(db, ownerId, { expires_at: '2026-06-01T00:00:00.000Z' }); + const res = await request(app) + .post(`/api/admin/events/${id}/extend`) + .set('Authorization', `Bearer ${ownerToken}`) + .send({ days: 10 }); + expect(res.status).toBe(200); + expect(new Date(res.body.expires_at).toISOString()).toBe('2026-06-11T00:00:00.000Z'); + }); + + it('403s a non-owning editor trying to extend someone else\'s gallery', async () => { + const id = await insertEvent(db, ownerId); // owned by the super_admin + const res = await request(app) + .post(`/api/admin/events/${id}/extend`) + .set('Authorization', `Bearer ${editorToken}`) + .send({ days: 30 }); + expect(res.status).toBe(403); // requireEventOwnership blocks it + }); + + it('validates the days field', async () => { + const id = await insertEvent(db, ownerId); + const res = await request(app) + .post(`/api/admin/events/${id}/extend`) + .set('Authorization', `Bearer ${ownerToken}`) + .send({ days: 9999 }); + expect(res.status).toBe(400); + }); + }); +}); diff --git a/backend/server.js b/backend/server.js index 1bf04dc7..346a71f2 100644 --- a/backend/server.js +++ b/backend/server.js @@ -38,7 +38,6 @@ const { // Import routes const authRoutes = require('./src/routes/auth'); -const eventRoutes = require('./src/routes/events'); const galleryRoutes = require('./src/routes/gallery'); const adminRoutes = require('./src/routes/admin'); const adminAuthRoutes = require('./src/routes/adminAuth'); @@ -695,8 +694,7 @@ app.get('/health', async (req, res) => { // Routes app.use('/api/setup', setupRoutes); // public first-run bootstrap (self-closes after setup) app.use('/api/auth', authRoutes); - app.use('/api/events', eventRoutes); - app.use('/api/admin/external-media', require('./src/routes/adminExternalMedia')); +app.use('/api/admin/external-media', require('./src/routes/adminExternalMedia')); // Gallery routes - main routes first, then feedback routes app.use('/api/gallery', galleryRoutes); app.use('/api/gallery', require('./src/routes/galleryFeedback')); diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index 238c4456..d8dfbf13 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -1596,4 +1596,51 @@ module.exports = (router) => { } }); + // Extend a gallery's expiration. Migrated from the legacy /api/events router + // (removed — GHSA-4j34-x562-5vfq), now on the canonical mount with the same + // permission + ownership guards as every other gallery mutation, so a + // non-owning editor/viewer can no longer touch a gallery they don't own. + router.post('/:id/extend', adminAuth, requirePermission('events.edit'), requireEventOwnership, [ + body('days').isInt({ min: 1, max: 365 }) + ], async (req, res) => { + try { + const errors = validationResult(req); + if (!errors.isEmpty()) { + return res.status(400).json({ errors: errors.array() }); + } + + const { id } = req.params; + const { days } = req.body; + + let eventQuery = db('events').where('id', id); + // Editor role can only touch their own events (defence in depth alongside + // requireEventOwnership). + if (req.admin.roleName === 'editor') { + eventQuery = eventQuery.where('created_by', req.admin.id); + } + const event = await eventQuery.first(); + if (!event) { + return res.status(404).json({ error: 'Event not found' }); + } + + const newExpiration = new Date(event.expires_at); + newExpiration.setDate(newExpiration.getDate() + days); + + await db('events').where('id', id).update({ + expires_at: newExpiration, + is_active: formatBoolean(true) // reactivate if it had expired + }); + + await logActivity('event_expiration_extended', + { eventName: event.event_name, days }, + id, + { type: 'admin', id: req.admin.id, name: req.admin.username } + ); + + res.json({ expires_at: newExpiration }); + } catch (error) { + errorResponse(res, error, 500, 'Failed to extend expiration'); + } + }); + }; diff --git a/backend/src/routes/events.js b/backend/src/routes/events.js deleted file mode 100644 index 18ac1e47..00000000 --- a/backend/src/routes/events.js +++ /dev/null @@ -1,443 +0,0 @@ -const express = require('express'); -const { body, validationResult } = require('express-validator'); -const bcrypt = require('bcrypt'); -const crypto = require('crypto'); -const { db } = require('../database/db'); -const { formatBoolean } = require('../utils/dbCompat'); -const { slugify } = require('../utils/slug'); -const { validatePasswordInContext, getBcryptRounds } = require('../utils/passwordValidation'); -const { adminAuth } = require('../middleware/auth'); -const fs = require('fs').promises; -const path = require('path'); -const router = express.Router(); -const { buildShareLinkVariants } = require('../services/shareLinkService'); -const { parseBooleanInput, parseStringInput } = require('../utils/parsers'); -const eventTypeService = require('../services/eventTypeService'); -const { IDENTITY_PRESERVING_NORMALIZE_EMAIL } = require('../utils/emailNormalization'); -const logger = require('../utils/logger'); - -// Use parseStringInput from shared parsers for customer data extraction -const getCustomerNameFromPayload = (payload = {}) => parseStringInput(payload.customer_name); -const getCustomerEmailFromPayload = (payload = {}) => parseStringInput(payload.customer_email); -const getCustomerPhoneFromPayload = (payload = {}) => parseStringInput(payload.customer_phone); - -// Whether the global "phone field" toggle (#322) is enabled. Same shape as -// the helper in adminEvents.js — kept local so this route doesn't import -// from a sibling route file. -const isPhoneFieldEnabled = async () => { - try { - const row = await db('app_settings').where('setting_key', 'event_phone_field_enabled').first(); - if (!row) return false; - let value = row.setting_value; - if (typeof value === 'string') { - try { value = JSON.parse(value); } catch { /* keep raw */ } - } - return value === true; - } catch { - return false; - } -}; - -const mapEventForApi = (event) => { - if (!event || typeof event !== 'object') { - return event; - } - - const { - host_name, - host_email, - customer_name, - customer_email, - ...rest - } = event; - - return { - ...rest, - customer_name: customer_name ?? host_name ?? null, - customer_email: customer_email ?? host_email ?? null - }; -}; - -let customerColumnCache = null; -const hasCustomerContactColumns = async () => { - if (customerColumnCache === true) { - return true; - } - - try { - const hasColumn = await db.schema.hasColumn('events', 'customer_email'); - if (hasColumn) { - customerColumnCache = true; - } - return hasColumn; - } catch (error) { - return false; - } -}; - -// Create new event -router.post('/', adminAuth, [ - body('event_type').notEmpty().trim().custom(async (value) => { - const isValid = await eventTypeService.isValidEventType(value); - if (!isValid) { - throw new Error('Invalid event type'); - } - return true; - }), - body('event_name').notEmpty(), - body('event_date').isDate(), - body('customer_name').notEmpty().trim(), - body('customer_email').isEmail().normalizeEmail(IDENTITY_PRESERVING_NORMALIZE_EMAIL), - body('customer_phone').optional({ nullable: true, checkFalsy: true }) - .isString().trim() - .isLength({ max: 32 }).withMessage('Phone number must be at most 32 characters'), - body('admin_email').isEmail(), - body('require_password').optional().isBoolean(), - body('password').optional().isString().custom((value, { req }) => { - const requirePassword = parseBooleanInput(req.body.require_password, true); - if (!requirePassword) { - return true; - } - if (typeof value !== 'string' || value.trim().length < 6) { - throw new Error('Password must be at least 6 characters long'); - } - return true; - }), - body('expiration_days').isInt({ min: 1, max: 365 }).optional() -], async (req, res) => { - try { - const errors = validationResult(req); - if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); - } - - const { - event_type, - event_name, - event_date, - admin_email, - password, - require_password: requirePasswordInput = true, - welcome_message, - color_theme, - expiration_days = 30 - } = req.body; - - const customerEmail = getCustomerEmailFromPayload(req.body); - const customerName = getCustomerNameFromPayload(req.body); - - if (!customerName || !customerEmail) { - return res.status(400).json({ error: 'customer_name and customer_email are required' }); - } - - const customerColumnsAvailable = await hasCustomerContactColumns(); - const phoneEnabled = await isPhoneFieldEnabled(); - const customerPhone = phoneEnabled ? getCustomerPhoneFromPayload(req.body) : null; - - const requirePassword = parseBooleanInput(requirePasswordInput, true); - - if (requirePassword) { - const passwordValidation = await validatePasswordInContext(password, 'gallery', { - eventName: event_name - }); - - if (!passwordValidation.valid) { - return res.status(400).json({ - error: 'Password does not meet security requirements', - details: passwordValidation.errors, - score: passwordValidation.score, - feedback: passwordValidation.feedback - }); - } - } - - // Generate unique slug — slugify() handles accents (see #525). - const baseSlug = `${event_type}-${slugify(event_name)}-${event_date}`; - let slug = baseSlug; - let counter = 1; - - while (await db('events').where({ slug }).first()) { - slug = `${baseSlug}-${counter}`; - counter++; - } - - // Generate share link variants (auto-detects short URL preference) - const shareToken = crypto.randomBytes(16).toString('hex'); - const { sharePath, shareUrl, shareLinkToStore } = await buildShareLinkVariants({ slug, shareToken }); - - // Hash password (or placeholder when not required) - const password_hash = requirePassword - ? await bcrypt.hash(password, getBcryptRounds()) - : await bcrypt.hash(crypto.randomBytes(32).toString('hex'), getBcryptRounds()); - - // Calculate expiration date (days after event date) - const expires_at = new Date(event_date); - expires_at.setDate(expires_at.getDate() + parseInt(expiration_days, 10)); - - // Create folder structure - const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../../../storage'); - const eventPath = path.join(storagePath, 'events/active', slug); - await fs.mkdir(path.join(eventPath, 'collages'), { recursive: true }); - await fs.mkdir(path.join(eventPath, 'individual'), { recursive: true }); - - // Insert into database - const insertResult = await db('events').insert({ - slug, - event_type, - event_name, - event_date, - ...(customerColumnsAvailable ? { customer_name: customerName, customer_email: customerEmail } : {}), - ...(customerPhone ? { customer_phone: customerPhone } : {}), - host_name: customerName, - host_email: customerEmail, - admin_email, - password_hash, - welcome_message, - color_theme, - share_link: shareLinkToStore, - share_token: shareToken, - expires_at, - require_password: formatBoolean(requirePassword) - }).returning('id'); - - // Handle both PostgreSQL (returns array of objects) and SQLite (returns array of IDs) - const eventId = insertResult[0]?.id || insertResult[0]; - - // Queue creation email - const { queueEmail } = require('../services/emailProcessor'); - await queueEmail(eventId, customerEmail, 'gallery_created', { - customer_name: customerName, - customer_email: customerEmail, - host_name: customerName, - event_name, - event_date: event_date, // Pass raw date - will be formatted by email processor - gallery_link: shareUrl, - gallery_password: requirePassword ? password : 'No password required', - expiry_date: expires_at.toISOString(), // Pass ISO string - will be formatted by email processor - welcome_message: welcome_message || '' - }); - - // WhatsApp gallery_ready notification (#647 follow-up). Mirrors the - // adminEvents.js path: fires when the customer supplied a phone, the - // feature is enabled, and a config exists. Non-fatal — a queue failure - // must never block gallery creation. - if (customerPhone) { - try { - const { queueWhatsapp, getWhatsAppConfig } = require('../services/whatsappProcessor'); - const waConfig = await getWhatsAppConfig(); - if (waConfig && waConfig.enabled) { - await queueWhatsapp(eventId, customerPhone, 'gallery_created', { - customer_name: customerName || '', - event_name, - gallery_link: shareUrl, - gallery_password: requirePassword ? password : '', - expiry_date: expires_at ? expires_at.toISOString() : null, - language: null, - }); - } - } catch (waError) { - logger.warn('Failed to queue WhatsApp notification on create', waError.message); - } - } - - // Webhook lifecycle (#327). Legacy public endpoint — events go live - // immediately so created + published fire together. Payload uses the - // canonical event subject (#341) — every event.* webhook now includes - // customer contact + share_token. - try { - const webhookService = require('../services/webhookService'); - const eventSubject = webhookService.buildEventSubject({ - id: eventId, - slug, - event_name, - event_type, - event_date, - share_url: shareUrl, - share_token: shareToken, - customer_name: customerName, - customer_email: customerEmail, - customer_phone: customerPhone, - }); - await webhookService.fire('event.created', { event: eventSubject }); - await webhookService.fire('event.published', { event: eventSubject }); - } catch (e) { /* non-fatal */ } - - res.json({ - id: eventId, - slug, - share_link: shareUrl, - expires_at, - require_password: requirePassword, - customer_name: customerName, - customer_email: customerEmail - }); - } catch (error) { - logger.error(error); - res.status(500).json({ error: 'Failed to create event' }); - } -}); - -// Get all events (admin) -router.get('/', adminAuth, async (req, res) => { - try { - const { status = 'all' } = req.query; - - let query = db('events').select('*'); - - if (status === 'active') { - query = query.where('is_active', formatBoolean(true)); - } else if (status === 'archived') { - query = query.where('is_archived', formatBoolean(true)); - } - - const events = await query.orderBy('created_at', 'desc'); - - // Add photo counts - for (const event of events) { - const photoCount = await db('photos').where('event_id', event.id).count('id as count').first(); - event.photo_count = photoCount.count; - } - - res.json(events.map(mapEventForApi)); - } catch (error) { - res.status(500).json({ error: 'Failed to fetch events' }); - } -}); - -// Update event -router.put('/:id', adminAuth, [ - body('customer_name').optional().trim().notEmpty(), - body('customer_email').optional().isEmail().normalizeEmail(IDENTITY_PRESERVING_NORMALIZE_EMAIL), - body('require_password').optional().isBoolean() -], async (req, res) => { - try { - const errors = validationResult(req); - if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); - } - - const { id } = req.params; - const updates = { ...req.body }; - const customerColumnsAvailable = await hasCustomerContactColumns(); - - // Don't allow updating certain fields - delete updates.id; - delete updates.slug; - delete updates.created_at; - delete updates.password_confirmation; - - if (Object.prototype.hasOwnProperty.call(updates, 'host_name') || Object.prototype.hasOwnProperty.call(updates, 'host_email')) { - return res.status(400).json({ error: 'host_name and host_email are no longer supported. Use customer_name and customer_email instead.' }); - } - - if (Object.prototype.hasOwnProperty.call(updates, 'customer_name')) { - const nextName = getCustomerNameFromPayload(updates); - if (nextName) { - if (customerColumnsAvailable) { - updates.customer_name = nextName; - } else { - delete updates.customer_name; - } - updates.host_name = nextName; - } else { - delete updates.customer_name; - } - } - - if (Object.prototype.hasOwnProperty.call(updates, 'customer_email')) { - const nextEmail = getCustomerEmailFromPayload(updates); - if (nextEmail) { - if (customerColumnsAvailable) { - updates.customer_email = nextEmail; - } else { - delete updates.customer_email; - } - updates.host_email = nextEmail; - } else { - delete updates.customer_email; - } - } - - const hasRequirePasswordUpdate = Object.prototype.hasOwnProperty.call(updates, 'require_password'); - let requirePasswordUpdate; - if (hasRequirePasswordUpdate) { - requirePasswordUpdate = parseBooleanInput(updates.require_password, true); - updates.require_password = formatBoolean(requirePasswordUpdate); - } - - let newPasswordPlain; - if (Object.prototype.hasOwnProperty.call(updates, 'password')) { - if (updates.password === undefined || updates.password === null || updates.password === '') { - delete updates.password; - } else { - newPasswordPlain = updates.password; - delete updates.password; - } - } - - const event = await db('events').where('id', id).first(); - if (!event) { - return res.status(404).json({ error: 'Event not found' }); - } - - const currentRequirePassword = parseBooleanInput(event.require_password, true); - - if (hasRequirePasswordUpdate && requirePasswordUpdate === true && !currentRequirePassword && !newPasswordPlain) { - return res.status(400).json({ error: 'Password must be provided when enabling password requirement.' }); - } - - if (newPasswordPlain) { - updates.password_hash = await bcrypt.hash(newPasswordPlain, getBcryptRounds()); - } else if (hasRequirePasswordUpdate && requirePasswordUpdate === false && currentRequirePassword) { - updates.password_hash = await bcrypt.hash(crypto.randomBytes(32).toString('hex'), getBcryptRounds()); - } - - await db('events').where('id', id).update(updates); - - res.json({ success: true }); - } catch (error) { - res.status(500).json({ error: 'Failed to update event' }); - } -}); - -// Delete event (mark as inactive) -router.delete('/:id', adminAuth, async (req, res) => { - try { - const { id } = req.params; - - await db('events').where('id', id).update({ is_active: formatBoolean(false) }); - - res.json({ success: true }); - } catch (error) { - res.status(500).json({ error: 'Failed to delete event' }); - } -}); - -// Extend expiration -router.post('/:id/extend', adminAuth, [ - body('days').isInt({ min: 1, max: 365 }) -], async (req, res) => { - try { - const { id } = req.params; - const { days } = req.body; - - const event = await db('events').where('id', id).first(); - if (!event) { - return res.status(404).json({ error: 'Event not found' }); - } - - const newExpiration = new Date(event.expires_at); - newExpiration.setDate(newExpiration.getDate() + days); - - await db('events').where('id', id).update({ - expires_at: newExpiration, - is_active: formatBoolean(true) // Reactivate if expired - }); - - res.json({ expires_at: newExpiration }); - } catch (error) { - res.status(500).json({ error: 'Failed to extend expiration' }); - } -}); - -module.exports = router; diff --git a/frontend/src/services/events.service.ts b/frontend/src/services/events.service.ts index 6d4e62f3..b929dcec 100644 --- a/frontend/src/services/events.service.ts +++ b/frontend/src/services/events.service.ts @@ -203,9 +203,10 @@ export const eventsService = { return response.data; }, - // Extend event expiration (admin) + // Extend event expiration (admin). Uses the canonical, ownership-guarded + // route; the old /events/:id/extend legacy endpoint was removed (GHSA-4j34). async extendExpiration(id: number, days: number): Promise { - const response = await api.post(`/events/${id}/extend`, { + const response = await api.post(`/admin/events/${id}/extend`, { days, }); return response.data;