Skip to main content

max / makenotwork

mt: stop trusting attacker-controlled X-Forwarded-For for rate limiting (ultra-fuzz C1) SmartIpKeyExtractor read X-Forwarded-For from every request, so any direct client could rotate the header to mint a fresh rate-limit bucket per request and bypass every limiter. Replace it with a TrustedProxyKeyExtractor that has no Default: the trusted-proxy set must be supplied explicitly. It honors X-Forwarded-For (rightmost entry, so an injected left hop is ignored even when the proxy appends) only when the direct peer is a trusted proxy, and otherwise keys on the peer and ignores the header. Trusting an untrusted client's XFF is now unwritable. Applied at all four limiter sites (write/search/auth + internal). The trusted set comes from TRUSTED_PROXIES (default loopback for the on-host Caddy). Also un-arms the internal limiter, which was XFF-bypassable. 7 extractor unit + 3 config-parse unit tests; 247 integration + 135 lib unit green; clippy clean. Closes the last Run #3 chronic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-19 02:36 UTC
Commit: 7f0555edbeb71a93c15c2414559d246228e0d1b8
Parent: 21d5371
8 files changed, +221 insertions, -12 deletions
M multithreaded/src/config.rs +56
@@ -1,5 +1,6 @@
1 1 //! Application configuration read from environment variables.
2 2
3 + use std::net::IpAddr;
3 4 use std::sync::Arc;
4 5 use uuid::Uuid;
5 6
@@ -16,6 +17,13 @@ pub struct Config {
16 17 pub s3: Option<S3Config>,
17 18 /// Shared secret for HMAC-signed internal API requests from MNW.
18 19 pub internal_shared_secret: Option<String>,
20 + /// Proxy IPs whose `X-Forwarded-For` the rate limiter trusts. A request
21 + /// whose direct peer is in this set is keyed on its forwarded client IP;
22 + /// any other peer is keyed on its own address and its XFF is ignored.
23 + /// Parsed from `TRUSTED_PROXIES` (comma-separated IPs); defaults to loopback
24 + /// (`127.0.0.1`, `::1`) for the on-host Caddy deployment. A loopback peer is
25 + /// only reachable on-box, so trusting its XFF cannot be spoofed remotely.
26 + pub trusted_proxies: Arc<[IpAddr]>,
19 27 }
20 28
21 29 #[derive(Clone)]
@@ -56,6 +64,54 @@ impl Config {
56 64 .unwrap_or(true),
57 65 s3: S3Config::from_env(),
58 66 internal_shared_secret: std::env::var("INTERNAL_SHARED_SECRET").ok(),
67 + trusted_proxies: parse_trusted_proxies(std::env::var("TRUSTED_PROXIES").ok().as_deref()),
59 68 }
60 69 }
61 70 }
71 +
72 + /// Parse `TRUSTED_PROXIES` (comma-separated IPs). Unset → loopback only; an
73 + /// explicit empty value → trust no proxy (every request keys on its peer).
74 + /// Unparseable entries are skipped with a warning rather than failing boot.
75 + fn parse_trusted_proxies(raw: Option<&str>) -> Arc<[IpAddr]> {
76 + match raw {
77 + None => Arc::from([IpAddr::from([127, 0, 0, 1]), IpAddr::from([0, 0, 0, 0, 0, 0, 0, 1])]),
78 + Some(s) => s
79 + .split(',')
80 + .map(str::trim)
81 + .filter(|s| !s.is_empty())
82 + .filter_map(|s| match s.parse::<IpAddr>() {
83 + Ok(ip) => Some(ip),
84 + Err(_) => {
85 + tracing::warn!(entry = %s, "ignoring unparseable TRUSTED_PROXIES entry");
86 + None
87 + }
88 + })
89 + .collect(),
90 + }
91 + }
92 +
93 + #[cfg(test)]
94 + mod tests {
95 + use super::*;
96 +
97 + #[test]
98 + fn unset_defaults_to_loopback() {
99 + let p = parse_trusted_proxies(None);
100 + assert!(p.contains(&IpAddr::from([127, 0, 0, 1])));
101 + assert!(p.contains(&IpAddr::from([0, 0, 0, 0, 0, 0, 0, 1])));
102 + }
103 +
104 + #[test]
105 + fn explicit_empty_trusts_nobody() {
106 + assert_eq!(parse_trusted_proxies(Some("")).len(), 0);
107 + assert_eq!(parse_trusted_proxies(Some(" ")).len(), 0);
108 + }
109 +
110 + #[test]
111 + fn parses_list_and_skips_garbage() {
112 + let p = parse_trusted_proxies(Some("100.64.0.1, garbage, 10.0.0.2"));
113 + assert_eq!(p.len(), 2);
114 + assert!(p.contains(&IpAddr::from([100, 64, 0, 1])));
115 + assert!(p.contains(&IpAddr::from([10, 0, 0, 2])));
116 + }
117 + }
M multithreaded/src/lib.rs +1
@@ -10,6 +10,7 @@ pub mod routes;
10 10 pub mod seed;
11 11 pub mod storage;
12 12 pub mod templates;
13 + pub mod trusted_proxy;
13 14
14 15 use config::Config;
15 16 use sqlx::PgPool;
M multithreaded/src/main.rs +6 -6
@@ -130,12 +130,12 @@ async fn main() {
130 130 .merge(multithreaded::routes::internal::internal_routes(state))
131 131 .nest_service("/static", ServeDir::new("static"));
132 132
133 - // Default to loopback. Rate limiting uses SmartIpKeyExtractor, which trusts
134 - // X-Forwarded-For — that is only safe behind a reverse proxy (Caddy) that
135 - // *overwrites* the header. Binding to 127.0.0.1 by default keeps the app
136 - // unreachable except via that proxy, so XFF can't be spoofed by a direct
137 - // client. Environments that intentionally expose the port (e.g. a
138 - // tailnet-direct staging box) set HOST explicitly.
133 + // Default to loopback. Rate limiting uses TrustedProxyKeyExtractor, which
134 + // honors X-Forwarded-For only from a configured trusted proxy (TRUSTED_PROXIES,
135 + // default loopback) and otherwise keys on the direct peer — so a spoofed XFF
136 + // from a non-proxy client is ignored regardless of bind address. Binding to
137 + // 127.0.0.1 is still the right default (only Caddy reaches the port); a
138 + // tailnet-direct staging box sets HOST and lists its proxy in TRUSTED_PROXIES.
139 139 let host = std::env::var("HOST").unwrap_or_else(|_| "127.0.0.1".to_string());
140 140 let port = std::env::var("PORT").unwrap_or_else(|_| "3400".to_string());
141 141 let addr = format!("{host}:{port}");
M multithreaded/src/routes/internal.rs +4 -2
@@ -9,7 +9,7 @@ use axum::{
9 9 Json, Router,
10 10 };
11 11 use serde::{Deserialize, Serialize};
12 - use tower_governor::{GovernorLayer, governor::GovernorConfigBuilder, key_extractor::SmartIpKeyExtractor};
12 + use tower_governor::{GovernorLayer, governor::GovernorConfigBuilder};
13 13 use uuid::Uuid;
14 14
15 15 use crate::internal_auth::InternalAuth;
@@ -374,7 +374,9 @@ async fn create_post(
374 374 pub fn internal_routes(state: AppState) -> Router {
375 375 let internal_rate_limit = std::sync::Arc::new(
376 376 GovernorConfigBuilder::default()
377 - .key_extractor(SmartIpKeyExtractor)
377 + .key_extractor(crate::trusted_proxy::TrustedProxyKeyExtractor::new(
378 + state.config.trusted_proxies.clone(),
379 + ))
378 380 .per_millisecond(50)
379 381 .burst_size(60)
380 382 .finish()
M multithreaded/src/routes/mod.rs +6 -4
@@ -22,9 +22,11 @@ use axum::{
22 22 routing::{get, post},
23 23 };
24 24 use serde::Deserialize;
25 - use tower_governor::{GovernorLayer, governor::GovernorConfigBuilder, key_extractor::SmartIpKeyExtractor};
25 + use tower_governor::{GovernorLayer, governor::GovernorConfigBuilder};
26 26 use tower_sessions::Session;
27 27
28 + use crate::trusted_proxy::TrustedProxyKeyExtractor;
29 +
28 30 use crate::auth::{self, MaybeUser};
29 31 use crate::csrf;
30 32 use crate::templates::*;
@@ -55,7 +57,7 @@ const AUTH_RATE_LIMIT_BURST: u32 = 10;
55 57 pub fn forum_routes(state: AppState) -> Router {
56 58 let write_rate_limit = std::sync::Arc::new(
57 59 GovernorConfigBuilder::default()
58 - .key_extractor(SmartIpKeyExtractor)
60 + .key_extractor(TrustedProxyKeyExtractor::new(state.config.trusted_proxies.clone()))
59 61 .per_millisecond(WRITE_RATE_LIMIT_MS)
60 62 .burst_size(WRITE_RATE_LIMIT_BURST)
61 63 .finish()
@@ -109,7 +111,7 @@ pub fn forum_routes(state: AppState) -> Router {
109 111 // Search — rate limited per IP (expensive full-text queries)
110 112 let search_rate_limit = std::sync::Arc::new(
111 113 GovernorConfigBuilder::default()
112 - .key_extractor(SmartIpKeyExtractor)
114 + .key_extractor(TrustedProxyKeyExtractor::new(state.config.trusted_proxies.clone()))
113 115 .per_millisecond(SEARCH_RATE_LIMIT_MS)
114 116 .burst_size(SEARCH_RATE_LIMIT_BURST)
115 117 .finish()
@@ -126,7 +128,7 @@ pub fn forum_routes(state: AppState) -> Router {
126 128 // amplifier against MNW).
127 129 let auth_rate_limit = std::sync::Arc::new(
128 130 GovernorConfigBuilder::default()
129 - .key_extractor(SmartIpKeyExtractor)
131 + .key_extractor(TrustedProxyKeyExtractor::new(state.config.trusted_proxies.clone()))
130 132 .per_millisecond(AUTH_RATE_LIMIT_MS)
131 133 .burst_size(AUTH_RATE_LIMIT_BURST)
132 134 .finish()
A multithreaded/src/trusted_proxy.rs +146
@@ -0,0 +1,146 @@
1 + //! Rate-limit key extraction that never trusts an attacker-controlled
2 + //! `X-Forwarded-For`.
3 + //!
4 + //! `tower_governor`'s `SmartIpKeyExtractor` reads `X-Forwarded-For` from *every*
5 + //! request, so any direct client can mint a fresh rate-limit bucket per request
6 + //! by rotating the header — defeating every limiter. [`TrustedProxyKeyExtractor`]
7 + //! closes that: it honors `X-Forwarded-For` only when the request's *direct
8 + //! peer* is a configured trusted proxy, and otherwise keys on the peer address
9 + //! and ignores the header entirely. There is deliberately **no `Default`** — the
10 + //! set of trusted proxies must be supplied explicitly, so "trust this header"
11 + //! can never be the silent default that the prior extractor made it.
12 +
13 + use std::net::{IpAddr, SocketAddr};
14 + use std::sync::Arc;
15 +
16 + use axum::http::{HeaderMap, Request};
17 + use tower_governor::errors::GovernorError;
18 + use tower_governor::key_extractor::KeyExtractor;
19 +
20 + /// A [`KeyExtractor`] that keys on the real client IP, trusting
21 + /// `X-Forwarded-For` only when the direct peer is a known proxy.
22 + ///
23 + /// Construction requires the trusted-proxy set; there is no `Default`. An empty
24 + /// set trusts no proxy — every request keys on its direct peer, which is safe
25 + /// but collapses to a single global bucket behind a reverse proxy, so a proxied
26 + /// deployment must list its proxy (loopback for an on-host Caddy).
27 + #[derive(Clone, Debug)]
28 + pub struct TrustedProxyKeyExtractor {
29 + trusted_proxies: Arc<[IpAddr]>,
30 + }
31 +
32 + impl TrustedProxyKeyExtractor {
33 + /// Build with the explicit set of proxy IPs whose `X-Forwarded-For` is
34 + /// trusted. Trusting the header is a decision the caller must make here.
35 + pub fn new(trusted_proxies: impl Into<Arc<[IpAddr]>>) -> Self {
36 + Self { trusted_proxies: trusted_proxies.into() }
37 + }
38 +
39 + fn peer_ip<T>(req: &Request<T>) -> Option<IpAddr> {
40 + req.extensions()
41 + .get::<axum::extract::ConnectInfo<SocketAddr>>()
42 + .map(|ci| ci.0.ip())
43 + }
44 + }
45 +
46 + impl KeyExtractor for TrustedProxyKeyExtractor {
47 + type Key = IpAddr;
48 +
49 + // `name`/`key_name` are only trait methods under tower_governor's `tracing`
50 + // feature, which this crate does not enable; omitted deliberately.
51 +
52 + fn extract<T>(&self, req: &Request<T>) -> Result<Self::Key, GovernorError> {
53 + let peer = Self::peer_ip(req).ok_or(GovernorError::UnableToExtractKey)?;
54 + // Only a trusted proxy's X-Forwarded-For is believed. A direct (untrusted)
55 + // peer keys on its own socket address, so it cannot spoof another key.
56 + if self.trusted_proxies.contains(&peer)
57 + && let Some(client) = rightmost_xff(req.headers())
58 + {
59 + return Ok(client);
60 + }
61 + Ok(peer)
62 + }
63 + }
64 +
65 + /// The rightmost parseable `X-Forwarded-For` entry — the address the trusted
66 + /// proxy observed connecting to it. Taking the *rightmost* entry (not the
67 + /// leftmost) means a client-injected hop to the left of the proxy's appended
68 + /// entry is ignored, so spoofing fails even when the proxy appends rather than
69 + /// overwrites.
70 + fn rightmost_xff(headers: &HeaderMap) -> Option<IpAddr> {
71 + headers
72 + .get("x-forwarded-for")
73 + .and_then(|hv| hv.to_str().ok())
74 + .and_then(|s| s.rsplit(',').find_map(|p| p.trim().parse::<IpAddr>().ok()))
75 + }
76 +
77 + #[cfg(test)]
78 + mod tests {
79 + use super::*;
80 +
81 + fn req(peer: Option<&str>, xff: Option<&str>) -> Request<()> {
82 + let mut b = Request::builder();
83 + if let Some(xff) = xff {
84 + b = b.header("x-forwarded-for", xff);
85 + }
86 + let mut req = b.body(()).unwrap();
87 + if let Some(peer) = peer {
88 + let addr: SocketAddr = format!("{peer}:1234").parse().unwrap();
89 + req.extensions_mut().insert(axum::extract::ConnectInfo(addr));
90 + }
91 + req
92 + }
93 +
94 + fn extractor(trusted: &[&str]) -> TrustedProxyKeyExtractor {
95 + let ips: Vec<IpAddr> = trusted.iter().map(|s| s.parse().unwrap()).collect();
96 + TrustedProxyKeyExtractor::new(ips)
97 + }
98 +
99 + #[test]
100 + fn trusted_peer_uses_forwarded_client() {
101 + // Peer is the trusted proxy → believe XFF (rightmost = real client).
102 + let key = extractor(&["127.0.0.1"]).extract(&req(Some("127.0.0.1"), Some("203.0.113.7"))).unwrap();
103 + assert_eq!(key, "203.0.113.7".parse::<IpAddr>().unwrap());
104 + }
105 +
106 + #[test]
107 + fn untrusted_peer_ignores_forwarded_header() {
108 + // A direct client spoofing XFF gets keyed on its OWN address, not the
109 + // forged one — the whole point of the fix.
110 + let key = extractor(&["127.0.0.1"]).extract(&req(Some("198.51.100.9"), Some("203.0.113.7"))).unwrap();
111 + assert_eq!(key, "198.51.100.9".parse::<IpAddr>().unwrap());
112 + }
113 +
114 + #[test]
115 + fn trusted_peer_no_header_falls_back_to_peer() {
116 + let key = extractor(&["127.0.0.1"]).extract(&req(Some("127.0.0.1"), None)).unwrap();
117 + assert_eq!(key, "127.0.0.1".parse::<IpAddr>().unwrap());
118 + }
119 +
120 + #[test]
121 + fn rightmost_entry_defeats_injected_left_hop() {
122 + // Attacker sends "1.2.3.4", proxy appends the real client on the right.
123 + // Rightmost wins, so the injected left entry is ignored.
124 + let key = extractor(&["127.0.0.1"])
125 + .extract(&req(Some("127.0.0.1"), Some("1.2.3.4, 203.0.113.7")))
126 + .unwrap();
127 + assert_eq!(key, "203.0.113.7".parse::<IpAddr>().unwrap());
128 + }
129 +
130 + #[test]
131 + fn trusted_peer_garbage_header_falls_back_to_peer() {
132 + let key = extractor(&["127.0.0.1"]).extract(&req(Some("127.0.0.1"), Some("not-an-ip"))).unwrap();
133 + assert_eq!(key, "127.0.0.1".parse::<IpAddr>().unwrap());
134 + }
135 +
136 + #[test]
137 + fn empty_trusted_set_always_keys_on_peer() {
138 + let key = extractor(&[]).extract(&req(Some("203.0.113.7"), Some("10.0.0.1"))).unwrap();
139 + assert_eq!(key, "203.0.113.7".parse::<IpAddr>().unwrap());
140 + }
141 +
142 + #[test]
143 + fn no_peer_errors() {
144 + assert!(extractor(&["127.0.0.1"]).extract(&req(None, Some("203.0.113.7"))).is_err());
145 + }
146 + }
M multithreaded/tests/harness/mod.rs +1
@@ -62,6 +62,7 @@ impl TestHarness {
62 62 cookie_secure: false,
63 63 s3: None,
64 64 internal_shared_secret: None,
65 + trusted_proxies: std::sync::Arc::from([std::net::IpAddr::from([127, 0, 0, 1])]),
65 66 };
66 67
67 68 let state = AppState {
M multithreaded/tests/workflows/internal_api.rs +1
@@ -36,6 +36,7 @@ impl InternalTestHarness {
36 36 cookie_secure: false,
37 37 s3: None,
38 38 internal_shared_secret: Some(TEST_SECRET.to_string()),
39 + trusted_proxies: std::sync::Arc::from([std::net::IpAddr::from([127, 0, 0, 1])]),
39 40 };
40 41
41 42 let state = multithreaded::AppState {