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

New issue

Closed

opened 2026-09-16 17:01:40 +00:00 by claude-bot · 2 comments

claude-bot commented

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

Collaborator

Found by the audit #671 asked for, during the fix in #672. Reported, not fixed — the audience is narrow enough that this is a consistency question rather than a leak.

What happens

GET /api/admin/settings/notifications (webapp/backend/app/routers/users.py:636-657) returns NotificationSettingsResponse, which carries discord_webhook_url in full (users.py:106).

That is the same shape as #671 — one response model serving both "show me the settings" and "fill in the settings form", so a credential comes back in the response body — but the route is require_admin, and the admin is the person who set the value. So the exposure is small.

Why it is still worth a decision

The SMTP password in the same response is already reduced to a boolean (users.py:101, configured: bool). So the model is internally inconsistent: one credential is returned, the one next to it is not, and nothing in the code says why. Whoever reads it next has to guess which is the intended pattern.

A Discord webhook URL is a credential in the same sense the SMTP password is — anyone holding it can post to that channel as Quest Board, unauthenticated and untraceably. The instance-wide one is the fallback used by every campaign that has not set its own, so it is the higher-value of the two.

Options

Match the SMTP treatment: discord_webhook_configured: bool, never return the URL. Needs a look at whether the admin form relies on reading it back to edit it — the campaign-level form did not, which is why #672's redaction was clean.
Leave it, and write down why. A comment saying "the admin set this and may need to read it back" is a legitimate answer; the problem is that there is no comment either way.

Option 1 is the consistent one. Option 2 is honest if there is a real reason.

Checked and clean during the same audit

Listed so nobody repeats the sweep — 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.
routers/campaigns.py:6820-6845 — CampaignAiSettingsResponse: booleans only; docstring reads "What a GM may see. Never a key, in any form".
routers/users.py:164-190 — BotSettingsResponse: bot_token_configured, api_key_configured.
routers/admin.py:861, :985-986 — the admin campaign inventory boolean-ises has_guild / has_notification_channel, and tests/test_admin_campaign_inventory.py:395-427 pins the key set and scans the serialised body for the webhook, share token, invite code and description. This is the house pattern; it is worth citing as such.
InviteResponse.code — a credential, behind require_membership_manager (campaigns.py:1405-1413). Correct.
routers/bot.py:55 webhook_url — behind require_bot_auth; the bot is the intended consumer.
routers/public.py:35-61 — the unauthenticated analytics response returns neither the share token nor any Discord field.
schemas/session.py:164 recording_url — only ever populated by import (import_service.py:276); not a URL with an embedded secret in practice.

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

Found by the audit #671 asked for, during the fix in #672. **Reported, not fixed** — the audience is narrow enough that this is a consistency question rather than a leak. ## What happens `GET /api/admin/settings/notifications` (`webapp/backend/app/routers/users.py:636-657`) returns `NotificationSettingsResponse`, which carries `discord_webhook_url` in full (`users.py:106`). That is the same shape as #671 — one response model serving both "show me the settings" and "fill in the settings form", so a credential comes back in the response body — but the route is `require_admin`, and the admin is the person who set the value. So the exposure is small. ## Why it is still worth a decision **The SMTP password in the same response is already reduced to a boolean** (`users.py:101`, `configured: bool`). So the model is internally inconsistent: one credential is returned, the one next to it is not, and nothing in the code says why. Whoever reads it next has to guess which is the intended pattern. A Discord webhook URL is a credential in the same sense the SMTP password is — anyone holding it can post to that channel as Quest Board, unauthenticated and untraceably. The instance-wide one is the fallback used by every campaign that has not set its own, so it is the higher-value of the two. ## Options 1. **Match the SMTP treatment**: `discord_webhook_configured: bool`, never return the URL. Needs a look at whether the admin form relies on reading it back to edit it — the campaign-level form did not, which is why #672's redaction was clean. 2. **Leave it, and write down why.** A comment saying "the admin set this and may need to read it back" is a legitimate answer; the problem is that there is no comment either way. Option 1 is the consistent one. Option 2 is honest if there is a real reason. ## Checked and clean during the same audit Listed so nobody repeats the sweep — 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. - `routers/campaigns.py:6820-6845` — `CampaignAiSettingsResponse`: booleans only; docstring reads "What a GM may see. Never a key, in any form". - `routers/users.py:164-190` — `BotSettingsResponse`: `bot_token_configured`, `api_key_configured`. - `routers/admin.py:861`, `:985-986` — the admin campaign inventory boolean-ises `has_guild` / `has_notification_channel`, and `tests/test_admin_campaign_inventory.py:395-427` pins the key set *and* scans the serialised body for the webhook, share token, invite code and description. **This is the house pattern; it is worth citing as such.** - `InviteResponse.code` — a credential, behind `require_membership_manager` (`campaigns.py:1405-1413`). Correct. - `routers/bot.py:55` `webhook_url` — behind `require_bot_auth`; the bot is the intended consumer. - `routers/public.py:35-61` — the unauthenticated analytics response returns neither the share token nor any Discord field. - `schemas/session.py:164` `recording_url` — only ever populated by import (`import_service.py:276`); not a URL with an embedded secret in practice. Found by reading the code; not exploited or tested against any running instance.

