dashboard-auth: trust X-Forwarded-For only from a loopback peer, and only its last hop

[ghost]

Severity: high — defeats the only brute-force control in front of the
password login, and forges the source IP on every auth audit event.

Line references below are against cd6585abf.

---

Problem. _client_ip takes the first element of X-Forwarded-For, which
is entirely client-supplied. Three byte-identical copies:
hermes_cli/dashboard_auth/routes.py:108-112,
hermes_cli/dashboard_auth/token_auth.py:83-87, and
hermes_cli/dashboard_auth/middleware.py:89-93.

fwd = request.headers.get("x-forwarded-for", "")
if fwd:
    return fwd.split(",")[0].strip()          # attacker-controlled
return request.client.host if request.client else ""

That return value is:

• the sole bucket key for the password-login throttle —
  routes.py:667 feeds it to _password_rate_limited (routes.py:615-634,
  10 attempts / 60s, routes.py:609-610). Sending a different first hop on
  every request allocates a fresh bucket every time, so the limiter never
  fires and online guessing runs unmetered against the provider's scrypt
  verify (_SCRYPT_N = 2**14, plugins/dashboard_auth/basic/init.py:95).
• the ip= field on every auth audit record — routes.py:673, 688, 701, 714, 724 (login failure/success) and token_auth.py:178, 189
  (TOKEN_AUTH_FAILURE). Any caller can write an arbitrary source IP into the
  audit trail, so post-incident attribution is unusable.
• the per-IP cap on the native authorization flow — routes.py:347 passes it as
  native_flow.register_pending(client_ip=...), and native_flow.py:195-197
  compares it for equality (v.client_ip == client_ip) against
  _MAX_PENDING_PER_IP. So the value is not merely logged: it is a live
  comparison key, and rotating the header lifts that cap too.

A prepended hop costs the attacker one header. No proxy configuration
mitigates it, because the header is read raw: uvicorn's proxy_headers
(hermes_cli/web_server.py:17242, enabled when the gate is active) rewrites
request.client from the forwarded headers, but _client_ip never consults
request.client when the header is present.

Change. Replace the first-hop read with a trusted-peer gate, in both
copies:

peer = request.client.host if request.client else ""
if not is_loopback_peer(peer):
    return peer
fwd = request.headers.get("x-forwarded-for", "")
hops = [h.strip() for h in fwd.split(",") if h.strip()]
return hops[-1] if hops else peer

with the loopback test shared from dashboard_auth/base.py (which neither copy
imported from before, and which has no intra-package imports, so there is no
cycle):

def is_loopback_peer(peer: str) -> bool:
    try:
        return ipaddress.ip_address(peer).is_loopback
    except ValueError:
        return False        # unix socket, empty, hostname — never trusted

Three details that a literal peer not in ("127.0.0.1", "::1") test gets wrong,
each of which fails closed on availability rather than open on security — i.e.
they lock legitimate users out rather than letting attackers in, which is why
they are easy to miss in review:

• IPv4-mapped loopback. Under a dual-stack bind (--host ::) the IPv4
  loopback proxy peer arrives as ::ffff:127.0.0.1. A literal tuple rejects it,
  X-Forwarded-For is discarded, and every client collapses into one throttle
  bucket — 429 on correct credentials, plus 503 once
  native_flow._MAX_PENDING_PER_IP (native_flow.py:92) is hit.
• The rest of 127.0.0.0/8. is_loopback covers it; a two-element tuple
  does not.
• Empty trailing hop. A proxy emitting "10.0.0.7," yields "" from
  split(",")[-1].strip(), which lands every such caller in the shared
  _unknown_ bucket. Filtering empty hops avoids it.

Two rules, both minimal:

1. X-Forwarded-For is only read when the connection peer is loopback. A
   direct connection has no legitimate reason to carry the header, so on that
   path it is ignored outright and the real peer is used.
