paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(backup): inline DB dump + fail-loud guard so "Run Backup Now" can't ship files-only

Browse Source 
The previous file-backup workflow only LOOKED UP an existing
database dump via getDatabaseBackupInfo() and silently shipped a
files-only manifest when none was found. Admins clicking "Run
Backup Now" (or relying on the schedule) got an apparent success
that omitted every customer / quote / invoice / contract / payment-
log row. The data-loss footgun was discovered 2026-05-29 when an
admin who'd been "backing up" for weeks via the UI lost the entire
CRM after a routine docker compose down -v — every produced
manifest had database: { backup_file: null, size: 0, tables: {} }.
New helper `ensureDatabaseDumpForBackup(config)` encapsulates:
  1. Inline pg_dump (or SQLite copy) before the file scan, via
     databaseBackupService.backup(). Result lands in
     database_backup_runs and is picked up by the existing
     getDatabaseBackupInfo lookup that writes the manifest.
  2. Fail-loud guard: if no usable dump file is reachable (path
     missing, 0 bytes, or never existed), throw — the existing
     catch in runBackupInternal marks the backup_runs row failed
     with the error_message and emails the admin if configured.
     No more silent files-only manifests.
  3. Opt-out: `backup_database_inline_dump = false` skips the
     inline dump for admins who already run their own scheduled
     `backup_database_schedule`. The fail-loud guard still
     applies, so an opted-out install with no recent dump still
     aborts loudly instead of producing a partial backup. Default
     ON is encoded as "skip only when explicitly false" — undefined
     (existing installs upgrading) falls through to the safe-
     default ON branch.
