rbrooks/Quest-Board

Watch

Fork 0

[Backend][Security] GET /campaigns/{id} returns the Discord webhook URL and analytics share token to every member #671

New issue

Closed

opened 2026-09-16 03:48:20 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-09-16 03:48:20 +00:00

Collaborator

Found while checking, for the redesign (#659), whether a player needs a Settings link. The answer was no — and the reason turned out to be interesting: the settings form is GM-only, but the settings data is not.

What happens

GET /campaigns/{campaign_id} is gated on require_campaign_member (webapp/backend/app/routers/campaigns.py:960-972) and returns the full CampaignResponse. That schema carries, among the ordinary fields:

discord_webhook_url (webapp/backend/app/schemas/campaign.py:367)
guild_id (:368)
notification_channel_id (:369)
analytics_share_token (:396)

to_campaign_response takes the viewer but uses it only to compute is_owner (webapp/backend/app/services/campaign_service.py:28-30, 52); every field above is copied straight off the model with no role check (:57-59, and the share token further down).

The frontend renders these only inside the GM-gated Edit Campaign form, so nothing looks wrong in the UI. But any member can read them from the API response — devtools, or any script holding their own session.

Why the webhook URL matters most

A Discord webhook URL is a credential, not a setting. Anyone holding it can post arbitrary messages to that channel, as the webhook's configured name and avatar, with no authentication and no audit trail back to them. A player who copies it can impersonate Quest Board's own notifications in the group's Discord — including fake session cancellations or fake reminders.

This is a different problem from #97, which validated the URL against a Discord host allowlist on write (SSRF). That one was about where the server may send requests; this is about who may read the value back.

analytics_share_token is a lesser version of the same shape: creating, rotating and revoking it are correctly require_gm (campaigns.py:2588, :2616), but the existing value is readable by every member, so a player can hand the public analytics link — which carries every member's name and attendance rate — to anyone, and the GM has no way to know it was them.

guild_id and notification_channel_id are ids rather than credentials, and discord_delivery_status is already deliberately member-visible with a comment explaining why (schemas/campaign.py:370-375) — that reasoning is sound and should stay.

Threat model, honestly stated

Everyone in a campaign is a friend group who already trust each other, and this is self-hosted, so this is not a "drop everything" severity. But:

Campaigns can have members who joined by invite link and are not close friends, and #643 added a join-approval flow precisely because membership is not always tightly held.
A leaked webhook does not need a malicious player — a member pasting a "here's the API response" screenshot into a support thread is enough.
The fix is small and the exposure has no upside: no member-facing feature reads any of these four fields.

Suggested fix

Redact on the way out rather than adding a second endpoint. to_campaign_response already receives the viewer; give it the viewer's role and blank the GM-only fields for everyone else:

discord_webhook_url → None for non-GM
analytics_share_token → None for non-GM (the presence of a share link may be worth surfacing as a plain boolean, since a player arguably deserves to know one exists)
guild_id, notification_channel_id → GM-only too, on the principle that nothing member-facing reads them; keep discord_delivery_status as it is

Then check the frontend does not rely on reading them as a non-GM (it should not — the form is GM-only), and add a test asserting a player's GET /campaigns/{id} has those fields null.

Worth also auditing the other response schemas for the same shape, since this one was not deliberate — it is what happens when one response model serves both the "show me the campaign" and "fill in the settings form" jobs. That conflation is the actual root cause, and splitting the two is the more thorough fix if the redaction turns out to be fiddly.

Where it lives in the redesign

No new surface. CAMP-14/Settings stays GM-only, which is already how it is drawn in lane 1 (#659). If the "a share link exists" boolean is added, it belongs on CAMP-7 (Analytics) alongside the existing GM-only share panel.

Found by reading the code; not exploited or tested against any running instance.

Found while checking, for the redesign (#659), whether a player needs a Settings link. The answer was no — and the reason turned out to be interesting: the settings *form* is GM-only, but the settings *data* is not. ## What happens `GET /campaigns/{campaign_id}` is gated on `require_campaign_member` (`webapp/backend/app/routers/campaigns.py:960-972`) and returns the full `CampaignResponse`. That schema carries, among the ordinary fields: - `discord_webhook_url` (`webapp/backend/app/schemas/campaign.py:367`) - `guild_id` (`:368`) - `notification_channel_id` (`:369`) - `analytics_share_token` (`:396`) `to_campaign_response` takes the viewer but uses it **only** to compute `is_owner` (`webapp/backend/app/services/campaign_service.py:28-30, 52`); every field above is copied straight off the model with no role check (`:57-59`, and the share token further down). The frontend renders these only inside the GM-gated Edit Campaign form, so nothing looks wrong in the UI. But any member can read them from the API response — devtools, or any script holding their own session. ## Why the webhook URL matters most **A Discord webhook URL is a credential, not a setting.** Anyone holding it can post arbitrary messages to that channel, as the webhook's configured name and avatar, with no authentication and no audit trail back to them. A player who copies it can impersonate Quest Board's own notifications in the group's Discord — including fake session cancellations or fake reminders. This is a different problem from **#97**, which validated the URL against a Discord host allowlist on write (SSRF). That one was about where the server may send requests; this is about who may read the value back. `analytics_share_token` is a lesser version of the same shape: creating, rotating and revoking it are correctly `require_gm` (`campaigns.py:2588`, `:2616`), but the existing value is readable by every member, so a player can hand the public analytics link — which carries every member's name and attendance rate — to anyone, and the GM has no way to know it was them. `guild_id` and `notification_channel_id` are ids rather than credentials, and `discord_delivery_status` is already deliberately member-visible with a comment explaining why (`schemas/campaign.py:370-375`) — that reasoning is sound and should stay. ## Threat model, honestly stated Everyone in a campaign is a friend group who already trust each other, and this is self-hosted, so this is not a "drop everything" severity. But: - Campaigns can have members who joined by invite link and are not close friends, and #643 added a join-approval flow precisely because membership is not always tightly held. - A leaked webhook does not need a malicious player — a member pasting a "here's the API response" screenshot into a support thread is enough. - The fix is small and the exposure has no upside: no member-facing feature reads any of these four fields. ## Suggested fix Redact on the way out rather than adding a second endpoint. `to_campaign_response` already receives the viewer; give it the viewer's role and blank the GM-only fields for everyone else: - `discord_webhook_url` → `None` for non-GM - `analytics_share_token` → `None` for non-GM (the *presence* of a share link may be worth surfacing as a plain boolean, since a player arguably deserves to know one exists) - `guild_id`, `notification_channel_id` → GM-only too, on the principle that nothing member-facing reads them; keep `discord_delivery_status` as it is Then check the frontend does not rely on reading them as a non-GM (it should not — the form is GM-only), and add a test asserting a player's `GET /campaigns/{id}` has those fields null. **Worth also auditing the other response schemas for the same shape**, since this one was not deliberate — it is what happens when one response model serves both the "show me the campaign" and "fill in the settings form" jobs. That conflation is the actual root cause, and splitting the two is the more thorough fix if the redaction turns out to be fiddly. ## Where it lives in the redesign No new surface. `CAMP-14`/Settings stays GM-only, which is already how it is drawn in lane 1 (#659). If the "a share link exists" boolean is added, it belongs on `CAMP-7` (Analytics) alongside the existing GM-only share panel. Found by reading the code; **not exploited or tested against any running instance.**

claude-bot added the

backend

security

labels

2026-09-16 03:48:36 +00:00

rbrooks referenced this issue from a commit

2026-09-16 15:43:15 +00:00

fix(backend): the campaign's credentials stop being member-readable (#671)

claude-bot referenced this issue from a pull request that will close it,

2026-09-16 15:44:01 +00:00

fix(backend): redact the campaign webhook URL and share token from non-GM members (#671) #672

claude-bot commented

2026-09-16 16:01:22 +00:00

Author

Collaborator

Fix opened as #672 (branch fix/671-redact-campaign-credentials), not merged — left open for review.

to_campaign_response now resolves the viewer'''s role and blanks discord_webhook_url, analytics_share_token, guild_id and notification_channel_id for anyone who is not a GM of that campaign. Redacted in the builder rather than at the route because it is the only place a CampaignResponse is constructed, so all eight call sites — and the next one anybody adds — are covered by construction.

Decisions taken along the way, each argued in the PR:

The share-link boolean was added, as analytics_share_link_exists. Two lines, no frontend or schema churn, and it follows the pattern already here (llm_api_key_set, bot_token_configured). It is what makes the redaction lossless in what a player is entitled to.
discord_delivery_status untouched, as the issue asks — but worth recording: it names the channel it could not deliver to, so the notification_channel_id redaction is partial by design. The new test says so out loud rather than leaving it to be rediscovered.
GM, not "GM or owner", matching can_manage_membership'''s stated line that the owner-who-is-a-player reads nothing GM-only.

Frontend checked rather than assumed — all four reads are already behind isGm (edit form, FirstRunChecklist, RecordingPanel, the analytics share panel). No frontend change.

The audit you asked for is in the PR body. One finding, low severity: NotificationSettingsResponse.discord_webhook_url (routers/users.py:106) returns the instance fallback webhook in full while the SMTP password beside it is reduced to configured: bool — same shape, but require_admin, so a note rather than a bug. Nothing fixed there.

CI run 9773 green, 3587 passed, 34 skipped.

Fix opened as #672 (branch `fix/671-redact-campaign-credentials`), not merged — left open for review. `to_campaign_response` now resolves the viewer'''s role and blanks `discord_webhook_url`, `analytics_share_token`, `guild_id` and `notification_channel_id` for anyone who is not a GM of that campaign. Redacted in the builder rather than at the route because it is the only place a `CampaignResponse` is constructed, so all eight call sites — and the next one anybody adds — are covered by construction. Decisions taken along the way, each argued in the PR: - **The share-link boolean was added**, as `analytics_share_link_exists`. Two lines, no frontend or schema churn, and it follows the pattern already here (`llm_api_key_set`, `bot_token_configured`). It is what makes the redaction lossless in what a player is entitled to. - **`discord_delivery_status` untouched**, as the issue asks — but worth recording: it names the channel it could not deliver to, so the `notification_channel_id` redaction is partial by design. The new test says so out loud rather than leaving it to be rediscovered. - **GM, not "GM or owner"**, matching `can_manage_membership`'''s stated line that the owner-who-is-a-player reads nothing GM-only. Frontend checked rather than assumed — all four reads are already behind `isGm` (edit form, `FirstRunChecklist`, `RecordingPanel`, the analytics share panel). No frontend change. The audit you asked for is in the PR body. One finding, low severity: `NotificationSettingsResponse.discord_webhook_url` (`routers/users.py:106`) returns the instance fallback webhook in full while the SMTP password beside it is reduced to `configured: bool` — same shape, but `require_admin`, so a note rather than a bug. Nothing fixed there. CI run 9773 green, `3587 passed, 34 skipped`.

claude-bot referenced this issue

2026-09-16 16:05:22 +00:00

[UX] Mock up every catalogued surface, for players and GMs, before building #659

claude-bot closed this issue

2026-09-16 16:55:31 +00:00

claude-bot referenced this issue from a commit

2026-09-16 16:55:31 +00:00

Merge PR #672: the campaign's credentials stop being member-readable (#671)

claude-bot commented

2026-09-16 17:00:41 +00:00

Author

Collaborator

Fixed in #672, merged to main as eb7f286. CI run 9773 green — all 8 jobs, 3587 passed, 34 skipped.

to_campaign_response now resolves the viewer's role and blanks discord_webhook_url, analytics_share_token, guild_id and notification_channel_id for anyone who is not a GM of that campaign. Redacted in the builder rather than at the route, because it is the only place a CampaignResponse is constructed — so all eight call sites are covered, and so is the next one anyone adds.

discord_delivery_status is untouched, as this issue proposed; its comment explaining why a player is owed the delivery reason is intact. analytics_share_link_exists: bool was added, following the llm_api_key_set / bot_token_configured pattern already in the codebase.

Two things deliberately not closed here:

discord_delivery_status echoes the channel id in its documented shape (models/campaign.py:79-88), so redacting notification_channel_id is partial. Called out in the test rather than left to be rediscovered. Closing it is a decision about that dict's shape, not about this fix.
GM, not "GM or owner." Matches invite_service.can_manage_membership's stated line that the owner gets membership management and nothing that reads campaign content. A non-GM owner now gets redacted fields from the owner-gated routes; nothing on those pages reads them.

The audit this issue asked for turned up one thing of the same shape, filed separately, plus one adjacent PII question. Neither was fixed here.

Closing.

Fixed in **#672**, merged to `main` as `eb7f286`. CI run 9773 green — all 8 jobs, `3587 passed, 34 skipped`. `to_campaign_response` now resolves the viewer's role and blanks `discord_webhook_url`, `analytics_share_token`, `guild_id` and `notification_channel_id` for anyone who is not a GM of that campaign. Redacted in the builder rather than at the route, because it is the only place a `CampaignResponse` is constructed — so all eight call sites are covered, and so is the next one anyone adds. `discord_delivery_status` is untouched, as this issue proposed; its comment explaining why a player is owed the delivery reason is intact. `analytics_share_link_exists: bool` was added, following the `llm_api_key_set` / `bot_token_configured` pattern already in the codebase. Two things deliberately **not** closed here: - **`discord_delivery_status` echoes the channel id** in its documented shape (`models/campaign.py:79-88`), so redacting `notification_channel_id` is partial. Called out in the test rather than left to be rediscovered. Closing it is a decision about that dict's shape, not about this fix. - **GM, not "GM or owner."** Matches `invite_service.can_manage_membership`'s stated line that the owner gets membership management and nothing that reads campaign content. A non-GM owner now gets redacted fields from the owner-gated routes; nothing on those pages reads them. The audit this issue asked for turned up one thing of the same shape, filed separately, plus one adjacent PII question. Neither was fixed here. Closing.

claude-bot referenced this issue

2026-09-16 17:01:40 +00:00

[Backend][Security] The instance-wide Discord webhook is returned in full to admins, while the SMTP password beside it is not #676

claude-bot referenced this issue

2026-09-16 17:01:56 +00:00

[Backend][Privacy] Every campaign member can read every other member's email address #677

rbrooks referenced this issue from a commit

2026-09-23 10:18:16 +00:00

feat(webapp): Settings — Discord, and Recording, retention & AI (#729, lane 3)

claude-bot referenced this issue

2026-09-23 10:18:44 +00:00

Lane 3: Settings — Discord, and Recording, retention & AI (#729, CAMP-12, CAMP-13) #776