claude-bot referenced this issue

2026-10-12 23:26:16 +00:00

[Hosted] The launch gate #1153

claude-bot added this to the v4.7.0 - Browse the Rules milestone

2026-10-27 17:38:55 +00:00

claude-bot commented

2026-10-30 19:29:50 +00:00

Author

Collaborator

The owner's decision (2026-10-30): option 1, as recommended. Treat the instance-wide Discord webhook like the SMTP password: GET /api/admin/settings/notifications returns discord_webhook_configured: bool and never the URL. The admin form shows whether one is set, replaces it when a new one is typed, and keeps it when left blank. For v4.7.0.

**The owner's decision (2026-10-30): option 1**, as recommended. Treat the instance-wide Discord webhook like the SMTP password: `GET /api/admin/settings/notifications` returns `discord_webhook_configured: bool` and never the URL. The admin form shows whether one is set, replaces it when a new one is typed, and keeps it when left blank. For v4.7.0.

claude-bot commented

2026-10-30 19:31:17 +00:00

Author

Collaborator

Already resolved; closing (2026-10-30). When picked up for the owner's option 1, the instance-wide webhook this issue describes turned out not to exist any more on main:

e9aebde5 ("remove D67 Discord fallbacks") dropped discord_webhook_url from GET/PUT /api/admin/settings/notifications. NotificationSettingsResponse now carries only smtp.
Migration c4e6a8b0d2f1_drop_stored_discord_webhook_fallback deleted the stored default_discord_webhook row.
test_admin_notification_settings_ignores_discord_webhook_url (tests/test_webhook_ssrf.py:116) shows a PUT carrying the field is ignored and the key is absent from the response.

Also checked: the per-campaign webhook is returned only to that campaign's GMs (campaign_service.py:283, discord_webhook_url=… if viewer_is_gm else None), who set it themselves. Players never receive it, so nothing more is needed there. No code change; no changelog entry.

**Already resolved; closing (2026-10-30).** When picked up for the owner's option 1, the instance-wide webhook this issue describes turned out not to exist any more on `main`: - `e9aebde5` ("remove D67 Discord fallbacks") dropped `discord_webhook_url` from GET/PUT `/api/admin/settings/notifications`. `NotificationSettingsResponse` now carries only `smtp`. - Migration `c4e6a8b0d2f1_drop_stored_discord_webhook_fallback` deleted the stored `default_discord_webhook` row. - `test_admin_notification_settings_ignores_discord_webhook_url` (`tests/test_webhook_ssrf.py:116`) shows a PUT carrying the field is ignored and the key is absent from the response. Also checked: the **per-campaign** webhook is returned only to that campaign's GMs (`campaign_service.py:283`, `discord_webhook_url=… if viewer_is_gm else None`), who set it themselves. Players never receive it, so nothing more is needed there. No code change; no changelog entry.

claude-bot closed this issue

2026-10-30 19:31:17 +00:00