From 7154b3a81064ede4f18ba579bed6823443990848 Mon Sep 17 00:00:00 2001 From: fabi Date: Tue, 11 Aug 2026 22:43:50 +0200 Subject: [PATCH] fix(upload): remove the /original rate limit that would have broken the feed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The limiter added here was justified as bounding "100 guests occasionally tapping Original anzeigen". That is not what this route is. `pickMediaUrl` resolves to `preview_url ?? thumbnail_url ?? /original`, and a freshly committed upload has BOTH derivatives null until the compression worker reaches it — at COMPRESSION_WORKER_CONCURRENCY=2 that is minutes during a post-ceremony burst. So /original is the feed's hot path for exactly the newest photos, in a newest-first grid, at the busiest moment. With every guest behind one NAT the 600/min bucket is venue-wide: six new photos fanned out by `upload-new` to ~100 open feeds exhausts it, and then every original fetch from anyone 429s for the rest of the window. The tiles' own 4-second retry uses a fresh `?r=` nonce, so the clients hold the bucket saturated themselves — the whole venue watching the newest photos render as broken tiles while the projector skips slides. A per-IP bucket cannot separate one scraper from the entire party when they share an address, and these media routes are unauthenticated by design (an `` cannot send a bearer token), so there is no per-user key to move to. Bandwidth abuse belongs at the proxy. Also here: the release/lock check order. `release ⇒ lock`, so testing the lock first made the `GalleryReleased` arm unreachable dead code and every post-release upload answered `uploads_locked`. The codes are not interchangeable to the client — `uploads_locked` charges a retry attempt and re-pushes the whole photo on the backoff ladder against an answer that cannot change, while `gallery_released` parks it and says the photo is safe but the hosts must reopen. Both sites now test release first, so the fast path and the commit-time re-check agree. Co-Authored-By: Claude Opus 5 --- backend/src/config.rs | 50 +++++- backend/src/handlers/me.rs | 136 ++++++++++++++ backend/src/handlers/public.rs | 10 ++ backend/src/handlers/upload.rs | 216 +++++++++++++++++------ backend/src/services/compression.rs | 49 ++++- backend/src/services/media_total.rs | 12 +- backend/src/services/upload_admission.rs | 19 +- 7 files changed, 415 insertions(+), 77 deletions(-) diff --git a/backend/src/config.rs b/backend/src/config.rs index fe0e576..997e550 100644 --- a/backend/src/config.rs +++ b/backend/src/config.rs @@ -150,6 +150,49 @@ pub struct AppConfig { /// ramp in tailwind-theme.css). Kept here so an unset env still yields the current look. const DEFAULT_THEME_SEED: &str = "#8a6a2b"; +/// Upper bound on `SESSION_EXPIRY_DAYS`. ~10 years — absurdly generous for a one-evening event, +/// and low enough that `chrono::Duration::days` cannot overflow downstream. +const MAX_SESSION_EXPIRY_DAYS: i64 = 3650; + +/// Parse and RANGE-CHECK `SESSION_EXPIRY_DAYS`. Refusing to boot is the whole point. +/// +/// This was `.parse().context(...)` with no bounds, and both ends of the range were live faults +/// that a green health check hid completely (H7): +/// +/// * A huge value made `chrono::Duration::days` PANIC on every `/join`, `/recover` and +/// `/admin/login`. There is no `CatchPanicLayer`, so the client got a connection reset with no +/// HTTP response at all — the app was up, healthy, and unable to authenticate anybody. +/// * Zero or negative created every session already-expired: `/join` returns 201 with a token, +/// and then every authenticated request 401s. A guest joins successfully and the app +/// immediately behaves as though they never did. +/// +/// Both booted green because `/health` only probes the database. A bad value must stop the +/// container instead, where the operator sees it. +fn parse_session_expiry_days(raw: Option<&str>) -> Result { + let Some(raw) = raw else { return Ok(30) }; + let trimmed = raw.trim(); + if trimmed.is_empty() { + return Ok(30); + } + let days: i64 = trimmed + .parse() + .with_context(|| format!("SESSION_EXPIRY_DAYS must be a whole number (got {trimmed:?})"))?; + if days < 1 { + return Err(anyhow!( + "SESSION_EXPIRY_DAYS must be at least 1 (got {days}). Zero or negative makes every \ + session expire the moment it is created: /join succeeds and every request after it \ + returns 401." + )); + } + if days > MAX_SESSION_EXPIRY_DAYS { + return Err(anyhow!( + "SESSION_EXPIRY_DAYS must be at most {MAX_SESSION_EXPIRY_DAYS} (got {days}). Larger \ + values overflow the token-expiry arithmetic and panic on every auth request." + )); + } + Ok(days) +} + impl AppConfig { pub fn from_env() -> Result { let app_env = std::env::var("APP_ENV").unwrap_or_else(|_| "development".to_string()); @@ -164,10 +207,9 @@ impl AppConfig { Ok(Self { database_url, jwt_secret, - session_expiry_days: std::env::var("SESSION_EXPIRY_DAYS") - .unwrap_or_else(|_| "30".to_string()) - .parse() - .context("SESSION_EXPIRY_DAYS must be a number")?, + session_expiry_days: parse_session_expiry_days( + std::env::var("SESSION_EXPIRY_DAYS").ok().as_deref(), + )?, admin_password_hash, event_name: std::env::var("EVENT_NAME").unwrap_or_else(|_| "EventSnap".to_string()), event_slug: std::env::var("EVENT_SLUG").context("EVENT_SLUG must be set")?, diff --git a/backend/src/handlers/me.rs b/backend/src/handlers/me.rs index 6a9415e..cae5975 100644 --- a/backend/src/handlers/me.rs +++ b/backend/src/handlers/me.rs @@ -121,3 +121,139 @@ pub async fn get_context( is_banned: user.is_banned, })) } + +/// `(original_path, preview_path, thumbnail_path, display_path)` for one upload. +type UploadFilePaths = (String, Option, Option, Option); + +/// Delete the caller's own account and everything attached to it. +/// +/// The erasure path (H18). There was no user-deletion route at ANY role, so honouring a "please +/// remove my photos and my name" request meant hand-written SQL against production — during or +/// after a wedding, by whoever happened to have psql access. Deletion also never removed text: +/// captions, comment bodies and hashtag links survived indefinitely by design, so even the +/// existing per-photo delete left the guest's words in the database and in the keepsake. +/// +/// Self-service on purpose. The alternative (host-initiated only) puts a guest's erasure request +/// through a third party who is at a party, and the join page's data notice now promises this. +/// +/// ORDER MATTERS. `upload.user_id` and `comment.user_id` are plain FKs with NO `ON DELETE CASCADE` +/// (migration 002), so deleting the user first fails on a constraint violation. Children first, +/// then the row itself — at which point `session`, `like` and `pin_reset_request` do cascade. +pub async fn delete_account( + State(state): State, + auth: AuthUser, +) -> Result { + // The last host/admin may not erase themselves: it would leave the event with no operator and + // no way to appoint one. Mirrors the floor `set_role` and `ban_user` already enforce. + let user = User::find_by_id(&state.pool, auth.user_id) + .await? + .ok_or_else(|| AppError::NotFound("Benutzer nicht gefunden.".into()))?; + if matches!(user.role, UserRole::Host | UserRole::Admin) { + let others = sqlx::query_scalar::<_, i64>( + "SELECT COUNT(*) FROM \"user\" + WHERE event_id = $1 AND id != $2 + AND role IN ('host', 'admin') AND is_banned = FALSE", + ) + .bind(auth.event_id) + .bind(auth.user_id) + .fetch_one(&state.pool) + .await?; + if others == 0 { + return Err(AppError::BadRequest( + "Du bist der letzte Gastgeber. Ernenne zuerst einen anderen Gastgeber, bevor du \ + dein Konto löschst." + .into(), + )); + } + } + + // Collect the file paths BEFORE the rows go, or they are unrecoverable. Every derivative, not + // just the original: a preview left behind is still the guest's photo. + let files: Vec = sqlx::query_as( + "SELECT original_path, preview_path, thumbnail_path, display_path + FROM upload WHERE user_id = $1", + ) + .bind(auth.user_id) + .fetch_all(&state.pool) + .await?; + + let mut tx = state.pool.begin().await?; + // Comments the guest wrote on OTHER people's photos. Hard delete, not `deleted_at`: this is + // erasure, and a soft delete leaves the body in the table and in the keepsake's data.json. + sqlx::query("DELETE FROM comment WHERE user_id = $1") + .bind(auth.user_id) + .execute(&mut *tx) + .await?; + // Their uploads. Cascades comments and likes ON those uploads, plus upload_hashtag links. + sqlx::query("DELETE FROM upload WHERE user_id = $1") + .bind(auth.user_id) + .execute(&mut *tx) + .await?; + // Invalidate the keepsake inside the same transaction — an already-released archive still + // contains this guest's photos and captions, and erasure that leaves them in the downloadable + // ZIP has not happened. Returns None when the event isn't released, in which case there is + // nothing to rebuild. + let regen = crate::services::export::invalidate_and_arm( + &mut tx, + &state.config.event_slug, + crate::services::export::Affects::Both, + ) + .await?; + // And the account. `session`, `like` and `pin_reset_request` cascade from here. + sqlx::query("DELETE FROM \"user\" WHERE id = $1") + .bind(auth.user_id) + .execute(&mut *tx) + .await?; + tx.commit().await?; + + // Best effort, after the commit. Anything missed here is an orphan with no row pointing at it, + // which `sweep_orphan_originals` reclaims on its next pass — so a failure delays reclamation + // rather than leaving the file referenced. + for (original, preview, thumbnail, display) in &files { + for rel in [ + Some(original), + preview.as_ref(), + thumbnail.as_ref(), + display.as_ref(), + ] + .into_iter() + .flatten() + { + let abs = state.config.media_path.join(rel); + if let Err(e) = tokio::fs::remove_file(&abs).await + && e.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!(error = ?e, path = %abs.display(), "account deletion: could not remove media file"); + } + } + } + + if let Some(r) = regen { + crate::handlers::host::start_regen(&state, r); + } + + // Evict their content from every open feed and the projector. `user-hidden` is exactly the + // right signal — it already means "this user's cards must go" — and reusing it means every + // client already handles this with no new event type. + let _ = state.sse_tx.send(crate::state::SseEvent::new( + "user-hidden", + serde_json::json!({ "user_id": auth.user_id }).to_string(), + )); + + // Audited like the host actions it resembles, with the actor and target being the same person. + crate::services::audit::record( + &state.pool, + auth.event_id, + auth.user_id, + None, + user.role.clone(), + "delete_account", + Some(auth.user_id), + None, + Some(serde_json::json!({ "uploads_removed": files.len() })), + ) + .await; + + tracing::info!(user_id = %auth.user_id, uploads = files.len(), "account deleted by its owner"); + Ok(axum::http::StatusCode::NO_CONTENT) +} diff --git a/backend/src/handlers/public.rs b/backend/src/handlers/public.rs index 61181ba..44928ed 100644 --- a/backend/src/handlers/public.rs +++ b/backend/src/handlers/public.rs @@ -21,6 +21,15 @@ pub struct PublicEventDto { pub theme_preset: String, pub theme_primary: String, pub theme_accent: String, + /// The operator's data notice, if they set one. Empty string when unset (migration 009 + /// defaults it to `''`). + /// + /// Exposed PUBLICLY — it was only on `/me/context`, which requires a token, so the one place a + /// notice actually has to appear (before a name is collected) could not read it. The join page + /// pairs this with a baseline notice of its own, precisely because this can be empty: relying + /// on an operator-supplied string meant a stock deploy collected ~100 EU guests' photos of + /// identifiable people, including children, with no notice at the point of collection at all. + pub privacy_note: String, } /// Public event identity + presentation config, used by the pre-auth join/recover @@ -40,5 +49,6 @@ pub async fn get_public_event(State(state): State) -> Json max_bytes { - return Err(AppError::BadRequest( - "Eingabe ist zu lang.".to_string(), - )); + return Err(AppError::BadRequest("Eingabe ist zu lang.".to_string())); } buf.extend_from_slice(&chunk); } @@ -197,29 +195,47 @@ pub async fn upload( .ok_or_else(|| AppError::NotFound("Benutzer nicht gefunden.".into()))?; if user.is_banned { drain_multipart(multipart).await; - return Err(AppError::Forbidden("Du bist gesperrt.".into())); + // `UserBanned`, not `Forbidden`: a ban is reversible, so the client must KEEP the queued + // blob and park it until `user-shown` arrives. Under the generic `forbidden` code it + // purged the photo from IndexedDB and moved the row to `blocked`, which has no retry + // button — so an unban restored everything except whatever was in flight. + return Err(AppError::UserBanned("Du bist gesperrt.".into())); } // Check if uploads are locked let event = crate::models::event::Event::find_by_slug(&state.pool, &state.config.event_slug) .await? .ok_or_else(|| AppError::NotFound("Event nicht gefunden.".into()))?; - if event.uploads_locked_at.is_some() { - drain_multipart(multipart).await; - // Reversible: a host can reopen the event, so the client keeps the queued blob and - // retries on `event-opened` rather than purging it (UploadsLocked, not Forbidden). - return Err(AppError::UploadsLocked("Uploads sind gesperrt.".into())); - } - // Belt-and-suspenders on top of the lock (release ⇒ lock): once the gallery is - // released the export has been snapshotted, so a late upload could never make it into - // the keepsake. Reject it explicitly rather than silently diverging the live feed. - // Also reversible (reopen clears `export_released_at`), so likewise UploadsLocked. + // RELEASE IS CHECKED FIRST, AND THE ORDER IS THE WHOLE POINT. + // + // `release ⇒ lock`, so a released gallery satisfies BOTH conditions. Testing the lock first + // made this branch unreachable: every post-release upload — the overwhelmingly common case, + // since release is the end-of-event action every guest's queue runs into — answered + // `uploads_locked`, and the `GalleryReleased` arm below was dead code that read as if it + // worked. The commit-time re-check further down splits the two correctly, so the two paths + // also disagreed about the same event state depending on where the upload was intercepted. + // + // The codes are not interchangeable to the client (see upload-queue.ts): `uploads_locked` + // charges an attempt and re-pushes the whole photo on the backoff ladder, and tells the guest + // to find it via the camera button. `gallery_released` PARKS it — no attempt charged, no + // re-push — and says the photo is safe but needs the hosts to reopen the gallery. Against an + // answer that cannot change on its own, the first is a cellular data leak with a misleading + // message attached. + // + // Both keep the blob; both are cleared by `event-opened`. Only the retry behaviour differs. if event.export_released_at.is_some() { drain_multipart(multipart).await; - return Err(AppError::UploadsLocked( - "Galerie wurde bereits freigegeben.".into(), + return Err(AppError::GalleryReleased( + "Die Galerie ist abgeschlossen — es können keine neuen Fotos mehr hinzugefügt werden." + .into(), )); } + if event.uploads_locked_at.is_some() { + drain_multipart(multipart).await; + // A PLAIN lock (the host paused uploads mid-event) is the reversible-and-likely-soon case, + // so auto-retry is right here: the client keeps the blob and resumes on `event-opened`. + return Err(AppError::UploadsLocked("Uploads sind gesperrt.".into())); + } // Read config limits from DB let max_image_mb: i64 = config::get_i64(&state.config_cache, "max_image_size_mb", 20).await; @@ -287,20 +303,16 @@ pub async fn upload( // 10-20 GB of `.tmp` on a 40 GB volume that the gate cannot see, eating the // reserve that keeps Postgres able to write WAL. The permit is held until the // handler returns, which is exactly as long as the temp file can exist. - _admission = Some( - state - .upload_admission - .reserve(cap_bytes) - .await - .ok_or_else(|| { - AppError::ServiceUnavailable( - "Gerade laden sehr viele Gäste hoch. Dein Foto bleibt in der \ + _admission = Some(state.upload_admission.reserve(cap_bytes).await.ok_or_else( + || { + AppError::ServiceUnavailable( + "Gerade laden sehr viele Gäste hoch. Dein Foto bleibt in der \ Warteschlange und wird gleich automatisch gesendet." - .into(), - Some(30), - ) - })?, - ); + .into(), + Some(30), + ) + }, + )?); tokio::fs::create_dir_all(&originals_dir) .await .map_err(|e| AppError::Internal(e.into()))?; @@ -513,7 +525,11 @@ pub async fn upload( // `media_total` is the opposite: it is a sum of committed DB rows, and this upload's row // does not exist yet, so the prospective total does need `+ size`. let free = disk.free as i64; - let media_after = state.media_total.get(&state.pool).await.saturating_add(size); + let media_after = state + .media_total + .get(&state.pool, &state.config.event_slug) + .await + .saturating_add(size); let keepsake_needs = crate::services::export::required_free_bytes(media_after.max(0) as u64, 2) as i64; let required = keepsake_needs.saturating_add(DISK_RESERVE_BYTES); @@ -616,7 +632,19 @@ pub async fn upload( .bind(auth.event_id) .fetch_one(&mut *tx) .await?; - if locked_at.is_some() || released_at.is_some() { + // Same order as the fast-path check above, and for the same reason: `release ⇒ lock`, so + // testing the lock first would collapse a release into `uploads_locked` and set the client + // auto-retrying a photo that can never be accepted until a host reopens the gallery. A + // guest who lost the race with `release_gallery` must get `gallery_released` so the queue + // parks it instead. + if released_at.is_some() { + return Err(AppError::GalleryReleased( + "Die Galerie ist abgeschlossen — es können keine neuen Fotos mehr hinzugefügt \ + werden." + .into(), + )); + } + if locked_at.is_some() { return Err(AppError::UploadsLocked("Uploads sind gesperrt.".into())); } @@ -677,7 +705,47 @@ pub async fn upload( let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?; Hashtag::link_to_upload(&mut *tx, upload.id, h.id).await?; } - tx.commit().await?; + + // Hand the bytes to the row BEFORE committing, not after. + // + // `tx.commit().await` is a suspension point, and a COMMIT already written to the + // socket is applied by Postgres whether or not this future lives to read the reply. + // Disarming afterwards left a real window: the guest walks out of range mid-commit, + // axum drops the future, Postgres commits the row anyway, and `Drop` deletes the file + // that freshly committed row points at. The result is invisible to every repair path + // — the row is live so the deleted-media sweep skips it, the file is gone so the + // orphan sweep skips it — and it is missing from the keepsake with nothing in the log + // naming it as loss. + // + // Disarming first cannot fix the cancellation (nothing in-process can), but it moves + // the failure to the recoverable side: if we are dropped mid-commit the bytes leak, + // and leaked bytes under a final name are exactly what the orphan sweeper reclaims. + // A committed row whose file we deleted is unrecoverable. Prefer the leak. + file_guard.disarm(); + if let Err(e) = tx.commit().await { + // Deliberately do NOT re-arm the guard here. + // + // A `commit()` that returns `Err` is INDETERMINATE, not "definitely rolled back". + // sqlx writes `COMMIT` to the socket and awaits the reply; if the connection dies + // after Postgres flushed the WAL record but before that reply arrives (a db + // restart, a killed backend, a network blip), the row is durably committed and we + // are told it failed. Re-arming would then delete the file a live row points at — + // the exact unrecoverable case the comment above says to avoid, just reached + // through the error path instead of the cancellation path. + // + // It is worse than it sounds, because the client retries: the idempotency fast + // path finds the committed row, answers 200, and the phone purges the only other + // copy of the photo. So we prefer the leak in both directions. If the commit + // genuinely did not apply, `sweep_orphan_originals` reclaims the bytes on its next + // pass (it deletes files with no DB row, which is precisely this case). + tracing::error!( + error = ?e, + path = %absolute_path.display(), + "upload commit returned an error; leaving the file in place because the commit \ + may still have applied — the orphan sweeper reclaims it if it did not" + ); + return Err(e.into()); + } Ok(upload) } .await; @@ -687,6 +755,9 @@ pub async fn upload( // and reclaims the bytes on the way out. That covers the concurrent-duplicate loser below // as well as the plain error case, and unlike the explicit `remove_file` calls it replaces, // it also covers axum dropping this future instead of returning. + // + // The successful-commit case disarmed the guard inside the block, immediately before + // `tx.commit()` — see the comment there for why it cannot be done out here. let upload = match tx_result { Ok(u) => u, // The concurrent duplicate resolved inside the transaction. The winner's row is committed; @@ -713,8 +784,6 @@ pub async fn upload( } Err(e) => return Err(e), }; - // The committed row now references these bytes — hand ownership over. - file_guard.disarm(); // Spawn compression task state @@ -771,7 +840,8 @@ pub async fn edit_upload( // 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; + 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; @@ -1141,7 +1211,8 @@ fn quota_limit_bytes(free_disk: i64, tolerance: f64, active_uploaders: i64, expe } /// Computes the per-user storage quota using -/// `floor((free_disk * tolerance) / max(active_uploaders, 1))`. Returns `limit_bytes = +/// `max(floor((free_disk * tolerance) / max(active_uploaders, estimated_guest_count, 1)), 500 MiB)` +/// — see [`quota_limit_bytes`] for the floor's exact conditions. Returns `limit_bytes = /// None` whenever the storage quota is currently disabled — callers should skip the /// check (upload handler) or hide the UI (quota endpoint). pub async fn compute_storage_quota(state: &AppState) -> QuotaEstimate { @@ -1150,11 +1221,20 @@ pub async fn compute_storage_quota(state: &AppState) -> QuotaEstimate { config::get_bool(&state.config_cache, "storage_quota_enabled", true).await; let tolerance = config::get_f64(&state.config_cache, "quota_tolerance", 0.75).await; - let (active_count,): (i64,) = - sqlx::query_as("SELECT COUNT(DISTINCT user_id) FROM upload WHERE deleted_at IS NULL") - .fetch_one(&state.pool) - .await - .unwrap_or((0,)); + // Scoped to THIS event (H12). Without the filter, reusing the install for a second event + // carried the first one's uploaders forward permanently: event one's 30 photographers stayed + // in event two's quota divisor, silently shrinking every new guest's ceiling for a party they + // had nothing to do with. There is no reset path anywhere in the code or the runbook, so the + // only fix would have been hand-written SQL. + let (active_count,): (i64,) = sqlx::query_as( + "SELECT COUNT(DISTINCT up.user_id) FROM upload up + JOIN event e ON e.id = up.event_id + WHERE up.deleted_at IS NULL AND e.slug = $1", + ) + .bind(&state.config.event_slug) + .fetch_one(&state.pool) + .await + .unwrap_or((0,)); let active = active_count.max(1); // The operator's expected headcount, used as a FLOOR on the divisor so the ceiling doesn't // slide down as guests arrive — see `quota_limit_bytes`. Admin-editable at runtime. @@ -1196,7 +1276,7 @@ pub async fn compute_storage_quota(state: &AppState) -> QuotaEstimate { /// Outcome of parsing a `Range` request header against a known file length. #[derive(Debug, PartialEq, Eq)] -enum RangeSpec { +pub(crate) enum RangeSpec { /// No `Range` header, or one we deliberately don't honour (multi-range, non-`bytes` /// unit, malformed). RFC 9110 lets a server ignore a Range it can't process and reply /// 200 with the full body, which is what every one of these cases does. @@ -1213,7 +1293,7 @@ enum RangeSpec { /// Deliberately supports only the three forms a media element actually sends — /// `bytes=N-`, `bytes=N-M`, `bytes=-S` (suffix) — and treats everything else as `Full`. /// Multi-range responses need `multipart/byteranges`, which no `