fix(security): close authorization/ownership gaps (token scope, mass-assignment, category hero, project docs) (#943)

* fix(security): close authorization/ownership gaps (token scope, mass-assignment, category hero, project docs)

* fix(security): block archive columns in event mass-assignment per review

* fix(security): comprehensive event mass-assignment denylist + deal-cascade cross-domain permission gate (codex r2)

* fix(security): case-insensitive complete event denylist + project_id + empty-update no-op (codex r3)

---------

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

backend/src/routes/adminApiTokens.js

@@ -20,7 +20,10 @@ const router = express.Router();
 // the plaintext, never recoverable after creation.
 router.get('/', adminAuth, requirePermission('settings.view'), async (req, res) => {
   try {
-    const tokens = await db('api_tokens')
+    // Scope to the caller's own tokens unless super_admin — the previous
+    // query returned every admin's token metadata (name/preview/scopes/
+    // owner) to any settings.view holder (GHSA-jm7j).
+    const tokensQuery = db('api_tokens')
       .leftJoin('admin_users', 'admin_users.id', 'api_tokens.created_by')
       .select(
         'api_tokens.id',
@@ -34,6 +37,10 @@ router.get('/', adminAuth, requirePermission('settings.view'), async (req, res)
         'admin_users.username as owner_username'
       )
       .orderBy('api_tokens.created_at', 'desc');
+    if (req.admin.roleName !== 'super_admin') {
+      tokensQuery.where('api_tokens.created_by', req.admin.id);
+    }
+    const tokens = await tokensQuery;
     // toIso: last_used_at / revoked_at were written as raw Dates before
     // this fix — SQLite installs hold epoch numbers in existing rows.
     res.json(tokens.map((t) => ({
@@ -110,6 +117,12 @@ router.delete('/:id', adminAuth, requirePermission('settings.edit'), async (req,
     const { id } = req.params;
     const row = await db('api_tokens').where({ id }).first();
     if (!row) return res.status(404).json({ error: 'Token not found' });
+    // Only the token's owner (or a super_admin) may revoke it — otherwise
+    // any settings.edit holder could revoke another admin's tokens
+    // (GHSA-gprq). 404 rather than 403 so a non-owner can't probe token ids.
+    if (req.admin.roleName !== 'super_admin' && row.created_by !== req.admin.id) {
+      return res.status(404).json({ error: 'Token not found' });
+    }
     if (row.revoked_at) return res.status(400).json({ error: 'Token already revoked' });
 
     await db('api_tokens').where({ id }).update({ revoked_at: new Date().toISOString() });

backend/src/routes/adminCategories.js

@@ -152,8 +152,18 @@ router.put('/:id', adminAuth, requirePermission('settings.edit'), [
         .trim()
     };
 
-    // Update hero_photo_id if provided (including null to clear it)
+    // Update hero_photo_id if provided (including null to clear it). A
+    // non-null hero must belong to this category (GHSA-j2f4) — the general
+    // update path previously wrote it with no membership check at all.
     if (Object.prototype.hasOwnProperty.call(req.body, 'hero_photo_id')) {
+      if (hero_photo_id) {
+        const heroPhoto = await db('photos')
+          .where({ id: hero_photo_id, category_id: id })
+          .first();
+        if (!heroPhoto) {
+          return res.status(404).json({ error: 'Photo not found in this category' });
+        }
+      }
       updateData.hero_photo_id = hero_photo_id || null;
     }
 
@@ -203,11 +213,16 @@ router.put('/:id/hero', adminAuth, requirePermission('settings.edit'), [
       return res.status(404).json({ error: 'Category not found' });
     }
 
-    // If hero_photo_id is provided, verify it belongs to a photo in this category
+    // If hero_photo_id is provided, verify the photo actually belongs to
+    // THIS category — checking existence alone let an admin point a
+    // category's hero at a photo from a different category or event
+    // (GHSA-j2f4).
     if (hero_photo_id) {
-      const photo = await db('photos').where('id', hero_photo_id).first();
+      const photo = await db('photos')
+        .where({ id: hero_photo_id, category_id: id })
+        .first();
       if (!photo) {
-        return res.status(404).json({ error: 'Photo not found' });
+        return res.status(404).json({ error: 'Photo not found in this category' });
       }
     }
 

backend/src/routes/adminEvents/crud.js

@@ -1278,6 +1278,52 @@ module.exports = (router) => {
 
       const { id } = req.params;
       const updates = { ...req.body };
+
+      // Strip identity/provenance/secret columns from the mass-assigned
+      // body (GHSA-3rqx). The handler spreads req.body straight into the
+      // events UPDATE, so without this an events.edit holder could rewrite
+      // ownership (created_by), routing identity (slug/share_link), the
+      // share/client tokens, or the password hashes directly. Plaintext
+      // `password`/`client_password` inputs are NOT stripped — those are the
+      // supported way to change credentials and get hashed below; the
+      // tokens are regenerated internally where needed.
+      // The handler spreads req.body straight into the events UPDATE, so any
+      // column an events.edit holder names is writable unless blocked here.
+      // This is a COMPLETE deny-set of every server-managed / permission-gated
+      // events column (enumerated from the schema); everything else is a
+      // legitimate edit-form field and passes through, including input-only
+      // keys (password/client_password) the handler transforms below. New
+      // server-managed columns MUST be added here. (codex review — GHSA-3rqx.)
+      const IMMUTABLE_EVENT_COLUMNS = [
+        // Identity / provenance
+        'id', 'created_by', 'created_at', 'updated_at', 'slug',
+        // Routing + share/client tokens (generated at create / internally)
+        'share_link', 'share_token', 'client_share_token', 'show_share_token',
+        // Secrets (set via the plaintext password/client_password inputs)
+        'password_hash', 'client_password_hash',
+        // Server-consumed file paths — e.g. DELETE /:id/logo fs.unlink()s
+        // hero_logo_path, so a forged value is an arbitrary-delete primitive.
+        'hero_logo_path', 'hero_logo_url', 'archive_path', 'download_zip_path',
+        // Server-managed timestamps
+        'download_zip_generated_at', 'archived_at', 'revealed_at', 'event_reminder_sent_at',
+        // Lifecycle — governed by dedicated permission-gated routes
+        // (events.archive/restore, publish, activate/deactivate), not events.edit.
+        'is_archived', 'is_draft', 'is_active',
+        // Relationships — managed by projectService.assignEvent + its
+        // customer-consistency checks, and events.edit ≠ quotes/contracts perms.
+        'project_id', 'quote_id',
+        // Legacy mirrors — rejected explicitly below in favour of customer_*.
+        'host_name', 'host_email',
+      ];
+      // Case-insensitive match: SQLite treats quoted identifiers
+      // case-insensitively, so a `{ "Password_Hash": ... }` key would
+      // otherwise survive a case-sensitive delete and still hit the real
+      // column (codex review).
+      const denied = new Set(IMMUTABLE_EVENT_COLUMNS.map((c) => c.toLowerCase()));
+      for (const key of Object.keys(updates)) {
+        if (denied.has(key.toLowerCase())) delete updates[key];
+      }
+
       const customerColumnsAvailable = await hasCustomerContactColumns();
 
       if (Object.prototype.hasOwnProperty.call(updates, 'host_name') || Object.prototype.hasOwnProperty.call(updates, 'host_email')) {
@@ -1549,10 +1595,15 @@ module.exports = (router) => {
         }
       }
 
-      // Update event
-      await db('events')
-        .where('id', id)
-        .update(updates);
+      // Update event. Skip the write when the denylist (or masked secrets)
+      // left nothing to change — Knex rejects .update({}) with an error,
+      // which would surface as a 500 for an otherwise-valid no-op request
+      // (e.g. a body of only protected fields). (codex review.)
+      if (Object.keys(updates).length > 0) {
+        await db('events')
+          .where('id', id)
+          .update(updates);
+      }
 
       // Customer-account assignments (#354). Same skip semantics as POST:
       // ignore when the customer portal flag is off so stale tabs don't

backend/src/routes/adminProjects.js

@@ -15,8 +15,31 @@ const { requirePermission, userHasAnyPermission } = require('../middleware/permi
 const { handleAsync, validateRequest, successResponse } = require('../utils/routeHelpers');
 const projectService = require('../services/projectService');
 const { db } = require('../database/db');
+const { ForbiddenError } = require('../utils/errors');
 
 const router = express.Router();
+
+// A deal that spans both quotes and contracts cascades a project link across
+// BOTH tables (projectService.linkDealToProject). So attaching one document
+// must also require manage permission on the OTHER domain the cascade will
+// touch — otherwise quotes.manage alone could re-point a linked contract, and
+// vice versa (GHSA-v4vw / codex review). No-op when the deal touches only the
+// one domain, or on older instances without the deal_uuid column.
+async function assertCascadePermitted(req, docTable, docId, otherTable, otherPerm) {
+  let doc;
+  try {
+    doc = await db(docTable).where({ id: docId }).first('deal_uuid');
+  } catch { return; }
+  if (!doc || !doc.deal_uuid) return;
+  let linked;
+  try {
+    linked = await db(otherTable).where({ deal_uuid: doc.deal_uuid }).first('id');
+  } catch { return; }
+  if (!linked) return;
+  if (!(await userHasAnyPermission(req.admin.id, [otherPerm]))) {
+    throw new ForbiddenError(`This deal also links a ${otherTable.replace(/s$/, '')}; the ${otherPerm} permission is required`);
+  }
+}
 router.use(adminAuth);
 
 // Projects is feature-flagged like bills/quotes — when off, the whole cockpit
@@ -105,23 +128,30 @@ router.post('/:id/events',
 );
 
 // Attach a quote to the project (quotes carry no event_id — migration 121).
+// Requires quotes.manage in addition to events.edit — attaching a quote
+// mutates a separately-permissioned document domain (GHSA-v4vw).
 router.post('/:id/quotes',
-  requirePermission('events.edit'),
+  requirePermission(['events.edit', 'quotes.manage'], { requireAll: true }),
   [param('id').isInt({ min: 1 }), body('quoteId').isInt({ min: 1 })],
   handleAsync(async (req, res) => {
     validateRequest(req);
-    const result = await projectService.assignQuote(parseInt(req.params.id, 10), parseInt(req.body.quoteId, 10));
+    const quoteId = parseInt(req.body.quoteId, 10);
+    await assertCascadePermitted(req, 'quotes', quoteId, 'contracts', 'contracts.manage');
+    const result = await projectService.assignQuote(parseInt(req.params.id, 10), quoteId);
     return successResponse(res, result, 200, 'Quote attached to project');
   }),
 );
 
-// Attach a contract to the project.
+// Attach a contract to the project. Requires contracts.manage in addition
+// to events.edit (GHSA-v4vw).
 router.post('/:id/contracts',
-  requirePermission('events.edit'),
+  requirePermission(['events.edit', 'contracts.manage'], { requireAll: true }),
   [param('id').isInt({ min: 1 }), body('contractId').isInt({ min: 1 })],
   handleAsync(async (req, res) => {
     validateRequest(req);
-    const result = await projectService.assignContract(parseInt(req.params.id, 10), parseInt(req.body.contractId, 10));
+    const contractId = parseInt(req.body.contractId, 10);
+    await assertCascadePermitted(req, 'contracts', contractId, 'quotes', 'quotes.manage');
+    const result = await projectService.assignContract(parseInt(req.params.id, 10), contractId);
     return successResponse(res, result, 200, 'Contract attached to project');
   }),
 );