From 835312e8e630da69c5a5b43f00f61ed496c113f7 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 10:54:37 +0200 Subject: [PATCH] fix(security): harden four smaller gallery and contract paths, drop the unmounted photo auth middleware - the customer contract PDF stream applies assertContractPdfPath like the admin and public contract routes - OG previews fall back to the site card for draft, archived and deactivated galleries instead of leaking name, date and welcome message - video Range requests are validated before the 206 is written; a NaN, inverted or out-of-file range now answers 416 - share-token comparisons in gallery resolve/info use the constant-time helper share-login already used - middleware/photoAuth.js and the galleryAuth/photoAuth/verifyGalleryAccess exports of middleware/auth.js were unreferenced since the static mounts went; the auth.js copy had neither slug binding nor issuer pin, so it is removed before anyone mounts it --- .../photoAuth.thumbnailScope.test.js | 103 ---------- backend/src/middleware/auth.js | 178 +----------------- backend/src/middleware/photoAuth.js | 177 ----------------- backend/src/routes/customer.js | 9 +- backend/src/routes/gallery.js | 22 ++- backend/src/services/galleryOgService.js | 13 +- 6 files changed, 37 insertions(+), 465 deletions(-) delete mode 100644 backend/__tests__/middleware/photoAuth.thumbnailScope.test.js delete mode 100644 backend/src/middleware/photoAuth.js diff --git a/backend/__tests__/middleware/photoAuth.thumbnailScope.test.js b/backend/__tests__/middleware/photoAuth.thumbnailScope.test.js deleted file mode 100644 index df4050f7..00000000 --- a/backend/__tests__/middleware/photoAuth.thumbnailScope.test.js +++ /dev/null @@ -1,103 +0,0 @@ -/** - * 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/middleware/auth.js b/backend/src/middleware/auth.js index 4e22b7cf..40af97b7 100644 --- a/backend/src/middleware/auth.js +++ b/backend/src/middleware/auth.js @@ -5,7 +5,7 @@ const { isMissingRolesSchema } = require('../utils/dbErrors'); const { isTokenRevoked } = require('../utils/tokenRevocation'); const { isTokenBeforeCutoff } = require('../utils/sessionCutoff'); const logger = require('../utils/logger'); -const { getAdminTokenFromRequest, getGalleryTokenFromRequest } = require('../utils/tokenUtils'); +const { getAdminTokenFromRequest } = require('../utils/tokenUtils'); /** * Enhanced admin authentication middleware with revocation checking @@ -150,180 +150,6 @@ async function adminAuth(req, res, next) { } } -/** - * Enhanced gallery authentication middleware with revocation checking - */ -async function galleryAuth(req, res, next) { - try { - const slug = req.params?.slug || req.requestedSlug; - const token = getGalleryTokenFromRequest(req, slug); - if (!token) { - return res.status(401).json({ error: 'No token provided' }); - } - - let decoded; - try { - decoded = jwt.verify(token, process.env.JWT_SECRET, { - algorithms: ['HS256'], - issuer: 'picpeak-auth', - complete: true - }); - decoded = decoded.payload; - } catch (err) { - if (err.name === 'TokenExpiredError') { - return res.status(401).json({ error: 'Session expired', code: 'TOKEN_EXPIRED' }); - } - return res.status(401).json({ error: 'Invalid session' }); - } - - // Check if token is revoked - if (await isTokenRevoked(decoded)) { - return res.status(401).json({ error: 'Session has been invalidated', code: 'TOKEN_REVOKED' }); - } - - // Reject sessions issued before the global restore cutoff. - if (await isTokenBeforeCutoff(decoded)) { - return res.status(401).json({ error: 'Session invalidated', code: 'SESSION_INVALIDATED' }); - } - - // Verify token type - if (decoded.type !== 'gallery') { - return res.status(403).json({ error: 'Invalid access token' }); - } - - // Check if event still exists and is active - const event = await db('events') - .where({ - id: decoded.eventId, - is_active: true, - is_archived: false - }) - .first(); - - if (!event) { - return res.status(404).json({ error: 'Gallery not found or expired' }); - } - - // Check if gallery has expired (only if expires_at is set) - // Galleries with null expires_at never expire - if (event.expires_at && new Date(event.expires_at) < new Date()) { - return res.status(410).json({ - error: 'Gallery has expired', - code: 'GALLERY_EXPIRED' - }); - } - - // Add event info to request - req.event = event; - req.galleryToken = decoded; - req.token = token; - - next(); - } catch (error) { - logger.error('Gallery auth middleware error:', error); - res.status(401).json({ error: 'Authentication failed' }); - } -} - -/** - * Photo access authentication - * Validates both admin and gallery tokens for photo access - */ -async function photoAuth(req, res, next) { - try { - const slug = req.params?.slug || req.requestedSlug; - const token = getAdminTokenFromRequest(req) || getGalleryTokenFromRequest(req, slug); - if (!token) { - return res.status(401).json({ error: 'Authentication required' }); - } - - let decoded; - try { - decoded = jwt.verify(token, process.env.JWT_SECRET, { algorithms: ['HS256'] }); - } catch (err) { - return res.status(401).json({ error: 'Invalid token' }); - } - - // Check if token is revoked - if (await isTokenRevoked(decoded)) { - return res.status(401).json({ error: 'Token has been revoked', code: 'TOKEN_REVOKED' }); - } - - // Reject sessions issued before the global restore cutoff. - if (await isTokenBeforeCutoff(decoded)) { - return res.status(401).json({ error: 'Session invalidated', code: 'SESSION_INVALIDATED' }); - } - - // Allow both admin and gallery tokens - if (decoded.type === 'admin') { - const admin = await db('admin_users') - .where({ id: decoded.id, is_active: formatBoolean(true) }) - .first(); - - if (!admin) { - return res.status(401).json({ error: 'Invalid token' }); - } - - req.auth = { type: 'admin', user: admin }; - } else if (decoded.type === 'gallery') { - const event = await db('events') - .where({ - id: decoded.eventId, - is_active: true, - is_archived: false - }) - .first(); - - if (!event) { - return res.status(404).json({ error: 'Gallery not found' }); - } - - // For gallery tokens, ensure they can only access their event's photos - req.auth = { type: 'gallery', event: event }; - } else { - return res.status(403).json({ error: 'Invalid token type' }); - } - - next(); - } catch (error) { - logger.error('Photo auth middleware error:', error); - res.status(401).json({ error: 'Authentication failed' }); - } -} - -/** - * Verify gallery access for specific operations - */ -async function verifyGalleryAccess(req, res, next) { - try { - if (!req.auth) { - return res.status(401).json({ error: 'Authentication required' }); - } - - const { eventId } = req.params; - - // Admins can access any gallery - if (req.auth.type === 'admin') { - return next(); - } - - // Gallery tokens can only access their own event - if (req.auth.type === 'gallery') { - if (req.auth.event.id !== parseInt(eventId)) { - return res.status(403).json({ error: 'Access denied' }); - } - return next(); - } - - res.status(403).json({ error: 'Access denied' }); - } catch (error) { - res.status(500).json({ error: 'Access verification failed' }); - } -} - module.exports = { - adminAuth, - galleryAuth, - photoAuth, - verifyGalleryAccess + adminAuth }; diff --git a/backend/src/middleware/photoAuth.js b/backend/src/middleware/photoAuth.js deleted file mode 100644 index faed44fc..00000000 --- a/backend/src/middleware/photoAuth.js +++ /dev/null @@ -1,177 +0,0 @@ -const bcrypt = require('bcrypt'); -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 { isTokenBeforeCutoff } = require('../utils/sessionCutoff'); -const logger = require('../utils/logger'); - -async function photoAuth(req, res, next) { - try { - // Extract event slug from the path - let eventSlug; - - // For thumbnails, we need to parse the filename to get the event info - if (req.path.startsWith('/thumb_')) { - // For now, we'll rely on JWT token for thumbnail access - eventSlug = null; - } else { - // For regular photos, the slug is the first part of the path - eventSlug = req.path.split('/')[1]; - } - - // First check for JWT token (from gallery access) - const tokenFromRequest = getGalleryTokenFromRequest(req, eventSlug); - if (tokenFromRequest) { - const token = tokenFromRequest; - try { - // Try to verify with issuer first, fallback to no issuer for backward compatibility - 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, { algorithms: ['HS256'] }); - } else { - throw issuerError; - } - } - - // Check if it's a gallery token - if (decoded.type === 'gallery') { - // For thumbnails, we need to verify the token is for a valid event - if (!eventSlug) { - // Resolve the token's event (by id, or legacy slug fallback)... - let event = null; - if (decoded.eventId) { - event = await db('events') - .where({ id: decoded.eventId, is_active: formatBoolean(true) }) - .first(); - } - 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(); - } - } - } - // For regular photos, check if token matches the event - else if (decoded.eventSlug === eventSlug) { - const event = await db('events') - .where({ slug: eventSlug, is_active: formatBoolean(true) }) - .first(); - if (event) { - req.event = event; - return next(); - } - } - } - - // Check if it's an admin token (admins can view all photos) - if (decoded.type === 'admin') { - // 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) || await isTokenBeforeCutoff(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. isTokenBeforeCutoff is only the GLOBAL restore cutoff, not - // a per-admin password change, 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) { - // Token invalid, fall through to password check - logger.warn('JWT verification failed in photoAuth', { error: err.message }); - } - } - - // Check for password header (legacy support) - const password = req.headers['x-gallery-password']; - - // If no eventSlug (thumbnails), and we don't have valid auth yet, deny access - if (!eventSlug && !password && !tokenFromRequest) { - return res.status(401).json({ error: 'Authentication required for thumbnails' }); - } - - const event = await db('events').where({ slug: eventSlug, is_active: formatBoolean(true) }).first(); - if (!event) { - return res.status(404).json({ error: 'Gallery not found' }); - } - - const requiresPassword = !(event.require_password === false || event.require_password === 0 || event.require_password === '0'); - - if (!requiresPassword) { - req.event = event; - return next(); - } - - if (!password && !tokenFromRequest) { - return res.status(401).json({ error: 'Authentication required' }); - } - - if (password) { - const validPassword = await bcrypt.compare(password, event.password_hash); - if (!validPassword) { - await db('access_logs').insert({ - event_id: event.id, - ip_address: req.ip, - user_agent: req.headers['user-agent'], - action: 'login_fail' - }); - return res.status(401).json({ error: 'Invalid password' }); - } - } else { - // No valid authentication - return res.status(401).json({ error: 'Invalid authentication' }); - } - - req.event = event; - next(); - } catch (error) { - logger.error('Photo auth error', { error: error.message, stack: error.stack }); - res.status(500).json({ error: 'Authentication error' }); - } -} - -module.exports = photoAuth; diff --git a/backend/src/routes/customer.js b/backend/src/routes/customer.js index 185e7278..b43c818c 100644 --- a/backend/src/routes/customer.js +++ b/backend/src/routes/customer.js @@ -17,6 +17,7 @@ const jwt = require('jsonwebtoken'); const { body, param, validationResult } = require('express-validator'); const { db, logActivity } = require('../database/db'); const { getBcryptRounds, MAX_PASSWORD_LENGTH } = require('../utils/passwordValidation'); +const { assertContractPdfPath } = require('../utils/safePath'); const logger = require('../utils/logger'); const { errorResponse, safeValidationErrors } = require('../utils/routeHelpers'); const { getClientIp } = require('../utils/requestIp'); @@ -705,9 +706,13 @@ router.get('/contracts/:id/pdf', customerAuth, async (req, res) => { res.set('Content-Disposition', `inline; filename="${contract.contract_number}.pdf"`); return res.send(buf); } + // Same containment the admin and public contract routes apply: the DB + // path is written by the service layer today, but a bad row must not + // turn this into an arbitrary-file read. + const safePath = assertContractPdfPath(filePath); res.set('Content-Type', 'application/pdf'); - res.set('Content-Disposition', `inline; filename="${path.basename(filePath)}"`); - fs.createReadStream(filePath).pipe(res); + res.set('Content-Disposition', `inline; filename="${path.basename(safePath)}"`); + fs.createReadStream(safePath).pipe(res); } catch (error) { errorResponse(res, error, 500, 'Failed to render contract PDF'); } diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 3e27d595..a937959a 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -11,6 +11,7 @@ const { getAppSetting } = require('../utils/appSettings'); const archiver = require('archiver'); const path = require('path'); const { resolvePhotoContentType } = require('../utils/photoContentType'); +const { timingSafeEqualStr } = require('../utils/timingSafe'); const router = express.Router(); // #756: a NULL per-event hero_logo_visible means "inherit the global @@ -259,7 +260,7 @@ router.get('/:slug/verify-token/:token', noStoreCache, handleAsync(async (req, r } const expectedToken = getEventShareToken(event); - if (token !== expectedToken) { + if (!expectedToken || !timingSafeEqualStr(String(token), expectedToken)) { throw new NotFoundError('Gallery', 'Invalid gallery link'); } @@ -344,7 +345,7 @@ router.get('/:slug/info', async (req, res) => { // If token provided, verify it matches the share link if (token) { const expectedToken = getEventShareToken(event); - if (!expectedToken || token !== expectedToken) { + if (!expectedToken || !timingSafeEqualStr(String(token), expectedToken)) { return res.status(404).json({ error: 'Invalid gallery link' }); } } @@ -2598,10 +2599,19 @@ router.get('/:slug/photo/:photoId', const parts = range.replace(/bytes=/, '').split('-'); const start = parseInt(parts[0], 10); const end = parts[1] ? parseInt(parts[1], 10) : fileSize - 1; - const chunksize = (end - start) + 1; + // Validate before writing the 206: a NaN, inverted or out-of-file + // range used to be committed to the headers and then throw while + // streaming (or read past the end). + if (!Number.isInteger(start) || !Number.isInteger(end) + || start < 0 || end < start || start >= fileSize) { + res.set('Content-Range', `bytes */${fileSize}`); + return res.status(416).end(); + } + const boundedEnd = Math.min(end, fileSize - 1); + const chunksize = (boundedEnd - start) + 1; res.writeHead(206, { - 'Content-Range': `bytes ${start}-${end}/${fileSize}`, + 'Content-Range': `bytes ${start}-${boundedEnd}/${fileSize}`, 'Accept-Ranges': 'bytes', 'Content-Length': chunksize, 'Content-Type': resolvePhotoContentType(photo), @@ -2610,8 +2620,8 @@ router.get('/:slug/photo/:photoId', }); const file = useStorageBackend - ? await storage.getRange(storageKey, start, end) - : fs.createReadStream(filePath, { start, end }); + ? await storage.getRange(storageKey, start, boundedEnd) + : fs.createReadStream(filePath, { start, end: boundedEnd }); pipeStreamToResponse(file, res, { context: `video range for photo ${photo.id}` }); } else { res.writeHead(200, { diff --git a/backend/src/services/galleryOgService.js b/backend/src/services/galleryOgService.js index 95a14c86..2106d0a4 100644 --- a/backend/src/services/galleryOgService.js +++ b/backend/src/services/galleryOgService.js @@ -169,8 +169,19 @@ async function formatEventDate(value) { } } +// Draft, archived and deactivated galleries are refused by /info; the OG +// preview must not leak their name, date and welcome message to crawlers. +function isPubliclyVisible(event) { + if (!event) return false; + const truthy = (v) => v === true || v === 1 || v === '1' || v === 'true'; + if (truthy(event.is_draft) || truthy(event.is_archived)) return false; + if (event.is_active === false || event.is_active === 0 || event.is_active === '0') return false; + return true; +} + async function buildOgMetadata(slug, requestPath) { - const event = await resolveSlug(slug); + const resolved = await resolveSlug(slug); + const event = isPubliclyVisible(resolved) ? resolved : null; const branding = await fetchBranding(); const base = await frontendBase(); const siteName = branding.companyName || 'PicPeak';