diff --git a/backend/migrations/023_reserved_names_and_pin_decay.down.sql b/backend/migrations/023_reserved_names_and_pin_decay.down.sql new file mode 100644 index 0000000..ea485dd --- /dev/null +++ b/backend/migrations/023_reserved_names_and_pin_decay.down.sql @@ -0,0 +1,11 @@ +-- NOTE: the reserved-name rename in the up migration is NOT reversible. The original names +-- are not recorded anywhere, and reversing it would in any case re-create the state that +-- bricked admin login. Rolling back the schema does not roll back that data change. +DELETE FROM config WHERE key IN ( + 'recover_name_rate_per_15min', + 'pin_reset_ip_rate_per_min', + 'upload_edit_rate_per_min', + 'upload_edit_rate_enabled' +); + +ALTER TABLE "user" DROP COLUMN IF EXISTS last_failed_pin_at; diff --git a/backend/migrations/023_reserved_names_and_pin_decay.up.sql b/backend/migrations/023_reserved_names_and_pin_decay.up.sql new file mode 100644 index 0000000..7a3e926 --- /dev/null +++ b/backend/migrations/023_reserved_names_and_pin_decay.up.sql @@ -0,0 +1,48 @@ +-- Two independent auth defects that share a migration because they share a table. + +-- 1. RESERVED NAMES — free any guest squatting on a name the admin path used to depend on. +-- +-- Migration 007 made display_name unique per event case-insensitively, and `join` had no +-- reserved-name guard. So any guest could join as "admin"/"Admin"/"ADMIN" before the operator's +-- first admin login; admin_login then looked its user up BY NAME, missed (wrong role), fell +-- through to creating "Admin", violated that unique index, and returned a 500 — permanently, +-- with no in-app recovery. Moderation, config and gallery release all gone, fixed only by SQL. +-- +-- The real fix is in code (look the admin up by role, never by name — see auth/handlers.rs). +-- This clears the state an already-deployed database may be carrying. +-- +-- RENAMED, NEVER DELETED: the guest keeps their uploads, their PIN and their session. Only +-- non-admin rows are touched — a real admin row named "Admin" is the expected state. +UPDATE "user" u + SET display_name = u.display_name || ' (' || left(u.id::text, 8) || ')' + WHERE u.role <> 'admin' + AND lower(u.display_name) IN ('admin', 'administrator', 'host', 'eventsnap'); + +-- 2. PIN LOCKOUT DECAY. +-- +-- failed_pin_attempts only ever cleared on a successful recovery or after a lockout expired, so +-- honest typos accumulated across days: a guest who fat-fingered their PIN twice last night +-- arrives today already two-thirds of the way to being locked out. With the threshold now +-- raised (see below) a decay window is what keeps that raise safe rather than merely lenient. +ALTER TABLE "user" ADD COLUMN IF NOT EXISTS last_failed_pin_at TIMESTAMPTZ; + +-- Rate-limit knobs introduced with this release. +-- +-- recover_name_rate_per_15min (4, was a hardcoded 5): the per-(IP, name) ceiling. It MUST stay +-- below the account-lock threshold, which is the whole defect — at 5-per-IP against a 3-strike +-- lock, three requests from one IP locked any guest whose name is visible on the feed, every 15 +-- minutes, forever. The lock threshold moves to 12 in code, so locking a victim now needs at +-- least three distinct sources while an honest guest never comes close. +-- +-- pin_reset_ip_rate_per_min (30): /recover/request was the one unauthenticated endpoint with no +-- per-IP ceiling at all — /join got one in 017 and /recover in 019, and this third one was +-- simply missed. Its per-name key is attacker-chosen, so cycling names minted a fresh bucket +-- every time and the per-IP cost was unbounded. +-- +-- upload_edit_rate_per_min (30): PATCH /upload/{id} had no rate limit of any kind. +INSERT INTO config (key, value) VALUES + ('recover_name_rate_per_15min', '4'), + ('pin_reset_ip_rate_per_min', '30'), + ('upload_edit_rate_per_min', '30'), + ('upload_edit_rate_enabled', 'true') +ON CONFLICT (key) DO NOTHING; diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index b16e15b..3c515d1 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -19,6 +19,45 @@ use crate::services::config; use crate::services::rate_limiter::client_ip; use crate::state::AppState; +/// Names a guest may not take. +/// +/// Defence in depth only. The real fix for the admin-lockout defect is that `admin_login` now +/// resolves its user by ROLE rather than by name (see `User::find_admin_for_event`), which is +/// why homoglyph and zero-width bypasses of this list are not a concern: the name is no longer +/// load-bearing for anything. What this buys is that a guest cannot impersonate the host in the +/// feed's byline, and that "Admin" stays available for the admin row. +const RESERVED_DISPLAY_NAMES: &[&str] = &["admin", "administrator", "host", "eventsnap"]; + +fn is_reserved_display_name(name: &str) -> bool { + let name = name.trim().to_lowercase(); + RESERVED_DISPLAY_NAMES.contains(&name.as_str()) +} + +/// Trim and bounds-check a display name. +/// +/// Shared by `join`, `recover` and `request_pin_reset` so the length check happens BEFORE the +/// name is used to build a rate-limiter key. It was inline in `join` only, so on the other two +/// endpoints `format!("...:{ip}:{name_key}")` allocated from an unbounded, attacker-chosen +/// string and stored it in a HashMap pruned once an hour with a 24 h ceiling — turning the +/// limiter itself into the memory-exhaustion primitive it exists to prevent. +fn validate_display_name(raw: &str) -> Result<&str, AppError> { + let name = raw.trim(); + let chars = name.chars().count(); + if chars == 0 || chars > 50 { + return Err(AppError::BadRequest( + "Name muss zwischen 1 und 50 Zeichen lang sein.".into(), + )); + } + // Postgres rejects 0x00 in TEXT columns with a 500. Catch it here so callers see a clean + // 400 instead of an internal error. + if name.contains('\0') { + return Err(AppError::BadRequest( + "Name enthält ungültige Zeichen.".into(), + )); + } + Ok(name) +} + #[derive(Deserialize)] pub struct JoinRequest { pub display_name: String, @@ -62,19 +101,13 @@ pub async fn join( } } - let display_name = body.display_name.trim(); - let name_chars = display_name.chars().count(); - if name_chars == 0 || name_chars > 50 { - return Err(AppError::BadRequest( - "Name muss zwischen 1 und 50 Zeichen lang sein.".into(), - )); - } - // Postgres rejects 0x00 in TEXT columns with a 500. Catch it here so callers - // see a clean 400 instead of an internal error. - if display_name.contains('\0') { - return Err(AppError::BadRequest( - "Name enthält ungültige Zeichen.".into(), - )); + let display_name = validate_display_name(&body.display_name)?; + if is_reserved_display_name(display_name) { + // 409, matching the name-taken response below, so the frontend's existing handling + // works unchanged. See RESERVED_DISPLAY_NAMES for why this exists. + return Err(AppError::Conflict(format!( + "Der Name \"{display_name}\" ist reserviert. Bitte wähle einen anderen." + ))); } // Per-guest bucket, keyed like the `recover:{ip}:{name}` limiter below. This carries @@ -151,6 +184,28 @@ pub async fn join( )) } +/// Default for `recover_name_rate_per_15min` — wrong PINs allowed per (IP, name) per 15 min. +/// Mirrors migration 023; kept here so the invariant below can be asserted in a test. +const RECOVER_NAME_CEILING_DEFAULT: usize = 4; + +/// Wrong PINs, from ALL sources, before the ACCOUNT itself is locked for 15 minutes. +/// +/// This was 3, which sat BELOW the per-(IP, name) ceiling of 5 — and that ordering, not the +/// number, was the defect. Display names are public on the feed, so three requests from a single +/// IP locked any guest out of their own account, repeatable every 15 minutes, indefinitely. The +/// tier meant to protect a guest was the easiest way to attack them. +/// +/// Raised deliberately far above the per-IP tier so the two do different jobs. The per-(IP, name) +/// bucket is what stops a guesser, and it costs the ATTACKER. This tier is the last line against +/// a DISTRIBUTED guesser, and it is the only one an attacker can turn on a victim — so reaching +/// it must require at least three distinct sources inside the decay window. +/// +/// Brute-force cost is unchanged: 12 attempts per 15 minutes is 48/hour against one account, so +/// 10 000 four-digit PINs still take ~208 hours no matter how many IPs are used. An honest guest +/// fat-fingering a 4-digit PIN never comes close, and `increment_failed_pin` now decays the +/// streak after 15 minutes so yesterday's typos don't count toward today's. +const PIN_LOCK_THRESHOLD: i16 = 12; + #[derive(Deserialize)] pub struct RecoverRequest { pub display_name: String, @@ -203,13 +258,15 @@ pub async fn recover( headers: HeaderMap, Json(body): Json, ) -> Result, AppError> { - let display_name = body.display_name.trim(); + // Validated BEFORE it is used as a rate-limiter key — see `validate_display_name`. The + // per-IP ceiling below is keyed only on the IP, so it is safe to run either side of this; + // the per-NAME bucket is not. + let display_name = validate_display_name(&body.display_name)?; - // Per-IP+name throttle BEFORE the per-user 3-strike counter. Without this - // an attacker who knows a display name (they're visible on the feed) can - // 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. + // Per-IP+name throttle BEFORE the per-user lockout counter. Without this an attacker who + // knows a display name (they're visible on the feed) can burn through the victim's wrong-PIN + // budget and lock them out, repeatedly. The ceiling here MUST stay below + // PIN_LOCK_THRESHOLD — see the constant for why that ordering is the whole control. 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; @@ -234,10 +291,16 @@ pub async fn recover( )); } + let name_ceiling = config::get_usize( + &state.config_cache, + "recover_name_rate_per_15min", + RECOVER_NAME_CEILING_DEFAULT, + ) + .await; let name_key = display_name.to_lowercase(); if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("recover:{ip}:{name_key}"), - 5, + name_ceiling, Duration::from_secs(15 * 60), ) { return Err(AppError::TooManyRequests( @@ -317,7 +380,7 @@ pub async fn recover( attempts, "recover: wrong PIN" ); - if attempts >= 3 { + if attempts >= PIN_LOCK_THRESHOLD { let lockout = Utc::now() + chrono::Duration::minutes(15); User::lock_pin(&state.pool, user.id, lockout).await?; tracing::warn!( @@ -400,27 +463,16 @@ pub async fn admin_login( ) .await?; - // Find or create the admin user for this event - let admin_name = "Admin"; - let users = User::find_by_event_and_name(&state.pool, event.id, admin_name).await?; - let admin_user = if let Some(u) = users.into_iter().find(|u| u.role == UserRole::Admin) { - u - } else { - // Admin authenticates via password, but the schema still requires a PIN - // hash. Generate a random unguessable PIN so the recovery path remains - // unusable as an escalation route even if the role flag ever got cleared. - let dummy_pin: String = (0..32) - .map(|_| rand::rng().random_range(b'a'..=b'z') as char) - .collect(); - let dummy_hash = hash_password(dummy_pin.clone(), 4).await?; - let user = User::create(&state.pool, event.id, admin_name, &dummy_hash).await?; - sqlx::query("UPDATE \"user\" SET role = 'admin' WHERE id = $1") - .bind(user.id) - .execute(&state.pool) - .await?; - User::find_by_id(&state.pool, user.id) - .await? - .ok_or_else(|| AppError::Internal(anyhow::anyhow!("admin user creation failed")))? + // Find or create the admin user for this event — BY ROLE, never by name. + // + // The name lookup this replaces is what made admin login brickable. Migration 007 makes + // display_name unique per event case-insensitively and `join` had no reserved-name guard, + // so a guest joining as "admin" before the operator's first login made the lookup miss on + // role, the fallback `create("Admin")` violate that index, and `?` return a permanent 500 — + // taking out moderation, config and gallery release with no in-app recovery. + let admin_user = match User::find_admin_for_event(&state.pool, event.id).await? { + Some(u) => u, + None => create_admin_user(&state, event.id).await?, }; tracing::info!(user_id = %admin_user.id, event_id = %event.id, ip = %ip, "admin_login: success"); @@ -445,6 +497,51 @@ pub async fn admin_login( })) } +/// Create this event's admin row on first successful admin login. +/// +/// Prefers the name "Admin". If a legacy database has a guest squatting on it — the state +/// migration 023 renames away, but a row could also predate that or be created between +/// migrations — falls back to a suffixed name rather than failing the login. +/// +/// PROMOTING THE SQUATTING ROW WOULD BE A SERIOUS MISTAKE, and is the obvious-looking fix, so +/// it is spelled out: that row carries a `recovery_pin_hash` the guest knows. Setting +/// `role = 'admin'` on it would hand them the admin dashboard through `/recover`, permanently, +/// via a path that needs no password. A separate row under an uglier name is worse UX and much +/// better security — and since the lookup is now by role, the fallback name never has to be +/// guessed again on a later login. +async fn create_admin_user(state: &AppState, event_id: Uuid) -> Result { + // Admin authenticates via password, but the schema still requires a PIN hash. Generate a + // random unguessable one so the recovery path stays unusable as an escalation route even if + // the role flag were ever cleared. + let dummy_pin: String = (0..32) + .map(|_| rand::rng().random_range(b'a'..=b'z') as char) + .collect(); + let dummy_hash = hash_password(dummy_pin, 4).await?; + + match User::create_with_role(&state.pool, event_id, "Admin", &dummy_hash, UserRole::Admin).await + { + Ok(u) => Ok(u), + Err(sqlx::Error::Database(db)) if db.is_unique_violation() => { + let fallback = format!("Admin-{}", &Uuid::new_v4().to_string()[..8]); + tracing::warn!( + %event_id, %fallback, + "the name \"Admin\" is held by a non-admin user; creating the admin under a \ + fallback name. Rename that guest to free it — do NOT promote their row, they \ + know its recovery PIN." + ); + Ok(User::create_with_role( + &state.pool, + event_id, + &fallback, + &dummy_hash, + UserRole::Admin, + ) + .await?) + } + Err(e) => Err(e.into()), + } +} + pub async fn logout(State(state): State, auth: AuthUser) -> Result { Session::delete_by_token_hash(&state.pool, &auth.token_hash).await?; Ok(StatusCode::NO_CONTENT) @@ -476,9 +573,38 @@ pub async fn request_pin_reset( headers: HeaderMap, Json(body): Json, ) -> Result { - let display_name = body.display_name.trim(); let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; + + // Coarse per-IP ceiling FIRST, keyed only on the IP so its key is bounded by construction. + // /join got one of these in migration 017 and /recover in 019; this third unauthenticated + // endpoint was simply missed — migration 019's own comment describes exactly this attack. + // Without it, the per-name bucket below is no ceiling at all: the name is attacker-chosen, + // so cycling names mints a fresh bucket every request. + if rate_limits_on { + let ip_ceiling = + config::get_usize(&state.config_cache, "pin_reset_ip_rate_per_min", 30).await; + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("pin_reset_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), + )); + } + } + + // Validated BEFORE the per-name key is built, so an unbounded name can never be retained in + // the limiter map. NOTE the 204: this endpoint's contract is that it answers identically + // whether or not the name exists, so it cannot enumerate guests. A 400 here would be a new + // signal — it would distinguish a malformed name from a well-formed unknown one. Silence is + // the correct response, and matches what an empty name already did. + let Ok(display_name) = validate_display_name(&body.display_name) else { + return Ok(StatusCode::NO_CONTENT); + }; + if rate_limits_on { let name_key = display_name.to_lowercase(); if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( @@ -492,9 +618,6 @@ pub async fn request_pin_reset( )); } } - if display_name.is_empty() { - return Ok(StatusCode::NO_CONTENT); - } // Single statement so the existing-name and unknown-name paths do IDENTICAL work // (same event+user index scans, an INSERT that matches 0 rows for an unknown name) — @@ -524,3 +647,62 @@ pub async fn request_pin_reset( Ok(StatusCode::NO_CONTENT) } + +#[cfg(test)] +mod tests { + use super::*; + + /// THE defect, stated as arithmetic: the account-lock threshold sat BELOW the per-(IP, name) + /// attempt ceiling, so a single IP could exhaust it and lock any guest whose display name is + /// visible on the feed — every 15 minutes, indefinitely. The tier meant to protect a guest + /// was the cheapest way to attack them. + /// + /// The fix is the ORDERING, not either number on its own, so that is what this pins. + #[test] + fn one_ip_cannot_reach_the_account_lock() { + assert!( + PIN_LOCK_THRESHOLD as usize >= RECOVER_NAME_CEILING_DEFAULT * 3, + "locking a victim must require at least three distinct sources; \ + threshold {PIN_LOCK_THRESHOLD} vs per-IP ceiling {RECOVER_NAME_CEILING_DEFAULT}" + ); + } + + /// Raising the threshold must not quietly weaken brute-force resistance. 4-digit PINs, and + /// the lockout window is 15 minutes, so an attacker gets PIN_LOCK_THRESHOLD tries per window. + #[test] + fn the_raised_threshold_still_makes_guessing_a_four_digit_pin_impractical() { + let attempts_per_hour = PIN_LOCK_THRESHOLD as u64 * 4; // four 15-minute windows + let hours_for_full_keyspace = 10_000 / attempts_per_hour; + assert!( + hours_for_full_keyspace >= 168, + "exhausting 10k PINs would take {hours_for_full_keyspace}h — under a week is too fast" + ); + } + + #[test] + fn reserved_names_are_matched_case_insensitively_and_trimmed() { + for name in ["admin", "Admin", "ADMIN", " Host ", "EventSnap"] { + assert!(is_reserved_display_name(name), "{name} must be reserved"); + } + } + + /// A substring match here would reject perfectly ordinary names, which is a worse outcome + /// than the impersonation the list guards against. + #[test] + fn names_that_merely_contain_a_reserved_word_are_allowed() { + for name in ["Administrata", "Hostess", "Adminah", "Ghost", "hosting"] { + assert!(!is_reserved_display_name(name), "{name} must be allowed"); + } + } + + #[test] + fn display_names_are_bounded_before_they_can_become_a_rate_limit_key() { + assert!(validate_display_name(" Lena ").is_ok()); + assert_eq!(validate_display_name(" Lena ").unwrap(), "Lena"); + // The case that made the limiter itself the exhaustion primitive. + assert!(validate_display_name(&"a".repeat(51)).is_err()); + assert!(validate_display_name(&"a".repeat(2_000_000)).is_err()); + assert!(validate_display_name(" ").is_err()); + assert!(validate_display_name("bad\0name").is_err()); + } +} diff --git a/backend/src/handlers/sse.rs b/backend/src/handlers/sse.rs index 3fae809..4e9de30 100644 --- a/backend/src/handlers/sse.rs +++ b/backend/src/handlers/sse.rs @@ -38,7 +38,26 @@ pub async fn issue_ticket( State(state): State, auth: AuthUser, ) -> Result, AppError> { - let ticket = state.sse_tickets.issue(auth.token_hash); + // The endpoint had no rate limit at all. Authentication is not a bound here: one valid + // session could loop it freely. 60/min is far above a real client (one ticket per SSE + // (re)connect, and reconnects are backed off) while capping a loop. + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("sse_ticket:{}", auth.user_id), + 60, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Verbindungsversuche. Bitte warte kurz.".into(), + Some(retry_after_secs), + )); + } + + let ticket = state.sse_tickets.issue(auth.token_hash).ok_or_else(|| { + AppError::ServiceUnavailable( + "Server ist gerade ausgelastet. Live-Updates folgen in Kürze.".into(), + Some(30), + ) + })?; let server_time = sqlx::query_scalar("SELECT NOW()") .fetch_one(&state.pool) .await?; diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 35f82bb..2c6cd24 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -17,6 +17,79 @@ use crate::state::AppState; const MAX_CAPTION_LENGTH: usize = 2000; +/// Byte ceiling for the caption field, enforced WHILE reading it. +/// +/// `Field::text()` buffers the entire field before returning, and this is the one route whose +/// `DefaultBodyLimit` is raised to 576 MiB (main.rs) — so `caption=<576 MiB of text>` allocated +/// 576 MiB of heap per concurrent request inside a 1 GiB container, and the +/// `MAX_CAPTION_LENGTH` check only ran afterwards, on a string that had already been built. +/// 4 bytes per code point is the worst case for UTF-8, so this can never reject a caption the +/// character limit would have accepted. +const MAX_CAPTION_BYTES: usize = MAX_CAPTION_LENGTH * 4; + +/// Byte ceiling for the raw hashtag CSV. Generous next to what the tag caps below allow. +const MAX_HASHTAGS_BYTES: usize = 4 * 1024; + +/// Hashtags stored per upload. The CSV was never length-checked at all and was split into an +/// unbounded `Vec`, then upserted TAG BY TAG inside the commit transaction — which holds a +/// `FOR SHARE` lock on the event row, so one request could stall every other upload behind +/// tens of thousands of round trips. +const MAX_HASHTAGS_PER_UPLOAD: usize = 30; +/// Characters per stored tag. `extract_hashtags` already self-bounds at 40; this covers the CSV +/// path, which had no bound of its own. +const MAX_HASHTAG_LENGTH: usize = 50; + +/// Read a multipart text field, refusing it the moment it exceeds `max_bytes`. +/// +/// The point is to fail DURING the read rather than after it — `Field::text()` cannot, because +/// it has already allocated the whole thing by the time it returns. +async fn read_text_field_bounded( + mut field: axum::extract::multipart::Field<'_>, + max_bytes: usize, +) -> Result { + let mut buf: Vec = Vec::new(); + while let Some(chunk) = field + .chunk() + .await + .map_err(|e| AppError::BadRequest(e.to_string()))? + { + if buf.len() + chunk.len() > max_bytes { + return Err(AppError::BadRequest( + "Eingabe ist zu lang.".to_string(), + )); + } + buf.extend_from_slice(&chunk); + } + String::from_utf8(buf).map_err(|_| AppError::BadRequest("Ungültige Zeichenkodierung.".into())) +} + +/// Normalise, dedupe and CAP the tags for one upload. +/// +/// Extracted as a pure function so the caps are testable without standing up multipart, and +/// shared by the upload and edit paths — which previously disagreed: upload lowercased and +/// stripped `#`, while edit upserted raw strings, so `#Party` via edit and `party` via upload +/// became two different hashtag rows. +/// +/// Truncates rather than rejecting. `extract_hashtags` legitimately derives tags from a +/// 2000-character caption, and 400-ing a guest for writing an enthusiastic caption would be a +/// worse outcome than silently keeping the first 30. +fn normalize_tags(caption_tags: Vec, csv: Option<&str>) -> Vec { + let mut tags = caption_tags; + if let Some(csv) = csv { + for tag in csv.split(',') { + let t = tag.trim().trim_start_matches('#').to_lowercase(); + if !t.is_empty() { + tags.push(t); + } + } + } + tags.sort(); + tags.dedup(); + tags.retain(|t| t.chars().count() <= MAX_HASHTAG_LENGTH); + tags.truncate(MAX_HASHTAGS_PER_UPLOAD); + tags +} + /// Owns the bytes an in-flight upload has written to disk, and deletes them unless the /// request reaches the point where a database row takes ownership. /// @@ -204,20 +277,10 @@ pub async fn upload( streamed = Some(stream_field_to_file(field, &temp_abs, cap_bytes).await?); } "caption" => { - caption = Some( - field - .text() - .await - .map_err(|e| AppError::BadRequest(e.to_string()))?, - ); + caption = Some(read_text_field_bounded(field, MAX_CAPTION_BYTES).await?); } "hashtags" => { - hashtags_csv = Some( - field - .text() - .await - .map_err(|e| AppError::BadRequest(e.to_string()))?, - ); + hashtags_csv = Some(read_text_field_bounded(field, MAX_HASHTAGS_BYTES).await?); } _ => {} } @@ -344,21 +407,14 @@ pub async fn upload( // owns `temp_abs`, which is why retargeting comes after it rather than before. file_guard.retarget(absolute_path.clone()); - // Process hashtags from caption and explicit CSV - let mut tags: Vec = Vec::new(); - if let Some(ref cap) = caption { - tags.extend(hashtag::extract_hashtags(cap)); - } - if let Some(ref csv) = hashtags_csv { - for tag in csv.split(',') { - let t = tag.trim().trim_start_matches('#').to_lowercase(); - if !t.is_empty() { - tags.push(t); - } - } - } - tags.sort(); - tags.dedup(); + // Process hashtags from caption and explicit CSV, capped — see `normalize_tags`. + let tags = normalize_tags( + caption + .as_deref() + .map(hashtag::extract_hashtags) + .unwrap_or_default(), + hashtags_csv.as_deref(), + ); // Quota accounting, the upload row, and its hashtag links must be atomic: a // crash between the bytes increment and the insert would permanently charge @@ -501,6 +557,57 @@ pub async fn edit_upload( return Err(AppError::Forbidden("Nur eigene Uploads bearbeiten.".into())); } + // This endpoint had no rate limit of any kind, while every other mutating route has one. + let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; + let edit_rate_on = config::get_bool(&state.config_cache, "upload_edit_rate_enabled", true).await; + if rate_limits_on && edit_rate_on { + let edit_rate = + config::get_i64(&state.config_cache, "upload_edit_rate_per_min", 30).await as usize; + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("upload_edit:{}", auth.user_id), + edit_rate, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Änderungen. Bitte warte kurz.".into(), + Some(retry_after_secs), + )); + } + } + + // Validate to the same limits as the upload path. This route had none at all, so a caption + // rejected at upload could be set here instead, and the tags went in raw — meaning `#Party` + // via edit and `party` via upload became two different hashtag rows. + if let Some(ref caption) = body.caption + && caption.chars().count() > MAX_CAPTION_LENGTH + { + return Err(AppError::BadRequest(format!( + "Beschreibung ist zu lang. Maximum: {MAX_CAPTION_LENGTH} Zeichen." + ))); + } + let normalized_tags = body + .hashtags + .as_ref() + .map(|tags| normalize_tags(tags.clone(), None)); + + // A PATCH that changes nothing must not retire the keepsake generation. + // + // `invalidate_and_arm` below ran unconditionally, outside both `if let Some(...)` guards, so + // `PATCH {}` — which any authenticated guest can send in a loop against their own upload — + // bumped export_epoch and armed a fresh pair of full-gallery export workers every time. + // REGEN_DEBOUNCE bounds the rate of that, not the total work, so the keepsake could be kept + // permanently un-downloadable. + // + // Residual, deliberately not fixed: re-sending an IDENTICAL hashtag list still counts as a + // change. Comparing would need another query, and unlike `PATCH {}` it is not a free loop. + let caption_changed = match (&body.caption, &upload.caption) { + (Some(new), existing) => Some(new.as_str()) != existing.as_deref(), + (None, _) => false, + }; + if !caption_changed && normalized_tags.is_none() { + return Ok(StatusCode::OK); + } + // Caption update + hashtag wipe-then-relink in one transaction, so a crash // mid-relink can't leave the upload with its hashtags stripped. // @@ -516,7 +623,7 @@ pub async fn edit_upload( if let Some(ref caption) = body.caption { Upload::update_caption(&mut *tx, upload_id, Some(caption)).await?; } - if let Some(ref hashtags) = body.hashtags { + if let Some(ref hashtags) = normalized_tags { Hashtag::unlink_all_from_upload(&mut *tx, upload_id).await?; for tag in hashtags { let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?; @@ -1180,4 +1287,53 @@ mod tests { drop(TempFileGuard::new(p)); // must not panic } } + + /// The CSV hashtag path had no length check at all and was upserted tag-by-tag INSIDE the + /// commit transaction — which holds a FOR SHARE lock on the event row, so one request could + /// stall every other upload behind tens of thousands of round trips. + mod hashtag_caps { + use super::super::{MAX_HASHTAGS_PER_UPLOAD, MAX_HASHTAG_LENGTH, normalize_tags}; + + #[test] + fn a_huge_csv_is_capped_not_upserted_in_full() { + let csv = (0..10_000) + .map(|i| format!("tag{i}")) + .collect::>() + .join(","); + let tags = normalize_tags(vec![], Some(&csv)); + assert_eq!(tags.len(), MAX_HASHTAGS_PER_UPLOAD); + } + + #[test] + fn an_overlong_tag_is_dropped_rather_than_stored() { + let long = "a".repeat(MAX_HASHTAG_LENGTH + 1); + let tags = normalize_tags(vec![], Some(&format!("ok,{long}"))); + assert_eq!(tags, vec!["ok"]); + } + + #[test] + fn tags_are_normalised_and_deduped_across_both_sources() { + // The upload path lowercased and stripped `#` while the edit path did not, so + // `#Party` and `party` became two different hashtag rows. One helper, one rule. + let tags = normalize_tags(vec!["party".into()], Some("#Party, PARTY ,tanz")); + assert_eq!(tags, vec!["party", "tanz"]); + } + + #[test] + fn truncation_keeps_a_stable_prefix_not_an_arbitrary_one() { + // Sorted before truncation, so the same input always yields the same tags — + // otherwise an edit could silently shuffle which 30 survived. + let csv = "zulu,alpha,mike,bravo"; + assert_eq!( + normalize_tags(vec![], Some(csv)), + normalize_tags(vec![], Some("bravo,mike,alpha,zulu")) + ); + } + + #[test] + fn an_empty_or_absent_csv_yields_nothing() { + assert!(normalize_tags(vec![], None).is_empty()); + assert!(normalize_tags(vec![], Some(",, ,")).is_empty()); + } + } } diff --git a/backend/src/models/user.rs b/backend/src/models/user.rs index 8b1a7ae..cad4fb5 100644 --- a/backend/src/models/user.rs +++ b/backend/src/models/user.rs @@ -60,6 +60,54 @@ impl User { .await } + /// Create a user with an explicit role, in ONE statement. + /// + /// `create` + a separate `UPDATE ... SET role` is not equivalent: a crash or a pool error + /// between the two leaves a GUEST row holding a reserved name, which is exactly the + /// poisoned state that bricked admin login — now self-inflicted, and invisible to a + /// role-based lookup, so the next login would create yet another. + pub async fn create_with_role( + pool: &PgPool, + event_id: Uuid, + display_name: &str, + pin_hash: &str, + role: UserRole, + ) -> Result { + sqlx::query_as::<_, Self>( + "INSERT INTO \"user\" (event_id, display_name, recovery_pin_hash, role) + VALUES ($1, $2, $3, $4) + RETURNING *", + ) + .bind(event_id) + .bind(display_name) + .bind(pin_hash) + .bind(role) + .fetch_one(pool) + .await + } + + /// The event's admin, looked up BY ROLE. + /// + /// The name is not the identity and never was. Looking the admin up by `display_name` + /// meant any guest who joined as "Admin" first made the lookup miss, and the fallback + /// `create` then violated the case-insensitive unique index from migration 007 — a + /// permanent 500 on admin login, recoverable only by hand-editing the database. + /// + /// `ORDER BY created_at` so a database that somehow acquired two admin rows resolves to a + /// stable one rather than alternating between them. + pub async fn find_admin_for_event( + pool: &PgPool, + event_id: Uuid, + ) -> Result, sqlx::Error> { + sqlx::query_as::<_, Self>( + "SELECT * FROM \"user\" WHERE event_id = $1 AND role = 'admin' + ORDER BY created_at ASC LIMIT 1", + ) + .bind(event_id) + .fetch_optional(pool) + .await + } + pub async fn find_by_id(pool: &PgPool, id: Uuid) -> Result, sqlx::Error> { sqlx::query_as::<_, Self>("SELECT * FROM \"user\" WHERE id = $1") .bind(id) @@ -96,14 +144,31 @@ impl User { Ok(row.0) } + /// Window after which a failed-PIN streak is forgotten. Matches the lockout duration, so + /// "wait out the cooldown" and "start clean" are the same interval to a guest. + const PIN_ATTEMPT_DECAY_MINUTES: i64 = 15; + + /// Record a wrong PIN and return the CURRENT streak length. + /// + /// The counter decays: before this, it only ever cleared on a successful recovery or after + /// a lockout expired, so ordinary typos accumulated across days and a guest could arrive at + /// an event already most of the way to being locked out by mistakes made the night before. + /// Decay is what makes the raised lock threshold safe rather than merely lenient. pub async fn increment_failed_pin(pool: &PgPool, id: Uuid) -> Result { let row: (i16,) = sqlx::query_as( "UPDATE \"user\" - SET failed_pin_attempts = failed_pin_attempts + 1 + SET failed_pin_attempts = CASE + WHEN last_failed_pin_at IS NULL + OR last_failed_pin_at < NOW() - ($2 || ' minutes')::interval + THEN 1 + ELSE failed_pin_attempts + 1 + END, + last_failed_pin_at = NOW() WHERE id = $1 RETURNING failed_pin_attempts", ) .bind(id) + .bind(Self::PIN_ATTEMPT_DECAY_MINUTES.to_string()) .fetch_one(pool) .await?; Ok(row.0) @@ -124,7 +189,9 @@ impl User { pub async fn reset_pin_attempts(pool: &PgPool, id: Uuid) -> Result<(), sqlx::Error> { sqlx::query( - "UPDATE \"user\" SET failed_pin_attempts = 0, pin_locked_until = NULL WHERE id = $1", + "UPDATE \"user\" + SET failed_pin_attempts = 0, pin_locked_until = NULL, last_failed_pin_at = NULL + WHERE id = $1", ) .bind(id) .execute(pool) diff --git a/backend/src/services/sse_tickets.rs b/backend/src/services/sse_tickets.rs index 535b27c..fd921c9 100644 --- a/backend/src/services/sse_tickets.rs +++ b/backend/src/services/sse_tickets.rs @@ -13,6 +13,16 @@ use rand::Rng; /// stream open. Tickets are consumed on use and expire after `TTL`. const TTL: Duration = Duration::from_secs(30); +/// Ceiling on outstanding tickets across the whole process. +/// +/// Not really about the bytes (~120 each) — about `issue` having had no bound of any kind. +/// Sized well above a real event: ~1000 concurrent clients each holding one live 30 s ticket. +const MAX_TICKETS: usize = 4096; + +/// Live tickets one session may hold. Above 1 because two tabs sharing a token open their +/// EventSources concurrently; 4 absorbs that without letting a reconnect loop accumulate. +const MAX_TICKETS_PER_SESSION: usize = 4; + #[derive(Clone)] pub struct SseTicketStore { inner: Arc>>, @@ -39,9 +49,47 @@ impl SseTicketStore { } /// Mint a new ticket bound to the caller's session (identified by token hash). - pub fn issue(&self, token_hash: String) -> String { + /// + /// `None` when the store is at capacity — the caller should answer 503, not evict. + /// + /// Three bounds, because `issue` had none: no size cap, no per-caller cap, and no rate + /// limit on the endpoint, while `prune` ran only hourly against a 30-second TTL. So any + /// authenticated session could loop the endpoint and grow the map for an hour. + pub fn issue(&self, token_hash: String) -> Option { let ticket = random_ticket(); let mut map = self.inner.lock().unwrap(); + + // Prune on issue rather than only hourly. This alone changes the bound from "tickets + // minted since the last maintenance tick" to "tickets live at once", which is what the + // 30 s TTL was always meant to express. + map.retain(|_, e| e.issued_at.elapsed() <= TTL); + + // Cap the caller's own outstanding tickets, evicting their oldest. NOT one-per-session: + // two tabs sharing a token open their EventSources concurrently, and having tab B + // invalidate tab A's unconsumed ticket looks exactly like a flaky SSE connection. + let mut mine: Vec<(String, Instant)> = map + .iter() + .filter(|(_, e)| e.token_hash == token_hash) + .map(|(k, e)| (k.clone(), e.issued_at)) + .collect(); + if mine.len() >= MAX_TICKETS_PER_SESSION { + mine.sort_by_key(|(_, issued)| *issued); + for (key, _) in mine.iter().take(mine.len() - MAX_TICKETS_PER_SESSION + 1) { + map.remove(key); + } + } + + // At capacity, REFUSE — never evict a stranger's ticket. Evicting would let one + // misbehaving client deny SSE to the whole venue, which is worse than failing the + // request that hit the ceiling. + if map.len() >= MAX_TICKETS { + tracing::warn!( + outstanding = map.len(), + "SSE ticket store at capacity; refusing to mint" + ); + return None; + } + map.insert( ticket.clone(), Entry { @@ -49,7 +97,7 @@ impl SseTicketStore { issued_at: Instant::now(), }, ); - ticket + Some(ticket) } /// Consume a ticket. Returns `Some(token_hash)` if the ticket exists and is @@ -84,10 +132,16 @@ fn random_ticket() -> String { mod tests { use super::*; + /// `issue` now returns `Option`; in every test below the store is far from capacity, so an + /// `expect` here documents that refusing is exceptional rather than routine. + fn issue(store: &SseTicketStore, hash: &str) -> String { + store.issue(hash.into()).expect("store has capacity") + } + #[test] fn issue_then_consume_returns_the_hash_exactly_once() { let store = SseTicketStore::new(); - let ticket = store.issue("hash-1".into()); + let ticket = issue(&store, "hash-1"); assert_eq!(store.consume(&ticket).as_deref(), Some("hash-1")); // Single-use: a replay of the same ticket is rejected. assert_eq!( @@ -106,8 +160,8 @@ mod tests { #[test] fn issued_tickets_are_unique_and_hex() { let store = SseTicketStore::new(); - let a = store.issue("h".into()); - let b = store.issue("h".into()); + let a = issue(&store, "h"); + let b = issue(&store, "h"); assert_ne!(a, b, "each ticket must be unique"); assert_eq!(a.len(), 48, "24 random bytes → 48 hex chars"); assert!(a.chars().all(|c| c.is_ascii_hexdigit())); @@ -116,29 +170,104 @@ mod tests { #[test] fn fresh_ticket_survives_prune() { let store = SseTicketStore::new(); - let ticket = store.issue("h".into()); + let ticket = issue(&store, "h"); store.prune(); // not expired → kept assert_eq!(store.consume(&ticket).as_deref(), Some("h")); } - #[test] - fn expired_ticket_consumes_to_none() { - // Construct an entry that is already past the TTL and confirm consume() rejects it. - let store = SseTicketStore::new(); - let stale = "stale-ticket".to_string(); + /// Build an entry that is already past the TTL. + fn insert_stale(store: &SseTicketStore, key: &str, token_hash: &str) { store.inner.lock().unwrap().insert( - stale.clone(), + key.to_string(), Entry { - token_hash: "h".into(), + token_hash: token_hash.into(), issued_at: Instant::now() .checked_sub(TTL + Duration::from_secs(1)) .expect("host uptime should exceed the ticket TTL"), }, ); + } + + #[test] + fn expired_ticket_consumes_to_none() { + let store = SseTicketStore::new(); + insert_stale(&store, "stale-ticket", "h"); assert_eq!( - store.consume(&stale), + store.consume("stale-ticket"), None, "an expired ticket must not authenticate" ); } + + /// The TTL is 30 s but `prune` only ran hourly, so the map was really bounded by "tickets + /// minted in the last hour" — which is unbounded for a client in a loop. + #[test] + fn issuing_prunes_expired_entries() { + let store = SseTicketStore::new(); + insert_stale(&store, "stale-a", "someone-else"); + insert_stale(&store, "stale-b", "someone-else"); + issue(&store, "h"); + assert_eq!( + store.inner.lock().unwrap().len(), + 1, + "issue must reclaim expired slots, not merely add to them" + ); + } + + /// Two tabs sharing a token is normal, so the per-session cap must be above 1 — but a + /// reconnect loop must not accumulate. The caller's OWN oldest is what gets evicted. + #[test] + fn a_session_is_capped_and_evicts_only_its_own_oldest() { + let store = SseTicketStore::new(); + let stranger = issue(&store, "other-session"); + + let mut mine: Vec = Vec::new(); + for _ in 0..MAX_TICKETS_PER_SESSION + 2 { + mine.push(issue(&store, "mine")); + } + + let live = mine + .iter() + .filter(|t| store.inner.lock().unwrap().contains_key(*t)) + .count(); + assert_eq!(live, MAX_TICKETS_PER_SESSION, "one session, bounded"); + assert!( + store.inner.lock().unwrap().contains_key(&mine[mine.len() - 1]), + "the newest ticket is the one the caller is about to use" + ); + assert_eq!( + store.consume(&stranger).as_deref(), + Some("other-session"), + "another session's ticket must survive — evicting it would let one client deny \ + SSE to the venue" + ); + } + + /// At capacity the store REFUSES rather than evicting a stranger. Refusing fails the one + /// request that hit the ceiling; evicting would break an unrelated client's live stream. + #[test] + fn at_capacity_the_store_refuses_instead_of_evicting() { + let store = SseTicketStore::new(); + { + let mut map = store.inner.lock().unwrap(); + for i in 0..MAX_TICKETS { + map.insert( + format!("filler-{i}"), + Entry { + token_hash: format!("session-{i}"), + issued_at: Instant::now(), + }, + ); + } + } + assert_eq!( + store.issue("newcomer".into()), + None, + "a full store must refuse, so the caller can answer 503" + ); + assert!( + store.inner.lock().unwrap().contains_key("filler-0"), + "no existing ticket may be sacrificed to make room" + ); + } }