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

Auth/access-control audit fixes (all pre-existing on main; none are
regressions). Verified end-to-end where noted.

HIGH
- Thumbnail enumeration: photoAuth granted any gallery token access to any
  flat /thumbnails/thumb_* file, so a visitor to one gallery could
  enumerate another (password-protected) gallery's entire thumbnail set.
  Scope thumbnail access to the token's event via photos.thumbnail_path.
  Live-verified: cross-event fetch now 404s, own-event still 200s.
- Bulk ownership bypass: bulk-archive/bulk-delete acted on body-supplied
  event ids with no owner filter (single-event routes enforce
  requireEventOwnership), letting admin/editor archive or cascade-delete
  any event. Add filterOwnedEventIds; also guard rename + import-external;
  tighten photo-retry to scope admin (not just editor). Fix misleading
  bulk-delete comment.

MED
- verifyGalleryAccess never checked decoded.type — assert 'gallery'
  instead of relying on other token types incidentally lacking eventId.
- secure-images generate-token/secure-download missing denySlideshowToken
  (#646 bypass): a leaked slideshow token could download originals.
- Frontend: AuthenticatedImage + api.ts attached the gallery bearer token
  to absolute/external URLs — only attach to relative same-app paths.

LOW hardening
- Pin algorithms:['HS256'] on all auth-boundary jwt.verify calls.
- crypto.timingSafeEqual for share-token + HMAC compares (utils/timingSafe).
- Remove dead photoAuth import in galleryFeedback.

Tests: new regression suites for thumbnail scoping + filterOwnedEventIds;
fixed verifyGalleryAccess.customerRevoke fixture (real customer tokens
carry type:'gallery'). Full backend suite at the pre-existing baseline
(5 suites/27 tests fail on main too), zero new failures.
This commit is contained in:
Paul Nothaft
2026-07-03 10:27:28 +02:00
parent e513e8345b
commit 081f3edcdf
22 changed files with 372 additions and 60 deletions
@@ -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]);
});
});
@@ -0,0 +1,103 @@
/**
* Regression test for the cross-event thumbnail enumeration leak.
*
* Thumbnails are served flat from /thumbnails/thumb_<name> 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();
});
});
@@ -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
+3 -1
View File
@@ -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' });
}
+1
View File
@@ -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,
});
+13 -2
View File
@@ -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
+1
View File
@@ -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,
});
+33 -1
View File
@@ -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 };
+25 -12
View File
@@ -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_<name>)
// 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) {
+2 -2
View File
@@ -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);
}
+3 -2
View File
@@ -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 })
+37 -18
View File
@@ -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);
+2 -1
View File
@@ -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 || {};
+10 -4
View File
@@ -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') {
+2 -1
View File
@@ -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' });
}
-1
View File
@@ -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');
+3 -2
View File
@@ -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;
}
+3 -2
View File
@@ -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;
+1 -1
View File
@@ -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') {
+22
View File
@@ -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 };
@@ -137,18 +137,26 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
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<string, string> = {};
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, {
+12 -4
View File
@@ -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) {