From 839bf4e46440a938d749dc0ec48841a9c4891475 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 10:51:53 +0200 Subject: [PATCH] fix(security): close four middleware gaps around the API edge - maintenance mode classified paths case-sensitively while Express routes case-insensitively, so /API/... walked past the gate - the general rate limiter skipped anyone holding any verified JWT; a gallery token is minted for free on password-less galleries and slideshow links, so that was an unlimited budget for every /api route. Only admin sessions skip now - ?admin_preview=1 trusted a verified signature alone; it now applies the same revocation, restore-cutoff, deactivation and password-change checks adminAuth does, and reveal-mode reads the verified flag instead of re-decoding the token - the 50mb JSON limit is scoped to /api/admin and /api/v1; everything else gets 2mb, so an unauthenticated body can no longer stall JSON.parse - the CSRF Content-Type gate accepted multipart from any origin; cross-site form posts are now rejected via Sec-Fetch-Site / Origin, with a Host match fallback for same-origin installs that leave FRONTEND_URL unset --- .../securityHardeningBatch2.test.js | 123 ++++++++++++++++++ backend/server.js | 37 +++--- backend/src/middleware/gallery.js | 51 +++++++- backend/src/middleware/maintenance.js | 8 +- backend/src/routes/gallery.js | 4 +- backend/src/services/rateLimitService.js | 9 +- backend/src/utils/requestOrigin.js | 42 ++++++ backend/src/utils/revealMode.js | 9 +- 8 files changed, 249 insertions(+), 34 deletions(-) create mode 100644 backend/__tests__/middleware/securityHardeningBatch2.test.js create mode 100644 backend/src/utils/requestOrigin.js diff --git a/backend/__tests__/middleware/securityHardeningBatch2.test.js b/backend/__tests__/middleware/securityHardeningBatch2.test.js new file mode 100644 index 00000000..e255c4a0 --- /dev/null +++ b/backend/__tests__/middleware/securityHardeningBatch2.test.js @@ -0,0 +1,123 @@ +/** + * Second security sweep on the same branch as the password-strength DoS fix. + * Each block pins one gap the audit found: + * + * - maintenance gate classified paths case-sensitively while Express routes + * case-insensitively, so /API/... bypassed maintenance mode + * - the general rate limiter skipped anyone holding ANY verified JWT, + * including a gallery token minted for free on password-less galleries + * - the admin gallery preview trusted a verified signature alone, ignoring + * revocation, deactivation and password changes + * - the multipart branch of the CSRF Content-Type gate accepted cross-site + * form posts + */ +const jwt = require('jsonwebtoken'); + +process.env.JWT_SECRET = 'hardening-batch2-secret'; + +const fake = { maintenance: 'true', revoked: false, beforeCutoff: false, admin: { id: 1, password_changed_at: null } }; + +jest.mock('../../src/database/db', () => { + const db = jest.fn((table) => { + const q = { + where: jest.fn().mockReturnThis(), + select: jest.fn().mockReturnThis(), + first: jest.fn(async () => { + if (table === 'app_settings') { + return { setting_key: 'general_maintenance_mode', setting_value: fake.maintenance }; + } + if (table === 'admin_users') return fake.admin; + return null; + }), + }; + return q; + }); + return { db, withRetry: (fn) => fn() }; +}); +jest.mock('../../src/utils/logger', () => ({ error: jest.fn(), warn: jest.fn(), info: jest.fn(), debug: jest.fn() })); +jest.mock('../../src/utils/tokenRevocation', () => ({ isTokenRevoked: jest.fn(async () => fake.revoked) })); +jest.mock('../../src/utils/sessionCutoff', () => ({ isTokenBeforeCutoff: jest.fn(async () => fake.beforeCutoff) })); +jest.mock('../../src/utils/frontendUrl', () => ({ getFrontendBaseUrlSync: () => 'https://photos.example.com' })); + +const { maintenanceMiddleware, clearMaintenanceCache } = require('../../src/middleware/maintenance'); +const { isAuthenticated } = require('../../src/services/rateLimitService'); +const { verifyAdminPreview, isAdminPreview } = require('../../src/middleware/gallery'); +const { multipartOriginAllowed } = require('../../src/utils/requestOrigin'); + +const iat = Math.floor(Date.now() / 1000) - 10; +const adminToken = (extra = {}) => jwt.sign({ type: 'admin', id: 1, iat, ...extra }, process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); +const galleryToken = () => jwt.sign({ type: 'gallery', eventId: 1, iat }, process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); + +describe('maintenance gate is case-insensitive', () => { + async function run(path) { + clearMaintenanceCache(); + const req = { path, method: 'GET', headers: {} }; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn().mockReturnThis() }; + const next = jest.fn(); + await maintenanceMiddleware(req, res, next); + return next.mock.calls.length === 1; + } + it('gates /API/gallery/... exactly like /api/gallery/...', async () => { + expect(await run('/api/gallery/x/download-all')).toBe(false); + expect(await run('/API/gallery/x/download-all')).toBe(false); + expect(await run('/Og/gallery/x')).toBe(false); + }); +}); + +describe('general rate limiter skip', () => { + const req = (token) => ({ path: '/api/gallery/x/photos', headers: { authorization: `Bearer ${token}` }, cookies: {} }); + it('is granted to an admin session', () => { + expect(isAuthenticated(req(adminToken()))).toBe(true); + }); + it('is NOT granted to a gallery token', () => { + expect(isAuthenticated(req(galleryToken()))).toBe(false); + }); +}); + +describe('admin preview requires a live admin session', () => { + const req = (token) => ({ query: { admin_preview: '1' }, cookies: { admin_token: token }, headers: {} }); + beforeEach(() => { fake.revoked = false; fake.beforeCutoff = false; fake.admin = { id: 1, password_changed_at: null }; }); + + it('passes for a live session and sets req.isAdminPreview', async () => { + const r = req(adminToken()); + expect(isAdminPreview(r)).toBe(true); + expect(await verifyAdminPreview(r)).toBe(true); + expect(r.isAdminPreview).toBe(true); + }); + it('fails for a revoked token', async () => { + fake.revoked = true; + const r = req(adminToken()); + expect(await verifyAdminPreview(r)).toBe(false); + expect(r.isAdminPreview).toBeUndefined(); + }); + it('fails after the restore cutoff', async () => { + fake.beforeCutoff = true; + expect(await verifyAdminPreview(req(adminToken()))).toBe(false); + }); + it('fails for a deactivated or deleted admin', async () => { + fake.admin = null; + expect(await verifyAdminPreview(req(adminToken()))).toBe(false); + }); + it('fails for a token minted before the last password change', async () => { + fake.admin = { id: 1, password_changed_at: new Date((iat + 5) * 1000).toISOString() }; + expect(await verifyAdminPreview(req(adminToken()))).toBe(false); + }); +}); + +describe('multipart origin gate', () => { + const req = (headers) => ({ headers: { host: 'photos.example.com', ...headers } }); + it('accepts same-origin, same-site and non-browser requests', () => { + expect(multipartOriginAllowed(req({ 'sec-fetch-site': 'same-origin' }))).toBe(true); + expect(multipartOriginAllowed(req({ 'sec-fetch-site': 'same-site' }))).toBe(true); + expect(multipartOriginAllowed(req({ 'sec-fetch-site': 'none' }))).toBe(true); + expect(multipartOriginAllowed(req({}))).toBe(true); + expect(multipartOriginAllowed(req({ origin: 'https://photos.example.com' }))).toBe(true); + // Same-origin install without FRONTEND_URL: Origin matches the Host. + expect(multipartOriginAllowed({ headers: { host: 'gallery.local', origin: 'http://gallery.local' } })).toBe(true); + }); + it('rejects cross-site form posts', () => { + expect(multipartOriginAllowed(req({ 'sec-fetch-site': 'cross-site' }))).toBe(false); + expect(multipartOriginAllowed(req({ origin: 'https://evil.example' }))).toBe(false); + expect(multipartOriginAllowed(req({ origin: 'null' }))).toBe(false); + }); +}); diff --git a/backend/server.js b/backend/server.js index efeb35f2..4ceff39c 100644 --- a/backend/server.js +++ b/backend/server.js @@ -218,29 +218,16 @@ app.use((req, res, next) => { }); // CORS configuration (apply only to API routes) +const { isAllowedOrigin, multipartOriginAllowed } = require('./src/utils/requestOrigin'); + const corsOptions = { origin: function (origin, callback) { // getFrontendBaseUrlSync() resolves FRONTEND_URL, else the configured // general_site_url (#705) — without it, an install that leaves the // environment untouched and answers the setup wizard instead would have // its own public origin missing from the allowlist. - const allowedOrigins = [ - getFrontendBaseUrlSync() || 'http://localhost:3005', - process.env.ADMIN_URL || 'http://localhost:3005' - ]; - - // In development, also allow localhost origins - if (process.env.NODE_ENV === 'development') { - allowedOrigins.push( - 'http://localhost:5173', // Vite dev server - 'http://localhost:3002', // Backend server - 'http://localhost:3001', // For API testing - 'http://localhost:3000' // Direct backend access - ); - } - // Allow requests with no origin (like curl) and allow-listed origins - if (!origin || allowedOrigins.indexOf(origin) !== -1) { + if (!origin || isAllowedOrigin(origin)) { callback(null, true); } else { // Do not error globally; just omit CORS headers on disallowed origins @@ -527,8 +514,14 @@ app.use(createApiRateLimitGate(() => generalRateLimiter)); // and why it must stay unmounted. app.use(createAuthRateLimitGate(() => authRateLimiter)); -app.use(express.json({ limit: '50mb' })); -app.use(express.urlencoded({ extended: true, limit: '50mb' })); +// Body limits. 50mb is only needed by the authenticated admin and API-token +// surfaces (restore manifests, CMS and email templates, bulk operations); +// applied globally it let any unauthenticated caller hand JSON.parse a 50mb +// body and block the event loop. express.json skips a request whose body +// is already parsed, so the scoped parser must run first. +app.use(['/api/admin', '/api/v1'], express.json({ limit: '50mb' })); +app.use(express.json({ limit: '2mb' })); +app.use(express.urlencoded({ extended: true, limit: '2mb' })); // CSRF protection: require JSON Content-Type on mutating API requests // This blocks cross-origin form submissions which cannot set Content-Type: application/json @@ -540,6 +533,14 @@ app.use('/api', (req, res, next) => { if (contentLength > 0 && !contentType.includes('application/json') && !contentType.includes('multipart/form-data')) { return res.status(415).json({ error: 'Unsupported Content-Type. Use application/json or multipart/form-data.' }); } + // multipart is exactly what a cross-site
can send without a + // preflight, and in a split-origin deployment (SameSite=None) the admin + // cookie rides along to the upload routes. Browsers label such a + // submission Sec-Fetch-Site: cross-site (and always send Origin on a + // cross-origin POST); non-browser clients send neither header and pass. + if (contentType.includes('multipart/form-data') && !multipartOriginAllowed(req)) { + return res.status(403).json({ error: 'Cross-site multipart request rejected' }); + } } next(); }); diff --git a/backend/src/middleware/gallery.js b/backend/src/middleware/gallery.js index 14f9e7ce..32e40f56 100644 --- a/backend/src/middleware/gallery.js +++ b/backend/src/middleware/gallery.js @@ -3,6 +3,8 @@ const { db, withRetry } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); const { getGalleryTokenFromRequest } = require('../utils/tokenUtils'); const logger = require('../utils/logger'); +const { isTokenRevoked } = require('../utils/tokenRevocation'); +const { isTokenBeforeCutoff } = require('../utils/sessionCutoff'); /** * True when a logged-in admin is explicitly previewing this gallery (#868). @@ -24,8 +26,8 @@ const logger = require('../utils/logger'); * Fails closed on any verification error. Replaces the old `?preview=` * scheme, which leaked a 24h admin token into the address bar. */ -function isAdminPreview(req) { - if (req.query?.admin_preview !== '1') return false; +function decodeAdminPreview(req) { + if (req.query?.admin_preview !== '1') return null; // Cookie first, then a Bearer — but only an admin-typed token satisfies it. const candidates = []; if (req.cookies?.admin_token) candidates.push(req.cookies.admin_token); @@ -34,10 +36,46 @@ function isAdminPreview(req) { for (const token of candidates) { try { const decoded = jwt.verify(token, process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); - if (decoded.type === 'admin') return true; + if (decoded.type === 'admin') return decoded; } catch { /* try the next candidate */ } } - return false; + return null; +} + +function isAdminPreview(req) { + return decodeAdminPreview(req) !== null; +} + +/** + * The full session check behind the preview bypass. A verified signature is + * not a live session: adminAuth also rejects revoked tokens, tokens issued + * before the restore cutoff, deactivated admins and tokens minted before the + * admin's last password change. Without those a logged-out or deactivated + * admin token kept unlocking every draft and password gallery until `exp` + * (30 days with remember-me). Sets req.isAdminPreview on success so the + * downstream reveal-mode and logging checks read one verified flag. + */ +async function verifyAdminPreview(req) { + if (req.isAdminPreview === true) return true; + const decoded = decodeAdminPreview(req); + if (!decoded) return false; + try { + if (await isTokenRevoked(decoded) || await isTokenBeforeCutoff(decoded)) return false; + const admin = await withRetry(async () => db('admin_users') + .where({ id: decoded.id, is_active: formatBoolean(true) }) + .select('id', 'password_changed_at') + .first()); + if (!admin) return false; + if (admin.password_changed_at) { + const changedSeconds = Math.floor(new Date(admin.password_changed_at).getTime() / 1000); + if (decoded.iat < changedSeconds) return false; + } + } catch (err) { + logger.warn('Admin preview session check failed', { error: err.message }); + return false; + } + req.isAdminPreview = true; + return true; } // Middleware to verify gallery access @@ -51,7 +89,7 @@ async function verifyGalleryAccess(req, res, next) { // below. Per-request bypass — draft + password relaxed, NO gallery JWT // minted (a lingering guest cookie would muddy the coexisting-cookies case). // req.isAdminPreview flags downstream logging to keep it out of guest stats. - if (isAdminPreview(req)) { + if (await verifyAdminPreview(req)) { if (!requestedSlug) { return res.status(401).json({ error: 'No token provided' }); } @@ -247,5 +285,6 @@ function denySlideshowToken(req, res, next) { module.exports = { verifyGalleryAccess, denySlideshowToken, - isAdminPreview + isAdminPreview, + verifyAdminPreview }; diff --git a/backend/src/middleware/maintenance.js b/backend/src/middleware/maintenance.js index 9db25269..b2f83f4c 100644 --- a/backend/src/middleware/maintenance.js +++ b/backend/src/middleware/maintenance.js @@ -129,8 +129,12 @@ async function maintenanceMiddleware(req, res, next) { '/apple-touch-icon.png', '/apple-touch-icon-precomposed.png' ]; - const isBackendRendered = BACKEND_RENDERED_EXACT.includes(req.path) - || BACKEND_RENDERED_PREFIXES.some((prefix) => req.path.startsWith(prefix)); + // Express routes case-insensitively, so `/API/gallery/...` still reaches the + // API router; classify on the lowercased path or that spelling is treated + // as the SPA shell and walks straight past the gate. + const requestPath = String(req.path || '').toLowerCase(); + const isBackendRendered = BACKEND_RENDERED_EXACT.includes(requestPath) + || BACKEND_RENDERED_PREFIXES.some((prefix) => requestPath.startsWith(prefix)); const isSpaShell = req.method === 'GET' && !isBackendRendered; // Allow admin routes if admin is authenticated diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 1d647dee..3e27d595 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -25,7 +25,7 @@ function resolveHeroLogoVisible(perEvent, globalDefault) { } const watermarkService = require('../services/watermarkService'); const watermarkGeneratorService = require('../services/watermarkGeneratorService'); -const { verifyGalleryAccess, denySlideshowToken, isAdminPreview } = require('../middleware/gallery'); +const { verifyGalleryAccess, denySlideshowToken, verifyAdminPreview } = require('../middleware/gallery'); // Preserve the admin-preview flag across internal photo redirects (#981 review). // The redirected request carries no gallery JWT, so without the flag it would // fall back to the draft/password gate and 404 the derivative. @@ -335,7 +335,7 @@ router.get('/:slug/info', async (req, res) => { // Admin preview (#868) bypasses both the draft gate and — below — the // password gate. Computed once and reused. - const adminPreview = isAdminPreview(req); + const adminPreview = await verifyAdminPreview(req); // Check if event is a draft (allow admin preview) if (event.is_draft && !adminPreview) { return res.status(404).json({ error: 'Gallery is not yet published' }); diff --git a/backend/src/services/rateLimitService.js b/backend/src/services/rateLimitService.js index fca22c5d..4a3def69 100644 --- a/backend/src/services/rateLimitService.js +++ b/backend/src/services/rateLimitService.js @@ -115,8 +115,13 @@ function isAuthenticated(req) { return false; } - // Valid token found - check type - req.tokenType = decoded.type; // 'admin' or 'gallery' + // Only an admin session earns the skip. A gallery token is minted for + // free on password-less galleries and slideshow links, so treating it as + // "authenticated" handed anyone an unlimited budget on every /api route. + if (decoded.type !== 'admin') { + return false; + } + req.tokenType = decoded.type; req.tokenPayload = decoded; return true; diff --git a/backend/src/utils/requestOrigin.js b/backend/src/utils/requestOrigin.js new file mode 100644 index 00000000..ad70ff35 --- /dev/null +++ b/backend/src/utils/requestOrigin.js @@ -0,0 +1,42 @@ +/** + * Origin allow-listing shared by the CORS options and the multipart CSRF gate + * in server.js. Kept apart from server.js so it can be unit-tested without + * booting the app. + */ +const { getFrontendBaseUrlSync } = require('./frontendUrl'); + +function isAllowedOrigin(origin) { + const allowedOrigins = [ + getFrontendBaseUrlSync() || 'http://localhost:3005', + process.env.ADMIN_URL || 'http://localhost:3005' + ]; + if (process.env.NODE_ENV === 'development') { + allowedOrigins.push( + 'http://localhost:5173', // Vite dev server + 'http://localhost:3002', // Backend server + 'http://localhost:3001', // For API testing + 'http://localhost:3000' // Direct backend access + ); + } + return allowedOrigins.indexOf(origin) !== -1; +} + +// Origin check for multipart bodies (see the Content-Type gate below). +// Same-origin installs proxy /api through nginx and may not have FRONTEND_URL +// set, so an Origin matching the request Host is accepted alongside the CORS +// allowlist; Sec-Fetch-Site is authoritative when a browser sends it. +function multipartOriginAllowed(req) { + const site = req.headers['sec-fetch-site']; + if (site) return site !== 'cross-site'; + const origin = req.headers.origin; + if (!origin) return true; + if (isAllowedOrigin(origin)) return true; + try { + return new URL(origin).host === req.headers.host; + } catch { + return false; + } +} + + +module.exports = { isAllowedOrigin, multipartOriginAllowed }; diff --git a/backend/src/utils/revealMode.js b/backend/src/utils/revealMode.js index d10e074d..f01b492c 100644 --- a/backend/src/utils/revealMode.js +++ b/backend/src/utils/revealMode.js @@ -48,10 +48,11 @@ function isGalleryHidden(event, now = new Date()) { function bypassesReveal(req) { if (req.accessLevel === 'slideshow' || req.accessLevel === 'client') return true; if (req.viaCustomer) return true; - // Lazy require avoids a cycle: middleware/gallery requires nothing from - // here, but keeping the import local makes that permanent. - const { isAdminPreview } = require('../middleware/gallery'); - return Boolean(isAdminPreview(req)); + // req.isAdminPreview is set by verifyAdminPreview() only after the full + // session check (revocation, deactivation, password change). Re-decoding + // the token here would re-grant the bypass to a session that check just + // rejected. + return req.isAdminPreview === true; } /** Route guard result: is THIS request blocked by reveal mode? */