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/__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/__tests__/routes/adminPhotoUploadSizeLimit.test.js b/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js index 212d1b7e..51699068 100644 --- a/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js +++ b/backend/__tests__/routes/adminPhotoUploadSizeLimit.test.js @@ -63,10 +63,10 @@ describe('admin upload per-file size limit (general_max_file_size_mb)', () => { .set('Authorization', `Bearer ${adminToken}`) .attach('photos', Buffer.alloc(bytes, 0x41), { filename, contentType: 'image/jpeg' }); - const postChunkedInit = (fileSize) => request(app) + const postChunkedInit = (fileSize, filename = 'clip.mp4') => request(app) .post(`/api/admin/photos/${eventId}/chunked-upload/init`) .set('Authorization', `Bearer ${adminToken}`) - .send({ filename: 'clip.mp4', fileSize, mimeType: 'video/mp4', totalChunks: 1 }); + .send({ filename, fileSize, mimeType: 'video/mp4', totalChunks: 1 }); beforeAll(async () => { ({ db, cleanup } = await bootCrmDb()); @@ -108,6 +108,20 @@ describe('admin upload per-file size limit (general_max_file_size_mb)', () => { uploadSettings = require('../../src/services/uploadSettings'); + // chunked-upload/init now enforces the admin allow-list on the filename + // extension (the declared mimeType is ignored), exactly like the + // multipart path; the default list is images only, so admit mp4 here. + await db('app_settings') + .insert({ + setting_key: 'general_allowed_file_types', + setting_value: JSON.stringify('jpg,jpeg,png,webp,mp4'), + setting_type: 'general', + updated_at: new Date().toISOString(), + }) + .onConflict('setting_key') + .merge({ setting_value: JSON.stringify('jpg,jpeg,png,webp,mp4') }); + uploadSettings.clearAllowedTypesCache(); + app = express(); app.use(express.json()); app.use('/api/admin/photos', require('../../src/routes/adminPhotos')); @@ -129,6 +143,13 @@ describe('admin upload per-file size limit (general_max_file_size_mb)', () => { expect(res.body.error).toBe('File too large. Maximum size is 1 MB per file.'); }); + it('rejects a chunked upload whose extension is not on the allow-list, whatever MIME it declares', async () => { + await setLimitMb(50); + const res = await postChunkedInit(1024, 'page.html'); + expect(res.status).toBe(400); + expect(res.body.error).toBe('File type not allowed'); + }); + it('rejects chunk bytes over the limit regardless of the declared fileSize', async () => { await setLimitMb(1); const initRes = await postChunkedInit(1); diff --git a/backend/__tests__/routes/passwordStrengthDos.test.js b/backend/__tests__/routes/passwordStrengthDos.test.js new file mode 100644 index 00000000..ab568497 --- /dev/null +++ b/backend/__tests__/routes/passwordStrengthDos.test.js @@ -0,0 +1,80 @@ +/** + * POST /api/auth/password-strength is unauthenticated and feeds its body into + * zxcvbn, whose matching is superlinear and runs synchronously on the event + * loop. Behind express.json({ limit: '50mb' }) that made a single request a + * whole-process denial of service: measured on this codebase, 1,000 characters + * blocked for ~5 seconds and 5,000 did not return in two minutes. + * + * The control is the length cap inside validatePassword(), so it holds for + * every caller. These tests pin the cap itself rather than the route, and use + * a wall-clock ceiling that only an unbounded zxcvbn call can breach. + */ +const { validatePassword, MAX_PASSWORD_LENGTH } = require('../../src/utils/passwordValidation'); + +describe('password validation length cap (zxcvbn DoS)', () => { + it('rejects an over-length password without doing superlinear work', () => { + const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); // 4x the cap + const started = Date.now(); + const result = validatePassword(huge); + const elapsed = Date.now() - started; + + expect(result.valid).toBe(false); + expect(result.errors.join(' ')).toMatch(/at most 128 characters/); + // Unbounded, this input would not return for minutes. + expect(elapsed).toBeLessThan(250); + }); + + it('is bounded at the cap itself, the worst input it will still analyse', () => { + const atCap = 'aA1!'.repeat(MAX_PASSWORD_LENGTH / 4); + expect(atCap).toHaveLength(MAX_PASSWORD_LENGTH); + + // 128 was chosen so the worst input the validator will still analyse costs + // about as much as an ordinary request (~41ms measured); 512 cost 1.4s. + const started = Date.now(); + validatePassword(atCap); + expect(Date.now() - started).toBeLessThan(1000); + }); + + it('still accepts an ordinary strong password', () => { + const result = validatePassword('Tr0ub4dour&3-horse-battery'); + expect(result.valid).toBe(true); + }); + + it('does not spin when a caller asks for a length the cap forbids', async () => { + // Codex review. generateSecurePassword retried by recursing on any invalid + // candidate, so the new cap made every candidate invalid for length > 128 + // and turned the call into unbounded recursion. It now refuses up front, + // and the retry loop is bounded. + const { generateSecurePassword } = require('../../src/utils/passwordValidation'); + + expect(generateSecurePassword({ length: 16 })).toHaveLength(16); + expect(generateSecurePassword({ length: MAX_PASSWORD_LENGTH })) + .toHaveLength(MAX_PASSWORD_LENGTH); + expect(() => generateSecurePassword({ length: MAX_PASSWORD_LENGTH + 1 })) + .toThrow(/at most 128/); + }); + + it('does not echo the rejected password back in the error body', async () => { + // Codex review round 2. express-validator's errors.array() carries the + // submitted `value`, so the 400 for an oversized password returned the + // password itself -- reflecting a credential, and re-allocating up to the + // 50mb body limit on an unauthenticated endpoint, which partly undid the + // DoS fix this branch exists for. + const src = require('fs').readFileSync( + require('path').join(__dirname, '../../src/routes/auth.js'), 'utf8'); + + // No route may hand errors.array() straight to the response. + expect(src).not.toMatch(/errors:\s*errors\.array\(\)/); + // ...and the shared helper that replaces it must drop `value`. + const helper = require('fs').readFileSync( + require('path').join(__dirname, '../../src/utils/routeHelpers.js'), 'utf8'); + expect(helper).toMatch(/safeValidationErrors\s*=\s*\(errors\)\s*=>\s*errors\.array\(\)\.map\(\(\{ value, \.\.\.rest \}\)/); + }); + + it('applies the cap through the context wrapper too', async () => { + const { validatePasswordInContext } = require('../../src/utils/passwordValidation'); + const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); + const result = await validatePasswordInContext(huge, 'admin', {}); + expect(result.valid).toBe(false); + }); +}); diff --git a/backend/__tests__/utils/photoContentType.test.js b/backend/__tests__/utils/photoContentType.test.js new file mode 100644 index 00000000..406cd196 --- /dev/null +++ b/backend/__tests__/utils/photoContentType.test.js @@ -0,0 +1,47 @@ +/** + * photos.mime_type is client-influenced (chunked uploads stored the declared + * type verbatim; the S3 importer stores whatever mime-types derives). Every + * serving route must go through resolvePhotoContentType so the header is + * always image/* or video/* and never the stored value as given. + */ +const fs = require('fs'); +const path = require('path'); +const { resolvePhotoContentType } = require('../../src/utils/photoContentType'); + +describe('resolvePhotoContentType', () => { + it('never echoes a non-media stored MIME', () => { + expect(resolvePhotoContentType({ filename: 'a.jpg', mime_type: 'text/html' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'text/html' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a.gif', mime_type: 'application/javascript' })).toBe('image/gif'); + }); + + it('never honours the scriptable svg / xml family or header-invalid values', () => { + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'image/svg+xml' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'image/x\r\nX-Injected: 1' })).toBe('image/jpeg'); + expect(resolvePhotoContentType({ filename: 'a.mp4', mime_type: 'video/mp4\r\nX: y' })).toBe('video/mp4'); + }); + + it('prefers the mapped extension for images and the stored type for videos', () => { + expect(resolvePhotoContentType({ filename: 'a.png', mime_type: 'image/jpeg' })).toBe('image/png'); + expect(resolvePhotoContentType({ filename: 'a.mov', mime_type: null })).toBe('video/quicktime'); + expect(resolvePhotoContentType({ filename: 'a.bin', media_type: 'video' })).toBe('video/mp4'); + expect(resolvePhotoContentType({ filename: 'a', mime_type: 'image/avif' })).toBe('image/avif'); + expect(resolvePhotoContentType({ filename: 'a.constructor', mime_type: null })).toBe('image/jpeg'); + }); +}); + +describe('serving routes use the resolver', () => { + const routes = ['gallery.js', 'secureImages.js', 'protectedImages.js', 'adminPhotos.js']; + it.each(routes)('%s sets no Content-Type from photo.mime_type directly', (name) => { + const src = fs.readFileSync(path.join(__dirname, '../../src/routes', name), 'utf8'); + expect(src).not.toMatch(/'Content-Type':\s*photo\.mime_type/); + expect(src).not.toMatch(/set\('Content-Type',\s*photo\.mime_type\)/); + expect(src).toMatch(/resolvePhotoContentType\(photo\)/); + }); + + it('chunked-upload init derives the MIME from the allow-listed extension', () => { + const src = fs.readFileSync(path.join(__dirname, '../../src/routes/adminPhotos.js'), 'utf8'); + expect(src).not.toMatch(/const \{ filename, fileSize, mimeType, totalChunks \} = req\.body/); + expect(src).toMatch(/allowedMimeTypes\.includes\(mimeType\)/); + }); +}); diff --git a/backend/__tests__/utils/safePathUploadedAssets.test.js b/backend/__tests__/utils/safePathUploadedAssets.test.js new file mode 100644 index 00000000..07ae5d6a --- /dev/null +++ b/backend/__tests__/utils/safePathUploadedAssets.test.js @@ -0,0 +1,59 @@ +/** + * Containment for the two admin-writable "delete the old file" paths. + * + * Settings → Branding persists logo_url / favicon_url verbatim and, on + * clear, unlinked `path.join(storage, url)` after a mere prefix check. + * Business profile did the same for logo_path behind a `/pdf-logo-\d+\./` + * marker. Both let an admin delete any file the process can reach. The + * helpers below only ever name a flat leaf inside the fixed directory. + */ +const path = require('path'); +const { uploadedAssetPath, uploadedPdfLogoPath } = require('../../src/utils/safePath'); + +const root = '/srv/picpeak/storage'; + +describe('uploadedAssetPath', () => { + it('resolves a flat leaf inside the named upload directory', () => { + expect(uploadedAssetPath('/uploads/logos/logo-1.png', 'logos', root)) + .toBe(path.join(root, 'uploads', 'logos', 'logo-1.png')); + expect(uploadedAssetPath('/uploads/favicons/fav.ico', 'favicons', root)) + .toBe(path.join(root, 'uploads', 'favicons', 'fav.ico')); + }); + + it.each([ + '/uploads/logos/../../../data/picpeak.db', + '/uploads/logos/..', + '/uploads/logos/', + '/uploads/logos/sub/dir.png', + '/uploads/favicons/x.ico', // wrong kind + 'uploads/logos/logo.png', // not /-rooted + 'https://example.com/uploads/logos/logo.png', + '', + null, + 42, + ])('refuses %p', (value) => { + expect(uploadedAssetPath(value, 'logos', root)).toBeNull(); + }); +}); + +describe('uploadedPdfLogoPath', () => { + it('resolves the file the upload route writes', () => { + expect(uploadedPdfLogoPath('/uploads/logos/pdf-logo-1700000000000.png', root)) + .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1700000000000.png')); + expect(uploadedPdfLogoPath('uploads/logos/pdf-logo-1.svg', root)) + .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1.svg')); + }); + + it.each([ + 'pdf-logo-1./../../../../etc/target', + '/uploads/logos/pdf-logo-1./../../secret', + '/etc/pdf-logo-1.x', + '/uploads/logos/pdf-logo-1.png/../other', + '/uploads/logos/other-logo.png', + '/uploads/contracts/signed/pdf-logo-1.pdf', + '', + null, + ])('refuses %p', (value) => { + expect(uploadedPdfLogoPath(value, root)).toBeNull(); + }); +}); diff --git a/backend/__tests__/utils/tokenRevocation.forgery.test.js b/backend/__tests__/utils/tokenRevocation.forgery.test.js new file mode 100644 index 00000000..a5b54584 --- /dev/null +++ b/backend/__tests__/utils/tokenRevocation.forgery.test.js @@ -0,0 +1,69 @@ +/** + * revokeToken() is reachable from the unauthenticated logout endpoints + * (POST /api/auth/logout, /gallery/logout, /customer-auth/logout). It used + * to base64-decode the payload without checking the signature and insert a + * row keyed on `${id}-${iat}-${type}` -- the same key isTokenRevoked() + * matches for real sessions. Anyone could therefore forge a payload naming + * another user's id, type and login second and log them out remotely, and + * with a far-future `exp` the row was never swept. + * + * The contract pinned here: only a token whose signature verifies under + * JWT_SECRET is written to revoked_tokens. Expired-but-genuine tokens are + * still accepted (logout must stay idempotent). + */ +const jwt = require('jsonwebtoken'); + +process.env.JWT_SECRET = 'revocation-forgery-test-secret'; + +const inserted = []; +jest.mock('../../src/database/db', () => { + const dbFn = () => ({ + insert(row) { + inserted.push(row); + return { onConflict: () => ({ ignore: async () => undefined }) }; + }, + }); + return { db: dbFn }; +}); +jest.mock('../../src/utils/logger', () => ({ + info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn(), +})); + +const { revokeToken } = require('../../src/utils/tokenRevocation'); + +const iat = Math.floor(Date.now() / 1000) - 60; + +describe('revokeToken signature check', () => { + beforeEach(() => { inserted.length = 0; }); + + it('refuses a forged three-part token and writes nothing', async () => { + const forgedPayload = Buffer.from(JSON.stringify({ + id: 1, iat, type: 'admin', exp: 9e9, + })).toString('base64'); + const forged = `eyJhbGciOiJIUzI1NiJ9.${forgedPayload}.notasignature`; + + const result = await revokeToken(forged, 'user_logout'); + + expect(result).toBe(false); + expect(inserted).toHaveLength(0); + }); + + it('refuses a token signed with a different secret', async () => { + const other = jwt.sign({ id: 1, iat, type: 'admin' }, 'some-other-secret', { expiresIn: '1h' }); + expect(await revokeToken(other, 'user_logout')).toBe(false); + expect(inserted).toHaveLength(0); + }); + + it('revokes a genuine token', async () => { + const genuine = jwt.sign({ id: 1, iat, type: 'admin' }, process.env.JWT_SECRET, { expiresIn: '1h' }); + expect(await revokeToken(genuine, 'user_logout')).toBe(true); + expect(inserted).toHaveLength(1); + expect(inserted[0].token_id).toBe(`1-${iat}-admin`); + }); + + it('still revokes a genuine token that has already expired', async () => { + const expired = jwt.sign({ id: 1, iat, type: 'admin', exp: iat + 1 }, process.env.JWT_SECRET); + expect(await revokeToken(expired, 'user_logout')).toBe(true); + expect(inserted).toHaveLength(1); + }); +}); diff --git a/backend/package-lock.json b/backend/package-lock.json index 99c734de..a495233a 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -1,12 +1,12 @@ { "name": "picpeak-backend", - "version": "3.107.1-beta.0", + "version": "3.122.5-beta.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "picpeak-backend", - "version": "3.107.1-beta.0", + "version": "3.122.5-beta.0", "dependencies": { "@aws-sdk/client-s3": "^3.850.0", "@aws-sdk/lib-storage": "^3.850.0", @@ -10594,12 +10594,13 @@ } }, "node_modules/qs": { - "version": "6.15.2", - "resolved": "https://registry.npmjs.org/qs/-/qs-6.15.2.tgz", - "integrity": "sha512-Rzq0KEyX/w/tEybncDgdkZrJgVUsUMk3xjh3t5bv3S1HTAtg+uOYt72+ZfwiQwKdysThkTBdL/rTi6HDmX9Ddw==", + "version": "6.16.0", + "resolved": "https://registry.npmjs.org/qs/-/qs-6.16.0.tgz", + "integrity": "sha512-h6fhOIaRrID2CbEY2fqs+7t+UXZo+MLAnU5gRIq85uFtdiUPCdsApMlHhXogKVM4HM2DVbIjGNTTYH2OcmP1vA==", "license": "BSD-3-Clause", "dependencies": { - "side-channel": "^1.1.0" + "es-define-property": "^1.0.1", + "side-channel": "^1.1.1" }, "engines": { "node": ">=0.6" @@ -11163,14 +11164,14 @@ } }, "node_modules/side-channel": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.0.tgz", - "integrity": "sha512-ZX99e6tRweoUXqR+VBrslhda51Nh5MTQwou5tnUDgbtyM0dBgmhEDtWGP/xbKn6hqfPRHujUNwz5fy/wbbhnpw==", + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.1.tgz", + "integrity": "sha512-6x6dK6zJdpTzF4sQeNYxwtvBzf6Eg4GtlesS94HOvTudUeyK2WXAaIfmDgsyslYrRBeFIlsi54AYsFGUuhmvrQ==", "license": "MIT", "dependencies": { "es-errors": "^1.3.0", - "object-inspect": "^1.13.3", - "side-channel-list": "^1.0.0", + "object-inspect": "^1.13.4", + "side-channel-list": "^1.0.1", "side-channel-map": "^1.0.1", "side-channel-weakmap": "^1.0.2" }, @@ -11182,13 +11183,13 @@ } }, "node_modules/side-channel-list": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/side-channel-list/-/side-channel-list-1.0.0.tgz", - "integrity": "sha512-FCLHtRD/gnpCiCHEiJLOwdmFP+wzCmDEkc9y7NsYxeF4u7Btsn1ZuwgwJGxImImHicJArLP4R0yX4c2KCrMrTA==", + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/side-channel-list/-/side-channel-list-1.0.1.tgz", + "integrity": "sha512-mjn/0bi/oUURjc5Xl7IaWi/OJJJumuoJFQJfDDyO46+hBWsfaVM65TBHq2eoZBhzl9EchxOijpkbRC8SVBQU0w==", "license": "MIT", "dependencies": { "es-errors": "^1.3.0", - "object-inspect": "^1.13.3" + "object-inspect": "^1.13.4" }, "engines": { "node": ">= 0.4" diff --git a/backend/server.js b/backend/server.js index e78da6f8..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(); }); @@ -596,14 +597,35 @@ const secureStatic = require('./src/middleware/secureStatic'); const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../storage'); process.env.EXTERNAL_MEDIA_ROOT = process.env.EXTERNAL_MEDIA_ROOT || '/external-media'; -// Static file serving for photos (protected) -app.use('/photos', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'events/active'))); +// The /photos and /thumbnails static mounts are gone. +// +// They served the raw originals tree and the thumbnail tree behind photoAuth +// alone, which authorises on a slug match. A static file server cannot apply +// the rules the gallery API applies per photo, so everything the API decides +// was simply absent here: allow_downloads, per-category allow_downloads, +// watermarking, the resolution cap, reveal-mode windows, visibility='hidden', +// download logging, and the customer-assignment re-check that lets an admin +// revoke access immediately. The filenames needed to exercise it are handed to +// every guest in the photos listing. +// +// Nothing builds these URLs: no reference in frontend/src, none in the email +// templates, and the only backend mentions are the /api/admin/photos/... API +// routes and a maintenance-mode prefix list. nginx still proxies /photos and +// /thumbnails; those locations now 404, which is the intended outcome. +// +// Serving these safely would mean reimplementing per-photo authorisation and +// image processing inside a static handler -- i.e. the gallery API, which +// already exists at /api/gallery/:slug/photo/:id and /thumbnail/:id. -// Static file serving for thumbnails (protected) -app.use('/thumbnails', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'thumbnails'))); - -// Static file serving for uploads (public - logos, favicons) -app.use('/uploads', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads'))); +// Static file serving for uploads. +// +// Narrowed to the two public asset trees. The mount used to expose the whole +// uploads/ root with no auth middleware at all, and that root also holds +// signed contract PDFs (uploads/contracts/signed) and client transfer files +// (uploads/transfers/) -- both reachable by anyone who learned or guessed +// a filename. Those are served by their own authorised routes. +app.use('/uploads/logos', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads/logos'))); +app.use('/uploads/favicons', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads/favicons'))); // Static file serving for self-hosted webfonts (public — gallery visitors // load these via @font-face). Replaces the previous Google Fonts CDN @@ -770,10 +792,15 @@ app.get( // whereas Firefox/Chrome do — so a 302 worked everywhere except // Safari. sendFile sets the right content-type from the extension. const rel = String(url).replace(/^\/+/, '').replace(/^uploads\//, ''); + // Containment is the two public asset trees, not the whole uploads/ + // root: that root also holds signed contracts and client transfer + // files, and the favicon URL is an admin-writable setting, so the + // wider check let `/uploads/contracts/signed/` be served here + // unauthenticated with a day of cache. const uploadsRoot = path.resolve(path.join(storagePath, 'uploads')); const resolved = path.resolve(path.join(uploadsRoot, rel)); - // Path containment — never serve outside the uploads dir. - if (resolved.startsWith(uploadsRoot + path.sep) && fs.existsSync(resolved)) { + const servableRoots = ['favicons', 'logos'].map((d) => path.join(uploadsRoot, d) + path.sep); + if (servableRoots.some((root) => resolved.startsWith(root)) && fs.existsSync(resolved)) { // This route streams the file directly, bypassing the secureStatic // middleware — so re-apply its SVG hardening here. An admin-uploaded // SVG favicon could contain