fix(security): block guest access to hidden/client-only photos across bulk + secure routes (stable) (#940)

* fix(security): block guest access to hidden/client-only photos across bulk + secure routes

* fix(security): harden hidden-photo fix per review (stale ZIP cache, legacy token mint, SQLite bool, client rebuild)

* fix(security): invalidate ZIP cache on photo visibility/category change (codex r2)

* fix(security): recheck photo visibility at signed/secure serve time (TOCTOU) + invalidate ZIP on client visibility change (codex r3)

---------

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

backend/src/routes/adminPhotos.js

@@ -757,6 +757,16 @@ router.patch('/:eventId/photos/:photoId', adminAuth, requirePermission('photos.e
       .where({ id: photoId, event_id: eventId })
       .update(updateData);
 
+    // A visibility or category change alters which photos belong in the
+    // guest download bundle — drop the cached ZIP so it rebuilds fresh,
+    // otherwise a hide→unhide cycle can leave the stale cache omitting
+    // photos added in between (codex review).
+    if (updateData.visibility !== undefined
+        || Object.prototype.hasOwnProperty.call(updateData, 'category_id')
+        || Object.prototype.hasOwnProperty.call(updateData, 'type')) {
+      downloadZipService.invalidate(parseInt(eventId, 10));
+    }
+
     // Fetch and return the updated photo
     const updatedPhoto = await db('photos')
       .where({ id: photoId, event_id: eventId })
@@ -905,6 +915,14 @@ router.post('/:eventId/photos/bulk-update', adminAuth, requirePermission('photos
       .where('event_id', eventId)
       .update(updateData);
 
+    // Visibility/category changes alter the guest download bundle — drop the
+    // cached ZIP so it rebuilds fresh (codex review).
+    if (updateData.visibility !== undefined
+        || Object.prototype.hasOwnProperty.call(updateData, 'category_id')
+        || Object.prototype.hasOwnProperty.call(updateData, 'type')) {
+      downloadZipService.invalidate(parseInt(eventId, 10));
+    }
+
     res.json({ message: `${photoIds.length} photos updated successfully` });
   } catch (error) {
     errorResponse(res, error, 500, 'Failed to update photos');

backend/src/routes/gallery.js

@@ -30,6 +30,7 @@ const { handleAsync, errorResponse } = require('../utils/routeHelpers');
 const { NotFoundError } = require('../utils/errors');
 const { ensureThumbnail, ensureHeroImage, ensurePreviewImage, withLocalCopy } = require('../services/imageProcessor');
 const downloadZipService = require('../services/downloadZipService');
+const { applyPhotoVisibilityFilter, canSeeHiddenPhotos } = require('../utils/photoVisibility');
 const {
   getUseOriginalFilenames,
   pickRawDownloadName,
@@ -784,6 +785,10 @@ router.patch('/:slug/photos/:photoId/visibility', verifyGalleryAccess, async (re
       .where({ id: photoId, event_id: req.event.id })
       .update({ visibility });
 
+    // A client hiding/showing a photo changes the guest download bundle —
+    // drop the cached ZIP so it rebuilds fresh (codex review).
+    downloadZipService.invalidate(req.event.id);
+
     res.json({ message: 'Photo visibility updated', visibility });
   } catch (error) {
     errorResponse(res, error, 500, 'Failed to update photo visibility');
@@ -812,6 +817,10 @@ router.patch('/:slug/photos/visibility/bulk', verifyGalleryAccess, async (req, r
       .where('event_id', req.event.id)
       .update({ visibility });
 
+    // Client bulk hide/show alters the guest download bundle — invalidate
+    // the cached ZIP (codex review).
+    downloadZipService.invalidate(req.event.id);
+
     res.json({ message: `${count} photos updated`, visibility });
   } catch (error) {
     errorResponse(res, error, 500, 'Failed to update photo visibility');
@@ -956,8 +965,21 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async
       return res.status(403).json({ error: 'Downloads are disabled for this gallery' });
     }
 
-    // Try to serve pre-generated zip (instant download with Content-Length)
-    const zipInfo = await downloadZipService.getZipInfo(req.event.id);
+    // Try to serve pre-generated zip (instant download with Content-Length).
+    // Guests may use the prebuilt cache ONLY when the event has no hidden
+    // photos: a cache built before a photo was hidden — or before this
+    // visibility-aware builder shipped — could otherwise still leak it, and
+    // getZipInfo only checks the DB pointer + file stat, not freshness. When
+    // hidden photos exist, guests fall through to the visibility-filtered
+    // stream below. PIN-clients always stream a full archive.
+    const isClient = canSeeHiddenPhotos(req.accessLevel);
+    const eventHasHidden = await db('photos')
+      .where({ event_id: req.event.id, visibility: 'hidden' })
+      .first()
+      .then(Boolean);
+    const zipInfo = (isClient || eventHasHidden)
+      ? null
+      : await downloadZipService.getZipInfo(req.event.id);
     if (zipInfo) {
       const storage = getStorage();
 
@@ -1006,23 +1028,31 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, async
       return;
     }
 
-    // Fallback: on-the-fly streaming (existing behavior)
-    // Also trigger background zip generation for next time
-    downloadZipService.generateZip(req.event.id).catch(err =>
-      logger.warn('Background zip generation failed', { eventId: req.event.id, error: err.message })
-    );
+    // Fallback: on-the-fly streaming (existing behavior). Only pre-build the
+    // guest cache when it will actually be served next time — a guest
+    // download of an event with no hidden photos. Client bypasses and
+    // hidden-photo events always stream, so rebuilding the guest archive on
+    // those requests is wasted I/O (codex review).
+    if (!isClient && !eventHasHidden) {
+      downloadZipService.generateZip(req.event.id).catch(err =>
+        logger.warn('Background zip generation failed', { eventId: req.event.id, error: err.message })
+      );
+    }
 
     // Fetch photos — exclude photos in categories that disabled downloads (#640).
     // Uncategorised photos are always included; categories without the column
     // (pre-migration-135) fall through the LEFT JOIN's null and are included.
-    const photos = await db('photos')
-      .leftJoin('photo_categories', 'photos.category_id', 'photo_categories.id')
-      .where('photos.event_id', req.event.id)
-      .where(function () {
-        this.whereNull('photos.category_id')
-          .orWhere('photo_categories.allow_downloads', true)
-          .orWhereNull('photo_categories.allow_downloads');
-      })
+    const photos = await applyPhotoVisibilityFilter(
+      db('photos')
+        .leftJoin('photo_categories', 'photos.category_id', 'photo_categories.id')
+        .where('photos.event_id', req.event.id)
+        .where(function () {
+          this.whereNull('photos.category_id')
+            .orWhere('photo_categories.allow_downloads', true)
+            .orWhereNull('photo_categories.allow_downloads');
+        }),
+      req.accessLevel
+    )
       .select('photos.*')
       .orderBy('photos.type', 'asc')
       .orderBy('photos.uploaded_at', 'desc');
@@ -1172,15 +1202,18 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken,
 
     // Fetch photos — exclude photos in categories that disabled downloads (#640).
     // Same LEFT JOIN pattern as the download-all endpoint.
-    const photos = await db('photos')
-      .leftJoin('photo_categories', 'photos.category_id', 'photo_categories.id')
-      .where('photos.event_id', req.event.id)
-      .whereIn('photos.id', photoIds)
-      .where(function () {
-        this.whereNull('photos.category_id')
-          .orWhere('photo_categories.allow_downloads', true)
-          .orWhereNull('photo_categories.allow_downloads');
-      })
+    const photos = await applyPhotoVisibilityFilter(
+      db('photos')
+        .leftJoin('photo_categories', 'photos.category_id', 'photo_categories.id')
+        .where('photos.event_id', req.event.id)
+        .whereIn('photos.id', photoIds)
+        .where(function () {
+          this.whereNull('photos.category_id')
+            .orWhere('photo_categories.allow_downloads', true)
+            .orWhereNull('photo_categories.allow_downloads');
+        }),
+      req.accessLevel
+    )
       .select('photos.*')
       .orderBy('photos.uploaded_at', 'desc');
 

backend/src/routes/protectedImages.js

@@ -7,6 +7,7 @@ const secureImageService = require('../services/secureImageService');
 const { getStorage } = require('../services/storage');
 const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('../services/photoResolver');
 const { withLocalCopy } = require('../services/imageProcessor');
+const { isPhotoHiddenFromViewer, canSeeHiddenPhotos } = require('../utils/photoVisibility');
 const crypto = require('crypto');
 const logger = require('../utils/logger');
 const { timingSafeEqualStr } = require('../utils/timingSafe');
@@ -16,10 +17,14 @@ const router = express.Router();
 /**
  * Generate a signed URL token for image access
  */
-function generateImageToken(photoId, expiresIn = 3600) {
+function generateImageToken(photoId, expiresIn = 3600, clientBypass = false) {
   const secret = process.env.JWT_SECRET;
   const expires = Date.now() + (expiresIn * 1000);
-  const data = `${photoId}:${expires}`;
+  // Third segment: whether the minter was a PIN-client, letting the serve
+  // route still deliver a photo hidden AFTER minting (TOCTOU) — a guest's
+  // token carries 0, so it stops working the moment the photo is hidden.
+  // Old two-segment tokens verify unchanged and read the flag as no-bypass.
+  const data = `${photoId}:${expires}:${clientBypass ? 1 : 0}`;
   const signature = crypto.createHmac('sha256', secret).update(data).digest('hex');
   return `${Buffer.from(data).toString('base64')}.${signature}`;
 }
@@ -32,20 +37,23 @@ function verifyImageToken(token) {
     const secret = process.env.JWT_SECRET;
     const [data, signature] = token.split('.');
     const decoded = Buffer.from(data, 'base64').toString();
-    const [photoId, expires] = decoded.split(':');
-    
+    const [photoId, expires, clientFlag] = decoded.split(':');
+
     // Verify signature (constant-time — avoids leaking the HMAC byte-by-byte)
     const expectedSignature = crypto.createHmac('sha256', secret).update(decoded).digest('hex');
     if (!timingSafeEqualStr(signature, expectedSignature)) {
       return null;
     }
-    
+
     // Check expiration
     if (Date.now() > parseInt(expires)) {
       return null;
     }
-    
-    return { photoId: parseInt(photoId), expires: parseInt(expires) };
+    return {
+      photoId: parseInt(photoId),
+      expires: parseInt(expires),
+      clientBypass: clientFlag === '1',
+    };
   } catch (error) {
     return null;
   }
@@ -79,6 +87,12 @@ router.get('/:slug/photo/:photoId/view', verifyGalleryAccess, async (req, res) =
       return res.status(404).json({ error: 'Photo not found' });
     }
 
+    // Block guest access to hidden/client-only photos (parity with the
+    // gallery single-photo routes).
+    if (isPhotoHiddenFromViewer(photo, req.accessLevel)) {
+      return res.status(403).json({ error: 'Photo not available' });
+    }
+
     // Check for suspicious activity
     const isSuspicious = await secureImageService.detectSuspiciousActivity(clientFingerprint, photoId);
     if (isSuspicious) {
@@ -191,15 +205,23 @@ router.post('/:slug/photo/:photoId/generate-secure-token', verifyGalleryAccess,
       return res.status(404).json({ error: 'Photo not found' });
     }
 
+    // Don't mint a secure-image capability for a hidden/client-only photo
+    // when the caller isn't a client — the serve route is token-only.
+    if (isPhotoHiddenFromViewer(photo, req.accessLevel)) {
+      return res.status(403).json({ error: 'Photo not available' });
+    }
+
     // Create client fingerprint
     const clientFingerprint = secureImageService.createClientFingerprint(req);
-    
-    // Generate secure token
+
+    // Generate secure token. clientBypass lets a client's token keep serving
+    // a photo hidden after minting; a guest's stops at the serve route.
     const token = secureImageService.generateSecureToken(photoId, req.sessionID || 'anonymous', {
       expiresIn,
       maxUses: protectionLevel === 'maximum' ? 1 : 3,
       clientFingerprint,
-      protectionLevel
+      protectionLevel,
+      clientBypass: canSeeHiddenPhotos(req.accessLevel)
     });
     
     res.json({ 
@@ -233,9 +255,19 @@ router.post('/:slug/photo/:photoId/generate-url', verifyGalleryAccess, async (re
     if (!photo) {
       return res.status(404).json({ error: 'Photo not found' });
     }
-    
-    // Generate signed token
-    const token = generateImageToken(photoId);
+
+    // Refuse to mint a signed URL for a hidden/client-only photo when the
+    // caller isn't a client. The signed-serve route below is token-only
+    // (no gallery auth), so the access decision has to happen here at mint
+    // time — mirroring how the reveal-bypass flag is baked into the token.
+    if (isPhotoHiddenFromViewer(photo, req.accessLevel)) {
+      return res.status(403).json({ error: 'Photo not available' });
+    }
+
+    // Generate signed token. The client-bypass flag lets a PIN-client's
+    // token keep serving a photo hidden after minting; a guest's token
+    // (clientBypass=0) stops the moment the photo is hidden.
+    const token = generateImageToken(photoId, 3600, canSeeHiddenPhotos(req.accessLevel));
     const signedUrl = `/api/images/${req.params.slug}/photo/${photoId}/signed/${token}`;
     
     res.json({ 
@@ -283,7 +315,14 @@ router.get('/:slug/photo/:photoId/signed/:token', async (req, res) => {
     if (!photo) {
       return res.status(404).json({ error: 'Photo not found' });
     }
-    
+
+    // Recheck visibility at serve time (TOCTOU): a photo hidden AFTER the
+    // URL was minted must stop serving, unless the token was minted by a
+    // client (clientBypass) — mirroring the reveal-mode check above.
+    if (photo.visibility === 'hidden' && !tokenData.clientBypass) {
+      return res.status(403).json({ error: 'Photo not available' });
+    }
+
     // Get watermark settings
     const watermarkSettings = await watermarkService.getWatermarkSettings();
 

backend/src/routes/secureImages.js

@@ -13,6 +13,7 @@ const {
   pickRawDownloadName,
 } = require('../services/downloadFilenameService');
 const { buildContentDisposition } = require('../utils/filenameSanitizer');
+const { isPhotoHiddenFromViewer, canSeeHiddenPhotos } = require('../utils/photoVisibility');
 
 const router = express.Router();
 
@@ -40,6 +41,12 @@ router.post('/:slug/generate-token', async (req, res, next) => {
       return res.status(404).json({ error: 'Photo not found' });
     }
 
+    // Don't mint a secure-image capability for a hidden/client-only photo
+    // when the caller isn't a client (the token is reusable up to 3×).
+    if (isPhotoHiddenFromViewer(photo, req.accessLevel)) {
+      return res.status(403).json({ error: 'Photo not available' });
+    }
+
     // Create client fingerprint
     const clientFingerprint = secureImageService.createClientFingerprint(req);
     
@@ -51,7 +58,10 @@ router.post('/:slug/generate-token', async (req, res, next) => {
       expiresIn: protectionLevel === 'maximum' ? 180 : 300, // 3-5 minutes
       maxUses: accessType === 'download' ? 1 : 3,
       clientFingerprint,
-      protectionLevel
+      protectionLevel,
+      // TOCTOU: a client's token keeps serving a photo hidden after minting;
+      // a guest's stops the moment it's hidden (checked at the serve route).
+      clientBypass: canSeeHiddenPhotos(req.accessLevel)
     };
 
     const token = secureImageService.generateSecureToken(
@@ -174,6 +184,13 @@ router.get('/:slug/secure/:photoId/:token',
         return res.status(404).json({ error: 'Photo not found' });
       }
 
+      // Recheck visibility at serve time (TOCTOU): a photo hidden AFTER the
+      // token was minted must stop serving, unless the token was minted by a
+      // client (clientBypass) — mirroring the reveal-mode check above.
+      if (photo.visibility === 'hidden' && !tokenValidation.data?.clientBypass) {
+        return res.status(403).json({ error: 'Photo not available' });
+      }
+
       // Resolve photo through storage backend (managed) or fall back to local
       // path (external reference mode). secureImageService needs a local file,
       // so we materialize a tmp copy via withLocalCopy in S3 mode.
@@ -330,6 +347,23 @@ router.get('/:slug/secure-download/:photoId/:token',
         return res.status(404).json({ error: 'Photo not found' });
       }
 
+      // Block guest access to hidden/client-only photos.
+      if (isPhotoHiddenFromViewer(photo, req.accessLevel)) {
+        return res.status(403).json({ error: 'Photo not available' });
+      }
+
+      // Per-category download opt-out (#640) — the regular single-photo
+      // download enforces this too; the secure path skipped it. SQLite
+      // returns the boolean as numeric 0, so check both forms.
+      if (photo.category_id) {
+        const cat = await db('photo_categories')
+          .where('id', photo.category_id)
+          .first('allow_downloads');
+        if (cat && (cat.allow_downloads === false || cat.allow_downloads === 0)) {
+          return res.status(403).json({ error: 'Downloads are disabled for this category' });
+        }
+      }
+
       // Resolve photo through storage backend (managed) or local disk (external).
       const storageKey = resolvePhotoStorageKey(req.event, photo);
 

backend/src/services/downloadZipService.js

@@ -112,8 +112,15 @@ class DownloadZipService {
       const event = await db('events').where({ id: eventId }).first();
       if (!event) return { success: false, error: 'Event not found' };
 
+      // The prebuilt zip is served to ordinary gallery guests (the
+      // download-all fast path), so it must exclude hidden/client-only
+      // photos — NULL visibility counts as visible (pre-migration rows).
+      // PIN-clients bypass this cache and stream a full archive instead.
       const photos = await db('photos')
         .where({ event_id: eventId })
+        .where(function () {
+          this.where('visibility', 'visible').orWhereNull('visibility');
+        })
         .select('*')
         .orderBy('type', 'asc')
         .orderBy('uploaded_at', 'desc');

backend/src/services/secureImageService.js

@@ -21,7 +21,11 @@ class SecureImageService {
       expiresIn = 300, // 5 minutes default
       maxUses = 1,
       clientFingerprint = '',
-      protectionLevel = 'standard'
+      protectionLevel = 'standard',
+      // Whether the minter was a PIN-client — lets the serve route keep
+      // delivering a photo hidden AFTER minting (TOCTOU). A guest's token
+      // carries false, so it stops the moment the photo is hidden.
+      clientBypass = false
     } = options;
 
     const tokenData = {
@@ -32,6 +36,7 @@ class SecureImageService {
       maxUses,
       usedCount: 0,
       protectionLevel,
+      clientBypass,
       createdAt: Date.now()
     };
 

backend/src/utils/photoVisibility.js

@@ -0,0 +1,46 @@
+/**
+ * Shared hidden-photo access control.
+ *
+ * PicPeak photos carry a `visibility` column: 'visible' (or NULL, for
+ * pre-migration rows) is shown to everyone; 'hidden' is client-only. A
+ * gallery viewer's `req.accessLevel` is 'client' for a PIN-client login and
+ * something else ('guest'/'slideshow'/…) for an ordinary guest.
+ *
+ * The main photo-list query and the single-photo download/view routes each
+ * enforced this inline, but several bulk/secure paths (download-all,
+ * download-selected, protected-image view, signed-URL mint, secure-token
+ * mint, secure-download) shipped without it — letting ordinary guests reach
+ * hidden/client-only photos. These helpers centralise the rule so every
+ * sink applies exactly the same predicate.
+ */
+
+// PIN-clients see hidden photos; everyone else does not.
+function canSeeHiddenPhotos(accessLevel) {
+  return accessLevel === 'client';
+}
+
+/**
+ * Append the guest visibility filter to a knex `photos` query. No-op for
+ * clients. NULL visibility is treated as visible (pre-migration default).
+ * The query must reference the table as `photos` (all call sites do).
+ */
+function applyPhotoVisibilityFilter(query, accessLevel) {
+  if (canSeeHiddenPhotos(accessLevel)) return query;
+  return query.where(function () {
+    this.where('photos.visibility', 'visible').orWhereNull('photos.visibility');
+  });
+}
+
+/**
+ * Single-photo predicate: true when this photo must be blocked for a viewer
+ * at the given access level. Mirrors the inline guards in gallery.js.
+ */
+function isPhotoHiddenFromViewer(photo, accessLevel) {
+  return !!photo && photo.visibility === 'hidden' && !canSeeHiddenPhotos(accessLevel);
+}
+
+module.exports = {
+  canSeeHiddenPhotos,
+  applyPhotoVisibilityFilter,
+  isPhotoHiddenFromViewer,
+};