diff --git a/backend/migrations/017_join_ip_rate.down.sql b/backend/migrations/017_join_ip_rate.down.sql new file mode 100644 index 0000000..86d16c5 --- /dev/null +++ b/backend/migrations/017_join_ip_rate.down.sql @@ -0,0 +1 @@ +DELETE FROM config WHERE key IN ('join_ip_rate_per_min', 'admin_login_rate_enabled'); diff --git a/backend/migrations/017_join_ip_rate.up.sql b/backend/migrations/017_join_ip_rate.up.sql new file mode 100644 index 0000000..135e49f --- /dev/null +++ b/backend/migrations/017_join_ip_rate.up.sql @@ -0,0 +1,18 @@ +-- Per-IP flood ceiling for /join, and the `admin_login_rate_enabled` toggle that +-- every prior migration forgot to seed. +-- +-- Rationale: /join was throttled at 5 requests per 60s keyed on the client IP. At a +-- venue every guest is behind one NAT, so the whole party shared a single bucket — +-- 12 guests scanning the QR code within a few seconds meant 5 got in and 7 were +-- turned away. The handler now keys the real anti-spam bucket per (ip, name), the +-- same shape as `recover:{ip}:{name}`, and keeps only a loose per-IP ceiling to bound +-- raw volume. 60/min comfortably covers a whole wedding arriving at once while still +-- capping a flood from a single source. +-- +-- `admin_login_rate_enabled` is read by auth::handlers::admin_login with a code +-- default of `true`, but no migration ever inserted it, so it was invisible to the +-- admin config UI and to the e2e reseed. Seed it explicitly. +INSERT INTO config (key, value) VALUES + ('join_ip_rate_per_min', '60'), + ('admin_login_rate_enabled', 'true') +ON CONFLICT (key) DO NOTHING; diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index f5ed12a..74f54f0 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -1,11 +1,12 @@ use std::time::Duration; use axum::Json; -use axum::extract::State; +use axum::extract::{ConnectInfo, State}; use axum::http::{HeaderMap, StatusCode}; use chrono::Utc; use rand::Rng; use serde::{Deserialize, Serialize}; +use std::net::SocketAddr; use uuid::Uuid; use crate::auth::jwt; @@ -33,22 +34,32 @@ pub struct JoinResponse { pub async fn join( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result<(StatusCode, Json), AppError> { - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let join_rate_on = config::get_bool(&state.config_cache, "join_rate_enabled", true).await; - if rate_limits_on - && join_rate_on - && !state - .rate_limiter - .check(format!("join:{ip}"), 5, Duration::from_secs(60)) - { - return Err(AppError::TooManyRequests( - "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, - )); + + // Coarse per-IP flood ceiling. `/join` is pre-auth so there is no user to key on, and + // at a venue EVERY guest arrives from one public IP — a tight per-IP bucket meant the + // 6th person through the door was turned away by the 5 ahead of them. So the per-IP + // limit here only bounds raw volume; the real anti-spam bucket is per-name below. + // Cheap enough to run before validation, which keeps a flood of malformed bodies from + // being free. + if rate_limits_on && join_rate_on { + let ip_ceiling = config::get_usize(&state.config_cache, "join_ip_rate_per_min", 60).await; + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("join_ip:{ip}"), + ip_ceiling, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), + Some(retry_after_secs), + )); + } } let display_name = body.display_name.trim(); @@ -66,6 +77,23 @@ pub async fn join( )); } + // Per-guest bucket, keyed like the `recover:{ip}:{name}` limiter below. This carries + // the original 5/60s anti-spam intent, but one guest retrying can no longer consume + // the allowance of everyone else sharing the venue's NAT. + if rate_limits_on && join_rate_on { + let name_key = display_name.to_lowercase(); + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("join:{ip}:{name_key}"), + 5, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), + Some(retry_after_secs), + )); + } + } + let event = Event::find_or_create( &state.pool, &state.config.event_slug, @@ -149,6 +177,7 @@ fn dummy_pin_hash() -> &'static str { pub async fn recover( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result, AppError> { @@ -159,19 +188,19 @@ pub async fn recover( // burn through 3 wrong PINs and lock the victim for 15 minutes — repeated // every 15 minutes, indefinitely. 5 attempts per 15 minutes per (IP, name) // softens that into a real cost. - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let recover_rate_on = config::get_bool(&state.config_cache, "recover_rate_enabled", true).await; if rate_limits_on && recover_rate_on { let name_key = display_name.to_lowercase(); - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("recover:{ip}:{name_key}"), 5, Duration::from_secs(15 * 60), ) { return Err(AppError::TooManyRequests( "Zu viele Versuche. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } @@ -200,9 +229,12 @@ pub async fn recover( // is effectively permanently fragile. if let Some(locked_until) = user.pin_locked_until { if Utc::now() < locked_until { + // The exact deadline is known, so surface it as Retry-After instead of + // making the client guess at the "15 Minuten" in the copy. + let retry_after_secs = (locked_until - Utc::now()).num_seconds().max(1) as u64; return Err(AppError::TooManyRequests( "Zu viele Versuche. Bitte warte 15 Minuten.".into(), - None, + Some(retry_after_secs), )); } // Lockout window expired — wipe the counter and the timestamp. @@ -274,6 +306,7 @@ pub struct AdminLoginResponse { pub async fn admin_login( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result, AppError> { @@ -287,19 +320,23 @@ pub async fn admin_login( // verify) but with no IP-level limit a determined attacker can still mount // a long-running guess campaign. 5 attempts / minute / IP is plenty for // honest typos. - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let admin_rate_on = config::get_bool(&state.config_cache, "admin_login_rate_enabled", true).await; + // Stays keyed by IP on purpose: this guards a single shared credential, so a per-user + // or per-name key would just hand an attacker a fresh bucket per guess. if rate_limits_on && admin_rate_on - && !state - .rate_limiter - .check(format!("admin_login:{ip}"), 5, Duration::from_secs(60)) + && let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("admin_login:{ip}"), + 5, + Duration::from_secs(60), + ) { return Err(AppError::TooManyRequests( "Zu viele Anmeldeversuche. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } @@ -390,22 +427,23 @@ pub struct PinResetRequestBody { /// feed already exposes. pub async fn request_pin_reset( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result { let display_name = body.display_name.trim(); - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; if rate_limits_on { let name_key = display_name.to_lowercase(); - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("pin_reset_req:{ip}:{name_key}"), 3, Duration::from_secs(15 * 60), ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index fb519c1..f29fe36 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -3,13 +3,13 @@ use std::time::Duration; use axum::Json; use axum::extract::{Query, State}; -use axum::http::{HeaderMap, StatusCode}; +use axum::http::StatusCode; use serde::{Deserialize, Serialize}; +use uuid::Uuid; use crate::auth::middleware::RequireAdmin; use crate::error::AppError; use crate::services::config; -use crate::services::rate_limiter::client_ip; use crate::state::AppState; // ── DTOs ───────────────────────────────────────────────────────────────────── @@ -120,6 +120,10 @@ pub async fn patch_config( ("upload_rate_per_hour", true, 1.0, 100_000.0), ("feed_rate_per_min", true, 1.0, 100_000.0), ("export_rate_per_day", true, 1.0, 100_000.0), + // Loose per-IP ceiling on /join. The real anti-spam bucket is per (ip, name); this + // only bounds raw volume from one source, so it must stay well above the size of a + // party arriving at once (see migration 017). + ("join_ip_rate_per_min", true, 1.0, 100_000.0), ("quota_tolerance", false, 0.0, 1.0), ("estimated_guest_count", true, 1.0, 1_000_000.0), ]; @@ -320,25 +324,26 @@ pub async fn export_ticket( } /// Validate a download ticket (single-use) and confirm its session still exists. -async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result<(), AppError> { +/// Resolve a single-use download ticket to the user who minted it. The caller needs the +/// id to key the export rate limit per-user (see `enforce_export_rate`). +async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result { let token_hash = state .sse_tickets .consume(ticket) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; - crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) + let session = crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) .await .map_err(|e| AppError::Internal(e.into()))? .ok_or_else(|| AppError::Unauthorized("Sitzung nicht gefunden.".into()))?; - Ok(()) + Ok(session.user_id) } pub async fn download_zip( State(state): State, Query(q): Query, - headers: HeaderMap, ) -> Result { - authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, &headers).await?; + let user_id = authenticate_download_ticket(&state, &q.ticket).await?; + enforce_export_rate(&state, user_id).await?; let path = resolve_export_file(&state, "zip", "Der ZIP-Export ist noch nicht verfügbar.").await?; @@ -389,10 +394,9 @@ async fn resolve_export_file( pub async fn download_html( State(state): State, Query(q): Query, - headers: HeaderMap, ) -> Result { - authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, &headers).await?; + let user_id = authenticate_download_ticket(&state, &q.ticket).await?; + enforce_export_rate(&state, user_id).await?; let path = resolve_export_file(&state, "html", "Der HTML-Export ist noch nicht verfügbar.").await?; @@ -476,21 +480,24 @@ pub async fn export_status( /// Centralised guard for the export rate limit. Same pattern as upload/feed: master /// switch + per-endpoint switch + numeric value, all stored in `config` and read on /// each request. -async fn enforce_export_rate(state: &AppState, headers: &HeaderMap) -> Result<(), AppError> { +async fn enforce_export_rate(state: &AppState, user_id: Uuid) -> Result<(), AppError> { let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let export_rate_on = config::get_bool(&state.config_cache, "export_rate_enabled", true).await; if !(rate_limits_on && export_rate_on) { return Ok(()); } - let ip = client_ip(headers, "unknown"); let limit = config::get_usize(&state.config_cache, "export_rate_per_day", 3).await; - if !state - .rate_limiter - .check(format!("export:{ip}"), limit, Duration::from_secs(86400)) - { + // Keyed per-user. This was the worst of the IP-keyed limiters: 3 downloads per DAY + // shared across every guest behind the venue's public IP, so the fourth person to + // fetch their keepsake was locked out until the next day. + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("export:{user_id}"), + limit, + Duration::from_secs(86400), + ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } Ok(()) diff --git a/backend/src/handlers/feed.rs b/backend/src/handlers/feed.rs index 3382539..2be8f5e 100644 --- a/backend/src/handlers/feed.rs +++ b/backend/src/handlers/feed.rs @@ -2,7 +2,6 @@ use std::time::Duration; use axum::Json; use axum::extract::{Query, State}; -use axum::http::HeaderMap; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -10,7 +9,6 @@ use uuid::Uuid; use crate::auth::middleware::AuthUser; use crate::error::AppError; use crate::services::config; -use crate::services::rate_limiter::client_ip; use crate::state::AppState; #[derive(Deserialize)] @@ -61,21 +59,23 @@ struct FeedRow { pub async fn feed( State(state): State, auth: AuthUser, - headers: HeaderMap, Query(q): Query, ) -> Result, AppError> { - let ip = client_ip(&headers, "unknown"); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let feed_rate_on = config::get_bool(&state.config_cache, "feed_rate_enabled", true).await; if rate_limits_on && feed_rate_on { let rate_limit = config::get_usize(&state.config_cache, "feed_rate_per_min", 60).await; - if !state - .rate_limiter - .check(format!("feed:{ip}"), rate_limit, Duration::from_secs(60)) - { + // Keyed per-user, exactly like `feed_delta` below: at a venue every guest shares + // one public IP, so an IP key gave the whole party a single 60/min bucket and the + // fastest scroller starved everyone else. + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("feed:{}", auth.user_id), + rate_limit, + Duration::from_secs(60), + ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } @@ -225,14 +225,14 @@ pub async fn feed_delta( let feed_rate_on = config::get_bool(&state.config_cache, "feed_rate_enabled", true).await; if rate_limits_on && feed_rate_on { let rate_limit = config::get_usize(&state.config_cache, "feed_rate_per_min", 60).await; - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("feed_delta:{}", auth.user_id), rate_limit, Duration::from_secs(60), ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } diff --git a/backend/src/handlers/test_admin.rs b/backend/src/handlers/test_admin.rs index 7e44c7c..c6f4202 100644 --- a/backend/src/handlers/test_admin.rs +++ b/backend/src/handlers/test_admin.rs @@ -13,9 +13,10 @@ use crate::auth::middleware::RequireAdmin; use crate::error::AppError; use crate::state::AppState; -/// Truncates every event-scoped table, wipes media on disk, and reseeds the -/// `config` table from migration defaults. Requires an admin JWT — even with -/// `EVENTSNAP_TEST_MODE=1` it cannot be hit anonymously. +/// Truncates every event-scoped table, wipes media on disk, and reseeds the `config` +/// table: numeric values from the migration defaults, but every feature toggle forced +/// OFF (production seeds them ON — see the note at the reseed below). Requires an admin +/// JWT — even with `EVENTSNAP_TEST_MODE=1` it cannot be hit anonymously. pub async fn truncate_all( State(state): State, RequireAdmin(_auth): RequireAdmin, @@ -40,8 +41,19 @@ pub async fn truncate_all( .execute(&state.pool) .await?; - // Reseed config — mirrors migrations 005, 009 and 015. Kept in sync by hand - // because pulling SQL out of the migration files at runtime is fragile. + // Reseed config. The NUMERIC values mirror migrations 005/015/016; the BOOLEAN + // toggles deliberately do NOT — migration 009 seeds every one of them `true` + // (production), and this forces them `false` so the suite isn't fighting rate limits + // and quotas it isn't testing. + // + // Be aware of what that costs: this runs as an auto-fixture before EVERY test, so no + // test starts from production's config unless it explicitly turns a toggle back on + // (02-upload/rate-limit, 07-adversarial/ddos, 01-auth/rate-limit-nat, …). That blind + // spot is exactly why an entire class of per-IP limiter bugs went unnoticed: the + // limiters were simply off. When adding a limiter or quota, add a spec that enables it. + // + // Kept in sync by hand because pulling SQL out of the migration files at runtime is + // fragile — if you add a config key in a migration, add it here too. sqlx::query( r#"INSERT INTO config (key, value) VALUES ('max_image_size_mb', '20'), @@ -49,6 +61,7 @@ pub async fn truncate_all( ('upload_rate_per_hour', '100'), ('feed_rate_per_min', '60'), ('export_rate_per_day', '3'), + ('join_ip_rate_per_min', '60'), ('quota_tolerance', '0.75'), ('estimated_guest_count', '100'), ('compression_concurrency', '2'), @@ -57,6 +70,7 @@ pub async fn truncate_all( ('feed_rate_enabled', 'false'), ('export_rate_enabled', 'false'), ('join_rate_enabled', 'false'), + ('admin_login_rate_enabled', 'false'), ('quota_enabled', 'false'), ('storage_quota_enabled', 'false'), ('upload_count_quota_enabled', 'false'), diff --git a/backend/src/main.rs b/backend/src/main.rs index e7327e1..bc64196 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -252,9 +252,15 @@ async fn main() -> Result<()> { let listener = tokio::net::TcpListener::bind(("0.0.0.0", config.app_port)).await?; tracing::info!("listening on {}", listener.local_addr()?); - axum::serve(listener, router) - .with_graceful_shutdown(shutdown_signal()) - .await?; + // `into_make_service_with_connect_info` is required by the pre-auth handlers, which + // extract `ConnectInfo` to use the peer address as the rate-limit key when + // X-Forwarded-For is absent. Without it those extractors fail at runtime. + axum::serve( + listener, + router.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(shutdown_signal()) + .await?; Ok(()) } diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index bc2f247..ca0df1f 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -17,13 +17,14 @@ impl RateLimiter { } } - /// Returns `true` if the request is allowed, `false` if rate-limited. - pub fn check(&self, key: impl Into, max: usize, window: Duration) -> bool { - self.check_with_retry(key, max, window).is_ok() - } - /// Returns `Ok(())` if allowed, `Err(retry_after_secs)` if rate-limited. /// `retry_after_secs` is how long until the oldest slot in the window expires. + /// + /// This is deliberately the ONLY entry point. There used to be a `check()` wrapper + /// returning a plain bool, and 7 of the 8 call sites used it and then hard-coded + /// `None` for the response's `Retry-After` — so a throttled client was told to back + /// off but never for how long. Forcing every caller through the `Result` makes the + /// retry delay impossible to discard by accident. pub fn check_with_retry( &self, key: impl Into, @@ -84,6 +85,11 @@ impl RateLimiter { /// appends is the real client. A client can prepend arbitrary spoofed values to /// the left of XFF to dodge throttles — those are ignored here. This assumes /// exactly one trusted proxy (Caddy); revisit if that changes. +/// +/// Pass the peer address as `fallback`, never a constant. Every caller used to pass +/// the literal `"unknown"`, so any request that arrived without XFF — i.e. anything +/// reaching the app directly rather than through Caddy — shared ONE bucket with every +/// other such request, turning the limiter into a self-inflicted global throttle. pub fn client_ip(headers: &axum::http::HeaderMap, fallback: &str) -> String { headers .get("x-forwarded-for") @@ -104,29 +110,35 @@ mod tests { #[test] fn allows_up_to_max_then_blocks() { let rl = RateLimiter::new(); - assert!(rl.check("k", 3, MIN)); - assert!(rl.check("k", 3, MIN)); - assert!(rl.check("k", 3, MIN)); - assert!(!rl.check("k", 3, MIN), "the 4th request must be blocked"); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!( + rl.check_with_retry("k", 3, MIN).is_err(), + "the 4th request must be blocked" + ); } #[test] fn keys_are_independent() { let rl = RateLimiter::new(); - assert!(rl.check("a", 1, MIN)); - assert!(!rl.check("a", 1, MIN)); - assert!(rl.check("b", 1, MIN), "a different key has its own window"); + assert!(rl.check_with_retry("a", 1, MIN).is_ok()); + assert!(rl.check_with_retry("a", 1, MIN).is_err()); + assert!( + rl.check_with_retry("b", 1, MIN).is_ok(), + "a different key has its own window" + ); } #[test] fn window_slides_and_allows_again_after_expiry() { let rl = RateLimiter::new(); let w = Duration::from_millis(40); - assert!(rl.check("k", 1, w)); - assert!(!rl.check("k", 1, w)); + assert!(rl.check_with_retry("k", 1, w).is_ok()); + assert!(rl.check_with_retry("k", 1, w).is_err()); std::thread::sleep(Duration::from_millis(55)); assert!( - rl.check("k", 1, w), + rl.check_with_retry("k", 1, w).is_ok(), "the slot should expire once the window passes" ); } @@ -191,10 +203,13 @@ mod tests { #[test] fn clear_resets_every_window() { let rl = RateLimiter::new(); - assert!(rl.check("k", 1, MIN)); - assert!(!rl.check("k", 1, MIN)); + assert!(rl.check_with_retry("k", 1, MIN).is_ok()); + assert!(rl.check_with_retry("k", 1, MIN).is_err()); rl.clear(); - assert!(rl.check("k", 1, MIN), "clear() must free the window"); + assert!( + rl.check_with_retry("k", 1, MIN).is_ok(), + "clear() must free the window" + ); } /// `prune()` is a memory-leak guard: without it a long-lived process keeps one HashMap @@ -216,7 +231,7 @@ mod tests { .insert("stale".to_string(), vec![ancient]); // ...alongside a key that is still inside its window. - assert!(rl.check("live", 5, MIN)); + assert!(rl.check_with_retry("live", 5, MIN).is_ok()); assert_eq!(rl.windows.lock().unwrap().len(), 2); rl.prune(); @@ -239,13 +254,13 @@ mod tests { // prune() dropped live keys, every background sweep would hand attackers a fresh // budget. let rl = RateLimiter::new(); - assert!(rl.check("k", 1, MIN)); - assert!(!rl.check("k", 1, MIN)); + assert!(rl.check_with_retry("k", 1, MIN).is_ok()); + assert!(rl.check_with_retry("k", 1, MIN).is_err()); rl.prune(); assert!( - !rl.check("k", 1, MIN), + rl.check_with_retry("k", 1, MIN).is_err(), "prune() must not clear a window that is still active" ); } diff --git a/e2e/specs/01-auth/join.spec.ts b/e2e/specs/01-auth/join.spec.ts index 58a2940..c5eadb9 100644 --- a/e2e/specs/01-auth/join.spec.ts +++ b/e2e/specs/01-auth/join.spec.ts @@ -13,7 +13,11 @@ test.describe('Auth — join flow', () => { const join = new JoinPage(page); await join.goto(); - await expect(page.getByRole('heading', { name: 'Willkommen!' })).toBeVisible(); + // The join form's landing state. There is no "Willkommen!" heading — the wedding + // redesign (f243bfe) split it into a "Willkommen bei" lead-in plus the event name as + // the

, and this assertion was never updated, so it had been failing since. + // Anchor on the testid the markup provides rather than on copy. + await expect(page.getByTestId('join-event-name')).toBeVisible(); const { pin } = await join.joinAs('Alice'); expect(pin).toMatch(/^\d{4}$/); diff --git a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts new file mode 100644 index 0000000..f025b30 --- /dev/null +++ b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts @@ -0,0 +1,144 @@ +/** + * Regression guard — the door must not close on a venue behind one NAT. + * + * `/join` was throttled 5 per 60s keyed purely on the client IP. Every guest at a venue + * arrives from the same public IP (that is what a NAT is), so the whole party shared one + * bucket: 12 guests scanning the QR code within a few seconds meant 5 got in and 7 were + * turned away — with no Retry-After to tell them when to try again. `/feed` (60/min) and + * `/export` (3/DAY) had the identical defect. + * + * These ran green for the same structural reason every time: the e2e reseed forces every + * limiter toggle OFF before each test, so nothing here was ever exercised. Enable them + * explicitly, exactly as 02-upload/rate-limit does. + */ +import { test, expect } from '../../fixtures/test'; +import { BASE } from '../../helpers/env'; + +test.describe('Rate limits — guests behind a shared NAT', () => { + test('a dozen guests can all join from one IP, and 429s carry Retry-After', async ({ + api, + adminToken, + }) => { + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + join_rate_enabled: 'true', + }); + + // Twelve DISTINCT guests, same source IP — the arrival burst at a real party. + const names = Array.from({ length: 12 }, (_, i) => `NatGuest${i}`); + const results = await Promise.all( + names.map((display_name) => + fetch(`${BASE}/api/v1/join`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name }), + }) + ) + ); + + const rejected = results.filter((r) => r.status === 429); + expect( + rejected.length, + `all 12 guests must get in from one IP; ${rejected.length} were turned away` + ).toBe(0); + expect(results.every((r) => r.status === 201)).toBe(true); + }); + + test('one guest retrying their own name is still throttled, and told for how long', async ({ + api, + adminToken, + }) => { + // The per-name bucket must still bite — otherwise the NAT fix would have simply + // removed the anti-spam limit rather than re-keyed it. + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + join_rate_enabled: 'true', + }); + + const attempt = () => + fetch(`${BASE}/api/v1/join`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: 'RepeatOffender' }), + }); + + // 5 per 60s for the same (ip, name): the first succeeds (201), the next four collide + // with the taken name (409), and the sixth exhausts the bucket. + const codes: number[] = []; + for (let i = 0; i < 6; i++) codes.push((await attempt()).status); + + expect(codes[0], 'the first join should succeed').toBe(201); + expect(codes.at(-1), 'the 6th attempt on one name must be throttled').toBe(429); + + const throttled = await attempt(); + expect(throttled.status).toBe(429); + const retryAfter = throttled.headers.get('retry-after'); + expect(retryAfter, '429 must tell the client when to come back').toBeTruthy(); + expect(Number(retryAfter)).toBeGreaterThan(0); + expect(Number(retryAfter)).toBeLessThanOrEqual(60); + }); + + test('the feed limit is per-user, not per-IP', async ({ api, adminToken, guest }) => { + // Two guests, one IP. With a limit of 3/min an IP key would let the first guest's + // three reads starve the second entirely. + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + feed_rate_enabled: 'true', + feed_rate_per_min: '3', + }); + + const a = await guest('FeedHog'); + const b = await guest('FeedVictim'); + const read = (jwt: string) => + fetch(`${BASE}/api/v1/feed`, { headers: { Authorization: `Bearer ${jwt}` } }); + + // Guest A burns their whole allowance. + for (let i = 0; i < 3; i++) expect((await read(a.jwt)).status).toBe(200); + expect((await read(a.jwt)).status, "A's own 4th read is throttled").toBe(429); + + // Guest B must be entirely unaffected. + expect((await read(b.jwt)).status, 'B must not inherit A’s exhausted bucket').toBe(200); + }); + + test('the export limit is per-user — one guest cannot spend the whole venue’s quota', async ({ + api, + adminToken, + guest, + host, + db, + }) => { + // The sharpest case: 3 downloads per DAY on an IP key meant the 4th guest to fetch + // their keepsake was locked out until tomorrow. + await db.setExportReleased('e2e-test-event', true); + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + export_rate_enabled: 'true', + export_rate_per_day: '1', + }); + + const mintAndFetch = async (jwt: string) => { + const res = await fetch(`${BASE}/api/v1/export/ticket`, { + method: 'POST', + headers: { Authorization: `Bearer ${jwt}` }, + }); + const { ticket } = await res.json(); + return fetch(`${BASE}/api/v1/export/zip?ticket=${encodeURIComponent(ticket)}`); + }; + + const a = await guest('ExportFirst'); + const b = await guest('ExportSecond'); + + // A spends their single daily allowance. The archive itself may not exist (404) — + // what matters is that the limiter admitted the request rather than 429ing it. + expect((await mintAndFetch(a.jwt)).status).not.toBe(429); + expect((await mintAndFetch(a.jwt)).status, 'A’s second download is throttled').toBe(429); + + // B shares A's IP and must still get their keepsake. + expect((await mintAndFetch(b.jwt)).status, 'B must not be locked out by A’s download').not.toBe( + 429 + ); + + // And the host too, for good measure. + expect((await mintAndFetch(host.jwt)).status).not.toBe(429); + }); +}); diff --git a/e2e/specs/07-adversarial/ddos.spec.ts b/e2e/specs/07-adversarial/ddos.spec.ts index 18fc752..c27b18c 100644 --- a/e2e/specs/07-adversarial/ddos.spec.ts +++ b/e2e/specs/07-adversarial/ddos.spec.ts @@ -15,7 +15,15 @@ test.describe('Adversarial — small-scale abuse', () => { await api.patchConfig(adminToken, { rate_limits_enabled: 'true', join_rate_enabled: 'true' }); }); - test('20 parallel /join from one IP — rate limiter catches the excess', async () => { + test('a /join flood from one IP is caught by the per-IP ceiling', async ({ api, adminToken }) => { + // This used to assert that 20 joins from one IP produced 429s under a 5/min per-IP + // bucket. That "protection" was the bug: at a venue every guest shares one public IP, + // so it turned real arriving guests away (see 01-auth/rate-limit-shared-nat). The + // anti-spam bucket is now per (ip, name); what remains per-IP is a loose ceiling whose + // job is only to bound raw volume. Squeeze the ceiling so a flood is reproducible here + // without firing 60+ requests. + await api.patchConfig(adminToken, { join_ip_rate_per_min: '5' }); + const requests = Array.from({ length: 20 }, (_, i) => fetch(`${BASE}/api/v1/join`, { method: 'POST', @@ -24,7 +32,7 @@ test.describe('Adversarial — small-scale abuse', () => { }) ); const statuses = (await Promise.all(requests)).map((r) => r.status); - // 5/min limit → at least some should be 429. + // Ceiling of 5 → the excess must be shed. expect(statuses.filter((s) => s === 429).length).toBeGreaterThan(0); // Server stays up — at least one succeeded. expect(statuses.some((s) => s === 201 || s === 409)).toBe(true);