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