From c2ce12c039d5564e4457fdbbd50a06bbcfec4d6a Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 2 Aug 2026 08:38:24 +0200 Subject: [PATCH] fix(security): authz/ownership gaps (token binding, auth revocation, feedback/customer ownership, token logging) (#950) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): close authz/ownership gaps (secure-download binding, photo-auth+logout revocation, feedback/customer ownership, token logging) * fix(security): codex round-1 — complete admin-token invalidation + preserve foreign assignments - photoAuth: mirror adminAuth's active-admin lookup + iat --- .../routes/feedbackOwnership.test.js | 101 ++++++++++++++++++ backend/src/middleware/photoAuth.js | 30 +++++- backend/src/routes/adminAuth.js | 17 ++- backend/src/routes/adminCustomers.js | 42 +++++++- backend/src/routes/adminFeedback.js | 36 ++++++- backend/src/routes/secureImages.js | 8 ++ backend/src/services/feedbackService.js | 8 +- backend/src/services/quoteService.js | 7 +- 8 files changed, 234 insertions(+), 15 deletions(-) create mode 100644 backend/__tests__/routes/feedbackOwnership.test.js diff --git a/backend/__tests__/routes/feedbackOwnership.test.js b/backend/__tests__/routes/feedbackOwnership.test.js new file mode 100644 index 00000000..7abc5bca --- /dev/null +++ b/backend/__tests__/routes/feedbackOwnership.test.js @@ -0,0 +1,101 @@ +/** + * GHSA-2qc2 / GHSA-32h4 / GHSA-3335 — feedback moderation, deletion, and the + * pending-moderation list are by-feedback-id (or global) and lacked ownership + * scoping, so a restricted editor could act on / enumerate feedback for events + * it does not own. super_admin keeps global access. + */ +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-fbown-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'fbown-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-fbown-storage-')); + +const request = require('supertest'); +const express = require('express'); +const cookieParser = require('cookie-parser'); +const bcrypt = require('bcrypt'); +const { bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken } = require('../integration/helpers/crmDb'); + +describe('feedback ownership scoping', () => { + let db; let cleanup; let app; + let superTok; let editorTok; let editorId; + let foreignFeedbackId; + + const auth = (req, tok) => req.set('Authorization', `Bearer ${tok}`); + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + const { adminId: superId } = await seedMinimal(db); + await assignAdminRole(db, superId, 'super_admin'); + superTok = mintAdminToken(superId); + + const ins = await db('admin_users').insert({ + username: 'editor', email: 'editor@example.com', + password_hash: await bcrypt.hash('x', 4), must_change_password: false, created_at: new Date(), + }).returning('id'); + editorId = ins[0]?.id ?? ins[0]; + await assignAdminRole(db, editorId, 'editor'); + editorTok = mintAdminToken(editorId); + + // Event owned by super_admin (NOT the editor). + const ev = await db('events').insert({ + slug: 'fbown-foreign', event_type: 'wedding', event_name: 'Foreign', + event_date: '2026-08-01', host_email: 'h@e.com', admin_email: 'a@e.com', + password_hash: 'x', share_link: '/g/fbown/s', share_token: 'fbown-share', + expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, is_archived: 0, is_draft: 0, created_by: superId, + created_at: new Date().toISOString(), + }).returning('id'); + const eventId = ev[0]?.id ?? ev[0]; + const ph = await db('photos').insert({ + event_id: eventId, filename: 'p.jpg', path: 'fbown-foreign/p.jpg', type: 'individual', + uploaded_at: new Date().toISOString(), + }).returning('id'); + const photoId = ph[0]?.id ?? ph[0]; + const fb = await db('photo_feedback').insert({ + photo_id: photoId, event_id: eventId, feedback_type: 'comment', + comment_text: 'hi', is_approved: 0, is_hidden: 0, created_at: new Date().toISOString(), + }).returning('id'); + foreignFeedbackId = fb[0]?.id ?? fb[0]; + + app = express(); + app.use(express.json()); + app.use(cookieParser()); + app.use('/api/admin/feedback', require('../../src/routes/adminFeedback')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('editor cannot moderate feedback on an event it does not own (404)', async () => { + const res = await auth(request(app).put(`/api/admin/feedback/feedback/${foreignFeedbackId}/approve`), editorTok); + expect(res.status).toBe(404); + const row = await db('photo_feedback').where({ id: foreignFeedbackId }).first(); + expect([false, 0]).toContain(row.is_approved); // untouched + }); + + it('editor cannot delete foreign feedback, row survives', async () => { + const res = await auth(request(app).delete(`/api/admin/feedback/feedback/${foreignFeedbackId}`), editorTok); + // Denied either at the events.delete permission layer (editor lacks it → + // 403) or the ownership layer (404) — both must leave the row intact. + expect([403, 404]).toContain(res.status); + expect(await db('photo_feedback').where({ id: foreignFeedbackId }).first()).toBeDefined(); + }); + + it('editor sees no foreign feedback in pending-moderation', async () => { + const res = await auth(request(app).get('/api/admin/feedback/feedback/pending-moderation'), editorTok); + expect(res.status).toBe(200); + expect(res.body.find((f) => f.id === foreignFeedbackId)).toBeUndefined(); + }); + + it('super_admin CAN moderate and see it', async () => { + const pending = await auth(request(app).get('/api/admin/feedback/feedback/pending-moderation'), superTok); + expect(pending.body.find((f) => f.id === foreignFeedbackId)).toBeDefined(); + const res = await auth(request(app).put(`/api/admin/feedback/feedback/${foreignFeedbackId}/approve`), superTok); + expect(res.status).toBe(200); + }); +}); diff --git a/backend/src/middleware/photoAuth.js b/backend/src/middleware/photoAuth.js index df7a1433..ae350f2d 100644 --- a/backend/src/middleware/photoAuth.js +++ b/backend/src/middleware/photoAuth.js @@ -3,6 +3,8 @@ const jwt = require('jsonwebtoken'); const { db } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); const { getGalleryTokenFromRequest } = require('../utils/tokenUtils'); +const { isTokenRevoked } = require('../utils/tokenRevocation'); +const { isTokenBeforeCutoff } = require('../utils/sessionCutoff'); const logger = require('../utils/logger'); async function photoAuth(req, res, next) { @@ -89,7 +91,33 @@ async function photoAuth(req, res, next) { // Check if it's an admin token (admins can view all photos) if (decoded.type === 'admin') { - // For both thumbnails and photos with admin token, allow access + // Enforce the same revocation / session-cutoff invalidation that + // adminAuth does — otherwise a validly-signed admin JWT keeps + // serving photos after logout, password change, or explicit + // revocation (GHSA-x55x). + if (await isTokenRevoked(decoded) || await isTokenBeforeCutoff(decoded)) { + return res.status(401).json({ error: 'Session expired' }); + } + // adminAuth also (a) rejects tokens for a now-deactivated admin and + // (b) rejects any token minted before the admin's last password + // change. isTokenBeforeCutoff is only the GLOBAL restore cutoff, not + // a per-admin password change, so without these two checks a stale + // or pre-password-change admin token still fetches every photo. + const admin = await db('admin_users') + .where({ id: decoded.id, is_active: formatBoolean(true) }) + .select('id', 'password_changed_at') + .first(); + if (!admin) { + return res.status(401).json({ error: 'Session expired' }); + } + if (admin.password_changed_at) { + const passwordChangedSeconds = Math.floor( + new Date(admin.password_changed_at).getTime() / 1000 + ); + if (decoded.iat < passwordChangedSeconds) { + return res.status(401).json({ error: 'Session expired' }); + } + } return next(); } } catch (err) { diff --git a/backend/src/routes/adminAuth.js b/backend/src/routes/adminAuth.js index 898f029c..29e98460 100644 --- a/backend/src/routes/adminAuth.js +++ b/backend/src/routes/adminAuth.js @@ -8,7 +8,7 @@ const { endSession } = require('../middleware/sessionTimeout'); const { validatePasswordStrength } = require('../utils/passwordGenerator'); const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers'); const { NotFoundError, ConflictError, ValidationError } = require('../utils/errors'); -const { setAdminAuthCookie } = require('../utils/tokenUtils'); +const { setAdminAuthCookie, clearAdminAuthCookie } = require('../utils/tokenUtils'); const { IDENTITY_PRESERVING_NORMALIZE_EMAIL } = require('../utils/emailNormalization'); const mfaService = require('../services/mfaService'); const router = express.Router(); @@ -168,12 +168,21 @@ router.post('/change-password', [ // Logout router.post('/logout', adminAuth, handleAsync(async (req, res) => { - // Get token from header - const token = req.headers.authorization?.split(' ')[1]; + // Use the token adminAuth actually authenticated with (req.token) — it may + // have come from the admin_token cookie, not the Authorization header. The + // old header-only read skipped revocation entirely for cookie-based logout, + // leaving the JWT valid until expiry while reporting a successful logout. + const token = req.token; if (token) { - // End the session + // End the in-memory session AND revoke the JWT (GHSA-cjqh) — the token + // is otherwise valid until expiry, so photoAuth/adminAuth would keep + // honouring it after logout. isTokenRevoked() checks this store. endSession(token); + const { revokeToken } = require('../utils/tokenRevocation'); + await revokeToken(token, 'logout'); } + // Clear the auth cookie so the browser stops sending the (now revoked) JWT. + clearAdminAuthCookie(res); // Log activity await logActivity('admin_logout', diff --git a/backend/src/routes/adminCustomers.js b/backend/src/routes/adminCustomers.js index 7eec471b..3ce02cb7 100644 --- a/backend/src/routes/adminCustomers.js +++ b/backend/src/routes/adminCustomers.js @@ -11,6 +11,8 @@ const { body, param, query } = require('express-validator'); const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); const { requireFeatureFlag } = require('../middleware/requireFeatureFlag'); +const { filterOwnedEventIds } = require('../middleware/ownership'); +const { db } = require('../database/db'); // Hour-entry routes are gated by the hoursLogging master so a direct API hit // can't read/edit/delete/bill logged hours while the feature is off (the @@ -528,9 +530,45 @@ router.put('/:id/events', [ body('event_ids.*').isInt({ min: 1 }), ], handleAsync(async (req, res) => { validateRequest(req); + const customerId = parseInt(req.params.id, 10); + const submitted = req.body.event_ids.map(Number); + + // The customer's CURRENT assignments. The "Manage galleries" dialog submits + // the full initial list back — including any events owned by OTHER admins — + // so we need this to tell "retain an existing foreign assignment" apart from + // "newly grant a foreign event". + const existingEventIds = (await db('event_customer_assignments') + .where('customer_account_id', customerId) + .pluck('event_id')).map(Number); + const existingSet = new Set(existingEventIds); + + // Events the caller may act on (GHSA-xr6x). A denied id is only acceptable + // when the customer ALREADY has that assignment (a foreign event the caller + // is merely keeping); a denied id that isn't already assigned is a fresh + // attempt to mint access to a foreign/nonexistent event → reject. + const { allowed } = await filterOwnedEventIds(req.admin, submitted); + const allowedSet = new Set(allowed.map(Number)); + const illegalNew = submitted.filter((id) => !allowedSet.has(id) && !existingSet.has(id)); + if (illegalNew.length) { + return res.status(403).json({ error: 'One or more events are not yours to assign' }); + } + + // setAssignmentsForCustomer replaces the FULL assignment list, deleting any + // existing row not in the submitted set. A restricted admin must not be able + // to revoke another admin's customer↔event links that way, so always retain + // the customer's existing assignments to events the caller does NOT own — + // regardless of whether the client echoed them back. super_admin owns + // everything, so nothing is force-preserved for them. + let finalEventIds = allowed.map(Number); + if (req.admin.roleName !== 'super_admin' && existingEventIds.length) { + const { allowed: ownedExisting } = await filterOwnedEventIds(req.admin, existingEventIds); + const ownedExistingSet = new Set(ownedExisting.map(Number)); + const foreignExisting = existingEventIds.filter((id) => !ownedExistingSet.has(id)); + finalEventIds = [...new Set([...finalEventIds, ...foreignExisting])]; + } const result = await customerAccountsService.setAssignmentsForCustomer( - parseInt(req.params.id, 10), - req.body.event_ids, + customerId, + finalEventIds, req.admin.id, ); successResponse(res, result); diff --git a/backend/src/routes/adminFeedback.js b/backend/src/routes/adminFeedback.js index 32721318..481ccdc8 100644 --- a/backend/src/routes/adminFeedback.js +++ b/backend/src/routes/adminFeedback.js @@ -165,6 +165,24 @@ router.get('/events/:eventId/feedback', } ); +// Ownership guard for by-feedback-id routes (GHSA-2qc2 / GHSA-32h4). These +// take a :feedbackId (not :eventId), so requireEventOwnership can't apply — +// resolve the feedback's event and enforce the same rule (super_admin sees +// all; others need to own the event, or it's ownerless/legacy). Returns +// false and sends a 404 (not 403 — don't leak which feedback ids exist) +// when the caller may not act on it. +async function assertOwnsFeedback(req, res, feedbackId) { + if (req.admin.roleName === 'super_admin') return true; + const fb = await db('photo_feedback').where('id', feedbackId).first('event_id'); + if (!fb) { res.status(404).json({ error: 'Feedback not found' }); return false; } + const event = await db('events').where('id', fb.event_id).first('created_by'); + if (event && event.created_by && event.created_by !== req.admin.id) { + res.status(404).json({ error: 'Feedback not found' }); + return false; + } + return true; +} + // Moderate feedback (approve/hide/reject) router.put('/feedback/:feedbackId/:action', adminAuth, @@ -172,11 +190,12 @@ router.put('/feedback/:feedbackId/:action', async (req, res) => { try { const { feedbackId, action } = req.params; - + if (!['approve', 'hide', 'reject'].includes(action)) { return res.status(400).json({ error: 'Invalid action' }); } - + if (!(await assertOwnsFeedback(req, res, feedbackId))) return; + await feedbackService.moderateFeedback(feedbackId, action, req.admin.id); res.json({ success: true }); @@ -194,7 +213,8 @@ router.delete('/feedback/:feedbackId', async (req, res) => { try { const { feedbackId } = req.params; - + if (!(await assertOwnsFeedback(req, res, feedbackId))) return; + await feedbackService.deleteFeedback(feedbackId, req.admin.id); res.json({ success: true }); @@ -353,7 +373,15 @@ router.get('/feedback/pending-moderation', requirePermission('events.view'), async (req, res) => { try { - const pending = await feedbackService.getPendingModeration(); + // Scope to the caller's owned events unless super_admin (GHSA-3335). + let ownedEventIds = null; + if (req.admin.roleName !== 'super_admin') { + const rows = await db('events') + .where((q) => q.whereNull('created_by').orWhere('created_by', req.admin.id)) + .select('id'); + ownedEventIds = rows.map((r) => r.id); + } + const pending = await feedbackService.getPendingModeration(null, ownedEventIds); res.json(pending); } catch (error) { logger.error('Error getting pending moderation:', error); diff --git a/backend/src/routes/secureImages.js b/backend/src/routes/secureImages.js index 820251fe..9e622071 100644 --- a/backend/src/routes/secureImages.js +++ b/backend/src/routes/secureImages.js @@ -349,6 +349,14 @@ router.get('/:slug/secure-download/:photoId/:token', return res.status(403).json({ error: 'Invalid or expired token' }); } + // Bind the token to the photo it was minted for (GHSA-crxv) — the + // /secure serve route does this, but secure-download did not, so a + // token minted for photo A could download photo B (incl. a hidden one). + const tokenPhotoId = Number(tokenValidation.data?.photoId); + if (!Number.isInteger(tokenPhotoId) || tokenPhotoId !== Number(photoId)) { + return res.status(403).json({ error: 'Token not valid for this photo' }); + } + // Verify photo exists const photo = await db('photos') .where({ id: photoId, event_id: req.event.id }) diff --git a/backend/src/services/feedbackService.js b/backend/src/services/feedbackService.js index 0502e08b..65dacf13 100644 --- a/backend/src/services/feedbackService.js +++ b/backend/src/services/feedbackService.js @@ -504,7 +504,7 @@ class FeedbackService { /** * Get feedback requiring moderation */ - async getPendingModeration(eventId = null) { + async getPendingModeration(eventId = null, ownedEventIds = null) { try { let query = db('photo_feedback') .join('photos', 'photo_feedback.photo_id', 'photos.id') @@ -512,9 +512,13 @@ class FeedbackService { .where('photo_feedback.is_approved', false) .where('photo_feedback.is_hidden', false) .where('photo_feedback.feedback_type', 'comment'); - + if (eventId) { query = query.where('photo_feedback.event_id', eventId); + } else if (Array.isArray(ownedEventIds)) { + // Scope to the caller's owned events (GHSA-3335) — an empty set + // matches nothing, so a restricted admin sees only their own. + query = query.whereIn('photo_feedback.event_id', ownedEventIds.length ? ownedEventIds : [-1]); } const pending = await query diff --git a/backend/src/services/quoteService.js b/backend/src/services/quoteService.js index 91f7d906..470f723b 100644 --- a/backend/src/services/quoteService.js +++ b/backend/src/services/quoteService.js @@ -1029,7 +1029,9 @@ async function sendQuote(id, adminId) { }); try { - await logActivity('quote_sent', { quoteId: id, token }, null, `admin:${adminId}`); + // Do NOT log the raw bearer token — it grants quote actions and the + // activity log is readable later (GHSA-prch). The quoteId is the audit key. + await logActivity('quote_sent', { quoteId: id }, null, `admin:${adminId}`); } catch (_) {} // Fire the quote.sent workflow trigger (best-effort; emit is fail-closed when @@ -1254,7 +1256,8 @@ async function recordResponse({ token, action, ip, tosAccepted }) { }); try { - await logActivity(`quote_${newStatus}`, { quoteId: quote.id, token: tokenRow.token }, null, 'customer:public'); + // Raw bearer token must not reach the activity log (GHSA-prch). + await logActivity(`quote_${newStatus}`, { quoteId: quote.id }, null, 'customer:public'); } catch (_) {} // Defer the workflow emit until the 15-min toggle window locks — so accepting