diff --git a/backend/src/config.rs b/backend/src/config.rs index 3cc63de..fe0e576 100644 --- a/backend/src/config.rs +++ b/backend/src/config.rs @@ -17,6 +17,30 @@ fn looks_placeholder(s: &str) -> bool { || lower.contains("placeholder") } +/// A bcrypt hash is exactly 60 characters and opens with `$2$$`. +/// +/// Checking the SHAPE, not just placeholder-ness, is what catches a hash silently mangled in +/// transit. The `$` segments are variable-expansion bait for both shell quoting and Docker +/// Compose's `env_file` parsing, and a mangled hash is not a placeholder — so without this it +/// passes every other guard here, the app boots green, `/health` returns `ok`, and every admin +/// login 401s. +/// +/// That failure is unrecoverable mid-event, which is why it is worth a hard fail at boot: the +/// Admin row is created BY a successful admin login (`auth/handlers.rs`), and only an Admin or +/// Host can promote a Host. No admin login therefore means no host at all — the event cannot be +/// closed, the gallery cannot be released, and nothing can be moderated. +fn looks_bcrypt(s: &str) -> bool { + let b = s.as_bytes(); + s.len() == 60 + && b[0] == b'$' + && b[1] == b'2' + && matches!(b[2], b'a' | b'b' | b'x' | b'y') + && b[3] == b'$' + && b[4].is_ascii_digit() + && b[5].is_ascii_digit() + && b[6] == b'$' +} + /// Enforce secret hygiene. In production every guard is hard-fail: a booting app /// with a publicly-known signing key is worse than one that refuses to start. /// Outside production the dev sentinel is tolerated (warned) so local dev is frictionless. @@ -46,6 +70,18 @@ fn validate_secrets( "ADMIN_PASSWORD_HASH is unset or still the .env.example placeholder — generate one \ (docker run --rm caddy:2-alpine caddy hash-password --plaintext '').", ); + } else if !looks_bcrypt(admin_password_hash) { + problems.push( + "ADMIN_PASSWORD_HASH is not a well-formed bcrypt hash: expected exactly 60 \ + characters starting `$2b$12$…`. First look at the value that actually reached \ + the app — `docker compose exec app printenv ADMIN_PASSWORD_HASH` — and compare \ + it to .env character for character. In .env, SINGLE-QUOTE the hash \ + ('$2b$12$…'): Compose uses single-quoted env_file values literally, so the `$` \ + segments survive. Double them to `$$` ONLY when setting the value under \ + `environment:` in docker-compose.yml — doing that in .env corrupts a hash that \ + would otherwise have worked. Regenerate with: \ + docker run --rm caddy:2-alpine caddy hash-password --plaintext ''", + ); } // The DATABASE_URL carries the Postgres password, so a placeholder here means the stack is // running on `CHANGE_ME_use_a_strong_password` — a credential published in the repo. The @@ -173,7 +209,10 @@ mod tests { use super::*; const REAL_SECRET: &str = "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6e7f8a9b0c1d2"; - const REAL_HASH: &str = "$2y$12$abcdefghijklmnopqrstuv.wxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012"; + // A structurally valid bcrypt hash: exactly 60 chars, `$2y$12$` + 53 of salt/digest. + // The shape matters — `looks_bcrypt` enforces it, so a fixture of the wrong length + // would assert the opposite of what these tests claim. + const REAL_HASH: &str = "$2y$12$abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY01"; const REAL_DB_URL: &str = "postgres://eventsnap:7f3a9c1e5b2d8a4f@db:5432/eventsnap"; #[test] @@ -198,6 +237,31 @@ mod tests { assert!(validate_secrets(true, "tooshort", REAL_HASH, REAL_DB_URL).is_err()); } + /// The failure this guards is silent and unrecoverable mid-event: a hash whose `$` + /// segments were eaten by shell or Compose interpolation is NOT a placeholder, so every + /// other guard passes, the app boots green and `/health` reports ok — and then every + /// admin login 401s, which (because the Admin row is created by a successful login, and + /// only an Admin/Host can promote a Host) means no host exists for the whole event. + #[test] + fn prod_rejects_mangled_admin_hash() { + // What `$2y$12$…` degrades to once `$2y`/`$12` are read as unset variables. + assert!(validate_secrets(true, REAL_SECRET, "abcdefghijklmnop", REAL_DB_URL).is_err()); + // Right prefix, truncated body — still not a usable hash. + assert!(validate_secrets(true, REAL_SECRET, "$2y$12$tooshort", REAL_DB_URL).is_err()); + // Correct length but no bcrypt prefix at all. + assert!(validate_secrets(true, REAL_SECRET, &"x".repeat(60), REAL_DB_URL).is_err()); + // All three shipped bcrypt variants stay acceptable. + let body = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY01"; + for variant in ["2a", "2b", "2y"] { + let hash = format!("${variant}$12${body}"); + assert_eq!(hash.len(), 60); + assert!( + validate_secrets(true, REAL_SECRET, &hash, REAL_DB_URL).is_ok(), + "bcrypt variant ${variant}$ must be accepted" + ); + } + } + #[test] fn prod_rejects_missing_or_placeholder_admin_hash() { assert!(validate_secrets(true, REAL_SECRET, "", REAL_DB_URL).is_err()); diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index 3233268..ab8ec6d 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -338,13 +338,25 @@ pub struct DownloadQuery { pub async fn export_ticket( State(state): State, auth: crate::auth::middleware::AuthUser, -) -> Json { +) -> Result, AppError> { // NOTE: intentionally NOT gated on `is_banned`. A banned user keeps *read* access // by design (USER_JOURNEYS §10.3, FEATURES: "Can still download the export once // released — Spec design choice"). The export is read-only, so it stays available // to them, consistent with the read-only-ban model. + + // The rate limit is enforced HERE rather than on the download itself, and that placement is + // the whole point: the download is an iframe navigation, so its response is invisible to the + // page. Limiting it there meant a guest over the limit tapped "Herunterladen", the ticket + // POST returned 200, the iframe silently received a 429, and absolutely nothing happened — + // forever, with no explanation, on the one screen that is the emotional payoff of the app. + // Minting is a normal `fetch`, so a 429 here reaches the user as a German message. + // + // Moving it does not weaken the limit: tickets are single-use with a 30s TTL and can only be + // obtained from this authenticated endpoint, so one mint is at most one download. + enforce_export_rate(&state, auth.user_id).await?; + let ticket = state.sse_tickets.issue(auth.token_hash); - Json(serde_json::json!({ "ticket": ticket })) + Ok(Json(serde_json::json!({ "ticket": ticket }))) } /// Validate a download ticket (single-use) and confirm its session still exists. @@ -366,8 +378,9 @@ pub async fn download_zip( State(state): State, Query(q): Query, ) -> Result { - let user_id = authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, user_id).await?; + // 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?; let path = resolve_export_file(&state, "zip", "Der ZIP-Export ist noch nicht verfügbar.").await?; @@ -419,8 +432,8 @@ pub async fn download_html( State(state): State, Query(q): Query, ) -> Result { - let user_id = authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, user_id).await?; + // See `download_zip`: the limit is charged at ticket mint, where the client can see it. + authenticate_download_ticket(&state, &q.ticket).await?; let path = resolve_export_file(&state, "html", "Der HTML-Export ist noch nicht verfügbar.").await?; @@ -536,8 +549,13 @@ async fn enforce_export_rate(state: &AppState, user_id: Uuid) -> Result<(), AppE limit, Duration::from_secs(86400), ) { + // Names the real window. The generic "warte kurz" wording this used to share with the + // per-minute limiters is actively wrong here — the bucket is a DAY, so a guest told to + // wait a moment would keep tapping a button that cannot work again until tomorrow. return Err(AppError::TooManyRequests( - "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), + "Du hast das Tageslimit für Downloads erreicht. Versuch es später noch einmal — \ + deine Galerie bleibt gespeichert." + .into(), Some(retry_after_secs), )); } diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index ca0df1f..757cbd4 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -40,8 +40,18 @@ impl RateLimiter { timestamps.push(now); Ok(()) } else { - // The oldest timestamp expires at oldest + window; compute remaining seconds - let oldest = timestamps[0]; + // The oldest timestamp expires at oldest + window; compute remaining seconds. + // + // `first()`, not `[0]`: with `max == 0` the length check above is false even on an + // empty vec, so indexing would panic — WHILE HOLDING THIS MUTEX. That poisons it + // process-wide, so every subsequent `.lock().unwrap()` panics too: upload, feed, + // join, recover, social, export and the hourly maintenance task all die, and only + // a container restart brings them back. `max == 0` is not reachable through the + // admin API (every numeric spec has min = 1) but a direct DB edit would do it, and + // the blast radius does not justify the sharper syntax. + let Some(&oldest) = timestamps.first() else { + return Ok(()); + }; let elapsed = now.duration_since(oldest); let remaining = window.saturating_sub(elapsed); Err(remaining.as_secs().max(1))