From 626f2845e2fcd7e5662bba7c1ec18b78eba5e8c3 Mon Sep 17 00:00:00 2001 From: bsodfather Date: Sat, 11 Jul 2026 10:54:17 +0300 Subject: [PATCH] =?UTF-8?q?=D0=A3=D1=81=D0=B8=D0=BB=D0=B8=D1=82=D1=8C=20?= =?UTF-8?q?=D0=BF=D1=80=D0=BE=D0=B2=D0=B5=D1=80=D0=BA=D1=83=20=D0=B8=D1=81?= =?UTF-8?q?=D1=82=D0=BE=D1=87=D0=BD=D0=B8=D0=BA=D0=B0=20=D0=B8=20=D0=BE?= =?UTF-8?q?=D0=B3=D1=80=D0=B0=D0=BD=D0=B8=D1=87=D0=B5=D0=BD=D0=B8=D0=B5=20?= =?UTF-8?q?=D0=B7=D0=B0=D0=BF=D1=80=D0=BE=D1=81=D0=BE=D0=B2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .env.example | 3 + Cargo.lock | 1 + apps/admin-api/src/main.rs | 4 +- apps/admin-api/src/rate_limit.rs | 160 ++++++++++++++---- apps/admin-api/src/state.rs | 6 + .../tests/integration/auth_rate_limit.rs | 16 +- apps/admin-api/tests/integration/common.rs | 1 + crates/crank-community-mcp/Cargo.toml | 1 + crates/crank-community-mcp/src/transport.rs | 94 ++++++++-- deploy/community/.env.images.example | 4 + deploy/community/docker-compose.images.yml | 1 + deploy/community/docker-compose.yml | 1 + docker-compose.yml | 1 + 13 files changed, 236 insertions(+), 57 deletions(-) diff --git a/.env.example b/.env.example index eac43b9..3d4a62f 100644 --- a/.env.example +++ b/.env.example @@ -29,6 +29,9 @@ CRANK_MASTER_KEY=change-me-master-key CRANK_SESSION_SECRET=change-me-session-secret CRANK_PASSWORD_PEPPER=change-me-password-pepper CRANK_SESSION_TTL_HOURS=24 +# Trust X-Real-IP / X-Forwarded-For for client rate limiting. Enable only when +# admin-api runs behind the bundled nginx (or another trusted reverse proxy). +CRANK_TRUST_FORWARDED_HEADERS=true CRANK_BOOTSTRAP_ADMIN_EMAIL=owner@crank.local CRANK_BOOTSTRAP_ADMIN_PASSWORD=change-me-admin-password CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME=Crank Owner diff --git a/Cargo.lock b/Cargo.lock index 98ff761..99f3c80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -618,6 +618,7 @@ dependencies = [ "crank-schema", "crank-test-support", "futures-util", + "reqwest", "serde", "serde_json", "sha2 0.10.9", diff --git a/apps/admin-api/src/main.rs b/apps/admin-api/src/main.rs index 8906671..0d9e860 100644 --- a/apps/admin-api/src/main.rs +++ b/apps/admin-api/src/main.rs @@ -83,9 +83,11 @@ async fn main() -> Result<(), Box> { } else { RequestRateLimiter::new(api_rate_limit) }, + trust_forwarded_headers: env_flag("CRANK_TRUST_FORWARDED_HEADERS"), }; let app = build_app(state); let listener = TcpListener::bind(socket_addr).await?; + let make_service = app.into_make_service_with_connect_info::(); info!( runtime_max_concurrent_unary = runtime_limits.max_concurrent_unary, @@ -101,7 +103,7 @@ async fn main() -> Result<(), Box> { ); info!("admin-api listening on {}", socket_addr); - axum::serve(listener, app).await?; + axum::serve(listener, make_service).await?; Ok(()) } diff --git a/apps/admin-api/src/rate_limit.rs b/apps/admin-api/src/rate_limit.rs index 0100eb2..b41fec8 100644 --- a/apps/admin-api/src/rate_limit.rs +++ b/apps/admin-api/src/rate_limit.rs @@ -1,19 +1,30 @@ +use std::net::{IpAddr, SocketAddr}; + use axum::{ - extract::{Request, State}, - http::header::{COOKIE, HeaderMap}, + extract::{ConnectInfo, Request, State}, + http::HeaderMap, middleware::Next, response::Response, }; use crank_runtime::RateLimitRejection; -use crate::{auth::SESSION_COOKIE_NAME, error::ApiError, state::AppState}; +use crate::{error::ApiError, state::AppState}; pub async fn apply_api_rate_limit( State(state): State, request: Request, next: Next, ) -> Result { - let key = rate_limit_key(request.headers(), request.uri().path()); + let peer_ip = request + .extensions() + .get::>() + .map(|ConnectInfo(address)| address.ip()); + let key = rate_limit_key( + request.headers(), + request.uri().path(), + peer_ip, + state.trust_forwarded_headers, + ); if let Err(rejection) = state.api_rate_limiter.check(&key).await { return Err(ApiError::rate_limited_with_context( "request rate limit exceeded", @@ -30,56 +41,64 @@ fn rejection_context(rejection: RateLimitRejection) -> serde_json::Value { }) } -fn rate_limit_key(headers: &HeaderMap, path: &str) -> String { - if let Some(session_id) = session_id_from_headers(headers) { - return format!("session:{session_id}"); +fn rate_limit_key( + headers: &HeaderMap, + path: &str, + peer_ip: Option, + trust_forwarded_headers: bool, +) -> String { + if trust_forwarded_headers && let Some(client_ip) = forwarded_client_ip(headers) { + return format!("ip:{client_ip}"); } - if let Some(forwarded_for) = header_value(headers, "x-forwarded-for") { - let ip = forwarded_for - .split(',') - .next() - .map(str::trim) - .filter(|value| !value.is_empty()) - .unwrap_or("unknown"); - return format!("ip:{ip}"); - } - - if let Some(real_ip) = header_value(headers, "x-real-ip") { - return format!("ip:{real_ip}"); + if let Some(peer_ip) = peer_ip { + return format!("ip:{peer_ip}"); } format!("anonymous:{path}") } -fn session_id_from_headers(headers: &HeaderMap) -> Option { - let cookies = headers.get(COOKIE)?.to_str().ok()?; - for part in cookies.split(';') { - let (name, value) = part.trim().split_once('=')?; - if name != SESSION_COOKIE_NAME { - continue; - } - let (session_id, _) = value.split_once('.')?; - if !session_id.is_empty() { - return Some(session_id.to_owned()); - } +/// Resolves the client IP from proxy headers, assuming a single trusted proxy. +/// +/// `X-Real-IP` is preferred because a trusted proxy (e.g. nginx) sets it to the +/// real peer address. For `X-Forwarded-For` the proxy *appends* the observed +/// peer, so the last entry is the trustworthy hop; taking the first entry (as +/// naive implementations do) would let a client spoof its address by sending a +/// pre-populated header. +fn forwarded_client_ip(headers: &HeaderMap) -> Option { + if let Some(real_ip) = header_value(headers, "x-real-ip").and_then(parse_ip) { + return Some(real_ip); } - None + header_value(headers, "x-forwarded-for")? + .split(',') + .map(str::trim) + .rfind(|value| !value.is_empty()) + .and_then(parse_ip) } fn header_value<'a>(headers: &'a HeaderMap, name: &'static str) -> Option<&'a str> { headers.get(name)?.to_str().ok().map(str::trim) } +fn parse_ip(value: &str) -> Option { + value.parse().ok() +} + #[cfg(test)] mod tests { + use std::net::{IpAddr, Ipv4Addr}; + use axum::http::{HeaderMap, HeaderValue, header::COOKIE}; use super::rate_limit_key; + fn peer() -> Option { + Some(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 7))) + } + #[test] - fn keys_by_session_cookie_first() { + fn unverified_session_cookie_cannot_change_client_key() { let mut headers = HeaderMap::new(); headers.insert( COOKIE, @@ -88,19 +107,86 @@ mod tests { headers.insert("x-forwarded-for", HeaderValue::from_static("10.0.0.5")); assert_eq!( - rate_limit_key(&headers, "/api/auth/login"), - "session:sess_123" + rate_limit_key(&headers, "/api/auth/login", peer(), true), + "ip:10.0.0.5" ); } #[test] - fn falls_back_to_forwarded_ip() { + fn ignores_forwarded_headers_when_untrusted() { + let mut headers = HeaderMap::new(); + headers.insert("x-forwarded-for", HeaderValue::from_static("10.0.0.5")); + headers.insert("x-real-ip", HeaderValue::from_static("10.0.0.9")); + + assert_eq!( + rate_limit_key(&headers, "/api/auth/login", peer(), false), + "ip:203.0.113.7" + ); + } + + #[test] + fn prefers_real_ip_when_trusted() { let mut headers = HeaderMap::new(); headers.insert( "x-forwarded-for", - HeaderValue::from_static("10.0.0.5, 10.0.0.6"), + HeaderValue::from_static("1.2.3.4, 10.0.0.6"), + ); + headers.insert("x-real-ip", HeaderValue::from_static("10.0.0.9")); + + assert_eq!( + rate_limit_key(&headers, "/api/auth/login", peer(), true), + "ip:10.0.0.9" + ); + } + + #[test] + fn uses_last_forwarded_hop_when_trusted() { + // A client can prepend spoofed entries; the trusted proxy appends the + // real peer, so the last entry is authoritative. + let mut headers = HeaderMap::new(); + headers.insert( + "x-forwarded-for", + HeaderValue::from_static("1.2.3.4, 10.0.0.6"), ); - assert_eq!(rate_limit_key(&headers, "/api/auth/login"), "ip:10.0.0.5"); + assert_eq!( + rate_limit_key(&headers, "/api/auth/login", peer(), true), + "ip:10.0.0.6" + ); + } + + #[test] + fn falls_back_to_peer_ip_without_headers() { + let headers = HeaderMap::new(); + + assert_eq!( + rate_limit_key(&headers, "/api/auth/login", peer(), true), + "ip:203.0.113.7" + ); + } + + #[test] + fn ignores_invalid_forwarded_ip_values() { + let mut headers = HeaderMap::new(); + headers.insert("x-real-ip", HeaderValue::from_static("not-an-ip")); + headers.insert( + "x-forwarded-for", + HeaderValue::from_static("198.51.100.8, also-not-an-ip"), + ); + + assert_eq!( + rate_limit_key(&headers, "/api/auth/login", peer(), true), + "ip:203.0.113.7" + ); + } + + #[test] + fn falls_back_to_path_without_peer() { + let headers = HeaderMap::new(); + + assert_eq!( + rate_limit_key(&headers, "/api/auth/login", None, false), + "anonymous:/api/auth/login" + ); } } diff --git a/apps/admin-api/src/state.rs b/apps/admin-api/src/state.rs index 99b5165..54b7293 100644 --- a/apps/admin-api/src/state.rs +++ b/apps/admin-api/src/state.rs @@ -5,4 +5,10 @@ use crank_runtime::RequestRateLimiter; pub struct AppState { pub service: AdminService, pub api_rate_limiter: RequestRateLimiter, + /// Whether to trust `X-Real-IP` / `X-Forwarded-For` for client identification. + /// + /// Only enable when the service sits behind a trusted reverse proxy that + /// overwrites these headers (e.g. the bundled nginx). When disabled the + /// real TCP peer address is used, which a client cannot spoof. + pub trust_forwarded_headers: bool, } diff --git a/apps/admin-api/tests/integration/auth_rate_limit.rs b/apps/admin-api/tests/integration/auth_rate_limit.rs index a0fb2df..f4d3b39 100644 --- a/apps/admin-api/tests/integration/auth_rate_limit.rs +++ b/apps/admin-api/tests/integration/auth_rate_limit.rs @@ -109,15 +109,19 @@ async fn rejects_rapid_login_requests_with_429() { api_rate_limiter: crank_runtime::RequestRateLimiter::new( crank_runtime::RequestRateLimitConfig::new(1, 1).unwrap(), ), + trust_forwarded_headers: false, }); let (shutdown_tx, shutdown_rx) = tokio::sync::oneshot::channel(); let handle = tokio::spawn(async move { - axum::serve(listener, app) - .with_graceful_shutdown(async move { - let _ = shutdown_rx.await; - }) - .await - .unwrap(); + axum::serve( + listener, + app.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(async move { + let _ = shutdown_rx.await; + }) + .await + .unwrap(); }); let client = reqwest::Client::new(); diff --git a/apps/admin-api/tests/integration/common.rs b/apps/admin-api/tests/integration/common.rs index 691fbb2..4d1df4a 100644 --- a/apps/admin-api/tests/integration/common.rs +++ b/apps/admin-api/tests/integration/common.rs @@ -104,6 +104,7 @@ pub(super) fn build_test_app( api_rate_limiter: crank_runtime::RequestRateLimiter::new( crank_runtime::RequestRateLimitConfig::new(10_000, 10_000).unwrap(), ), + trust_forwarded_headers: false, }) } diff --git a/crates/crank-community-mcp/Cargo.toml b/crates/crank-community-mcp/Cargo.toml index 16ca3cf..b6171e6 100644 --- a/crates/crank-community-mcp/Cargo.toml +++ b/crates/crank-community-mcp/Cargo.toml @@ -15,6 +15,7 @@ crank-registry = { path = "../crank-registry" } crank-runtime = { path = "../crank-runtime" } crank-schema = { path = "../crank-schema" } futures-util = "0.3" +reqwest.workspace = true serde.workspace = true serde_json.workspace = true sha2.workspace = true diff --git a/crates/crank-community-mcp/src/transport.rs b/crates/crank-community-mcp/src/transport.rs index 69b54d4..191359a 100644 --- a/crates/crank-community-mcp/src/transport.rs +++ b/crates/crank-community-mcp/src/transport.rs @@ -12,6 +12,7 @@ use axum::{ }, }; use futures_util::stream; +use reqwest::Url; use serde_json::Value; use crate::jsonrpc::{ @@ -42,21 +43,44 @@ impl AllowedOrigins { } pub(super) fn is_allowed(&self, origin: &str) -> bool { - if origin.starts_with("http://localhost") - || origin.starts_with("http://127.0.0.1") - || origin.starts_with("https://localhost") - || origin.starts_with("https://127.0.0.1") - { + let Some(origin) = parse_origin(origin, true) else { + return false; + }; + + if is_loopback_origin(&origin) { return true; } match &self.public_origin { - Some(public_origin) => public_origin == origin, + Some(public_origin) => public_origin == &origin.origin().ascii_serialization(), None => false, } } } +fn is_loopback_origin(origin: &Url) -> bool { + matches!(origin.host_str(), Some("localhost" | "127.0.0.1" | "[::1]")) +} + +fn parse_origin(value: &str, origin_only: bool) -> Option { + let origin = Url::parse(value).ok()?; + if !matches!(origin.scheme(), "http" | "https") + || origin.host_str().is_none() + || !origin.username().is_empty() + || origin.password().is_some() + { + return None; + } + + if origin_only + && (origin.path() != "/" || origin.query().is_some() || origin.fragment().is_some()) + { + return None; + } + + Some(origin) +} + pub(super) fn validate_origin( allowed_origins: &AllowedOrigins, headers: &HeaderMap, @@ -298,14 +322,58 @@ pub(super) fn is_valid_request_id(value: &str) -> bool { } pub(super) fn extract_origin(url: &str) -> Option { - let mut parts = url.split('/'); - let scheme = parts.next()?; - let empty = parts.next()?; - let authority = parts.next()?; + Some(parse_origin(url, false)?.origin().ascii_serialization()) +} - if empty.is_empty() { - return Some(format!("{scheme}//{authority}")); +#[cfg(test)] +mod tests { + use super::AllowedOrigins; + + #[test] + fn allows_loopback_origins_with_and_without_port() { + let origins = AllowedOrigins::new(None); + + assert!(origins.is_allowed("http://localhost")); + assert!(origins.is_allowed("http://localhost:3000")); + assert!(origins.is_allowed("https://127.0.0.1:8443")); + assert!(origins.is_allowed("http://[::1]:3000")); } - None + #[test] + fn rejects_hosts_that_only_prefix_match_loopback() { + let origins = AllowedOrigins::new(None); + + assert!(!origins.is_allowed("http://localhost.attacker.com")); + assert!(!origins.is_allowed("http://127.0.0.1.attacker.com")); + assert!(!origins.is_allowed("http://localhost@attacker.com")); + assert!(!origins.is_allowed("http://attacker.com/http://localhost")); + assert!(!origins.is_allowed("http://[::1].attacker.com")); + assert!(!origins.is_allowed("http://attacker@[::1]")); + } + + #[test] + fn matches_configured_public_origin_exactly() { + let origins = AllowedOrigins::new(Some("https://crank.example.com".to_owned())); + + assert!(origins.is_allowed("https://crank.example.com")); + assert!(!origins.is_allowed("https://crank.example.com.attacker.com")); + assert!(!origins.is_allowed("https://evil.example.com")); + } + + #[test] + fn rejects_non_http_schemes() { + let origins = AllowedOrigins::new(None); + + assert!(!origins.is_allowed("file://localhost")); + assert!(!origins.is_allowed("javascript://localhost")); + } + + #[test] + fn rejects_values_that_are_not_origins() { + let origins = AllowedOrigins::new(None); + + assert!(!origins.is_allowed("http://localhost/path")); + assert!(!origins.is_allowed("http://localhost?query=value")); + assert!(!origins.is_allowed("http://localhost#fragment")); + } } diff --git a/deploy/community/.env.images.example b/deploy/community/.env.images.example index bb75d21..edc2cd1 100644 --- a/deploy/community/.env.images.example +++ b/deploy/community/.env.images.example @@ -26,6 +26,10 @@ CRANK_MASTER_KEY=change-me-master-key CRANK_SESSION_SECRET=change-me-session-secret CRANK_PASSWORD_PEPPER=change-me-password-pepper CRANK_SESSION_TTL_HOURS=24 +# Trust X-Real-IP / X-Forwarded-For for client rate limiting. Keep enabled only +# when admin-api runs behind the bundled nginx (or another trusted reverse +# proxy). Set to false if you expose admin-api directly to untrusted clients. +CRANK_TRUST_FORWARDED_HEADERS=true CRANK_BOOTSTRAP_ADMIN_EMAIL=owner@crank.local CRANK_BOOTSTRAP_ADMIN_PASSWORD=change-me-admin-password CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME=Crank Owner diff --git a/deploy/community/docker-compose.images.yml b/deploy/community/docker-compose.images.yml index dfbe41c..c11d783 100644 --- a/deploy/community/docker-compose.images.yml +++ b/deploy/community/docker-compose.images.yml @@ -54,6 +54,7 @@ services: CRANK_SESSION_SECRET: ${CRANK_SESSION_SECRET} CRANK_PASSWORD_PEPPER: ${CRANK_PASSWORD_PEPPER} CRANK_SESSION_TTL_HOURS: ${CRANK_SESSION_TTL_HOURS:-24} + CRANK_TRUST_FORWARDED_HEADERS: ${CRANK_TRUST_FORWARDED_HEADERS:-true} CRANK_BOOTSTRAP_ADMIN_EMAIL: ${CRANK_BOOTSTRAP_ADMIN_EMAIL} CRANK_BOOTSTRAP_ADMIN_PASSWORD: ${CRANK_BOOTSTRAP_ADMIN_PASSWORD} CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME: ${CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME:-Crank Owner} diff --git a/deploy/community/docker-compose.yml b/deploy/community/docker-compose.yml index 4bff0a6..f31dd38 100644 --- a/deploy/community/docker-compose.yml +++ b/deploy/community/docker-compose.yml @@ -38,6 +38,7 @@ services: CRANK_SESSION_SECRET: ${CRANK_SESSION_SECRET} CRANK_PASSWORD_PEPPER: ${CRANK_PASSWORD_PEPPER} CRANK_SESSION_TTL_HOURS: ${CRANK_SESSION_TTL_HOURS:-24} + CRANK_TRUST_FORWARDED_HEADERS: ${CRANK_TRUST_FORWARDED_HEADERS:-true} CRANK_BOOTSTRAP_ADMIN_EMAIL: ${CRANK_BOOTSTRAP_ADMIN_EMAIL} CRANK_BOOTSTRAP_ADMIN_PASSWORD: ${CRANK_BOOTSTRAP_ADMIN_PASSWORD} CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME: ${CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME:-Crank Owner} diff --git a/docker-compose.yml b/docker-compose.yml index ffdc1a1..084ef18 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -36,6 +36,7 @@ services: CRANK_SESSION_SECRET: ${CRANK_SESSION_SECRET} CRANK_PASSWORD_PEPPER: ${CRANK_PASSWORD_PEPPER} CRANK_SESSION_TTL_HOURS: ${CRANK_SESSION_TTL_HOURS:-24} + CRANK_TRUST_FORWARDED_HEADERS: ${CRANK_TRUST_FORWARDED_HEADERS:-true} CRANK_BOOTSTRAP_ADMIN_EMAIL: ${CRANK_BOOTSTRAP_ADMIN_EMAIL} CRANK_BOOTSTRAP_ADMIN_PASSWORD: ${CRANK_BOOTSTRAP_ADMIN_PASSWORD} CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME: ${CRANK_BOOTSTRAP_ADMIN_DISPLAY_NAME:-Crank Owner}