The helper returns the verified `databaseInfo` so the manifest-build
step at runBackupInternal:917 reuses it instead of calling
getDatabaseBackupInfo a second time. S3/future destinations that
override `result.databaseInfo` are still respected (the existing
`result.databaseInfo ||` fallback shape stays put).
Test suite covers: default-on happy path, dump-throws-aborts-run,
opt-out + recent dump + proceeds, opt-out + no-dump + fail-loud,
opt-out + 0-byte dump + fail-loud. Mocks
databaseBackupService.backup so the tests don't depend on pg_dump
or sqlite3 CLI binaries being installed.
Stage A of three-stage backup hardening plan. Stage B (config-
driven walker) and Stage C (audit + diagnostic UI) follow in
separate commits.
This commit is contained in:
Luca
2026-05-29 22:00:15 +02:00
parent 7c230bdc24
commit 7fdf01ad21
2 changed files with 93 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/services/backupService.js
+79 -60
View File
@@ -259,6 +259,73 @@ async function hasDatabaseChanged(sinceTime) {
}
}
/**
* Run an inline database dump (default ON) and then verify a usable dump
* is actually on disk before letting the file-backup proceed. Returns the
* verified `databaseInfo` so the caller can pass it straight into the
* manifest builder without re-querying.
*
* Why this lives here and not inline in `runBackupInternal`:
* - Encapsulates the "Run Backup Now must include DB" guarantee
* introduced when the silent files-only bug was discovered
* (2026-05-29 — admin lost CRM after `docker compose down -v`)
* - Lets the manifest path share the same `databaseInfo` object
* instead of doing a second `getDatabaseBackupInfo()` round-trip
* - Thrown errors bubble up to `runBackupInternal`'s catch, which
* marks the `backup_runs` row failed and queues the admin email
*
* Default-ON semantics: `backup_database_inline_dump` is only treated
* as disabled when explicitly set to false. `undefined` (the case on
* every existing install that predates the setting) falls through to
* the safe-default ON branch. `normalizeBoolean(undefined)` returns
* false, so a naive `!== false` check would silently disable the
* inline dump for every upgrading install.
*/
async function ensureDatabaseDumpForBackup(config) {
const inlineDumpExplicitlyOff = config.backup_database_inline_dump !== undefined
&& config.backup_database_inline_dump !== null
&& normalizeBoolean(config.backup_database_inline_dump) === false;
if (!inlineDumpExplicitlyOff) {
logger.info('Running inline database dump before file backup...');
const { databaseBackupService } = require('./databaseBackup');
const dumpResult = await databaseBackupService.backup({});
logger.info(`Inline database dump completed: ${dumpResult.path} ` +
`(${(dumpResult.size / 1024 / 1024).toFixed(2)} MB)`);
}
const databaseInfo = await service.getDatabaseBackupInfo();
if (!databaseInfo.backupFile) {
throw new Error(
'No database backup available to include in this file backup. ' +
'Either keep backup_database_inline_dump enabled (default) or configure ' +
'backup_database_schedule and let it run at least once first.'
);
}
let dumpStat;
try {
dumpStat = await fs.stat(databaseInfo.backupFile);
} catch (statErr) {
if (statErr.code === 'ENOENT') {
throw new Error(
`Database backup file at ${databaseInfo.backupFile} is missing from disk. ` +
'Refusing to proceed with file backup; configure backup_database_schedule or ' +
'keep backup_database_inline_dump enabled.'
);
}
throw statErr;
}
if (!dumpStat.size) {
throw new Error(
`Database backup file at ${databaseInfo.backupFile} is empty (0 bytes). ` +
'Refusing to proceed with file backup to avoid shipping a manifest with no DB content.'
);
}
return databaseInfo;
}
async function getDatabaseBackupInfoInternal() {
try {
const recent = await db('database_backup_runs')
@@ -814,65 +881,11 @@ async function runBackupInternal(isManual = false) {
}).returning('id');
runId = insertResult[0]?.id || insertResult[0];
// Inline database dump (default ON). Previously, runBackup only LOOKED UP
// an existing database dump via getDatabaseBackupInfo and silently shipped
// a files-only manifest when none was found — admins clicking "Run Backup
// Now" got an apparent success that omitted every customer / quote /
// invoice / contract row. Triggering pg_dump (or the SQLite copy) here
// makes "file backup" always include a fresh database snapshot. Admins
// who run their own scheduled dumps via backup_database_schedule can opt
// out with backup_database_inline_dump = false; the fail-loud guard
// below still catches the case where no recent dump exists.
//
// Default ON is encoded as "skip only when explicitly false". `undefined`
// (setting not yet inserted on existing installs) falls through to the
// ON path, which is the data-loss-safe default. normalizeBoolean(undefined)
// returns false, so checking inequality against false would inadvertently
// disable on unset — guard with `!== undefined` first.
const inlineDumpExplicitlyOff = config.backup_database_inline_dump !== undefined
&& config.backup_database_inline_dump !== null
&& normalizeBoolean(config.backup_database_inline_dump) === false;
if (!inlineDumpExplicitlyOff) {
logger.info('Running inline database dump before file backup...');
const { databaseBackupService } = require('./databaseBackup');
const dumpResult = await databaseBackupService.backup({});
logger.info(`Inline database dump completed: ${dumpResult.path} ` +
`(${(dumpResult.size / 1024 / 1024).toFixed(2)} MB)`);
}
// Fail-loud guard: a "file backup" without a DB component is a data-loss
// trap. Whether the dump came from the inline step above or from a
// separately-scheduled database backup, we require a usable dump file
// before proceeding. Throws — the catch block marks the backup_runs row
// failed with this error_message and emails the admin if configured.
const dbInfoCheck = await service.getDatabaseBackupInfo();
if (!dbInfoCheck.backupFile) {
throw new Error(
'No database backup available to include in this file backup. ' +
'Either keep backup_database_inline_dump enabled (default) or configure ' +
'backup_database_schedule and let it run at least once first.'
);
}
try {
const dumpStat = await fs.stat(dbInfoCheck.backupFile);
if (!dumpStat.size || dumpStat.size === 0) {
throw new Error(
`Database backup file at ${dbInfoCheck.backupFile} is empty (0 bytes). ` +
'Refusing to proceed with file backup to avoid shipping a manifest with no DB content.'
);
}
} catch (statErr) {
// fs.stat throws if file doesn't exist; preserve the more specific
// empty-file error from the inner block.
if (statErr.code === 'ENOENT') {
throw new Error(
`Database backup file at ${dbInfoCheck.backupFile} is missing from disk. ` +
'Refusing to proceed with file backup; configure backup_database_schedule or ' +
'keep backup_database_inline_dump enabled.'
);
}
throw statErr;
}
// Inline DB dump + fail-loud verification. The returned `databaseInfo`
// is reused at manifest-build time below so we don't pay a second
// `getDatabaseBackupInfo()` round-trip — see `ensureDatabaseDumpForBackup`
// for the full rationale.
const verifiedDatabaseInfo = await ensureDatabaseDumpForBackup(config);
const files = await service.getFilesToBackup(config.backup_include_archived);
logger.info(`Found ${files.length} files to check for backup`);
@@ -901,7 +914,13 @@ async function runBackupInternal(isManual = false) {
const previousBackup = await getPreviousSuccessfulBackup(runId);
const manifestFiles = buildManifestFiles(result.backedUpFiles, files);
const databaseInfo = result.databaseInfo || await service.getDatabaseBackupInfo();
// `verifiedDatabaseInfo` came from ensureDatabaseDumpForBackup at the
// top of this run — reuse it so manifest building doesn't pay a
// second `getDatabaseBackupInfo()` round-trip. The
// `result.databaseInfo` branch is kept for destination implementations
// (S3, future destinations) that override the local info on the result
// object; falls back to the verified copy otherwise.
const databaseInfo = result.databaseInfo || verifiedDatabaseInfo;
const manifestOptions = {
backupType: previousBackup ? 'incremental' : 'full',