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

beaconmcp.yaml.example

@@ -41,7 +41,11 @@ server:
     - https://www.perplexity.ai
     - https://gemini.google.com
 
-  # Trust X-Forwarded-For only from direct peers you operate.
+  # Direct peers you operate whose forwarded headers are trusted. Governs both
+  # X-Forwarded-For (auth rate-limit client IP) and X-Forwarded-Host (the host
+  # advertised in the OAuth issuer and the "paste this MCP URL" strings). When
+  # empty, both fall back to the request's own peer / Host and forwarded values
+  # from any peer are ignored.
   # IPs and CIDRs are accepted.
   # - Keep loopback entries when your reverse proxy runs on the same host.
   # - Add `cloudflare` to auto-expand to Cloudflare's published proxy CIDRs.

docs/cloudflare.md

@@ -124,8 +124,10 @@ server:
   # SDK rejects requests with 421 Misdirected Request (DNS-rebinding guard).
   allowed_hosts:
     - mcp.example.com
-  # Trust Cloudflare's edge so X-Forwarded-For is honoured for auth rate
-  # limiting. The literal "cloudflare" auto-expands to Cloudflare's IP ranges.
+  # Trust Cloudflare's edge so its forwarded headers are honoured: X-Forwarded-
+  # For for the auth rate-limit client IP, and X-Forwarded-Host for the host in
+  # the OAuth issuer and token MCP URLs. The literal "cloudflare" auto-expands
+  # to Cloudflare's IP ranges.
   trusted_proxies:
     - cloudflare
 ```

docs/configuration.md

@@ -36,7 +36,7 @@ Tailscale IP, a VPN address, a bastion.
 |-----|-------|
 | `server.allowed_hosts` | DNS-rebinding allowlist. **Must** include the public FQDN behind your reverse proxy, or requests come back `421 Misdirected Request`. |
 | `server.allowed_origins` | Web-origin allowlist, used for browser CORS preflights and for OAuth HTTPS redirect URIs. Desktop and CLI callbacks (`vscode://`, `cursor://`, loopback) are handled separately. |
-| `server.trusted_proxies` | Direct peers allowed to supply `X-Forwarded-For`, as IPs or CIDRs. The value `cloudflare` auto-expands to Cloudflare's edge ranges. |
+| `server.trusted_proxies` | Direct peers whose forwarded headers are trusted, as IPs or CIDRs. Governs `X-Forwarded-For` (the auth rate-limit client IP) **and** `X-Forwarded-Host` (the host advertised in the OAuth issuer and the token/connector MCP URLs). When empty, both fall back to the request's own peer / `Host` and forwarded values from any peer are ignored. The value `cloudflare` auto-expands to Cloudflare's edge ranges. |
 | `server.tokens_db` | SQLite file persisting *named* API tokens (the `/app/tokens` page) across restarts. Created owner-only (0600). Defaults to `tokens.db` next to `clients_file`. Env override: `BEACONMCP_TOKENS_DB`. |
 | `server.named_token_ttl` | Lifetime of named API tokens, in seconds. Default `2592000` (30 days); `0` means never expires, revoke-only. Internal OAuth and session bearers keep their fixed 24 h TTL either way. Env override: `BEACONMCP_NAMED_TOKEN_TTL`. |
 | `server.audit_log` | JSON-lines audit log covering tool calls, dashboard logins, OAuth authorize and client revokes. Created owner-only (0600). Default `/opt/beaconmcp/audit.log`; `-` keeps stderr only. Env override: `BEACONMCP_AUDIT_LOG`. |

src/beaconmcp/__main__.py

@@ -1088,7 +1088,7 @@ def _run_http(mcp, host: str, port: int):
         TotpResult,
         current_bearer_token,
     )
-    from .ratelimit import RateLimiter, client_ip
+    from .ratelimit import RateLimiter, client_ip, forwarded_host
     from .server import config
 
     from .metrics import REGISTRY, auth_events, http_requests
@@ -1206,8 +1206,10 @@ def totp_record_success(client_id: str) -> None:
 
     def _issuer(request: Request) -> str:
         scheme = request.headers.get("x-forwarded-proto", request.url.scheme)
