fix(security): gate spoofable X-Forwarded-Host + the vm-create config path - #38
fix(security): gate spoofable X-Forwarded-Host + the vm-create config path#38Ailcope wants to merge 3 commits into
Conversation
X-Forwarded-Host is attacker-controlled on a direct request, but the issuer builder, the dashboard host resolution, the tokens/connectors pages and the mint flow all trusted it unconditionally -- mirroring the X-Forwarded-For handling that client_ip() already gates on trusted_proxies. Add ratelimit.forwarded_host(), which honours X-Forwarded-Host only when the direct peer is a declared trusted proxy and otherwise falls back to the request's own Host header. Route every host-building call site through it. Scheme handling (X-Forwarded-Proto) is deliberately left untouched: a TLS-terminating edge (Cloudflare tunnel, nginx) legitimately needs it to report https even with trusted_proxies unset, and gating it would silently downgrade the Secure-cookie flag and the OAuth issuer to http.
proxmox_vm_create forwards its `config` dict straight to the PVE API. A config can carry code-execution keys -- `hookscript` (a script PVE runs on VM lifecycle events) or raw QEMU `args` -- so an injected instruction in a chat turn could create+start a VM that runs code on the host, without ever hitting the approval modal that gates ssh_run / proxmox_run / proxmox_vm_config(updates=...). Add proxmox_vm_create to _CONFIRM_WHEN_ARG_PRESENT keyed on `config`, mirroring the proxmox_vm_config `updates` treatment: `config` is a declared parameter, so reading it is sound, and gating on its presence leaves the bare no-config create (a harmless empty shell) unattended.
There was a problem hiding this comment.
Pull request overview
Hardens BeaconMCP’s reverse-proxy trust model and the dashboard chat approval gate by (1) only honoring X-Forwarded-Host when the direct peer is a configured trusted proxy, and (2) requiring human confirmation when proxmox_vm_create is invoked with a config payload.
Changes:
- Added
ratelimit.forwarded_host()and switched dashboard/mint/issuer URL construction to use it withtrusted_proxies. - Extended the chat approval gate so
proxmox_vm_create(config=...)triggers confirmation, plus updated gating coverage tests. - Added tests covering the new forwarded-host trust behavior and the vm-create-with-config confirmation behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/beaconmcp/ratelimit.py |
Adds forwarded_host() to gate X-Forwarded-Host on trusted direct proxies. |
src/beaconmcp/dashboard/app.py |
Uses forwarded_host() when constructing externally-facing URLs in the dashboard. |
src/beaconmcp/__main__.py |
Uses forwarded_host() for OAuth issuer construction. |
src/beaconmcp/dashboard/chat.py |
Adds proxmox_vm_create to the “confirm when arg present” gate. |
tests/test_ratelimit.py |
Adds unit tests for forwarded-host trust behavior. |
tests/test_dashboard_chat.py |
Adds tests for vm-create-with-config confirmation + updates tool gating classification list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scheme = request.headers.get("x-forwarded-proto", request.url.scheme) | ||
| host_hdr = request.headers.get( | ||
| "x-forwarded-host", request.headers.get("host", "localhost"), | ||
| ) | ||
| host_hdr = forwarded_host(request, deps.trusted_proxies) | ||
| url = f"{scheme}://{host_hdr}/mcp/c/{row.slug}" |
| # ``proxmox_vm_create`` is here for the same reason: a bare create is a | ||
| # harmless empty shell, but a ``config`` dict can carry code-execution keys | ||
| # -- ``hookscript`` (a script PVE runs on VM lifecycle events) or raw QEMU | ||
| # ``args`` -- which an injected instruction could set to run code on the PVE | ||
| # host. ``config`` is a declared parameter of the tool, so reading it is | ||
| # sound; gating only on its presence keeps the no-config create ungated. |
Showdown76py
left a comment
Showdown76py
left a comment
There was a problem hiding this comment.
Reviewed both commits against main@87caf1a, checked the claims in the code, ran the suite locally (510 passed, ruff check src/ tests/ clean) and reproduced the runtime behaviour of the new helper through uvicorn's actual middleware. CI is 6/6 green; mergeable_state: blocked is the required review, not a failure.
Both findings are real. The vm-create one I would merge as is. The X-Forwarded-Host one needs a change before it does what it says.
proxmox_vm_create gate: good. config is splatted straight into the PVE API call, hookscript and raw QEMU args are host code execution, and the codebase had already written that reasoning down for proxmox_vm_config in _PANEL_CONFIG_KEYS while leaving the same dict ungated one tool over. The gate is additive, the guard test move is coherent. One undocumented side effect on the panel relay, noted inline.
X-Forwarded-Host gate: the trusted-proxy branch cannot open in the running server. uvicorn.run() defaults to proxy_headers=True / forwarded_allow_ips="127.0.0.1", and its ProxyHeadersMiddleware rewrites scope["client"] to the XFF client before the app sees the request. So request.client.host is the public client IP, never the declared proxy, and forwarded_host drops X-Forwarded-Host even for a proxy listed in trusted_proxies. Verified against the real middleware:
with uvicorn proxy-headers (default ON): client=203.0.113.5 forwarded_host=127.0.0.1:8420
without the middleware: client=127.0.0.1 forwarded_host=beacon.example.com
It fails closed, so nothing is less safe. But on a deployment whose proxy does not preserve Host, _issuer() now advertises http://127.0.0.1:8420 in the OAuth metadata and clients cannot complete discovery. The fake _Req objects in tests/test_ratelimit.py bypass uvicorn, which is why the suite stays green. Fix is one argument to uvicorn.run (proxy_headers=False, or forwarded_allow_ips fed from trusted_proxies), plus a test through the real ASGI stack.
Two smaller points inline: the left-most chain entry contradicts client_ip's right-most walk, and trusted_proxies is documented as XFF-only in three places that this commit silently widens.
Worth stating plainly about the security value: allowed_hosts is enforced only on the MCP transport, so a direct request with a forged Host still poisons the issuer without any X-Forwarded-Host involved. The inconsistency this closes is real, but the poisoning itself needs the server.public_url anchor already listed under "Known and left alone". The passkeys.py deferral is sound: the browser binds the assertion to the real origin, so a spoofed host breaks a login rather than forging one.
Suggestion: land commit 2 now, revise commit 1 for the uvicorn interaction first.
Generated by Claude Code
| client = getattr(request, "client", None) | ||
| direct_peer = getattr(client, "host", None) if client is not None else None | ||
| direct_ip = _coerce_ip(str(direct_peer)) if direct_peer is not None else None | ||
| if direct_ip and _is_trusted_proxy(direct_ip, trusted_proxies): |
There was a problem hiding this comment.
This branch cannot open in the running server, and the fallback can break OAuth discovery.
__main__.py:2246 calls uvicorn.run(app, host=host, port=port, log_level="info"). uvicorn defaults to proxy_headers=True with forwarded_allow_ips="127.0.0.1", and its ProxyHeadersMiddleware rewrites scope["client"] to the client address taken from X-Forwarded-For whenever the direct peer is loopback. So by the time this function runs, request.client.host is the public client IP, never the proxy, and _is_trusted_proxy returns False even for a proxy that is correctly declared in trusted_proxies.
Reproduced against the real middleware (same-host nginx, trusted_proxies=("127.0.0.1", "::1")):
with uvicorn proxy-headers (default ON): client=203.0.113.5 forwarded_host=127.0.0.1:8420
without the middleware: client=127.0.0.1 forwarded_host=beacon.example.com
Failing closed is fine for the attack case, but it also means a declared proxy is never believed. On a deployment whose proxy does not preserve Host (proxy_pass without proxy_set_header Host $host;) _issuer() now returns http://127.0.0.1:8420, so /.well-known/oauth-authorization-server advertises an unreachable issuer and MCP clients stop being able to complete the flow. That is a behaviour change for existing installs, not just hardening.
The tests do not catch it because _Req in tests/test_ratelimit.py is a hand built object that never goes through uvicorn.
Two options, either works:
uvicorn.run(..., proxy_headers=False). The app already implements its own XFF trust model inclient_ip, and every scheme read goes through thex-forwarded-protoheader directly rather thanrequest.url.schemealone, so nothing else regresses.uvicorn.run(..., forwarded_allow_ips=",".join(config.server.trusted_proxies))and keep uvicorn as the single owner of that decision.
Either way it would be worth one test that drives a request through the real ASGI stack, so the fake-request tests cannot drift from runtime behaviour again.
Generated by Claude Code
| # A proxy chain may append entries; the first is the client-facing host. | ||
| first = fwd.split(",")[0].strip() |
There was a problem hiding this comment.
Minor, but this is the one place where the docstring's "mirrors client_ip's trust model" does not hold. client_ip walks the chain right to left precisely so a client supplied left-most entry cannot win when a proxy appends. Here the left-most entry is returned, so a proxy that appends to X-Forwarded-Host rather than overwriting it hands the attacker value straight back.
Most proxies do overwrite (nginx proxy_set_header X-Forwarded-Host $host;, Traefik, Caddy), so this is a small residual, but the test at tests/test_ratelimit.py pins the left-most choice as intended behaviour. Either take the last entry for symmetry with client_ip, or say in the docstring that the trusted proxy is assumed to overwrite rather than append.
Generated by Claude Code
| host_header = forwarded_host( | ||
| request, tuple(config.server.trusted_proxies), | ||
| ) |
There was a problem hiding this comment.
Two notes on the call site itself, both about scope rather than correctness.
trusted_proxies now governs how the issuer host is derived, not just X-Forwarded-For, and the failure mode when it is unset is a wrong discovery URL rather than a coarser rate-limit key. The three places that document the field still describe it as XFF only, so they drift with this commit: beaconmcp.yaml.example:44 ("Trust X-Forwarded-For only from direct peers you operate"), docs/configuration.md:39, docs/cloudflare.md:127.
Second, worth stating in the PR description so the residual is not read as closed: allowed_hosts is only enforced by the MCP transport (server.py:129, TransportSecuritySettings), so /oauth/* and /app/* have no Host validation at all. An attacker reaching the server directly can still send Host: evil.example and get the same poisoned issuer out of this function, without touching X-Forwarded-Host. The header inconsistency this commit closes is real, but the poisoning itself only really goes away with the server.public_url anchor listed under "Known and left alone".
Generated by Claude Code
| _CONFIRM_WHEN_ARG_PRESENT: dict[str, str] = { | ||
| "proxmox_vm_config": "updates", | ||
| "beaconmcp_self_update": "confirm", | ||
| "proxmox_vm_create": "config", |
There was a problem hiding this comment.
This one is right and I would take it as is.
The mechanism holds: proxmox_vm_create is not in _NEEDS_CONFIRMATION, so the early return in _tool_call_requires_confirmation adds a gate rather than replacing a stronger one, and vms.py:135 does create_params = config or {} then client.post(..., **create_params), so the dict really does reach the PVE API untouched. The repo had already reached this conclusion for the same keys one tool over: the _PANEL_CONFIG_KEYS note below names hookscript and raw QEMU args as the reason proxmox_vm_config cannot be panel exempt, while the identical dict was going through vm_create unattended. Closing that is consistent.
One side effect that is not in the description and is worth a line there: panel_call_allowed falls through to _tool_call_requires_confirmation, so a panel can no longer call proxmox_vm_create with a config on its own. No shipped panel does, so nothing breaks today, but it is a real change to the relay's allow set.
Generated by Claude Code
|
The uvicorn finding is right, and it's the one that matters -- I wrote and tested I'll take On the chain walk: you're right, take the last entry, not the first. Docs drift: agreed -- Residual stated plainly, into the description: Two smaller ones:
Plan matches your suggestion: commit 2 stands as is, I'll revise commit 1 ( |
The trusted-proxy branch added in 6b531df could never open in the running server. uvicorn.run defaults to proxy_headers=True / forwarded_allow_ips="127.0.0.1", so ProxyHeadersMiddleware rewrites scope["client"] to the X-Forwarded-For client before the app runs, and request.client.host is never the declared proxy. It failed closed, but "closed" meant _issuer() advertised http://127.0.0.1:8420 on any proxy that does not preserve Host -- an OAuth discovery regression, not hardening. The hand-built _Req tests missed it by bypassing the ASGI stack. - __main__: uvicorn.run(proxy_headers=False). The app already owns its forwarded-header trust (client_ip for XFF, forwarded_host for XFH, both on the real peer) and reads x-forwarded-proto directly, so uvicorn must not pre-rewrite the peer. - ratelimit.forwarded_host: take the last X-Forwarded-Host entry, not the first, so a proxy that appends cannot hand back a client-supplied prefix -- symmetric with client_ip's right-to-left walk. - tests: drive forwarded_host through a real Starlette Request so the hand-built _Req cases cannot drift from runtime again. - docs: trusted_proxies now governs the advertised host too, not just XFF (beaconmcp.yaml.example, configuration.md, cloudflare.md). - chat.py: tighten the vm-create gate comment -- the check is bool(config), so an empty {} is ungated like a bare create.
|
Pushed
On passkeys -- I said I'd thread
|
A re-audit of
mainat87caf1a, after the MCP Apps panels (#34), the update notifier + self-update tools (#37) and passkey sign-in (#36) landed. Two findings, one commit each, plus a post-review revision of the first (commit 3) after @Showdown76py caught that its trusted-proxy branch could not open under uvicorn's proxy-headers middleware.Why
A fresh read of
src/on the currentmain-- not a diff of one branch. The surface held up as it did in #24: OAuth PKCE S256 is mandatory with TOTP replay protection,redirect_uriis allowlisted at both/authorizeand the DCR register endpoint, SQL is parameterised throughout,client_idisolation is intact on conversations / usage / tokens / DCR slugs, shell construction is quoted or argv (pct/qmviashlex.quote, the QEMU agent via argv,ipmitoolvia argv +IPMI_PASSWORD), the_staging_pathtraversal guard is sound, sessions are AES-GCM, and the new MCP-Apps panel relay is correctly gated -- its sandboxed iframe carries no cookie or CSRF token, sopanel_call_allowedon the server is the boundary, and it authorises nothing the model could not already call without a modal.Two seams were left open, one of which #24 flagged by name and deferred.
_issuer()trustedX-Forwarded-Hostunconditionally. #24 listed this under Known and left alone -- OAuth discovery URLs poisonable in theory, but the client sets its own Host andserver.allowed_hostscovers/mcp. The seam is the inconsistency:X-Forwarded-Foris already validated againstserver.trusted_proxiesinratelimit.client_ip, whileX-Forwarded-Hostwas believed from any peer -- and since then the same header also builds the "paste this MCP URL" strings on the tokens / connectors pages. On a directly-exposed deployment, or behind a proxy that does not strip a client-supplied value, the header is attacker-set.The confirmation gate covered
proxmox_vm_config(updates=...)but notproxmox_vm_create(config=...). #24 added_CONFIRM_WHEN_ARG_PRESENTfor exactly this shape -- a tool that reads without an arg and mutates with it.proxmox_vm_createforwards itsconfigdict straight to the PVE API, and a config can carryhookscript(a script PVE runs on VM lifecycle events) or raw QEMUargs: code execution on the PVE host. An injected instruction in a chat turn could create + start such a VM with no modal, while the equivalentproxmox_runwas blocked -- the same "exec through the side door" shape #24 closed forproxmox_write_file.Changes
X-Forwarded-Host trust (
ratelimit.py,__main__.py,dashboard/app.py)ratelimit.forwarded_host(), mirroringclient_ip's trust model:X-Forwarded-Hostis honoured only when the direct peer is a declaredtrusted_proxy, otherwise the request's ownHostheader wins. A chain takes the last entry (the nearest proxy's value), so a proxy that appends cannot hand back a client-supplied prefix -- symmetric withclient_ip's right-to-left walk._issuerand the three dashboard host builders (_resolve_mcp_url, the tokens page,connectors_mint).uvicorn.run(proxy_headers=False)(commit 3). uvicorn defaults toproxy_headers=True/forwarded_allow_ips="127.0.0.1", so itsProxyHeadersMiddlewarerewritesscope["client"]to theX-Forwarded-Forclient before the app runs --request.client.hostis then never the declared proxy, the trusted-proxy branch can never open, and_issuer()would advertisehttp://127.0.0.1:8420behind any proxy that does not preserveHost. The app already owns its forwarded-header trust end to end (client_ipfor XFF,forwarded_hostfor XFH, both on the real peer) and readsx-forwarded-protodirectly, so uvicorn must not interpret the forwarded headers for us.X-Forwarded-Protois left untouched: a TLS-terminating edge (Cloudflare Tunnel, nginx) legitimately needs it to reporthttpseven whentrusted_proxiesis unset, so gating it would silently downgrade theSecurecookie flag and the OAuth issuer tohttp. WebAuthn origin / RP-ID derivation is also left as-is: I looked at threadingtrusted_proxiesthroughpasskeys.pyfor consistency, but it runs on the register / authenticate path and the browser binds the assertion to the real origin -- a spoofed host breaks a login rather than forging one -- so it is a separate, testable change across every passkey call site, not something to fold quietly into a security fix.Confirmation gate (
dashboard/chat.py)proxmox_vm_createadded to_CONFIRM_WHEN_ARG_PRESENT, keyed onconfig, mirroring theproxmox_vm_config/updatestreatment. A bare create -- noconfig, or an empty{}-- stays unattended; a create carrying a non-emptyconfigraises the modal.configis a declared parameter of the tool, so reading it is sound.Known and left alone
Hostvalidation on/oauth/*and/app/*--allowed_hostsis enforced only by the MCP transport (server.py,TransportSecuritySettings), so a direct request with a forgedHoststill poisons the issuer without anyX-Forwarded-Hostat all. This PR closes the header inconsistency; the poisoning itself only fully goes away with an explicitserver.public_urlanchoring scheme + host, which is a config addition rather than a patch.panel_call_allowedfalls through to the confirmation gate, so a panel can no longer callproxmox_vm_createwith aconfigon its own. No shipped panel does, so nothing breaks today, but it is a real narrowing of the relay's allow set and belongs on the record.X-Forwarded-Prototrust -- load-bearing for TLS-terminating edges, as above; theserver.public_urlanchor is the real fix.proxmox verify_ssl/ BMCverify_tlsdefaultfalse-- BMCs and homelab PVE ship self-signed; fix(security): close findings from a full security audit #24 madeverify_tlsparse for real and documented it. Flipping the default is an operator decision./metricsunauthenticated -- unchanged since fix(security): close findings from a full security audit #24: network-ACL-controlled, no label leaks a secret.known_hostsunset accepts any key -- the documented trusted-LAN default; per-hoststrict_host_key_checkingis already available for public targets.Related
Follows the audit in #24: closes its deferred
_issuer/X-Forwarded-Hostitem and extends the_CONFIRM_WHEN_ARG_PRESENTmechanism it introduced. Re-audits the surface added by #34 (MCP Apps panels), #37 (self-update tools) and #36 (passkeys).Tests
475 passed, 1 skipped(+3),ruff check src/ tests/clean, Python 3.12.test_forwarded_host_only_trusts_declared_proxy(tests/test_ratelimit.py) pins the trust model -- an untrusted peer'sX-Forwarded-Hostis dropped, a trusted proxy's is believed, the last entry of a chain wins, and theHostfallback holds;test_forwarded_host_through_a_real_starlette_requestdrives the helper through an actual StarletteRequestbuilt from an ASGI scope, so the hand-built request cases cannot drift from runtime again;test_vm_create_with_config_needs_confirmation(tests/test_dashboard_chat.py) asserts a config-bearing create gates while a bare create does not.