rbrooks/Quest-Board

Watch

Fork 0

[Backend][DR] Restoring a database onto a host with a different SECRET_KEY destroys every encrypted setting, and raises an unhandled InvalidTag #430

New issue

Closed

opened 2026-08-28 21:20:33 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-08-28 21:20:33 +00:00

Collaborator

Severity: HIGH (disaster recovery). Hit for real while restoring production data onto dev to rehearse the v4.0.0 migrations.

The defect

Sensitive settings are encrypted at rest with a key derived from SECRET_KEY (crypto._derive_key). SECRET_KEY lives in .env, which is not in the database and therefore not in any database backup.

So restoring a database onto a host whose SECRET_KEY differs — a rebuilt host, a new machine, a DR event, or a dev host taking a copy of production — leaves every _ENCRYPTED_KEYS row permanently unreadable. Whisper endpoint, LLM endpoint and key, Discord bot token, SMTP password: all present, all garbage.

Observed exactly that:

llm:     RAISED InvalidTag
whisper: RAISED InvalidTag

Two things make it worse than it needs to be

1. It raises where every caller expects None.

get_llm_config's own docstring commits to returning None when the config is unusable, and settings_service's module docstring lays out four policies callers use for exactly that case. A decryption failure bypasses all of it: decrypt_setting raises InvalidTag out of get_setting, and nothing catches it. Every LLM-touching endpoint and task fails with an opaque cryptography exception instead of the well-defined "not configured" path that already exists.

Worth noting the ordering hazard too: an undecryptable row is worse than an absent one, because absent degrades gracefully and undecryptable does not.

2. Nothing warns you, before or after.

docs/OPERATIONS.md mentions SECRET_KEY exactly once, in an env-var table. Its restore and backup sections say nothing about encrypted settings being tied to it. Following the documented restore procedure onto a fresh host produces a system that looks restored and is not — and the failure surfaces later, as InvalidTag, somewhere unrelated to the restore.

Proposed fix

Fail gracefully. Catch the decryption failure in get_setting, log once and clearly ("setting llm_config could not be decrypted — it was encrypted with a different SECRET_KEY; re-enter it in Admin"), and return None so the existing not-configured policies take over.
Detect it at startup, not on first use. A check that every _ENCRYPTED_KEYS row decrypts, reported loudly, so an operator learns during the restore rather than when a session fails to summarise.
Surface it in Admin. A settings page that shows "could not be decrypted — re-enter" is far better than one that renders an empty field indistinguishable from never-configured.
Document it. SECRET_KEY is part of the backup set. A database dump alone is not a recoverable system, and the restore procedure should say so and list what must be re-entered if the key is lost.

Acceptance criteria

A row that fails to decrypt returns None from get_setting with a clear one-time log line, never an unhandled InvalidTag
A startup check reports any _ENCRYPTED_KEYS row that cannot be decrypted
Admin distinguishes "not configured" from "configured but undecryptable"
docs/OPERATIONS.md states that SECRET_KEY must be backed up alongside the database, and documents recovery when it is lost
A test restores a settings row encrypted under one key and reads it under another, asserting graceful degradation rather than a raise

#428 — the bot API key is stored in plaintext, so it is the only sensitive setting that survives this. That is not a mitigation; it is a separate bug that happens to mask this one.
#429 — backups written by a client the server cannot restore. Same investigation: the backup exists, the restore does not work.

