fix(backup): address .picpeak review — table filter, superuser guard, tests
From the-luap's review: - Import no longer trusts manifest.tables blindly. It now intersects the manifest's table list with the real data tables of THIS database (listDataTables(), which already excludes knex_migrations/_lock) and drops anything else. A crafted/corrupted .picpeak listing knex_migrations or a non-existent table can no longer wipe it; skipped tables are logged. - The Postgres session_replication_role='replica' SET (needs superuser) is now wrapped: on a managed-PG non-superuser it fails BEFORE any rows are deleted (transaction rolls back) and surfaces a clear, actionable 400 instead of a cryptic permission error. - Export: on an archiver error, the temp out dir (a partial plaintext-secret archive) is now removed instead of orphaned. Tests (+4, now 26): engine-mismatch rejection, forward-only newer-refused, non-picpeak rejection, and files/ restored + filesRestored asserted.
This commit is contained in:
@@ -18,13 +18,14 @@ let cleanup;
|
|||||||
let tmpDir;
|
let tmpDir;
|
||||||
let createPicpeak;
|
let createPicpeak;
|
||||||
let importFromPicpeak;
|
let importFromPicpeak;
|
||||||
|
let validateManifest;
|
||||||
let superAdminRoleId;
|
let superAdminRoleId;
|
||||||
|
|
||||||
beforeAll(async () => {
|
beforeAll(async () => {
|
||||||
({ db, cleanup, tmpDir } = await bootCrmDb());
|
({ db, cleanup, tmpDir } = await bootCrmDb());
|
||||||
process.env.STORAGE_PATH = tmpDir;
|
process.env.STORAGE_PATH = tmpDir;
|
||||||
({ createPicpeak } = require('../../src/services/picpeakExportService'));
|
({ createPicpeak } = require('../../src/services/picpeakExportService'));
|
||||||
({ importFromPicpeak } = require('../../src/services/picpeakImportService'));
|
({ importFromPicpeak, validateManifest } = require('../../src/services/picpeakImportService'));
|
||||||
const role = await db('roles').where({ name: 'super_admin' }).first();
|
const role = await db('roles').where({ name: 'super_admin' }).first();
|
||||||
superAdminRoleId = role.id;
|
superAdminRoleId = role.id;
|
||||||
}, 60000);
|
}, 60000);
|
||||||
@@ -116,4 +117,64 @@ describe('.picpeak roundtrip (export → import)', () => {
|
|||||||
fs.rmSync(path.dirname(filePath), { recursive: true, force: true });
|
fs.rmSync(path.dirname(filePath), { recursive: true, force: true });
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('restores files/ and reports filesRestored', async () => {
|
||||||
|
// A business-doc that lives in storage → travels in the backup.
|
||||||
|
const docDir = path.join(tmpDir, 'business-docs');
|
||||||
|
const marker = path.join(docDir, 'roundtrip-doc.txt');
|
||||||
|
fs.mkdirSync(docDir, { recursive: true });
|
||||||
|
fs.writeFileSync(marker, 'hello');
|
||||||
|
await db('admin_users').del();
|
||||||
|
const [id] = await db('admin_users').insert(adminRow('[email protected]', 'H')).returning('id');
|
||||||
|
const currentAdminId = typeof id === 'object' ? id.id : id;
|
||||||
|
|
||||||
|
const { filePath } = await createPicpeak({ includePhotos: false });
|
||||||
|
try {
|
||||||
|
fs.rmSync(marker); // delete on disk so the restore must bring it back
|
||||||
|
const result = await importFromPicpeak({ picpeakPath: filePath, currentAdminId });
|
||||||
|
expect(result.filesRestored).toBeGreaterThanOrEqual(1);
|
||||||
|
expect(fs.existsSync(marker)).toBe(true);
|
||||||
|
expect(fs.readFileSync(marker, 'utf8')).toBe('hello');
|
||||||
|
} finally {
|
||||||
|
fs.rmSync(path.dirname(filePath), { recursive: true, force: true });
|
||||||
|
fs.rmSync(docDir, { recursive: true, force: true });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('.picpeak manifest validation', () => {
|
||||||
|
it('rejects a database-engine mismatch', async () => {
|
||||||
|
// Harness runs on SQLite, so a pg manifest must be refused.
|
||||||
|
const blockers = await validateManifest({
|
||||||
|
kind: 'picpeak-backup', format: 1, database: { engine: 'pg' }, tables: {},
|
||||||
|
});
|
||||||
|
expect(blockers.some((b) => /engine/i.test(b))).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('rejects a backup from a newer schema (forward-only)', async () => {
|
||||||
|
// validateManifest reads knex_migrations for the target's latest migration;
|
||||||
|
// the harness has none, so create it with an older migration than the backup.
|
||||||
|
await db.schema.createTable('knex_migrations', (t) => {
|
||||||
|
t.increments('id');
|
||||||
|
t.string('name');
|
||||||
|
t.integer('batch');
|
||||||
|
t.timestamp('migration_time');
|
||||||
|
});
|
||||||
|
try {
|
||||||
|
await db('knex_migrations').insert({ name: '100_baseline', batch: 1 });
|
||||||
|
const blockers = await validateManifest({
|
||||||
|
kind: 'picpeak-backup', format: 1,
|
||||||
|
database: { engine: 'sqlite', latest_migration: '999_from_the_future' },
|
||||||
|
tables: {},
|
||||||
|
});
|
||||||
|
expect(blockers.some((b) => /newer/i.test(b))).toBe(true);
|
||||||
|
} finally {
|
||||||
|
await db.schema.dropTableIfExists('knex_migrations');
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it('rejects a file that is not a PicPeak backup', async () => {
|
||||||
|
const blockers = await validateManifest({ some: 'random-json' });
|
||||||
|
expect(blockers.length).toBeGreaterThan(0);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -184,23 +184,31 @@ async function createPicpeak({ includePhotos = false, outDir } = {}) {
|
|||||||
const stamp = manifest.created_at.replace(/[:.]/g, '-');
|
const stamp = manifest.created_at.replace(/[:.]/g, '-');
|
||||||
const filePath = path.join(targetDir, `picpeak-backup-${stamp}.picpeak`);
|
const filePath = path.join(targetDir, `picpeak-backup-${stamp}.picpeak`);
|
||||||
|
|
||||||
await new Promise((resolve, reject) => {
|
try {
|
||||||
const output = fs.createWriteStream(filePath);
|
await new Promise((resolve, reject) => {
|
||||||
const archive = archiver('zip', { zlib: { level: 9 } });
|
const output = fs.createWriteStream(filePath);
|
||||||
output.on('close', resolve);
|
const archive = archiver('zip', { zlib: { level: 9 } });
|
||||||
output.on('error', reject);
|
output.on('close', resolve);
|
||||||
archive.on('error', reject);
|
output.on('error', reject);
|
||||||
// Surface archiver warnings (e.g. a file vanished mid-run) instead of
|
archive.on('error', reject);
|
||||||
// silently shipping an incomplete archive.
|
// Surface archiver warnings (e.g. a file vanished mid-run) instead of
|
||||||
archive.on('warning', (err) => reject(err));
|
// silently shipping an incomplete archive.
|
||||||
archive.pipe(output);
|
archive.on('warning', (err) => reject(err));
|
||||||
archive.file(path.join(staging, 'manifest.json'), { name: 'manifest.json' });
|
archive.pipe(output);
|
||||||
archive.directory(dataDir, 'data');
|
archive.file(path.join(staging, 'manifest.json'), { name: 'manifest.json' });
|
||||||
for (const f of files) {
|
archive.directory(dataDir, 'data');
|
||||||
archive.file(f.abs, { name: path.posix.join('files', f.rel.split(path.sep).join('/')) });
|
for (const f of files) {
|
||||||
}
|
archive.file(f.abs, { name: path.posix.join('files', f.rel.split(path.sep).join('/')) });
|
||||||
archive.finalize();
|
}
|
||||||
});
|
archive.finalize();
|
||||||
|
});
|
||||||
|
} catch (err) {
|
||||||
|
// Archiver failed → the partial .picpeak holds plaintext secrets and is
|
||||||
|
// useless; remove our own temp out dir so it isn't orphaned. A
|
||||||
|
// caller-supplied outDir is left untouched.
|
||||||
|
if (!outDir) await fsp.rm(targetDir, { recursive: true, force: true }).catch(() => {});
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
|
||||||
logger.info(
|
logger.info(
|
||||||
`[picpeak-export] wrote ${filePath} (${tables.length} tables, ${files.length} files, includePhotos=${!!includePhotos})`
|
`[picpeak-export] wrote ${filePath} (${tables.length} tables, ${files.length} files, includePhotos=${!!includePhotos})`
|
||||||
@@ -215,6 +223,7 @@ async function createPicpeak({ includePhotos = false, outDir } = {}) {
|
|||||||
|
|
||||||
module.exports = {
|
module.exports = {
|
||||||
PICPEAK_FORMAT_VERSION,
|
PICPEAK_FORMAT_VERSION,
|
||||||
|
EXCLUDED_TABLES,
|
||||||
createPicpeak,
|
createPicpeak,
|
||||||
// exported for reuse/testing
|
// exported for reuse/testing
|
||||||
listDataTables,
|
listDataTables,
|
||||||
|
|||||||
@@ -23,7 +23,7 @@ const knexConfig = require('../../knexfile');
|
|||||||
const { getStoragePath } = require('../config/storage');
|
const { getStoragePath } = require('../config/storage');
|
||||||
const { hasColumnCached } = require('../utils/schemaCache');
|
const { hasColumnCached } = require('../utils/schemaCache');
|
||||||
const logger = require('../utils/logger');
|
const logger = require('../utils/logger');
|
||||||
const { PICPEAK_FORMAT_VERSION } = require('./picpeakExportService');
|
const { PICPEAK_FORMAT_VERSION, EXCLUDED_TABLES, listDataTables } = require('./picpeakExportService');
|
||||||
|
|
||||||
const isPostgres = () => knexConfig.client === 'pg';
|
const isPostgres = () => knexConfig.client === 'pg';
|
||||||
|
|
||||||
@@ -129,8 +129,23 @@ function serialiseJsonColumns(rows, jsonCols) {
|
|||||||
// in the data set, so the target's schema/migration state is left intact.
|
// in the data set, so the target's schema/migration state is left intact.
|
||||||
async function replaceAllTables(tables, dataDir, currentAdmin) {
|
async function replaceAllTables(tables, dataDir, currentAdmin) {
|
||||||
await db.transaction(async (trx) => {
|
await db.transaction(async (trx) => {
|
||||||
if (isPostgres()) await trx.raw("SET session_replication_role = 'replica'");
|
if (isPostgres()) {
|
||||||
else await trx.raw('PRAGMA defer_foreign_keys = ON');
|
try {
|
||||||
|
await trx.raw("SET session_replication_role = 'replica'");
|
||||||
|
} catch (_) {
|
||||||
|
// session_replication_role requires a Postgres SUPERUSER. The bundled
|
||||||
|
// postgres image's role is one; managed Postgres (RDS / Cloud SQL / …)
|
||||||
|
// app users usually are not. Fail fast with a clear message BEFORE any
|
||||||
|
// rows are deleted — the transaction rolls back, so nothing is wiped.
|
||||||
|
const err = new Error(
|
||||||
|
'Restore needs a PostgreSQL superuser to suspend foreign-key checks during the full replace, but this instance’s database user is not a superuser (common on managed Postgres such as RDS or Cloud SQL). Restore onto the bundled Postgres, or grant the role superuser for the restore.'
|
||||||
|
);
|
||||||
|
err.statusCode = 400;
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
await trx.raw('PRAGMA defer_foreign_keys = ON');
|
||||||
|
}
|
||||||
|
|
||||||
for (const table of tables) {
|
for (const table of tables) {
|
||||||
await trx(table).del();
|
await trx(table).del();
|
||||||
@@ -223,7 +238,18 @@ async function importFromPicpeak({ picpeakPath, currentAdminId }) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const dataDir = path.join(staging, 'data');
|
const dataDir = path.join(staging, 'data');
|
||||||
const tables = Object.keys(manifest.tables || {});
|
// Only touch tables that (a) the uploaded manifest lists AND (b) actually
|
||||||
|
// exist as real tables in THIS database. listDataTables() already excludes
|
||||||
|
// knex_migrations/_lock (EXCLUDED_TABLES), so a crafted or corrupted
|
||||||
|
// .picpeak can never make the restore delete the migration bookkeeping — or
|
||||||
|
// any table that isn't a genuine data table here.
|
||||||
|
const dbTables = new Set(await listDataTables());
|
||||||
|
const manifestTables = Object.keys(manifest.tables || {});
|
||||||
|
const tables = manifestTables.filter((tbl) => dbTables.has(tbl) && !EXCLUDED_TABLES.has(tbl));
|
||||||
|
const skipped = manifestTables.filter((tbl) => !tables.includes(tbl));
|
||||||
|
if (skipped.length) {
|
||||||
|
logger.warn(`[picpeak-import] ignoring ${skipped.length} backup table(s) not present in this DB (or protected): ${skipped.join(', ')}`);
|
||||||
|
}
|
||||||
|
|
||||||
await replaceAllTables(tables, dataDir, currentAdmin);
|
await replaceAllTables(tables, dataDir, currentAdmin);
|
||||||
const filesRestored = await restoreFiles(staging);
|
const filesRestored = await restoreFiles(staging);
|
||||||
|
|||||||
Reference in New Issue
Block a user