Merge pull request #737 from PicPeak/fix/auth-access-control

fix(security): cross-event thumbnail leak, bulk-op ownership bypass + auth hardening

backend/src/routes/adminEventRename.js

@@ -7,6 +7,7 @@ const express = require('express');
 const { body, validationResult } = require('express-validator');
 const { adminAuth } = require('../middleware/auth');
 const { requirePermission } = require('../middleware/permissions');
+const { requireEventOwnership } = require('../middleware/ownership');
 const eventRenameService = require('../services/eventRenameService');
 const router = express.Router();
 
@@ -14,7 +15,7 @@ const router = express.Router();
  * POST /api/admin/events/:eventId/rename
  * Rename an event
  */
-router.post('/:eventId/rename', adminAuth, requirePermission('events.edit'), [
+router.post('/:eventId/rename', adminAuth, requirePermission('events.edit'), requireEventOwnership, [
   body('newEventName')
     .trim()
     .isLength({ min: 3, max: 100 })
@@ -59,7 +60,7 @@ router.post('/:eventId/rename', adminAuth, requirePermission('events.edit'), [
  * POST /api/admin/events/:eventId/validate-rename
  * Validate a potential rename without executing it
  */
-router.post('/:eventId/validate-rename', adminAuth, requirePermission('events.edit'), [
+router.post('/:eventId/validate-rename', adminAuth, requirePermission('events.edit'), requireEventOwnership, [
   body('newEventName')
     .trim()
     .isLength({ min: 3, max: 100 })

backend/src/routes/adminEvents.js

@@ -24,7 +24,7 @@ const eventTypeService = require('../services/eventTypeService');
 const { normaliseEventTimeTriple } = require('../services/eventService');
 const { hasColumnCached } = require('../utils/schemaCache');
 const { validateFileType } = require('../utils/fileSecurityUtils');
-const { requireEventOwnership } = require('../middleware/ownership');
+const { requireEventOwnership, filterOwnedEventIds } = require('../middleware/ownership');
 const { requireFeatureFlag } = require('../middleware/requireFeatureFlag');
 const { getAppSetting } = require('../utils/appSettings');
 const { clampIntOrUndefined } = require('../utils/numericHelpers');
@@ -2267,25 +2267,39 @@ router.post('/bulk-archive', adminAuth, requirePermission('events.archive'), [
     }
 
     const { eventIds } = req.body;
-    
+
     if (eventIds.length === 0) {
       return res.status(400).json({ error: 'No events selected for archiving' });
     }
 
-    // Get all events to archive
-    const events = await db('events')
-      .whereIn('id', eventIds)
-      .where('is_archived', formatBoolean(false));
-
-    if (events.length === 0) {
-      return res.status(400).json({ error: 'No valid events found to archive' });
-    }
+    // Ownership scope: a non-super_admin may only archive events they own.
+    // Foreign/non-existent ids are dropped and reported as failures so this
+    // route can't archive another admin's events (the single-event
+    // /:id/archive route enforces the same via requireEventOwnership).
+    const { allowed: allowedIds, denied: deniedIds } = await filterOwnedEventIds(req.admin, eventIds);
 
     const results = {
       successful: [],
-      failed: []
+      failed: deniedIds.map((id) => ({ id, name: null, error: 'Access denied or event not found' }))
     };
 
+    // Get all events to archive
+    const events = allowedIds.length
+      ? await db('events')
+        .whereIn('id', allowedIds)
+        .where('is_archived', formatBoolean(false))
+      : [];
+
+    if (events.length === 0) {
+      if (results.failed.length > 0) {
+        return res.json({
+          message: `Bulk archive completed: 0 succeeded, ${results.failed.length} failed`,
+          results
+        });
+      }
+      return res.status(400).json({ error: 'No valid events found to archive' });
+    }
+
     // Process each event
     for (const event of events) {
       try {
@@ -2360,16 +2374,21 @@ router.post('/bulk-delete', adminAuth, requirePermission('events.delete'), [
 
     const { eventIds } = req.body;
 
-    // Editor-role events.delete permission is already gated by the route
-    // middleware. We do NOT additionally filter to created_by here because
-    // the per-event delete-cascade is global (matches DELETE /:id which
-    // also has no role-based filter — that's why events.delete is a
-    // sensitive permission).
+    // Ownership scope: a non-super_admin may only delete events they own.
+    // The single-event DELETE /:id route enforces this via
+    // requireEventOwnership; this bulk route must match it, otherwise an
+    // admin/editor scoped to their own events could cascade-delete any
+    // event by id. Foreign/non-existent ids are dropped and reported as
+    // failures (indistinguishable, to avoid an existence oracle).
+    const { allowed: allowedIds, denied: deniedIds } = await filterOwnedEventIds(req.admin, eventIds);
 
-    const results = { successful: [], failed: [] };
+    const results = {
+      successful: [],
+      failed: deniedIds.map((id) => ({ id, name: null, error: 'Access denied or event not found' }))
+    };
     const adminContext = { id: req.admin.id, username: req.admin.username };
 
-    for (const eventId of eventIds) {
+    for (const eventId of allowedIds) {
       try {
         const deleted = await deleteEventCascade(eventId, adminContext);
         results.successful.push(deleted);

backend/src/routes/adminExternalMedia.js

@@ -3,6 +3,7 @@ const path = require('path');
 const fs = require('fs').promises;
 const { adminAuth } = require('../middleware/auth');
 const { requirePermission } = require('../middleware/permissions');
+const { requireEventOwnership } = require('../middleware/ownership');
 const { list, resolveExternalPath, getExternalMediaRoot } = require('../services/externalMediaService');
 const { db, logActivity } = require('../database/db');
 const sharp = require('sharp');
@@ -48,7 +49,7 @@ async function walkDir(dir, baseDir) {
 
 // POST /api/admin/events/:id/import-external
 // Body: { external_path: string, recursive?: boolean, map?: { individual?: string, collages?: string } }
-router.post('/events/:id/import-external', adminAuth, requirePermission('photos.upload'), async (req, res) => {
+router.post('/events/:id/import-external', adminAuth, requirePermission('photos.upload'), requireEventOwnership, async (req, res) => {
   try {
     const eventId = parseInt(req.params.id);
     const { external_path, recursive = true, map = { individual: 'individual', collages: 'collages' } } = req.body || {};

backend/src/routes/adminPhotos.js

@@ -601,12 +601,18 @@ router.post(
       const photo = await db('photos').where({ id: req.params.photoId }).first();
       if (!photo) return res.status(404).json({ error: 'Photo not found' });
 
-      // Editor role: only allow retry on photos in events they own.
-      if (req.admin.roleName === 'editor') {
+      // Ownership scope: any non-super_admin may only retry photos in events
+      // they own — matching requireEventOwnership (which scopes both the
+      // admin and editor roles; only super_admin bypasses). Previously this
+      // checked the editor role alone, leaving admin-role users able to
+      // reprocess another admin's photos.
+      if (req.admin.roleName !== 'super_admin') {
         const event = await db('events')
-          .where({ id: photo.event_id, created_by: req.admin.id })
+          .where({ id: photo.event_id })
           .first();
-        if (!event) return res.status(404).json({ error: 'Photo not found' });
+        if (event && event.created_by && event.created_by !== req.admin.id) {
+          return res.status(404).json({ error: 'Photo not found' });
+        }
       }
 
       if (photo.processing_status !== 'failed') {

backend/src/routes/auth.js

@@ -14,6 +14,7 @@ const {
 } = require('../utils/authSecurity');
 const { endSession } = require('../middleware/sessionTimeout');
 const { revokeToken } = require('../utils/tokenRevocation');
+const { timingSafeEqualStr } = require('../utils/timingSafe');
 const logger = require('../utils/logger');
 const {
   setAdminAuthCookie,
@@ -413,7 +414,7 @@ router.post('/gallery/share-login', [
 
     const expectedToken = getEventShareToken(event);
 
-    if (!expectedToken || token !== expectedToken) {
+    if (!expectedToken || !timingSafeEqualStr(token, expectedToken)) {
       await trackFailedAttempt(shareIdentifier, ipAddress, userAgent);
       return res.status(401).json({ error: 'Invalid or expired share link' });
     }

backend/src/routes/galleryFeedback.js

@@ -1,6 +1,5 @@
 const express = require('express');
 const router = express.Router();
-const { photoAuth } = require('../middleware/photoAuth');
 const { verifyGalleryAccess, denySlideshowToken } = require('../middleware/gallery');
 const { feedbackRateLimit, generateGuestIdentifier } = require('../middleware/feedbackRateLimit');
 const { resolveGuest } = require('../middleware/guestAuth');

backend/src/routes/protectedImages.js

@@ -8,6 +8,7 @@ const { getStorage } = require('../services/storage');
 const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('../services/photoResolver');
 const { withLocalCopy } = require('../services/imageProcessor');
 const crypto = require('crypto');
+const { timingSafeEqualStr } = require('../utils/timingSafe');
 
 const router = express.Router();
 
@@ -32,9 +33,9 @@ function verifyImageToken(token) {
     const decoded = Buffer.from(data, 'base64').toString();
     const [photoId, expires] = decoded.split(':');
     
-    // Verify signature
+    // Verify signature (constant-time — avoids leaking the HMAC byte-by-byte)
     const expectedSignature = crypto.createHmac('sha256', secret).update(decoded).digest('hex');
-    if (signature !== expectedSignature) {
+    if (!timingSafeEqualStr(signature, expectedSignature)) {
       return null;
     }
     

backend/src/routes/secureImages.js

@@ -1,6 +1,6 @@
 const express = require('express');
 const { db } = require('../database/db');
-const { verifyGalleryAccess } = require('../middleware/gallery');
+const { verifyGalleryAccess, denySlideshowToken } = require('../middleware/gallery');
 const secureImageService = require('../services/secureImageService');
 const secureImageMiddleware = require('../middleware/secureImageMiddleware');
 const logger = require('../utils/logger');
@@ -23,7 +23,7 @@ router.post('/:slug/generate-token', async (req, res, next) => {
   // Add slug to request for verifyGalleryAccess
   req.requestedSlug = req.params.slug;
   next();
-}, verifyGalleryAccess, async (req, res) => {
+}, verifyGalleryAccess, denySlideshowToken, async (req, res) => {
   try {
     const { photoId, accessType = 'view' } = req.body;
     
@@ -273,6 +273,7 @@ router.get('/:slug/secure-download/:photoId/:token',
     next();
   },
   verifyGalleryAccess,
+  denySlideshowToken,
   async (req, res) => {
     try {
       const { photoId, token } = req.params;