From 5a0124e7cb6f105c4342b2a8689e298479cf0ba8 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 31 May 2026 15:12:02 +0200 Subject: [PATCH] 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. --- server/src/db/migrations.ts | 73 ++++--- .../tests/unit/db/migration-hygiene.test.ts | 194 ++++++++++++++++++ 2 files changed, 237 insertions(+), 30 deletions(-) create mode 100644 server/tests/unit/db/migration-hygiene.test.ts diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index a05cd371..e0623524 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -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 diff --git a/server/tests/unit/db/migration-hygiene.test.ts b/server/tests/unit/db/migration-hygiene.test.ts new file mode 100644 index 00000000..f4001613 --- /dev/null +++ b/server/tests/unit/db/migration-hygiene.test.ts @@ -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 " " 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] + 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 DROP COLUMN (and bare ALTER ... DROP ) + 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 (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 + 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 _new, + * INSERT ... SELECT to copy rows, DROP old , ALTER ... RENAME _new TO . + * Rows are preserved across the rebuild. + */ +const ALLOWED_DESTRUCTIVE: Record = { + // ── 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(); + } + }); +});