diff --git a/.env.example b/.env.example index 63e12ef..a2a5579 100644 --- a/.env.example +++ b/.env.example @@ -51,6 +51,13 @@ SESSION_TTL_DAYS=30 # rate-limiting reverse proxy that already enforces a budget). AUTH_RATE_PER_SEC=5 AUTH_RATE_BURST=10 +# Trust a proxy-supplied X-Forwarded-For as the client IP for per-IP auth +# rate limiting. Enable ONLY when the backend sits behind a trusted proxy +# that overrides the header (the compose deploy: SvelteKit forwards the real +# peer IP). When false, the header is ignored and a single shared bucket is +# used — a directly-exposed backend MUST keep this off or clients could spoof +# their IP to dodge the limit. +AUTH_TRUSTED_PROXY=false # ----- CORS ----- # Comma-separated origins allowed to call the API with credentials. diff --git a/backend/Cargo.lock b/backend/Cargo.lock index e7d0c52..2e924a7 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1558,7 +1558,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.124.11" +version = "0.124.12" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 6e5d191..4287635 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.124.11" +version = "0.124.12" edition = "2021" default-run = "mangalord" diff --git a/backend/src/api/auth.rs b/backend/src/api/auth.rs index 4f5d615..22fef2c 100644 --- a/backend/src/api/auth.rs +++ b/backend/src/api/auth.rs @@ -17,7 +17,7 @@ use serde::{Deserialize, Serialize}; use uuid::Uuid; use crate::app::AppState; -use crate::auth::extractor::{CurrentUser, SESSION_COOKIE_NAME}; +use crate::auth::extractor::{ClientIp, CurrentUser, SESSION_COOKIE_NAME}; use crate::auth::password::{hash_password_async, verify_password_async}; use crate::auth::token::{generate_token, hash_token}; use crate::config::AuthConfig; @@ -107,6 +107,7 @@ pub struct CreatedTokenResponse { async fn register( State(state): State, + ClientIp(client_ip): ClientIp, jar: CookieJar, Json(input): Json, ) -> AppResult { @@ -114,7 +115,7 @@ async fn register( // the toggle can't be probed for the toggle state via timing — // disabled and enabled paths both consume a token, and disabled // returns 403 instead of running argon2. - check_auth_rate_limit(&state, "register")?; + check_auth_rate_limit(&state, "register", client_ip)?; // Private mode force-blocks self-registration regardless of // ALLOW_SELF_REGISTER — operators of locked-down instances mint // accounts via `POST /admin/users` instead. @@ -133,10 +134,11 @@ async fn register( async fn login( State(state): State, + ClientIp(client_ip): ClientIp, jar: CookieJar, Json(input): Json, ) -> AppResult { - check_auth_rate_limit(&state, "login")?; + check_auth_rate_limit(&state, "login", client_ip)?; let username = input.username.trim(); if username.is_empty() || input.password.is_empty() { return Err(AppError::InvalidInput( @@ -214,10 +216,11 @@ async fn me(CurrentUser(user): CurrentUser) -> AppResult> { async fn change_password( State(state): State, CurrentUser(user): CurrentUser, + ClientIp(client_ip): ClientIp, jar: CookieJar, Json(input): Json, ) -> AppResult { - check_auth_rate_limit(&state, "change_password")?; + check_auth_rate_limit(&state, "change_password", client_ip)?; // Cap current_password before verify_password runs argon2 (same DoS // vector as login). new_password is bounded by validate_password below. reject_oversized_password(&input.current_password)?; @@ -417,9 +420,13 @@ fn build_expired_cookie(cfg: &AuthConfig) -> Cookie<'static> { /// any one of them in a tight loop should trip the limit. `endpoint` /// is included in the rate-limit-hit log line so operators can tell /// which endpoint is being probed. -fn check_auth_rate_limit(state: &AppState, endpoint: &'static str) -> AppResult<()> { +fn check_auth_rate_limit( + state: &AppState, + endpoint: &'static str, + client_ip: Option, +) -> AppResult<()> { use crate::auth::rate_limit::AcquireResult; - match state.auth_limiter.try_acquire() { + match state.auth_limiter.try_acquire(client_ip) { AcquireResult::Allowed => Ok(()), AcquireResult::Denied { retry_after_secs } => { tracing::warn!( diff --git a/backend/src/auth/extractor.rs b/backend/src/auth/extractor.rs index 72b3755..fbc3efd 100644 --- a/backend/src/auth/extractor.rs +++ b/backend/src/auth/extractor.rs @@ -70,6 +70,45 @@ impl FromRequestParts for CurrentUser { } } +/// Client IP for per-IP auth rate limiting. Resolves to the first hop of +/// `X-Forwarded-For` **only** when [`crate::config::AuthConfig::trusted_proxy`] +/// is set (the backend is behind a proxy that overrides the header — the +/// compose deploy). Otherwise `None`, so the limiter uses its shared bucket. +/// Never fails: a missing or malformed header simply yields `None`. +pub struct ClientIp(pub Option); + +/// Parse the client IP from an `X-Forwarded-For` value: the left-most hop is +/// the original client (later hops are intermediary proxies). Factored out so +/// the parsing is unit-testable without constructing a request. +pub(crate) fn first_forwarded_ip(header: &str) -> Option { + header + .split(',') + .next() + .map(str::trim) + .filter(|s| !s.is_empty()) + .and_then(|s| s.parse::().ok()) +} + +#[async_trait] +impl FromRequestParts for ClientIp { + type Rejection = std::convert::Infallible; + + async fn from_request_parts( + parts: &mut Parts, + state: &AppState, + ) -> Result { + if !state.auth.trusted_proxy { + return Ok(ClientIp(None)); + } + let ip = parts + .headers + .get("x-forwarded-for") + .and_then(|v| v.to_str().ok()) + .and_then(first_forwarded_ip); + Ok(ClientIp(ip)) + } +} + /// Cookie-only authentication. Bot/API tokens are explicitly NOT accepted /// here — this is the substrate for [`RequireAdmin`] and exists precisely /// to keep admin authority out of bearer-token reach. @@ -120,3 +159,34 @@ impl FromRequestParts for RequireAdmin { Ok(RequireAdmin(user)) } } + +#[cfg(test)] +mod tests { + use super::first_forwarded_ip; + use std::net::IpAddr; + + #[test] + fn parses_left_most_client_hop() { + // The client is the first entry; later entries are intermediary proxies. + assert_eq!( + first_forwarded_ip("203.0.113.5, 10.0.0.1, 10.0.0.2"), + Some("203.0.113.5".parse::().unwrap()) + ); + assert_eq!( + first_forwarded_ip(" 198.51.100.9 "), + Some("198.51.100.9".parse::().unwrap()) + ); + assert_eq!( + first_forwarded_ip("2001:db8::1, 10.0.0.1"), + Some("2001:db8::1".parse::().unwrap()) + ); + } + + #[test] + fn rejects_empty_or_garbage() { + assert_eq!(first_forwarded_ip(""), None); + assert_eq!(first_forwarded_ip(" "), None); + assert_eq!(first_forwarded_ip("not-an-ip"), None); + assert_eq!(first_forwarded_ip(", 10.0.0.1"), None); + } +} diff --git a/backend/src/auth/rate_limit.rs b/backend/src/auth/rate_limit.rs index 1212651..38a9236 100644 --- a/backend/src/auth/rate_limit.rs +++ b/backend/src/auth/rate_limit.rs @@ -15,9 +15,17 @@ //! tests run in isolated buckets and won't bleed across `#[sqlx::test]` //! cases that share a process. +use std::collections::HashMap; +use std::net::IpAddr; use std::sync::Mutex; use std::time::Instant; +/// Upper bound on distinct client IPs tracked at once, so a spray from many +/// spoofed/rotating source IPs can't grow the map without limit. When full, +/// idle (refilled-to-burst) buckets are pruned first; if none are idle a new +/// IP falls back to the shared global bucket for that request. +const MAX_TRACKED_IPS: usize = 10_000; + /// Tunable limits. `per_sec == 0` disables the limiter — used by the /// test harness and by anyone who wants to opt out via env config. #[derive(Clone, Copy, Debug)] @@ -50,6 +58,42 @@ struct Bucket { last_refill: Instant, } +impl Bucket { + fn new(burst: u32) -> Self { + Self { + tokens: f64::from(burst), + last_refill: Instant::now(), + } + } + + /// Refill by elapsed time then try to consume one token. + fn try_take(&mut self, cfg: &RateLimitConfig, now: Instant) -> AcquireResult { + let elapsed = now.duration_since(self.last_refill).as_secs_f64(); + self.tokens = + (self.tokens + elapsed * f64::from(cfg.per_sec)).min(f64::from(cfg.burst)); + self.last_refill = now; + if self.tokens >= 1.0 { + self.tokens -= 1.0; + AcquireResult::Allowed + } else { + let deficit = 1.0 - self.tokens; + let wait_secs = (deficit / f64::from(cfg.per_sec)).ceil() as u64; + AcquireResult::Denied { + retry_after_secs: wait_secs.max(1), + } + } + } + + /// Whether this bucket has fully refilled (i.e. the client has been idle). + /// Such buckets carry no state worth keeping — dropping and lazily + /// recreating one yields an identical full bucket — so they're the safe + /// eviction target under memory pressure. + fn is_idle(&self, cfg: &RateLimitConfig, now: Instant) -> bool { + let elapsed = now.duration_since(self.last_refill).as_secs_f64(); + (self.tokens + elapsed * f64::from(cfg.per_sec)) >= f64::from(cfg.burst) + } +} + /// Outcome of [`AuthRateLimiter::try_acquire`]. When `Denied`, the /// caller can use `retry_after_secs` for a `Retry-After: N` header /// (RFC 6585 §4) so well-behaved clients back off correctly rather @@ -60,51 +104,64 @@ pub enum AcquireResult { Denied { retry_after_secs: u64 }, } -/// Single-bucket token-bucket limiter. `try_acquire` is cheap (one -/// mutex acquire, no allocations) so the auth path doesn't pay a real -/// cost for the check. +/// Token-bucket limiter keyed by client IP, with a shared global bucket for +/// requests whose IP is unknown (no trusted proxy). +/// +/// The old design was a single global bucket. That let one attacker at the +/// sustained rate deny auth to *every* user (the bucket stayed drained). With +/// the SvelteKit proxy now forwarding the peer IP (`X-Forwarded-For`, honoured +/// only when `AUTH_TRUSTED_PROXY` is set), each source IP gets its own bucket, +/// so an attacker only throttles themselves. When no trustworthy IP is +/// available the caller passes `None` and the shared `global` bucket applies — +/// exactly the previous behaviour. pub struct AuthRateLimiter { cfg: RateLimitConfig, - bucket: Mutex, + global: Mutex, + per_ip: Mutex>, } impl AuthRateLimiter { pub fn new(cfg: RateLimitConfig) -> Self { Self { cfg, - bucket: Mutex::new(Bucket { - tokens: cfg.burst as f64, - last_refill: Instant::now(), - }), + global: Mutex::new(Bucket::new(cfg.burst)), + per_ip: Mutex::new(HashMap::new()), } } - /// Consume one token if available. Returns `Denied` with a - /// rounded-up seconds-until-refill so the caller can emit a - /// `Retry-After` header. - pub fn try_acquire(&self) -> AcquireResult { + /// Consume one token from the bucket for `key` (per-IP when `Some`, the + /// shared global bucket when `None`). Returns `Denied` with a rounded-up + /// seconds-until-refill so the caller can emit a `Retry-After` header. + pub fn try_acquire(&self, key: Option) -> AcquireResult { if self.cfg.per_sec == 0 { return AcquireResult::Allowed; } let now = Instant::now(); - let mut bucket = self.bucket.lock().expect("rate limiter mutex poisoned"); - let elapsed = now.duration_since(bucket.last_refill).as_secs_f64(); - bucket.tokens = - (bucket.tokens + elapsed * f64::from(self.cfg.per_sec)).min(f64::from(self.cfg.burst)); - bucket.last_refill = now; - if bucket.tokens >= 1.0 { - bucket.tokens -= 1.0; - AcquireResult::Allowed - } else { - // ceil((1 - tokens) / per_sec), minimum 1 — a `Retry-After: 0` - // would tell clients to retry immediately, which is what we're - // actively trying to discourage. - let deficit = 1.0 - bucket.tokens; - let wait_secs = (deficit / f64::from(self.cfg.per_sec)).ceil() as u64; - AcquireResult::Denied { - retry_after_secs: wait_secs.max(1), + let Some(ip) = key else { + return self + .global + .lock() + .expect("rate limiter mutex poisoned") + .try_take(&self.cfg, now); + }; + let mut map = self.per_ip.lock().expect("rate limiter mutex poisoned"); + if map.len() >= MAX_TRACKED_IPS && !map.contains_key(&ip) { + map.retain(|_, b| !b.is_idle(&self.cfg, now)); + if map.len() >= MAX_TRACKED_IPS { + // Still saturated with active attackers — degrade to the shared + // bucket rather than grow unbounded. Worst case is the old + // global-bucket behaviour under an extreme distributed flood. + drop(map); + return self + .global + .lock() + .expect("rate limiter mutex poisoned") + .try_take(&self.cfg, now); } } + map.entry(ip) + .or_insert_with(|| Bucket::new(self.cfg.burst)) + .try_take(&self.cfg, now) } } @@ -119,7 +176,7 @@ mod tests { burst: 0, }); for _ in 0..1000 { - assert_eq!(rl.try_acquire(), AcquireResult::Allowed); + assert_eq!(rl.try_acquire(None), AcquireResult::Allowed); } } @@ -130,10 +187,10 @@ mod tests { per_sec: 1, burst: 3, }); - assert_eq!(rl.try_acquire(), AcquireResult::Allowed); - assert_eq!(rl.try_acquire(), AcquireResult::Allowed); - assert_eq!(rl.try_acquire(), AcquireResult::Allowed); - match rl.try_acquire() { + assert_eq!(rl.try_acquire(None), AcquireResult::Allowed); + assert_eq!(rl.try_acquire(None), AcquireResult::Allowed); + assert_eq!(rl.try_acquire(None), AcquireResult::Allowed); + match rl.try_acquire(None) { AcquireResult::Denied { retry_after_secs } => { // Bucket is at ~0 tokens, refill rate 1/sec → ~1s wait. assert!( @@ -152,11 +209,11 @@ mod tests { per_sec: 10, burst: 1, }); - assert_eq!(rl.try_acquire(), AcquireResult::Allowed); - assert!(matches!(rl.try_acquire(), AcquireResult::Denied { .. })); + assert_eq!(rl.try_acquire(None), AcquireResult::Allowed); + assert!(matches!(rl.try_acquire(None), AcquireResult::Denied { .. })); std::thread::sleep(std::time::Duration::from_millis(150)); assert_eq!( - rl.try_acquire(), + rl.try_acquire(None), AcquireResult::Allowed, "token should have refilled" ); @@ -170,10 +227,64 @@ mod tests { per_sec: 1, burst: 1, }); - slow.try_acquire(); - match slow.try_acquire() { + slow.try_acquire(None); + match slow.try_acquire(None) { AcquireResult::Denied { retry_after_secs } => assert_eq!(retry_after_secs, 1), _ => panic!("expected Denied"), } } + + fn ip(s: &str) -> Option { + Some(s.parse().unwrap()) + } + + #[test] + fn per_ip_buckets_are_independent() { + // The whole point of the fix: one IP draining its bucket must not deny + // a different IP. + let rl = AuthRateLimiter::new(RateLimitConfig { + per_sec: 1, + burst: 2, + }); + let a = ip("203.0.113.7"); + let b = ip("198.51.100.9"); + assert_eq!(rl.try_acquire(a), AcquireResult::Allowed); + assert_eq!(rl.try_acquire(a), AcquireResult::Allowed); + assert!(matches!(rl.try_acquire(a), AcquireResult::Denied { .. })); + // b is untouched. + assert_eq!(rl.try_acquire(b), AcquireResult::Allowed); + assert_eq!(rl.try_acquire(b), AcquireResult::Allowed); + assert!(matches!(rl.try_acquire(b), AcquireResult::Denied { .. })); + } + + #[test] + fn none_key_shares_the_global_bucket_independent_of_per_ip() { + let rl = AuthRateLimiter::new(RateLimitConfig { + per_sec: 1, + burst: 2, + }); + // Drain the global (None) bucket. + assert_eq!(rl.try_acquire(None), AcquireResult::Allowed); + assert_eq!(rl.try_acquire(None), AcquireResult::Allowed); + assert!(matches!(rl.try_acquire(None), AcquireResult::Denied { .. })); + // A real IP is on its own bucket, unaffected by the drained global one. + assert_eq!(rl.try_acquire(ip("203.0.113.1")), AcquireResult::Allowed); + } + + #[test] + fn tracked_ip_map_is_bounded() { + let rl = AuthRateLimiter::new(RateLimitConfig { + per_sec: 1, + burst: 1, + }); + // Distinct IPs beyond the cap must not grow the map without bound — + // excess requests fall back to the shared bucket instead. + for i in 0..(MAX_TRACKED_IPS as u64 + 50) { + let octet_a = (i >> 8) as u8; + let octet_b = (i & 0xff) as u8; + let addr = format!("10.20.{octet_a}.{octet_b}"); + let _ = rl.try_acquire(Some(addr.parse().unwrap())); + } + assert!(rl.per_ip.lock().unwrap().len() <= MAX_TRACKED_IPS); + } } diff --git a/backend/src/config.rs b/backend/src/config.rs index 552cd3b..25e9915 100644 --- a/backend/src/config.rs +++ b/backend/src/config.rs @@ -27,6 +27,14 @@ pub struct AuthConfig { /// so a private instance is locked down with a single switch. /// Defaults to `false` (current public behaviour). pub private_mode: bool, + /// Whether to trust a proxy-supplied `X-Forwarded-For` header as the + /// client IP for per-IP auth rate limiting. Enable ONLY when the backend + /// sits behind a trusted reverse proxy that overrides the header (the + /// compose deploy: SvelteKit's hooks.server.ts sets it from the real peer + /// address). When `false` (default), the header is ignored and the auth + /// limiter uses a single shared bucket — a directly-exposed backend must + /// keep this off or clients could spoof their IP to dodge the limit. + pub trusted_proxy: bool, } impl Default for AuthConfig { @@ -42,6 +50,7 @@ impl Default for AuthConfig { rate_limit: crate::auth::rate_limit::RateLimitConfig::default(), allow_self_register: true, private_mode: false, + trusted_proxy: false, } } } @@ -584,6 +593,7 @@ impl Config { }, allow_self_register: env_bool("ALLOW_SELF_REGISTER", true), private_mode: env_bool("PRIVATE_MODE", false), + trusted_proxy: env_bool("AUTH_TRUSTED_PROXY", false), }, upload: UploadConfig { max_request_bytes: env_usize("MAX_REQUEST_BYTES", 200 * 1024 * 1024), diff --git a/docker-compose.yml b/docker-compose.yml index eed7c01..fb9291e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -80,6 +80,10 @@ services: SESSION_TTL_DAYS: ${SESSION_TTL_DAYS:-30} AUTH_RATE_PER_SEC: ${AUTH_RATE_PER_SEC:-5} AUTH_RATE_BURST: ${AUTH_RATE_BURST:-10} + # The SvelteKit container is the single trusted hop in front of the + # backend and stamps the real client IP, so per-IP rate limiting is on + # by default here (unlike the backend's safe-by-default `false`). + AUTH_TRUSTED_PROXY: ${AUTH_TRUSTED_PROXY:-true} # CORS — same-origin by default; populate when serving the API on # a different host than the frontend. CORS_ALLOWED_ORIGINS: ${CORS_ALLOWED_ORIGINS:-} diff --git a/frontend/package.json b/frontend/package.json index 8fd9062..31e792c 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.124.11", + "version": "0.124.12", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/hooks.server.test.ts b/frontend/src/hooks.server.test.ts index 97137c5..e5653a2 100644 --- a/frontend/src/hooks.server.test.ts +++ b/frontend/src/hooks.server.test.ts @@ -9,6 +9,7 @@ import { } from 'vitest'; import { handle, + setForwardedFor, shouldBypassProxyTimeout, stripHopByHopHeaders } from './hooks.server'; @@ -351,3 +352,29 @@ describe('stripHopByHopHeaders', () => { expect(src.get('connection')).toBe('close'); }); }); + +describe('setForwardedFor', () => { + it('overrides any client-supplied x-forwarded-for with the real address', () => { + const headers = new Headers({ 'x-forwarded-for': '1.2.3.4', 'x-real-ip': '1.2.3.4' }); + setForwardedFor(headers, '203.0.113.9'); + expect(headers.get('x-forwarded-for')).toBe('203.0.113.9'); + // x-real-ip is dropped so it can't be used to spoof either. + expect(headers.get('x-real-ip')).toBeNull(); + }); + + it('strips the header entirely when no client address is available', () => { + const headers = new Headers({ 'x-forwarded-for': '9.9.9.9' }); + setForwardedFor(headers, undefined); + expect(headers.get('x-forwarded-for')).toBeNull(); + }); + + it('trims whitespace and treats blank as absent', () => { + const a = new Headers(); + setForwardedFor(a, ' 198.51.100.2 '); + expect(a.get('x-forwarded-for')).toBe('198.51.100.2'); + + const b = new Headers({ 'x-forwarded-for': 'spoofed' }); + setForwardedFor(b, ' '); + expect(b.get('x-forwarded-for')).toBeNull(); + }); +}); diff --git a/frontend/src/hooks.server.ts b/frontend/src/hooks.server.ts index 2ea07e2..b809092 100644 --- a/frontend/src/hooks.server.ts +++ b/frontend/src/hooks.server.ts @@ -47,6 +47,26 @@ export function stripHopByHopHeaders(src: Headers): Headers { return out; } +/** + * Stamp the real client address onto `x-forwarded-for` for the upstream + * request so axum can key its per-IP auth rate limiter on the actual client + * (the backend only honours this when `AUTH_TRUSTED_PROXY=true`). We + * **override** rather than append, and drop any incoming `x-forwarded-for` / + * `x-real-ip`, so a browser can't spoof its own IP to dodge the limit — this + * proxy is the single trusted hop. A missing/blank address leaves the headers + * untouched (backend then falls back to its shared bucket). Exported for + * unit-test coverage. + */ +export function setForwardedFor(headers: Headers, clientAddress: string | undefined): Headers { + headers.delete('x-real-ip'); + if (clientAddress && clientAddress.trim() !== '') { + headers.set('x-forwarded-for', clientAddress.trim()); + } else { + headers.delete('x-forwarded-for'); + } + return headers; +} + /** * Cap each proxied request at 5 minutes. The bound exists to surface * a wedged backend (stuck on a slow DB query, deadlocked, etc.) as a @@ -90,6 +110,17 @@ export const handle: Handle = async ({ event, resolve }) => { const target = `${BACKEND_URL}${event.url.pathname}${event.url.search}`; const headers = stripHopByHopHeaders(event.request.headers); + // Forward the real client IP for the backend's per-IP auth rate + // limiter, overriding any client-supplied value (anti-spoof). + // `getClientAddress()` throws if the adapter can't determine it — fall + // back to stripping the header so no spoofed value survives. + let clientAddress: string | undefined; + try { + clientAddress = event.getClientAddress(); + } catch { + clientAddress = undefined; + } + setForwardedFor(headers, clientAddress); // AbortController times the upstream fetch out so a backend // wedged on a slow DB query doesn't keep the browser request