extract_client_ip misses TCP peer-addr fallback — IP auto-login broken from browser → proxy #31

New issue

Closed

opened 2026-04-26 15:53:31 +00:00 by sameh-farouk · 0 comments

sameh-farouk commented

2026-04-26 15:53:31 +00:00

Member

Problem

extract_client_ip (in crates/hero_proxy_server/src/proxy.rs) reads only X-Real-Ip and X-Forwarded-For headers. Both are upstream-proxy headers — neither is sent by a browser that connects to hero_proxy directly. Result: when hero_proxy is the edge (no nginx/caddy/Cloudflare in front, which is its designed deployment shape), client_ip = None for every browser request.

This breaks the IP-based auto-login path: find_user_for_ip(None) always returns None, so the proxy never injects X-Hero-User from user_networks rows, even when the operator has explicitly configured 127.0.0.1/32 → user X via the admin UI. The whole IP-auto-login feature is effectively dead from any browser → proxy path.

Verification

# proxy.users.user_networks has loopback rows mapped to user 1 with auto_accept=1
sqlite3 ~/hero/var/db/proxy.db "SELECT * FROM user_networks"
# 1|1|127.0.0.1/32|1|localhost dev access|...
# 2|1|::1/128|1|localhost IPv6|...
# 3|1|::ffff:127.0.0.1/128|1|localhost IPv4-mapped-IPv6|...

# Hit the proxy directly from the same loopback:
curl -s http://127.0.0.1:9997/hero_collab/rpc/rpc \
  -H 'Content-Type: application/json' \
  -d '{"jsonrpc":"2.0","id":1,"method":"user.me","params":{}}'
# Today: returns {"authenticated":false, "user":null}
# (no X-Hero-User injected; collab in proxy mode then fails-closed elsewhere)

After the fix, the same call returns the auto-resolved user — IP-auto-login works as configured.

Root cause

extract_client_ip is missing the standard third tier of every edge HTTP server's client-IP lookup chain:

X-Real-Ip (single IP from a trusted upstream proxy)
X-Forwarded-For (chain from CDN / multi-hop proxy)
TCP peer address from the kernel (← missing)

The whole point of hero_proxy is that it's the edge. Putting another reverse proxy in front of it would defeat its purpose (duplicate TLS termination, duplicate auth surface). So step 3 is what's needed for the actual deployment shape this service is designed for.

axum-extra's ClientIp, tower-http's RealIp, nginx's real_ip_module, actix-web's ConnectionInfo — all use this same three-tier pattern.

Fix shape

Two surgical changes:

extract_client_ip signature gains an Option<&SocketAddr> parameter and falls back to peer_addr.map(|a| a.ip().to_string()) after the existing header lookups. Header precedence is unchanged so deployments behind a CDN keep working.
proxy_handler extracts axum::extract::ConnectInfo<SocketAddr> and passes the peer addr through. Listener wiring in main.rs (HTTP, HTTPS-ACME, HTTPS-self-signed) switches from into_make_service() to into_make_service_with_connect_info::<SocketAddr>() so the extractor has the data to extract.

Pre-existing trust gap (separate issue)

This patch does NOT address: extract_client_ip honors X-Forwarded-For from any peer. With IP auto-login now actually working, anyone who can reach the listener directly can spoof X-Forwarded-For: 127.0.0.1 and impersonate the operator. Pre-existing in current code; the patch inherits it.

Filing as a separate issue: add trusted_proxies: Vec<IpNetwork> setting; only honor X-Real-Ip / X-Forwarded-For when the immediate peer is in that list. Default empty (= edge mode).

PR

Fix in feat/peer-addr-fallback. ~30 LOC. No schema change.

## Problem `extract_client_ip` (in `crates/hero_proxy_server/src/proxy.rs`) reads only `X-Real-Ip` and `X-Forwarded-For` headers. Both are *upstream-proxy* headers — neither is sent by a browser that connects to hero_proxy directly. Result: when hero_proxy is the edge (no nginx/caddy/Cloudflare in front, which is its designed deployment shape), `client_ip = None` for every browser request. This breaks the IP-based auto-login path: `find_user_for_ip(None)` always returns `None`, so the proxy never injects `X-Hero-User` from `user_networks` rows, even when the operator has explicitly configured `127.0.0.1/32 → user X` via the admin UI. The whole IP-auto-login feature is effectively dead from any browser → proxy path. ## Verification ```bash # proxy.users.user_networks has loopback rows mapped to user 1 with auto_accept=1 sqlite3 ~/hero/var/db/proxy.db "SELECT * FROM user_networks" # 1|1|127.0.0.1/32|1|localhost dev access|... # 2|1|::1/128|1|localhost IPv6|... # 3|1|::ffff:127.0.0.1/128|1|localhost IPv4-mapped-IPv6|... # Hit the proxy directly from the same loopback: curl -s http://127.0.0.1:9997/hero_collab/rpc/rpc \ -H 'Content-Type: application/json' \ -d '{"jsonrpc":"2.0","id":1,"method":"user.me","params":{}}' # Today: returns {"authenticated":false, "user":null} # (no X-Hero-User injected; collab in proxy mode then fails-closed elsewhere) ``` After the fix, the same call returns the auto-resolved user — IP-auto-login works as configured. ## Root cause `extract_client_ip` is missing the standard third tier of every edge HTTP server's client-IP lookup chain: 1. `X-Real-Ip` (single IP from a trusted upstream proxy) 2. `X-Forwarded-For` (chain from CDN / multi-hop proxy) 3. **TCP peer address from the kernel** (← missing) The whole point of hero_proxy is that it's the edge. Putting another reverse proxy in front of it would defeat its purpose (duplicate TLS termination, duplicate auth surface). So step 3 is what's needed for the actual deployment shape this service is designed for. axum-extra's `ClientIp`, tower-http's `RealIp`, nginx's `real_ip_module`, actix-web's `ConnectionInfo` — all use this same three-tier pattern. ## Fix shape Two surgical changes: 1. **`extract_client_ip` signature** gains an `Option<&SocketAddr>` parameter and falls back to `peer_addr.map(|a| a.ip().to_string())` after the existing header lookups. Header precedence is unchanged so deployments behind a CDN keep working. 2. **`proxy_handler`** extracts `axum::extract::ConnectInfo<SocketAddr>` and passes the peer addr through. Listener wiring in `main.rs` (HTTP, HTTPS-ACME, HTTPS-self-signed) switches from `into_make_service()` to `into_make_service_with_connect_info::<SocketAddr>()` so the extractor has the data to extract. ## Pre-existing trust gap (separate issue) This patch does NOT address: `extract_client_ip` honors `X-Forwarded-For` from any peer. With IP auto-login now actually working, anyone who can reach the listener directly can spoof `X-Forwarded-For: 127.0.0.1` and impersonate the operator. Pre-existing in current code; the patch inherits it. Filing as a separate issue: add `trusted_proxies: Vec<IpNetwork>` setting; only honor `X-Real-Ip` / `X-Forwarded-For` when the immediate peer is in that list. Default empty (= edge mode). ## PR Fix in `feat/peer-addr-fallback`. ~30 LOC. No schema change.