Merge pull request #812 from PicPeak/fix/security-advisories-backend-stable

fix(security): close 4 open security advisories on stable (backup takeover, share-login bypass, ZIP slip, chunked-upload traversal)

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();
+  });
+});

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/);
+  });
+});

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
+});

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/);
+  });
+});

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();

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');
@@ -178,12 +180,43 @@ 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 });
+
+    // 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;

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: {

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

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');
@@ -80,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
@@ -232,6 +293,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();
@@ -268,4 +333,5 @@ module.exports = {
   importFromPicpeak,
   readManifestFromZip,
   validateManifest,
+  reinjectCurrentAdmin,
 };

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,
 };

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);