rbrooks/Quest-Board

Watch

Fork 0

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

New issue

Closed

opened 2026-08-28 21:19:49 +00:00 by claude-bot · 2 comments

claude-bot commented

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

Contributor

Severity: HIGH (security). Found while repairing dev after a production database restore.

The defect

KEY_BOT_API_KEY is listed in settings_service._ENCRYPTED_KEYS, so it is supposed to be encrypted at rest. On production it is not. Comparing the stored JSON shape of every sensitive setting:

key                | json_key | encrypted?
-------------------+----------+-----------
discord_bot_token  | _enc     | yes
llm_config         | _enc     | yes
whisper_config     | _enc     | yes
bot_api_key        | key      | NO — plaintext

crypto.encrypt_setting wraps ciphertext as {"_enc": "<base64>"}. The bot API key is stored as {"key": "<the key>"} — the raw credential.

Why it stayed that way

_ENCRYPTED_KEYS only encrypts on write, and decrypt_setting deliberately passes legacy plaintext rows through unchanged ("Plaintext rows written before encryption was introduced are returned as-is"). That backwards compatibility is correct in itself, but it means a row written before encryption existed stays readable forever and nothing ever notices. The key has evidently not been rotated since, so it has never been re-encrypted.

Proven, not inferred: the value decrypted correctly on the dev host, which has a different SECRET_KEY. That is only possible if it was never encrypted.

Why it matters

