From 9b3fe23499e3bf679485e6f747f6966b38b4f4f8 Mon Sep 17 00:00:00 2001 From: "glm-5.1" Date: Fri, 12 Jun 2026 05:05:10 +0000 Subject: [PATCH] Add clarifying comments for correct-but-non-obvious behaviors (C3, W8, W10, W11, S9) --- src/config/validation.rs | 4 ++++ src/proxy/body_limit.rs | 4 ++++ src/proxy/handler.rs | 3 +++ src/proxy/headers.rs | 4 ++++ src/rate_limit/mod.rs | 5 +++++ 5 files changed, 20 insertions(+) diff --git a/src/config/validation.rs b/src/config/validation.rs index 03edc05..810dfe7 100644 --- a/src/config/validation.rs +++ b/src/config/validation.rs @@ -181,6 +181,10 @@ pub fn validate( } } + // Health check always binds to 127.0.0.1 (hardcoded in src/health.rs), so this + // conflict check is conservative — it warns even when the health check port + // wouldn't actually conflict (e.g., health check on 127.0.0.1:80 vs listener + // on 203.0.113.10:80). This is acceptable for Phase 1. if static_config.health_check_port > 0 { for listener in &static_config.listeners { if static_config.health_check_port == listener.https_port { diff --git a/src/proxy/body_limit.rs b/src/proxy/body_limit.rs index 7d231e7..3959db4 100644 --- a/src/proxy/body_limit.rs +++ b/src/proxy/body_limit.rs @@ -23,6 +23,10 @@ pub async fn body_limit_middleware( limit }; + // Early rejection: if Content-Length is present and exceeds the limit, reject + // immediately without reading the body. For requests without Content-Length + // (chunked, HTTP/2), the Limited body wrapper below enforces the limit during + // streaming. This is a two-layer defense. if let Some(content_length) = request.headers().get("content-length") { if let Ok(length_str) = content_length.to_str() { if let Ok(length) = length_str.parse::() { diff --git a/src/proxy/handler.rs b/src/proxy/handler.rs index ffd3716..261dd34 100644 --- a/src/proxy/handler.rs +++ b/src/proxy/handler.rs @@ -110,6 +110,9 @@ async fn proxy_handler( ); let (mut parts, body) = upstream_resp.into_parts(); remove_hop_by_hop(&mut parts.headers); + // The upstream Server header is intentionally removed. As a security-focused + // reverse proxy, we hide upstream server identity as a defense-in-depth measure. + // The proxy does not add its own Server header either. See W8 in review #002. parts.headers.remove("server"); let body = Body::new(body); Response::from_parts(parts, body) diff --git a/src/proxy/headers.rs b/src/proxy/headers.rs index e417569..ec2c7f1 100644 --- a/src/proxy/headers.rs +++ b/src/proxy/headers.rs @@ -25,6 +25,10 @@ pub fn inject_proxy_headers(headers: &mut HeaderMap, remote_addr: SocketAddr) { headers.insert(HeaderName::from_static("x-real-ip"), ip_value.clone()); + // X-Forwarded-For is SET (not appended) because this proxy is the outermost + // edge proxy. Any existing X-Forwarded-For from the client is untrusted and + // must be replaced with the actual client IP from ConnectInfo. + // See ADR-021 for the edge proxy model rationale. headers.insert(HeaderName::from_static("x-forwarded-for"), ip_value); // X-Forwarded-Proto is always "https" because this proxy only forwards requests diff --git a/src/rate_limit/mod.rs b/src/rate_limit/mod.rs index 69bc6e5..f80fc18 100644 --- a/src/rate_limit/mod.rs +++ b/src/rate_limit/mod.rs @@ -76,6 +76,11 @@ pub async fn rate_limit_middleware( }); let Some(ip) = client_ip else { + // If no client IP can be identified, the request passes through without rate + // limiting. In practice, ConnectInfo is always set by the server's + // ConnectInfoService, so this branch is unreachable. If the proxy were ever + // deployed without ConnectInfo propagation, rate limiting would silently become + // a no-op. Consider adding a warning log or returning 429 in a future phase. return next.run(req).await; };