diff --git a/server/scripts/migrate-encryption.ts b/server/scripts/migrate-encryption.ts index ae8bfdf03..d3e113614 100644 --- a/server/scripts/migrate-encryption.ts +++ b/server/scripts/migrate-encryption.ts @@ -273,8 +273,24 @@ async function main() { } db.transaction(() => { - // --- app_settings: oidc_client_secret, smtp_pass, admin_webhook_url, admin_ntfy_token --- - for (const key of ['oidc_client_secret', 'smtp_pass', 'admin_webhook_url', 'admin_ntfy_token']) { + // --- app_settings: oidc_client_secret, smtp_pass, admin_webhook_url, admin_ntfy_token, + // plus the instance-wide provider keys (#1939) --- + // + // The last two mirror INSTANCE_API_KEY_NAMES in + // src/nest/settings/instance-api-keys.ts. Spelled out rather than imported, + // because that module pulls in DatabaseService and this script deliberately + // stays independent of src/ (see the crypto note above). Miss one and a + // rotation leaves it encrypted under the old key, which reads back as "no + // key" and silently drops the install to OpenStreetMap, so the copy is + // pinned by tests/unit/db/migrate-encryption-parity.test.ts. + for (const key of [ + 'oidc_client_secret', + 'smtp_pass', + 'admin_webhook_url', + 'admin_ntfy_token', + 'maps_api_key', + 'unsplash_api_key', + ]) { const row = db.prepare('SELECT value FROM app_settings WHERE key = ?').get(key) as { value: string } | undefined; if (!row?.value) continue; const newVal = migrateApiKeyValue(row.value, `app_settings.${key}`); @@ -284,7 +300,10 @@ async function main() { } // --- users: api key columns + synology credentials --- - const apiKeyColumns = ['maps_api_key', 'openweather_api_key', 'immich_api_key', 'synology_password', 'synology_sid', 'synology_did']; + // unsplash_api_key was missing here for as long as the column has existed. It + // matters now that the resolver reads it as the per-user fallback (#1939): left + // out of a rotation it stays encrypted under the old key and reads back as unset. + const apiKeyColumns = ['maps_api_key', 'unsplash_api_key', 'openweather_api_key', 'immich_api_key', 'synology_password', 'synology_sid', 'synology_did']; const users = db.prepare('SELECT id FROM users').all() as { id: number }[]; for (const user of users) { diff --git a/server/tests/unit/db/migrate-encryption-parity.test.ts b/server/tests/unit/db/migrate-encryption-parity.test.ts new file mode 100644 index 000000000..7b0a9584a --- /dev/null +++ b/server/tests/unit/db/migrate-encryption-parity.test.ts @@ -0,0 +1,49 @@ +/** + * Parity guard for scripts/migrate-encryption.ts. + * + * The rotation script re-encrypts at-rest secrets from one ENCRYPTION_KEY to the + * next, and it walks a hardcoded list of app_settings keys. It cannot import the + * canonical list from src/, because it deliberately runs without config.ts and + * without the Nest container, so the list is a copy. + * + * A missing name does not fail loudly: the value stays encrypted under the old + * key, decrypt_api_key returns null on the next read, and the instance quietly + * behaves as if no key were configured. This pins the copy instead. + */ +import { describe, it, expect } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import { INSTANCE_API_KEY_NAMES } from '../../../src/nest/settings/instance-api-keys'; + +const script = fs.readFileSync( + path.join(__dirname, '..', '..', '..', 'scripts', 'migrate-encryption.ts'), + 'utf8', +); + +// The app_settings loop only, so a name that happens to appear elsewhere in the +// script (a users column of the same name, a comment) cannot satisfy the test. +const appSettingsLoop = (() => { + const start = script.indexOf("'oidc_client_secret'"); + expect(start).toBeGreaterThan(-1); + const end = script.indexOf('--- users:', start); + expect(end).toBeGreaterThan(start); + return script.slice(start, end); +})(); + +describe('migrate-encryption.ts app_settings parity', () => { + it('ROTPAR-001: rotates every instance-wide API key', () => { + for (const name of INSTANCE_API_KEY_NAMES) { + expect(appSettingsLoop).toContain(`'${name}'`); + } + }); + + it('ROTPAR-002: still rotates the app_settings secrets that predate them', () => { + for (const name of ['oidc_client_secret', 'smtp_pass', 'admin_webhook_url', 'admin_ntfy_token']) { + expect(appSettingsLoop).toContain(`'${name}'`); + } + }); + + it('ROTPAR-003: the canonical list is not empty, so ROTPAR-001 cannot pass vacuously', () => { + expect(INSTANCE_API_KEY_NAMES.length).toBeGreaterThan(0); + }); +}); diff --git a/wiki/Encryption-Key-Rotation.md b/wiki/Encryption-Key-Rotation.md index 867e218cf..847f459d7 100644 --- a/wiki/Encryption-Key-Rotation.md +++ b/wiki/Encryption-Key-Rotation.md @@ -4,7 +4,8 @@ TREK encrypts sensitive settings at rest using AES-256-GCM. The following values are stored encrypted in the database: -- Google Maps API key (per user) +- Google Maps API key (instance-wide, in `app_settings`; the per-user column is still read as a fallback) +- Unsplash access key (instance-wide, in `app_settings`; the per-user column is still read as a fallback) - Mapbox access token (per user) - OpenWeather API key (per user) - Immich API key (per user) @@ -59,8 +60,8 @@ The script: 2. Asks for confirmation before making any changes. 3. Creates a timestamped backup of the database (e.g. `travel.db.backup-1713484800000`) before modifying anything. 4. Re-encrypts all stored secrets across all tables: - - `app_settings`: `oidc_client_secret`, `smtp_pass`, `admin_webhook_url`, `admin_ntfy_token` - - `users` (per user): `maps_api_key`, `openweather_api_key`, `immich_api_key`, `synology_password`, `synology_sid`, `synology_did`, `mfa_secret` + - `app_settings`: `oidc_client_secret`, `smtp_pass`, `admin_webhook_url`, `admin_ntfy_token`, `maps_api_key`, `unsplash_api_key` + - `users` (per user): `maps_api_key`, `unsplash_api_key`, `openweather_api_key`, `immich_api_key`, `synology_password`, `synology_sid`, `synology_did`, `mfa_secret` - `settings` (per user): `webhook_url`, `ntfy_token`, `mapbox_access_token` - `trip_album_links`: `passphrase` - `trek_photos`: `passphrase`