From 1cf82d81a7935ca106b36521cbf02f63cd2e14b6 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 16 Jul 2026 10:55:10 +0200 Subject: [PATCH 1/5] fix(security): preserve current admin on .picpeak restore (GHSA-qxfx-4493-4v8f) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit adminAuth populates req.admin, not req.user, so currentAdminId was always undefined in the /api/admin/picpeak/import handler. reinjectCurrentAdmin() then had no account to preserve and the admin_users table was fully replaced by the uploaded backup — a crafted .picpeak let any admin with backup.restore take over every admin account (critical). One-line fix: pass req.admin.id. Closes GHSA-qxfx-4493-4v8f and its duplicate GHSA-pjp6-jcrj-3cr5. --- backend/src/routes/adminBackup.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/backend/src/routes/adminBackup.js b/backend/src/routes/adminBackup.js index 67d1479c..b1ee0e97 100644 --- a/backend/src/routes/adminBackup.js +++ b/backend/src/routes/adminBackup.js @@ -178,7 +178,11 @@ router.post('/picpeak/import', adminAuth, requirePermission('backup.restore'), p const picpeakPath = req.file.path; try { const { importFromPicpeak } = require('../services/picpeakImportService'); - const result = await importFromPicpeak({ picpeakPath, currentAdminId: req.user && req.user.id }); + // adminAuth populates req.admin, not req.user. Passing req.user.id here + // left currentAdminId undefined, so reinjectCurrentAdmin() had no account + // to preserve and the admin_users table was fully replaced by the backup — + // letting a crafted .picpeak take over every admin account (GHSA-qxfx-4493-4v8f). + const result = await importFromPicpeak({ picpeakPath, currentAdminId: req.admin && req.admin.id }); res.json({ success: true, tables: result.tables, From 28f69e4bf3b1d99748d53eb2671617ab06e4fedb Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 16 Jul 2026 10:55:10 +0200 Subject: [PATCH 2/5] fix(security): share-login must not bypass gallery password (GHSA-9hmx-68vc-qpqw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /auth/gallery/share-login validated only the 128-bit share token and then minted a full type:'gallery' access token regardless of require_password — computing requiresPassword at the end only to echo it, never enforce it. Anyone holding a gallery's share link could read and download every photo in a password-protected gallery via a direct API call, no password needed. Fix: compute requiresPassword before minting; for a password-protected gallery return { requires_password: true } with NO token and NO cookie. The client then goes through /gallery/verify, which does bcrypt.compare the password. The public (no-password) auto-login path is unchanged. The frontend already falls through to the password prompt when share-login returns no token/event. Adds route regression test covering the bypass, the public path, and bad tokens. --- .../routes/authShareLoginPassword.test.js | 127 ++++++++++++++++++ backend/src/routes/auth.js | 14 +- 2 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 backend/__tests__/routes/authShareLoginPassword.test.js diff --git a/backend/__tests__/routes/authShareLoginPassword.test.js b/backend/__tests__/routes/authShareLoginPassword.test.js new file mode 100644 index 00000000..aadece62 --- /dev/null +++ b/backend/__tests__/routes/authShareLoginPassword.test.js @@ -0,0 +1,127 @@ +/** + * Regression test for GHSA-9hmx-68vc-qpqw — share-link login must not bypass + * the gallery password. + * + * POST /auth/gallery/share-login validates only the share token. For a + * password-protected gallery it previously minted a full `type:'gallery'` + * access token on the share token alone, letting anyone holding the share URL + * read the gallery without the password. The fix: when the gallery requires a + * password, return `{ requires_password: true }` with NO token and NO cookie. + */ + +const express = require('express'); +const request = require('supertest'); + +process.env.JWT_SECRET = 'share-login-test-secret'; + +const events = []; + +jest.mock('../../src/database/db', () => { + function dbFn(table) { + if (table === 'events') { + let filter = () => true; + return { + where(criteria) { + filter = (row) => Object.entries(criteria).every(([k, v]) => { + if (k === 'is_active') return Boolean(row.is_active) === Boolean(v); + if (k === 'is_archived') return Boolean(row.is_archived) === Boolean(v); + return row[k] === v; + }); + return this; + }, + async first() { return events.find(filter); }, + }; + } + return { where() { return this; }, async first() { return undefined; } }; + } + dbFn.raw = async () => {}; + return { db: dbFn, logActivity: async () => {} }; +}); + +// Share token is stored plainly on the fake event row. +jest.mock('../../src/services/shareLinkService', () => ({ + getEventShareToken: (event) => event.share_token, + resolveShareIdentifier: async () => ({ event: null }), +})); + +const mockSetGalleryAuthCookies = jest.fn(); +jest.mock('../../src/utils/tokenUtils', () => ({ + setGalleryAuthCookies: (...args) => mockSetGalleryAuthCookies(...args), + clearGalleryAuthCookies: jest.fn(), + getGalleryTokenFromRequest: jest.fn(), + setAdminAuthCookies: jest.fn(), +})); + +jest.mock('../../src/utils/authSecurity', () => ({ + trackFailedAttempt: jest.fn(async () => {}), + trackSuccessfulLogin: jest.fn(async () => {}), + checkAccountLockout: jest.fn(async () => ({ isLocked: false })), + resetLockout: jest.fn(async () => {}), +})); + +// Collaborators the router imports at load but the share-login path doesn't hit. +jest.mock('../../src/services/recaptcha', () => ({ verifyRecaptcha: async () => true })); +jest.mock('../../src/services/mfaService', () => ({})); +jest.mock('../../src/middleware/sessionTimeout', () => ({ endSession: jest.fn(), sessionTimeoutMiddleware: (req, res, next) => next() })); +jest.mock('../../src/utils/tokenRevocation', () => ({ revokeToken: jest.fn(async () => {}), isTokenRevoked: async () => false })); + +const authRouter = require('../../src/routes/auth'); + +function makeApp() { + const app = express(); + app.use(express.json()); + app.use('/auth', authRouter); + return app; +} + +const SHARE_TOKEN = 'a'.repeat(64); + +beforeEach(() => { + events.length = 0; + mockSetGalleryAuthCookies.mockClear(); +}); + +describe('POST /auth/gallery/share-login password enforcement', () => { + it('does NOT mint a token for a password-protected gallery', async () => { + events.push({ + id: 1, slug: 'private-gallery', is_active: 1, is_archived: 0, + require_password: 1, share_token: SHARE_TOKEN, event_name: 'Private', + }); + const res = await request(makeApp()) + .post('/auth/gallery/share-login') + .send({ slug: 'private-gallery', token: SHARE_TOKEN }); + + expect(res.status).toBe(200); + expect(res.body.requires_password).toBe(true); + expect(res.body.token).toBeUndefined(); + expect(mockSetGalleryAuthCookies).not.toHaveBeenCalled(); + }); + + it('mints a token for a public (no-password) gallery', async () => { + events.push({ + id: 2, slug: 'public-gallery', is_active: 1, is_archived: 0, + require_password: false, share_token: SHARE_TOKEN, event_name: 'Public', + }); + const res = await request(makeApp()) + .post('/auth/gallery/share-login') + .send({ slug: 'public-gallery', token: SHARE_TOKEN }); + + expect(res.status).toBe(200); + expect(typeof res.body.token).toBe('string'); + expect(res.body.event).toBeDefined(); + expect(mockSetGalleryAuthCookies).toHaveBeenCalledTimes(1); + }); + + it('rejects a wrong share token regardless of password setting', async () => { + events.push({ + id: 3, slug: 'public-gallery', is_active: 1, is_archived: 0, + require_password: false, share_token: SHARE_TOKEN, event_name: 'Public', + }); + const res = await request(makeApp()) + .post('/auth/gallery/share-login') + .send({ slug: 'public-gallery', token: 'b'.repeat(64) }); + + expect(res.status).toBe(401); + expect(mockSetGalleryAuthCookies).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index 8915380f..13a6acf0 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -543,6 +543,18 @@ router.post('/gallery/share-login', [ return res.status(401).json({ error: 'Invalid or expired share link' }); } + const requiresPassword = !(event.require_password === false || event.require_password === 0 || event.require_password === '0'); + + // The share link only proves the holder was given the link — it is NOT the + // gallery password. For a password-protected gallery, minting a full + // `type:'gallery'` token here would let anyone with the share URL bypass + // the password entirely (GHSA-9hmx-68vc-qpqw). Signal that a password is + // still required and return WITHOUT a token/cookie; the client then goes + // through POST /gallery/verify, which does check the password. + if (requiresPassword) { + return res.json({ requires_password: true }); + } + const jwtToken = jwt.sign({ eventId: event.id, eventSlug: event.slug, @@ -557,8 +569,6 @@ router.post('/gallery/share-login', [ await trackSuccessfulLogin(`gallery:${event.slug}:share`, ipAddress, userAgent); setGalleryAuthCookies(res, jwtToken, event.slug); - const requiresPassword = !(event.require_password === false || event.require_password === 0 || event.require_password === '0'); - res.json({ token: jwtToken, event: { From cde0b465a90169348475c0415d0d96df1cd5cc44 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 16 Jul 2026 10:55:10 +0200 Subject: [PATCH 3/5] fix(security): reject ZIP-slip entries in archive/backup restore (GHSA-jfhw-fj23-fx6x) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node-stream-zip's extract(null, root) writes each entry to path.join(root, entry.name) without neutralising '../', so a crafted archive entry named '../../uploads/logos/evil.svg' escaped the target dir and overwrote arbitrary files (logos, .env, route files → RCE on source deploys). Requires admin with archives.restore. Adds assertZipEntriesWithin() to utils/safePath.js — a lexical containment check run on the entry list BEFORE extract() — and guards both extract sinks: adminArchives.js (the reported route) and picpeakImportService.js (the sibling .picpeak import, same sink). Adds unit tests for traversal, absolute-path, and sibling-prefix entries. --- .../utils/safePathZipEntries.test.js | 41 +++++++++++++++++++ backend/src/routes/adminArchives.js | 11 +++++ backend/src/services/picpeakImportService.js | 5 +++ backend/src/utils/safePath.js | 34 +++++++++++++++ 4 files changed, 91 insertions(+) create mode 100644 backend/__tests__/utils/safePathZipEntries.test.js diff --git a/backend/__tests__/utils/safePathZipEntries.test.js b/backend/__tests__/utils/safePathZipEntries.test.js new file mode 100644 index 00000000..97d00acc --- /dev/null +++ b/backend/__tests__/utils/safePathZipEntries.test.js @@ -0,0 +1,41 @@ +const path = require('path'); +const { assertZipEntriesWithin } = require('../../src/utils/safePath'); + +describe('assertZipEntriesWithin (ZIP-slip guard, GHSA-jfhw-fj23-fx6x)', () => { + const root = path.join('/tmp', 'picpeak-extract-root'); + + it('accepts entries that stay within the extraction root', () => { + const entries = [ + { name: 'photo.jpg' }, + { name: 'category/nested/photo.png' }, + { name: 'photos_manifest.json' }, + { name: 'subdir/' }, + ]; + expect(() => assertZipEntriesWithin(entries, root)).not.toThrow(); + }); + + it('rejects a parent-traversal entry', () => { + const entries = [{ name: '../../uploads/logos/evil.svg' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); + + it('rejects an absolute-path entry', () => { + const entries = [{ name: '/etc/cron.d/evil' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); + + it('rejects when a safe entry is mixed with a traversal entry', () => { + const entries = [{ name: 'ok.jpg' }, { name: '../escape.txt' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); + + it('tolerates empty / nameless entries', () => { + expect(() => assertZipEntriesWithin([{}, { name: '' }, null], root)).not.toThrow(); + }); + + it('does not treat a sibling prefix directory as inside the root', () => { + // root is .../picpeak-extract-root; ../picpeak-extract-root-evil must not pass + const entries = [{ name: '../picpeak-extract-root-evil/x' }]; + expect(() => assertZipEntriesWithin(entries, root)).toThrow(/escapes the extraction directory/); + }); +}); diff --git a/backend/src/routes/adminArchives.js b/backend/src/routes/adminArchives.js index d81c8713..fcebccfa 100644 --- a/backend/src/routes/adminArchives.js +++ b/backend/src/routes/adminArchives.js @@ -9,6 +9,7 @@ const { requirePermission } = require('../middleware/permissions'); const archiver = require('archiver'); const StreamZip = require('node-stream-zip'); const { requireEventOwnership } = require('../middleware/ownership'); +const { assertZipEntriesWithin } = require('../utils/safePath'); const logger = require('../utils/logger'); const { getPagination } = require('../utils/routeHelpers'); const router = express.Router(); @@ -183,6 +184,16 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re const entries = Object.values(await zip.entries()); logger.info(`Archive contains ${entries.length} entries`); + // Reject ZIP-slip entries before writing anything to disk — extract() + // does not neutralise `../` in entry names (GHSA-jfhw-fj23-fx6x). + try { + assertZipEntriesWithin(entries, eventDir); + } catch (slipErr) { + await zip.close(); + logger.warn(`Refusing archive restore — unsafe entry path: ${slipErr.message}`); + return res.status(400).json({ error: 'Archive contains invalid entry paths' }); + } + // Stream-extract everything to disk await zip.extract(null, eventDir); await zip.close(); diff --git a/backend/src/services/picpeakImportService.js b/backend/src/services/picpeakImportService.js index b1a6b0f3..a7a6294b 100644 --- a/backend/src/services/picpeakImportService.js +++ b/backend/src/services/picpeakImportService.js @@ -18,6 +18,7 @@ const fsp = require('fs').promises; const path = require('path'); const os = require('os'); const StreamZip = require('node-stream-zip'); +const { assertZipEntriesWithin } = require('../utils/safePath'); const { db } = require('../database/db'); const knexConfig = require('../../knexfile'); const { getStoragePath } = require('../config/storage'); @@ -232,6 +233,10 @@ async function importFromPicpeak({ picpeakPath, currentAdminId }) { try { const zip = new StreamZip.async({ file: picpeakPath }); try { + // Reject ZIP-slip entries before extracting — a crafted .picpeak could + // otherwise write outside the staging dir via `../` entry names + // (same class as GHSA-jfhw-fj23-fx6x). + assertZipEntriesWithin(Object.values(await zip.entries()), staging); await zip.extract(null, staging); } finally { await zip.close(); diff --git a/backend/src/utils/safePath.js b/backend/src/utils/safePath.js index a1dca605..a90553bf 100644 --- a/backend/src/utils/safePath.js +++ b/backend/src/utils/safePath.js @@ -118,7 +118,41 @@ function assertContractPdfPath(filePath) { ]); } +/** + * ZIP-slip guard. `node-stream-zip`'s `extract(null, root)` writes each entry + * to `path.join(root, entry.name)` without neutralising `../` — a crafted + * archive with an entry named `../../uploads/logos/evil.svg` escapes `root` + * and overwrites arbitrary files (GHSA-jfhw-fj23-fx6x). Call this with the + * entry list BEFORE extract() to reject any entry that resolves outside the + * target directory. + * + * Purely lexical (path.resolve, no realpath) because the extraction target + * does not exist on disk yet. Absolute entry names (`/etc/passwd`) resolve + * away from `root` and are caught too. Throws AppError 400 on the first + * offending entry so the whole archive is refused. + * + * @param {Array<{name?: string}>} entries node-stream-zip entry objects + * @param {string} extractRoot directory extract() will write into + */ +function assertZipEntriesWithin(entries, extractRoot) { + const rootResolved = path.resolve(extractRoot); + const prefix = rootResolved.endsWith(path.sep) ? rootResolved : rootResolved + path.sep; + for (const entry of entries || []) { + const name = entry && entry.name; + if (!name) continue; + const target = path.resolve(rootResolved, name); + if (target !== rootResolved && !target.startsWith(prefix)) { + throw new AppError( + `Archive contains an entry that escapes the extraction directory: ${name}`, + 400, + 'ZIP_SLIP' + ); + } + } +} + module.exports = { assertPathInside, assertContractPdfPath, + assertZipEntriesWithin, }; From dcfcb67f9b2b6294b1ae033f7d0b707b0749a28a Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 16 Jul 2026 10:55:10 +0200 Subject: [PATCH 4/5] fix(security): sanitize chunked-upload filename (GHSA-pc72-jf53-w28j) The chunked video upload stored req.body.filename unmodified and later built the merged path as path.join(tempDir, uploadMeta.filename). path.join does not neutralise '../', so a filename like '../../uploads/logos/evil.svg' escaped the temp dir on merge and overwrote arbitrary files. Requires admin with photos.upload. Fix: path.basename() the client filename in initializeUpload() and reject names that collapse to nothing. Adds a regression test. --- .../services/chunkedUploadFilename.test.js | 52 +++++++++++++++++++ backend/src/services/chunkedUploadService.js | 14 ++++- 2 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 backend/__tests__/services/chunkedUploadFilename.test.js diff --git a/backend/__tests__/services/chunkedUploadFilename.test.js b/backend/__tests__/services/chunkedUploadFilename.test.js new file mode 100644 index 00000000..fd9fd34f --- /dev/null +++ b/backend/__tests__/services/chunkedUploadFilename.test.js @@ -0,0 +1,52 @@ +const path = require('path'); +const os = require('os'); +const fs = require('fs').promises; + +// Point storage at a throwaway temp dir before requiring the service so the +// module-level getStoragePath() picks it up if evaluated. +process.env.STORAGE_PATH = path.join(os.tmpdir(), `picpeak-chunk-test-${process.pid}`); + +const chunkedUpload = require('../../src/services/chunkedUploadService'); + +describe('chunkedUploadService.initializeUpload filename sanitisation (GHSA-pc72-jf53-w28j)', () => { + afterAll(async () => { + await fs.rm(process.env.STORAGE_PATH, { recursive: true, force: true }).catch(() => {}); + }); + + it('strips directory-traversal components from the stored filename', async () => { + const { uploadId } = await chunkedUpload.initializeUpload({ + filename: '../../uploads/logos/evil.svg', + fileSize: 10, + mimeType: 'video/mp4', + eventId: 1, + totalChunks: 1, + }); + const meta = chunkedUpload.getUploadStatus(uploadId); + // basename('../../uploads/logos/evil.svg') === 'evil.svg' — the traversal + // is gone, so path.join(tempDir, filename) can no longer escape tempDir. + expect(meta.filename).toBe('evil.svg'); + }); + + it('keeps a normal filename intact', async () => { + const { uploadId } = await chunkedUpload.initializeUpload({ + filename: 'clip.mp4', + fileSize: 10, + mimeType: 'video/mp4', + eventId: 1, + totalChunks: 1, + }); + expect(uploadId).toBeTruthy(); + }); + + it('rejects a filename that collapses to nothing', async () => { + await expect( + chunkedUpload.initializeUpload({ + filename: '../', + fileSize: 10, + mimeType: 'video/mp4', + eventId: 1, + totalChunks: 1, + }) + ).rejects.toThrow(/Invalid filename/); + }); +}); diff --git a/backend/src/services/chunkedUploadService.js b/backend/src/services/chunkedUploadService.js index 17cc8bb7..b3fe43ff 100644 --- a/backend/src/services/chunkedUploadService.js +++ b/backend/src/services/chunkedUploadService.js @@ -30,6 +30,16 @@ async function initializeUpload(options) { totalChunks } = options; + // Strip any directory components from the client-supplied filename. It is + // later joined onto the temp merge dir (path.join(tempDir, filename)), and + // path.join does NOT neutralise `../` — a filename like `../../uploads/ + // logos/evil.svg` would escape the temp dir and overwrite arbitrary files + // (GHSA-pc72-jf53-w28j). basename() collapses it to the leaf name only. + const safeFilename = path.basename(String(filename || '')); + if (!safeFilename || safeFilename === '.' || safeFilename === '..') { + throw new Error('Invalid filename'); + } + // Generate unique upload ID const uploadId = crypto.randomUUID(); @@ -43,7 +53,7 @@ async function initializeUpload(options) { // Store upload metadata const uploadMeta = { uploadId, - filename, + filename: safeFilename, fileSize, mimeType, eventId, @@ -59,7 +69,7 @@ async function initializeUpload(options) { logger.info('Initialized chunked upload', { uploadId, - filename, + filename: safeFilename, fileSize, expectedChunks, eventId From b00a16159eea4815c70dba8a6ebb13be58ef9492 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 16 Jul 2026 11:57:54 +0200 Subject: [PATCH 5/5] fix(security): harden .picpeak restore operator-preservation (GHSA-qxfx follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The req.admin.id fix activated reinjectCurrentAdmin(); hardening its preservation logic (found across Codex review rounds of #811): - MFA hijack: reinject wrote back only password_hash/is_active/ must_change_password, leaving a crafted backup's two_factor_* on the operator's row — it could strip or replace their second factor. The email- matched row is now updated with the operator's full AUTH set (login identity, password, and all two_factor_* columns). Relationship/audit FKs (role_id, created_by) are deliberately NOT forced from the snapshot: on a cross-instance restore those pre-restore ids may be absent from the backup and would dangle the FK (SQLite rolls back at commit); the restored row keeps its own valid values. - Cross-instance restore rollback / FK safety: reinject matched only by email, so a backup shipping a different admin with the default `admin` username hit UNIQUE(username) and rolled the whole restore back; email and username could even collide on two different rows. Reconciliation is now non-destructive: the email-matching row is updated in place (id preserved → restored FKs like events.created_by stay valid); any different row holding the operator's username is RENAMED, not deleted (deletion would fire ON DELETE actions / dangle references); only when no row has the operator's email is a fresh row inserted, with created_by nulled and an explicit max(id)+1 id (batchInsert left the Postgres identity sequence unadvanced, so a sequence-based insert could collide). - Stale session after restore: admin_users ids shift on restore, but the operator's live JWT is bound only to decoded.id (IP logged not enforced; the backup controls password_changed_at). The route now revokes the token (result checked and logged) and clears the admin cookie; the client redirects to a fresh login via a sessionInvalidated flag. Cookie clear is the unconditional guarantee. Adds SQLite-backed reinject regression tests (in-place login/MFA restore with id and FK columns preserved, username-only rename, email+username on different rows, clean insert with created_by nulled) and the frontend redirect on sessionInvalidated. Deferred (design decisions / pre-existing, need a Postgres test env — see PR discussion): global "invalidate all pre-restore sessions" cutoff; preserving the operator's ROLE semantics across an RBAC-table replace; and resyncing Postgres identity sequences after any restore (batchInsert leaves them behind max(id) — pre-existing, affects every restored table). --- .../services/picpeakReinjectAdmin.test.js | 111 ++++++++++++++++++ backend/src/routes/adminBackup.js | 29 +++++ backend/src/services/picpeakImportService.js | 87 ++++++++++++-- .../components/admin/PicpeakBackupCard.tsx | 8 ++ 4 files changed, 222 insertions(+), 13 deletions(-) create mode 100644 backend/__tests__/services/picpeakReinjectAdmin.test.js diff --git a/backend/__tests__/services/picpeakReinjectAdmin.test.js b/backend/__tests__/services/picpeakReinjectAdmin.test.js new file mode 100644 index 00000000..fe08e444 --- /dev/null +++ b/backend/__tests__/services/picpeakReinjectAdmin.test.js @@ -0,0 +1,111 @@ +/** + * Regression tests for reinjectCurrentAdmin — the operator-preservation step of + * the .picpeak restore (GHSA-qxfx-4493-4v8f follow-up). Runs against a real + * in-memory SQLite DB so the UNIQUE(email)/UNIQUE(username) constraints behave + * as in production. Reconciliation is non-destructive (update-in-place / rename, + * never delete) so restored rows referenced by FKs keep their ids. + */ +const knex = require('knex'); + +let db; +let reinjectCurrentAdmin; + +beforeAll(() => { + jest.doMock('../../knexfile', () => ({ client: 'sqlite3' }), { virtual: false }); + reinjectCurrentAdmin = require('../../src/services/picpeakImportService').reinjectCurrentAdmin; +}); + +beforeEach(async () => { + db = knex({ client: 'sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + await db.schema.createTable('admin_users', (t) => { + t.increments('id'); + t.string('username').notNullable().unique(); + t.string('email').notNullable().unique(); + t.string('password_hash'); + t.boolean('is_active').defaultTo(true); + t.boolean('must_change_password').defaultTo(false); + t.integer('role_id'); + t.integer('created_by'); + t.boolean('two_factor_enabled').defaultTo(false); + t.string('two_factor_secret'); + t.text('two_factor_recovery_codes'); + }); +}); + +afterEach(async () => { await db.destroy(); }); + +const operator = { + id: 1, username: 'admin', email: 'op@example.com', + password_hash: 'OP_HASH', is_active: 1, must_change_password: 0, role_id: 1, created_by: 99, + two_factor_enabled: 1, two_factor_secret: 'OP_SECRET', two_factor_recovery_codes: '["a","b"]', +}; + +test('restores login + MFA in place, keeping the row id and its FK columns (FK-safe)', async () => { + await db('admin_users').insert({ + id: 7, username: 'someoneelse', email: 'OP@example.com', + password_hash: 'ATTACKER', is_active: 1, must_change_password: 0, role_id: 4, created_by: 5, + two_factor_enabled: 0, two_factor_secret: 'ATTACKER_SECRET', two_factor_recovery_codes: null, + }); + await db.transaction((trx) => reinjectCurrentAdmin(trx, operator)); + + const rows = await db('admin_users'); + expect(rows).toHaveLength(1); + const row = rows[0]; + expect(row.id).toBe(7); // id preserved → FK refs hold + expect(row.username).toBe('admin'); + expect(row.password_hash).toBe('OP_HASH'); + expect(Boolean(row.two_factor_enabled)).toBe(true); + expect(row.two_factor_secret).toBe('OP_SECRET'); // attacker MFA secret gone + expect(row.two_factor_recovery_codes).toBe('["a","b"]'); + // Relationship/audit FKs are NOT forced from the operator snapshot (avoids + // dangling role_id/created_by on a cross-instance restore) — the restored + // row keeps its own already-valid values. + expect(row.role_id).toBe(4); + expect(row.created_by).toBe(5); +}); + +test('renames (not deletes) a different row holding the operator username', async () => { + await db('admin_users').insert({ + id: 3, username: 'admin', email: 'other@instance.test', + password_hash: 'OTHER', is_active: 1, role_id: 4, + }); + await expect(db.transaction((trx) => reinjectCurrentAdmin(trx, operator))).resolves.not.toThrow(); + + const rows = await db('admin_users').orderBy('id'); + expect(rows).toHaveLength(2); // the other admin survives (FK-safe) + const other = rows.find((r) => r.id === 3); + expect(other.username).toBe('admin__restored_3'); // renamed, id kept + expect(other.email).toBe('other@instance.test'); + const op = rows.find((r) => r.username === 'admin'); + expect(op.password_hash).toBe('OP_HASH'); +}); + +test('reconciles email and username colliding with DIFFERENT rows without deleting either', async () => { + await db('admin_users').insert([ + { id: 4, username: 'someoneelse', email: 'op@example.com', password_hash: 'A', role_id: 4 }, + { id: 5, username: 'admin', email: 'other@instance.test', password_hash: 'B', role_id: 4 }, + ]); + await expect(db.transaction((trx) => reinjectCurrentAdmin(trx, operator))).resolves.not.toThrow(); + + const rows = await db('admin_users').orderBy('id'); + expect(rows).toHaveLength(2); // both rows survive + const opRow = rows.find((r) => r.id === 4); // email match updated in place + expect(opRow.username).toBe('admin'); + expect(opRow.password_hash).toBe('OP_HASH'); + const renamed = rows.find((r) => r.id === 5); // username holder renamed, not deleted + expect(renamed.username).toBe('admin__restored_5'); +}); + +test('inserts the operator with a non-colliding id when neither key exists in the backup', async () => { + await db('admin_users').insert({ + id: 9, username: 'backupadmin', email: 'backup@instance.test', password_hash: 'B', role_id: 1, + }); + await db.transaction((trx) => reinjectCurrentAdmin(trx, operator)); + + const rows = await db('admin_users').orderBy('id'); + expect(rows).toHaveLength(2); // backup admin untouched + const opRow = rows.find((r) => r.username === 'admin'); + expect(opRow.password_hash).toBe('OP_HASH'); + expect(opRow.id).toBe(10); // max(9)+1, no collision + expect(opRow.created_by).toBeNull(); // self-ref FK nulled so the insert can't dangle +}); diff --git a/backend/src/routes/adminBackup.js b/backend/src/routes/adminBackup.js index b1ee0e97..61705c91 100644 --- a/backend/src/routes/adminBackup.js +++ b/backend/src/routes/adminBackup.js @@ -2,6 +2,8 @@ const express = require('express'); const { db } = require('../database/db'); const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); +const { clearAdminAuthCookie } = require('../utils/tokenUtils'); +const { revokeToken } = require('../utils/tokenRevocation'); const { triggerManualBackup, getBackupStatus, cleanupOldBackupRuns, getBackupManifest, validateBackupManifest } = require('../services/backupService'); const logger = require('../utils/logger'); const { errorResponse, getPagination } = require('../utils/routeHelpers'); @@ -183,11 +185,38 @@ router.post('/picpeak/import', adminAuth, requirePermission('backup.restore'), p // to preserve and the admin_users table was fully replaced by the backup — // letting a crafted .picpeak take over every admin account (GHSA-qxfx-4493-4v8f). const result = await importFromPicpeak({ picpeakPath, currentAdminId: req.admin && req.admin.id }); + + // The restore rewrote admin_users, so ids may have shifted. The operator's + // current JWT is bound only to the pre-restore admin id (adminAuth trusts + // `decoded.id` — IP is logged, not enforced, and the backup controls + // password_changed_at), which could now resolve to a DIFFERENT restored + // account and silently grant its permissions. Force a fresh login instead + // of trusting the old session: revoke the token and clear the cookie. + // Clearing the cookie is the guarantee — it drops the operator's browser + // session unconditionally. Revocation is the extra layer that also kills a + // Bearer-header copy of the JWT; revokeToken() swallows DB errors and + // returns false, so check the result and log loudly if the denylist write + // didn't land (the operator should still re-login, which the cookie clear + // forces). + let tokenRevoked = false; + try { + if (req.token) { + tokenRevoked = await revokeToken(req.token, 'picpeak-import', { adminId: req.admin && req.admin.id }); + } + } catch (revokeErr) { + logger.warn('[picpeak-import] failed to revoke session token after restore', { error: revokeErr.message }); + } + if (req.token && !tokenRevoked) { + logger.warn('[picpeak-import] session token was NOT added to the revocation denylist after restore; relying on cookie clear to force re-login'); + } + clearAdminAuthCookie(res); + res.json({ success: true, tables: result.tables, filesRestored: result.filesRestored, usesExternalMedia: result.usesExternalMedia, + sessionInvalidated: true, }); } catch (error) { const status = error.statusCode || 500; diff --git a/backend/src/services/picpeakImportService.js b/backend/src/services/picpeakImportService.js index a7a6294b..27ac33a0 100644 --- a/backend/src/services/picpeakImportService.js +++ b/backend/src/services/picpeakImportService.js @@ -81,25 +81,85 @@ function parseNdjson(filePath) { } // Re-insert the operator's account inside the restore transaction so they keep -// working credentials. If the backup already loaded an admin with the same -// email, overwrite that row's credentials with the current account's (current -// creds win); otherwise insert the snapshot with a fresh id. +// working credentials after the wipe. +// +// The operator's login + credentials + MFA must be restored, not just the +// password. A crafted backup can carry a row with the operator's email whose +// two_factor_* fields are attacker-chosen — leaving those in place would let +// the backup strip or hijack the operator's MFA, or (cross-instance) pin a TOTP +// secret encrypted with the source instance's key the operator can never +// satisfy. These columns are scalar/text (recovery codes are a JSON string in a +// TEXT column), so writing them needs no special json handling. Relationship/ +// audit FKs (role_id, created_by) are deliberately NOT forced from the snapshot +// — see the update branch below. +// +// admin_users has UNIQUE constraints on BOTH email and username, and a restored +// backup can collide with the operator on either — possibly on two DIFFERENT +// rows (one shares the email, another shares the default `admin` username). We +// reconcile WITHOUT deleting any restored row: deleting would fire ON DELETE +// actions (SQLite) or dangle references such as events.created_by (Postgres, +// where replica mode suppresses cascades). Instead: +// - if a row already has the operator's email, overwrite it in place (its id +// is preserved, so every FK pointing at the operator stays valid); +// - if a DIFFERENT row holds the operator's username, rename that row (id +// preserved, its own FKs stay valid) to free the username; +// - only when no row has the operator's email do we insert a fresh row. async function reinjectCurrentAdmin(trx, currentAdmin) { if (!currentAdmin) return; - const existing = await trx('admin_users').whereRaw('lower(email) = lower(?)', [currentAdmin.email]).first(); - if (existing) { - await trx('admin_users').where({ id: existing.id }).update({ - password_hash: currentAdmin.password_hash, - is_active: currentAdmin.is_active, - must_change_password: currentAdmin.must_change_password, - }); + + const emailMatch = await trx('admin_users') + .whereRaw('lower(email) = lower(?)', [currentAdmin.email]) + .first(); + + // Free the operator's username if a different row holds it (rename, not delete). + const usernameHolder = await trx('admin_users') + .whereRaw('lower(username) = lower(?)', [currentAdmin.username]) + .first(); + if (usernameHolder && (!emailMatch || usernameHolder.id !== emailMatch.id)) { + await trx('admin_users') + .where({ id: usernameHolder.id }) + .update({ username: `${usernameHolder.username}__restored_${usernameHolder.id}` }); + } + + if (emailMatch) { + // Update in place — keeps emailMatch.id so restored FKs to the operator + // hold. Write only the AUTH-critical columns (login identity + credentials + // + MFA), never the relationship/audit FKs (role_id → roles, created_by → + // admin_users). Forcing the operator's pre-restore role_id/created_by here + // could reference rows absent from a cross-instance backup and dangle the + // FK (SQLite rolls back at commit); the row already carries the backup's + // own valid values for those. This still closes the MFA-hijack gap — a + // crafted backup can't strip or replace the operator's second factor. + const authUpdate = {}; + for (const field of PRESERVED_AUTH_FIELDS) { + if (field in currentAdmin) authUpdate[field] = currentAdmin[field]; + } + await trx('admin_users').where({ id: emailMatch.id }).update(authUpdate); } else { - const row = { ...currentAdmin }; - delete row.id; // let the engine assign a fresh id to avoid collision - await trx('admin_users').insert(row); + // The operator's email isn't in the backup, so nothing restored references + // their id — a fresh row can't dangle a reference TO the operator. Null the + // self-referential created_by (its target admin may be absent from this + // backup; ON DELETE SET NULL makes null the correct "unknown inviter" + // value) so the insert itself can't dangle. Use an explicit max(id)+1 + // rather than the identity sequence, which batchInsert left unadvanced on + // Postgres (a sequence-based insert could collide with a restored id). + const snapshot = { ...currentAdmin }; + delete snapshot.id; + if ('created_by' in snapshot) snapshot.created_by = null; + const maxRow = await trx('admin_users').max({ m: 'id' }).first(); + snapshot.id = (Number(maxRow && maxRow.m) || 0) + 1; + await trx('admin_users').insert(snapshot); } } +// AUTH-critical admin_users columns preserved when overwriting a restored row +// that shares the operator's email. Deliberately excludes relationship/audit +// FKs (role_id, created_by) — see reinjectCurrentAdmin for why. +const PRESERVED_AUTH_FIELDS = [ + 'username', 'email', 'password_hash', 'is_active', 'must_change_password', + 'two_factor_enabled', 'two_factor_secret', 'two_factor_recovery_codes', 'two_factor_enrolled_at', +]; + // The json/jsonb columns of a table (Postgres only). The pg driver returns // jsonb as parsed JS values, so on re-insert they must be serialised back to // valid JSON text — otherwise a scalar like the string "PicPeak" is sent @@ -273,4 +333,5 @@ module.exports = { importFromPicpeak, readManifestFromZip, validateManifest, + reinjectCurrentAdmin, }; diff --git a/frontend/src/components/admin/PicpeakBackupCard.tsx b/frontend/src/components/admin/PicpeakBackupCard.tsx index 10ae1e60..1c9f947f 100644 --- a/frontend/src/components/admin/PicpeakBackupCard.tsx +++ b/frontend/src/components/admin/PicpeakBackupCard.tsx @@ -16,6 +16,7 @@ interface RestoreResult { tables: number; filesRestored: number; usesExternalMedia: boolean; + sessionInvalidated?: boolean; } // ── Download half (Dashboard) ──────────────────────────────────────────────── @@ -114,6 +115,13 @@ export const PicpeakRestoreCard: React.FC = () => { setResult(res.data); setPendingFile(null); toast.success(t('backup.picpeak.restoreDone', 'Backup restored.')); + // The restore rewrote admin_users and the backend revoked our session + // (ids may have shifted). Send the operator to a fresh login rather than + // letting the now-stale token resolve to a different restored account. + if (res.data?.sessionInvalidated) { + toast.success(t('backup.picpeak.reloginRequired', 'Restore complete — please sign in again.')); + setTimeout(() => { window.location.href = '/admin/login'; }, 1500); + } } catch (e: any) { const msg = e.response?.data?.error || t('backup.picpeak.restoreFailed', 'Restore failed.'); toast.error(msg);