v3.3.0 — Hardening: bugs & security #131

Merged
claude-bot merged 24 commits from hardening/v3.3.0 into main 2026-07-15 04:37:44 +00:00
Contributor

Cuts the v3.3.0 release: all 13 issues from the v3.3.0 milestone (July 2026 full-project review), plus reference-doc corrections and a changelog reconciliation.

Included

  • Security: Discord webhook SSRF allowlist (#97), recording authorization + session↔guild ownership (#100), reject placeholder/weak secrets at startup (#106), DML-only app DB role (#102), recap-email HTML escaping + /ask error hygiene (#109), bot loopback bind + body cap (#111).
  • Bug fixes: /record start guard leak (#83), audio-cleanup data loss (#85), duplicate reminders / retire legacy ETA path (#90), deterministic lore-match index (#88), Discord embed size budget (#94), public analytics share-page proxy (#92).
  • Docs: corrected recording architecture / DB roles / stack versions across CLAUDE.md, ARCHITECTURE.md, docs/API.md, docs/DEVELOPMENT.md (#113); reconciled the changelog with the actual v0.10.0–v3.2.0 releases.

⚠️ Breaking for existing installs

  • DB roles (#102): prod/dev must add POSTGRES_APP_PASSWORD, set POSTGRES_USER=questboard_admin, and run the one-time role migration in docs/OPERATIONS.md (§Existing Installs). Init scripts only run on an empty data dir, so existing databases need the manual migration.
  • Secret validation (#106): the backend now refuses to boot on .env.example placeholder secrets or a SECRET_KEY shorter than 32 chars — verify each environment's .env before deploying.

Verified locally: bot 158 passed, backend 358 passed, ruff clean, nginx config valid, DB privilege + pg_dump chain proven end-to-end.

🤖 Generated with Claude Code

Cuts the **v3.3.0** release: all 13 issues from the v3.3.0 milestone (July 2026 full-project review), plus reference-doc corrections and a changelog reconciliation. ## Included - **Security**: Discord webhook SSRF allowlist (#97), recording authorization + session↔guild ownership (#100), reject placeholder/weak secrets at startup (#106), DML-only app DB role (#102), recap-email HTML escaping + `/ask` error hygiene (#109), bot loopback bind + body cap (#111). - **Bug fixes**: `/record start` guard leak (#83), audio-cleanup data loss (#85), duplicate reminders / retire legacy ETA path (#90), deterministic lore-match index (#88), Discord embed size budget (#94), public analytics share-page proxy (#92). - **Docs**: corrected recording architecture / DB roles / stack versions across `CLAUDE.md`, `ARCHITECTURE.md`, `docs/API.md`, `docs/DEVELOPMENT.md` (#113); reconciled the changelog with the actual v0.10.0–v3.2.0 releases. ## ⚠️ Breaking for existing installs - **DB roles (#102)**: prod/dev must add `POSTGRES_APP_PASSWORD`, set `POSTGRES_USER=questboard_admin`, and run the one-time role migration in `docs/OPERATIONS.md` (§Existing Installs). Init scripts only run on an empty data dir, so existing databases need the manual migration. - **Secret validation (#106)**: the backend now refuses to boot on `.env.example` placeholder secrets or a `SECRET_KEY` shorter than 32 chars — verify each environment's `.env` before deploying. Verified locally: bot 158 passed, backend 358 passed, ruff clean, nginx config valid, DB privilege + `pg_dump` chain proven end-to-end. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
record_start added guild.id to _starting before the synchronous UUID and
voice-channel checks but only entered the try/finally after them, so either
early return left the guild permanently locked out until process restart.
Move the reservation down to immediately before the try, matching the
already-correct start_from_api pattern.

Closes #83

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
on_ready fires on every gateway reconnect, not just process start, so
_cleanup_audio_temp was rmtree-ing every session subdirectory on routine
Discord reconnects — including recordings still being written to and audio
already handed off to the backend but not yet picked up by the process_audio
Celery task. Move the sweep into setup_hook (runs exactly once) and only
remove subdirectories/raw WAVs older than a 60-minute threshold.

Verified the backend's process_audio task does NOT delete the session
directory on success (per its own docstring, it stays on the volume until a
GM approves/trashes the session) — noted in a comment as the long-term
ownership gap this bot-side sweep is a fallback for, not a replacement.

Closes #85

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Discord caps the whole embed (title + description + all field names/values +
footer) at 6000 chars even though description alone can be up to 4096. /recap
and the session_summarised/summary_approved notification embeds only guarded
the description with a bare [:4000] slice, so a long summary plus GM notes
plus feedback fields could still exceed 6000 and fail the send outright.

Add utils/embed_budget.fit_embed(), which enforces the individual
description/field-value caps and then compresses the description (the
compressible part) if the combined total is still over budget. Used at all
three call sites in place of the ad hoc slices.

Closes #94

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The multi-pass lore pipeline checkpointed per-type match results in
lore_extract_cache under a synthetic negative chunk_index derived from
the builtin hash(). That hash is PYTHONHASHSEED-salted, so a worker
restart mid-pipeline (deploy, OOM, crash) gave the new interpreter a new
seed: the checkpointed rows became unfindable, lore_consolidate_proposals
retried to MaxRetriesExceeded, and the run failed with "Consolidation
timed out" — defeating the crash-recovery purpose of the cache. Multiple
worker containers broke the same way.

Both the write and read sites now go through one shared
lore_match_chunk_index() helper backed by sha1, so they cannot drift.
No migration: lore_extract_cache is a transient checkpoint cache.

Closes #88

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
http_host defaulted to 0.0.0.0, exposing the bot's internal /notify and
/record/* endpoints on every interface for bare-metal self-hosters (Docker
Compose was unaffected — it only uses expose, not published ports). Default
to 127.0.0.1 and have docker-compose.yml set HTTP_HOST=0.0.0.0 explicitly for
the bot service so backend-to-bot notifications keep working over the
compose network.

Also pin aiohttp's Application client_max_size to 1 MiB explicitly (matches
its own current default) so a future default change can't silently widen
the accepted /notify body size.

Closes #111

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stored discord_webhook_url values are POSTed server-side from the Celery
worker, so any non-Discord URL is an SSRF primitive against the internal
network (cloud metadata, localhost ports, Redis/Postgres siblings).

Write-time validation existed only for campaigns and only as a
string-prefix check, which accepts nothing useful an attacker wants but
also left two gaps: the admin fallback webhook (PUT
/admin/settings/notifications) was stored verbatim with no validator at
all, and it is used for every campaign without its own webhook; and rows
written before the campaign validator existed were sent blind.

- One shared validator in url_policy_service parses with urlsplit and
  requires https, an exact host in the Discord allowlist (so
  discord.com.evil.example is rejected), no embedded credentials, no
  off-port target, and an /api/webhooks/ path.
- Applied to CampaignCreate, CampaignUpdate and NotificationSettingsRequest.
- Re-validated immediately before every outbound POST (discord backend and
  the vote notification task): a legacy invalid row is logged and skipped
  rather than sent, so no migration is needed.

The additional Discord hosts (discordapp.com, ptb., canary.) are now
accepted — intended.

Closes #97

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Neither nginx (prod) nor the Vite dev proxy forwarded /public/*, so
GET /public/analytics/{token} fell through to the SPA catch-all and
returned index.html, breaking the unauthenticated share-link feature.
Mirror the existing /api/ proxy block/entry for /public/ in both.

Closes #92

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
.env.example ships guessable placeholders (SECRET_KEY=replace-with-
openssl-rand-hex-32, POSTGRES_PASSWORD=changeme, and the same passwords
embedded in DATABASE_URL/DATABASE_MIGRATE_URL). A deployment that copies
the template and misses one value booted successfully on guessable
credentials. SECRET_KEY is the worst case: it signs sessions and derives
the AES-GCM key that encrypts every credential in the settings table.

Settings now validates at construction time:
- SECRET_KEY: rejects the placeholder, requires >= 32 chars.
- DATABASE_URL / DATABASE_MIGRATE_URL: rejects the placeholder password,
  matched against the parsed password component (make_url), not a
  substring of the whole URL.
- BOT_API_KEY: empty stays legal (require_bot_auth rejects every request
  when no key is stored, so "unset" is fail-closed); when set, known
  placeholders and values under 16 chars are rejected.

Each message names the env var and how to fix it. CI's SECRET_KEY was 27
chars and would now fail Settings() in both pytest and Alembic, so it is
replaced with a compliant dummy.

Closes #106

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
POSTGRES_USER was created by the postgres Docker entrypoint as a
SUPERUSER, so the app connected with full DDL/admin rights despite the
documented two-role (DML app user / DDL migrate user) model. Postgres
unconditionally refuses to strip SUPERUSER from the initdb bootstrap
role (ALTER ROLE ... NOSUPERUSER fails with "the bootstrap user must
have the SUPERUSER attribute", regardless of which role issues it), so
init.sh instead renames the bootstrap role aside (NOLOGIN) and creates
a brand-new, never-superuser role with the original name/password, so
DATABASE_URL keeps authenticating unchanged. The new role is granted
DML on current and future (questboard_migrate-created) tables/sequences
only.

Also removes webapp/postgres/init.sql: a dead file, mounted nowhere,
that hardcoded a migrate-role password and only had the app-user
tightening as a comment.

Added an "Existing Installs" runbook to docs/OPERATIONS.md with the
equivalent one-time migration (init scripts only run on an empty data
dir) and a break-glass single-user-mode recovery procedure, since the
renamed bootstrap role becomes permanently unreachable.

Verified against a throwaway postgres:16-alpine container on an empty
data dir: init.sh completes without error; the app user cannot CREATE
TABLE; questboard_migrate can; the app user can still SELECT/INSERT on
a migrate-created table via default privileges.

Confirmed webapp/backend/alembic/env.py overrides sqlalchemy.url with
settings.database_migrate_url, so Alembic already connects via the
migrate role and needs no change.

Closes #102

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root CLAUDE.md's "Recording architecture" section described a
not-yet-implemented, single mixed-mono-MP3 design; the shipped flow
(webapp/backend/app/routers/bot.py, bot/questboard_bot/cogs/recording.py)
actually records per-speaker 16 kHz WAVs + speakers.json to a per-session
directory on audio_temp, POSTs the dir path to
/api/bot/sessions/{id}/audio, and the process_audio Celery task
transcribes each speaker independently and merges an attributed
transcript before summarising. Rewrote the section to match, removed
the "not yet implemented" status block and mono-MP3 wording.

webapp/CLAUDE.md's DB-role wording now states the two-role model as
enforced by webapp/postgres/init.sh for fresh installs (per #102 on
this branch) and points existing installs at the new OPERATIONS.md
runbook; removed its reference to the now-deleted init.sql.

webapp/CLAUDE.md's stack table updated to match webapp/frontend/package.json
(React 19.2.7, Vite 8.1.4, Tailwind 4.3.2, react-router-dom 7.15.0)
instead of the stale React 18/Vite 6/Tailwind 3/react-router-dom 6 claim.

Root CLAUDE.md and the Dockerfile's stage-header comments claimed
Python 3.12 while both backend and bot stages build FROM
python:3.14-slim; updated the comments to say 3.14 and noted the
pending 3.12-vs-3.14 decision is tracked in #87 (v3.4.0, not in scope
here) -- no base image or CI Python version changed.

Closes #113

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The recap email body was built by f-string interpolation with no
escaping, so the GM-controlled session title and campaign name and the
LLM-generated summary were delivered as live markup (MIMEText(..., "html"))
to every recipient's mail client. Values are now html.escape()d before the
newline→<br> conversion, so the inserted <br> tags survive, and CR/LF is
stripped from the title before it goes into the Subject header.

The bot /ask endpoint returned detail=f"LLM error: {exc}", which can leak
the configured endpoint URL and other internals and violates the project's
error contract. It now returns a generic detail and logs the exception
server-side.

Closes #109

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The age gate added for #85 wasn't enough: process_audio never deletes the
session directory on success, so a real bot restart (deploy, crash, host
reboot) could still sweep a dir sitting on the volume for days awaiting
Whisper/Celery or GM review — there is no age at which that's safe to delete.

RecordingCog._process now writes a HANDOFF_MARKER (.handed-off) into the
session dir immediately after a successful post_audio_tracks ack, and only
then — a failed upload leaves the dir unmarked and still a genuine orphan.
_cleanup_audio_temp skips any marked directory regardless of age; the
existing age gate now only applies to unmarked dirs. The leading-dot marker
name is invisible to process_audio's session_dir.glob("*.wav") and its
speakers.json read.

Completes the fix started for #85.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Session reminders had two coexisting delivery mechanisms with independent
dedup stores: the poll_session_reminders beat task (deduped by
session_reminders_sent rows in Postgres) and the legacy ETA path
(send_session_reminder tasks scheduled with apply_async, deduped by a Redis
key). Neither suppressed the other.

The reschedule path did BOTH — it cleared the SessionReminderSent rows so
the poller re-fires at the new time AND called _schedule_reminders — so
every reminder for a rescheduled session was delivered twice. confirm_session
already relied on the poller alone.

Reschedule now matches confirm_session: clear the sent rows and send the
confirmation notice for the new time. _schedule_reminders was the only
caller of send_session_reminder, and the only writer of
session.celery_task_ids, so the whole ETA path is retired:
send_session_reminder, its Redis dedup key, _schedule_reminders and the
_revoke_reminders helper are gone. The celery_task_ids column stays (no
migration); nothing writes it.

One case the poller did not cover: reschedule is allowed from in_progress
(update_session), which leaves the status as in_progress while the
confirmed_time moves back into the future, and the poller only selected
confirmed sessions. The poller now selects confirmed + in_progress, which
is a no-op for genuinely running sessions (their confirmed_time is already
outside the lookback window).

Closes #90

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This reverts commit 86708836bc.
Reverts and replaces the previous fix for #102 (rename-aside approach,
commit 8670836), which was correct in diagnosis but wrong in shape: it
destroyed break-glass by leaving no login-capable superuser in the
cluster, and it silently broke nightly pg_dump backups, which ran as
the app user and only worked because that user was still a superuser
(webapp/backend/app/cli.py:_backup_now,
webapp/backend/app/tasks/reminder_tasks.py:_run_backup_async).

New design: stop making the app user the bootstrap role at all.

- POSTGRES_USER becomes a dedicated administrative account
  (questboard_admin). The postgres Docker entrypoint always creates it
  as the cluster's bootstrap superuser (Postgres will never let that
  role lose SUPERUSER), but since the app never connects as it, that's
  just intentional break-glass admin access -- not a runtime privilege
  the app carries.
- webapp/postgres/init.sh (running as questboard_admin) creates two
  purpose-built roles: questboard_migrate (LOGIN, DDL, owns the
  schema -- used by Alembic and now by pg_dump) and questboard (LOGIN,
  NOSUPERUSER/NOCREATEDB/NOCREATEROLE, DML-only -- used by the app).
  questboard gets SELECT/INSERT/UPDATE/DELETE on tables/sequences that
  exist now or that questboard_migrate creates in the future via
  ALTER DEFAULT PRIVILEGES. No rename hackery, no temporary SUPERUSER
  grants, no permanently NOLOGIN roles.
- .env.example: POSTGRES_USER=questboard_admin, new
  POSTGRES_APP_PASSWORD for the questboard role, DATABASE_URL updated
  to match. Placeholder passwords stay changeme-style on purpose --
  #106 makes the backend reject them at startup as a fail-fast check.
- cli.py's backup_now and reminder_tasks.py's scheduled backup task
  now build their pg_dump invocation from database_migrate_url instead
  of database_url, since the migrate role (not the now DML-only app
  role) owns the tables pg_dump needs to see. Touched only the backup
  function/call site in each file.
- docs/OPERATIONS.md: rewrote "Existing Installs" for the new shape --
  rename the existing questboard superuser to questboard_admin
  (keeping SUPERUSER + LOGIN) and create a fresh DML-only questboard,
  rather than ending with no superuser at all. Dropped the
  single-user-mode break-glass section; it's no longer needed since a
  login-capable superuser always remains.

Verified on a throwaway postgres:16-alpine container (empty data dir)
plus the real backend-prod image:
- init.sh completes with no errors, creating all three roles with the
  intended attributes (questboard_admin: superuser+login;
  questboard_migrate and questboard: login, not superuser).
- `alembic upgrade head` against DATABASE_MIGRATE_URL (questboard_migrate)
  runs the full real migration chain to completion (exit 0) and is a
  clean no-op on a second run.
- questboard (app role): CREATE TABLE fails with "permission denied
  for schema public"; INSERT/SELECT/UPDATE/DELETE against the real,
  migrated `campaigns` table all succeed.
- questboard_admin: still logs in and rolsuper = true.
- `pg_dump --format=custom` as questboard_migrate against the migrated
  schema succeeds (exit 0), produces a 94584-byte archive whose TOC
  lists all 31 real tables (confirmed via pg_restore --list and a
  plain-format re-dump grepped for CREATE TABLE).

Closes #102

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the #102 rework: webapp/CLAUDE.md now describes the
questboard_admin / questboard_migrate / questboard split enforced by
init.sh, replacing the aspirational DML-only claim.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two authorization gaps closed together (cross-component change).

Bot — /record start|stop were invocable by any guild member. Both handlers
now check authorization first, before reserving any recording state: a member
is allowed only if they hold Discord Manage Server, or are the campaign's
verified-linked GM (resolved via a new GET /api/bot/guilds/{guild_id}/gms).
The check fails closed — an API error or an unlinked guild denies with a
distinct "couldn't verify" message so a retry is understood as possibly
resolving it, versus a plain "not authorized".

Backend — POST /api/bot/sessions/{session_id}/audio authorized only the
X-Bot-Key and never checked that the session's campaign belonged to the
guild_id in the payload, so audio from guild A could be attached to a session
in guild B. It now loads the session's campaign and requires
campaign.guild_id == data.guild_id, returning 404 on mismatch (no cross-guild
existence probing) and queuing no Celery task.

Endpoint audit: /audio is the only /api/bot/* endpoint carrying both a
session_id and a bot-supplied guild_id; the other session-keyed endpoints take
no guild claim to cross-check, and /guilds/{guild_id}/* endpoints already scope
by guild, so no further ownership checks are needed.

Tests: bot permission gate for start/stop (Manage Server allow, GM allow,
non-GM deny, API-failure/unlinked deny, and denial-before-state-mutation);
backend cross-guild upload rejected with no task queued, plus the GM lookup
endpoint.

Closes #100

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the v3.3.0 hardening batch to CHANGELOG.md, and bring the reference
docs in line with what the milestone actually changed:

- ARCHITECTURE.md: recording pipeline rewritten from "planned refactor" to
  the shipped per-speaker WAV + /audio + Celery flow; reminders described as
  poll-based (ETA path retired); SSRF row updated to the host allowlist;
  celery_task_ids noted as legacy.
- docs/API.md: /audio guild-ownership check + corrected response; new
  /bot/guilds/{guild_id}/gms endpoint; cancel no longer "revokes" reminders.
- webapp/CLAUDE.md: Celery task table and celery_task_ids constraint.
- bot/CLAUDE.md: Python 3.14 (#87), FFmpeg role as resampling.
- docs/DEVELOPMENT.md, README.md: init.sh roles, recording wording.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The changelog stopped being cut into version sections after the old
per-component scheme (webapp-0.3.3 / bot-0.9.0, mid-March 2026), while the
project kept shipping — released and retroactively tagged as v0.10.0 through
v3.2.0. All of that already-released work had accumulated under [Unreleased].

Carve it into proper version sections. Each existing bullet was attributed to
its release via `git blame` → earliest containing tag, so interleaved content
lands in the right version (e.g. "session summary search" → 3.1.0, "re-post
approved summary" → 2.0.0). Versions that never received changelog bullets
(0.12.0, 1.0.0, 1.1.0, 2.1.0, 3.2.0) get concise entries drawn from their
Forgejo release notes, so the log is gap-free from 0.10.0 to 3.2.0.

[Unreleased] now holds only the genuinely-unreleased v3.3.0 hardening work.
All 78 previously-misfiled bullets preserved verbatim (one double-encoded
em-dash normalised); legacy per-component sections left untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
docs: cut v3.3.0 changelog section
All checks were successful
CI / Frontend tests, audit, and build (pull_request) Successful in 1m27s
CI / Backend lint (ruff) (pull_request) Successful in 1m4s
CI / Bot tests and audit (pull_request) Successful in 2m14s
CI / Backend migration, tests, and audit (pull_request) Successful in 3m47s
CI / Docker image build (pull_request) Successful in 4m34s
140862b88a
Promote the v3.3.0 hardening block from [Unreleased] to a dated
## [3.3.0] section so the release workflow extracts its notes on tag push.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign in to join this conversation.
No description provided.