**Severity: HIGH (disaster recovery).** Hit for real while restoring production data onto dev to rehearse the v4.0.0 migrations. ## The defect Sensitive settings are encrypted at rest with a key derived from `SECRET_KEY` (`crypto._derive_key`). `SECRET_KEY` lives in `.env`, which is **not** in the database and therefore **not** in any database backup. So restoring a database onto a host whose `SECRET_KEY` differs — a rebuilt host, a new machine, a DR event, or a dev host taking a copy of production — leaves every `_ENCRYPTED_KEYS` row permanently unreadable. Whisper endpoint, LLM endpoint and key, Discord bot token, SMTP password: all present, all garbage. Observed exactly that: ``` llm: RAISED InvalidTag whisper: RAISED InvalidTag ``` ## Two things make it worse than it needs to be **1. It raises where every caller expects `None`.** `get_llm_config`'s own docstring commits to returning `None` when the config is unusable, and `settings_service`'s module docstring lays out four policies callers use for exactly that case. A decryption failure bypasses all of it: `decrypt_setting` raises `InvalidTag` out of `get_setting`, and nothing catches it. Every LLM-touching endpoint and task fails with an opaque cryptography exception instead of the well-defined "not configured" path that already exists. Worth noting the ordering hazard too: an *undecryptable* row is worse than an *absent* one, because absent degrades gracefully and undecryptable does not. **2. Nothing warns you, before or after.** `docs/OPERATIONS.md` mentions `SECRET_KEY` exactly once, in an env-var table. Its restore and backup sections say nothing about encrypted settings being tied to it. Following the documented restore procedure onto a fresh host produces a system that looks restored and is not — and the failure surfaces later, as `InvalidTag`, somewhere unrelated to the restore. ## Proposed fix - **Fail gracefully.** Catch the decryption failure in `get_setting`, log once and clearly ("setting `llm_config` could not be decrypted — it was encrypted with a different SECRET_KEY; re-enter it in Admin"), and return `None` so the existing not-configured policies take over. - **Detect it at startup**, not on first use. A check that every `_ENCRYPTED_KEYS` row decrypts, reported loudly, so an operator learns during the restore rather than when a session fails to summarise. - **Surface it in Admin.** A settings page that shows "could not be decrypted — re-enter" is far better than one that renders an empty field indistinguishable from never-configured. - **Document it.** `SECRET_KEY` is part of the backup set. A database dump alone is not a recoverable system, and the restore procedure should say so and list what must be re-entered if the key is lost. ## Acceptance criteria - [ ] A row that fails to decrypt returns `None` from `get_setting` with a clear one-time log line, never an unhandled `InvalidTag` - [ ] A startup check reports any `_ENCRYPTED_KEYS` row that cannot be decrypted - [ ] Admin distinguishes "not configured" from "configured but undecryptable" - [ ] `docs/OPERATIONS.md` states that `SECRET_KEY` must be backed up alongside the database, and documents recovery when it is lost - [ ] A test restores a settings row encrypted under one key and reads it under another, asserting graceful degradation rather than a raise ## Related - #428 — the bot API key is stored in plaintext, so it is the *only* sensitive setting that survives this. That is not a mitigation; it is a separate bug that happens to mask this one. - #429 — backups written by a client the server cannot restore. Same investigation: the backup exists, the restore does not work.

claude-bot added this to the v4.0.1 - Retention Safety & Secrets at Rest milestone

2026-08-28 21:20:45 +00:00

claude-bot added the

backend

bug

labels

2026-08-28 21:21:08 +00:00

rbrooks referenced this issue from a commit

2026-08-29 00:55:36 +00:00

