From 0dfba37b11ff2ca908ae2df85b55f4f4c9b7fd8b Mon Sep 17 00:00:00 2001 From: Gille <4317663+helix4u@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:35:22 -0600 Subject: [PATCH] fix(dashboard): trust configured reverse proxies (#94126) * fix(dashboard): trust configured reverse proxies * fix(dashboard): trust IPv6 loopback proxies --- cli-config.yaml.example | 10 ++ hermes_cli/config_defaults.py | 5 + hermes_cli/web_server.py | 67 +++++++++++++ pyproject.toml | 3 +- tests/hermes_cli/test_dashboard_auth_gate.py | 95 +++++++++++++++++++ uv.lock | 2 +- website/docs/user-guide/configuration.md | 2 + website/docs/user-guide/docker.md | 18 ++++ .../docs/user-guide/features/web-dashboard.md | 23 ++++- 9 files changed, 220 insertions(+), 5 deletions(-) diff --git a/cli-config.yaml.example b/cli-config.yaml.example index 37248c3f95..17aeced2fd 100644 --- a/cli-config.yaml.example +++ b/cli-config.yaml.example @@ -1868,6 +1868,16 @@ updates: # # default — works on Fly.io out of the box). # # # # public_url: "https://example.com/hermes" +# # +# # Reverse proxies connecting from another container or host are not +# # trusted by default. Add only the proxy's exact IP address, or a bounded +# # CIDR for a dedicated proxy network, so X-Forwarded-Proto and +# # X-Forwarded-For can be honored. Loopback is always trusted. Wildcards +# # and /0 networks are rejected. +# # +# # trusted_proxies: +# # - "172.20.0.5" +# # # - "172.20.0.0/24" # dedicated network, if the IP is dynamic # # ----------------------------------------------------------------------------- # Self-hosted OIDC dashboard auth (generic OpenID Connect — Authentik, diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index 1077612733..6544c4f83f 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -1633,6 +1633,11 @@ DEFAULT_CONFIG = { # Set this to True to re-enable the surfaces with the understanding # that the numbers are a local lower-bound estimate, not billing. "show_token_analytics": False, + # IP addresses or bounded CIDR networks of reverse proxies allowed to + # supply X-Forwarded-Proto / X-Forwarded-For. Loopback remains trusted + # automatically. Wildcards and /0 networks are rejected so arbitrary + # clients cannot spoof their scheme or source address. + "trusted_proxies": [], # WebSocket keepalive for the dashboard/desktop web server (#79635). # Applied to NON-loopback binds only: loopback always disables the # protocol ping (see hermes_cli/web_server.py — an event-loop stall diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 6fa676c85f..73b2014e8e 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -25,6 +25,7 @@ import hashlib import hmac import inspect import importlib.util +import ipaddress import json import logging import math @@ -19557,6 +19558,66 @@ def _report_port_in_use(host: str, port: int) -> None: ) +_DEFAULT_DASHBOARD_FORWARDED_ALLOW_IPS = ("127.0.0.1", "::1") + + +def _dashboard_forwarded_allow_ips(dashboard_config: dict[str, Any]) -> list[str]: + """Return the bounded proxy addresses uvicorn may trust. + + Uvicorn's default trusts loopback. Preserve that behavior and extend it + only with explicit IP addresses or CIDR networks from config. Invalid or + unbounded entries fail closed instead of turning arbitrary client-supplied + forwarding headers into request metadata. + """ + configured = dashboard_config.get("trusted_proxies", []) + if configured in (None, ""): + configured = [] + elif isinstance(configured, str): + configured = [configured] + elif not isinstance(configured, (list, tuple)): + _log.warning( + "dashboard.trusted_proxies must be a list of IP addresses or CIDR networks; " + "ignoring %r", + configured, + ) + configured = [] + + trusted = list(_DEFAULT_DASHBOARD_FORWARDED_ALLOW_IPS) + for raw_entry in configured: + if not isinstance(raw_entry, str) or not raw_entry.strip(): + _log.warning( + "Ignoring invalid dashboard.trusted_proxies entry %r; expected an IP " + "address or CIDR network", + raw_entry, + ) + continue + + entry = raw_entry.strip() + try: + if "/" in entry: + network = ipaddress.ip_network(entry, strict=False) + if network.prefixlen == 0: + raise ValueError("unbounded network") + normalized = str(network) + else: + normalized = str(ipaddress.ip_address(entry)) + except ValueError: + _log.warning( + "Ignoring unsafe dashboard.trusted_proxies entry %r; use a bounded IP " + "address or CIDR network, never '*' or a /0 network", + raw_entry, + ) + continue + + if normalized not in trusted: + trusted.append(normalized) + + if trusted != list(_DEFAULT_DASHBOARD_FORWARDED_ALLOW_IPS): + _log.info("Dashboard trusted proxies: %s", ", ".join(trusted)) + + return trusted + + def start_server( host: str = "127.0.0.1", port: int = 9119, @@ -19804,6 +19865,12 @@ def start_server( # decide cookie Secure flags, so we flip proxy_headers on for that # mode. proxy_headers=bool(app.state.auth_required), + # Keep uvicorn's loopback-only default unless the operator explicitly + # trusts the address or bounded network of an upstream proxy. This is + # what lets a separate-container TLS terminator supply HTTPS/client + # metadata without accepting spoofed X-Forwarded-* headers from every + # caller. + forwarded_allow_ips=_dashboard_forwarded_allow_ips(_dash_cfg), # Half-open detection for public binds only (see above). Loopback # disables the protocol ping (None) so an event-loop stall can never # trigger a false disconnect; a genuinely dead local client is still diff --git a/pyproject.toml b/pyproject.toml index 94ab59f90d..d1f213e492 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -117,7 +117,8 @@ dependencies = [ # .gitignore-aware file matching for desktop build stamp. "pathspec==1.1.1", "fastapi>=0.104.0,<1", - "uvicorn[standard]>=0.24.0,<1", + # CIDR-aware forwarded_allow_ips requires uvicorn >=0.31.0. + "uvicorn[standard]>=0.31.0,<1", # Streaming multipart uploads for the dashboard file manager (NS-501). # FastAPI's UploadFile/Form depend on python-multipart; it is NOT pulled in # by fastapi itself, so the dashboard's multipart upload endpoint would 500 diff --git a/tests/hermes_cli/test_dashboard_auth_gate.py b/tests/hermes_cli/test_dashboard_auth_gate.py index 18eef3d39c..40f1c00089 100644 --- a/tests/hermes_cli/test_dashboard_auth_gate.py +++ b/tests/hermes_cli/test_dashboard_auth_gate.py @@ -3,6 +3,9 @@ Phase 0 — establish a baseline pin on the current (pre-OAuth) behavior so later phases can prove they didn't break loopback mode. """ +import asyncio +import logging + import pytest # Phase 5 / Phase 6: these tests mutate ``web_server.app.state.auth_required`` @@ -229,10 +232,102 @@ def test_start_server_gate_with_provider_proceeds_and_sets_proxy_headers(monkeyp assert web_server.app.state.auth_required is True assert captured["kwargs"].get("host") == "0.0.0.0" assert captured["kwargs"].get("proxy_headers") is True + assert captured["kwargs"].get("forwarded_allow_ips") == [ + "127.0.0.1", + "::1", + ] finally: clear_providers() +def test_start_server_passes_bounded_trusted_proxy_networks(monkeypatch, caplog): + """A configured proxy network reaches uvicorn without broadening to all peers.""" + from hermes_cli.dashboard_auth import clear_providers, register_provider + from tests.hermes_cli.conftest_dashboard_auth import StubAuthProvider + + clear_providers() + register_provider(StubAuthProvider()) + captured = _stub_uvicorn_run(monkeypatch) + monkeypatch.setattr( + web_server, + "load_config", + lambda: {"dashboard": {"trusted_proxies": ["172.18.0.23/16"]}}, + ) + try: + with caplog.at_level(logging.INFO, logger=web_server._log.name): + web_server.start_server( + host="0.0.0.0", port=9119, + open_browser=False, allow_public=False, + ) + assert captured["kwargs"]["forwarded_allow_ips"] == [ + "127.0.0.1", + "::1", + "172.18.0.0/16", + ] + assert ( + "Dashboard trusted proxies: 127.0.0.1, ::1, 172.18.0.0/16" + in caplog.text + ) + finally: + clear_providers() + + +def test_trusted_proxy_allowlist_rejects_unbounded_entries(caplog): + """Wildcard and whole-address-space trust must fail closed.""" + trusted = web_server._dashboard_forwarded_allow_ips({ + "trusted_proxies": ["*", "0.0.0.0/0", "::/0", "172.18.0.7"], + }) + + assert trusted == ["127.0.0.1", "::1", "172.18.0.7"] + assert "never '*' or a /0 network" in caplog.text + + +def test_trusted_container_proxy_controls_https_detection(): + """Only a configured bridge peer may turn X-Forwarded-Proto into HTTPS.""" + from hermes_cli.dashboard_auth.cookies import detect_https + from starlette.requests import Request + from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware + + trusted = web_server._dashboard_forwarded_allow_ips({ + "trusted_proxies": ["172.18.0.0/16"], + }) + + async def detected_scheme(peer: str) -> bool: + observed: dict[str, bool] = {} + + async def downstream(scope, receive, send): + observed["https"] = detect_https(Request(scope)) + + middleware = ProxyHeadersMiddleware(downstream, trusted_hosts=trusted) + scope = { + "type": "http", + "asgi": {"version": "3.0"}, + "http_version": "1.1", + "method": "GET", + "scheme": "http", + "path": "/auth/login", + "raw_path": b"/auth/login", + "query_string": b"", + "root_path": "", + "headers": [(b"x-forwarded-proto", b"https")], + "client": (peer, 43120), + "server": ("hermes", 9119), + } + + async def receive(): + return {"type": "http.disconnect"} + + async def send(message): + return None + + await middleware(scope, receive, send) + return observed["https"] + + assert asyncio.run(detected_scheme("172.18.0.9")) is True + assert asyncio.run(detected_scheme("::1")) is True + assert asyncio.run(detected_scheme("198.51.100.9")) is False + + def test_public_url_aware_gate_requires_auth_for_loopback_proxy(monkeypatch): """The shared gate decision includes an external browser-facing URL.""" from hermes_cli.web_server import should_require_dashboard_auth diff --git a/uv.lock b/uv.lock index 248f1f92ed..3f7b7946fb 100644 --- a/uv.lock +++ b/uv.lock @@ -1942,7 +1942,7 @@ requires-dist = [ { name = "ty", marker = "extra == 'dev'", specifier = "==0.0.21" }, { name = "tzdata", marker = "sys_platform == 'win32'", specifier = "==2025.3" }, { name = "urllib3", specifier = ">=2.7.0,<3" }, - { name = "uvicorn", extras = ["standard"], specifier = ">=0.24.0,<1" }, + { name = "uvicorn", extras = ["standard"], specifier = ">=0.31.0,<1" }, { name = "uvicorn", extras = ["standard"], marker = "extra == 'web'", specifier = "==0.41.0" }, { name = "vercel", marker = "extra == 'vercel'", specifier = "==0.7.2" }, { name = "websockets", specifier = "==15.0.1" }, diff --git a/website/docs/user-guide/configuration.md b/website/docs/user-guide/configuration.md index 89ec6e2c15..86f6693134 100644 --- a/website/docs/user-guide/configuration.md +++ b/website/docs/user-guide/configuration.md @@ -2630,6 +2630,7 @@ dashboard: theme: "default" # "default" | "midnight" | "ember" | "mono" | "cyberpunk" | "rose" show_token_analytics: false # Re-enable the (local-estimate-only) token/cost analytics surfaces public_url: "" # Full public authority for OAuth redirect_uri (env: HERMES_DASHBOARD_PUBLIC_URL) + trusted_proxies: [] # Proxy IPs/CIDRs allowed to supply X-Forwarded-* headers oauth: # Portal OAuth gate (engaged with --host and not --insecure) client_id: "" # agent:{instance_id} — Portal provisions this portal_url: "" # blank → plugin default (production Portal) @@ -2651,6 +2652,7 @@ dashboard: - `theme` — dashboard visual theme. - `show_token_analytics` — off by default. The Analytics page and token/cost figures are a **local lower-bound estimate** (they exclude auxiliary calls, retries, fallbacks, and cache writes), so they can read far below the provider bill. Set `true` only if you understand they're not billing. - `public_url` — when set, this is the complete authority (scheme + host + optional path prefix) the OAuth `redirect_uri` is built from. Set it for deploys behind reverse proxies that don't reliably forward `X-Forwarded-*` headers. Leave empty to use proxy-header reconstruction. +- `trusted_proxies` — IP addresses or bounded CIDR networks allowed to supply `X-Forwarded-Proto` and `X-Forwarded-For`. Loopback remains trusted automatically. Configure this when the TLS reverse proxy connects from another container or host. Prefer the proxy's exact IP; use a small dedicated network only when its address is dynamic. Wildcards and `/0` networks are rejected. - `oauth` / `basic_auth` / `drain_auth` — auth provider config read by the bundled dashboard-auth plugins. The drain secret itself is **not** set here; it's provisioned via the `HERMES_DASHBOARD_DRAIN_SECRET` env var. See [Web Dashboard](/user-guide/features/web-dashboard) for full auth setup. - `ws_ping_interval` / `ws_ping_timeout` — WebSocket keepalive tuning for non-loopback binds (loopback connections never ping). Raise these on high-latency links (Tailscale, distant SSH tunnels) where the 20 s defaults can manufacture spurious 1006 disconnects. - `ws_orphan_reap_grace_s` — how long a WS-detached session waits before the orphan reaper collects it. Raise alongside the keepalive values if clients reconnect slowly. (`HERMES_TUI_WS_ORPHAN_REAP_GRACE_S` remains as an internal override.) diff --git a/website/docs/user-guide/docker.md b/website/docs/user-guide/docker.md index 747e6b40ed..da737a4b17 100644 --- a/website/docs/user-guide/docker.md +++ b/website/docs/user-guide/docker.md @@ -138,6 +138,24 @@ There are three bundled ways to satisfy the second condition: Whichever you choose, the gate redirects callers to a login page before they can reach any protected route. See [Web Dashboard → Authentication](features/web-dashboard.md#authentication-gated-mode) for all three providers. +When a reverse proxy such as Traefik or nginx runs in another container, its +bridge-network address is not trusted by default. Set the dashboard's public +URL and trust only that proxy's exact IP, or a bounded CIDR for a dedicated +proxy network, in the mounted `config.yaml`: + +```yaml +dashboard: + public_url: "https://dashboard.example.com" + trusted_proxies: + - "172.20.0.5" + # Or, if the proxy address is dynamic on a dedicated network: + # - "172.20.0.0/24" +``` + +This allows the proxy's `X-Forwarded-Proto: https` to control secure OAuth +cookies while leaving forwarding headers from other peers untrusted. Do not +use `*`, `0.0.0.0/0`, or `::/0`; Hermes rejects those unbounded entries. + If no provider is registered and the bind is non-loopback, the dashboard **fails closed at startup** with a specific error pointing at the missing env var. There is no longer an escape hatch that serves the dashboard unauthenticated on a public bind: `HERMES_DASHBOARD_INSECURE=1` is now a deprecated no-op (it logs a warning and is ignored). Configure a provider, or bind `HERMES_DASHBOARD_HOST=127.0.0.1` and reach the dashboard over an SSH tunnel / Tailscale instead. :::warning Why `--insecure` was removed diff --git a/website/docs/user-guide/features/web-dashboard.md b/website/docs/user-guide/features/web-dashboard.md index fb60a60280..be4ed441d7 100644 --- a/website/docs/user-guide/features/web-dashboard.md +++ b/website/docs/user-guide/features/web-dashboard.md @@ -935,6 +935,8 @@ For deploys behind reverse proxies that don't reliably forward those headers (ma ```yaml dashboard: public_url: "https://dashboard.example.com/hermes" + trusted_proxies: + - "172.20.0.5" ``` When set, the OAuth callback URL becomes `/auth/callback` verbatim — `X-Forwarded-Prefix` is ignored on that code path because the operator has explicitly declared the public URL. This is intentional: stacking the prefix on top would double-prefix the common case where the prefix is already baked into `public_url`. @@ -950,8 +952,23 @@ Declaring a non-loopback `public_url` always engages the dashboard auth gate, even when the backend binds to loopback. Configure a password or OAuth provider first; without one, Hermes fails closed at startup. This prevents the local SPA session token from becoming a remote authentication mechanism through the -proxy. Uvicorn also enables trusted proxy-header processing in this mode so a -local TLS terminator can supply `X-Forwarded-Proto: https` for secure cookies. +proxy. Uvicorn also enables proxy-header processing in this mode. Loopback +proxies are trusted automatically. If the TLS terminator connects from another +container or host, add its exact IP address to `dashboard.trusted_proxies`, or +add a bounded CIDR for a dedicated proxy network when the address is dynamic: + +```yaml +dashboard: + public_url: "https://dashboard.example.com/hermes" + trusted_proxies: + - "172.20.0.0/24" +``` + +Only listed peers may supply `X-Forwarded-Proto` and `X-Forwarded-For`. +Hermes always preserves loopback trust and rejects `*`, `0.0.0.0/0`, and +`::/0`. Trusting a network means every container or machine on that network +can supply forwarding metadata, so prefer an exact proxy IP or a dedicated +proxy-only network. ```bash # Backend remains reachable only on this machine. @@ -978,7 +995,7 @@ Same precedence as the other dashboard settings — env wins over `config.yaml`: Validation rejects values without `http://` / `https://` scheme, without a host, or containing quote / angle / whitespace / control characters. A malformed value silently falls through to header reconstruction so the login flow keeps working rather than dispatching the user to a hostile URL. -> **Note:** `public_url` overrides the OAuth callback URL only. The `Secure` cookie flag is still controlled by `request.url.scheme` (X-Forwarded-Proto under proxy_headers), so an `http://` `public_url` on a TLS-terminated public deploy will produce non-Secure cookies. This is an operator footgun — pair `public_url` with proper TLS termination upstream. +> **Note:** `public_url` overrides the OAuth callback URL only. The `Secure` cookie flag is still controlled by `request.url.scheme`, using `X-Forwarded-Proto` only when the connecting peer is loopback or listed in `trusted_proxies`. Pair an HTTPS `public_url` with TLS termination and a bounded trusted-proxy entry when the proxy is not on loopback. ### OAuth flow