fix(backend): redact the campaign webhook URL and share token from non-GM members (#671) #672
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/671-redact-campaign-credentials"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #671.
What was leaking, and why it matters
GET /campaigns/{campaign_id}is gated onrequire_campaign_memberand returns the wholeCampaignResponse.to_campaign_responsetook the viewer but used it only foris_owner, so every field came straight off the model — including the campaign's Discord webhook URL.That is a credential, not a setting. Anyone holding it can post to the group's channel as Quest Board, unauthenticated, with nothing tying the message back to them: fake session cancellations, fake reminders. The analytics share token is the milder version of the same shape — the link it builds carries every member's name and attendance rate, and creating/rotating/revoking it is correctly
require_gmwhile reading it was not.Nothing showed a player any of this; the Edit Campaign form is GM-gated on both sides. But the response body carried it, so devtools or any script holding an ordinary session cookie had it. This is what one response model serving both "show me the campaign" and "fill in the settings form" produces. #97 fixed where the server may send a webhook (SSRF); this is who may read one back.
What changed
webapp/backend/app/services/campaign_service.py—to_campaign_responsenow resolves the viewer's role and blanks four fields for anyone who is not a GM of that campaign:discord_webhook_urlnullanalytics_share_tokennullguild_idnullnotification_channel_idnullRedacted in the builder rather than at the route on purpose: it is the only place a
CampaignResponseis constructed (verified — there is nomodel_validatepath), so all eight call sites are covered at once and so is the next one anybody adds.discord_delivery_statusis untouched and must stay that way. It carries no secret, and a player wondering why the reminders stopped deserves the same answer their GM gets (#369). The comment onschemas/campaign.pyexplaining that is intact. Worth knowing, though: that status dict names the channel it could not deliver to, so redactingnotification_channel_idis partial by design, not by oversight — the test says so out loud.GM, not "GM or owner." An owner who is only a player already reads nothing GM-only —
invite_service.can_manage_membershipsays so in as many words, granting the owner membership management and nothing that reads campaign content — and an owner who wants the webhook back can give themselves the role. The consequence is that the owner-gated routes (/archive,/restore,/undelete) return redacted fields to a non-GM owner; nothing on those pages reads them.The share-link boolean
Added, as
analytics_share_link_exists: bool. It was a clean two lines with no frontend or schema churn, and it is the thing that makes the redaction lossless in what a player is actually entitled to: whether a public link to their own name and attendance rate exists is their business; being able to hand it out is not. It follows the pattern this codebase already uses for exactly this —llm_api_key_set,asr_api_key_set,bot_token_configured,api_key_configured. No consumer yet; per #671 it belongs onCAMP-7in the redesign.Call sites and frontend
All eight
to_campaign_responsecall sites checked. The GM-gated ones (PATCH /campaigns/{id}, create) are unaffected, so the Edit Campaign form round-trips exactly as before. The two service-level test callers pass no viewer and assert on retention/language fields only.Frontend checked rather than assumed — all four reads are already GM-gated:
pages/CampaignDetail.jsx:497-499— the edit form, rendered behindisGmcomponents/FirstRunChecklist.jsx:63— rendered behindisGm && !campaign.is_archivedcomponents/RecordingPanel.jsx:274— the component returnsnullat:207for a non-GM, before it looks atguild_idpages/CampaignAnalytics.jsx:43— the share panel is behind its ownisGmat:116No frontend change needed.
Tests
Two added in
webapp/backend/tests/test_campaigns.py: a plain member'sGET /campaigns/{id}returnsnullfor all four (and the body is scanned so neither credential can leak through some other field), and a GM's returns all four intact.I could not run the suite. Docker Desktop is not running on this machine and the backend tests need Postgres; the project's documented command is a Docker one, and I did not invent a substitute.
ruff checkandruff format --checkonwebapp/backendboth pass under the pinned ruff 0.4.4. This PR leans on CI for the pytest result — please do not merge on my word that it passes.No version bump: this does not touch
/api/bot/*.Audit: other response schemas with the same shape
Bounded scan of
webapp/backend/app/schemas/plus the inline response models in the routers, looking for a credential, token, secret, or URL-with-embedded-secret returned to a wider audience than the thing that writes it. Reported only — nothing fixed here.One finding, low severity:
webapp/backend/app/routers/users.py:106—NotificationSettingsResponse.discord_webhook_urlreturns the instance-wide fallback webhook in full, fromGET /api/admin/settings/notifications(users.py:636-657). Same field class and the same "one model for showing and for filling the form" conflation as #671. It isrequire_admin, so the audience is the operator who set it, which is why this is a note and not a bug — but the SMTP password sitting beside it in the same response is reduced toconfigured: bool(users.py:101), so the inconsistency is within a single model.Checked and clean (each returns its secret only to the party that owns it, or already uses a boolean):
schemas/calendar.py:13-14—CalendarFeedResponse.token/feed_url: a URL with an embedded secret, but the routes are/api/me/calendar-feed(users.py:346,:359) — self-scoped, so the only reader is the owner.routers/campaigns.py:6820-6845—CampaignAiSettingsResponse: docstring reads "What a GM may see. Never a key, in any form";llm_api_key_set/asr_api_key_setare booleans.routers/users.py:164-190—BotSettingsResponse:bot_token_configured,api_key_configured— booleans, admin-gated.routers/admin.py:861,:985-986— the admin campaign inventory already boolean-iseshas_guild/has_notification_channel, andtests/test_admin_campaign_inventory.py:395-427pins the key set and asserts the webhook, share token, invite code and description are absent from the serialised body. This is the discipline #671 wants, already written down.schemas/campaign.pyInviteResponse.code— an invite code is a credential, andGET /{campaign_id}/invitesisrequire_membership_manager(campaigns.py:1405-1413), i.e. GM or owner. Correct.routers/bot.py:55webhook_url— behindrequire_bot_auth; the bot is the intended consumer.routers/public.py:35-61— the unauthenticated analytics response returns neither the token nor any Discord field.schemas/session.py:164recording_url— a column only ever populated by import (import_service.py:276) and echoed to the exporter; not a URL with an embedded secret in practice.Adjacent, different class, flagged not filed:
MemberResponse.email(schemas/campaign.py, built incampaign_service.list_members) hands every member's email address to every other member. That is PII rather than a credential, so it is outside this issue's shape and outside what I changed, but it is the same "who is the audience for this field" question and someone should decide it deliberately.🤖 Generated with Claude Code
`GET /campaigns/{id}` is gated on membership and returns the whole `CampaignResponse`, so every player could read the campaign's Discord webhook URL. That is a credential, not a setting: anyone holding it can post to the group's channel as Quest Board, unauthenticated, with nothing tying the message back to them — fake cancellations, fake reminders. The analytics share token is the milder version of the same thing, since the link it builds carries every member's name and attendance rate and revoking it is `require_gm`. Nothing showed a player any of this — the Edit Campaign form is GM-gated on both sides — but the response carried it, so devtools or any script holding an ordinary session cookie had it. This is what happens when one response model serves both "show me the campaign" and "fill in the settings form"; #97 fixed where the server may *send* a webhook, not who may *read* one back. So `to_campaign_response` now blanks four fields for anyone who is not a GM of that campaign: `discord_webhook_url`, `analytics_share_token`, `guild_id` and `notification_channel_id`. Redacted in the builder rather than at the route because it is the only place a `CampaignResponse` is constructed — all eight call sites are covered at once, and so is the next one. GM callers are untouched, which is what keeps the edit form working. Two deliberate non-changes and one addition: - `discord_delivery_status` stays visible to everyone. It carries no secret, and a player wondering why the reminders stopped deserves the same answer their GM gets (#369). Note that it names the channel it could not deliver to, so redacting `notification_channel_id` is partial by design rather than by oversight. - GM, not "GM or owner". An owner who is only a player already reads nothing GM-only — `can_manage_membership` says so in as many words — and an owner who wants the webhook can give themselves the role. - `analytics_share_link_exists` keeps the one thing the token honestly told a player: that a public link to their own attendance exists. A boolean, following `llm_api_key_set` and `bot_token_configured`. Frontend checked rather than assumed: all four reads are GM-gated already — the edit form and `FirstRunChecklist` behind `isGm`, `RecordingPanel` returns null for a non-GM before it looks at `guild_id`, and the analytics share panel is behind its own `isGm`. No frontend change needed. Tests: two, on the member/GM split. Could not be run locally — Docker Desktop is not running on this machine and the suite needs Postgres, so this leans on CI. ruff check and ruff format --check both pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>CI result, since the PR body could only say "leans on CI" when it was written.
Run 9773, all eight jobs green — including Backend migration, tests, and audit:
3587 passed, 34 skipped, 78 warnings in 605.16s. So the two new tests intests/test_campaigns.pyran against a real Postgres and passed, and nothing else in the suite noticed the redaction. Backend lint (ruff) green too, which matches what I got locally against the pinned 0.4.4.Still unmerged and still wants a human read — the judgement calls worth checking are (a) GM rather than "GM or owner", and (b) that
discord_delivery_statusdeliberately keeps naming the channel, so thenotification_channel_idredaction is partial on purpose.