From 03087c798c8414505fcd694df7cd53bc08126b32 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:24:28 +0200 Subject: [PATCH] fix(security): close GHSA-g94x (cross-gallery photo read) + GHSA-pv6w (admin DB export) (#924) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): close two access-control advisories (GHSA-g94x, GHSA-pv6w) GHSA-g94x-8vv8-3c9f (HIGH) — the secure-image VIEW route (/secure-images/:slug/secure/:photoId/:token) validated only the token signature and took the gallery/photo from the URL, so a token minted on any PUBLIC gallery read every other gallery's photos with no password (its download sibling has verifyGalleryAccess; the view route can't — it serves via with no header). Bind the token to its scope instead: the URL photoId must equal the token's minted photoId (photos belong to exactly one gallery, and minting is gallery-scoped), and the gallery embedded in the token's sessionId must equal the URL gallery. GHSA-pv6w-rj34-wj9v (MEDIUM) — GET /admin/backup/picpeak/export dumps every table unredacted (bcrypt hashes, 2FA, SMTP/SSO/WhatsApp/webhook/S3 secrets) and was gated only by backup.create, which the built-in admin role holds. Gate it behind super_admin, matching the restore side (backup.restore, already admin-denied) and the masked config APIs. Regression tests pin both: cross-gallery token reads 403 (photo and gallery checks), backup export 403 for admin / passes for super_admin. * test: stub requireSuperAdmin in the backup masking mock adminBackup now calls requireSuperAdmin() at load (GHSA-pv6w export gate), and backupSecretMasking mocks the permissions module — add the new function to the mock so the module loads. * fix(security): review follow-ups on the export gate (GHSA-pv6w) - test: place the mocked export in its own mkdtemp dir. The route recursively deletes path.dirname(filePath) after download, so a stub in bare os.tmpdir() made the super_admin test wipe the whole temp root — other jest workers' DB files included (latent CI flake). - ui: hide PicpeakExportCard from non-super_admins. The role keeps settings.view + backup.create, so after the gate its Download button always 403'd with a generic toast; gate the card on role super_admin to match the endpoint. * fix(security): keep the token-mismatch audit values within varchar(20) (GHSA-g94x review) image_access_logs.access_type is varchar(20) (migration 038), but 'token_gallery_mismatch' is 22 chars — on Postgres the audit write threw value-too-long and logImageAccess swallowed it, so the security event went unrecorded (the 403 still fired; log is best-effort). Shorten to 'photo_mismatch' / 'gallery_mismatch' (14/16). --------- Co-authored-by: Paul Nothaft --- .../integration/backupSecretMasking.test.js | 1 + .../routes/backupExportSuperadmin.test.js | 95 ++++++++++++ .../routes/secureImageTokenBinding.test.js | 136 ++++++++++++++++++ backend/src/routes/adminBackup.js | 9 +- backend/src/routes/secureImages.js | 28 ++++ frontend/src/pages/admin/BackupManagement.tsx | 8 +- 6 files changed, 274 insertions(+), 3 deletions(-) create mode 100644 backend/__tests__/routes/backupExportSuperadmin.test.js create mode 100644 backend/__tests__/routes/secureImageTokenBinding.test.js diff --git a/backend/__tests__/integration/backupSecretMasking.test.js b/backend/__tests__/integration/backupSecretMasking.test.js index 1fdbed87..a78655df 100644 --- a/backend/__tests__/integration/backupSecretMasking.test.js +++ b/backend/__tests__/integration/backupSecretMasking.test.js @@ -22,6 +22,7 @@ jest.mock('../../src/middleware/auth', () => ({ })); jest.mock('../../src/middleware/permissions', () => ({ requirePermission: () => (_req, _res, next) => next(), + requireSuperAdmin: () => (_req, _res, next) => next(), })); describe('backup credential masking', () => { diff --git a/backend/__tests__/routes/backupExportSuperadmin.test.js b/backend/__tests__/routes/backupExportSuperadmin.test.js new file mode 100644 index 00000000..36aa6640 --- /dev/null +++ b/backend/__tests__/routes/backupExportSuperadmin.test.js @@ -0,0 +1,95 @@ +/** + * Full-instance export is super_admin only (GHSA-pv6w-rj34-wj9v). + * + * GET /api/admin/backup/picpeak/export dumps every table unredacted (bcrypt + * hashes, 2FA, SMTP/SSO/WhatsApp/webhook/S3 secrets). It was gated only by + * requirePermission('backup.create'), which the built-in `admin` role holds — + * so any non-super_admin admin could download the whole database. Pins that + * `admin` now gets 403 and `super_admin` passes the gate. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-bkexport-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'bkexport-test-secret'; + +// The export otherwise walks the whole DB and writes a zip — stub it so the +// super_admin happy path is fast and deterministic; the gate is what's tested. +// The route deletes path.dirname(filePath) recursively after download, so the +// stub MUST live in its own dir — a bare os.tmpdir() file would make the route +// wipe the whole temp root (and other jest workers' DB files). +const mockExportDir = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-export-stub-')); +const mockExportPath = path.join(mockExportDir, 'export.picpeak'); +fs.writeFileSync(mockExportPath, 'stub'); +jest.mock('../../src/services/picpeakExportService', () => ({ + createPicpeak: jest.fn(async () => ({ filePath: mockExportPath })), +})); + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('backup export super_admin gate (GHSA-pv6w)', () => { + let db; + let cleanup; + let app; + let adminToken; let superToken; + + const mkUser = async (username, roleName) => { + const role = await db('roles').where({ name: roleName }).first(); + const r = await db('admin_users').insert({ + username, + email: `${username}@example.com`, + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, + is_active: 1, + created_at: new Date(), + updated_at: new Date(), + }).returning('id'); + const id = r[0]?.id ?? r[0]; + return jwt.sign( + { id, username, type: 'admin', role: roleName, loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' }, + ); + }; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + adminToken = await mkUser('limited-admin', 'admin'); + superToken = await mkUser('root-admin', 'super_admin'); + + app = express(); + app.use(express.json()); + app.use('/api/admin/backup', require('../../src/routes/adminBackup')); + }, 120000); + + afterAll(async () => { + if (cleanup) await cleanup(); + fs.rmSync(mockExportDir, { recursive: true, force: true }); + }); + + it('denies the built-in admin role (was: full DB dump)', async () => { + const res = await request(app) + .get('/api/admin/backup/picpeak/export') + .set('Authorization', `Bearer ${adminToken}`); + expect(res.status).toBe(403); + }); + + it('allows super_admin', async () => { + const res = await request(app) + .get('/api/admin/backup/picpeak/export') + .set('Authorization', `Bearer ${superToken}`); + expect(res.status).not.toBe(403); + expect(res.status).toBeLessThan(500); + }); +}); diff --git a/backend/__tests__/routes/secureImageTokenBinding.test.js b/backend/__tests__/routes/secureImageTokenBinding.test.js new file mode 100644 index 00000000..d11b818d --- /dev/null +++ b/backend/__tests__/routes/secureImageTokenBinding.test.js @@ -0,0 +1,136 @@ +/** + * Secure-image view route token binding (GHSA-g94x-8vv8-3c9f). + * + * The view route GET /api/secure-images/:slug/secure/:photoId/:token serves + * via with the token in the URL, so it can't carry a gallery-token + * header like the download sibling. Before the fix it validated only the + * token signature and took the gallery/photo from the URL — so a token minted + * on any PUBLIC gallery read every other gallery's photos with no password. + * + * Pins that the route now enforces the scope inside the token: + * - the URL photoId must equal the token's minted photoId + * - the gallery embedded in the token's sessionId must equal the URL gallery + * A token minted on gallery A cannot read gallery B under either check; a + * token used on its own gallery+photo passes the binding. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-secimg-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'secimg-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-secimg-storage-')); + +// Stub the anti-bot/rate-limit middleware so the fingerprint is deterministic +// — the token below is minted with the same fingerprint, so verifySecureToken +// passes and the binding logic under test is what decides the outcome. +jest.mock('../../src/middleware/secureImageMiddleware', () => ({ + secureImageAccess: (req, _res, next) => { + req.clientInfo = { fingerprint: 'test-fp', ip: '127.0.0.1', userAgent: 'jest' }; + next(); + }, + getSecurityStatus: (_req, res) => res.json({ ok: true }), +})); + +const request = require('supertest'); +const express = require('express'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); +const secureImageService = require('../../src/services/secureImageService'); + +describe('secure-image view route token binding (GHSA-g94x)', () => { + let db; + let cleanup; + let app; + let galleryA; let galleryB; + let photoA; let photoB; + + const mkEvent = async (slug, requirePassword) => { + const r = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: slug, + event_date: '2026-08-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + require_password: requirePassword ? 1 : 0, + share_link: `/gallery/${slug}/share`, + share_token: `${slug}-share`, + expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + const mkPhoto = async (eventId, slug, filename) => { + const dir = path.join(process.env.STORAGE_PATH, 'events/active', slug); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, filename), Buffer.from('img')); + const r = await db('photos').insert({ + event_id: eventId, + filename, + path: `${slug}/${filename}`, + type: 'individual', + uploaded_at: new Date().toISOString(), + }).returning('id'); + return r[0]?.id ?? r[0]; + }; + + // Mint a token exactly as the mint route does — bound to (photoId, gallery + // sessionId, fingerprint) — bypassing the anti-bot HTTP path. + const mint = (photoId, eventId) => secureImageService.generateSecureToken( + photoId, + `gallery_public_${eventId}_${Date.now()}`, + { clientFingerprint: 'test-fp', maxUses: 100, expiresIn: 3600 }, + ); + + const view = (slug, photoId, token) => request(app) + .get(`/api/secure-images/${slug}/secure/${photoId}/${token}`); + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + galleryA = await mkEvent('secimg-public-a', false); // public — token source + galleryB = await mkEvent('secimg-private-b', true); // password-protected — victim + photoA = await mkPhoto(galleryA, 'secimg-public-a', 'a.jpg'); + photoB = await mkPhoto(galleryB, 'secimg-private-b', 'b.jpg'); + + app = express(); + app.use(express.json()); + app.use('/api/secure-images', require('../../src/routes/secureImages')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('rejects a gallery-A token used against gallery B (cross-photo)', async () => { + const token = mint(photoA, galleryA); + const res = await view('secimg-private-b', photoB, token); + expect(res.status).toBe(403); + expect(res.body.error).toMatch(/not valid for this photo/i); + }); + + it('rejects a gallery-A token replayed on gallery B with A\'s photoId', async () => { + const token = mint(photoA, galleryA); + // URL photoId matches the token, so the photo check passes — the gallery + // check (sessionId gallery A != URL gallery B) must catch it. + const res = await view('secimg-private-b', photoA, token); + expect(res.status).toBe(403); + expect(res.body.error).toMatch(/not valid for this gallery/i); + }); + + it('lets a token read its own gallery + photo (binding passes)', async () => { + const token = mint(photoA, galleryA); + const res = await view('secimg-public-a', photoA, token); + // Binding passes; serving may 200/404/500 depending on the pipeline, but + // it must NOT be rejected as a token mismatch. + expect(res.status).not.toBe(403); + }); +}); diff --git a/backend/src/routes/adminBackup.js b/backend/src/routes/adminBackup.js index 46673bc3..c60b2d31 100644 --- a/backend/src/routes/adminBackup.js +++ b/backend/src/routes/adminBackup.js @@ -1,7 +1,7 @@ const express = require('express'); const { db } = require('../database/db'); const { adminAuth } = require('../middleware/auth'); -const { requirePermission } = require('../middleware/permissions'); +const { requirePermission, requireSuperAdmin } = require('../middleware/permissions'); const { clearAdminAuthCookie } = require('../utils/tokenUtils'); const { revokeToken } = require('../utils/tokenRevocation'); const { triggerManualBackup, getBackupStatus, cleanupOldBackupRuns, getBackupManifest, validateBackupManifest } = require('../services/backupService'); @@ -150,7 +150,12 @@ router.post('/run', adminAuth, requirePermission('backup.create'), async (req, r // SECURITY: the file contains plaintext secrets (SMTP password, admin password // hashes, API keys). The download UI must warn before offering it. We surface // the flag as a response header too so the client can double-confirm. -router.get('/picpeak/export', adminAuth, requirePermission('backup.create'), async (req, res) => { +// Full-instance export dumps every table unredacted — bcrypt password +// hashes, 2FA columns, and all integration secrets (SMTP/SSO/WhatsApp/ +// webhook/S3) in cleartext. The built-in `admin` role holds backup.create, +// but is denied this data everywhere else (config APIs mask secrets as +// ********). Gate the raw dump behind super_admin (GHSA-pv6w-rj34-wj9v). +router.get('/picpeak/export', adminAuth, requireSuperAdmin(), async (req, res) => { const fsSync = require('fs'); try { const includePhotos = req.query.includePhotos === 'true' || req.query.includePhotos === '1'; diff --git a/backend/src/routes/secureImages.js b/backend/src/routes/secureImages.js index 2dda8c0a..91acb476 100644 --- a/backend/src/routes/secureImages.js +++ b/backend/src/routes/secureImages.js @@ -141,6 +141,34 @@ router.get('/:slug/secure/:photoId/:token', return res.status(404).json({ error: 'Gallery not found' }); } + // Bind the token to the gallery + photo it was minted for + // (GHSA-g94x-8vv8-3c9f). This route serves via with the + // token in the URL, so it can't require verifyGalleryAccess like the + // download sibling does. Instead enforce the scope already inside the + // token: it is minted for one photoId (and photos belong to exactly + // one gallery), and its sessionId records the minting gallery's id. + // Without this, a token minted on any PUBLIC gallery reads every other + // gallery's photos with no password. + const tokenPhotoId = Number(tokenValidation.data?.photoId); + if (!Number.isInteger(tokenPhotoId) || tokenPhotoId !== Number(photoId)) { + await secureImageService.logImageAccess( + photoId, event.id, req.clientInfo, 'photo_mismatch' + ); + return res.status(403).json({ error: 'Token not valid for this photo' }); + } + // Defense in depth: the sessionId embeds the gallery the token was + // minted for (`gallery_public__...` / `gallery__...`). Reject a + // token whose gallery is parseable and differs from this one. + const sessionEventId = Number( + (String(tokenValidation.data?.sessionId || '').match(/^gallery_(?:public_)?(\d+)_/) || [])[1] + ); + if (Number.isInteger(sessionEventId) && sessionEventId !== Number(event.id)) { + await secureImageService.logImageAccess( + photoId, event.id, req.clientInfo, 'gallery_mismatch' + ); + return res.status(403).json({ error: 'Token not valid for this gallery' }); + } + // Reveal mode (#838): tokens minted by plain guests die the moment the // gallery is (re-)hidden — bypass contexts keep working. if (isGalleryHidden(event) && !tokenValidation.data?.revealBypass) { diff --git a/frontend/src/pages/admin/BackupManagement.tsx b/frontend/src/pages/admin/BackupManagement.tsx index d94aa613..ab6ddc41 100644 --- a/frontend/src/pages/admin/BackupManagement.tsx +++ b/frontend/src/pages/admin/BackupManagement.tsx @@ -19,6 +19,7 @@ import { useTranslation } from 'react-i18next'; import { Button, Card, Loading } from '../../components/common'; import { useLocalizedDate } from '../../hooks/useLocalizedDate'; import { useMutationWithToast } from '../../hooks'; +import { useAdminAuth } from '../../contexts/AdminAuthContext'; import { BackupDashboard } from '../../components/admin/BackupDashboard'; import { BackupConfiguration } from '../../components/admin/BackupConfiguration'; import { BackupHistory } from '../../components/admin/BackupHistory'; @@ -33,6 +34,11 @@ type TabId = 'dashboard' | 'configuration' | 'history' | 'restore' | 'integrity' export const BackupManagement: React.FC = () => { const [activeTab, setActiveTab] = useState('dashboard'); const { t } = useTranslation(); + // Full-instance export contains every secret, so the endpoint is + // super_admin-only (GHSA-pv6w) — hide the card for other roles instead + // of showing a button that always 403s. + const { user } = useAdminAuth(); + const isSuperAdmin = user?.role?.name === 'super_admin'; const { formatDateTime: fmtDateTime } = useLocalizedDate(); const tabs = [ @@ -201,7 +207,7 @@ export const BackupManagement: React.FC = () => { onRunBackup={() => manualBackupMutation.mutate()} isBackupRunning={backupStatus?.isRunning || manualBackupMutation.isPending} /> - + {isSuperAdmin && } )}