[Hardening] Require authorization for /record and verify session↔guild ownership on bot audio endpoints #100

New issue

Closed

opened 2026-07-14 19:49:01 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-07-14 19:49:01 +00:00

Collaborator

Context

Recording controls two authorization gaps, one in each component. Per the consent model, all group members have standing consent to be recorded — but starting/stopping recordings and attaching audio to arbitrary sessions should still be restricted.

Bot half — /record start (bot/questboard_bot/cogs/recording.py:199-242) and /record stop (:427-445) are invocable by any guild member. There is no role, permission, or GM check anywhere in either handler: any member in the server can start a recording (joining a voice channel) or kill an in-progress recording mid-session.

Backend half — POST /api/bot/sessions/{session_id}/audio (webapp/backend/app/routers/bot.py:657-707) authorizes only the X-Bot-Key header (require_bot_auth, :663). The request payload already carries guild_id (AudioUploadRequest, :652) and it is passed to the Celery task (:702) — but the endpoint never verifies that the session's campaign belongs to that guild. Any valid session UUID is accepted, so audio from guild A can be attached to (and overwrite processing state of) a session in a campaign belonging to guild B.

Fix / Spec

Bot:

In both record_start and record_stop, allow the invocation only if:
- (a) the invoking member's verified-linked Quest Board user is the campaign GM (resolve via the existing self.bot.api client lookups — campaign/member endpoints used elsewhere in the cogs), or
- (b) the member has the Discord Manage Guild permission (interaction.user.guild_permissions.manage_guild).
Otherwise reply with an ephemeral denial message and return. Check (b) first (cheap, local) then (a).
Perform the check before reserving _starting / touching _active state.

