fix(security): gate spoofable X-Forwarded-Host + the vm-create config path

[Ailcope]

A re-audit of main at 87caf1a, 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 current main -- 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_uri is allowlisted at both /authorize and the DCR register endpoint, SQL is parameterised throughout, client_id isolation is intact on conversations / usage / tokens / DCR slugs, shell construction is quoted or argv (pct / qm via shlex.quote, the QEMU agent via argv, ipmitool via argv + IPMI_PASSWORD), the _staging_path traversal 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, so panel_call_allowed on 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() trusted X-Forwarded-Host unconditionally. #24 listed this under Known and left alone -- OAuth discovery URLs poisonable in theory, but the client sets its own Host and server.allowed_hosts covers /mcp. The seam is the inconsistency: X-Forwarded-For is already validated against server.trusted_proxies in ratelimit.client_ip, while X-Forwarded-Host was 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 not proxmox_vm_create(config=...). #24 added _CONFIRM_WHEN_ARG_PRESENT for exactly this shape -- a tool that reads without an arg and mutates with it. proxmox_vm_create forwards its config dict straight to the PVE API, and a config can carry hookscript (a script PVE runs on VM lifecycle events) or raw QEMU args: code execution on the PVE host. An injected instruction in a chat turn could create + start such a VM with no modal, while the equivalent proxmox_run was blocked -- the same "exec through the side door" shape #24 closed for proxmox_write_file.

Changes

X-Forwarded-Host trust (ratelimit.py, __main__.py, dashboard/app.py)

• New ratelimit.forwarded_host(), mirroring client_ip's trust model: X-Forwarded-Host is honoured only when the direct peer is a declared trusted_proxy, otherwise the request's own Host header 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 with client_ip's right-to-left walk.
• Applied to _issuer and the three dashboard host builders (_resolve_mcp_url, the tokens page, connectors_mint).
• uvicorn.run(proxy_headers=False) (commit 3). uvicorn defaults to proxy_headers=True / forwarded_allow_ips="127.0.0.1", so its ProxyHeadersMiddleware rewrites scope["client"] to the X-Forwarded-For client before the app runs -- request.client.host is then never the declared proxy, the trusted-proxy branch can never open, and _issuer() would advertise http://127.0.0.1:8420 behind any proxy that does not preserve Host. The app already owns its forwarded-header trust end to end (client_ip for XFF, forwarded_host for XFH, both on the real peer) and reads x-forwarded-proto directly, so uvicorn must not interpret the forwarded headers for us.
• Deliberately host-only. X-Forwarded-Proto is left untouched: a TLS-terminating edge (Cloudflare Tunnel, nginx) legitimately needs it to report https even when trusted_proxies is unset, so gating it would silently downgrade the Secure cookie flag and the OAuth issuer to http. WebAuthn origin / RP-ID derivation is also left as-is: I looked at threading trusted_proxies through passkeys.py for 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_create added to _CONFIRM_WHEN_ARG_PRESENT, keyed on config, mirroring the proxmox_vm_config / updates treatment. A bare create -- no config, or an empty {} -- stays unattended; a create carrying a non-empty config raises the modal. config is a declared parameter of the tool, so reading it is sound.

Known and left alone

