diff --git a/backend/__tests__/middleware/ownership.filterOwnedEventIds.test.js b/backend/__tests__/middleware/ownership.filterOwnedEventIds.test.js new file mode 100644 index 00000000..066080c0 --- /dev/null +++ b/backend/__tests__/middleware/ownership.filterOwnedEventIds.test.js @@ -0,0 +1,78 @@ +/** + * Regression test for the bulk archive/delete ownership bypass. + * + * bulk-archive and bulk-delete acted on body-supplied event ids with no + * ownership filter, so an admin/editor scoped to their own events (the + * single-event routes enforce requireEventOwnership) could archive or + * cascade-delete ANY event by id. filterOwnedEventIds is the helper those + * routes now use to drop foreign/non-existent ids. + */ + +// events owned by admin 7; event 3 owned by someone else; event 4 is +// ownerless (legacy). The mock models: +// whereIn('id', ids).andWhere(created_by IS NULL OR created_by = admin.id) +const EVENTS = [ + { id: 1, created_by: 7 }, + { id: 2, created_by: 7 }, + { id: 3, created_by: 99 }, // foreign + { id: 4, created_by: null }, // ownerless/legacy +]; + +jest.mock('../../src/database/db', () => ({ + db: () => { + const q = { + _ids: null, + _adminId: null, + whereIn(_col, ids) { this._ids = ids; return this; }, + andWhere(cb) { + // Emulate the (created_by IS NULL OR created_by = admin.id) builder + // by capturing the admin id the callback closes over via a probe. + const probe = { + _adminId: null, + whereNull() { return this; }, + orWhere(_col, id) { this._adminId = id; return this; }, + }; + cb(probe); + this._adminId = probe._adminId; + return this; + }, + select() { + return Promise.resolve( + EVENTS + .filter((e) => this._ids.includes(e.id)) + .filter((e) => e.created_by === null || e.created_by === this._adminId) + .map((e) => ({ id: e.id })) + ); + }, + }; + return q; + }, +})); + +const { filterOwnedEventIds } = require('../../src/middleware/ownership'); + +describe('filterOwnedEventIds', () => { + it('super_admin gets every id, nothing denied', async () => { + const { allowed, denied } = await filterOwnedEventIds( + { id: 7, roleName: 'super_admin' }, [1, 3, 4, 999] + ); + expect(allowed).toEqual([1, 3, 4, 999]); + expect(denied).toEqual([]); + }); + + it('non-super_admin keeps owned + ownerless, denies foreign and non-existent', async () => { + const { allowed, denied } = await filterOwnedEventIds( + { id: 7, roleName: 'admin' }, [1, 2, 3, 4, 999] + ); + expect(allowed.sort()).toEqual([1, 2, 4]); // owns 1,2; 4 is ownerless + expect(denied.sort()).toEqual([3, 999]); // 3 foreign, 999 missing + }); + + it('foreign-only request yields empty allowed', async () => { + const { allowed, denied } = await filterOwnedEventIds( + { id: 7, roleName: 'editor' }, [3] + ); + expect(allowed).toEqual([]); + expect(denied).toEqual([3]); + }); +}); diff --git a/backend/__tests__/middleware/photoAuth.thumbnailScope.test.js b/backend/__tests__/middleware/photoAuth.thumbnailScope.test.js new file mode 100644 index 00000000..df4050f7 --- /dev/null +++ b/backend/__tests__/middleware/photoAuth.thumbnailScope.test.js @@ -0,0 +1,103 @@ +/** + * Regression test for the cross-event thumbnail enumeration leak. + * + * Thumbnails are served flat from /thumbnails/thumb_ with + * deterministic, enumerable filenames. photoAuth previously granted any + * holder of a gallery token for ANY active event access to ANY thumbnail + * (it set eventSlug=null and returned next() as long as the token's event + * existed), so a visitor to one gallery could pull another (password- + * protected) gallery's entire thumbnail set. The fix scopes thumbnail + * access to the token's event by matching the requested file against + * photos.thumbnail_path for that event_id. + */ + +process.env.JWT_SECRET = 'test-secret-thumbnail-scope-000000000000'; + +const jwt = require('jsonwebtoken'); + +// Two events, each owning one thumbnail. The photos mock resolves a row +// only when BOTH event_id and thumbnail_path match — i.e. it models the +// real ownership query. +const EVENTS = [ + { id: 10, slug: 'event-a', is_active: 1 }, + { id: 20, slug: 'event-b', is_active: 1 }, +]; +const PHOTOS = [ + { id: 1, event_id: 10, thumbnail_path: 'thumbnails/thumb_event-a_ceremony_0001.jpg' }, + { id: 2, event_id: 20, thumbnail_path: 'thumbnails/thumb_event-b_ceremony_0001.jpg' }, +]; + +jest.mock('../../src/database/db', () => ({ + db: (table) => ({ + _cond: null, + where(cond) { this._cond = cond; return this; }, + first() { + if (table === 'events') { + return Promise.resolve(EVENTS.find((e) => e.id === this._cond.id) || null); + } + if (table === 'photos') { + return Promise.resolve( + PHOTOS.find((p) => p.event_id === this._cond.event_id + && p.thumbnail_path === this._cond.thumbnail_path) || null + ); + } + return Promise.resolve(null); + }, + }), +})); + +jest.mock('../../src/utils/logger', () => ({ + info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn(), +})); + +const photoAuth = require('../../src/middleware/photoAuth'); + +function galleryToken(eventId) { + return jwt.sign({ type: 'gallery', eventId }, process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); +} + +function makeReqRes(token, thumbPath) { + const req = { path: thumbPath, headers: { authorization: `Bearer ${token}` }, cookies: {} }; + const res = { + statusCode: null, + body: null, + status(code) { this.statusCode = code; return this; }, + json(payload) { this.body = payload; return this; }, + }; + return { req, res }; +} + +describe('photoAuth — thumbnail ownership scoping', () => { + it('denies a gallery token for event A fetching event B\'s thumbnail', async () => { + const { req, res } = makeReqRes(galleryToken(10), '/thumb_event-b_ceremony_0001.jpg'); + const next = jest.fn(); + + await photoAuth(req, res, next); + + // Access denied: middleware must not pass the request through. + expect(next).not.toHaveBeenCalled(); + expect(res.statusCode).toBeGreaterThanOrEqual(400); + expect(req.event).toBeUndefined(); + }); + + it('allows a gallery token to fetch its own event\'s thumbnail', async () => { + const { req, res } = makeReqRes(galleryToken(20), '/thumb_event-b_ceremony_0001.jpg'); + const next = jest.fn(); + + await photoAuth(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.event).toMatchObject({ id: 20 }); + }); + + it('denies a traversal / foreign filename that matches no owned thumbnail', async () => { + const { req, res } = makeReqRes(galleryToken(10), '/thumb_../../etc/passwd'); + const next = jest.fn(); + + await photoAuth(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.statusCode).toBeGreaterThanOrEqual(400); + expect(req.event).toBeUndefined(); + }); +}); diff --git a/backend/src/__tests__/verifyGalleryAccess.customerRevoke.test.js b/backend/src/__tests__/verifyGalleryAccess.customerRevoke.test.js index 1ebbefef..55302bbb 100644 --- a/backend/src/__tests__/verifyGalleryAccess.customerRevoke.test.js +++ b/backend/src/__tests__/verifyGalleryAccess.customerRevoke.test.js @@ -101,6 +101,7 @@ describe('verifyGalleryAccess — customer-minted JWT with active assignment', ( it('allows access when the event_customer_assignments row exists', async () => { getGalleryTokenFromRequest.mockReturnValue('tkn'); jwt.verify.mockReturnValue({ + type: 'gallery', eventId: 42, via: 'customer', customerId: 7, @@ -131,6 +132,7 @@ describe('verifyGalleryAccess — customer-minted JWT after revocation', () => { it('returns 403 CUSTOMER_ASSIGNMENT_REVOKED when the junction row is gone', async () => { getGalleryTokenFromRequest.mockReturnValue('tkn'); jwt.verify.mockReturnValue({ + type: 'gallery', eventId: 42, via: 'customer', customerId: 7, @@ -160,6 +162,7 @@ describe('verifyGalleryAccess — customer-minted JWT after revocation', () => { // and start 403'ing per-event-password sessions. getGalleryTokenFromRequest.mockReturnValue('tkn'); jwt.verify.mockReturnValue({ + type: 'gallery', eventId: 42, customerId: 7, // intentionally no `via` claim @@ -191,6 +194,7 @@ describe('verifyGalleryAccess — per-event-password JWT', () => { it('does NOT touch event_customer_assignments and passes through', async () => { getGalleryTokenFromRequest.mockReturnValue('tkn'); jwt.verify.mockReturnValue({ + type: 'gallery', eventId: 42, // No via, no customerId — this is the legacy per-event-password // flow where every guest mints their own JWT after entering the diff --git a/backend/src/middleware/auth.js b/backend/src/middleware/auth.js index 214485f1..ccb49903 100644 --- a/backend/src/middleware/auth.js +++ b/backend/src/middleware/auth.js @@ -18,6 +18,7 @@ async function adminAuth(req, res, next) { let decoded; try { decoded = jwt.verify(token, process.env.JWT_SECRET, { + algorithms: ['HS256'], issuer: 'picpeak-auth', complete: true }); @@ -140,6 +141,7 @@ async function galleryAuth(req, res, next) { let decoded; try { decoded = jwt.verify(token, process.env.JWT_SECRET, { + algorithms: ['HS256'], issuer: 'picpeak-auth', complete: true }); @@ -209,7 +211,7 @@ async function photoAuth(req, res, next) { let decoded; try { - decoded = jwt.verify(token, process.env.JWT_SECRET); + decoded = jwt.verify(token, process.env.JWT_SECRET, { algorithms: ['HS256'] }); } catch (err) { return res.status(401).json({ error: 'Invalid token' }); } diff --git a/backend/src/middleware/customerAuth.js b/backend/src/middleware/customerAuth.js index 1d2e717b..e1b9c545 100644 --- a/backend/src/middleware/customerAuth.js +++ b/backend/src/middleware/customerAuth.js @@ -34,6 +34,7 @@ async function customerAuth(req, res, next) { let decoded; try { const verified = jwt.verify(token, process.env.JWT_SECRET, { + algorithms: ['HS256'], issuer: 'picpeak-auth', complete: true, }); diff --git a/backend/src/middleware/gallery.js b/backend/src/middleware/gallery.js index 8d4ccf56..a1ee968a 100644 --- a/backend/src/middleware/gallery.js +++ b/backend/src/middleware/gallery.js @@ -66,18 +66,29 @@ async function verifyGalleryAccess(req, res, next) { let decoded; try { decoded = jwt.verify(token, process.env.JWT_SECRET, { + algorithms: ['HS256'], issuer: 'picpeak-auth' }); } catch (error) { // If verification fails with issuer, try without issuer (backward compatibility) if (error.name === 'JsonWebTokenError' && error.message.includes('jwt issuer invalid')) { - decoded = jwt.verify(token, process.env.JWT_SECRET); + decoded = jwt.verify(token, process.env.JWT_SECRET, { algorithms: ['HS256'] }); } else { throw error; } } logger.debug('[verifyGalleryAccess] Token decoded successfully', { eventId: decoded.eventId, slug: requestedSlug }); - + + // Only gallery-scoped tokens grant gallery access. Every legitimate + // path (password login, share link, client access, customer-minted, + // slideshow) mints type:'gallery'. Reject anything else — e.g. a guest + // identity token (type:'guest', for feedback attribution) that carries a + // matching eventId — instead of relying on other token types incidentally + // lacking an eventId to fail the id match below. + if (decoded.type !== 'gallery') { + return res.status(403).json({ error: 'Invalid token type for gallery access' }); + } + // If we have a slug in the URL params or from pre-middleware, verify it matches if (requestedSlug) { // Verify by slug and ensure it matches the token's event diff --git a/backend/src/middleware/guestAuth.js b/backend/src/middleware/guestAuth.js index f50e9d1d..a21eddb5 100644 --- a/backend/src/middleware/guestAuth.js +++ b/backend/src/middleware/guestAuth.js @@ -23,6 +23,7 @@ async function resolveGuest(req, res, next) { let decoded; try { const verified = jwt.verify(token, process.env.JWT_SECRET, { + algorithms: ['HS256'], issuer: 'picpeak-auth', complete: true, }); diff --git a/backend/src/middleware/ownership.js b/backend/src/middleware/ownership.js index 7ebad2ff..322c0aad 100644 --- a/backend/src/middleware/ownership.js +++ b/backend/src/middleware/ownership.js @@ -32,4 +32,36 @@ function requireEventOwnership(req, res, next) { }); } -module.exports = { requireEventOwnership }; +/** + * Return the subset of `eventIds` the admin may act on, mirroring + * requireEventOwnership for bulk routes that can't use it (they take an + * array in the body, not an :id param). super_admin gets everything; + * other roles get events they created plus ownerless legacy/system + * events (created_by IS NULL). Ids that are foreign OR non-existent both + * land in `denied` — deliberately indistinguishable, so bulk routes + * don't become an ownership/existence oracle. + * + * @returns {Promise<{allowed: Array, denied: Array}>} + */ +async function filterOwnedEventIds(admin, eventIds) { + if (admin.roleName === 'super_admin') { + return { allowed: [...eventIds], denied: [] }; + } + const rows = await db('events') + .whereIn('id', eventIds) + .andWhere((q) => q.whereNull('created_by').orWhere('created_by', admin.id)) + .select('id'); + const allowedSet = new Set(rows.map((r) => r.id)); + const allowed = []; + const denied = []; + for (const id of eventIds) { + if (allowedSet.has(id) || allowedSet.has(Number(id))) { + allowed.push(id); + } else { + denied.push(id); + } + } + return { allowed, denied }; +} + +module.exports = { requireEventOwnership, filterOwnedEventIds }; diff --git a/backend/src/middleware/photoAuth.js b/backend/src/middleware/photoAuth.js index 6f832aa1..df7a1433 100644 --- a/backend/src/middleware/photoAuth.js +++ b/backend/src/middleware/photoAuth.js @@ -28,12 +28,13 @@ async function photoAuth(req, res, next) { let decoded; try { decoded = jwt.verify(token, process.env.JWT_SECRET, { + algorithms: ['HS256'], issuer: 'picpeak-auth' }); } catch (issuerError) { // If verification fails with issuer, try without issuer (backward compatibility) if (issuerError.name === 'JsonWebTokenError' && issuerError.message.includes('jwt issuer invalid')) { - decoded = jwt.verify(token, process.env.JWT_SECRET); + decoded = jwt.verify(token, process.env.JWT_SECRET, { algorithms: ['HS256'] }); } else { throw issuerError; } @@ -43,24 +44,36 @@ async function photoAuth(req, res, next) { if (decoded.type === 'gallery') { // For thumbnails, we need to verify the token is for a valid event if (!eventSlug) { - // Extract event ID from the decoded token + // Resolve the token's event (by id, or legacy slug fallback)... + let event = null; if (decoded.eventId) { - const event = await db('events') + event = await db('events') .where({ id: decoded.eventId, is_active: formatBoolean(true) }) .first(); - if (event) { + } + if (!event && decoded.eventSlug) { + event = await db('events') + .where({ slug: decoded.eventSlug, is_active: formatBoolean(true) }) + .first(); + } + // ...then confirm the REQUESTED thumbnail actually belongs to + // that event. Thumbnails are stored flat (thumbnails/thumb_) + // with deterministic, enumerable filenames derived from the + // public event name + a sequential counter. Without this + // ownership check any holder of a gallery token for any event + // could enumerate and fetch another (password-protected) event's + // entire thumbnail set, defeating the gallery password. A + // traversal or foreign filename simply fails to match → denied. + if (event) { + const requestedKey = `thumbnails${req.path}`; + const ownsThumbnail = await db('photos') + .where({ event_id: event.id, thumbnail_path: requestedKey }) + .first(); + if (ownsThumbnail) { req.event = event; return next(); } } - // Fallback to slug - const event = await db('events') - .where({ slug: decoded.eventSlug, is_active: formatBoolean(true) }) - .first(); - if (event) { - req.event = event; - return next(); - } } // For regular photos, check if token matches the event else if (decoded.eventSlug === eventSlug) { diff --git a/backend/src/middleware/sessionTimeout.js b/backend/src/middleware/sessionTimeout.js index 3c7968bb..88ef5dc3 100644 --- a/backend/src/middleware/sessionTimeout.js +++ b/backend/src/middleware/sessionTimeout.js @@ -86,7 +86,7 @@ async function sessionTimeoutMiddleware(req, res, next) { try { // Verify token is valid - const decoded = jwt.verify(token, process.env.JWT_SECRET); + const decoded = jwt.verify(token, process.env.JWT_SECRET, { algorithms: ['HS256'] }); // Check if this is an admin token if (!decoded.id) { @@ -127,7 +127,7 @@ async function sessionTimeoutMiddleware(req, res, next) { for (const [oldToken, _] of sessions.entries()) { if (oldToken !== token) { try { - const oldDecoded = jwt.verify(oldToken, process.env.JWT_SECRET); + const oldDecoded = jwt.verify(oldToken, process.env.JWT_SECRET, { algorithms: ['HS256'] }); if (oldDecoded.id === userId) { sessions.delete(oldToken); } diff --git a/backend/src/routes/adminEventRename.js b/backend/src/routes/adminEventRename.js index 00bdbfc4..cc820568 100644 --- a/backend/src/routes/adminEventRename.js +++ b/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 }) diff --git a/backend/src/routes/adminEvents.js b/backend/src/routes/adminEvents.js index 4b30546d..f6040c71 100644 --- a/backend/src/routes/adminEvents.js +++ b/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 { getFrontendBaseUrl } = require('../utils/frontendUrl'); @@ -2260,25 +2260,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 { @@ -2353,16 +2367,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); diff --git a/backend/src/routes/adminExternalMedia.js b/backend/src/routes/adminExternalMedia.js index 8566bfff..de9e6c06 100644 --- a/backend/src/routes/adminExternalMedia.js +++ b/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 || {}; diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 50496a52..56429d8d 100644 --- a/backend/src/routes/adminPhotos.js +++ b/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') { diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index e99b5861..3ad12e8c 100644 --- a/backend/src/routes/auth.js +++ b/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' }); } diff --git a/backend/src/routes/galleryFeedback.js b/backend/src/routes/galleryFeedback.js index db58bc8a..9ca5f00d 100644 --- a/backend/src/routes/galleryFeedback.js +++ b/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'); diff --git a/backend/src/routes/protectedImages.js b/backend/src/routes/protectedImages.js index 15ddbb1b..572ebcd1 100644 --- a/backend/src/routes/protectedImages.js +++ b/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; } diff --git a/backend/src/routes/secureImages.js b/backend/src/routes/secureImages.js index 79093eee..c4335b54 100644 --- a/backend/src/routes/secureImages.js +++ b/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; diff --git a/backend/src/utils/rateLimitSecurity.js b/backend/src/utils/rateLimitSecurity.js index d7614f6a..0ba002c4 100644 --- a/backend/src/utils/rateLimitSecurity.js +++ b/backend/src/utils/rateLimitSecurity.js @@ -32,7 +32,7 @@ function hasValidAdminToken(req) { // Critical: Verify token is valid before skipping rate limit // This prevents invalid tokens from bypassing rate limiting - const decoded = jwt.verify(token, process.env.JWT_SECRET); + const decoded = jwt.verify(token, process.env.JWT_SECRET, { algorithms: ['HS256'] }); // Additional validation if (!decoded || typeof decoded !== 'object') { diff --git a/backend/src/utils/timingSafe.js b/backend/src/utils/timingSafe.js new file mode 100644 index 00000000..9fb4b2e7 --- /dev/null +++ b/backend/src/utils/timingSafe.js @@ -0,0 +1,22 @@ +const crypto = require('crypto'); + +/** + * Constant-time string comparison for secrets (share tokens, HMAC + * signatures, etc.). Returns false for non-strings or length mismatch + * without leaking timing beyond the (non-secret) length. Prevents an + * attacker from recovering a token byte-by-byte via response-time + * differences of a naive `a === b`. + */ +function timingSafeEqualStr(a, b) { + if (typeof a !== 'string' || typeof b !== 'string') { + return false; + } + const ab = Buffer.from(a); + const bb = Buffer.from(b); + if (ab.length !== bb.length) { + return false; + } + return crypto.timingSafeEqual(ab, bb); +} + +module.exports = { timingSafeEqualStr }; diff --git a/frontend/src/components/common/AuthenticatedImage.tsx b/frontend/src/components/common/AuthenticatedImage.tsx index 71ad2c2f..ea862703 100644 --- a/frontend/src/components/common/AuthenticatedImage.tsx +++ b/frontend/src/components/common/AuthenticatedImage.tsx @@ -137,18 +137,26 @@ export const AuthenticatedImage: React.FC = ({ throw new Error('No URL provided'); } - // Build full URL for the image + // Build full URL for the image. Only relative paths are app-owned; + // an absolute URL is passed through untouched. + const isRelative = rawUrl.startsWith('/'); const fullImageUrl = rawUrl.startsWith('/admin') ? buildResourceUrl(`/api${rawUrl}`) - : rawUrl.startsWith('/') + : isRelative ? buildResourceUrl(rawUrl) : rawUrl; const headers: Record = {}; - const slugForRequest = resolveSlug(rawUrl); - const token = getGalleryToken(slugForRequest); - if (token) { - headers.Authorization = `Bearer ${token}`; + // Attach the gallery bearer token ONLY to relative (same-app) image + // paths. Never send it to an absolute/external URL — that would leak + // gallery credentials cross-origin. AuthenticatedImage does not + // support external URLs by design. + if (isRelative) { + const slugForRequest = resolveSlug(rawUrl); + const token = getGalleryToken(slugForRequest); + if (token) { + headers.Authorization = `Bearer ${token}`; + } } const response = await fetch(fullImageUrl, { diff --git a/frontend/src/config/api.ts b/frontend/src/config/api.ts index d2e609ae..697afea4 100644 --- a/frontend/src/config/api.ts +++ b/frontend/src/config/api.ts @@ -56,11 +56,19 @@ api.interceptors.request.use( const pathname = rawPath.startsWith('/') ? rawPath : `/${rawPath}`; - const isGalleryEndpoint = /^\/gallery\//.test(pathname) - || /^\/secure-images\//.test(pathname) - || /^\/auth\/gallery\//.test(pathname); + // Never attach the gallery token to an absolute URL. Requests to the + // app's own API use relative paths (axios prepends baseURL); an + // absolute URL could point at any origin, and extracting its + // `/gallery/...` pathname would otherwise match below and leak the + // bearer token cross-origin. + const isAbsoluteUrl = /^https?:\/\//i.test(config.url || ''); - const isGallerySessionCheck = pathname === '/auth/session' + const isGalleryEndpoint = !isAbsoluteUrl && ( + /^\/gallery\//.test(pathname) + || /^\/secure-images\//.test(pathname) + || /^\/auth\/gallery\//.test(pathname)); + + const isGallerySessionCheck = !isAbsoluteUrl && pathname === '/auth/session' && (!!paramSlug || window.location.pathname.startsWith('/gallery/')); if (isGalleryEndpoint || isGallerySessionCheck) {