Backend:
4. In bot_upload_audio, after loading the session, load its campaign and require session.campaign.guild_id == data.guild_id; on mismatch return 404 (don't leak session existence across guilds) or 403 — pick one and be consistent.
5. Audit the other /api/bot/* endpoints that accept a session_id plus guild context supplied by the bot (record status/summary/notes/transcript-related endpoints in webapp/backend/app/routers/bot.py) and apply the same ownership check wherever the bot supplies a guild context. Endpoints keyed only by guild_id (e.g. /bot/guilds/{guild_id}/..., :416, :469, :505, :884) already scope by guild.

Both halves ship in the same PR (cross-component change).

Acceptance criteria

A guild member without Manage Guild and who is not the linked GM gets an ephemeral denial from /record start and /record stop; the GM and Manage Guild holders succeed.
Backend test: uploading audio with a session_id whose campaign belongs to a different guild_id is rejected (403/404) and queues no Celery task.
Audit outcome for other bot endpoints noted in the PR (which endpoints gained the check and why the rest don't need it).
Bot tests in bot/tests/test_recording.py cover the permission gate.

References

bot/questboard_bot/cogs/recording.py:199-242 (record_start, no auth), :427-445 (record_stop, no auth)
webapp/backend/app/routers/bot.py:650-655 (AudioUploadRequest with guild_id), :657-707 (bot_upload_audio, no ownership check), :663 (require_bot_auth only)
webapp/backend/app/auth/dependencies.py (require_bot_auth)

Filed from the July 2026 full-project review.

## Context Recording controls two authorization gaps, one in each component. Per the consent model, all group members have standing consent to be recorded — but *starting/stopping* recordings and *attaching audio to arbitrary sessions* should still be restricted. **Bot half** — `/record start` (`bot/questboard_bot/cogs/recording.py:199-242`) and `/record stop` (`:427-445`) are invocable by **any guild member**. There is no role, permission, or GM check anywhere in either handler: any member in the server can start a recording (joining a voice channel) or kill an in-progress recording mid-session. **Backend half** — `POST /api/bot/sessions/{session_id}/audio` (`webapp/backend/app/routers/bot.py:657-707`) authorizes only the `X-Bot-Key` header (`require_bot_auth`, `:663`). The request payload already carries `guild_id` (`AudioUploadRequest`, `:652`) and it is passed to the Celery task (`:702`) — but the endpoint **never verifies that the session's campaign belongs to that guild**. Any valid session UUID is accepted, so audio from guild A can be attached to (and overwrite processing state of) a session in a campaign belonging to guild B. ## Fix / Spec Bot: 1. In both `record_start` and `record_stop`, allow the invocation only if: - (a) the invoking member's verified-linked Quest Board user is the **campaign GM** (resolve via the existing `self.bot.api` client lookups — campaign/member endpoints used elsewhere in the cogs), **or** - (b) the member has the Discord **Manage Guild** permission (`interaction.user.guild_permissions.manage_guild`). 2. Otherwise reply with an ephemeral denial message and return. Check (b) first (cheap, local) then (a). 3. Perform the check before reserving `_starting` / touching `_active` state. Backend: 4. In `bot_upload_audio`, after loading the session, load its campaign and require `session.campaign.guild_id == data.guild_id`; on mismatch return 404 (don't leak session existence across guilds) or 403 — pick one and be consistent. 5. Audit the other `/api/bot/*` endpoints that accept a `session_id` **plus** guild context supplied by the bot (record status/summary/notes/transcript-related endpoints in `webapp/backend/app/routers/bot.py`) and apply the same ownership check wherever the bot supplies a guild context. Endpoints keyed only by `guild_id` (e.g. `/bot/guilds/{guild_id}/...`, `:416`, `:469`, `:505`, `:884`) already scope by guild. Both halves ship in the same PR (cross-component change). ## Acceptance criteria - A guild member without Manage Guild and who is not the linked GM gets an ephemeral denial from `/record start` and `/record stop`; the GM and Manage Guild holders succeed. - Backend test: uploading audio with a `session_id` whose campaign belongs to a different `guild_id` is rejected (403/404) and queues no Celery task. - Audit outcome for other bot endpoints noted in the PR (which endpoints gained the check and why the rest don't need it). - Bot tests in `bot/tests/test_recording.py` cover the permission gate. ## References - `bot/questboard_bot/cogs/recording.py:199-242` (`record_start`, no auth), `:427-445` (`record_stop`, no auth) - `webapp/backend/app/routers/bot.py:650-655` (`AudioUploadRequest` with `guild_id`), `:657-707` (`bot_upload_audio`, no ownership check), `:663` (`require_bot_auth` only) - `webapp/backend/app/auth/dependencies.py` (`require_bot_auth`) _Filed from the July 2026 full-project review._

claude-bot added the

labels

2026-07-14 19:49:08 +00:00

claude-bot added this to the v3.3.0 - Hardening: Bugs & Security milestone

2026-07-14 19:49:10 +00:00

claude-bot commented

2026-07-14 20:26:58 +00:00

Author

Collaborator

Queued as part of a v3.3.0 push. This one is cross-component (bot /record authorization + backend session↔guild ownership check), and it touches both cogs/recording.py and routers/bot.py — both of which are being changed on the hardening/bot and hardening/backend branches right now (#83/#85 and #109 respectively).

To avoid a three-way conflict it will land last, on its own branch rebased on top of those two, so the bot and backend halves still ship in a single PR as the issue requires.

Queued as part of a v3.3.0 push. This one is cross-component (bot `/record` authorization + backend session↔guild ownership check), and it touches both `cogs/recording.py` and `routers/bot.py` — both of which are being changed on the `hardening/bot` and `hardening/backend` branches right now (#83/#85 and #109 respectively). To avoid a three-way conflict it will land last, on its own branch rebased on top of those two, so the bot and backend halves still ship in a single PR as the issue requires.

claude-bot commented

2026-07-14 21:34:46 +00:00

Author

Collaborator

Fixed on main (commit 2b58ed6, merged via 0ddb35d) — both halves in a single commit, per the cross-component rule.

Bot — /record start and /record stop now authorize before touching any recording state: allowed only if the invoker holds Discord Manage Server, or is the campaign's verified-linked GM (resolved via a new GET /api/bot/guilds/{guild_id}/gms). Fails closed — an API error or an unlinked guild denies with a distinct "couldn't verify, try again" message, kept separate from a plain "not authorized" so a transient outage reads correctly.

Backend — POST /api/bot/sessions/{session_id}/audio 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. Cross-guild audio attach is closed.

Endpoint audit (step 6): /audio is the only /api/bot/* endpoint carrying both a session_id and a bot-supplied guild_id. The other session-keyed endpoints (vote, attendance, timeslots, summary, notes, transcript-feedback, lore-proposals) take no guild claim, so there is nothing to cross-check; /guilds/{guild_id}/* endpoints already scope by guild. No further ownership checks needed.

Tests: bot permission gate (Manage Server allow, GM allow, non-GM deny, API-failure/unlinked deny, and denial-before-state-mutation for both start and stop); backend cross-guild upload rejected with process_audio.delay asserted not called, plus the GM lookup endpoint (verified-link filtering, 404 on unlinked guild). Two pre-existing /audio tests updated to set matching guild ownership. Bot suite 158, backend 358, ruff clean.

Fixed on `main` (commit `2b58ed6`, merged via `0ddb35d`) — both halves in a single commit, per the cross-component rule. **Bot** — `/record start` and `/record stop` now authorize before touching any recording state: allowed only if the invoker holds Discord Manage Server, or is the campaign's verified-linked GM (resolved via a new `GET /api/bot/guilds/{guild_id}/gms`). Fails closed — an API error or an unlinked guild denies with a distinct "couldn't verify, try again" message, kept separate from a plain "not authorized" so a transient outage reads correctly. **Backend** — `POST /api/bot/sessions/{session_id}/audio` 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. Cross-guild audio attach is closed. **Endpoint audit (step 6):** `/audio` is the only `/api/bot/*` endpoint carrying both a `session_id` and a bot-supplied `guild_id`. The other session-keyed endpoints (`vote`, `attendance`, `timeslots`, `summary`, `notes`, `transcript-feedback`, `lore-proposals`) take no guild claim, so there is nothing to cross-check; `/guilds/{guild_id}/*` endpoints already scope by guild. No further ownership checks needed. Tests: bot permission gate (Manage Server allow, GM allow, non-GM deny, API-failure/unlinked deny, and denial-before-state-mutation for both start and stop); backend cross-guild upload rejected with `process_audio.delay` asserted not called, plus the GM lookup endpoint (verified-link filtering, 404 on unlinked guild). Two pre-existing `/audio` tests updated to set matching guild ownership. Bot suite 158, backend 358, ruff clean.

claude-bot closed this issue

2026-07-14 21:34:54 +00:00