diff --git a/Caddyfile b/Caddyfile index a8bebb8..138ea14 100644 --- a/Caddyfile +++ b/Caddyfile @@ -9,17 +9,31 @@ header { Strict-Transport-Security "max-age=31536000; includeSubDomains" X-Content-Type-Options "nosniff" - X-Frame-Options "DENY" Referrer-Policy "strict-origin-when-cross-origin" } + # X-Frame-Options: DENY everywhere EXCEPT the keepsake download endpoints, which + # are navigated in a HIDDEN, SAME-ORIGIN iframe so a 404/429 can't unload the PWA + # (see frontend/src/routes/export/+page.svelte). WebKit enforces XFO *before* + # honouring Content-Disposition, so a blanket DENY makes the download silently do + # nothing on iOS Safari — the app's primary platform. SAMEORIGIN still blocks + # cross-origin framing. + # + # Split into two disjoint matchers rather than an override: Caddy applies the + # FIRST header directive outermost, so it wins on write — a later, more specific + # `header` would be silently ignored. + @framable path /api/v1/export/zip /api/v1/export/html + @not_framable not path /api/v1/export/zip /api/v1/export/html + header @framable X-Frame-Options "SAMEORIGIN" + header @not_framable X-Frame-Options "DENY" + # SvelteKit frontend — static assets with long-lived cache (content-hashed filenames) @hashed_assets path_regexp hashed /_app/immutable/.*\.[a-f0-9]{8,}\.(js|css|woff2)$ header @hashed_assets Cache-Control "public, max-age=31536000, immutable" - # Preview/thumbnail images. These are now served by the app through a - # visibility-checked alias (/api/v1/upload/{id}/{preview,thumbnail}) so moderation - # can revoke access; direct /media/previews|thumbnails is 404-blocked at the app. + # Preview/thumbnail images. These are served by the app through a visibility-checked + # alias (/api/v1/upload/{id}/{preview,thumbnail}) so moderation can revoke access; + # the app serves no /media route at all, so there is no direct path to the bytes. # Privately cacheable for a short window (the app sets the same header; this is the # edge carve-out from the blanket no-store below). Kept short so a moderated image # stops being served to a direct-URL holder promptly. @@ -33,7 +47,14 @@ } header @api Cache-Control "no-store" - # Route API and media requests to the Rust backend + # Route API and media requests to the Rust backend. + # + # The app serves no /media route at all (see the note in backend/src/main.rs) — media + # bytes are reachable only through the visibility-checked /api/v1/upload aliases, so + # /media/* forwards to a plain 404. The proxy line is kept deliberately: it means the + # edge faithfully hands /media to the app, so if a future change ever re-introduces a + # static media route the e2e gating specs see it here exactly as production would, + # instead of being masked by the SvelteKit 404 page. reverse_proxy /api/* app:3000 reverse_proxy /media/* app:3000 diff --git a/PROJECT.md b/PROJECT.md index 7095f83..034a906 100644 --- a/PROJECT.md +++ b/PROJECT.md @@ -1133,16 +1133,19 @@ eventsnap/ ### Backup Strategy -```bash -# Daily (e.g. as a separate Compose service or cron on the VPS) -pg_dump $DATABASE_URL | gzip > /media/backups/db_$(date +%Y-%m-%d).sql.gz +Three artefacts in three places: the database, the `media_data` volume +(originals + derivatives), and the **separate** `exports_data` volume. See +[README.md](README.md#backup) for the exact commands. -# Weekly: rsync /media volume to Hetzner Storage Box -rsync -az /opt/eventsnap/media/ \ - user@u123456.your-storagebox.de:backup/eventsnap/ -``` +Everything runs through `docker compose` / `docker run`, because `DATABASE_URL` +and the `/media` and `/exports` paths only exist inside the compose network — +they are not host paths, and `DATABASE_URL` is never exported into an operator's +shell. -The `/media` volume contains originals, previews, thumbnails, generated exports, and DB backups — a single volume to back up. +Export archives are deliberately outside `MEDIA_PATH` (`EXPORT_PATH=/exports`): a +keepsake contains every photo in the event, and keeping it off the media tree is +what stops it being reachable except through the ticket-gated handler. A backup +of the media volume alone silently loses every generated keepsake. --- diff --git a/README.md b/README.md index f4f1751..73d880c 100644 --- a/README.md +++ b/README.md @@ -162,23 +162,58 @@ See [.env.example](.env.example) for the full list with descriptions and default └────────┘ ``` -- `/api/*` and `/media/*` → Rust backend +- `/api/*` → Rust backend - Everything else → SvelteKit frontend (`adapter-node`) -- Named volumes: `postgres_data`, `media_data`, `caddy_data` +- Named volumes: `postgres_data`, `media_data`, `exports_data`, `caddy_data` + +Media is **not** served as static files. Every image goes through a +visibility-checked alias (`/api/v1/upload/{id}/{preview,display,thumbnail,original}`) +so a host takedown or a ban actually revokes access to the bytes. --- ## Backup -```bash -# Database snapshot -pg_dump $DATABASE_URL | gzip > /media/backups/db_$(date +%Y-%m-%d).sql.gz +There are **three** things to back up, and they live in three different places. +`DATABASE_URL` and the container paths (`/media`, `/exports`) are meaningful only +*inside* the compose network — they are not host paths, and `DATABASE_URL` is +never exported into an operator's shell — so every command below runs through +`docker compose` from the repo directory. -# Weekly offsite sync (Hetzner Storage Box or similar) -rsync -az /opt/eventsnap/media/ user@storagebox.example.com:backup/eventsnap/ +```bash +# 1. Database snapshot. Runs pg_dump inside the db container (the app image has no +# postgres client), reading credentials from the compose environment. +mkdir -p ./backups +docker compose exec -T db \ + sh -c 'pg_dump -U "$POSTGRES_USER" "$POSTGRES_DB"' \ + | gzip > ./backups/db_$(date +%Y-%m-%d).sql.gz + +# 2. Uploaded media (originals + derivatives) out of the named volume. +# NOTE the mountpoint is /src, not /media: if the volume is ever empty, Docker +# pre-populates a fresh mount from the image's own directory, and alpine ships a +# /media containing cdrom/floppy/usb. Mounting somewhere the image has nothing +# avoids silently tarring (and polluting the volume with) those. +docker run --rm \ + -v eventsnap_media_data:/src:ro -v "$PWD/backups":/backup \ + alpine tar czf /backup/media_$(date +%Y-%m-%d).tar.gz -C /src . + +# 3. Export archives — a SEPARATE volume (see the security note below). +docker run --rm \ + -v eventsnap_exports_data:/src:ro -v "$PWD/backups":/backup \ + alpine tar czf /backup/exports_$(date +%Y-%m-%d).tar.gz -C /src . + +# Weekly offsite sync of the three artefacts above. +rsync -az ./backups/ user@storagebox.example.com:backup/eventsnap/ ``` -The `/media` volume holds originals, previews, thumbnails, exports, and DB backups — a single path to back up. +Volume names are prefixed with the compose project name — `eventsnap_` if you run +from a directory called `eventsnap`. Confirm yours with `docker volume ls`. + +> **Exports are deliberately NOT under `/media`.** They live on their own +> `exports_data` volume (`EXPORT_PATH=/exports`) because a keepsake archive +> contains every photo in the event; keeping it outside the media tree is what +> stops it being reachable except through the ticket-gated download handler. +> Backing up only the media volume therefore loses every generated keepsake. --- diff --git a/backend/migrations/017_join_ip_rate.down.sql b/backend/migrations/017_join_ip_rate.down.sql new file mode 100644 index 0000000..86d16c5 --- /dev/null +++ b/backend/migrations/017_join_ip_rate.down.sql @@ -0,0 +1 @@ +DELETE FROM config WHERE key IN ('join_ip_rate_per_min', 'admin_login_rate_enabled'); diff --git a/backend/migrations/017_join_ip_rate.up.sql b/backend/migrations/017_join_ip_rate.up.sql new file mode 100644 index 0000000..135e49f --- /dev/null +++ b/backend/migrations/017_join_ip_rate.up.sql @@ -0,0 +1,18 @@ +-- Per-IP flood ceiling for /join, and the `admin_login_rate_enabled` toggle that +-- every prior migration forgot to seed. +-- +-- Rationale: /join was throttled at 5 requests per 60s keyed on the client IP. At a +-- venue every guest is behind one NAT, so the whole party shared a single bucket — +-- 12 guests scanning the QR code within a few seconds meant 5 got in and 7 were +-- turned away. The handler now keys the real anti-spam bucket per (ip, name), the +-- same shape as `recover:{ip}:{name}`, and keeps only a loose per-IP ceiling to bound +-- raw volume. 60/min comfortably covers a whole wedding arriving at once while still +-- capping a flood from a single source. +-- +-- `admin_login_rate_enabled` is read by auth::handlers::admin_login with a code +-- default of `true`, but no migration ever inserted it, so it was invisible to the +-- admin config UI and to the e2e reseed. Seed it explicitly. +INSERT INTO config (key, value) VALUES + ('join_ip_rate_per_min', '60'), + ('admin_login_rate_enabled', 'true') +ON CONFLICT (key) DO NOTHING; diff --git a/backend/migrations/018_derivatives_rev.down.sql b/backend/migrations/018_derivatives_rev.down.sql new file mode 100644 index 0000000..9e2db5a --- /dev/null +++ b/backend/migrations/018_derivatives_rev.down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS idx_upload_derivatives_rev; +ALTER TABLE upload DROP COLUMN IF EXISTS derivatives_rev; diff --git a/backend/migrations/018_derivatives_rev.up.sql b/backend/migrations/018_derivatives_rev.up.sql new file mode 100644 index 0000000..1625e97 --- /dev/null +++ b/backend/migrations/018_derivatives_rev.up.sql @@ -0,0 +1,18 @@ +-- Track which revision of the derivative pipeline produced an upload's preview/display. +-- +-- Rev 1 applies the EXIF orientation tag. Everything generated before it decoded the raw +-- sensor pixels and re-encoded to JPEG (which writes no EXIF), so every portrait phone photo +-- was stored sideways in the feed preview, the diashow display and the keepsake — while the +-- untouched original still rendered upright. +-- +-- Existing rows default to 0 so the startup backfill can find and re-generate them exactly +-- once; bump the constant in services/compression.rs if the pipeline ever changes again. +ALTER TABLE upload ADD COLUMN IF NOT EXISTS derivatives_rev SMALLINT NOT NULL DEFAULT 0; + +-- Only image derivatives are affected — video thumbnails are extracted by ffmpeg, which +-- already honours the rotation matrix. Mark them current so the backfill skips them. +UPDATE upload SET derivatives_rev = 1 WHERE mime_type NOT LIKE 'image/%'; + +CREATE INDEX IF NOT EXISTS idx_upload_derivatives_rev + ON upload (derivatives_rev) + WHERE deleted_at IS NULL; diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index f5ed12a..74f54f0 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -1,11 +1,12 @@ use std::time::Duration; use axum::Json; -use axum::extract::State; +use axum::extract::{ConnectInfo, State}; use axum::http::{HeaderMap, StatusCode}; use chrono::Utc; use rand::Rng; use serde::{Deserialize, Serialize}; +use std::net::SocketAddr; use uuid::Uuid; use crate::auth::jwt; @@ -33,22 +34,32 @@ pub struct JoinResponse { pub async fn join( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result<(StatusCode, Json), AppError> { - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let join_rate_on = config::get_bool(&state.config_cache, "join_rate_enabled", true).await; - if rate_limits_on - && join_rate_on - && !state - .rate_limiter - .check(format!("join:{ip}"), 5, Duration::from_secs(60)) - { - return Err(AppError::TooManyRequests( - "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, - )); + + // Coarse per-IP flood ceiling. `/join` is pre-auth so there is no user to key on, and + // at a venue EVERY guest arrives from one public IP — a tight per-IP bucket meant the + // 6th person through the door was turned away by the 5 ahead of them. So the per-IP + // limit here only bounds raw volume; the real anti-spam bucket is per-name below. + // Cheap enough to run before validation, which keeps a flood of malformed bodies from + // being free. + if rate_limits_on && join_rate_on { + let ip_ceiling = config::get_usize(&state.config_cache, "join_ip_rate_per_min", 60).await; + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("join_ip:{ip}"), + ip_ceiling, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), + Some(retry_after_secs), + )); + } } let display_name = body.display_name.trim(); @@ -66,6 +77,23 @@ pub async fn join( )); } + // Per-guest bucket, keyed like the `recover:{ip}:{name}` limiter below. This carries + // the original 5/60s anti-spam intent, but one guest retrying can no longer consume + // the allowance of everyone else sharing the venue's NAT. + if rate_limits_on && join_rate_on { + let name_key = display_name.to_lowercase(); + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("join:{ip}:{name_key}"), + 5, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), + Some(retry_after_secs), + )); + } + } + let event = Event::find_or_create( &state.pool, &state.config.event_slug, @@ -149,6 +177,7 @@ fn dummy_pin_hash() -> &'static str { pub async fn recover( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result, AppError> { @@ -159,19 +188,19 @@ pub async fn recover( // burn through 3 wrong PINs and lock the victim for 15 minutes — repeated // every 15 minutes, indefinitely. 5 attempts per 15 minutes per (IP, name) // softens that into a real cost. - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let recover_rate_on = config::get_bool(&state.config_cache, "recover_rate_enabled", true).await; if rate_limits_on && recover_rate_on { let name_key = display_name.to_lowercase(); - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("recover:{ip}:{name_key}"), 5, Duration::from_secs(15 * 60), ) { return Err(AppError::TooManyRequests( "Zu viele Versuche. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } @@ -200,9 +229,12 @@ pub async fn recover( // is effectively permanently fragile. if let Some(locked_until) = user.pin_locked_until { if Utc::now() < locked_until { + // The exact deadline is known, so surface it as Retry-After instead of + // making the client guess at the "15 Minuten" in the copy. + let retry_after_secs = (locked_until - Utc::now()).num_seconds().max(1) as u64; return Err(AppError::TooManyRequests( "Zu viele Versuche. Bitte warte 15 Minuten.".into(), - None, + Some(retry_after_secs), )); } // Lockout window expired — wipe the counter and the timestamp. @@ -274,6 +306,7 @@ pub struct AdminLoginResponse { pub async fn admin_login( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result, AppError> { @@ -287,19 +320,23 @@ pub async fn admin_login( // verify) but with no IP-level limit a determined attacker can still mount // a long-running guess campaign. 5 attempts / minute / IP is plenty for // honest typos. - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let admin_rate_on = config::get_bool(&state.config_cache, "admin_login_rate_enabled", true).await; + // Stays keyed by IP on purpose: this guards a single shared credential, so a per-user + // or per-name key would just hand an attacker a fresh bucket per guess. if rate_limits_on && admin_rate_on - && !state - .rate_limiter - .check(format!("admin_login:{ip}"), 5, Duration::from_secs(60)) + && let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("admin_login:{ip}"), + 5, + Duration::from_secs(60), + ) { return Err(AppError::TooManyRequests( "Zu viele Anmeldeversuche. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } @@ -390,22 +427,23 @@ pub struct PinResetRequestBody { /// feed already exposes. pub async fn request_pin_reset( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result { let display_name = body.display_name.trim(); - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; if rate_limits_on { let name_key = display_name.to_lowercase(); - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("pin_reset_req:{ip}:{name_key}"), 3, Duration::from_secs(15 * 60), ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index fb519c1..f29fe36 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -3,13 +3,13 @@ use std::time::Duration; use axum::Json; use axum::extract::{Query, State}; -use axum::http::{HeaderMap, StatusCode}; +use axum::http::StatusCode; use serde::{Deserialize, Serialize}; +use uuid::Uuid; use crate::auth::middleware::RequireAdmin; use crate::error::AppError; use crate::services::config; -use crate::services::rate_limiter::client_ip; use crate::state::AppState; // ── DTOs ───────────────────────────────────────────────────────────────────── @@ -120,6 +120,10 @@ pub async fn patch_config( ("upload_rate_per_hour", true, 1.0, 100_000.0), ("feed_rate_per_min", true, 1.0, 100_000.0), ("export_rate_per_day", true, 1.0, 100_000.0), + // Loose per-IP ceiling on /join. The real anti-spam bucket is per (ip, name); this + // only bounds raw volume from one source, so it must stay well above the size of a + // party arriving at once (see migration 017). + ("join_ip_rate_per_min", true, 1.0, 100_000.0), ("quota_tolerance", false, 0.0, 1.0), ("estimated_guest_count", true, 1.0, 1_000_000.0), ]; @@ -320,25 +324,26 @@ pub async fn export_ticket( } /// Validate a download ticket (single-use) and confirm its session still exists. -async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result<(), AppError> { +/// Resolve a single-use download ticket to the user who minted it. The caller needs the +/// id to key the export rate limit per-user (see `enforce_export_rate`). +async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result { let token_hash = state .sse_tickets .consume(ticket) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; - crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) + let session = crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) .await .map_err(|e| AppError::Internal(e.into()))? .ok_or_else(|| AppError::Unauthorized("Sitzung nicht gefunden.".into()))?; - Ok(()) + Ok(session.user_id) } pub async fn download_zip( State(state): State, Query(q): Query, - headers: HeaderMap, ) -> Result { - authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, &headers).await?; + let user_id = authenticate_download_ticket(&state, &q.ticket).await?; + enforce_export_rate(&state, user_id).await?; let path = resolve_export_file(&state, "zip", "Der ZIP-Export ist noch nicht verfügbar.").await?; @@ -389,10 +394,9 @@ async fn resolve_export_file( pub async fn download_html( State(state): State, Query(q): Query, - headers: HeaderMap, ) -> Result { - authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, &headers).await?; + let user_id = authenticate_download_ticket(&state, &q.ticket).await?; + enforce_export_rate(&state, user_id).await?; let path = resolve_export_file(&state, "html", "Der HTML-Export ist noch nicht verfügbar.").await?; @@ -476,21 +480,24 @@ pub async fn export_status( /// Centralised guard for the export rate limit. Same pattern as upload/feed: master /// switch + per-endpoint switch + numeric value, all stored in `config` and read on /// each request. -async fn enforce_export_rate(state: &AppState, headers: &HeaderMap) -> Result<(), AppError> { +async fn enforce_export_rate(state: &AppState, user_id: Uuid) -> Result<(), AppError> { let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let export_rate_on = config::get_bool(&state.config_cache, "export_rate_enabled", true).await; if !(rate_limits_on && export_rate_on) { return Ok(()); } - let ip = client_ip(headers, "unknown"); let limit = config::get_usize(&state.config_cache, "export_rate_per_day", 3).await; - if !state - .rate_limiter - .check(format!("export:{ip}"), limit, Duration::from_secs(86400)) - { + // Keyed per-user. This was the worst of the IP-keyed limiters: 3 downloads per DAY + // shared across every guest behind the venue's public IP, so the fourth person to + // fetch their keepsake was locked out until the next day. + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("export:{user_id}"), + limit, + Duration::from_secs(86400), + ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } Ok(()) diff --git a/backend/src/handlers/feed.rs b/backend/src/handlers/feed.rs index 3382539..2be8f5e 100644 --- a/backend/src/handlers/feed.rs +++ b/backend/src/handlers/feed.rs @@ -2,7 +2,6 @@ use std::time::Duration; use axum::Json; use axum::extract::{Query, State}; -use axum::http::HeaderMap; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -10,7 +9,6 @@ use uuid::Uuid; use crate::auth::middleware::AuthUser; use crate::error::AppError; use crate::services::config; -use crate::services::rate_limiter::client_ip; use crate::state::AppState; #[derive(Deserialize)] @@ -61,21 +59,23 @@ struct FeedRow { pub async fn feed( State(state): State, auth: AuthUser, - headers: HeaderMap, Query(q): Query, ) -> Result, AppError> { - let ip = client_ip(&headers, "unknown"); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let feed_rate_on = config::get_bool(&state.config_cache, "feed_rate_enabled", true).await; if rate_limits_on && feed_rate_on { let rate_limit = config::get_usize(&state.config_cache, "feed_rate_per_min", 60).await; - if !state - .rate_limiter - .check(format!("feed:{ip}"), rate_limit, Duration::from_secs(60)) - { + // Keyed per-user, exactly like `feed_delta` below: at a venue every guest shares + // one public IP, so an IP key gave the whole party a single 60/min bucket and the + // fastest scroller starved everyone else. + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("feed:{}", auth.user_id), + rate_limit, + Duration::from_secs(60), + ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } @@ -225,14 +225,14 @@ pub async fn feed_delta( let feed_rate_on = config::get_bool(&state.config_cache, "feed_rate_enabled", true).await; if rate_limits_on && feed_rate_on { let rate_limit = config::get_usize(&state.config_cache, "feed_rate_per_min", 60).await; - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("feed_delta:{}", auth.user_id), rate_limit, Duration::from_secs(60), ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } diff --git a/backend/src/handlers/test_admin.rs b/backend/src/handlers/test_admin.rs index 7e44c7c..c6f4202 100644 --- a/backend/src/handlers/test_admin.rs +++ b/backend/src/handlers/test_admin.rs @@ -13,9 +13,10 @@ use crate::auth::middleware::RequireAdmin; use crate::error::AppError; use crate::state::AppState; -/// Truncates every event-scoped table, wipes media on disk, and reseeds the -/// `config` table from migration defaults. Requires an admin JWT — even with -/// `EVENTSNAP_TEST_MODE=1` it cannot be hit anonymously. +/// Truncates every event-scoped table, wipes media on disk, and reseeds the `config` +/// table: numeric values from the migration defaults, but every feature toggle forced +/// OFF (production seeds them ON — see the note at the reseed below). Requires an admin +/// JWT — even with `EVENTSNAP_TEST_MODE=1` it cannot be hit anonymously. pub async fn truncate_all( State(state): State, RequireAdmin(_auth): RequireAdmin, @@ -40,8 +41,19 @@ pub async fn truncate_all( .execute(&state.pool) .await?; - // Reseed config — mirrors migrations 005, 009 and 015. Kept in sync by hand - // because pulling SQL out of the migration files at runtime is fragile. + // Reseed config. The NUMERIC values mirror migrations 005/015/016; the BOOLEAN + // toggles deliberately do NOT — migration 009 seeds every one of them `true` + // (production), and this forces them `false` so the suite isn't fighting rate limits + // and quotas it isn't testing. + // + // Be aware of what that costs: this runs as an auto-fixture before EVERY test, so no + // test starts from production's config unless it explicitly turns a toggle back on + // (02-upload/rate-limit, 07-adversarial/ddos, 01-auth/rate-limit-nat, …). That blind + // spot is exactly why an entire class of per-IP limiter bugs went unnoticed: the + // limiters were simply off. When adding a limiter or quota, add a spec that enables it. + // + // Kept in sync by hand because pulling SQL out of the migration files at runtime is + // fragile — if you add a config key in a migration, add it here too. sqlx::query( r#"INSERT INTO config (key, value) VALUES ('max_image_size_mb', '20'), @@ -49,6 +61,7 @@ pub async fn truncate_all( ('upload_rate_per_hour', '100'), ('feed_rate_per_min', '60'), ('export_rate_per_day', '3'), + ('join_ip_rate_per_min', '60'), ('quota_tolerance', '0.75'), ('estimated_guest_count', '100'), ('compression_concurrency', '2'), @@ -57,6 +70,7 @@ pub async fn truncate_all( ('feed_rate_enabled', 'false'), ('export_rate_enabled', 'false'), ('join_rate_enabled', 'false'), + ('admin_login_rate_enabled', 'false'), ('quota_enabled', 'false'), ('storage_quota_enabled', 'false'), ('upload_count_quota_enabled', 'false'), diff --git a/backend/src/main.rs b/backend/src/main.rs index fd5b3a8..569ee17 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -2,7 +2,6 @@ use anyhow::Result; use axum::Router; use axum::extract::DefaultBodyLimit; use axum::routing::{delete, get, patch, post}; -use tower_http::services::ServeDir; use tower_http::trace::TraceLayer; use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt}; @@ -45,10 +44,12 @@ async fn main() -> Result<()> { let state = AppState::new(pool.clone(), config.clone()); - // Backfill the big-screen display derivative for image uploads processed before it - // existed (v0.17.x). Fire-and-forget behind the compression semaphore; each upload - // falls back to its original in the diashow until its display is generated. - state.compression.backfill_missing_display().await; + // Regenerate image derivatives an older pipeline produced: the big-screen display for + // uploads processed before it existed (v0.17.x), and anything predating the current + // DERIVATIVES_REV (rev 1 applies the EXIF orientation, without which every portrait + // phone photo is stored sideways). Fire-and-forget behind the compression semaphore; + // originals are never touched, so a failure just retries on the next start. + state.compression.backfill_stale_derivatives().await; // Re-spawn exports for events that were released but whose keepsake never finished // (crash mid-export). Needs the media/export paths + SSE sender, so it runs here @@ -229,45 +230,39 @@ async fn main() -> Result<()> { api }; - // Serve media files from disk - let media_service = ServeDir::new(&config.media_path); - + // NOTE: media is deliberately NOT served over HTTP. + // + // Files live under `media_path` so the compression worker and the export job can read + // them off disk, but nothing may pull them straight from `/media/**` — that bypasses + // the visibility checks (soft-delete + ban-hide) that make a host takedown stick. + // Every legitimate fetch goes through `/api/v1/upload/{id}/{original,preview,display, + // thumbnail}`, which filter via `find_visible_media`; those are the only media URLs the + // backend ever emits (see `handlers::feed`). + // + // This used to be a `ServeDir` on `/media` with four `nest_service` blockers on the + // subtrees above it. That was bypassable: axum routes on the RAW path while `ServeDir` + // percent-decodes afterwards, so `/media/%70reviews/{id}.jpg` missed every blocker, + // fell through to the `ServeDir`, and was decoded back to `previews/` on disk — serving + // a taken-down photo to anyone, unauthenticated. Any single escaped byte worked, in all + // four subtrees. Deleting the route removes the vector outright rather than racing the + // decoder; `/media/**` now 404s regardless of encoding. let router = Router::new() .route("/health", get(|| async { "ok" })) .merge(api) - // Block direct HTTP access to ALL media subtrees. The files live under - // `media_path` (so the compression worker and export can read them off disk) but - // must NOT be pullable straight from `/media/**` — that bypasses the visibility - // checks (soft-delete + ban-hide) in the gated handlers. Every legitimate fetch - // goes through `/api/v1/upload/{id}/{original,preview,thumbnail}`, which filter - // via `find_visible_media`. The more specific nests take precedence over the - // `/media` ServeDir below (which, with all three subtrees blocked, now serves - // nothing — kept as a backstop). - .nest_service( - "/media/originals", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/previews", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/displays", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/thumbnails", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service("/media", media_service) .layer(TraceLayer::new_for_http()) .with_state(state); let listener = tokio::net::TcpListener::bind(("0.0.0.0", config.app_port)).await?; tracing::info!("listening on {}", listener.local_addr()?); - axum::serve(listener, router) - .with_graceful_shutdown(shutdown_signal()) - .await?; + // `into_make_service_with_connect_info` is required by the pre-auth handlers, which + // extract `ConnectInfo` to use the peer address as the rate-limit key when + // X-Forwarded-For is absent. Without it those extractors fail at runtime. + axum::serve( + listener, + router.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(shutdown_signal()) + .await?; Ok(()) } diff --git a/backend/src/models/upload.rs b/backend/src/models/upload.rs index 3c47cb2..bcfb153 100644 --- a/backend/src/models/upload.rs +++ b/backend/src/models/upload.rs @@ -148,6 +148,21 @@ impl Upload { Ok(()) } + /// Stamp which revision of the derivative pipeline produced this row's preview/display, + /// so the startup backfill can find rows generated by an older one exactly once. + pub async fn set_derivatives_rev( + pool: &PgPool, + id: Uuid, + rev: i16, + ) -> Result<(), sqlx::Error> { + sqlx::query("UPDATE upload SET derivatives_rev = $2 WHERE id = $1") + .bind(id) + .bind(rev) + .execute(pool) + .await?; + Ok(()) + } + pub async fn set_thumbnail_path( pool: &PgPool, id: Uuid, diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index eeb8491..ff74358 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; use anyhow::{Context, Result}; +use image::ImageDecoder; use sqlx::PgPool; use tokio::sync::{Semaphore, broadcast}; use uuid::Uuid; @@ -48,6 +49,16 @@ impl CompressionWorker { self.generation.fetch_add(1, Ordering::SeqCst); } + /// How many times `do_process` is attempted before an upload is given up on. The + /// give-up path is user-visible (the photo disappears), so transient infrastructure + /// errors must not reach it. + const MAX_PROCESS_ATTEMPTS: u32 = 3; + + /// Revision of the image-derivative pipeline. Bump this whenever a change makes existing + /// previews/displays wrong, so `backfill_stale_derivatives` regenerates them once on the + /// next start. Rev 1 = EXIF orientation is applied. + const DERIVATIVES_REV: i16 = 1; + /// Spawn a background task to process an uploaded file. pub fn process(&self, upload_id: Uuid, original_path: String, mime_type: String) { let worker = self.clone(); @@ -60,10 +71,35 @@ impl CompressionWorker { if worker.generation.load(Ordering::SeqCst) != born_at { return; } - match worker - .do_process(upload_id, &original_path, &mime_type) - .await - { + // Retry before giving up. Most failures here are transient and self-clearing — + // an ENOSPC spike while several guests upload at once, a momentary DB-pool + // exhaustion, a panic inside the image codec — and the give-up path is + // user-visible data loss, so it is worth a few seconds to avoid entering it. + let mut attempt = 1u32; + let outcome = loop { + match worker + .do_process(upload_id, &original_path, &mime_type) + .await + { + Ok(v) => break Ok(v), + Err(e) if attempt < Self::MAX_PROCESS_ATTEMPTS => { + tracing::warn!( + error = ?e, %upload_id, attempt, + "compression attempt failed; retrying" + ); + tokio::time::sleep(std::time::Duration::from_secs(2u64.pow(attempt))) + .await; + attempt += 1; + // The data may have been reset while we slept (e2e TRUNCATE). + if worker.generation.load(Ordering::SeqCst) != born_at { + return; + } + } + Err(e) => break Err(e), + } + }; + + match outcome { Ok(_) => { tracing::info!("compression completed for upload {upload_id}"); let _ = worker.sse_tx.send(SseEvent { @@ -72,21 +108,31 @@ impl CompressionWorker { }); } Err(e) => { - tracing::error!("compression failed for upload {upload_id}: {e:#}"); - // Auto-cleanup: a failed transcode would otherwise leave a - // permanently broken feed card, silently charge the uploader's - // quota, and orphan the original on disk. Refund + soft-delete - // (one tx, so v_feed excludes it), remove the orphan file, then - // tell the uploader (upload-error toast) and evict the card - // everywhere (upload-deleted, already handled by the feed). + tracing::error!( + "compression failed for upload {upload_id} after {attempt} attempt(s): {e:#}" + ); + // Refund + soft-delete (one tx, so v_feed excludes it) so a failed + // transcode doesn't leave a permanently broken feed card or silently + // charge the uploader's quota. Then tell the uploader (upload-error + // toast) and evict the card everywhere (upload-deleted). + // + // The ORIGINAL IS DELIBERATELY KEPT. This path used to `remove_file` it + // unconditionally, which meant any transient error — a disk-full blip + // while saving a derivative, a pool hiccup, a panic in the image codec — + // irreversibly destroyed the guest's only copy of a photo they can never + // retake. The row is only soft-deleted, so keeping the bytes makes the + // upload fully recoverable; the file is orphaned rather than lost, and + // the path is logged so it can be found. `backfill_stale_derivatives` + // already refuses to destroy data on error for exactly this reason. let _ = Upload::set_compression_status(&worker.pool, upload_id, "failed").await; if let Err(del) = Upload::soft_delete(&worker.pool, upload_id).await { tracing::warn!(error = ?del, %upload_id, "failed to soft-delete after compression failure"); } - let orphan = worker.media_path.join(&original_path); - if let Err(rm) = tokio::fs::remove_file(&orphan).await { - tracing::warn!(error = ?rm, path = %orphan.display(), "failed to remove orphaned original"); - } + tracing::warn!( + %upload_id, + path = %worker.media_path.join(&original_path).display(), + "original retained for recovery after compression failure" + ); let _ = worker.sse_tx.send(SseEvent { event_type: "upload-error".to_string(), data: serde_json::json!({ "upload_id": upload_id, "error": e.to_string() }) @@ -117,6 +163,7 @@ impl CompressionWorker { .await?; Upload::set_preview_path(&self.pool, upload_id, &preview_rel).await?; Upload::set_display_path(&self.pool, upload_id, &display_rel).await?; + Upload::set_derivatives_rev(&self.pool, upload_id, Self::DERIVATIVES_REV).await?; tracing::info!("preview + display generated for upload {upload_id}"); } else if mime_type.starts_with("video/") { let thumb_rel = self.generate_video_thumbnail(upload_id, &original).await?; @@ -172,7 +219,24 @@ impl CompressionWorker { limits.max_image_height = Some(12_000); limits.max_alloc = Some(256 * 1024 * 1024); reader.limits(limits); - let img = reader.decode().context("failed to decode image")?; + + // Apply the EXIF orientation. Phones do not rotate the sensor data — they record + // the physical camera orientation in a tag and store the pixels as shot. `decode()` + // hands back those raw pixels, and the JPEG re-encode below writes no EXIF at all, + // so skipping this stores EVERY portrait photo sideways in the feed preview, the + // 2048px diashow display and the keepsake — while the untouched original still + // renders upright, which is why it looks like a viewer bug rather than a pipeline + // one. `into_decoder` carries the limits set above through to the decoder, so the + // decompression-bomb guard is unaffected. + let mut decoder = reader.into_decoder().context("failed to decode image")?; + // A missing or malformed tag is not a failure: most images simply have none. + let orientation = decoder + .orientation() + .unwrap_or(image::metadata::Orientation::NoTransforms); + let mut img = + image::DynamicImage::from_decoder(decoder).context("failed to decode image")?; + img.apply_orientation(orientation); + let img = img; // Preview: max 800px, preserving aspect ratio (data-saver feed). img.resize( @@ -221,33 +285,41 @@ impl CompressionWorker { )) } - /// One-time backfill: existing image uploads processed before the display derivative - /// existed have a preview but no `display_path`. Regenerate both derivatives for them - /// (decode is cheap and idempotent) and set the path. Unlike the failure path in - /// `process`, a backfill error is logged and skipped — it must NEVER soft-delete an - /// upload that already has a working preview. Fire-and-forget from startup. - pub async fn backfill_missing_display(&self) { + /// Regenerate image derivatives that an older pipeline produced. Fire-and-forget from + /// startup; picks up two cases, both of which leave the ORIGINAL untouched: + /// + /// - uploads processed before the `display` derivative existed (preview but no + /// `display_path`), and + /// - uploads whose derivatives predate `DERIVATIVES_REV` — currently rev 1, which applies + /// the EXIF orientation. Everything generated before it is stored sideways for any + /// portrait phone photo. + /// + /// Unlike the failure path in `process`, a backfill error is logged and skipped — it must + /// NEVER destroy or soft-delete an upload that already has a working preview. + pub async fn backfill_stale_derivatives(&self) { let rows = sqlx::query_as::<_, (Uuid, String, String)>( "SELECT id, original_path, mime_type FROM upload - WHERE display_path IS NULL AND preview_path IS NOT NULL - AND deleted_at IS NULL AND mime_type LIKE 'image/%'", + WHERE deleted_at IS NULL AND mime_type LIKE 'image/%' + AND original_path IS NOT NULL + AND ( + (display_path IS NULL AND preview_path IS NOT NULL) + OR derivatives_rev < $1 + )", ) + .bind(Self::DERIVATIVES_REV) .fetch_all(&self.pool) .await; let rows = match rows { Ok(r) => r, Err(e) => { - tracing::warn!(error = ?e, "display backfill query failed"); + tracing::warn!(error = ?e, "derivative backfill query failed"); return; } }; if rows.is_empty() { return; } - tracing::info!( - "backfilling display derivative for {} upload(s)", - rows.len() - ); + tracing::info!("regenerating derivatives for {} upload(s)", rows.len()); for (id, original_path, mime_type) in rows { let worker = self.clone(); tokio::spawn(async move { @@ -260,12 +332,19 @@ impl CompressionWorker { Ok((preview_rel, display_rel)) => { let _ = Upload::set_preview_path(&worker.pool, id, &preview_rel).await; let _ = Upload::set_display_path(&worker.pool, id, &display_rel).await; - tracing::info!("display backfilled for upload {id}"); + let _ = Upload::set_derivatives_rev( + &worker.pool, + id, + Self::DERIVATIVES_REV, + ) + .await; + tracing::info!("derivatives regenerated for upload {id}"); } Err(e) => { - // Leave the existing preview intact; the diashow falls back to the - // original for this upload until a later successful pass. - tracing::warn!(error = ?e, %id, "display backfill failed; leaving as-is"); + // Leave the existing derivatives and the original intact; this row is + // simply retried on the next start. The rev stays behind, which is the + // marker that it still needs doing. + tracing::warn!(error = ?e, %id, "derivative backfill failed; leaving as-is"); } } }); diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index bc2f247..ca0df1f 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -17,13 +17,14 @@ impl RateLimiter { } } - /// Returns `true` if the request is allowed, `false` if rate-limited. - pub fn check(&self, key: impl Into, max: usize, window: Duration) -> bool { - self.check_with_retry(key, max, window).is_ok() - } - /// Returns `Ok(())` if allowed, `Err(retry_after_secs)` if rate-limited. /// `retry_after_secs` is how long until the oldest slot in the window expires. + /// + /// This is deliberately the ONLY entry point. There used to be a `check()` wrapper + /// returning a plain bool, and 7 of the 8 call sites used it and then hard-coded + /// `None` for the response's `Retry-After` — so a throttled client was told to back + /// off but never for how long. Forcing every caller through the `Result` makes the + /// retry delay impossible to discard by accident. pub fn check_with_retry( &self, key: impl Into, @@ -84,6 +85,11 @@ impl RateLimiter { /// appends is the real client. A client can prepend arbitrary spoofed values to /// the left of XFF to dodge throttles — those are ignored here. This assumes /// exactly one trusted proxy (Caddy); revisit if that changes. +/// +/// Pass the peer address as `fallback`, never a constant. Every caller used to pass +/// the literal `"unknown"`, so any request that arrived without XFF — i.e. anything +/// reaching the app directly rather than through Caddy — shared ONE bucket with every +/// other such request, turning the limiter into a self-inflicted global throttle. pub fn client_ip(headers: &axum::http::HeaderMap, fallback: &str) -> String { headers .get("x-forwarded-for") @@ -104,29 +110,35 @@ mod tests { #[test] fn allows_up_to_max_then_blocks() { let rl = RateLimiter::new(); - assert!(rl.check("k", 3, MIN)); - assert!(rl.check("k", 3, MIN)); - assert!(rl.check("k", 3, MIN)); - assert!(!rl.check("k", 3, MIN), "the 4th request must be blocked"); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!( + rl.check_with_retry("k", 3, MIN).is_err(), + "the 4th request must be blocked" + ); } #[test] fn keys_are_independent() { let rl = RateLimiter::new(); - assert!(rl.check("a", 1, MIN)); - assert!(!rl.check("a", 1, MIN)); - assert!(rl.check("b", 1, MIN), "a different key has its own window"); + assert!(rl.check_with_retry("a", 1, MIN).is_ok()); + assert!(rl.check_with_retry("a", 1, MIN).is_err()); + assert!( + rl.check_with_retry("b", 1, MIN).is_ok(), + "a different key has its own window" + ); } #[test] fn window_slides_and_allows_again_after_expiry() { let rl = RateLimiter::new(); let w = Duration::from_millis(40); - assert!(rl.check("k", 1, w)); - assert!(!rl.check("k", 1, w)); + assert!(rl.check_with_retry("k", 1, w).is_ok()); + assert!(rl.check_with_retry("k", 1, w).is_err()); std::thread::sleep(Duration::from_millis(55)); assert!( - rl.check("k", 1, w), + rl.check_with_retry("k", 1, w).is_ok(), "the slot should expire once the window passes" ); } @@ -191,10 +203,13 @@ mod tests { #[test] fn clear_resets_every_window() { let rl = RateLimiter::new(); - assert!(rl.check("k", 1, MIN)); - assert!(!rl.check("k", 1, MIN)); + assert!(rl.check_with_retry("k", 1, MIN).is_ok()); + assert!(rl.check_with_retry("k", 1, MIN).is_err()); rl.clear(); - assert!(rl.check("k", 1, MIN), "clear() must free the window"); + assert!( + rl.check_with_retry("k", 1, MIN).is_ok(), + "clear() must free the window" + ); } /// `prune()` is a memory-leak guard: without it a long-lived process keeps one HashMap @@ -216,7 +231,7 @@ mod tests { .insert("stale".to_string(), vec![ancient]); // ...alongside a key that is still inside its window. - assert!(rl.check("live", 5, MIN)); + assert!(rl.check_with_retry("live", 5, MIN).is_ok()); assert_eq!(rl.windows.lock().unwrap().len(), 2); rl.prune(); @@ -239,13 +254,13 @@ mod tests { // prune() dropped live keys, every background sweep would hand attackers a fresh // budget. let rl = RateLimiter::new(); - assert!(rl.check("k", 1, MIN)); - assert!(!rl.check("k", 1, MIN)); + assert!(rl.check_with_retry("k", 1, MIN).is_ok()); + assert!(rl.check_with_retry("k", 1, MIN).is_err()); rl.prune(); assert!( - !rl.check("k", 1, MIN), + rl.check_with_retry("k", 1, MIN).is_err(), "prune() must not clear a window that is still active" ); } diff --git a/e2e/Caddyfile.test b/e2e/Caddyfile.test index 02fb85c..e481ea7 100644 --- a/e2e/Caddyfile.test +++ b/e2e/Caddyfile.test @@ -12,10 +12,17 @@ # Mirror prod's security headers (minus HSTS, which is HTTPS-only). header { X-Content-Type-Options "nosniff" - X-Frame-Options "DENY" Referrer-Policy "strict-origin-when-cross-origin" } + # Mirror prod's export carve-out: the keepsake download targets a hidden + # same-origin iframe, and WebKit enforces XFO before Content-Disposition. + # Two disjoint matchers, not an override — see the comment in ../Caddyfile. + @framable path /api/v1/export/zip /api/v1/export/html + @not_framable not path /api/v1/export/zip /api/v1/export/html + header @framable X-Frame-Options "SAMEORIGIN" + header @not_framable X-Frame-Options "DENY" + reverse_proxy /api/* app:3000 reverse_proxy /media/* app:3000 reverse_proxy /health app:3000 diff --git a/e2e/docker-compose.test.yml b/e2e/docker-compose.test.yml index adf77d5..15a0440 100644 --- a/e2e/docker-compose.test.yml +++ b/e2e/docker-compose.test.yml @@ -42,11 +42,20 @@ services: EVENT_NAME: E2E Test Event APP_PORT: '3000' MEDIA_PATH: /media + # Exports MUST live outside MEDIA_PATH — see the note on the volume below and + # config.rs::validate. Omitting this left exports on the container's writable + # layer at the /exports default, so the test stack diverged from the prod layout + # it claims to mirror, and export-leak/export-video wrote real archives into + # ephemeral storage. + EXPORT_PATH: /exports SESSION_EXPIRY_DAYS: '30' EVENTSNAP_TEST_MODE: '1' # ENABLES /admin/__truncate — never set in prod RUST_LOG: eventsnap_backend=info,tower_http=warn volumes: - media_data:/media + # Separate volume, exactly as in production: a keepsake archive contains every + # photo in the event, so it is kept off the media tree. + - exports_data:/exports expose: - '3000' @@ -75,3 +84,4 @@ services: volumes: media_data: + exports_data: diff --git a/e2e/fixtures/db.ts b/e2e/fixtures/db.ts index 1dafdb0..cdbe6e2 100644 --- a/e2e/fixtures/db.ts +++ b/e2e/fixtures/db.ts @@ -54,6 +54,27 @@ export const db = { ); }, + async compressionStatus(uploadId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ compression_status: string }>( + `SELECT compression_status FROM upload WHERE id = $1`, + [uploadId] + ); + return r.rows[0]?.compression_status ?? null; + }); + }, + + /** Which revision of the derivative pipeline produced this row's preview/display. */ + async derivativesRev(uploadId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ derivatives_rev: number }>( + `SELECT derivatives_rev FROM upload WHERE id = $1`, + [uploadId] + ); + return r.rows[0]?.derivatives_rev ?? null; + }); + }, + async countUploadsForUser(userId: string): Promise { return withClient(async (c) => { const r = await c.query<{ count: string }>( diff --git a/e2e/helpers/webkit.ts b/e2e/helpers/webkit.ts new file mode 100644 index 0000000..9c98cd6 --- /dev/null +++ b/e2e/helpers/webkit.ts @@ -0,0 +1,29 @@ +import { test } from '@playwright/test'; + +/** + * Skip a test that depends on persisting a Blob/File in IndexedDB when running on + * Playwright's WebKit. + * + * The client upload queue (`frontend/src/lib/upload-queue.ts`) stores the file itself in + * IndexedDB so a backgrounded or reloaded phone can resume the upload. Playwright's Linux + * WebKit build cannot store Blobs in IndexedDB at all — `put()` fails with + * "UnknownError: Error preparing Blob/File data to be stored in object store". Verified to + * be the harness, not the app: a Blob constructed in-page with `new Blob([bytes])` fails + * exactly the same way, while Chromium stores both that and a `setInputFiles` File fine. + * Real iOS Safari supports Blobs in IndexedDB, so this is NOT evidence of a bug on the + * platform these tests exist to protect. + * + * Any test that drives the composer (FAB → UploadSheet → /upload → submit) hits this, + * because `handleSubmit` awaits `addToQueue`, which throws before it can navigate. + * + * This is deliberately narrow. WebKit still runs every API-driven upload test, the whole of + * 01-auth, 03-feed and 06-export — including the keepsake download, which only WebKit can + * meaningfully verify. If Playwright's WebKit ever gains IndexedDB Blob support, delete this + * helper and the four call sites. + */ +export function skipIfNoIdbBlobs(browserName: string) { + test.skip( + browserName === 'webkit', + "Playwright's Linux WebKit cannot store Blobs in IndexedDB (harness limitation, not an iOS one) — the client upload queue can't be exercised there" + ); +} diff --git a/e2e/page-objects/export-page.ts b/e2e/page-objects/export-page.ts index d8dce62..687f2f6 100644 --- a/e2e/page-objects/export-page.ts +++ b/e2e/page-objects/export-page.ts @@ -37,12 +37,19 @@ export class ExportPage { /** * The "Download" button inside the card whose heading is `heading`. * - * Scoped to the card element (`div.rounded-xl`) rather than "any div containing the - * heading" — the latter also matches the page wrapper, which contains BOTH cards' buttons. + * Scoped to the card element rather than "any div containing the heading" — the latter + * also matches the page wrapper, which contains BOTH cards' buttons. + * + * The scope class is `div.card` (see the ZIP/HTML cards in + * frontend/src/routes/export/+page.svelte). It was previously `div.rounded-xl`, which + * matched NOTHING: `.card` is a Tailwind `@apply` component class + * (frontend/src/lib/styles/components.css) so the DOM only ever carries `class="card p-5"` + * — and the utility it applies is `rounded-2xl` anyway. Both card-scoped locators were + * therefore dead, which is why the "shows enabled download buttons" test was red. */ private cardButton(heading: string): Locator { return this.page - .locator('div.rounded-xl') + .locator('div.card') .filter({ has: this.page.getByRole('heading', { name: heading, exact: true }) }) .getByRole('button', { name: 'Download', exact: true }); } diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index a39125b..15c6a7e 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -146,8 +146,18 @@ export default defineConfig({ // that on `@smoke` — which exists on exactly two specs — meant the entire iOS guarantee was // one happy path and one join test. Every other UA here is a secondary browser and a smoke // check is proportionate; WebKit is not. Give it the core journeys the guest actually walks: - // join/recover, upload, and browse the feed. - testMatch: ['**/__smoke/**', '**/01-auth/**', '**/02-upload/**', '**/03-feed/**'], + // join/recover, upload, browse the feed — and take the keepsake home. + // + // 06-export is here because WebKit is the ONLY engine that enforces X-Frame-Options on the + // hidden download iframe. Excluding it is what let a site-wide `XFO: DENY` ship a keepsake + // download that silently did nothing on iOS. See 06-export/download-iframe.spec.ts. + testMatch: [ + '**/__smoke/**', + '**/01-auth/**', + '**/02-upload/**', + '**/03-feed/**', + '**/06-export/**', + ], }, { name: 'firefox-android', diff --git a/e2e/specs/01-auth/join.spec.ts b/e2e/specs/01-auth/join.spec.ts index 58a2940..c5eadb9 100644 --- a/e2e/specs/01-auth/join.spec.ts +++ b/e2e/specs/01-auth/join.spec.ts @@ -13,7 +13,11 @@ test.describe('Auth — join flow', () => { const join = new JoinPage(page); await join.goto(); - await expect(page.getByRole('heading', { name: 'Willkommen!' })).toBeVisible(); + // The join form's landing state. There is no "Willkommen!" heading — the wedding + // redesign (f243bfe) split it into a "Willkommen bei" lead-in plus the event name as + // the