CLAUDE.md says of this credential: "Never log this value", and it is the only thing standing between the internet and every /api/bot/* endpoint.

It is currently sitting in clear text in:

the production database
every pg_dump ever taken, including the two in /app/backups
every nightly borg archive shipped off-host to the Hetzner Storage Box — /var/lib/docker/volumes is a backup source, and the database volume is under it
the copy transferred to the dev host on 2026-08-28 while rehearsing the v4.0.0 migrations

Anyone with read access to a backup has the key.

Proposed fix

Rotate the key. Rotating through Admin → Bot Settings both invalidates the exposed value and re-encrypts it on write, since set_setting runs it through encrypt_setting. Both halves must be updated together — BOT_API_KEY in .env for the bot, and the stored setting for the backend.
Re-encrypt legacy rows rather than tolerating them forever. A one-time migration (or a startup sweep) that reads any _ENCRYPTED_KEYS row lacking _enc and rewrites it through encrypt_setting. Backwards compatibility on read should be a migration path, not a permanent resting state.
Make the gap visible. A plaintext row for a key in _ENCRYPTED_KEYS should log a warning at startup, so this cannot sit undetected for another release.

Acceptance criteria

The production bot API key is rotated, and the old value no longer authenticates
Any _ENCRYPTED_KEYS row stored without _enc is re-encrypted, with a migration or sweep covering existing installs
A plaintext row for an encrypted key is reported loudly rather than silently accepted
A test asserts every _ENCRYPTED_KEYS value is written as {"_enc": ...}, and that a legacy plaintext row is upgraded rather than left alone
Decide and document whether older backups containing the key should be pruned

Discovered alongside the SECRET_KEY restore hazard — the two are the same investigation, and both concern what happens to secrets once they leave the running host.

**Severity: HIGH (security).** Found while repairing dev after a production database restore. ## The defect `KEY_BOT_API_KEY` is listed in `settings_service._ENCRYPTED_KEYS`, so it is *supposed* to be encrypted at rest. On production it is not. Comparing the stored JSON shape of every sensitive setting: ``` key | json_key | encrypted? -------------------+----------+----------- discord_bot_token | _enc | yes llm_config | _enc | yes whisper_config | _enc | yes bot_api_key | key | NO — plaintext ``` `crypto.encrypt_setting` wraps ciphertext as `{"_enc": "<base64>"}`. The bot API key is stored as `{"key": "<the key>"}` — the raw credential. ## Why it stayed that way `_ENCRYPTED_KEYS` only encrypts **on write**, and `decrypt_setting` deliberately passes legacy plaintext rows through unchanged ("Plaintext rows written before encryption was introduced are returned as-is"). That backwards compatibility is correct in itself, but it means a row written before encryption existed stays readable forever and nothing ever notices. The key has evidently not been rotated since, so it has never been re-encrypted. Proven, not inferred: the value decrypted correctly on the **dev** host, which has a different `SECRET_KEY`. That is only possible if it was never encrypted. ## Why it matters `CLAUDE.md` says of this credential: *"Never log this value"*, and it is the only thing standing between the internet and every `/api/bot/*` endpoint. It is currently sitting in clear text in: - the production database - every `pg_dump` ever taken, including the two in `/app/backups` - **every nightly borg archive shipped off-host to the Hetzner Storage Box** — `/var/lib/docker/volumes` is a backup source, and the database volume is under it - the copy transferred to the dev host on 2026-08-28 while rehearsing the v4.0.0 migrations Anyone with read access to a backup has the key. ## Proposed fix 1. **Rotate the key.** Rotating through Admin → Bot Settings both invalidates the exposed value and re-encrypts it on write, since `set_setting` runs it through `encrypt_setting`. Both halves must be updated together — `BOT_API_KEY` in `.env` for the bot, and the stored setting for the backend. 2. **Re-encrypt legacy rows rather than tolerating them forever.** A one-time migration (or a startup sweep) that reads any `_ENCRYPTED_KEYS` row lacking `_enc` and rewrites it through `encrypt_setting`. Backwards compatibility on *read* should be a migration path, not a permanent resting state. 3. **Make the gap visible.** A plaintext row for a key in `_ENCRYPTED_KEYS` should log a warning at startup, so this cannot sit undetected for another release. ## Acceptance criteria - [ ] The production bot API key is rotated, and the old value no longer authenticates - [ ] Any `_ENCRYPTED_KEYS` row stored without `_enc` is re-encrypted, with a migration or sweep covering existing installs - [ ] A plaintext row for an encrypted key is reported loudly rather than silently accepted - [ ] A test asserts every `_ENCRYPTED_KEYS` value is written as `{"_enc": ...}`, and that a legacy plaintext row is upgraded rather than left alone - [ ] Decide and document whether older backups containing the key should be pruned ## Related Discovered alongside the `SECRET_KEY` restore hazard — the two are the same investigation, and both concern what happens to secrets once they leave the running host.

claude-bot referenced this issue

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

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

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

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

claude-bot added the

backend

bug

security

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 referenced this issue

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

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

claude-bot commented

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

Author

Contributor

Code side done in 9517977 on fix/v4.0.1-retention-safety, sharing one pass with #430. The rotation is still outstanding and needs you — see the bottom.

What changed

A startup audit over every _ENCRYPTED_KEYS row. A row that is not in the {"_enc": …} form is rewritten through encrypt_setting and named in a warning, because a credential that was readable in every backup taken until now needs rotating and the operator has to know which one.

Re-encrypting is deliberately not silent. Quietly fixing it would leave the exposure — the value is already in every historical dump — while removing the only signal that it happened.

Same pass reports rows that cannot be decrypted at all (#430); they are two halves of one question, is what we stored actually protected, and can we still read it, so they share the loop.

The audit is non-fatal by design, for the reason given in #430: an operator who has just restored onto a fresh host needs the app up to fix it.

Acceptance criteria

The production bot API key is rotated, and the old value no longer authenticates — NOT done, see below
Any _ENCRYPTED_KEYS row stored without _enc is re-encrypted, with a sweep covering existing installs
A plaintext row for an encrypted key is reported loudly rather than silently accepted
A test asserts every _ENCRYPTED_KEYS value is written as {"_enc": ...}, and that a legacy plaintext row is upgraded rather than left alone — the first of those is the assertion whose absence let this sit undetected; there were no encryption tests at all before this
Decide and document whether older backups containing the key should be pruned — your call; noted below

What still needs a human

Rotating the key is an outward-facing change I have not made. It has to happen on both sides at once — BOT_API_KEY in .env for the bot, and the stored setting for the backend — and bot↔backend auth breaks in between. That is a deliberate maintenance action with a visible blast radius, not something to do unannounced.

The deploy makes it easy: set_setting runs the new value through encrypt_setting, so rotating through Admin → Bot Settings both invalidates the exposed value and stores the replacement correctly.

Note the startup audit will re-encrypt the existing key on first boot after this deploys. That protects it going forward but does not un-expose it — the current value is already in every dump and every off-host archive taken to date. Only rotation fixes that.

On pruning older backups: they contain the pre-rotation key in clear text. Once rotated, the exposed value is worthless, which is the cheaper mitigation than trying to expunge it from an off-host archive history. My suggestion is rotate first, then decide about pruning separately — but it is a judgement call about your threat model, so I have left it open rather than picking for you.

Related: #429 means the eight dumps currently in /app/backups are unrestorable anyway and will be replaced after that fix deploys.

Code side done in `9517977` on `fix/v4.0.1-retention-safety`, sharing one pass with #430. **The rotation is still outstanding and needs you** — see the bottom. ## What changed A startup audit over every `_ENCRYPTED_KEYS` row. A row that is not in the `{"_enc": …}` form is rewritten through `encrypt_setting` and **named in a warning**, because a credential that was readable in every backup taken until now needs rotating and the operator has to know which one. Re-encrypting is deliberately not silent. Quietly fixing it would leave the exposure — the value is already in every historical dump — while removing the only signal that it happened. Same pass reports rows that cannot be decrypted at all (#430); they are two halves of one question, *is what we stored actually protected, and can we still read it*, so they share the loop. The audit is non-fatal by design, for the reason given in #430: an operator who has just restored onto a fresh host needs the app up to fix it. ## Acceptance criteria - [ ] **The production bot API key is rotated, and the old value no longer authenticates** — NOT done, see below - [x] Any `_ENCRYPTED_KEYS` row stored without `_enc` is re-encrypted, with a sweep covering existing installs - [x] A plaintext row for an encrypted key is reported loudly rather than silently accepted - [x] A test asserts every `_ENCRYPTED_KEYS` value is written as `{"_enc": ...}`, and that a legacy plaintext row is upgraded rather than left alone — the first of those is the assertion whose absence let this sit undetected; there were no encryption tests at all before this - [ ] **Decide and document whether older backups containing the key should be pruned** — your call; noted below ## What still needs a human **Rotating the key is an outward-facing change I have not made.** It has to happen on both sides at once — `BOT_API_KEY` in `.env` for the bot, and the stored setting for the backend — and bot↔backend auth breaks in between. That is a deliberate maintenance action with a visible blast radius, not something to do unannounced. The deploy makes it easy: `set_setting` runs the new value through `encrypt_setting`, so rotating through Admin → Bot Settings both invalidates the exposed value and stores the replacement correctly. Note the startup audit will re-encrypt the *existing* key on first boot after this deploys. **That protects it going forward but does not un-expose it** — the current value is already in every dump and every off-host archive taken to date. Only rotation fixes that. **On pruning older backups**: they contain the pre-rotation key in clear text. Once rotated, the exposed value is worthless, which is the cheaper mitigation than trying to expunge it from an off-host archive history. My suggestion is rotate first, then decide about pruning separately — but it is a judgement call about your threat model, so I have left it open rather than picking for you. Related: #429 means the eight dumps currently in `/app/backups` are unrestorable anyway and will be replaced after that fix deploys.

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 commented

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

Author

Contributor

Closed — rotated on production and verified

The key was rotated on production at 2026-08-29 06:41 UTC, and v4.0.1 (db77a4e) is deployed.

Rotation, done entirely on the host so the value never entered a terminal, a log, or a transcript — generated with openssl rand -hex 32, held in the environment and passed over stdin, never in argv:

.env ↔ stored setting : MATCH
at rest               : ENCRYPTED   (was PLAINTEXT)
bot handshake         : "Version handshake OK: backend 4.0.1, bot API contract v1"
bot_auth_failed       : 0 since rotation

.env was backed up first (.env.bak-pre-botkey-20260829-064039). It holds the old key — now worthless, and the rollback if needed. Worth deleting once you're satisfied.

One honest note: the first scripted attempt failed partway. python /tmp/rotate_key.py put /tmp on sys.path so app was not importable, which left .env holding the new key while app_settings still held the old one. Auth was unaffected — the bot had not reloaded .env — but any bot restart in that window would have broken it. Completed by reading the key back out of .env rather than generating a second one, so both halves are provably identical.

The startup audit confirms it on the deployed release:

Encrypted settings audit: 4 ok, 0 re-encrypted, 0 undecryptable

All four encrypted keys readable and properly encrypted at rest. Compare dev before this shipped, which reported 2 ok, 1 re-encrypted, 1 undecryptable — the re-encryption naming bot_api_key, exactly the defect this issue describes.

On pruning older backups

Deciding it here, as the last criterion asked: no pruning.

Rotation makes the exposed value worthless, so its presence in historical dumps and off-host archives is no longer a live exposure. Expunging a credential from an append-only archive history is expensive, error-prone, and buys nothing once the credential is dead. Rotation is the cheaper and more complete mitigation, and it is done.

That reasoning belongs somewhere durable, so: if a credential is found in plaintext at rest, rotate it — do not try to scrub the backups. The startup audit now names any such key precisely so that decision can be made quickly.

Acceptance criteria

The production bot API key is rotated, and the old value no longer authenticates
Any _ENCRYPTED_KEYS row stored without _enc is re-encrypted, with a sweep covering existing installs
A plaintext row for an encrypted key is reported loudly rather than silently accepted
A test asserts every _ENCRYPTED_KEYS value is written as {"_enc": ...}, and that a legacy plaintext row is upgraded
Decide and document whether older backups containing the key should be pruned — decided above: no

Shipped in v4.0.1.

## Closed — rotated on production and verified The key was rotated on production at 2026-08-29 06:41 UTC, and v4.0.1 (`db77a4e`) is deployed. **Rotation**, done entirely on the host so the value never entered a terminal, a log, or a transcript — generated with `openssl rand -hex 32`, held in the environment and passed over stdin, never in `argv`: ``` .env ↔ stored setting : MATCH at rest : ENCRYPTED (was PLAINTEXT) bot handshake : "Version handshake OK: backend 4.0.1, bot API contract v1" bot_auth_failed : 0 since rotation ``` `.env` was backed up first (`.env.bak-pre-botkey-20260829-064039`). It holds the **old** key — now worthless, and the rollback if needed. Worth deleting once you're satisfied. One honest note: the first scripted attempt failed partway. `python /tmp/rotate_key.py` put `/tmp` on `sys.path` so `app` was not importable, which left `.env` holding the new key while `app_settings` still held the old one. Auth was unaffected — the bot had not reloaded `.env` — but any bot restart in that window would have broken it. Completed by reading the key back out of `.env` rather than generating a second one, so both halves are provably identical. **The startup audit confirms it on the deployed release:** ``` Encrypted settings audit: 4 ok, 0 re-encrypted, 0 undecryptable ``` All four encrypted keys readable and properly encrypted at rest. Compare dev before this shipped, which reported `2 ok, 1 re-encrypted, 1 undecryptable` — the re-encryption naming `bot_api_key`, exactly the defect this issue describes. ## On pruning older backups Deciding it here, as the last criterion asked: **no pruning.** Rotation makes the exposed value worthless, so its presence in historical dumps and off-host archives is no longer a live exposure. Expunging a credential from an append-only archive history is expensive, error-prone, and buys nothing once the credential is dead. Rotation is the cheaper and more complete mitigation, and it is done. That reasoning belongs somewhere durable, so: **if a credential is found in plaintext at rest, rotate it — do not try to scrub the backups.** The startup audit now names any such key precisely so that decision can be made quickly. ## Acceptance criteria - [x] The production bot API key is rotated, and the old value no longer authenticates - [x] Any `_ENCRYPTED_KEYS` row stored without `_enc` is re-encrypted, with a sweep covering existing installs - [x] A plaintext row for an encrypted key is reported loudly rather than silently accepted - [x] A test asserts every `_ENCRYPTED_KEYS` value is written as `{"_enc": ...}`, and that a legacy plaintext row is upgraded - [x] Decide and document whether older backups containing the key should be pruned — decided above: no Shipped in **v4.0.1**.

claude-bot closed this issue

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

rbrooks referenced this issue from a commit

2026-08-29 07:43:15 +00:00

chore: allowlist the test-fixture secret so the scanner stays trustworthy (#132)

claude-bot referenced this issue

2026-08-29 07:43:26 +00:00

chore: allowlist the test-fixture secret so the scanner stays trustworthy (#132) #438

claude-bot referenced this issue

2026-08-29 18:03:43 +00:00

[Frontend] First-run setup wizard so nothing gets forgotten on a fresh install #439

claude-bot referenced this issue