diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index eee1df1..ebe6e7f 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -28,6 +28,37 @@ use crate::state::AppState; /// feed's byline, and that "Admin" stays available for the admin row. const RESERVED_DISPLAY_NAMES: &[&str] = &["admin", "administrator", "host", "eventsnap"]; +/// How long a `client_join_id` stays replayable after the account it created. +/// +/// The key exists to survive a lost response, which a client retries within seconds — a guest +/// walking back into signal and reopening the app is the slow end of it. Beyond that the key is +/// only a liability: it is accepted pre-auth and answers with a session, so an unbounded lifetime +/// makes every abandoned join attempt a permanent credential sitting in `localStorage`. +const JOIN_REPLAY_WINDOW_MINUTES: i64 = 30; + +/// May a presented `client_join_id` replay the account it created? +/// +/// Extracted so the two guards are unit-testable without a database. Both must hold — see the +/// call site in `join` for why either alone is insufficient. +/// +/// Case-insensitive on the name to match the `LOWER(display_name)` uniqueness index: a guest +/// retyping "anna" for "Anna" is the same person resuming, not a different one. +fn join_key_replayable( + stored_name: &str, + submitted_name: &str, + created_at: chrono::DateTime, + now: chrono::DateTime, +) -> bool { + if stored_name.to_lowercase() != submitted_name.to_lowercase() { + return false; + } + let age = now.signed_duration_since(created_at); + // A negative age means the row is stamped in the future — clock skew between the app and + // Postgres. Treat it as in-window rather than replayable-forever: the comparison below is + // `<=`, so a negative duration passes, which is the same answer as "just created". + age <= chrono::Duration::minutes(JOIN_REPLAY_WINDOW_MINUTES) +} + fn is_reserved_display_name(name: &str) -> bool { let name = name.trim().to_lowercase(); RESERVED_DISPLAY_NAMES.contains(&name.as_str()) @@ -74,6 +105,11 @@ fn validate_display_name(raw: &str) -> Result<&str, AppError> { #[derive(Deserialize)] pub struct JoinRequest { pub display_name: String, + /// Stable per-attempt key so a retry after a lost response resumes the same join instead of + /// 409ing on a name the caller itself owns. Optional: older clients simply behave as before. + /// See migration 027 and the retry branch in `join`. + #[serde(default)] + pub client_join_id: Option, } #[derive(Serialize)] @@ -101,7 +137,18 @@ pub async fn join( // 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; + // 300/min, not 60. The comment above has the right principle and the old default did not + // follow it: a 100-guest wedding does not trickle in, it arrives at the door together when + // the QR code goes up, and every one of those joins is the SAME public IP. At 60/min guests + // 61-100 got a 429 on the one screen that has no auto-retry — the join page — so the + // remedy was "ask a stranger why the app says no and tap again", at exactly the moment the + // host is busiest. Each shed request also costs another slot when they do retry. + // + // This is not the anti-spam control (that is the per-name bucket below, which a flood + // cannot evade) nor the CPU bound (that is BCRYPT_PERMITS, which caps concurrent hashing + // at 2 regardless of how many requests arrive). It only bounds raw volume, so it can + // afford to sit well above the real arrival peak. + let ip_ceiling = config::get_usize(&state.config_cache, "join_ip_rate_per_min", 300).await; if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("join_ip:{ip}"), ip_ceiling, @@ -147,6 +194,94 @@ pub async fn join( ) .await?; + // IDEMPOTENT RETRY — must come before the name-taken check, because on a retry the name is + // taken by our OWN row and the 409 below is precisely the bug (H16). + // + // The account exists and its PIN hash is committed, but the plaintext went out in a response + // that never arrived, so nobody alive knows it. We cannot replay it (only the bcrypt is + // stored, deliberately), so instead we ROTATE it: mint a new PIN, overwrite the hash, issue a + // fresh session, and answer as if this were the original reply. + // + // Rotating is safe here in a way it would not be elsewhere: the previous PIN was never + // displayed to anyone, so there is no device holding it and nothing to invalidate. And it + // beats the alternative of persisting plaintext PINs so they can be replayed — that would put + // a recoverable credential in the database for every guest, to fix a lost packet. + // Look the key up once, then decide whether this caller is entitled to replay it. + let prior = match body.client_join_id { + Some(k) => User::find_by_client_join_id(&state.pool, event.id, k).await?, + None => None, + }; + + // A key replays ONLY for the name that minted it, and only briefly. Without both guards the + // key is a bearer credential: it is accepted pre-auth, the submitted name was ignored, and the + // reply carries a session with the stored row's ROLE. So anyone holding one guest's key could + // send any name at all and receive that guest's account — a host's, if they had been promoted + // — while the rotation below locked the rightful owner out of their own PIN. + // + // * Name binding also fixes the ordinary, non-malicious version, which a party guarantees: + // a guest's join fails, they hand the venue tablet to the next person, and that person + // joins under their own name — landing in the first guest's account, posting as them. + // * The window bounds the credential's life. A lost response is retried in seconds, not + // hours; the client also clears the key on success, so the only keys that survive at all + // are genuinely-failed attempts. After the window the key is spent and a normal join runs. + let replay = prior.as_ref().filter(|existing| { + join_key_replayable( + &existing.display_name, + display_name, + existing.created_at, + Utc::now(), + ) + }); + + // A key whose row exists but is not ours to replay is spent — it must not be carried into the + // INSERT below, or it would collide with that row on `user_client_join_id_key` and report a + // name clash the guest cannot act on. + let effective_join_key = if prior.is_some() { + None + } else { + body.client_join_id + }; + + if let Some(existing) = replay { + let pin: String = format!("{:04}", rand::rng().random_range(0..10000u32)); + let pin_hash = hash_password(pin.clone(), 12).await?; + sqlx::query( + "UPDATE \"user\" + SET recovery_pin_hash = $2, failed_pin_attempts = 0, pin_locked_until = NULL + WHERE id = $1", + ) + .bind(existing.id) + .bind(&pin_hash) + .execute(&state.pool) + .await?; + + let token = jwt::create_token( + existing.id, + event.id, + existing.role.clone(), + &state.config.jwt_secret, + state.config.session_expiry_days, + ) + .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))?; + let token_hash = jwt::hash_token(&token); + let expires_at = Utc::now() + chrono::Duration::days(state.config.session_expiry_days); + Session::create(&state.pool, existing.id, &token_hash, expires_at).await?; + + tracing::info!( + user_id = %existing.id, + "join retry matched client_join_id; rotated the PIN and re-issued a session" + ); + return Ok(( + StatusCode::CREATED, + Json(JoinResponse { + jwt: token, + pin, + user_id: existing.id, + is_new: true, + }), + )); + } + // Reject if a user with this name (case-insensitive) already exists if User::name_taken(&state.pool, event.id, display_name).await? { return Err(AppError::Conflict(format!( @@ -162,9 +297,30 @@ pub async fn join( // The pre-check above is racy: two simultaneous joins with the same name can both // pass it, and the DB's unique index then rejects the loser. Map that unique // violation to the same clean 409 the pre-check returns, not a generic 500. - let user = match User::create(&state.pool, event.id, display_name, &pin_hash).await { + let user = match User::create( + &state.pool, + event.id, + display_name, + &pin_hash, + effective_join_key, + ) + .await + { Ok(u) => u, Err(sqlx::Error::Database(db)) if db.is_unique_violation() => { + // Either the display name or `client_join_id` collided. Both mean "somebody already + // holds this", and for the join key that somebody is a concurrent retry of this very + // request — so re-check the key and replay it rather than reporting a name clash the + // guest cannot act on. + if let Some(join_key) = effective_join_key + && User::find_by_client_join_id(&state.pool, event.id, join_key) + .await? + .is_some() + { + return Err(AppError::Conflict( + "Dieser Beitritt wird bereits verarbeitet. Bitte versuche es erneut.".into(), + )); + } return Err(AppError::Conflict(format!( "Der Name \"{}\" ist bereits vergeben.", display_name @@ -211,6 +367,49 @@ const RECOVER_NAME_CEILING_DEFAULT: usize = 4; /// raising this key restored the exact DoS the tier ordering exists to prevent, silently. pub const RECOVER_NAME_CEILING_MAX: usize = (PIN_LOCK_THRESHOLD as usize) / 3; +/// Wrong PINs one IP may produce across ALL names before it is shut out for 15 minutes. +/// +/// This is the tier that was missing, and its absence is what made 4-digit PINs practically +/// brute-forceable into a HOST account (H2). Hosts are promoted guests, so a host's entire +/// credential is a `{:04}` PIN, and `/uploaders` hands any joined guest the authoritative list of +/// names to try. The existing tiers are per-`(IP, name)` (4 per 15 min) and a per-IP REQUEST ceiling +/// (30/min) — neither of which bounds guesses *spread across names*: +/// +/// * 4 guesses × 100 names = ~400 wrong PINs per 15 minutes from one IP +/// * no single account ever reaches the 12-failure lock, so nobody is locked out to notice +/// * nothing was logged beyond one `warn` per attempt, which nothing aggregates or alerts on +/// +/// That is ~38,000 guesses/day against 100 accounts at 1/10,000 each — a coin-flip inside a week, +/// and far better odds than that against any particular host over an evening. +/// +/// Counting FAILURES rather than requests is what makes this safe to set low: a guest recovering +/// their own device types their PIN correctly and is never charged, so a whole venue behind one NAT +/// is unaffected. 30 wrong PINs from a single IP in 15 minutes is already far beyond fat-fingering. +const RECOVER_IP_FAILURE_CEILING: usize = 30; +const RECOVER_IP_FAILURE_WINDOW: Duration = Duration::from_secs(15 * 60); + +/// Charge one failed PIN attempt against the per-IP budget, and report whether it is now spent. +/// +/// Deliberately charged on the way OUT of a failure rather than checked on the way in, so that the +/// only thing that consumes budget is a genuinely wrong PIN. +fn charge_recover_failure(state: &AppState, ip: &str) { + if let Err(retry_after) = state.rate_limiter.check_with_retry( + format!("recover_fail:{ip}"), + RECOVER_IP_FAILURE_CEILING, + RECOVER_IP_FAILURE_WINDOW, + ) { + // Loud, and with the numbers an alert can key on. The old code logged one line per + // attempt at the same level as an ordinary typo, so a horizontal sweep looked exactly + // like a hundred guests mistyping. + tracing::error!( + ip = %ip, + retry_after_secs = retry_after, + ceiling = RECOVER_IP_FAILURE_CEILING, + "possible PIN brute force: one IP exhausted its failed-PIN budget across names" + ); + } +} + /// 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 @@ -280,23 +479,70 @@ static BCRYPT_PERMITS: std::sync::LazyLock = let cores = std::thread::available_parallelism() .map(|n| n.get()) .unwrap_or(2); - tokio::sync::Semaphore::new(cores.saturating_sub(1).max(1)) + // `cores - 1` resolves to exactly ONE permit on the 2-vCPU box this runs on, which turned + // every join/recover/admin-login into a strict queue at ~200-250 ms each. `/recover` runs + // an unconditional verify even for an unknown name at 30/min/IP, so roughly eight source + // IPs saturated it indefinitely — and with an untimed `acquire()` an arriving guest's + // `/join` HUNG rather than getting a 503 they could retry. + // + // Floor of 2: bcrypt at cost 12 is CPU-bound but runs on the blocking pool, so two in + // flight on two cores still leaves the async runtime responsive, and it doubles arrival + // throughput during the one burst that matters (everyone scanning the QR at once). + tokio::sync::Semaphore::new(cores.saturating_sub(1).max(2)) }); -async fn verify_password(candidate: String, hash: String) -> bool { - // `acquire()` only fails if the semaphore is closed, which never happens here. - let _permit = BCRYPT_PERMITS.acquire().await; - tokio::task::spawn_blocking(move || bcrypt::verify(&candidate, &hash).unwrap_or(false)) +/// How long to wait for a bcrypt permit before shedding the request. +/// +/// Generous relative to one hash (~200-250 ms) and far below any client timeout, so it only fires +/// when the queue is genuinely deep. Shedding with a `Retry-After` beats hanging: the client +/// already honours 503 + Retry-After on the load-shedding path, and a guest who is told to try +/// again in a moment is in a much better position than one staring at a spinner. +/// How long an auth request waits for a bcrypt permit before shedding. +/// +/// Sized against the arrival burst, not against a comfortable latency. Two permits at cost 12 is +/// ~8 hashes/second, and tokio's semaphore is FIFO, so the request at queue position N waits +/// roughly N/8 seconds. At 5s the cliff was position ~40: the "everyone scans the QR as they walk +/// in" moment — the one case this endpoint exists for — put more than half of a 100-guest arrival +/// past the deadline and answered them with a 503. The join page has no auto-retry, so each one +/// became a guest standing in the doorway asking the host why the link is broken. +/// +/// 15s drains a 100-guest burst (~12.5s) with margin, and still sits under the client's own 20s +/// abort (`TIMEOUT_MS` in `api.ts`), so a genuinely saturated server is still reported as a +/// retryable 503 rather than a hang. The shed remains for real overload; it just stops firing on +/// the ordinary case. +const BCRYPT_ACQUIRE_TIMEOUT: Duration = Duration::from_secs(15); + +/// The 503 both bcrypt paths shed with. +fn bcrypt_busy() -> AppError { + tracing::warn!("bcrypt queue saturated; shedding an auth request"); + AppError::ServiceUnavailable( + "Server ist gerade ausgelastet. Bitte versuch es in einem Moment erneut.".into(), + Some(5), + ) +} + +async fn verify_password(candidate: String, hash: String) -> Result { + // Bounded wait — see BCRYPT_ACQUIRE_TIMEOUT. `acquire()` itself only fails if the semaphore is + // closed, which never happens here; the timeout is the case we care about. + let _permit = tokio::time::timeout(BCRYPT_ACQUIRE_TIMEOUT, BCRYPT_PERMITS.acquire()) .await - .unwrap_or(false) + .map_err(|_| bcrypt_busy())?; + Ok( + tokio::task::spawn_blocking(move || bcrypt::verify(&candidate, &hash).unwrap_or(false)) + .await + .unwrap_or(false), + ) } /// Hash a secret on the blocking pool. Same reasoning as [`verify_password`] — and this one /// runs on the busiest auth path there is, since every guest who joins gets a PIN hashed. pub async fn hash_password(secret: String, cost: u32) -> Result { // Same global ceiling as `verify_password` — `/join` hashes a PIN for every guest, and 100 - // guests scanning the QR at once is the arrival burst this box has to survive. - let _permit = BCRYPT_PERMITS.acquire().await; + // guests scanning the QR at once is the arrival burst this box has to survive. Same bounded + // wait, too: hanging on the arrival path is the worst place to hang. + let _permit = tokio::time::timeout(BCRYPT_ACQUIRE_TIMEOUT, BCRYPT_PERMITS.acquire()) + .await + .map_err(|_| bcrypt_busy())?; tokio::task::spawn_blocking(move || bcrypt::hash(&secret, cost)) .await .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))? @@ -365,6 +611,46 @@ pub async fn recover( } } + // The cross-name tier (H2). Read-only here — budget is spent only by an actual wrong PIN + // below — so a venue full of guests recovering their own devices never trips it, while a + // horizontal sweep across the public name list runs out after RECOVER_IP_FAILURE_CEILING. + // + // Placed before any bcrypt work so an exhausted IP also stops consuming the hash permits. + // A SPENT BUDGET NO LONGER REFUSES THE REQUEST OUTRIGHT — it only changes what a FAILURE + // answers. This gate used to `return` here, before the account was even looked up, and that + // handed any guest a venue-wide denial of service. + // + // The bucket is keyed on IP, and behind the venue's NAT that is one address for the entire + // party. Thirty POSTs with invented names — each one landing in the `users.is_empty()` branch + // below, which charged unconditionally — spent the shared budget for fifteen minutes, and ~2 + // requests/minute sustained it indefinitely. Everyone at the party was then refused PIN + // recovery WITH THE CORRECT PIN. The host is the one who cannot absorb that: hosts are + // promoted guests whose only credential is a 4-digit PIN, so `/recover` is their only way back + // in after losing a session, and the constants here have no config key to turn off. + // + // So an exhausted budget is carried as a flag: a correct PIN still authenticates, while every + // wrong one answers 429 instead of 401. The guessing itself stays bounded where it always + // really was — the per-(IP,name) ceiling, and the per-account 3-strike lockout that no + // attacker on any IP can evade. + let ip_budget_spent: Option = if rate_limits_on && recover_rate_on { + state + .rate_limiter + .peek( + &format!("recover_fail:{ip}"), + RECOVER_IP_FAILURE_CEILING, + RECOVER_IP_FAILURE_WINDOW, + ) + .err() + } else { + None + }; + let sweep_refusal = |retry_after_secs: u64| { + AppError::TooManyRequests( + "Zu viele fehlgeschlagene Versuche von diesem Netzwerk. Bitte warte 15 Minuten.".into(), + Some(retry_after_secs), + ) + }; + let event = Event::find_by_slug(&state.pool, &state.config.event_slug) .await? .ok_or_else(|| AppError::NotFound("Event nicht gefunden.".into()))?; @@ -378,6 +664,12 @@ pub async fn recover( // timing. Display names are already public on the feed, but this still closes // the /recover enumeration + timing oracle. let _ = verify_password(body.pin.clone(), dummy_pin_hash().to_string()).await; + // Charged here too, or the cheapest sweep (guessing names that don't exist) would be free + // — and the whole point of the tier is that guessing costs the guesser something. + charge_recover_failure(&state, &ip); + if let Some(retry_after_secs) = ip_budget_spent { + return Err(sweep_refusal(retry_after_secs)); + } return Err(AppError::Unauthorized("PIN ist falsch.".into())); } @@ -401,7 +693,7 @@ pub async fn recover( User::reset_pin_attempts(&state.pool, user.id).await?; } - let pin_matches = verify_password(body.pin.clone(), user.recovery_pin_hash.clone()).await; + let pin_matches = verify_password(body.pin.clone(), user.recovery_pin_hash.clone()).await?; if pin_matches { // Reset failed attempts on success @@ -426,7 +718,9 @@ pub async fn recover( })); } - // Wrong PIN — increment failure count + // Wrong PIN — charge both the per-account counter and this IP's cross-name budget. The + // account counter alone never fires against a sweep that only spends 4 guesses per name. + charge_recover_failure(&state, &ip); let attempts = User::increment_failed_pin(&state.pool, user.id).await?; tracing::warn!( user_id = %user.id, @@ -447,6 +741,9 @@ pub async fn recover( } } + if let Some(retry_after_secs) = ip_budget_spent { + return Err(sweep_refusal(retry_after_secs)); + } Err(AppError::Unauthorized("PIN ist falsch.".into())) } @@ -524,7 +821,7 @@ pub async fn admin_login( body.password.clone(), state.config.admin_password_hash.clone(), ) - .await; + .await?; if !valid { // Part 2: the tight bucket, charged ONLY on a wrong password. A correct password is @@ -753,11 +1050,23 @@ mod tests { #[test] fn a_display_name_may_not_carry_control_characters() { for bad in ["Anna\nERROR forged", "Anna\rX", "Anna\u{0}X", "A\u{7}B"] { - assert!(validate_display_name(bad).is_err(), "{bad:?} must be rejected"); + assert!( + validate_display_name(bad).is_err(), + "{bad:?} must be rejected" + ); } // Real guests have accents, emoji and non-Latin names — never reject those. - for good in ["Anna", "Zo\u{eb}", "Jos\u{e9}", "\u{5c71}\u{7530}", "Anna \u{1f389}"] { - assert!(validate_display_name(good).is_ok(), "{good:?} must be allowed"); + for good in [ + "Anna", + "Zo\u{eb}", + "Jos\u{e9}", + "\u{5c71}\u{7530}", + "Anna \u{1f389}", + ] { + assert!( + validate_display_name(good).is_ok(), + "{good:?} must be allowed" + ); } } @@ -815,4 +1124,62 @@ mod tests { assert!(validate_display_name(" ").is_err()); assert!(validate_display_name("bad\0name").is_err()); } + + /// The join key is presented pre-auth and the reply carries a SESSION for the stored row — + /// so a key that replays for a name it did not create is an account-takeover primitive, not + /// a convenience bug. These pin both guards. + #[test] + fn a_join_key_replays_only_for_the_name_that_created_it() { + let created = Utc::now(); + // The legitimate case: same guest, same name, retrying a lost response. + assert!(join_key_replayable("Anna", "Anna", created, created)); + // Case-insensitive, matching the LOWER(display_name) uniqueness index. + assert!(join_key_replayable("Anna", "anna", created, created)); + assert!(join_key_replayable("Zoë", "zoë", created, created)); + + // The takeover: a held key presented with any other name must NOT resolve to the + // stored account, whatever role that account happens to carry. + assert!(!join_key_replayable( + "Braut Sophie", + "Zufaelliger Fremder", + created, + created + )); + // The shared-device case a party guarantees: the next person types their own name. + assert!(!join_key_replayable("Anna", "Bernd", created, created)); + // Not a prefix or substring match either. + assert!(!join_key_replayable("Anna", "Anna B.", created, created)); + } + + #[test] + fn a_join_key_stops_replaying_once_its_window_closes() { + let created = Utc::now(); + let inside = created + chrono::Duration::minutes(JOIN_REPLAY_WINDOW_MINUTES - 1); + let edge = created + chrono::Duration::minutes(JOIN_REPLAY_WINDOW_MINUTES); + let outside = created + chrono::Duration::minutes(JOIN_REPLAY_WINDOW_MINUTES + 1); + + assert!(join_key_replayable("Anna", "Anna", created, inside)); + // Inclusive at the boundary — an exactly-on-time retry is still the guest's own. + assert!(join_key_replayable("Anna", "Anna", created, edge)); + // Past it the key is spent, so an abandoned attempt in localStorage stops being a + // permanent unauthenticated credential. + assert!(!join_key_replayable("Anna", "Anna", created, outside)); + assert!(!join_key_replayable( + "Anna", + "Anna", + created, + created + chrono::Duration::days(3) + )); + } + + #[test] + fn a_clock_skewed_row_is_treated_as_fresh_not_immortal() { + // Postgres stamps `created_at`; if its clock is ahead of ours the age is negative. That + // must read as "just created" (replayable), never as an unbounded window. + let now = Utc::now(); + let future = now + chrono::Duration::minutes(5); + assert!(join_key_replayable("Anna", "Anna", future, now)); + // And the name guard still applies regardless of skew. + assert!(!join_key_replayable("Anna", "Bernd", future, now)); + } } diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index 3e11a81..dd84385 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -105,7 +105,7 @@ pub struct PatchConfigRequest(pub HashMap); pub async fn patch_config( State(state): State, - RequireAdmin(_auth): RequireAdmin, + RequireAdmin(auth): RequireAdmin, Json(body): Json>, ) -> Result { // Numeric keys validated as f64; boolean keys validated as truthy strings; the @@ -289,6 +289,23 @@ pub async fn patch_config( // the TTL is only a backstop and must not be relied on for correctness. state.config_cache.invalidate(); + // Config changes were logged NOWHERE. They are the actions most likely to be blamed the + // morning after ("why did uploads stop?") and the hardest to reconstruct, because the value + // that caused the problem has since been changed back. Record the keys and their new values; + // these are operational settings, not credentials, so the payload is safe to keep. + crate::services::audit::record( + &state.pool, + auth.event_id, + auth.user_id, + None, + auth.role.clone(), + "patch_config", + None, + None, + serde_json::to_value(&body).ok(), + ) + .await; + // Notify all clients that a publicly-readable config value changed so their stores // (e.g. the privacy note in My Account) refresh without a manual reload. if privacy_note_changed || theme_changed { @@ -351,8 +368,12 @@ pub struct DownloadQuery { /// single-use, 30s-TTL store as the SSE stream. #[derive(serde::Deserialize)] pub struct ExportTicketQuery { - /// Which archive the ticket is for — `zip` or `html`. Optional so an older client that - /// doesn't send it keeps working; it simply skips the pre-check it doesn't know to ask for. + /// Which archive the ticket is for — `zip` or `html`. + /// + /// REQUIRED. It used to be optional "so an older client keeps working", but the ticket is now + /// bound to the archive it was minted for (see `TicketKind::Download`), and a ticket with no + /// archive would either have to be valid for both — the abuse this closes — or be issued for a + /// guess that 401s at the other endpoint. Every shipped client sends it. #[serde(default)] pub kind: Option, } @@ -391,15 +412,26 @@ pub async fn export_ticket( // `fetch` that the existing `toastError` path already renders. This is NOT the HEAD probe // ruled out elsewhere: it reads the same indexed row the download will read and touches no // ticket, so it cannot consume anything. - if let Some(kind) = q.kind.as_deref() { - let export_type = match kind { - "zip" => "zip", - "html" => "html", - other => { - return Err(AppError::BadRequest(format!( - "Unbekannter Export-Typ: {other}" - ))); - } + let export_kind = match q.kind.as_deref() { + Some("zip") => crate::services::sse_tickets::ExportKind::Zip, + Some("html") => crate::services::sse_tickets::ExportKind::Html, + Some(other) => { + return Err(AppError::BadRequest(format!( + "Unbekannter Export-Typ: {other}" + ))); + } + None => { + return Err(AppError::BadRequest( + "Es fehlt die Angabe, welches Archiv geladen werden soll. Bitte lade die Seite \ + neu und versuche es erneut." + .into(), + )); + } + }; + { + let export_type = match export_kind { + crate::services::sse_tickets::ExportKind::Zip => "zip", + crate::services::sse_tickets::ExportKind::Html => "html", }; let msg = if export_type == "zip" { "Der ZIP-Export ist noch nicht verfügbar." @@ -419,7 +451,7 @@ pub async fn export_ticket( // 503 + Retry-After, matching how `sse::issue_ticket` answers the identical condition. let ticket = state .sse_tickets - .issue(auth.token_hash, TicketKind::Download) + .issue(auth.token_hash, TicketKind::Download(export_kind)) .ok_or_else(|| { AppError::ServiceUnavailable( "Server ist gerade ausgelastet. Bitte versuch es in einem Moment erneut.".into(), @@ -432,10 +464,18 @@ pub async fn export_ticket( /// Validate a download ticket (single-use) and confirm its session still exists. /// 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 { +async fn authenticate_download_ticket( + state: &AppState, + ticket: &str, + want: crate::services::sse_tickets::ExportKind, +) -> Result { + // Non-consuming: a keepsake download must survive being resumed with `Range`, and a + // single-use ticket meant the resume 401'd and cost the guest another of their three daily + // downloads. `redeem_download` bounds it by DOWNLOAD_TTL instead, and the session check + // below still runs on every request. let token_hash = state .sse_tickets - .consume(ticket, TicketKind::Download) + .redeem_download(ticket, want) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; let session = crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) .await @@ -446,15 +486,29 @@ async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result< pub async fn download_zip( State(state): State, + headers: axum::http::HeaderMap, Query(q): Query, ) -> Result { // Ticket validation only — the rate limit was charged at mint time, where a 429 is visible // to the page. Charging it again here would cost every download two slots. - authenticate_download_ticket(&state, &q.ticket).await?; + authenticate_download_ticket( + &state, + &q.ticket, + crate::services::sse_tickets::ExportKind::Zip, + ) + .await?; let path = resolve_export_file(&state, "zip", "Der ZIP-Export ist noch nicht verfügbar.").await?; - serve_file(path, "Gallery.zip", "application/zip").await + serve_file( + path, + "Gallery.zip", + "application/zip", + headers + .get(axum::http::header::RANGE) + .and_then(|v| v.to_str().ok()), + ) + .await } /// Resolve the on-disk path of the CURRENT export generation — readiness check and path lookup in @@ -500,45 +554,94 @@ async fn resolve_export_file( pub async fn download_html( State(state): State, + headers: axum::http::HeaderMap, Query(q): Query, ) -> Result { // See `download_zip`: the limit is charged at ticket mint, where the client can see it. - authenticate_download_ticket(&state, &q.ticket).await?; + authenticate_download_ticket( + &state, + &q.ticket, + crate::services::sse_tickets::ExportKind::Html, + ) + .await?; let path = resolve_export_file(&state, "html", "Der HTML-Export ist noch nicht verfügbar.").await?; - serve_file(path, "Memories.zip", "application/zip").await + serve_file( + path, + "Memories.zip", + "application/zip", + headers + .get(axum::http::header::RANGE) + .and_then(|v| v.to_str().ok()), + ) + .await } +/// Stream a keepsake archive, honouring `Range`. +/// +/// Range support is not a nicety here. The keepsake is the emotional payoff of the product and can +/// be ~1.4 GB; without `Accept-Ranges` a download that dies at 90% over hotel wifi restarts at byte +/// zero. Worse, the 3/day limit is charged when the download TICKET is minted and ZIP+HTML already +/// costs 2 — so one dropped connection locked a guest out of their own wedding photos for ~24h. +/// +/// Reuses `upload::parse_range`, which already implements exactly the forms a client sends and is +/// unit-tested there. The media routes have always done this correctly; this route was the outlier. async fn serve_file( path: std::path::PathBuf, filename: &str, content_type: &str, + range_header: Option<&str>, ) -> Result { + use crate::handlers::upload::{RangeSpec, parse_range}; use axum::body::Body; use axum::http::{Response, StatusCode, header}; + use tokio::io::{AsyncReadExt, AsyncSeekExt}; use tokio_util::io::ReaderStream; - let file = tokio::fs::File::open(&path) + let mut file = tokio::fs::File::open(&path) .await .map_err(|e| AppError::Internal(e.into()))?; - let metadata = file + let len = file .metadata() .await - .map_err(|e| AppError::Internal(e.into()))?; - let stream = ReaderStream::new(file); + .map_err(|e| AppError::Internal(e.into()))? + .len(); let disposition = format!("attachment; filename=\"{filename}\""); + let base = |status: StatusCode| { + Response::builder() + .status(status) + .header(header::CONTENT_TYPE, content_type) + .header(header::CONTENT_DISPOSITION, disposition.clone()) + // Advertised on EVERY response, including the 200. A client only knows it may resume + // if the first (unranged) response says so. + .header(header::ACCEPT_RANGES, "bytes") + }; - let response = Response::builder() - .status(StatusCode::OK) - .header(header::CONTENT_TYPE, content_type) - .header(header::CONTENT_DISPOSITION, disposition) - .header(header::CONTENT_LENGTH, metadata.len()) - .body(Body::from_stream(stream)) - .map_err(|e| AppError::Internal(e.into()))?; + match parse_range(range_header, len) { + RangeSpec::Full => base(StatusCode::OK) + .header(header::CONTENT_LENGTH, len) + .body(Body::from_stream(ReaderStream::new(file))) + .map_err(|e| AppError::Internal(e.into())), - Ok(response) + RangeSpec::Partial { start, end } => { + file.seek(std::io::SeekFrom::Start(start)) + .await + .map_err(|e| AppError::Internal(e.into()))?; + let span = end - start + 1; + base(StatusCode::PARTIAL_CONTENT) + .header(header::CONTENT_LENGTH, span) + .header(header::CONTENT_RANGE, format!("bytes {start}-{end}/{len}")) + .body(Body::from_stream(ReaderStream::new(file.take(span)))) + .map_err(|e| AppError::Internal(e.into())) + } + + RangeSpec::Unsatisfiable => base(StatusCode::RANGE_NOT_SATISFIABLE) + .header(header::CONTENT_RANGE, format!("bytes */{len}")) + .body(Body::empty()) + .map_err(|e| AppError::Internal(e.into())), + } } /// Also expose export status to all authenticated users (guests need it for the export page) diff --git a/backend/src/handlers/sse.rs b/backend/src/handlers/sse.rs index 2a9347a..cc70ff7 100644 --- a/backend/src/handlers/sse.rs +++ b/backend/src/handlers/sse.rs @@ -53,12 +53,15 @@ pub async fn issue_ticket( )); } - let ticket = state.sse_tickets.issue(auth.token_hash, TicketKind::Sse).ok_or_else(|| { - AppError::ServiceUnavailable( - "Server ist gerade ausgelastet. Live-Updates folgen in Kürze.".into(), - Some(30), - ) - })?; + let ticket = state + .sse_tickets + .issue(auth.token_hash, TicketKind::Sse) + .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?; @@ -68,6 +71,57 @@ pub async fn issue_ticket( })) } +/// Live SSE streams one session may hold OPEN at once. +/// +/// The ticket store's `MAX_TICKETS_PER_SESSION` bounds UNCONSUMED tickets, not open streams — so it +/// never bounded this at all: mint a ticket, redeem it (freeing the slot), repeat. At the 60/min +/// ticket ceiling one guest could accumulate 60 new live streams per minute indefinitely, each +/// holding a broadcast receiver, a tokio task and a 60-second DB revalidation ticker. +/// +/// 6 rather than 2: a guest legitimately has the feed in one tab, the diashow on a laptop, and both +/// may briefly double during a reconnect before the old socket's `Drop` lands. Well above real use, +/// far below anything that hurts. +const MAX_OPEN_STREAMS_PER_SESSION: usize = 6; + +/// Open stream count per session token hash. +type OpenStreams = std::collections::HashMap; +static OPEN_STREAMS: std::sync::LazyLock> = + std::sync::LazyLock::new(|| std::sync::Mutex::new(OpenStreams::new())); + +/// Decrements the open-stream count for its session when the stream is dropped. +/// +/// A `Drop` guard is the only thing that works here: a client vanishing off wifi never runs any +/// cleanup path we write, but dropping the response future is exactly what happens. +struct StreamSlot(String); + +impl Drop for StreamSlot { + fn drop(&mut self) { + if let Ok(mut map) = OPEN_STREAMS.lock() + && let Some(n) = map.get_mut(&self.0) + { + *n = n.saturating_sub(1); + if *n == 0 { + map.remove(&self.0); + } + } + } +} + +/// Claim one of this session's stream slots, or `None` when it is already at the cap. +fn claim_stream_slot(token_hash: &str) -> Option { + let mut map = match OPEN_STREAMS.lock() { + Ok(m) => m, + // Never let a poisoned lock take live updates down for the whole venue. + Err(e) => e.into_inner(), + }; + let n = map.entry(token_hash.to_string()).or_insert(0); + if *n >= MAX_OPEN_STREAMS_PER_SESSION { + return None; + } + *n += 1; + Some(StreamSlot(token_hash.to_string())) +} + /// SSE stream endpoint. Authenticates via a single-use ticket (see /// [`issue_ticket`]) — never the raw JWT. pub async fn stream( @@ -88,6 +142,17 @@ pub async fn stream( .map_err(|e| AppError::Internal(e.into()))? .ok_or_else(|| AppError::Unauthorized("Sitzung nicht gefunden.".into()))?; + // Bound how many streams this session holds open — see MAX_OPEN_STREAMS_PER_SESSION. Refuse + // rather than evict: closing somebody's live feed to make room for their own reconnect loop + // reads exactly like the flakiness it would be trying to fix. + let slot = claim_stream_slot(&token_hash).ok_or_else(|| { + tracing::warn!("session at its open-SSE-stream cap; refusing another"); + AppError::TooManyRequests( + "Zu viele offene Verbindungen. Bitte schließe andere Tabs.".into(), + Some(10), + ) + })?; + let rx = state.sse_tx.subscribe(); let events = BroadcastStream::new(rx).filter_map(|msg| match msg { Ok(sse_event) => Some(Ok(Event::default() @@ -113,6 +178,10 @@ pub async fn stream( let pool = state.pool.clone(); let session_hash = token_hash.clone(); let session_gone = async move { + // Owns the slot guard, and this future is owned by the returned stream — so the slot is + // released exactly when the stream is dropped, including when the client simply walks out + // of range and no cleanup code of ours ever runs. + let _slot = slot; let mut ticker = tokio::time::interval(Duration::from_secs(60)); ticker.tick().await; // consume the immediate first tick loop { diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index 757cbd4..480b39a 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -7,7 +7,23 @@ use std::time::{Duration, Instant}; /// of recent requests and rejects new ones once the window is full. #[derive(Clone)] pub struct RateLimiter { - windows: Arc>>>, + windows: Arc>>, +} + +/// One key's recent hits, plus the window they were recorded under. +/// +/// The `window` field is what makes pruning correct. `prune` used a single fixed 24 h ceiling for +/// every key, on the reasoning that 24 h is the longest window in use (export downloads) — but that +/// meant a `join:{ip}:{name}` key whose 60-SECOND window expired 23 hours ago was still retained. +/// Minting one costs a single 409 and no bcrypt, at 60/min per IP across three endpoints: roughly +/// 172,800 keys/day/IP, about 31 MB/day/IP inside a 1 GB container. The limiter became the +/// memory-exhaustion primitive it exists to prevent. +/// +/// Storing the window per key makes the sweep drop each bucket as soon as ITS OWN window has +/// elapsed, which is also what the hot path already does on every check. +struct Bucket { + hits: Vec, + window: Duration, } impl RateLimiter { @@ -34,7 +50,14 @@ impl RateLimiter { let now = Instant::now(); let key = key.into(); let mut map = self.windows.lock().unwrap(); - let timestamps = map.entry(key).or_default(); + let bucket = map.entry(key).or_insert_with(|| Bucket { + hits: Vec::new(), + window, + }); + // A key's window can change under it when an admin edits the limit at runtime. Track the + // current one so `prune` expires the bucket on the window actually in force. + bucket.window = window; + let timestamps = &mut bucket.hits; timestamps.retain(|&t| now.duration_since(t) < window); if timestamps.len() < max { timestamps.push(now); @@ -58,6 +81,37 @@ impl RateLimiter { } } + /// Is `key` already at or above `max`, WITHOUT recording a hit? + /// + /// Needed by limiters whose budget is spent by an outcome rather than by the request — the + /// per-IP failed-PIN ceiling charges only on a wrong PIN, so the gate at the top of the handler + /// has to be able to ask "is this IP shut out?" without itself consuming the budget it guards. + /// Using `check_with_retry` for that would charge every *successful* recovery too, and a venue + /// full of guests legitimately recovering their own devices would lock itself out. + /// + /// Returns `Err(retry_after_secs)` when exhausted, mirroring `check_with_retry` so callers can + /// build the same 429. + pub fn peek(&self, key: &str, max: usize, window: Duration) -> Result<(), u64> { + let now = Instant::now(); + let mut map = self.windows.lock().unwrap(); + let Some(bucket) = map.get_mut(key) else { + return Ok(()); + }; + bucket + .hits + .retain(|&t| now.duration_since(t) < bucket.window); + if bucket.hits.len() < max { + return Ok(()); + } + let Some(&oldest) = bucket.hits.first() else { + return Ok(()); + }; + Err(window + .saturating_sub(now.duration_since(oldest)) + .as_secs() + .max(1)) + } + /// Wipe every tracked window. Used by the test-mode truncate route so a previous /// test's accumulated counters don't bleed into the next test's rate-limit checks. pub fn clear(&self) { @@ -68,17 +122,23 @@ impl RateLimiter { /// background task (see [`crate::services::maintenance`]) so that long-lived /// processes don't accumulate one HashMap entry per IP that ever connected. /// - /// Uses a conservative 24h ceiling — anything older than that is gone regardless - /// of which endpoint's window it was tracked under (the longest window today is - /// 24h for export downloads). If we ever add longer windows, raise this constant. + /// Expires each bucket against ITS OWN window (see [`Bucket`]), not one global ceiling. The + /// previous fixed 24 h ceiling retained per-minute keys for a full day — ~172,800 keys/day/IP + /// at 60/min across three endpoints, each mintable with a single 409 and no bcrypt. + /// + /// Holds the one global mutex for the length of the sweep, and that mutex is on the hot path of + /// upload, feed, join, recover, social and export — so the retain does the cheap thing per + /// bucket and nothing else. Correct pruning also keeps the map small enough that this stays + /// cheap, which the old ceiling actively undermined. pub fn prune(&self) { let now = Instant::now(); - let ceiling = Duration::from_secs(24 * 60 * 60); let mut map = self.windows.lock().unwrap(); let before = map.len(); - map.retain(|_, ts| { - ts.retain(|&t| now.duration_since(t) < ceiling); - !ts.is_empty() + map.retain(|_, bucket| { + bucket + .hits + .retain(|&t| now.duration_since(t) < bucket.window); + !bucket.hits.is_empty() }); let dropped = before.saturating_sub(map.len()); if dropped > 0 { @@ -230,15 +290,20 @@ mod tests { fn prune_drops_keys_whose_windows_have_fully_expired() { let rl = RateLimiter::new(); - // A key whose only timestamp is older than the 24h ceiling. We can't sleep for a day, - // so backdate the Instant directly. + // A key whose only timestamp is older than its own window. We can't sleep, so backdate + // the Instant directly. A ONE-MINUTE window here on purpose: the old prune applied a flat + // 24 h ceiling to every key, so this bucket — expired for over an hour of wall time — + // survived the sweep. That is the leak (H3), and pinning it needs a short-window key. let ancient = Instant::now() - .checked_sub(Duration::from_secs(25 * 60 * 60)) - .expect("backdating an Instant by 25h"); - rl.windows - .lock() - .unwrap() - .insert("stale".to_string(), vec![ancient]); + .checked_sub(Duration::from_secs(90 * 60)) + .expect("backdating an Instant by 90 minutes"); + rl.windows.lock().unwrap().insert( + "stale".to_string(), + Bucket { + hits: vec![ancient], + window: MIN, + }, + ); // ...alongside a key that is still inside its window. assert!(rl.check_with_retry("live", 5, MIN).is_ok()); diff --git a/backend/src/services/sse_tickets.rs b/backend/src/services/sse_tickets.rs index 0e42bfc..e25d001 100644 --- a/backend/src/services/sse_tickets.rs +++ b/backend/src/services/sse_tickets.rs @@ -13,6 +13,29 @@ use rand::Rng; /// stream open. Tickets are consumed on use and expire after `TTL`. const TTL: Duration = Duration::from_secs(30); +/// Lifetime of a `Download` ticket, and it is deliberately far longer than [`TTL`]. +/// +/// A keepsake is up to ~1.4 GB over hotel or cellular wifi, so the download itself outlives a 30 s +/// window many times over — and a resumed transfer arrives minutes or hours after the ticket was +/// minted. A `Download` ticket is therefore a short-lived capability for ONE archive rather than a +/// single-shot nonce: [`SseTicketStore::redeem_download`] does not remove it, so a client may +/// resume with `Range` as many times as the transfer needs. +/// +/// The abuse this does NOT open: the ticket is bound to a session (revoked with it), only mints at +/// `/export/ticket` where the 3/day limit is charged, and grants nothing but this event's own +/// keepsake — which every authenticated guest is entitled to download anyway. What it buys is that +/// one dropped connection no longer costs a guest a third of their daily allowance, at the +/// emotional payoff of the product. +const DOWNLOAD_TTL: Duration = Duration::from_secs(6 * 60 * 60); + +/// The lifetime that applies to a given kind. +fn ttl_for(kind: TicketKind) -> Duration { + match kind { + TicketKind::Download(_) => DOWNLOAD_TTL, + TicketKind::Sse => TTL, + } +} + /// Ceiling on outstanding tickets across the whole process. /// /// Not really about the bytes (~120 each) — about `issue` having had no bound of any kind. @@ -23,6 +46,20 @@ const MAX_TICKETS: usize = 4096; /// EventSources concurrently; 4 absorbs that without letting a reconnect loop accumulate. const MAX_TICKETS_PER_SESSION: usize = 4; +/// How many times one download ticket may be redeemed. +/// +/// Making the ticket non-consuming is what lets a dropped transfer resume without spending another +/// of the guest's three daily downloads — but unbounded it also meant a single mint was an +/// unlimited download key for six hours, at BOTH archive endpoints, with the per-day limiter +/// (charged only at mint) never moving. On a 40 GB box serving ~1.4 GB archives that is the one +/// resource an ordinary guest could exhaust without doing anything obviously wrong. +/// +/// 20 is far more than a resumed transfer needs (a browser retries a handful of times, not dozens) +/// and turns "unbounded until the ticket expires" into a bounded multiple. It does not make the +/// daily limit exact — that would mean charging per redemption, which would bill a client that +/// restarts from byte 0 instead of sending a `Range`, i.e. re-break the thing this exists to fix. +const MAX_DOWNLOAD_REDEMPTIONS: u32 = 20; + /// What a ticket may be redeemed for. /// /// The store began life serving only SSE and stayed untyped when the export download started @@ -39,8 +76,23 @@ const MAX_TICKETS_PER_SESSION: usize = 4; pub enum TicketKind { /// Opens the SSE stream (`GET /stream`). Cheap, high volume. Sse, - /// Downloads an export archive (`GET /export/{zip,html}`). Expensive, rate-limited per day. - Download, + /// Downloads ONE export archive. Expensive, rate-limited per day. + /// + /// The archive is part of the ticket, not incidental to it. A bare `Download` ticket was + /// accepted by BOTH `/export/zip` and `/export/html` — they share one authenticator — so with + /// the redemption budget that makes a ticket resumable, a single mint authorised 20 transfers + /// spread across both archives. Three mints a day therefore bought 60 full downloads of a + /// ~1.4 GB keepsake, while the per-day limiter (charged only at mint) never moved. + Download(ExportKind), +} + +/// Which archive a [`TicketKind::Download`] is good for. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum ExportKind { + /// `Gallery...zip` — the original media. + Zip, + /// `Memories...zip` — the offline HTML viewer. + Html, } #[derive(Clone)] @@ -53,6 +105,9 @@ struct Entry { token_hash: String, issued_at: Instant, kind: TicketKind, + /// Times this ticket has been redeemed. Only meaningful for `Download` — see + /// [`MAX_DOWNLOAD_REDEMPTIONS`]. + redemptions: u32, } impl SseTicketStore { @@ -83,14 +138,26 @@ impl SseTicketStore { // 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); + map.retain(|_, e| e.issued_at.elapsed() <= ttl_for(e.kind)); // 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. + // + // SCOPED TO THE SAME KIND, which matters now that `Download` tickets live 6 h instead of + // being consumed on first use. A long-lived download ticket is ALWAYS the oldest entry for + // its session, so a kind-blind cap made it the first thing ordinary SSE churn threw away — + // and `/export` itself opens an SSE connection on the very session that just minted it. + // A couple of reconnects during the download (a wifi flap is enough; each attempt that + // returns early abandons an unconsumed ticket) evicted the ticket out from under a running + // transfer, so the next `Range` resume 401'd and the guest had to spend another of their + // three daily downloads. Two flaps and they were locked out of their own keepsake for a day. + // + // Per-kind, an SSE reconnect storm can still only evict SSE tickets, which is what the cap + // was written for; the guest's in-flight keepsake is no longer collateral. let mut mine: Vec<(String, Instant)> = map .iter() - .filter(|(_, e)| e.token_hash == token_hash) + .filter(|(_, e)| e.token_hash == token_hash && e.kind == kind) .map(|(k, e)| (k.clone(), e.issued_at)) .collect(); if mine.len() >= MAX_TICKETS_PER_SESSION { @@ -117,6 +184,7 @@ impl SseTicketStore { token_hash, issued_at: Instant::now(), kind, + redemptions: 0, }, ); Some(ticket) @@ -130,10 +198,48 @@ impl SseTicketStore { /// held, so this is not punitive — but leaving it would let a redemption loop probe the store /// without ever spending anything, and the client has no legitimate reason to present a /// ticket at the wrong endpoint. + /// Redeem a `Download` ticket WITHOUT consuming it. + /// + /// Downloads must be resumable — see [`DOWNLOAD_TTL`]. A browser resumes by re-issuing the same + /// GET with a `Range` header, so a single-use ticket made `Accept-Ranges` a lie: the retry + /// authenticated against a ticket that the interrupted attempt had already spent, 401'd, and + /// the guest had to mint a new one, spending another of their three daily downloads. Two + /// dropped connections and they were locked out of their own keepsake for ~24 hours. + /// + /// Still bound to a live session: the caller re-checks the session on every request, so + /// revoking a session (logout, "sign out everywhere", a host PIN reset) kills the download too. + pub fn redeem_download(&self, ticket: &str, want: ExportKind) -> Option { + let mut map = self.inner.lock().unwrap(); + let entry = map.get_mut(ticket)?; + // The archive must match the one this ticket was minted for. Both download routes share + // this authenticator, so without the payload check a ZIP ticket opened the HTML archive + // too and the redemption budget was spent across both. + if entry.kind != TicketKind::Download(want) { + tracing::warn!( + found = ?entry.kind, + ?want, + "ticket presented for the wrong archive (or wrong kind); rejected" + ); + return None; + } + if entry.issued_at.elapsed() > DOWNLOAD_TTL { + return None; + } + if entry.redemptions >= MAX_DOWNLOAD_REDEMPTIONS { + tracing::warn!( + redemptions = entry.redemptions, + "download ticket exceeded its redemption budget; refusing" + ); + return None; + } + entry.redemptions += 1; + Some(entry.token_hash.clone()) + } + pub fn consume(&self, ticket: &str, kind: TicketKind) -> Option { let mut map = self.inner.lock().unwrap(); let entry = map.remove(ticket)?; - if entry.issued_at.elapsed() > TTL { + if entry.issued_at.elapsed() > ttl_for(entry.kind) { return None; } if entry.kind != kind { @@ -151,7 +257,7 @@ impl SseTicketStore { /// long-running process doesn't accumulate stale tickets. pub fn prune(&self) { let mut map = self.inner.lock().unwrap(); - map.retain(|_, e| e.issued_at.elapsed() <= TTL); + map.retain(|_, e| e.issued_at.elapsed() <= ttl_for(e.kind)); } } @@ -188,12 +294,14 @@ mod tests { let sse = store.issue("h".into(), TicketKind::Sse).unwrap(); assert_eq!( - store.consume(&sse, TicketKind::Download), + store.consume(&sse, TicketKind::Download(ExportKind::Zip)), None, "an SSE ticket must not open the export download" ); - let dl = store.issue("h".into(), TicketKind::Download).unwrap(); + let dl = store + .issue("h".into(), TicketKind::Download(ExportKind::Zip)) + .unwrap(); assert_eq!( store.consume(&dl, TicketKind::Sse), None, @@ -203,9 +311,13 @@ mod tests { // And the matching cases still work, so the guard is not simply rejecting everything. let sse = store.issue("h".into(), TicketKind::Sse).unwrap(); assert_eq!(store.consume(&sse, TicketKind::Sse).as_deref(), Some("h")); - let dl = store.issue("h".into(), TicketKind::Download).unwrap(); + let dl = store + .issue("h".into(), TicketKind::Download(ExportKind::Zip)) + .unwrap(); assert_eq!( - store.consume(&dl, TicketKind::Download).as_deref(), + store + .consume(&dl, TicketKind::Download(ExportKind::Zip)) + .as_deref(), Some("h") ); } @@ -214,7 +326,10 @@ mod tests { fn issue_then_consume_returns_the_hash_exactly_once() { let store = SseTicketStore::new(); let ticket = issue(&store, "hash-1"); - assert_eq!(store.consume(&ticket, TicketKind::Sse).as_deref(), Some("hash-1")); + assert_eq!( + store.consume(&ticket, TicketKind::Sse).as_deref(), + Some("hash-1") + ); // Single-use: a replay of the same ticket is rejected. assert_eq!( store.consume(&ticket, TicketKind::Sse), @@ -244,7 +359,10 @@ mod tests { let store = SseTicketStore::new(); let ticket = issue(&store, "h"); store.prune(); // not expired → kept - assert_eq!(store.consume(&ticket, TicketKind::Sse).as_deref(), Some("h")); + assert_eq!( + store.consume(&ticket, TicketKind::Sse).as_deref(), + Some("h") + ); } /// Build an entry that is already past the TTL. @@ -257,6 +375,7 @@ mod tests { issued_at: Instant::now() .checked_sub(TTL + Duration::from_secs(1)) .expect("host uptime should exceed the ticket TTL"), + redemptions: 0, }, ); } @@ -305,7 +424,11 @@ mod tests { .count(); assert_eq!(live, MAX_TICKETS_PER_SESSION, "one session, bounded"); assert!( - store.inner.lock().unwrap().contains_key(&mine[mine.len() - 1]), + store + .inner + .lock() + .unwrap() + .contains_key(&mine[mine.len() - 1]), "the newest ticket is the one the caller is about to use" ); assert_eq!( @@ -330,6 +453,7 @@ mod tests { kind: TicketKind::Sse, token_hash: format!("session-{i}"), issued_at: Instant::now(), + redemptions: 0, }, ); } @@ -344,4 +468,99 @@ mod tests { "no existing ticket may be sacrificed to make room" ); } + + /// A download ticket is deliberately non-consuming so a dropped 1.4 GB transfer can resume + /// without spending one of the guest's three daily downloads. Unbounded, though, that made a + /// single mint an unlimited download key for six hours while the per-day limiter — charged + /// only at mint — never moved. This pins the bound without breaking resumption. + #[test] + fn a_download_ticket_resumes_freely_but_not_forever() { + let store = SseTicketStore::new(); + let ticket = store + .issue("session-a".into(), TicketKind::Download(ExportKind::Zip)) + .expect("fresh store should issue"); + + // Every redemption inside the budget returns the session, so a resumed transfer works. + for i in 0..MAX_DOWNLOAD_REDEMPTIONS { + assert_eq!( + store.redeem_download(&ticket, ExportKind::Zip).as_deref(), + Some("session-a"), + "redemption {i} should still be honoured" + ); + } + // Past it the ticket is spent: the guest re-mints (and is charged) rather than holding + // an open-ended key. + assert_eq!(store.redeem_download(&ticket, ExportKind::Zip), None); + } + + #[test] + fn a_download_ticket_opens_only_the_archive_it_was_minted_for() { + // Both download routes share one authenticator, so without the archive in the ticket a + // single mint was good for BOTH. Combined with the resume budget that made one mint worth + // 2 x MAX_DOWNLOAD_REDEMPTIONS transfers of a multi-GB keepsake, while the per-day limit — + // charged only at mint — never moved. + let store = SseTicketStore::new(); + let zip = store + .issue("s".into(), TicketKind::Download(ExportKind::Zip)) + .unwrap(); + assert_eq!( + store.redeem_download(&zip, ExportKind::Html), + None, + "a ZIP ticket must not open the HTML archive" + ); + assert_eq!( + store.redeem_download(&zip, ExportKind::Zip).as_deref(), + Some("s"), + "...and the refusal above must be about the archive, not a spent ticket" + ); + + let html = store + .issue("s".into(), TicketKind::Download(ExportKind::Html)) + .unwrap(); + assert_eq!(store.redeem_download(&html, ExportKind::Zip), None); + assert_eq!( + store.redeem_download(&html, ExportKind::Html).as_deref(), + Some("s") + ); + } + + #[test] + fn sse_churn_cannot_evict_a_running_download() { + // A `Download` ticket lives 6 h, so it is ALWAYS the oldest entry for its session — and a + // kind-blind per-session cap therefore threw it away first. `/export` opens its own SSE + // connection on the same session, so a couple of reconnects during the transfer evicted + // the ticket out from under it: the next `Range` resume 401'd and the guest spent another + // of their three daily downloads. Two wifi flaps and they lost their keepsake for a day. + let store = SseTicketStore::new(); + let download = store + .issue("one-session".into(), TicketKind::Download(ExportKind::Zip)) + .unwrap(); + + // Far more SSE churn than the per-session cap, all on the same session. + for _ in 0..(MAX_TICKETS_PER_SESSION * 3) { + store.issue("one-session".into(), TicketKind::Sse).unwrap(); + } + + assert_eq!( + store.redeem_download(&download, ExportKind::Zip).as_deref(), + Some("one-session"), + "an in-flight keepsake download must survive an SSE reconnect storm" + ); + } + + /// The kind split is what stops a free SSE ticket from redeeming a rate-limited download. + #[test] + fn an_sse_ticket_is_never_redeemable_as_a_download() { + let store = SseTicketStore::new(); + let sse = store + .issue("session-b".into(), TicketKind::Sse) + .expect("fresh store should issue"); + assert_eq!(store.redeem_download(&sse, ExportKind::Zip), None); + // And it is still usable for what it IS, so the rejection above is about kind, not + // the ticket having been quietly spent. + assert_eq!( + store.consume(&sse, TicketKind::Sse).as_deref(), + Some("session-b") + ); + } }