chore(db): log swallowed errors in addon-disable migration + guard against destructive migrations

The migration that disables the legacy "memories" addon swallowed any
error in an empty catch, as did ~30 other catch blocks in the migration
runner (column adds, the journey rebuild, index probes). Replace each
silent catch with the existing console.warn('[migrations] ...') log so
failures are visible. Control flow is unchanged: every step stays
non-fatal, nothing new is thrown.

Add a static guardrail test that scans the migration source and fails
when a new destructive statement (DROP TABLE / DROP COLUMN / TRUNCATE /
DELETE FROM / ALTER ... DROP) appears outside a reviewed allowlist, and
when an empty/silent catch block is reintroduced. The existing
destructive statements are all legitimate table rebuilds or
bounded cleanups and are recorded in the allowlist with a reason.
This commit is contained in:
Maurice
2026-05-31 15:12:02 +02:00
parent 3977a5ecba
commit 5a0124e7cb
2 changed files with 237 additions and 30 deletions
+43 -30
View File
@@ -542,7 +542,7 @@ function runMigrations(db: Database.Database): void {
}
},
() => {
try { db.exec('ALTER TABLE budget_items ADD COLUMN expense_date TEXT DEFAULT NULL'); } catch {}
try { db.exec('ALTER TABLE budget_items ADD COLUMN expense_date TEXT DEFAULT NULL'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
},
() => {
db.exec(`
@@ -803,7 +803,14 @@ function runMigrations(db: Database.Database): void {
`);
},
() => {
try {db.exec("UPDATE addons SET enabled = 0 WHERE id = 'memories'");} catch (err) {}
try {
db.exec("UPDATE addons SET enabled = 0 WHERE id = 'memories'");
} catch (err) {
// Non-fatal: the addons table may not exist yet on very old databases.
// Disabling the legacy memories addon is best-effort, but we no longer
// swallow the error silently.
console.warn("[migrations] Non-fatal: failed to disable legacy 'memories' addon:", err);
}
},
// Migration 69: Place region cache for sub-national Atlas regions
() => {
@@ -1194,21 +1201,21 @@ function runMigrations(db: Database.Database): void {
// Migration 85: Journal — richer entry fields for magazine-style design
() => {
// Highlight tags (JSON array), visibility control, hero photo, color accent
try { db.exec('ALTER TABLE journey_entries ADD COLUMN highlight_tags TEXT'); } catch {}
try { db.exec("ALTER TABLE journey_entries ADD COLUMN visibility TEXT NOT NULL DEFAULT 'private'"); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN hero_photo_id TEXT'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN color_accent TEXT'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN place_name TEXT'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN place_id INTEGER REFERENCES places(id) ON DELETE SET NULL'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN lat REAL'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN lng REAL'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN highlight_tags TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec("ALTER TABLE journey_entries ADD COLUMN visibility TEXT NOT NULL DEFAULT 'private'"); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_entries ADD COLUMN hero_photo_id TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_entries ADD COLUMN color_accent TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_entries ADD COLUMN place_name TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_entries ADD COLUMN place_id INTEGER REFERENCES places(id) ON DELETE SET NULL'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_entries ADD COLUMN lat REAL'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_entries ADD COLUMN lng REAL'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
// Check-in: allow a single cover photo reference
try { db.exec('ALTER TABLE journey_checkins ADD COLUMN photo_id TEXT'); } catch {}
try { db.exec('ALTER TABLE journey_checkins ADD COLUMN photo_id TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
// Photos: add caption edit timestamp for gallery ordering
try { db.exec('ALTER TABLE journey_photos ADD COLUMN width INTEGER'); } catch {}
try { db.exec('ALTER TABLE journey_photos ADD COLUMN height INTEGER'); } catch {}
try { db.exec('ALTER TABLE journey_photos ADD COLUMN width INTEGER'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_photos ADD COLUMN height INTEGER'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
},
// Migration 86: Journey multi-trip support + sharing/collaboration
() => {
@@ -1239,8 +1246,8 @@ function runMigrations(db: Database.Database): void {
db.exec('CREATE INDEX IF NOT EXISTS idx_journey_members_user ON journey_members(user_id)');
// author tracking on entries and checkins
try { db.exec('ALTER TABLE journey_entries ADD COLUMN user_id INTEGER REFERENCES users(id) ON DELETE SET NULL'); } catch {}
try { db.exec('ALTER TABLE journey_checkins ADD COLUMN user_id INTEGER REFERENCES users(id) ON DELETE SET NULL'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN user_id INTEGER REFERENCES users(id) ON DELETE SET NULL'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_checkins ADD COLUMN user_id INTEGER REFERENCES users(id) ON DELETE SET NULL'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
},
// Migration 87: Journey rebuild — new schema with trip sync
() => {
@@ -1255,9 +1262,9 @@ function runMigrations(db: Database.Database): void {
if (hasOldJourneys) {
// Save existing data before dropping
try { oldJourneys = db.prepare('SELECT * FROM journeys').all(); } catch {}
try { oldEntries = db.prepare('SELECT * FROM journey_entries').all(); } catch {}
try { oldPhotos = db.prepare('SELECT * FROM journey_photos').all(); } catch {}
try { oldJourneys = db.prepare('SELECT * FROM journeys').all(); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { oldEntries = db.prepare('SELECT * FROM journey_entries').all(); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { oldPhotos = db.prepare('SELECT * FROM journey_photos').all(); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
// Drop all old journey tables
db.exec('DROP TABLE IF EXISTS journey_location_trail');
@@ -1393,7 +1400,7 @@ function runMigrations(db: Database.Database): void {
INSERT OR IGNORE INTO journey_trips (journey_id, trip_id, added_at)
VALUES (?, ?, ?)
`).run(Number(res.lastInsertRowid), j.trip_id, ts);
} catch {}
} catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
}
}
@@ -1450,10 +1457,10 @@ function runMigrations(db: Database.Database): void {
},
// Migration 88: Journey photos — provider support (Immich/Synology)
() => {
try { db.exec("ALTER TABLE journey_photos ADD COLUMN provider TEXT NOT NULL DEFAULT 'local'"); } catch {}
try { db.exec('ALTER TABLE journey_photos ADD COLUMN asset_id TEXT'); } catch {}
try { db.exec('ALTER TABLE journey_photos ADD COLUMN owner_id INTEGER REFERENCES users(id)'); } catch {}
try { db.exec('ALTER TABLE journey_photos ADD COLUMN shared INTEGER NOT NULL DEFAULT 1'); } catch {}
try { db.exec("ALTER TABLE journey_photos ADD COLUMN provider TEXT NOT NULL DEFAULT 'local'"); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_photos ADD COLUMN asset_id TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_photos ADD COLUMN owner_id INTEGER REFERENCES users(id)'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
try { db.exec('ALTER TABLE journey_photos ADD COLUMN shared INTEGER NOT NULL DEFAULT 1'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
// file_path was NOT NULL — recreate table to make it nullable
const hasProvider = db.prepare("SELECT 1 FROM pragma_table_info('journey_photos') WHERE name = 'provider'").get();
if (hasProvider) {
@@ -1481,16 +1488,16 @@ function runMigrations(db: Database.Database): void {
ALTER TABLE journey_photos_new RENAME TO journey_photos;
CREATE INDEX idx_journey_photos_entry ON journey_photos(entry_id);
`);
} catch {}
} catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
}
},
// Migration 89: Journey cover image
() => {
try { db.exec('ALTER TABLE journeys ADD COLUMN cover_image TEXT'); } catch {}
try { db.exec('ALTER TABLE journeys ADD COLUMN cover_image TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
},
// Migration 90: Pros/Cons for journey entries
() => {
try { db.exec('ALTER TABLE journey_entries ADD COLUMN pros_cons TEXT'); } catch {}
try { db.exec('ALTER TABLE journey_entries ADD COLUMN pros_cons TEXT'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
},
// Migration 91: Journey share tokens
() => {
@@ -1512,7 +1519,7 @@ function runMigrations(db: Database.Database): void {
},
// Migration: Vacay week_start setting (0=Sunday, 1=Monday default)
() => {
try { db.exec("ALTER TABLE vacay_plans ADD COLUMN week_start INTEGER NOT NULL DEFAULT 1"); } catch {}
try { db.exec("ALTER TABLE vacay_plans ADD COLUMN week_start INTEGER NOT NULL DEFAULT 1"); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
},
// Migration: Unified Photo Provider Abstraction Layer (#584)
// Central trek_photos registry; trip_photos + journey_photos reference via photo_id
@@ -1655,7 +1662,7 @@ function runMigrations(db: Database.Database): void {
},
// Migration 99: hide_skeletons per-user setting on journey_contributors
() => {
try { db.exec('ALTER TABLE journey_contributors ADD COLUMN hide_skeletons INTEGER NOT NULL DEFAULT 0'); } catch {}
try { db.exec('ALTER TABLE journey_contributors ADD COLUMN hide_skeletons INTEGER NOT NULL DEFAULT 0'); } catch (err) { console.warn('[migrations] Non-fatal migration step failed:', err); }
},
// Migration 100: Idempotency keys for offline mutation replay
() => {
@@ -1895,7 +1902,10 @@ function runMigrations(db: Database.Database): void {
const names = new Set(cols.map((c) => c.name));
if (names.has('start_day_id')) db.exec('CREATE INDEX IF NOT EXISTS idx_day_accommodations_start_day_id ON day_accommodations(start_day_id)');
if (names.has('end_day_id')) db.exec('CREATE INDEX IF NOT EXISTS idx_day_accommodations_end_day_id ON day_accommodations(end_day_id)');
} catch { /* table may not exist on very old installs */ }
} catch (err) {
// Non-fatal: day_accommodations may not exist on very old installs.
console.warn('[migrations] Non-fatal migration step failed:', err);
}
try {
// notifications schema has varied; probe before indexing.
const cols = db.prepare("PRAGMA table_info('notifications')").all() as Array<{ name: string }>;
@@ -1903,7 +1913,10 @@ function runMigrations(db: Database.Database): void {
if (names.has('target') && names.has('scope')) {
db.exec('CREATE INDEX IF NOT EXISTS idx_notifications_target_scope ON notifications(target, scope)');
}
} catch { /* notifications table may not exist on very old installs */ }
} catch (err) {
// Non-fatal: notifications table may not exist on very old installs.
console.warn('[migrations] Non-fatal migration step failed:', err);
}
},
// Migration: widen idempotency_keys primary key to (key, user_id,
// method, path). The middleware lookup was widened in the same audit
@@ -0,0 +1,194 @@
/**
* Migration hygiene guardrails.
*
* These tests scan the migration source statically and fail when a NEW
* destructive operation (DROP TABLE / DROP COLUMN / TRUNCATE / DELETE FROM /
* ALTER ... DROP) is introduced, or when an empty/silent `catch` block creeps
* back into the migration runner.
*
* Migrations 1..N are append-only and immutable once shipped (the live schema
* has already applied them; rewriting an applied migration is a breaking
* change). The destructive statements that already exist were each reviewed
* and are legitimate — almost all are the standard SQLite "table rebuild"
* pattern (create *_new, copy rows, DROP old, RENAME), plus a handful of
* deliberate, data-preserving cleanups. They are recorded in
* ALLOWED_DESTRUCTIVE below with the reason. Anything not on that list is
* treated as a regression.
*/
import { describe, it, expect } from 'vitest';
import { readFileSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
import { dirname, resolve } from 'node:path';
import { createTestDb } from '../../helpers/test-db';
const here = dirname(fileURLToPath(import.meta.url));
const MIGRATIONS_PATH = resolve(here, '../../../src/db/migrations.ts');
const migrationsSource = readFileSync(MIGRATIONS_PATH, 'utf8');
/**
* Strip line and block comments so commented-out SQL (or prose mentioning
* "DROP TABLE") is never flagged. String/template contents are preserved —
* that is exactly where the real SQL lives.
*/
function stripComments(src: string): string {
return src
.replace(/\/\*[\s\S]*?\*\//g, ' ')
.replace(/(^|[^:])\/\/[^\n]*/g, '$1');
}
const scannableSource = stripComments(migrationsSource);
interface DestructiveHit {
/** Normalised signature used as the allowlist key, e.g. "DROP TABLE budget_items". */
signature: string;
/** The raw matched fragment, kept for diagnostics. */
fragment: string;
}
/**
* Detects destructive DDL/DML. For each match we build a normalised signature
* of "<OPERATION> <TARGET>" so cosmetic whitespace/quoting changes don't churn
* the allowlist, while a genuinely new target (or operation) shows up as a new
* signature.
*/
function findDestructiveStatements(src: string): DestructiveHit[] {
const hits: DestructiveHit[] = [];
const norm = (s: string) => s.replace(/[`"'\[\]]/g, '').replace(/\s+/g, ' ').trim();
// DROP TABLE [IF EXISTS] <name>
for (const m of src.matchAll(/DROP\s+TABLE\s+(IF\s+EXISTS\s+)?[`"'\[]?([A-Za-z_][\w]*)/gi)) {
hits.push({ signature: `DROP TABLE ${m[2]}`, fragment: norm(m[0]) });
}
// ALTER TABLE <t> DROP COLUMN <c> (and bare ALTER ... DROP <c>)
for (const m of src.matchAll(/ALTER\s+TABLE\s+[`"'\[]?([A-Za-z_][\w]*)[`"'\]]?\s+DROP\s+(COLUMN\s+)?[`"'\[]?([A-Za-z_][\w]*)/gi)) {
hits.push({ signature: `ALTER TABLE ${m[1]} DROP COLUMN ${m[3]}`, fragment: norm(m[0]) });
}
// TRUNCATE <t> (not valid SQLite, but guard anyway)
for (const m of src.matchAll(/TRUNCATE\s+(TABLE\s+)?[`"'\[]?([A-Za-z_][\w]*)/gi)) {
hits.push({ signature: `TRUNCATE ${m[2]}`, fragment: norm(m[0]) });
}
// DELETE FROM <t>
for (const m of src.matchAll(/DELETE\s+FROM\s+[`"'\[]?([A-Za-z_][\w]*)/gi)) {
hits.push({ signature: `DELETE FROM ${m[1]}`, fragment: norm(m[0]) });
}
return hits;
}
/**
* Allowlist of destructive statements already present and reviewed as
* legitimate. Keyed by normalised signature. NEVER add to this without a
* code-review-level justification — that is the whole point of the guard.
*
* Rebuild = standard SQLite 12-step ALTER emulation: CREATE <t>_new,
* INSERT ... SELECT to copy rows, DROP old <t>, ALTER ... RENAME <t>_new TO <t>.
* Rows are preserved across the rebuild.
*/
const ALLOWED_DESTRUCTIVE: Record<string, string> = {
// ── table rebuilds (data preserved) ──────────────────────────────────────
'DROP TABLE budget_items':
'Migration 12: rebuild to drop a stale NOT NULL DEFAULT on persons/days. Rows copied first.',
'DROP TABLE oauth_clients':
'Make oauth_clients.user_id nullable for anonymous DCR clients. Rebuild, rows copied.',
'DROP TABLE idempotency_keys':
'Widen PK to (key,user_id,method,path). Rebuild, rows copied (old PK is a subset).',
'DROP TABLE day_accommodations':
'Make place_id nullable + ON DELETE SET NULL. Rebuild, rows copied.',
'DROP TABLE schema_version':
'Add surrogate id PK to schema_version. Rebuild, version row copied.',
// ── photo/journey table rebuilds (data preserved) ────────────────────────
'DROP TABLE trip_photos':
'trip_photos normalisation + later photo_id FK refactor. Rebuilds, rows copied.',
'DROP TABLE trip_album_links':
'Normalise trip_album_links to provider+album_id schema. Rebuild, rows copied.',
'DROP TABLE journey_photos':
'Journey photo provider support + photo_id FK refactor. Rebuilds, rows copied.',
'DROP TABLE journey_photos_old':
'Migration 121 gallery refactor: drops the temporary *_old backup after backfill.',
'DROP TABLE journey_location_trail':
'Migration 87 journey rebuild: old data SELECTed into memory and re-inserted into new schema.',
'DROP TABLE journey_entries':
'Migration 87 journey rebuild: old data SELECTed into memory and re-inserted into new schema.',
'DROP TABLE journey_checkins':
'Migration 87 journey rebuild: old data SELECTed into memory and re-inserted into new schema.',
'DROP TABLE journey_members':
'Migration 87 journey rebuild: old data SELECTed into memory and re-inserted into new schema.',
'DROP TABLE journey_trips':
'Migration 87 journey rebuild: old data SELECTed into memory and re-inserted into new schema.',
'DROP TABLE journeys':
'Migration 87 journey rebuild: old data SELECTed into memory and re-inserted into new schema.',
// ── template/cache scaffolding drops (no user content lost) ──────────────
'DROP TABLE packing_template_items':
'IF EXISTS drop to recreate the template-items table with a category_id FK. Template scaffolding.',
'DROP TABLE notification_preferences':
'IF EXISTS drop AFTER migration 71 copied the data into notification_channel_preferences.',
// ── guarded column drop ──────────────────────────────────────────────────
'ALTER TABLE photo_providers DROP COLUMN config':
'Drop generated-only config column; guarded by a PRAGMA table_info check that it exists.',
// ── targeted, bounded DELETEs ────────────────────────────────────────────
'DELETE FROM oauth_tokens':
'SEC-H6: DELETE ... WHERE audience IS NULL — purge pre-audience-binding tokens that the MCP server now rejects.',
'DELETE FROM journey_entries':
"Migration 121: DELETE ... WHERE title IN ('Gallery','[Trip Photos]') — remove synthetic wrapper entries replaced by the gallery model.",
'DELETE FROM place_regions':
'Atlas enclave fix: DELETE ... WHERE place_id IN (places inside specific enclave boxes) — invalidate stale region cache; re-resolved on next request.',
};
describe('migration hygiene — destructive operation guard', () => {
it('introduces no destructive migration statement outside the reviewed allowlist', () => {
const hits = findDestructiveStatements(scannableSource);
const offenders = hits.filter((h) => !(h.signature in ALLOWED_DESTRUCTIVE));
if (offenders.length > 0) {
const detail = offenders
.map((o) => `${o.signature} (matched: "${o.fragment}")`)
.join('\n');
throw new Error(
`Found ${offenders.length} destructive migration statement(s) that are not on the ` +
`reviewed allowlist in tests/unit/db/migration-hygiene.test.ts.\n` +
`Migrations are append-only and destructive DDL/DML risks data loss on upgrade.\n` +
`If the statement is genuinely safe (e.g. a SQLite table rebuild that copies rows ` +
`first, or a tightly-bounded cache/cleanup DELETE), add its signature to ` +
`ALLOWED_DESTRUCTIVE with a justification.\n\nOffending statement(s):\n${detail}`,
);
}
expect(offenders).toEqual([]);
});
it('every allowlist entry still corresponds to a real statement (no dead allowlist rows)', () => {
const present = new Set(findDestructiveStatements(scannableSource).map((h) => h.signature));
const dead = Object.keys(ALLOWED_DESTRUCTIVE).filter((sig) => !present.has(sig));
expect(dead, `Allowlist entries no longer found in migrations.ts: ${dead.join(', ')}`).toEqual([]);
});
});
describe('migration hygiene — no silently swallowed errors', () => {
it('contains no empty catch block (catch must at least log)', () => {
// Matches `catch {}` and `catch (e) {}` where the body is only whitespace.
const emptyCatch = scannableSource.match(/catch\s*(\([^)]*\))?\s*\{\s*\}/g) ?? [];
expect(
emptyCatch,
`migrations.ts must not swallow errors silently. Give each catch a log line ` +
`(e.g. console.warn('[migrations] ...', err)). Found: ${emptyCatch.length}`,
).toEqual([]);
});
});
describe('migration hygiene — full chain smoke', () => {
it('migrates a fresh in-memory database from zero to the latest version', () => {
// createTestDb() runs createTables() + the entire runMigrations() chain.
// This proves the logging edits in the previously-empty catch blocks do
// not change control flow / break the migration runner.
const db = createTestDb();
try {
const row = db.prepare('SELECT version FROM schema_version').get() as { version: number };
expect(row.version).toBeGreaterThan(0);
} finally {
db.close();
}
});
});