fix(backend): make secrets at rest readable, or say so (#428, #430)

claude-bot commented

2026-08-29 01:08:57 +00:00

Author

Collaborator

Fixed in 9517977 on fix/v4.0.1-retention-safety, sharing one pass with #428.

What changed

decrypt_setting now raises UndecryptableSettingError instead of letting InvalidTag escape, and get_setting catches it and returns None — so the four "not configured" policies in the module docstring actually apply, which is what the issue asked for.

The catch in crypto is deliberately broad. A wrong key raises InvalidTag, but a truncated or hand-edited row raises from base64, from the slicing, or from json — and every one of them means the same thing to a caller: this value is not readable. Catching only InvalidTag would have left three other routes to the same opaque failure.

Worth stating plainly, and it is in the docstring: this is a demotion, not a repair. An undecryptable row is worse than an absent one, because the operator believes the setting is configured. Returning None only stops it taking the process down somewhere unrelated to the restore — hence the log line, the startup report, and the Admin surface.

Acceptance criteria

A row that fails to decrypt returns None with a clear one-time log line, never an unhandled InvalidTag — deduped per key, because settings are read on nearly every request and an un-deduped line would bury the startup report it points at
A startup check reports any _ENCRYPTED_KEYS row that cannot be decrypted — in the lifespan, and deliberately non-fatal: an operator who has just restored onto a fresh host needs the app up to re-enter the settings the report is telling them about, and a stricter check would lock them out of the only UI that can fix it
Admin distinguishes "not configured" from "configured but undecryptable" — GET /api/admin/settings/secrets-health, read-only (reencrypt=False) so hitting it cannot mutate data
docs/OPERATIONS.md states that SECRET_KEY must be backed up alongside the database, and documents recovery when it is lost — a callout at the head of the restore procedure, plus the env-var table, plus what the startup line looks like when this has happened
A test restores a settings row encrypted under one key and reads it under another, asserting graceful degradation rather than a raise

On the tests

There were no tests for encryption at all before this, which is most of the explanation for both this and #428: nothing asserted that an _ENCRYPTED_KEYS row is actually stored encrypted, and nothing ever read a row back under a different key.

The mismatch is driven by swapping SECRET_KEY in the environment between the write and the read — faithful, since _derive_key resolves it per call, so that genuinely is "this database came from another host".

Verified by reverting the catch: test_get_setting_returns_none_for_an_undecryptable_row and test_get_llm_config_degrades_rather_than_raising both fail there with the exception escaping, exactly as production did.

1,407 backend tests pass.

Fixed in `9517977` on `fix/v4.0.1-retention-safety`, sharing one pass with #428. ## What changed `decrypt_setting` now raises `UndecryptableSettingError` instead of letting `InvalidTag` escape, and `get_setting` catches it and returns `None` — so the four "not configured" policies in the module docstring actually apply, which is what the issue asked for. The catch in `crypto` is deliberately **broad**. A wrong key raises `InvalidTag`, but a truncated or hand-edited row raises from base64, from the slicing, or from json — and every one of them means the same thing to a caller: this value is not readable. Catching only `InvalidTag` would have left three other routes to the same opaque failure. Worth stating plainly, and it is in the docstring: **this is a demotion, not a repair.** An undecryptable row is worse than an absent one, because the operator believes the setting is configured. Returning `None` only stops it taking the process down somewhere unrelated to the restore — hence the log line, the startup report, and the Admin surface. ## Acceptance criteria - [x] A row that fails to decrypt returns `None` with a clear one-time log line, never an unhandled `InvalidTag` — deduped per key, because settings are read on nearly every request and an un-deduped line would bury the startup report it points at - [x] A startup check reports any `_ENCRYPTED_KEYS` row that cannot be decrypted — in the lifespan, and **deliberately non-fatal**: an operator who has just restored onto a fresh host needs the app up to re-enter the settings the report is telling them about, and a stricter check would lock them out of the only UI that can fix it - [x] Admin distinguishes "not configured" from "configured but undecryptable" — `GET /api/admin/settings/secrets-health`, read-only (`reencrypt=False`) so hitting it cannot mutate data - [x] `docs/OPERATIONS.md` states that `SECRET_KEY` must be backed up alongside the database, and documents recovery when it is lost — a callout at the head of the restore procedure, plus the env-var table, plus what the startup line looks like when this has happened - [x] A test restores a settings row encrypted under one key and reads it under another, asserting graceful degradation rather than a raise ## On the tests There were **no tests for encryption at all** before this, which is most of the explanation for both this and #428: nothing asserted that an `_ENCRYPTED_KEYS` row is actually stored encrypted, and nothing ever read a row back under a different key. The mismatch is driven by swapping `SECRET_KEY` in the environment between the write and the read — faithful, since `_derive_key` resolves it per call, so that genuinely is "this database came from another host". Verified by reverting the catch: `test_get_setting_returns_none_for_an_undecryptable_row` and `test_get_llm_config_degrades_rather_than_raising` both fail there with the exception escaping, exactly as production did. 1,407 backend tests pass.

claude-bot referenced this issue

2026-08-29 01:09:18 +00:00

[Backend][Security] The bot API key is stored in plaintext at rest and has leaked into every backup #428

claude-bot referenced this issue

2026-08-29 05:58:03 +00:00

v4.0.1 — retention safety and secrets at rest #435

claude-bot referenced this issue from a commit

2026-08-29 06:58:09 +00:00

Merge PR #435: v4.0.1 — retention safety and secrets at rest

claude-bot closed this issue

2026-08-29 07:09:33 +00:00

claude-bot referenced this issue