-        host_header = request.headers.get(
-            "x-forwarded-host", request.headers.get("host", "localhost")
+        # X-Forwarded-Host is only trusted from a declared proxy; otherwise the
+        # request's own Host header wins (see ratelimit.forwarded_host).
+        host_header = forwarded_host(
+            request, tuple(config.server.trusted_proxies),
         )
         return f"{scheme}://{host_header}"
 
@@ -2241,7 +2243,16 @@ async def lifespan(_app):
         print(f"Passkeys:  disabled - {passkey_reason}")
     if n_clients == 0:
         print("\nNo clients registered. Create one with: beaconmcp auth create --name 'My Client'")
-    uvicorn.run(app, host=host, port=port, log_level="info")
+    # proxy_headers=False: the app owns its own forwarded-header trust model
+    # end to end -- client_ip walks X-Forwarded-For against trusted_proxies and
+    # forwarded_host does the same for X-Forwarded-Host, both keyed on the real
+    # TCP peer. uvicorn's default ProxyHeadersMiddleware (proxy_headers=True,
+    # forwarded_allow_ips="127.0.0.1") rewrites scope["client"] to the XFF
+    # client before the app runs, which would hide the real peer from both
+    # helpers -- their trusted-proxy branch could never open. Every scheme read
+    # goes through the x-forwarded-proto header directly, so nothing else needs
+    # uvicorn to interpret the forwarded headers for us.
+    uvicorn.run(app, host=host, port=port, log_level="info", proxy_headers=False)
 
 
 def _build_dashboard_routes(client_store, token_store, totp_locked,

src/beaconmcp/dashboard/app.py

@@ -29,6 +29,7 @@
 
 from .. import audit
 from ..auth import TOTP_REPLAY_MESSAGE, TotpResult
+from ..ratelimit import forwarded_host
 from . import csrf as csrf
 from .chat import (
     ChatEngine,
@@ -867,9 +868,7 @@ async def connectors_mint(request: Request) -> Response:
         row = store.mint(owner_client_id=session.client_id, label=label)
 
         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}"
         return _render_connectors_page(
             request, session,
@@ -1871,9 +1870,7 @@ def _expires_label(expires_at: float) -> str:
         mcp_url = deps.mcp_public_url.rstrip("/") + "/mcp"
     else:
         scheme = request.headers.get("x-forwarded-proto", request.url.scheme)
-        host = request.headers.get(
-            "x-forwarded-host", request.headers.get("host", "localhost"),
-        )
+        host = forwarded_host(request, deps.trusted_proxies)
         mcp_url = f"{scheme}://{host}/mcp"
 
     return _render(
@@ -1929,9 +1926,7 @@ def _resolve_mcp_url(request: Request, deps: DashboardDeps) -> str:
         if deps.mcp_public_url:
             return deps.mcp_public_url.rstrip("/") + "/mcp"
         scheme = request.headers.get("x-forwarded-proto", request.url.scheme)
-        host = request.headers.get(
-            "x-forwarded-host", request.headers.get("host", "localhost"),
-        )
+        host = forwarded_host(request, deps.trusted_proxies)
         return f"{scheme}://{host}/mcp"
     import os as _os
     port = _os.environ.get("BEACONMCP_PORT", "8420")

src/beaconmcp/dashboard/chat.py

@@ -203,9 +203,19 @@ class UsageAccumulated:
 # ``confirm`` is a parameter the tool actually declares, so what we read is
 # what the tool will act on. The trap that note describes is an argument the
 # tool does *not* declare, which pydantic drops during validation.
+#
+# ``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; the gate is on a *non-empty* ``config`` (the check below is
+# ``bool(args.get(...))``), so a bare create -- no ``config`` or an empty
+# ``{}`` -- carries no code-execution keys and stays ungated.
 _CONFIRM_WHEN_ARG_PRESENT: dict[str, str] = {
     "proxmox_vm_config": "updates",
     "beaconmcp_self_update": "confirm",
+    "proxmox_vm_create": "config",
 }
 
 # Tools that actually implement a ``dry_run`` parameter and return a

src/beaconmcp/ratelimit.py

@@ -167,3 +167,57 @@ def client_ip(request: object, trusted_proxies: tuple[str, ...] = ()) -> str:
     if direct_peer_raw:
         return direct_peer_raw
     return "unknown"
+
+
+def forwarded_host(
+    request: object,
+    trusted_proxies: tuple[str, ...] = (),
+    *,
+    default: str = "localhost",
+) -> str:
+    """Client-facing Host for a Starlette ``Request``.
+
+    Mirrors :func:`client_ip`'s trust model for the *host* dimension: the
+    ``X-Forwarded-Host`` header is attacker-controlled on a direct request, so
+    it is honored only when the direct peer is a declared trusted proxy.
+    Otherwise the request's own ``Host`` header is used (then ``default``).
+
+    Like :func:`client_ip`, this keys on ``request.client.host`` being the real
+    TCP peer, so the server must run with uvicorn ``proxy_headers=False`` (see
+    ``__main__``): the default ``ProxyHeadersMiddleware`` would rewrite the peer
+    to the ``X-Forwarded-For`` client and the trusted-proxy branch below could
+    never open.
+
+    Kept deliberately narrow -- it does NOT touch the scheme. ``X-Forwarded-
+    Proto`` is still read directly by the callers, because a TLS-terminating
+    edge (Cloudflare tunnel, nginx) legitimately needs it to report https even
+    when ``trusted_proxies`` is unset; gating it would silently downgrade the
+    Secure-cookie flag and the OAuth issuer to http.
+    """
+    headers = getattr(request, "headers", None)
+    has_get = headers is not None and hasattr(headers, "get")
+
+    def _hdr(name: str) -> str | None:
+        return headers.get(name) if has_get else None
+
+    host_header = _hdr("host") or default
+    if not trusted_proxies:
+        return host_header
+
+    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):
+        fwd = _hdr("x-forwarded-host")
+        if fwd:
+            # Take the last entry, not the first. A proxy that appends rather
+            # than overwrites puts its own value last, so the last entry is the
+            # one the nearest trusted proxy wrote; returning the first would
+            # hand back a client-supplied prefix. Symmetric with client_ip's
+            # right-to-left walk. Proxies that overwrite (the common case:
+            # nginx ``proxy_set_header X-Forwarded-Host $host``) leave a single
+            # entry, so first and last coincide.
+            parts = [p.strip() for p in fwd.split(",") if p.strip()]
+            if parts:
+                return parts[-1]
+    return host_header

tests/test_dashboard_chat.py

@@ -919,6 +919,35 @@ def test_self_update_needs_confirmation_when_applied():
     assert not _tool_call_requires_confirmation("beaconmcp_check_update", {})
 
 
+def test_vm_create_with_config_needs_confirmation():
+    """A ``config`` dict on vm-create can carry code-execution keys.
+
+    ``proxmox_vm_create`` forwards its ``config`` straight to the PVE API, so
+    an injected instruction could smuggle a ``hookscript`` or raw QEMU
+    ``args`` that runs code on the host. The create-with-config shape must
+    reach the modal, while a bare create (an empty shell VM) stays a plain
+    unattended add like clone/start.
+    """
+    from beaconmcp.dashboard.chat import _tool_call_requires_confirmation
+
+    assert _tool_call_requires_confirmation(
+        "proxmox_vm_create",
+        {"node": "pve1", "vmid": 100, "config": {"hookscript": "local:snippets/x.sh"}},
+    )
+    # A benign config still gets the modal -- we gate on presence, not on
+    # inspecting keys, mirroring the proxmox_vm_config ``updates`` treatment.
+    assert _tool_call_requires_confirmation(
+        "proxmox_vm_create", {"node": "pve1", "vmid": 100, "config": {"cores": 2}},
+    )
+    # No config -> empty shell, nothing to smuggle, no modal.
+    assert not _tool_call_requires_confirmation(
+        "proxmox_vm_create", {"node": "pve1", "vmid": 100},
+    )
+    assert not _tool_call_requires_confirmation(
+        "proxmox_vm_create", {"node": "pve1", "vmid": 100, "config": None},
+    )
+
+
 def test_every_registered_tool_is_gated_or_deliberately_not():
     """Guard against a new tool quietly landing outside the gate.
 
@@ -957,7 +986,7 @@ def test_every_registered_tool_is_gated_or_deliberately_not():
         "proxmox_list_nodes", "proxmox_list_transfers", "proxmox_list_vms",
         "proxmox_logs_panel", "proxmox_network_config", "proxmox_node_status",
         "proxmox_read_file", "proxmox_snapshot_create", "proxmox_snapshot_list",
-        "proxmox_storage_status", "proxmox_vm_clone", "proxmox_vm_create",
+        "proxmox_storage_status", "proxmox_vm_clone",
         "proxmox_vm_panel", "proxmox_vm_start", "proxmox_vm_status",
         "security_end_session", "ssh_list_sessions", "vm_find",
     }

tests/test_ratelimit.py

@@ -4,7 +4,7 @@
 
 import time
 
-from beaconmcp.ratelimit import RateLimiter, client_ip
+from beaconmcp.ratelimit import RateLimiter, client_ip, forwarded_host
 
 
 def test_allows_up_to_limit_then_blocks() -> None:
@@ -101,3 +101,111 @@ def __init__(self, fwd: str | None, *, peer: str = "10.0.0.1") -> None:
 
     # Direct peer with no trust config -> use peer IP.
     assert client_ip(_Req(None, peer="203.0.113.10"), trusted_proxies=()) == "203.0.113.10"
+
+
+def test_forwarded_host_only_trusts_declared_proxy() -> None:
+    class _H:
+        def __init__(self, host: str | None, xfh: str | None) -> None:
+            self._host = host
+            self._xfh = xfh
+
+        def get(self, k: str) -> str | None:
+            k = k.lower()
+            if k == "host":
+                return self._host
+            if k == "x-forwarded-host":
+                return self._xfh
+            return None
+
+    class _Client:
+        host = "10.0.0.1"
+
+    class _Req:
+        def __init__(
+            self, host: str | None, xfh: str | None, *, peer: str = "10.0.0.1",
+        ) -> None:
+            self.headers = _H(host, xfh)
+            c = _Client()
+            c.host = peer
+            self.client = c
+
+    # No trusted proxies configured -> X-Forwarded-Host is ignored, even when
+    # the peer looks internal. The request's own Host header wins.
+    assert (
+        forwarded_host(_Req("real.example", "evil.attacker"), trusted_proxies=())
+        == "real.example"
+    )
+
+    # Trusted direct proxy -> the forwarded host is believed.
+    assert (
+        forwarded_host(
+            _Req("internal:8420", "public.example", peer="10.0.0.1"),
+            trusted_proxies=("10.0.0.1",),
+        )
+        == "public.example"
+    )
+
+    # A spoofed X-Forwarded-Host from an UNtrusted peer is dropped; Host wins.
+    assert (
+        forwarded_host(
+            _Req("real.example", "evil.attacker", peer="192.0.2.8"),
+            trusted_proxies=("10.0.0.0/8",),
+        )
+        == "real.example"
+    )
+
+    # Proxy chain: a client-supplied prefix must not win. A proxy that appends
+    # its own value puts it last, so the last entry is returned -- symmetric
+    # with client_ip's right-to-left walk.
+    assert (
+        forwarded_host(
+            _Req("internal", "evil.attacker, public.example", peer="10.0.0.1"),
+            trusted_proxies=("10.0.0.1",),
+        )
+        == "public.example"
+    )
+
+    # Nothing usable -> default.
+    assert forwarded_host(_Req(None, None), trusted_proxies=()) == "localhost"
+
+
+def test_forwarded_host_through_a_real_starlette_request() -> None:
+    # Guards against the hand-built _Req above drifting from runtime: build an
+    # actual Starlette Request from an ASGI scope and confirm the trusted-proxy
+    # branch opens. This is the shape the app sees once uvicorn is told not to
+    # rewrite scope["client"] (proxy_headers=False), so request.client.host is
+    # the real TCP peer -- the proxy -- not the X-Forwarded-For client.
+    from starlette.requests import Request
+
+    def _req(host: str, xfh: str | None, peer: str) -> Request:
+        headers = [(b"host", host.encode())]
+        if xfh is not None:
+            headers.append((b"x-forwarded-host", xfh.encode()))
+        scope = {
+            "type": "http",
+            "method": "GET",
+            "path": "/",
+            "headers": headers,
+            "client": (peer, 44444),
+            "scheme": "http",
+            "server": ("app", 80),
+        }
+        return Request(scope)
+
+    # Proxy peer is trusted -> the forwarded host is believed.
+    assert (
+        forwarded_host(
+            _req("127.0.0.1:8420", "beacon.example.com", "127.0.0.1"),
+            trusted_proxies=("127.0.0.1",),
+        )
+        == "beacon.example.com"
+    )
+
+    # Direct (untrusted) peer -> forwarded host dropped, own Host wins.
+    assert (
+        forwarded_host(
+            _req("real.example", "evil.attacker", "203.0.113.5"),
+            trusted_proxies=("127.0.0.1",),
+        )
+        == "real.example"
+    )