fix(security): authz/ownership gaps (token binding, auth revocation, feedback/customer ownership, token logging) (stable) (#951)

* 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<password_changed_at
  check in the admin branch, so a deactivated admin or a pre-password-change
  token can no longer fetch every photo (GHSA-x55x was only revoke+cutoff).
- adminAuth logout: revoke req.token (the token adminAuth authenticated with,
  cookie OR header) instead of header-only, and clear the auth cookie — a
  cookie-based logout previously left the JWT live (GHSA-cjqh).
- adminCustomers PUT /:id/events: preserve the customer's existing
  assignments to events the caller does NOT own, so a restricted admin can't
  revoke another admin's customer-event links via full-list replacement.

* fix(security): codex round-2 — don't 403 legit restricted-admin assignment edits

The Manage-galleries dialog submits the full initial assignment list, so a
restricted admin editing a customer that already has a foreign assignment hit
the denied.length 403 before the preservation logic ran. Reject only
NEWLY-supplied foreign/nonexistent ids; retain foreign ids the customer is
already assigned to (they can't be added or removed by a non-owner).

---------

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

backend/src/middleware/photoAuth.js

@@ -3,6 +3,7 @@ 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 logger = require('../utils/logger');
 
 async function photoAuth(req, res, next) {
@@ -89,7 +90,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)) {
+            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. Token revocation alone doesn't cover those, 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) {

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',

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);

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 });
@@ -348,7 +368,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);

backend/src/routes/secureImages.js

@@ -338,6 +338,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 })

backend/src/services/feedbackService.js

@@ -400,7 +400,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')
@@ -408,9 +408,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

backend/src/services/quoteService.js

@@ -1047,7 +1047,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
@@ -1272,7 +1274,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