2. When it is read, the LAST hop wins. The header is append-only: each hop
   appends the address it received the connection from. In a single-proxy
   deployment (the shipped shape — a loopback-bound dashboard fronted by one
   reverse proxy, cf. dashboard: reverse-proxy Host allowlist + auth-gate scoping (extra_hosts) #75907) the last element is exactly the address that
   proxy observed, and the one element a client cannot influence. Everything
   to its left is whatever the client sent.

Signature, name and return contract are unchanged (str, "" when there is no
discernible peer — which still lands in the throttle's shared _unknown_
bucket, routes.py:626). The stale clause in the throttle's block comment
(routes.py:604-607) is corrected to match the new granularity.

Deliberately not generalized into a configurable trusted-proxy list: with one
proxy, "last hop" is unconditionally correct and needs no configuration. A
multi-proxy deploy is the case that would need trusted_hops = N, and adding
that knob before anyone runs that shape invites mis-setting it — a too-large
value reintroduces exactly this bug.

Evidence/Repro. 50 password-login attempts from a loopback peer, rotating
only the forgeable first hop while the proxy-appended last hop stays constant:

from fastapi.testclient import TestClient
from hermes_cli import web_server
from hermes_cli.dashboard_auth import clear_providers
from hermes_cli.dashboard_auth import routes

clear_providers()
routes._reset_password_rate_limit()
web_server.app.state.bound_host = "dash.example"
web_server.app.state.bound_port = 443
web_server.app.state.auth_required = True
client = TestClient(web_server.app, base_url="https://dash.example",
                    client=("127.0.0.1", 51234))

throttled = sum(
    client.post("/auth/password-login",
                json={"provider": "p", "username": "admin", "password": "guess"},
                headers={"X-Forwarded-For": f"10.0.0.{i}, 198.51.100.5"}
                ).status_code == 429
    for i in range(50)
)
print(f"{50 - throttled}/50 attempts reached the credential path")

BEFORE (first hop):                  50/50 attempts reached the credential path,  0 throttled
AFTER  (last hop, loopback-gated):   10/50 attempts reached the credential path, 40 throttled

10/50 is the intended budget (_PW_RATE_MAX_ATTEMPTS = 10). Before the change
the limiter is a no-op against any attacker who rotates one header.

Tests. New file
tests/hermes_cli/test_dashboard_auth_xff_trusted_peer.py — 27 tests. The
helper contract is parametrized across both copies of _client_ip
(routes and token_auth) so they cannot drift apart again: spoofed multi-hop
chain from a loopback peer resolves to the last hop; ::1 and
::ffff:127.0.0.1 are trusted like 127.0.0.1; a trailing separator does not
degrade to the empty bucket key; a non-loopback peer (public and RFC1918)
ignores the header entirely; missing/empty header and absent request.client
keep the old fallbacks. Three end-to-end throttle tests drive the real route
through TestClient(..., client=<peer>): rotating the first hop exhausts one
bucket (404 × 10 then 429), two distinct last hops get distinct buckets,
and a non-loopback peer cannot reset its bucket with any header value.

./scripts/run_tests.sh tests/hermes_cli/test_dashboard_auth_xff_trusted_peer.py
→ 1 file, 27 passed, 0 failed

./scripts/run_tests.sh tests/hermes_cli/test_dashboard_auth_*.py \
                      tests/hermes_cli/test_dashboard_token_auth.py \
                      tests/hermes_cli/test_dashboard_oauth_endpoints_server_gate.py
→ 17 files, 160 passed, 0 failed

Reverting only the two _client_ip bodies against this test file fails 18 of
the 27, which is the intended regression surface.

Alternatives considered.

• Rely on uvicorn's proxy_headers. Doesn't apply — it normalizes
  request.client, and the buggy code path never reads request.client once
  the header exists. Fixing the reader is the actual fix. The two do compose
  cleanly afterwards: with proxy_headers on, uvicorn has already resolved
  request.client.host to the last untrusted hop, so is_loopback_peer is
  false and that value is returned unchanged; with it off, the peer is the
  loopback proxy and the header's last hop is read directly. Same answer either
  way.
• A second throttle bucket keyed on username. Rejected: it reintroduces a
  username-enumeration oracle that routes.py:660-704 deliberately avoids —
  unknown provider and bad password both return generic responses, and the
  basic provider spends a fixed _DUMMY_HASH scrypt round on unknown
  usernames for constant time (plugins/dashboard_auth/basic/__init__.py:255).
  A per-username bucket makes "this username exists" observable through
  429-vs-401 timing and counts.
• Configurable trusted-proxy CIDR list. More surface than the deployment
  needs; see above.

Residual, unchanged by this patch. Setting FORWARDED_ALLOW_IPS=* in the
environment makes uvicorn itself rewrite request.client from the first
X-Forwarded-For hop, which would put an attacker-chosen value back into peer.
The repo never sets it, and that is uvicorn's documented behaviour for *
rather than something this helper can defend against — noted so the trust
boundary is explicit.

All three copies are fixed here, not just the two with a rate-limit
consequence. The third lives at hermes_cli/dashboard_auth/middleware.py:89-93
and feeds ip= on the gate's own audit events (middleware.py:239, 434, 498, 505, 570, 582 — SESSION_VERIFY_FAILURE, REFRESH_FAILURE, LOGIN_START).
It has no throttle consequence, so it was tempting to defer; but leaving it
would mean shipping a security fix whose own shared-helper docstring is
falsifiable by a single grep -rn "def _client_ip", and would leave a third of
the audit trail forgeable. It is a three-line change against the same helper.

After this patch grep -rn 'split(",")\[0\]' hermes_cli/dashboard_auth/ returns
nothing.

[teknium1]

Thanks for the focused security fix. The premise is present on current main: hermes_cli/dashboard_auth/routes.py:108-112 trusts the first raw X-Forwarded-For hop, and routes.py:667-668 uses that result for the password-login throttle bucket. The same first-hop behavior exists in token_auth.py:83-87 and middleware.py:89-93.

Problems

• tests/hermes_cli/test_dashboard_auth_xff_trusted_peer.py:33 covers only routes._client_ip and token_auth._client_ip, although this PR also changes middleware._client_ip. That leaves the gate-audit implementation outside the new regression contract.

Suggested changes

• Add middleware._client_ip to _HELPERS at tests/hermes_cli/test_dashboard_auth_xff_trusted_peer.py:33 so every changed copy is tested for loopback gating, last-hop selection, and empty-hop handling.

Automated hermes-sweeper review.

[teknium1]

This tuple omits middleware._client_ip, although the PR changes that third copy too. Please add it here so the trusted-peer contract covers every modified implementation.

[ghost]

Fixed in 440bfc8.

_HELPERS listed routes and token_auth but not middleware, which this PR also changes:

_HELPERS = (_client_ip, token_auth._client_ip, middleware._client_ip)

The omission mattered more than a missing third parametrization usually would — middleware._client_ip is the copy feeding the audit log's ip= field on every authenticated request (SESSION_VERIFY_FAILURE, REFRESH_FAILURE, LOGIN_START`), so it was the one call site the patch touched with nothing exercising it.

Checked that the new coverage actually bites rather than just importing the symbol: reintroducing the first-hop read in middleware._client_ip alone fails 4 cases under the _client_ip2 parametrization, where before it failed none. 39 passing now.

[egilewski]

suggesting changes

The patch correctly limits X-Forwarded-For use to loopback peers and takes the final nonempty hop, but the trust boundary can still be bypassed when Uvicorn rewrites request.client from forwarded headers supplied by an untrusted peer. Pin forwarded-proxy trust to loopback and preserve the raw socket peer before middleware rewriting so all three helpers gate on that value.

• [P2] Loopback gate trusts Uvicorn-rewritten request.client under wildcard forwarded trust (hermes_cli/dashboard_auth/routes.py:124)
  With proxy_headers=True and FORWARDED_ALLOW_IPS=* (or another broad trust setting), Uvicorn's proxy-header middleware can replace request.client on a direct non-loopback connection using attacker-controlled X-Forwarded-For. The helper then accepts the rewritten value as the peer, allowing header rotation to create fresh password-login and native-flow per-IP buckets and to forge authentication audit IPs. Configure an explicit loopback-only forwarded-proxy allowlist, preserve the pre-rewrite peer, gate all three helpers on it, and add an integration test covering wildcard forwarded trust with a direct non-loopback peer.

Security evidence:

• trust boundary: The raw connection peer and X-Forwarded-For are untrusted until a trusted local proxy boundary. The three helpers feed password-login and native-flow rate limits plus authentication audit IP fields; Uvicorn proxy-header middleware runs before those helpers and can rewrite request.client.
• source/sink/invariant: Only a raw loopback peer may enable forwarded-header use, and the final nonempty hop must feed each rate-limit or audit sink. The patch enforces this predicate on request.client, which is mutable after proxy-header processing.
• current-main reproduction: The pre-patch helpers selected the first forwarded hop, so rotating that hop yielded distinct limiter keys and bypassed the shared attempt budget. The patched helper logic shares the final-hop bucket, but an upstream rewrite can still make an attacker-controlled hop appear to be the peer.
• PR-head or patch-replay validation: A run-owned local patch replay against current GitHub main was reviewed; this does not mean the submitted branch itself merges cleanly. Focused helper tests and dashboard-auth compilation passed, and a direct Uvicorn wildcard-trust probe reproduced the rewritten-peer case.
• positive/negative cases: Loopback, IPv6, IPv4-mapped loopback, whitespace, empty/trailing hops, and direct non-loopback peers are covered; the remaining negative case is a direct public peer whose request.client is rewritten from attacker-supplied forwarded headers.
• residual bypass search: The three _client_ip helpers no longer use the first forwarded hop, but web_server enables proxy_headers=True without pinning Uvicorn's forwarded allowlist, so broad deployment settings can reintroduce attacker-selected identities.
• reviewer validation: Source call sites, the changed helpers, focused tests, and the direct middleware probe were reviewed; the available checks support the residual trust-boundary finding.

Not checked:

• Ruff validation
• CodeRabbit review
• End-to-end throttle TestClient validation

Signed: GPT-5.6-luna-max in Codex