fix(server): trusted-proxy client-IP extraction for rate-limit/audit keying
Resolves coord todo 3c1f372a (Task-4 review SHOULD-FIX). Behind NPM-on-loopback, ConnectInfo was 127.0.0.1 so the rate limiter + lockout bucketed every client under one IP. New shared utils::ip_extract::client_ip() honors X-Real-IP / X-Forwarded-For (rightmost-untrusted hop) ONLY when the TCP peer is a configured trusted proxy (CONNECT_TRUSTED_PROXIES env, default loopback, fail-closed); untrusted peers are keyed by their true peer IP (forged headers ignored). Wired into the 3 rate-limit middleware, the validate_code lockout feed, and the agent/ viewer WS handlers so the limiter, lockout, and audit ip_address all key on the real client consistently. 13 unit tests (spoof rejection, XFF walk, fail-safe defaults). Code-reviewed APPROVED. Not cargo-check-verified locally (no toolchain); build-host/CI verification follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -19,12 +19,15 @@
|
||||
//! the brute-force defense for the support-code space (the code-validate
|
||||
//! route reports per-attempt success/failure into it).
|
||||
//!
|
||||
//! Client IP is taken from axum's [`ConnectInfo<SocketAddr>`] (the same source
|
||||
//! the relay uses for `client_ip`). `X-Forwarded-For` is intentionally NOT
|
||||
//! trusted here: the server terminates behind a known reverse proxy (NPM), and
|
||||
//! honoring a client-settable header would let an attacker trivially rotate the
|
||||
//! limiter key. If/when per-proxy XFF handling is needed it must be gated on a
|
||||
//! trusted-proxy allowlist — tracked as a follow-up, not done blindly here.
|
||||
//! Client IP is the REAL client IP from the shared trusted-proxy-aware extractor
|
||||
//! ([`crate::utils::ip_extract::client_ip`]) — the same source the relay and the
|
||||
//! audit/event log use, so all three never drift. The extractor honors
|
||||
//! `X-Forwarded-For` / `X-Real-IP` ONLY when the TCP peer is a configured trusted
|
||||
//! proxy (default: loopback, since NPM runs on the same host); a header from an
|
||||
//! untrusted peer is attacker-spoofable and is ignored. Keying on the real client
|
||||
//! IP is what makes the per-IP limiter and the failure lockout per-actual-client
|
||||
//! rather than per-proxy — without it, every external client buckets under the
|
||||
//! proxy's loopback address and one abuser could lock out the whole fleet.
|
||||
//!
|
||||
//! Memory is bounded by pruning expired entries opportunistically on each call
|
||||
//! and capping the map size; an unbounded attacker rotating source IPs cannot
|
||||
@@ -339,7 +342,7 @@ pub async fn login_rate_limit(
|
||||
request: axum::extract::Request,
|
||||
next: axum::middleware::Next,
|
||||
) -> Response {
|
||||
let ip = addr.ip();
|
||||
let ip = crate::utils::ip_extract::client_ip(&addr, request.headers(), &state.trusted_proxies);
|
||||
if !state.rate_limits.login.check(ip) {
|
||||
tracing::warn!("Rate limit exceeded on /api/auth/login from {}", ip);
|
||||
return too_many_requests(
|
||||
@@ -357,7 +360,7 @@ pub async fn change_password_rate_limit(
|
||||
request: axum::extract::Request,
|
||||
next: axum::middleware::Next,
|
||||
) -> Response {
|
||||
let ip = addr.ip();
|
||||
let ip = crate::utils::ip_extract::client_ip(&addr, request.headers(), &state.trusted_proxies);
|
||||
if !state.rate_limits.change_password.check(ip) {
|
||||
tracing::warn!(
|
||||
"Rate limit exceeded on /api/auth/change-password from {}",
|
||||
@@ -389,7 +392,7 @@ pub async fn code_validate_rate_limit(
|
||||
request: axum::extract::Request,
|
||||
next: axum::middleware::Next,
|
||||
) -> Response {
|
||||
let ip = addr.ip();
|
||||
let ip = crate::utils::ip_extract::client_ip(&addr, request.headers(), &state.trusted_proxies);
|
||||
|
||||
// 1. Brute-force lockout takes precedence.
|
||||
if state.rate_limits.code_validate_lockout.is_locked(ip) {
|
||||
|
||||
Reference in New Issue
Block a user