• Host validation on /oauth/* and /app/* -- allowed_hosts is enforced only by the MCP transport (server.py, TransportSecuritySettings), so a direct request with a forged Host still poisons the issuer without any X-Forwarded-Host at all. This PR closes the header inconsistency; the poisoning itself only fully goes away with an explicit server.public_url anchoring scheme + host, which is a config addition rather than a patch.
• Panel allow-set -- panel_call_allowed falls through to the confirmation gate, 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 narrowing of the relay's allow set and belongs on the record.
• X-Forwarded-Proto trust -- load-bearing for TLS-terminating edges, as above; the server.public_url anchor is the real fix.
• proxmox verify_ssl / BMC verify_tls default false -- BMCs and homelab PVE ship self-signed; fix(security): close findings from a full security audit #24 made verify_tls parse for real and documented it. Flipping the default is an operator decision.
• /metrics unauthenticated -- unchanged since fix(security): close findings from a full security audit #24: network-ACL-controlled, no label leaks a secret.
• SSH known_hosts unset accepts any key -- the documented trusted-LAN default; per-host strict_host_key_checking is already available for public targets.

Related

Follows the audit in #24: closes its deferred _issuer / X-Forwarded-Host item and extends the _CONFIRM_WHEN_ARG_PRESENT mechanism 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's X-Forwarded-Host is dropped, a trusted proxy's is believed, the last entry of a chain wins, and the Host fallback holds; test_forwarded_host_through_a_real_starlette_request drives the helper through an actual Starlette Request built 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.

[Copilot]

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 with trusted_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.

[Showdown76py]

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

[Showdown76py]

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 in client_ip, and every scheme read goes through the x-forwarded-proto header directly rather than request.url.scheme alone, 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

[Showdown76py]

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

[Showdown76py]

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

[Showdown76py]

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

[Ailcope]

The uvicorn finding is right, and it's the one that matters -- I wrote and tested forwarded_host against a hand-built _Req and never drove it through the ASGI stack it actually runs behind, so I missed that ProxyHeadersMiddleware gets there first. On defaults (proxy_headers=True, forwarded_allow_ips="127.0.0.1") scope["client"] is already rewritten to the XFF client by the time _issuer runs, so request.client.host is never the declared proxy and the trusted-proxy branch can't open. It fails closed, but "closed" here means _issuer() hands back http://127.0.0.1:8420 on any proxy that doesn't preserve Host -- a discovery regression for existing installs, not hardening. Confirmed on my side: __main__.py:2246 passes neither flag, so uvicorn owns the rewrite.

I'll take proxy_headers=False over forwarded_allow_ips. The app already owns its forwarded-header trust end to end -- client_ip does its own right-to-left XFF walk and every scheme read goes through the x-forwarded-proto header directly -- so letting uvicorn also rewrite client just gives two owners for one decision. Turning it off puts request.client.host back to the real TCP peer, which is exactly what both client_ip and forwarded_host are written to expect. Plus a test through the real ASGI stack so the fake-_Req cases can't drift from runtime again.

On the chain walk: you're right, take the last entry, not the first. client_ip walks right-to-left precisely so an appended client value can't win, and returning the left-most X-Forwarded-Host hands it straight back on any proxy that appends instead of overwriting. Last entry restores the symmetry and collapses to the same value on the overwrite case, so there's no downside.

Docs drift: agreed -- trusted_proxies now governs the issuer host, not just XFF, so the three call-outs (beaconmcp.yaml.example:44, docs/configuration.md:39, docs/cloudflare.md:127) need to say so rather than describing it as XFF-only.

Residual stated plainly, into the description: allowed_hosts only runs on the MCP transport, so /oauth/* and /app/* take a forged Host directly -- the header consistency this closes is real, but the poisoning itself only goes away with the server.public_url anchor still under Known and left alone. Same for the panel side effect you flagged on commit 2: panel_call_allowed now falls through the create-with-config gate, so a panel can't mint that call on its own -- no shipped panel does, but it's a real narrowing of the relay allow-set and belongs in the description, not just the diff.

Two smaller ones:

• passkeys.py:_forwarded_host() -- I'll thread trusted_proxies through it for consistency while I'm in the helper, but I agree with your read that it's cosmetic rather than a fix: the browser binds the assertion to the real origin, so a spoofed host breaks a login rather than forging one.
• chat.py comment -- the truthiness is intended (an empty config is a bare shell, no hookscript/args, nothing to gate), but "presence" is the wrong word for it; I'll say "a non-empty config" so the comment matches bool(args.get("config")).

Plan matches your suggestion: commit 2 stands as is, I'll revise commit 1 (proxy_headers=False + last-entry + docs + a real-ASGI test) and repush, with the two residual lines added to the description. Happy to split them into separate PRs if you'd rather land the vm-create gate first -- your call.

[Ailcope]

Pushed 8ec9a58. I kept commit 2 untouched and layered the commit-1 revision on top rather than rewriting it, so the "good as is" one stays a clean standalone -- squash on merge and the XFH story collapses to one change either way.

• uvicorn.run(proxy_headers=False). Confirmed your repro from the other side: on defaults request.client.host is the XFF client, so the trusted-proxy branch never opened and _issuer() fell back to http://127.0.0.1:8420. Turning the middleware off puts the real peer back, which is what both client_ip and forwarded_host are written to expect -- and it's the more correct state anyway, since it makes client_ip's own trusted-proxy walk actually reachable instead of leaning on uvicorn to have done it.
• forwarded_host now takes the last X-Forwarded-Host entry, not the first, per your chain-walk point -- a proxy that appends can't hand the client-supplied prefix back, and the overwrite case collapses to a single entry so nothing changes there.
• Docs: fixed the three call-outs (beaconmcp.yaml.example, docs/configuration.md, docs/cloudflare.md) so trusted_proxies reads as governing the advertised host, not just XFF.
• The _Req gap: added a test that drives forwarded_host through a real Starlette Request from an ASGI scope, so the fake requests can't quietly drift from runtime again.
• Description now carries the two residuals plainly -- allowed_hosts only running on the MCP transport (so /oauth/* and /app/* take a forged Host directly, and the real close is the server.public_url anchor), and the panel allow-set narrowing on commit 2.

On passkeys -- I said I'd thread trusted_proxies through _forwarded_host there while I was in it, and on second look I'm leaving it. It sits on the register/authenticate path, and doing it safely means plumbing trusted_proxies through every passkey call site, RP-ID derivation included, where getting it subtly wrong breaks logins rather than hardening anything -- and by your own read the spoof only breaks a login, never forges one. That's a standalone change with its own test surface, not a rider on this one. Happy to do it as a follow-up PR if you want the consistency.

475 passed, 1 skipped, ruff check src/ tests/ clean, Python 3.12. Commit 2 still stands as is -- merge order your call.