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
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
+19
-18
@@ -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 <form> 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();
|
||||
});
|
||||
|
||||
@@ -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=<raw-JWT>`
|
||||
* 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
|
||||
};
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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' });
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 };
|
||||
@@ -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? */
|
||||
|
||||
Reference in New Issue
Block a user