, and this assertion was never updated, so it had been failing since. + // Anchor on the testid the markup provides rather than on copy. + await expect(page.getByTestId('join-event-name')).toBeVisible(); const { pin } = await join.joinAs('Alice'); expect(pin).toMatch(/^\d{4}$/); diff --git a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts new file mode 100644 index 0000000..f025b30 --- /dev/null +++ b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts @@ -0,0 +1,144 @@ +/** + * Regression guard — the door must not close on a venue behind one NAT. + * + * `/join` was throttled 5 per 60s keyed purely on the client IP. Every guest at a venue + * arrives from the same public IP (that is what a NAT is), so the whole party shared one + * bucket: 12 guests scanning the QR code within a few seconds meant 5 got in and 7 were + * turned away — with no Retry-After to tell them when to try again. `/feed` (60/min) and + * `/export` (3/DAY) had the identical defect. + * + * These ran green for the same structural reason every time: the e2e reseed forces every + * limiter toggle OFF before each test, so nothing here was ever exercised. Enable them + * explicitly, exactly as 02-upload/rate-limit does. + */ +import { test, expect } from '../../fixtures/test'; +import { BASE } from '../../helpers/env'; + +test.describe('Rate limits — guests behind a shared NAT', () => { + test('a dozen guests can all join from one IP, and 429s carry Retry-After', async ({ + api, + adminToken, + }) => { + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + join_rate_enabled: 'true', + }); + + // Twelve DISTINCT guests, same source IP — the arrival burst at a real party. + const names = Array.from({ length: 12 }, (_, i) => `NatGuest${i}`); + const results = await Promise.all( + names.map((display_name) => + fetch(`${BASE}/api/v1/join`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name }), + }) + ) + ); + + const rejected = results.filter((r) => r.status === 429); + expect( + rejected.length, + `all 12 guests must get in from one IP; ${rejected.length} were turned away` + ).toBe(0); + expect(results.every((r) => r.status === 201)).toBe(true); + }); + + test('one guest retrying their own name is still throttled, and told for how long', async ({ + api, + adminToken, + }) => { + // The per-name bucket must still bite — otherwise the NAT fix would have simply + // removed the anti-spam limit rather than re-keyed it. + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + join_rate_enabled: 'true', + }); + + const attempt = () => + fetch(`${BASE}/api/v1/join`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: 'RepeatOffender' }), + }); + + // 5 per 60s for the same (ip, name): the first succeeds (201), the next four collide + // with the taken name (409), and the sixth exhausts the bucket. + const codes: number[] = []; + for (let i = 0; i < 6; i++) codes.push((await attempt()).status); + + expect(codes[0], 'the first join should succeed').toBe(201); + expect(codes.at(-1), 'the 6th attempt on one name must be throttled').toBe(429); + + const throttled = await attempt(); + expect(throttled.status).toBe(429); + const retryAfter = throttled.headers.get('retry-after'); + expect(retryAfter, '429 must tell the client when to come back').toBeTruthy(); + expect(Number(retryAfter)).toBeGreaterThan(0); + expect(Number(retryAfter)).toBeLessThanOrEqual(60); + }); + + test('the feed limit is per-user, not per-IP', async ({ api, adminToken, guest }) => { + // Two guests, one IP. With a limit of 3/min an IP key would let the first guest's + // three reads starve the second entirely. + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + feed_rate_enabled: 'true', + feed_rate_per_min: '3', + }); + + const a = await guest('FeedHog'); + const b = await guest('FeedVictim'); + const read = (jwt: string) => + fetch(`${BASE}/api/v1/feed`, { headers: { Authorization: `Bearer ${jwt}` } }); + + // Guest A burns their whole allowance. + for (let i = 0; i < 3; i++) expect((await read(a.jwt)).status).toBe(200); + expect((await read(a.jwt)).status, "A's own 4th read is throttled").toBe(429); + + // Guest B must be entirely unaffected. + expect((await read(b.jwt)).status, 'B must not inherit A’s exhausted bucket').toBe(200); + }); + + test('the export limit is per-user — one guest cannot spend the whole venue’s quota', async ({ + api, + adminToken, + guest, + host, + db, + }) => { + // The sharpest case: 3 downloads per DAY on an IP key meant the 4th guest to fetch + // their keepsake was locked out until tomorrow. + await db.setExportReleased('e2e-test-event', true); + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + export_rate_enabled: 'true', + export_rate_per_day: '1', + }); + + const mintAndFetch = async (jwt: string) => { + const res = await fetch(`${BASE}/api/v1/export/ticket`, { + method: 'POST', + headers: { Authorization: `Bearer ${jwt}` }, + }); + const { ticket } = await res.json(); + return fetch(`${BASE}/api/v1/export/zip?ticket=${encodeURIComponent(ticket)}`); + }; + + const a = await guest('ExportFirst'); + const b = await guest('ExportSecond'); + + // A spends their single daily allowance. The archive itself may not exist (404) — + // what matters is that the limiter admitted the request rather than 429ing it. + expect((await mintAndFetch(a.jwt)).status).not.toBe(429); + expect((await mintAndFetch(a.jwt)).status, 'A’s second download is throttled').toBe(429); + + // B shares A's IP and must still get their keepsake. + expect((await mintAndFetch(b.jwt)).status, 'B must not be locked out by A’s download').not.toBe( + 429 + ); + + // And the host too, for good measure. + expect((await mintAndFetch(host.jwt)).status).not.toBe(429); + }); +}); diff --git a/e2e/specs/02-upload/burst-queue.spec.ts b/e2e/specs/02-upload/burst-queue.spec.ts index 8b538d5..fb9762f 100644 --- a/e2e/specs/02-upload/burst-queue.spec.ts +++ b/e2e/specs/02-upload/burst-queue.spec.ts @@ -26,6 +26,7 @@ * large fixture files. */ import { test, expect } from '../../fixtures/test'; +import { skipIfNoIdbBlobs } from '../../helpers/webkit'; import { FeedPage, UploadSheet } from '../../page-objects'; import { mkdtempSync, copyFileSync, rmSync } from 'node:fs'; import { join } from 'node:path'; @@ -100,7 +101,9 @@ test.describe('Upload — client queue under a burst', () => { guest, signIn, db, + browserName, }) => { + skipIfNoIdbBlobs(browserName); const g = await guest('BurstSerial'); await signIn(page, g); await warmUploadChunk(page); @@ -163,7 +166,9 @@ test.describe('Upload — client queue under a burst', () => { guest, signIn, db, + browserName, }) => { + skipIfNoIdbBlobs(browserName); const g = await guest('BurstResume'); await signIn(page, g); await warmUploadChunk(page); diff --git a/e2e/specs/02-upload/exif-orientation.spec.ts b/e2e/specs/02-upload/exif-orientation.spec.ts new file mode 100644 index 0000000..7f994dd --- /dev/null +++ b/e2e/specs/02-upload/exif-orientation.spec.ts @@ -0,0 +1,80 @@ +/** + * Regression guard — EXIF orientation must be applied when generating derivatives. + * + * Phones do not rotate sensor data. They shoot in the sensor's native landscape and record + * how the camera was held in an EXIF `Orientation` tag. `image`'s `decode()` returns the raw + * pixels and ignores that tag, and the JPEG re-encode writes no EXIF at all — so every + * portrait photo was stored SIDEWAYS in the 800px feed preview, the 2048px diashow display + * and the keepsake, while "Original anzeigen" still rendered it upright (the original keeps + * its tag). That asymmetry is why it reads as a viewer bug instead of a pipeline one. + * + * The fixture is 40x20 landscape pixels tagged Orientation=6 ("rotate 90° CW to display"), + * so a correctly-processed derivative is PORTRAIT (20x40). Asserting on the aspect ratio + * rather than the bytes keeps this robust across encoder changes. + */ +import { test, expect } from '../../fixtures/test'; +import { uploadRaw } from '../../helpers/upload-client'; +import { BASE } from '../../helpers/env'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const EXIF_FIXTURE = join(process.cwd(), 'fixtures', 'media', 'portrait-exif6.jpg'); + +/** + * Read a baseline/progressive JPEG's pixel dimensions from its SOF marker. + * Avoids pulling an image dependency into the suite for one assertion. + */ +function jpegSize(buf: Buffer): { width: number; height: number } { + let i = 2; // skip SOI + while (i < buf.length) { + if (buf[i] !== 0xff) { + i++; + continue; + } + const marker = buf[i + 1]; + // SOF0..SOF15, excluding DHT (c4), JPGA (c8) and DAC (cc) + if (marker >= 0xc0 && marker <= 0xcf && marker !== 0xc4 && marker !== 0xc8 && marker !== 0xcc) { + return { height: buf.readUInt16BE(i + 5), width: buf.readUInt16BE(i + 7) }; + } + i += 2 + buf.readUInt16BE(i + 2); + } + throw new Error('no SOF marker found — not a JPEG?'); +} + +test.describe('Upload — EXIF orientation', () => { + test('a rotated photo is upright in the preview and the display derivative', async ({ + guest, + db, + }) => { + const g = await guest('SidewaysShooter'); + + const res = await uploadRaw(g.jwt, readFileSync(EXIF_FIXTURE), { + filename: 'portrait-exif6.jpg', + contentType: 'image/jpeg', + caption: 'hochkant', + }); + expect(res.status).toBe(201); + const { id } = (await res.json()) as { id: string }; + + // Sanity: the SOURCE really is stored landscape with the tag, otherwise this test + // could pass against a pipeline that does nothing. + const source = jpegSize(readFileSync(EXIF_FIXTURE)); + expect(source.width).toBeGreaterThan(source.height); + + await expect + .poll(() => db.compressionStatus(id), { timeout: 30_000, intervals: [250] }) + .toBe('done'); + + for (const variant of ['preview', 'display'] as const) { + const r = await fetch(`${BASE}/api/v1/upload/${id}/${variant}`, { + headers: { Authorization: `Bearer ${g.jwt}` }, + }); + expect(r.status, `${variant} must be served`).toBe(200); + const { width, height } = jpegSize(Buffer.from(await r.arrayBuffer())); + expect( + height, + `${variant} must be portrait (${width}x${height}) — EXIF orientation was not applied` + ).toBeGreaterThan(width); + } + }); +}); diff --git a/e2e/specs/02-upload/gallery-path.spec.ts b/e2e/specs/02-upload/gallery-path.spec.ts index 3eaa5d8..6c9b3a6 100644 --- a/e2e/specs/02-upload/gallery-path.spec.ts +++ b/e2e/specs/02-upload/gallery-path.spec.ts @@ -4,6 +4,7 @@ * IndexedDB queue resumption after refresh, and SSE `upload-processed`. */ import { test, expect } from '../../fixtures/test'; +import { skipIfNoIdbBlobs } from '../../helpers/webkit'; import { FeedPage, UploadSheet } from '../../page-objects'; import { SseListener } from '../../helpers/sse-listener'; import { join } from 'node:path'; @@ -49,7 +50,9 @@ test.describe('Upload — gallery path', () => { guest, signIn, db, + browserName, }) => { + skipIfNoIdbBlobs(browserName); // Previously fixme'd: the UI queue never fired a POST. Root cause was NOT a // navigation/blob timing quirk but an IndexedDB upgrade bug — the v1→v2 // `upgrade` callback opened a *new* transaction, which throws during a diff --git a/e2e/specs/02-upload/quota.spec.ts b/e2e/specs/02-upload/quota.spec.ts index f054d7b..34126eb 100644 --- a/e2e/specs/02-upload/quota.spec.ts +++ b/e2e/specs/02-upload/quota.spec.ts @@ -56,14 +56,21 @@ function upload(jwt: string, name: string) { /** * Pick a `quota_tolerance` that makes the per-user ceiling land on `targetBytes`. * limit = floor(free_disk * tolerance / max(active, 1)) ⇒ tolerance = target * active / free. + * + * `staffJwt` reads the calibration inputs, `jwt` is the guest the limit is being aimed at. + * They must be different tokens: `free_disk_bytes` and `active_uploaders` are raw server + * telemetry and `/me/quota` zeroes both for non-staff (handlers/me.rs — "must never reach a + * guest"). Calibrating off the guest's own response divides by zero and yields a NaN + * tolerance, which is what silently broke this whole describe block. */ async function setLimitTo( api: any, adminToken: string, + staffJwt: string, jwt: string, targetBytes: number ): Promise { - const q = await quotaOf(jwt); + const q = await quotaOf(staffJwt); expect( q.free_disk_bytes, 'the disk must be readable, else quota fails OPEN and proves nothing' @@ -72,6 +79,7 @@ async function setLimitTo( const tolerance = (targetBytes * active) / (q.free_disk_bytes as number); await api.patchConfig(adminToken, { quota_tolerance: tolerance.toExponential(12) }); + // Read back through the GUEST, whose ceiling is the one under test. const after = await quotaOf(jwt); expect(after.enabled).toBe(true); return after.limit_bytes as number; @@ -89,10 +97,11 @@ test.describe('Upload — storage quota enforcement', () => { api, adminToken, guest, + host, }) => { const g = await guest('QuotaOver'); // Ceiling below one file: the very first upload must be refused. - const limit = await setLimitTo(api, adminToken, g.jwt, Math.floor(SIZE / 2)); + const limit = await setLimitTo(api, adminToken, host.jwt, g.jwt, Math.floor(SIZE / 2)); expect(limit).toBeLessThan(SIZE); const res = await upload(g.jwt, 'too-big.jpg'); @@ -111,9 +120,10 @@ test.describe('Upload — storage quota enforcement', () => { api, adminToken, guest, + host, }) => { const g = await guest('QuotaUnder'); - await setLimitTo(api, adminToken, g.jwt, SIZE * 4); + await setLimitTo(api, adminToken, host.jwt, g.jwt, SIZE * 4); expect((await upload(g.jwt, 'fine.jpg')).status).toBe(201); expect((await quotaOf(g.jwt)).used_bytes).toBe(SIZE); @@ -123,11 +133,12 @@ test.describe('Upload — storage quota enforcement', () => { api, adminToken, guest, + host, }) => { const g = await guest('QuotaRacer'); // Room for exactly ONE file. - const limit = await setLimitTo(api, adminToken, g.jwt, Math.floor(SIZE * 1.5)); + const limit = await setLimitTo(api, adminToken, host.jwt, g.jwt, Math.floor(SIZE * 1.5)); expect(limit).toBeGreaterThanOrEqual(SIZE); expect(limit).toBeLessThan(SIZE * 2); @@ -170,10 +181,11 @@ test.describe('Upload — storage quota enforcement', () => { api, adminToken, guest, + host, }) => { // Zero test hits before this — and it is the source of the "X von Y MB genutzt" widget. const g = await guest('QuotaWidget'); - await setLimitTo(api, adminToken, g.jwt, SIZE * 10); + await setLimitTo(api, adminToken, host.jwt, g.jwt, SIZE * 10); const before = await quotaOf(g.jwt); expect(before.enabled).toBe(true); diff --git a/e2e/specs/02-upload/rejection-visible.spec.ts b/e2e/specs/02-upload/rejection-visible.spec.ts new file mode 100644 index 0000000..39a51ac --- /dev/null +++ b/e2e/specs/02-upload/rejection-visible.spec.ts @@ -0,0 +1,87 @@ +/** + * Regression guard — a rejected upload must tell the user something. + * + * `UploadQueue.svelte` was 162 lines of complete, working UI — the only renderer of an + * item's error text, the only "Erneut" retry button, the only rate-limit countdown — and it + * was never imported anywhere, so `retryItem`, `removeItem` and `clearCompleted` were all + * unreachable at runtime. On a terminal rejection the store dropped the blob, wrote a clear + * German reason into `entry.error` with the comment "so the UI shows a clear reason", and + * there was no such UI. + * + * Meanwhile the FAB badge counted only pending/uploading, so a rejected photo decremented it + * exactly as if it had succeeded. Net effect: the photo silently vanished — no toast, no + * queue row, no error text, and it never appeared in the feed. + */ +import { test, expect } from '../../fixtures/test'; +import { skipIfNoIdbBlobs } from '../../helpers/webkit'; +import { FeedPage, UploadSheet } from '../../page-objects'; +import { join } from 'node:path'; + +const SAMPLE_JPG = join(process.cwd(), 'fixtures', 'media', 'sample.jpg'); + +test.describe('Upload — a rejected upload is surfaced', () => { + test('a terminally rejected upload toasts, and stays visible in the queue', async ({ + page, + api, + host, + guest, + signIn, + browserName, + }) => { + skipIfNoIdbBlobs(browserName); + const g = await guest('RejectedUploader'); + await signIn(page, g); + + const feed = new FeedPage(page); + const sheet = new UploadSheet(page); + await feed.openUploadSheet(); + await sheet.stageFiles([SAMPLE_JPG]); + await sheet.captionInput.waitFor({ state: 'visible', timeout: 10_000 }); + + // Ban the uploader between staging and sending, so the POST comes back 403 — a + // terminal 4xx the server will keep rejecting, which is the path that purges the blob. + await api.banUser(host.jwt, g.userId); + + await sheet.submit(); + + // 1. The user is told, wherever they are (the flow lands them on /feed). + const toast = page.getByRole('region', { name: 'Benachrichtigungen' }); + await expect(toast).toContainText(/sample\.jpg/i, { timeout: 15_000 }); + + // 2. The queue row survives with its reason and is reachable on /upload — this is what + // the orphaned component made impossible. + await page.goto('/upload'); + const queue = page.getByText('Upload-Warteschlange'); + await expect(queue, 'the upload queue must be rendered somewhere').toBeVisible({ + timeout: 10_000, + }); + // Both the status chip ("Gesperrt") and the server's reason ("Du bist gesperrt.") must + // render — the reason is the part that had no UI at all before. + await expect(page.getByText('Gesperrt', { exact: true })).toBeVisible(); + await expect(page.getByText('Du bist gesperrt.')).toBeVisible(); + + // 3. The badge must not read as success. It counted only pending/uploading before, so a + // rejected item dropped it to 0 — indistinguishable from a completed upload. + await expect + .poll( + () => + page.evaluate(async () => { + return new Promise((resolve, reject) => { + const req = indexedDB.open('eventsnap-uploads', 3); + req.onerror = () => reject(req.error); + req.onsuccess = () => { + const tx = req.result.transaction('queue', 'readonly'); + const all = tx.objectStore('queue').getAll(); + all.onsuccess = () => + resolve( + all.result.filter((r: { status: string }) => r.status === 'blocked').length + ); + all.onerror = () => reject(all.error); + }; + }); + }), + { timeout: 10_000 } + ) + .toBe(1); + }); +}); diff --git a/e2e/specs/04-host/moderation-ui.spec.ts b/e2e/specs/04-host/moderation-ui.spec.ts new file mode 100644 index 0000000..33a7cd0 --- /dev/null +++ b/e2e/specs/04-host/moderation-ui.spec.ts @@ -0,0 +1,149 @@ +/** + * Regression guard — a host must be able to remove a guest's content FROM THE UI. + * + * `DELETE /host/upload/{id}` and `DELETE /host/comment/{id}` were fully implemented, + * transactional, SSE-broadcasting, audit-logged — and had zero frontend callers. The feed's + * context sheet offered "Löschen" only when `target.user_id === myUserId`, so the only lever + * a host actually had against an unwanted photo was banning the uploader. That is both + * disproportionate and ineffective: a ban doesn't retract what was already posted, and + * because the ban check runs BEFORE the ownership check on the guest delete route, banning + * the author makes their abusive comment permanently undeletable by them too. + * + * The API side was already covered (04-host/moderation). What was missing is the wiring, + * so these tests drive the real UI. + */ +import { test, expect } from '../../fixtures/test'; +import { seedUpload, seedComment } from '../../helpers/seed'; +import { BASE } from '../../helpers/env'; + +test.describe('Host — moderation from the UI', () => { + test("a host removes a guest's photo via the feed context sheet", async ({ + page, + host, + guest, + signIn, + }) => { + const g = await guest('PhotoOffender'); + const uploadId = await seedUpload(g.jwt); + + await signIn(page, host); + await page.goto('/feed'); + + const card = page.locator('article').filter({ hasText: g.displayName }).first(); + await expect(card).toBeVisible({ timeout: 15_000 }); + + await card.getByRole('button', { name: 'Mehr Aktionen' }).click(); + const remove = page.getByRole('button', { name: /beitrag entfernen/i }); + await expect(remove, 'a host must be offered a removal action on a guest post').toBeVisible(); + await remove.click(); + + const sheet = page.getByTestId('confirm-sheet'); + await expect(sheet).toBeVisible(); + // Moderation copy, not "delete my post" copy. + await expect(sheet).toContainText(/beitrag entfernen/i); + await page.getByTestId('confirm-sheet-confirm').click(); + + await expect(card).not.toBeVisible({ timeout: 10_000 }); + + // And it is really gone server-side, not just dropped from the local list. + const res = await fetch(`${BASE}/api/v1/feed`, { + headers: { Authorization: `Bearer ${host.jwt}` }, + }); + const body = await res.json(); + expect(body.uploads.some((u: { id: string }) => u.id === uploadId)).toBe(false); + }); + + test('a guest is NOT offered any delete action on someone else’s post', async ({ + page, + guest, + signIn, + }) => { + // The mirror that makes the test above meaningful: if this affordance rendered for + // everyone, the host test would still pass on a build that shipped moderation to guests. + const author = await guest('SomeAuthor'); + await seedUpload(author.jwt); + const viewer = await guest('NosyViewer'); + + await signIn(page, viewer); + await page.goto('/feed'); + + const card = page.locator('article').filter({ hasText: author.displayName }).first(); + await expect(card).toBeVisible({ timeout: 15_000 }); + await card.getByRole('button', { name: 'Mehr Aktionen' }).click(); + + await expect(page.getByRole('button', { name: /beitrag entfernen/i })).toHaveCount(0); + await expect(page.getByRole('button', { name: /^löschen$/i })).toHaveCount(0); + }); + + test('a promoted guest gets host powers without signing out and back in', async ({ + page, + api, + adminToken, + guest, + signIn, + }) => { + // The JWT is never reissued — the backend slides the session row forward and treats the + // DB row as authoritative. So the token of a promoted guest still claims `role: guest` + // for up to 30 days. The UI read that frozen claim, which meant a guest promoted at the + // party saw no Host-Dashboard and no moderation actions until they signed out and back + // in — while `/me/context` had been handing the client the real role all along. + const g = await guest('LatePromotion'); + await signIn(page, g); + + await page.goto('/account'); + await expect(page.getByRole('link', { name: /host-dashboard/i })).toHaveCount(0); + + // Promote mid-session. The token in localStorage is deliberately NOT refreshed. + await api.setRole(adminToken, g.userId, 'host'); + const claim = JSON.parse(Buffer.from(g.jwt.split('.')[1], 'base64').toString()); + expect( + claim.role, + 'the token must still carry the stale claim for this to prove anything' + ).toBe('guest'); + + await page.reload(); + await expect( + page.getByRole('link', { name: /host-dashboard/i }), + 'the live role from /me/context must win over the frozen JWT claim' + ).toBeVisible({ timeout: 10_000 }); + }); + + test('a host can remove the comment of a guest they have already banned', async ({ + page, + api, + host, + guest, + signIn, + }) => { + // The deadlock this closes. Ban first, exactly as a host would react to abuse: from then + // on the author gets 403 on their own delete, so if the host has no removal affordance + // the comment is stuck on screen forever. + // The photo belongs to an innocent third party — a ban hides the banned user's OWN + // uploads, so if the comment sat on their own photo the whole card would vanish and + // there would be nothing left to moderate. + const victim = await guest('PhotoOwner'); + const uploadId = await seedUpload(victim.jwt); + const author = await guest('CommentOffender'); + const commentId = await seedComment(author.jwt, uploadId, 'unangebrachter Kommentar'); + await api.banUser(host.jwt, author.userId); + + // Confirm the deadlock really exists — the author cannot retract it themselves. + const selfDelete = await fetch(`${BASE}/api/v1/comment/${commentId}`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${author.jwt}` }, + }); + expect(selfDelete.status, 'a banned author is blocked from their own delete').toBe(403); + + await signIn(page, host); + await page.goto('/feed'); + + const card = page.locator('article').filter({ hasText: victim.displayName }).first(); + await expect(card).toBeVisible({ timeout: 15_000 }); + await card.getByRole('button', { name: 'Bild vergrößern' }).click(); + + const comment = page.getByText('unangebrachter Kommentar'); + await expect(comment).toBeVisible({ timeout: 10_000 }); + await page.getByRole('button', { name: 'Kommentar entfernen' }).first().click(); + await expect(comment).toHaveCount(0, { timeout: 10_000 }); + }); +}); diff --git a/e2e/specs/06-export/download-iframe.spec.ts b/e2e/specs/06-export/download-iframe.spec.ts new file mode 100644 index 0000000..5fa7d47 --- /dev/null +++ b/e2e/specs/06-export/download-iframe.spec.ts @@ -0,0 +1,103 @@ +/** + * Regression guard — the keepsake download must actually download, in WebKit. + * + * The bug this exists to catch: `/export` streams the archive by pointing a HIDDEN, + * SAME-ORIGIN iframe at `/api/v1/export/zip` (deliberately — a top-level navigation to a + * 404/429 would unload the PWA). Caddy stamped a site-wide `X-Frame-Options: DENY` that + * also covered `/api/*`. Blink hands a `Content-Disposition: attachment` response to the + * download manager at the network layer, so Chromium never noticed; WebKit enforces XFO on + * the frame navigation FIRST and aborts the load. Result: on iOS Safari — the app's primary + * platform — tapping Download did nothing, silently, with no error anywhere. + * + * Why the old suite was structurally blind to it: + * - `06-export` ran on `chromium-desktop` only (webkit-iphone's testMatch excluded it), + * - and no test in the entire suite ever CLICKED a download button; every archive + * assertion used Node `fetch`, which has no frame and therefore no XFO enforcement. + * + * So this spec must keep both properties to be worth anything: a real click, in WebKit. + */ +import { test, expect } from '../../fixtures/test'; +import { ExportPage } from '../../page-objects'; +import { BASE } from '../../helpers/env'; + +const SLUG = 'e2e-test-event'; + +function post(path: string, jwt: string) { + return fetch(BASE + path, { method: 'POST', headers: { Authorization: `Bearer ${jwt}` } }); +} + +/** The real export job runs image processing; give it head-room over the tiny fixtures. */ +async function releaseAndWait(jwt: string) { + expect((await post('/api/v1/host/gallery/release', jwt)).status).toBe(204); + await expect + .poll( + async () => { + const res = await fetch(BASE + '/api/v1/export/status', { + headers: { Authorization: `Bearer ${jwt}` }, + }); + const s = await res.json(); + return s.released === true && s.zip?.status === 'done'; + }, + { timeout: 60_000, intervals: [500] } + ) + .toBe(true); +} + +test.describe('Export — the download actually fires in the browser', () => { + test.slow(); + + test('clicking Download triggers a real download event', async ({ page, host, signIn, db }) => { + await db.setExportReleased(SLUG, false); + await releaseAndWait(host.jwt); + + await signIn(page, host); + const exportPage = new ExportPage(page); + await exportPage.goto(); + await expect(exportPage.zipDownloadButton).toBeEnabled({ timeout: 10_000 }); + + // Capture the frame-level refusal that XFO produces, so a failure reports the CAUSE + // rather than just a timeout. WebKit logs "Refused to display ... in a frame because it + // set 'X-Frame-Options'"; Chromium logs nothing here, which is the whole problem. + const refusals: string[] = []; + page.on('console', (m) => { + if (/X-Frame-Options|Refused to display/i.test(m.text())) refusals.push(m.text()); + }); + + const downloadPromise = page.waitForEvent('download', { timeout: 30_000 }); + await exportPage.zipDownloadButton.click(); + + const download = await downloadPromise.catch((err) => { + throw new Error( + `No download event fired after clicking the ZIP button.` + + (refusals.length + ? ` The browser refused the iframe navigation: ${refusals.join(' | ')}` + : ' No X-Frame-Options refusal was logged; check the ticket/readiness path.') + + `\n${err}` + ); + }); + + expect(download.suggestedFilename()).toMatch(/\.zip$/i); + // The stream must produce real bytes, not a zero-length placeholder. + const path = await download.path(); + expect(path).toBeTruthy(); + expect(refusals, 'no frame should have been refused').toEqual([]); + }); + + test('the export endpoints are framable same-origin; everything else stays DENY', async () => { + // Locks the Caddy carve-out itself, independently of any browser. Cheap, and it fails + // loudly at the exact layer that regressed if someone reinstates a blanket DENY. + for (const path of ['/api/v1/export/zip', '/api/v1/export/html']) { + const res = await fetch(BASE + path); + expect(res.headers.get('x-frame-options')?.toUpperCase(), `${path} must be framable`).toBe( + 'SAMEORIGIN' + ); + } + + for (const path of ['/', '/api/v1/feed', '/api/v1/event']) { + const res = await fetch(BASE + path); + expect(res.headers.get('x-frame-options')?.toUpperCase(), `${path} must stay DENY`).toBe( + 'DENY' + ); + } + }); +}); diff --git a/e2e/specs/07-adversarial/ddos.spec.ts b/e2e/specs/07-adversarial/ddos.spec.ts index 18fc752..c27b18c 100644 --- a/e2e/specs/07-adversarial/ddos.spec.ts +++ b/e2e/specs/07-adversarial/ddos.spec.ts @@ -15,7 +15,15 @@ test.describe('Adversarial — small-scale abuse', () => { await api.patchConfig(adminToken, { rate_limits_enabled: 'true', join_rate_enabled: 'true' }); }); - test('20 parallel /join from one IP — rate limiter catches the excess', async () => { + test('a /join flood from one IP is caught by the per-IP ceiling', async ({ api, adminToken }) => { + // This used to assert that 20 joins from one IP produced 429s under a 5/min per-IP + // bucket. That "protection" was the bug: at a venue every guest shares one public IP, + // so it turned real arriving guests away (see 01-auth/rate-limit-shared-nat). The + // anti-spam bucket is now per (ip, name); what remains per-IP is a loose ceiling whose + // job is only to bound raw volume. Squeeze the ceiling so a flood is reproducible here + // without firing 60+ requests. + await api.patchConfig(adminToken, { join_ip_rate_per_min: '5' }); + const requests = Array.from({ length: 20 }, (_, i) => fetch(`${BASE}/api/v1/join`, { method: 'POST', @@ -24,7 +32,7 @@ test.describe('Adversarial — small-scale abuse', () => { }) ); const statuses = (await Promise.all(requests)).map((r) => r.status); - // 5/min limit → at least some should be 429. + // Ceiling of 5 → the excess must be shed. expect(statuses.filter((s) => s === 429).length).toBeGreaterThan(0); // Server stays up — at least one succeeded. expect(statuses.some((s) => s === 201 || s === 409)).toBe(true); diff --git a/e2e/specs/07-adversarial/media-gating.spec.ts b/e2e/specs/07-adversarial/media-gating.spec.ts index 29f1f16..d9b19c7 100644 --- a/e2e/specs/07-adversarial/media-gating.spec.ts +++ b/e2e/specs/07-adversarial/media-gating.spec.ts @@ -45,6 +45,23 @@ test.describe('Media gating — moderation revokes preview access (F2)', () => { const direct = await fetch(`${BASE}/media/previews/${id}.jpg`); expect(direct.status, 'direct /media/previews must be blocked').toBe(404); + // …and it must stay blocked under percent-encoding. The block used to be four + // `nest_service("/media/previews", 404)` route matches sitting above a `/media` + // ServeDir. axum routes on the RAW path while ServeDir percent-decodes afterwards, so + // ONE escaped byte (`%70` = `p`) missed every blocker, fell through to the ServeDir, + // and was decoded back to `previews/` on disk — serving the bytes unauthenticated. + // Asserting only the literal spelling is what let that sit here undetected. + for (const variant of [ + `/media/%70reviews/${id}.jpg`, // p + `/media/p%72eviews/${id}.jpg`, // r — any position works + `/media/%64isplays/${id}.jpg`, // d + `/media/%74humbnails/${id}.jpg`, // t + `/media/%6Friginals/${id}.jpg`, // o + ]) { + const res = await fetch(`${BASE}${variant}`, { redirect: 'manual' }); + expect(res.status, `${variant} must not bypass the media block`).toBe(404); + } + // Host deletes the upload → the preview must stop being served. const del = await fetch(`${BASE}/api/v1/host/upload/${id}`, { method: 'DELETE', diff --git a/frontend/src/lib/components/LightboxModal.svelte b/frontend/src/lib/components/LightboxModal.svelte index fa1d159..8c76170 100644 --- a/frontend/src/lib/components/LightboxModal.svelte +++ b/frontend/src/lib/components/LightboxModal.svelte @@ -4,6 +4,7 @@ import { api } from '$lib/api'; import { onSseEvent } from '$lib/sse'; import { getUserId } from '$lib/auth'; + import { isStaff } from '$lib/role-store'; import { dataMode, pickMediaUrl } from '$lib/data-mode-store'; import { doubletap } from '$lib/actions/doubletap'; import { focusTrap } from '$lib/actions/focus-trap'; @@ -114,9 +115,15 @@ } } - async function deleteComment(id: string) { + /** + * `asHost` routes to the moderation endpoint. The guest route only ever deletes the + * caller's OWN comment, and it refuses a banned author outright — so without this a + * host who banned an abusive guest was left with the abuse still on screen and no way + * to remove it, since the ban itself blocks the author's own delete. + */ + async function deleteComment(id: string, asHost: boolean) { try { - await api.delete(`/comment/${id}`); + await api.delete(asHost ? `/host/comment/${id}` : `/comment/${id}`); comments = comments.filter((c) => c.id !== id); } catch (e) { toastError(e); @@ -246,11 +253,11 @@ {formatTime(comment.created_at)} - {#if comment.user_id === userId} + {#if comment.user_id === userId || $isStaff}