diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml new file mode 100644 index 0000000..4d38500 --- /dev/null +++ b/.github/workflows/checks.yml @@ -0,0 +1,113 @@ +# The checks that were only ever running on a laptop. +# +# Before this file, CI ran Playwright (chromium-desktop) and the dependency audit — and nothing +# else. `cargo test` (40 tests), the frontend vitest suite (5 files, including the offline +# upload-queue and auth-token logic), svelte-check, and the e2e typecheck were all green on a +# developer's machine and gated NOTHING. A check that only ever runs locally is not running. +# +# In .github/workflows/ because Gitea Actions scans it too (see audit.yml). Rust is installed +# explicitly: the common Gitea runner images ship Node and Docker, not cargo. +name: Checks + +on: + pull_request: + push: + branches: [main] + +jobs: + backend: + name: Backend — cargo test + clippy + fmt + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v4 + + - name: Install Rust toolchain + run: | + if ! command -v cargo > /dev/null; then + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile minimal + echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" + fi + rustup component add clippy rustfmt + + - uses: actions/cache@v4 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + backend/target + key: cargo-${{ hashFiles('backend/Cargo.lock') }} + restore-keys: cargo- + + # SQLx runs its queries against a live database at TEST time (see backend/tests/), so the + # DB-backed tests need one. The pure unit tests don't care, but starting it unconditionally + # keeps the job simple and honest about what it covers. + - name: Start Postgres + run: | + docker run -d --name ci-pg -p 5432:5432 \ + -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=eventsnap_ci \ + postgres:16-alpine + for _ in $(seq 1 30); do + docker exec ci-pg pg_isready -U postgres > /dev/null 2>&1 && break + sleep 1 + done + + - name: Test + working-directory: ./backend + env: + DATABASE_URL: postgres://postgres:postgres@localhost:5432/eventsnap_ci + # `#[sqlx::test]` creates a fresh database per test and opens its own pool; run at unbounded + # parallelism against a default `max_connections=100` Postgres, a full suite can exhaust the + # server's connection slots and fail with PoolTimedOut — a pure infra flake, not a real + # failure. Cap the concurrency so the gate stays trustworthy on a loaded runner. + run: cargo test --all-features -- --test-threads=8 + + - name: Clippy + working-directory: ./backend + run: cargo clippy --all-targets -- -D warnings + + # NOTE: `cargo fmt --check` is deliberately NOT gated. The tree has never been rustfmt'd, so + # turning it on means a 112-file mechanical reformat that would bury every real diff under it. + # Formatting is not a correctness gate; do that cleanup on its own, then add the check here. + + frontend: + name: Frontend — vitest + svelte-check + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: '22' + cache: 'npm' + cache-dependency-path: 'frontend/package-lock.json' + + - name: Install deps + working-directory: ./frontend + run: npm ci || npm install + + - name: Unit tests + working-directory: ./frontend + run: npm run test:unit + + - name: svelte-check + working-directory: ./frontend + run: npx svelte-check --threshold error + + e2e-typecheck: + name: E2E — typecheck + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: '22' + - name: Install deps + working-directory: ./e2e + run: npm install + # Playwright TRANSPILES specs without typechecking them, so a type error in a spec is + # invisible until the assertion it guards silently does the wrong thing at runtime. + - name: tsc --noEmit + working-directory: ./e2e + run: npx tsc --noEmit diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 21ce556..eadda0b 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -46,6 +46,14 @@ jobs: working-directory: ./e2e run: npm run test:e2e -- --project=chromium-desktop + # 09-mobile is `testIgnore`d on chromium-desktop (it needs hasTouch + a phone viewport), so + # running only that project left 22 tests — focus traps, touch targets, safe-area insets, + # viewport reflow, upload-cancel — never executing in CI. On a phone-first event app, where + # essentially every real guest is on a phone, that was the wrong half of the suite to skip. + - name: Run E2E tests (mobile) + working-directory: ./e2e + run: npm run test:e2e -- --project=chromium-mobile + - name: Upload Playwright report if: failure() uses: actions/upload-artifact@v4 diff --git a/README.md b/README.md index 0300098..f4f1751 100644 --- a/README.md +++ b/README.md @@ -182,6 +182,28 @@ The `/media` volume holds originals, previews, thumbnails, exports, and DB backu --- +## Running the backend test suite + +```bash +cd backend + +# The DB-backed integration tests (backend/tests/) need a live Postgres. `#[sqlx::test]` creates a +# throwaway database per test and runs backend/migrations/ into it — it does NOT touch this one's data. +docker run -d --name eventsnap-test-pg -p 55433:5432 \ + -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=eventsnap postgres:16-alpine + +export DATABASE_URL=postgres://postgres:postgres@localhost:55433/eventsnap +cargo test # 44 unit + 12 DB-backed +cargo clippy --all-targets -- -D warnings +``` + +**`cargo test` requires `DATABASE_URL`** — without it the integration tests panic rather than skip. +That is deliberate. The riskiest code in this repo is SQL (the export epoch state machine, the +atomic quota increment, the `FOR SHARE` upload lock), and for a long time *not one line of it* was +executed by `cargo test` — every backend test was a pure-function test, so the tests clustered +tightly around the code that could not break and stopped exactly where it started to. Tests that +silently skip when the database is absent recreate that hole; they were meant to be a gate. + ## Running the E2E test suite Playwright-based end-to-end tests live in [`e2e/`](e2e/). They spin up an isolated docker-compose stack (Postgres on `:55432`, Caddy on `:3101`) and exercise the SvelteKit frontend against the real Rust backend with rate limits disabled. @@ -198,7 +220,14 @@ npm run stack:down # tear it down See [`e2e/README.md`](e2e/README.md) for the full UA matrix, Samsung Internet escalation tiers, and the Phase 2/3 roadmap. -CI runs this on every PR — see [`.github/workflows/e2e.yml`](.github/workflows/e2e.yml). +CI runs this on every PR — see [`.github/workflows/e2e.yml`](.github/workflows/e2e.yml) (desktop **and** +mobile projects), plus [`checks.yml`](.github/workflows/checks.yml) for `cargo test`/clippy, the frontend +unit tests, svelte-check and the e2e typecheck, and [`audit.yml`](.github/workflows/audit.yml) for +dependency advisories. + +**Playwright runs with `retries: 0`, including in CI.** This repo's real bugs are races, and from the +outside a race is indistinguishable from a flake — so a retry silently resolves that ambiguity in +favour of "flake" every time. A flake here is a bug report; treat it as one. --- diff --git a/backend/src/error.rs b/backend/src/error.rs index 14127d4..353aa36 100644 --- a/backend/src/error.rs +++ b/backend/src/error.rs @@ -75,10 +75,10 @@ impl IntoResponse for AppError { } let mut resp = (status, axum::Json(body)).into_response(); - if let Some(secs) = retry_after_secs { - if let Ok(val) = axum::http::HeaderValue::from_str(&secs.to_string()) { - resp.headers_mut().insert(axum::http::header::RETRY_AFTER, val); - } + if let Some(secs) = retry_after_secs + && let Ok(val) = axum::http::HeaderValue::from_str(&secs.to_string()) + { + resp.headers_mut().insert(axum::http::header::RETRY_AFTER, val); } resp } diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index 773e1b0..bcc424a 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -98,6 +98,10 @@ pub async fn get_config( Ok(Json(rows.into_iter().collect())) } +/// Documents the wire shape of `PATCH /admin/config` (a flat `{key: value}` object). +/// `patch_config` extracts the `HashMap` directly rather than going through this newtype, so it is +/// never constructed in Rust — it stays as the serde-derived description of the request body. +#[allow(dead_code)] #[derive(Deserialize)] pub struct PatchConfigRequest(pub HashMap); diff --git a/backend/src/handlers/test_admin.rs b/backend/src/handlers/test_admin.rs index ba330a5..b6c40ce 100644 --- a/backend/src/handlers/test_admin.rs +++ b/backend/src/handlers/test_admin.rs @@ -85,6 +85,19 @@ pub async fn truncate_all( // could serve the previous test's toggles. state.config_cache.invalidate(); + // The other two in-memory singletons that TRUNCATE used to leave standing. + // + // `disk_cache` holds a free-space reading for up to its TTL. TRUNCATE has just deleted every + // uploaded file, which materially changes free space — so without this the next test can + // compute a storage quota from the PREVIOUS test's disk. That was harmless only while quotas + // were globally disabled in e2e (they no longer are: see specs/02-upload/quota.spec.ts, which + // steers the per-user limit off `free_disk_bytes`), i.e. two holes were masking each other. + state.disk_cache.invalidate(); + + // `sse_tickets` maps a ticket to a session token hash. TRUNCATE deletes the sessions, so every + // surviving ticket is a dangling reference to a user that no longer exists. + state.sse_tickets.clear(); + Ok(StatusCode::NO_CONTENT) } diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 552ddd5..0edcc5a 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -168,14 +168,14 @@ pub async fn upload( // Validate caption length. Counted in chars (code points) to match the // "Zeichen" wording in the error message — `.len()` would be bytes and // reject perfectly valid German/emoji captions early. - if let Some(ref cap) = caption { - if cap.chars().count() > MAX_CAPTION_LENGTH { - let _ = tokio::fs::remove_file(&temp_abs).await; - return Err(AppError::BadRequest(format!( - "Beschreibung ist zu lang. Maximum: {} Zeichen.", - MAX_CAPTION_LENGTH - ))); - } + if let Some(ref cap) = caption + && cap.chars().count() > MAX_CAPTION_LENGTH + { + let _ = tokio::fs::remove_file(&temp_abs).await; + return Err(AppError::BadRequest(format!( + "Beschreibung ist zu lang. Maximum: {} Zeichen.", + MAX_CAPTION_LENGTH + ))); } // Determine the file type from its magic bytes and require it to be on the @@ -555,6 +555,9 @@ pub struct QuotaEstimate { pub limit_bytes: Option, pub active_uploaders: i64, pub free_disk_bytes: i64, + /// The tolerance factor the limit above was computed with. Carried on the snapshot so the + /// number is self-describing; no caller reads it back today. + #[allow(dead_code)] pub tolerance: f64, } diff --git a/backend/src/models/comment.rs b/backend/src/models/comment.rs index 7fafbcc..b7de8ff 100644 --- a/backend/src/models/comment.rs +++ b/backend/src/models/comment.rs @@ -3,6 +3,10 @@ use serde::Serialize; use sqlx::PgPool; use uuid::Uuid; +// Row shape for `comment`: every field is populated by sqlx from `SELECT *` / `RETURNING *`. +// `deleted_at` is not read in Rust today (the soft-delete filter lives in SQL), but it is part of +// the row and stays here so the struct keeps mirroring the table. +#[allow(dead_code)] #[derive(Debug, sqlx::FromRow)] pub struct Comment { pub id: Uuid, @@ -87,16 +91,8 @@ impl Comment { .await } - pub async fn soft_delete(pool: &PgPool, id: Uuid) -> Result<(), sqlx::Error> { - sqlx::query("UPDATE comment SET deleted_at = NOW() WHERE id = $1") - .bind(id) - .execute(pool) - .await?; - Ok(()) - } - - /// Event-scoped variant of [`Self::soft_delete`]. Returns `false` if the - /// comment doesn't exist or belongs to a different event. + /// Event-scoped soft delete. Returns `false` if the comment doesn't exist or belongs to a + /// different event. /// Executor-generic so the delete and the keepsake regeneration can share one transaction /// (see `Upload::soft_delete_in_event` for why that must be atomic). pub async fn soft_delete_in_event( diff --git a/backend/src/models/event.rs b/backend/src/models/event.rs index b4f891e..a70476a 100644 --- a/backend/src/models/event.rs +++ b/backend/src/models/event.rs @@ -2,6 +2,11 @@ use chrono::{DateTime, Utc}; use sqlx::PgPool; use uuid::Uuid; +// Row shape for `event`: every field is populated by sqlx from `SELECT *` / `RETURNING *`. Several +// (`slug`, `cover_image_path`, `export_epoch`, `created_at`) are not read through this struct today +// — callers that need them query the column directly — but they are part of the row and stay here so +// the struct keeps mirroring the table. +#[allow(dead_code)] #[derive(Debug, sqlx::FromRow)] pub struct Event { pub id: Uuid, diff --git a/backend/src/models/hashtag.rs b/backend/src/models/hashtag.rs index 7cd13c9..f7f20a2 100644 --- a/backend/src/models/hashtag.rs +++ b/backend/src/models/hashtag.rs @@ -1,6 +1,8 @@ -use sqlx::PgPool; use uuid::Uuid; +// Row shape for `hashtag`, populated by sqlx from `RETURNING *` in `upsert`. Callers only use +// `id` today; `event_id`/`tag` are the rest of the row and stay part of the struct. +#[allow(dead_code)] #[derive(Debug, sqlx::FromRow)] pub struct Hashtag { pub id: Uuid, @@ -61,22 +63,6 @@ impl Hashtag { .await?; Ok(()) } - - pub async fn tags_for_upload( - pool: &PgPool, - upload_id: Uuid, - ) -> Result, sqlx::Error> { - let rows: Vec<(String,)> = sqlx::query_as( - "SELECT h.tag FROM hashtag h - JOIN upload_hashtag uh ON uh.hashtag_id = h.id - WHERE uh.upload_id = $1 - ORDER BY h.tag", - ) - .bind(upload_id) - .fetch_all(pool) - .await?; - Ok(rows.into_iter().map(|r| r.0).collect()) - } } /// Extract `#hashtags` from text (caption or body). Tags are restricted to diff --git a/backend/src/models/session.rs b/backend/src/models/session.rs index 6a2a312..42df793 100644 --- a/backend/src/models/session.rs +++ b/backend/src/models/session.rs @@ -2,6 +2,9 @@ use chrono::{DateTime, Utc}; use sqlx::PgPool; use uuid::Uuid; +// Row shape for `session`, populated by sqlx from `RETURNING *`. Session validation is done in SQL +// (expiry/last-seen predicates), so no field is read in Rust — the struct is the row's shape. +#[allow(dead_code)] #[derive(Debug, sqlx::FromRow)] pub struct Session { pub id: Uuid, diff --git a/backend/src/models/upload.rs b/backend/src/models/upload.rs index 6131743..2312ccc 100644 --- a/backend/src/models/upload.rs +++ b/backend/src/models/upload.rs @@ -3,6 +3,10 @@ use serde::Serialize; use sqlx::PgPool; use uuid::Uuid; +// Row shape for `upload`: every field is populated by sqlx from `RETURNING *` in `create`. Callers +// mostly use `id` and hand the rest to the compression/feed queries, so most fields are never read +// through this struct — they stay here so it keeps mirroring the table. +#[allow(dead_code)] #[derive(Debug, sqlx::FromRow)] pub struct Upload { pub id: Uuid, @@ -75,15 +79,6 @@ impl Upload { .await } - pub async fn find_by_id(pool: &PgPool, id: Uuid) -> Result, sqlx::Error> { - sqlx::query_as::<_, Self>( - "SELECT * FROM upload WHERE id = $1 AND deleted_at IS NULL", - ) - .bind(id) - .fetch_optional(pool) - .await - } - /// Lean lookup for the public media aliases (`get_original`/`get_preview`/ /// `get_thumbnail`): returns ONLY the file paths + mime for a visible upload — /// excluding soft-deleted rows, hidden owners (`uploads_hidden`), and banned owners diff --git a/backend/src/models/user.rs b/backend/src/models/user.rs index bed7fbf..9416bde 100644 --- a/backend/src/models/user.rs +++ b/backend/src/models/user.rs @@ -22,6 +22,10 @@ impl UserRole { } } +// Row shape for `user`: every field is populated by sqlx from `SELECT *` / `RETURNING *`. +// `uploads_hidden`, `failed_pin_attempts` and `created_at` are enforced/updated in SQL rather than +// read in Rust, but they are part of the row and stay here so the struct keeps mirroring the table. +#[allow(dead_code)] #[derive(Debug, sqlx::FromRow)] pub struct User { pub id: Uuid, diff --git a/backend/src/services/disk.rs b/backend/src/services/disk.rs index 32cdac1..d465eec 100644 --- a/backend/src/services/disk.rs +++ b/backend/src/services/disk.rs @@ -34,6 +34,17 @@ impl DiskCache { } } + /// Drop the cached reading so the next `snapshot()` re-measures the filesystem. + /// + /// Used by the e2e TRUNCATE endpoint. Truncating deletes every uploaded file, which materially + /// changes free space — but the cached reading survives for up to the TTL, so the next test can + /// compute a quota from the PREVIOUS test's disk. That matters now that the quota tests steer + /// the per-user limit off `free_disk_bytes`: a stale reading makes the limit wrong and the test + /// flaky, for reasons that have nothing to do with the code under test. + pub fn invalidate(&self) { + *self.inner.write().unwrap() = None; + } + /// Cached `(total, free)` bytes for the filesystem that holds `media_path`. /// /// Returns `None` when the mount can't be resolved — callers MUST treat that as @@ -47,10 +58,11 @@ impl DiskCache { /// caller to ask about a different volume (e.g. the exports volume, which is a separate mount) /// would have silently got the wrong filesystem's free space. pub fn snapshot(&self, path: &Path) -> Option { - if let Some((cached_path, info, at)) = self.inner.read().unwrap().as_ref() { - if cached_path == path && at.elapsed() < TTL { - return Some(*info); - } + if let Some((cached_path, info, at)) = self.inner.read().unwrap().as_ref() + && cached_path == path + && at.elapsed() < TTL + { + return Some(*info); } let info = read_disk_for_path(path)?; *self.inner.write().unwrap() = Some((path.to_path_buf(), info, Instant::now())); diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index dba46bb..57f3763 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -389,6 +389,10 @@ async fn invalidate_missing_files( /// start immediately. pub const REGEN_DEBOUNCE: Duration = Duration::from_secs(20); +// Export worker entry point: every argument is state the spawned worker is BORN with (notably +// `epoch`). Bundling them into a struct would be a pure-refactor risk on the epoch logic for no +// gain, so the arity stands. +#[allow(clippy::too_many_arguments)] pub fn spawn_export_jobs( event_id: Uuid, event_name: String, diff --git a/backend/src/services/jobs.rs b/backend/src/services/jobs.rs deleted file mode 100644 index c41aa86..0000000 --- a/backend/src/services/jobs.rs +++ /dev/null @@ -1,73 +0,0 @@ -//! Shared shape for long-running background work. -//! -//! Today's [`compression`](crate::services::compression) and [`export`](crate::services::export) -//! pipelines each implement their own progress + SSE plumbing. They could converge on the -//! trait sketched here so future jobs (analytics, archival, ...) plug into one progress -//! pipeline. -//! -//! This module is intentionally a *sketch*: the existing services are not yet wired to -//! it. The aim is to (a) document the convention so new jobs follow it, (b) make the -//! refactor mechanical when someone is ready to do it. See `docs/IDEAS.md` — -//! "Maintainability principles" — for the rationale. -//! -//! Example of an eventual implementor: -//! -//! ```ignore -//! struct ZipExport { event_id: Uuid, /* … */ } -//! -//! impl BackgroundJob for ZipExport { -//! fn name(&self) -> &'static str { "zip-export" } -//! async fn run(self, ctx: JobContext) -> Result<()> { -//! for (i, item) in items.iter().enumerate() { -//! ctx.report(percent(i, items.len())).await?; -//! // … write to zip … -//! } -//! Ok(()) -//! } -//! } -//! ``` - -use anyhow::Result; - -/// Handle handed to a running job: reports progress and emits SSE events. -/// -/// Wraps the existing SSE broadcaster and an optional `export_job` row. Implementors -/// don't need to know about `state.sse_tx` directly — they call [`JobContext::report`] -/// and get the same effect. -pub struct JobContext { - pub job_id: Option, - pub event_kind: &'static str, - pub sse_tx: tokio::sync::broadcast::Sender, - pub pool: sqlx::PgPool, -} - -impl JobContext { - /// Update progress (0..=100) and broadcast an SSE tick. Cheap to call often — - /// rate-limit at the call site if a job emits at > 10 Hz. - pub async fn report(&self, percent: u8) -> Result<()> { - if let Some(job_id) = self.job_id { - sqlx::query("UPDATE export_job SET progress_pct = $1 WHERE id = $2") - .bind(percent as i16) - .bind(job_id) - .execute(&self.pool) - .await?; - } - let _ = self.sse_tx.send(crate::state::SseEvent::new( - self.event_kind, - serde_json::json!({ "progress_pct": percent }).to_string(), - )); - Ok(()) - } -} - -/// One unit of work that publishes progress through a [`JobContext`]. -/// -/// `run` consumes `self`; spawn with `tokio::spawn` at the caller. Errors propagate; -/// the caller is responsible for mapping them to `export_job.error_message` or -/// equivalent. Implementors stay small — the trait deliberately has no `cancel` -/// or `pause`; we have not needed those yet. -#[allow(async_fn_in_trait)] -pub trait BackgroundJob: Send + 'static { - fn name(&self) -> &'static str; - async fn run(self, ctx: JobContext) -> Result<()>; -} diff --git a/backend/src/services/mod.rs b/backend/src/services/mod.rs index 8f7eae2..08a97b2 100644 --- a/backend/src/services/mod.rs +++ b/backend/src/services/mod.rs @@ -2,7 +2,6 @@ pub mod compression; pub mod config; pub mod disk; pub mod export; -pub mod jobs; pub mod maintenance; pub mod rate_limiter; pub mod sse_tickets; diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index 91f0d73..d2501f0 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -123,12 +123,58 @@ mod tests { assert!(rl.check("k", 1, w), "the slot should expire once the window passes"); } + /// `retry_after` is not a "some number in range" — it is the time until the oldest slot + /// in the window frees up, and it is surfaced to clients as the backoff they sleep for + /// (see `upload-queue.ts`). Asserting only `(1..=60)` spans the entire reachable domain + /// of a 60s window, so a hardcoded `Err(1)` would satisfy it while telling every client + /// to hammer the server a second later. Pin the actual value. #[test] - fn retry_after_is_between_one_and_window() { + fn retry_after_is_the_remaining_window() { let rl = RateLimiter::new(); - assert!(rl.check_with_retry("k", 1, MIN).is_ok()); - let retry = rl.check_with_retry("k", 1, MIN).unwrap_err(); - assert!((1..=60).contains(&retry), "retry_after {retry} out of range"); + + // The slot was consumed just now, so essentially the whole window remains. + // `as_secs()` truncates the sub-second remainder, so a 30s window reports 29. + let w30 = Duration::from_secs(30); + assert!(rl.check_with_retry("a", 1, w30).is_ok()); + let a = rl.check_with_retry("a", 1, w30).unwrap_err(); + assert_eq!(a, 29, "retry_after must be the remaining window, got {a}"); + + // A different window must yield a different retry_after: no single constant can + // satisfy both this and the assertion above. + let w10 = Duration::from_secs(10); + assert!(rl.check_with_retry("b", 1, w10).is_ok()); + let b = rl.check_with_retry("b", 1, w10).unwrap_err(); + assert_eq!(b, 9, "retry_after must scale with the window, got {b}"); + } + + #[test] + fn retry_after_counts_down_as_the_window_elapses() { + let rl = RateLimiter::new(); + let w = Duration::from_secs(30); + assert!(rl.check_with_retry("k", 1, w).is_ok()); + let first = rl.check_with_retry("k", 1, w).unwrap_err(); + + std::thread::sleep(Duration::from_millis(1200)); + let second = rl.check_with_retry("k", 1, w).unwrap_err(); + + // A client that waits 1.2s must be told to wait ~1.2s less — otherwise the advertised + // backoff is a constant, not a deadline. + let shaved = first - second; + assert!( + (1..=2).contains(&shaved), + "1.2s of waiting must shorten the advertised backoff by ~1s (got {first} then {second})" + ); + } + + #[test] + fn retry_after_floors_at_one_second() { + let rl = RateLimiter::new(); + let w = Duration::from_millis(800); + assert!(rl.check_with_retry("k", 1, w).is_ok()); + let retry = rl.check_with_retry("k", 1, w).unwrap_err(); + // The sub-second remainder truncates to 0; clients must never be told "retry in 0s" + // (that's a busy-loop). The `.max(1)` floor is what prevents it. + assert_eq!(retry, 1, "a sub-second remainder must floor to 1, got {retry}"); } #[test] @@ -140,6 +186,59 @@ mod tests { assert!(rl.check("k", 1, MIN), "clear() must free the window"); } + /// `prune()` is a memory-leak guard: without it a long-lived process keeps one HashMap + /// entry per IP that ever connected. Nothing in the public API observes the map size, so + /// the only way to catch a no-op body (`fn prune(&self) {}`) is to look at the map — the + /// tests module can see the private field. + #[test] + fn prune_drops_keys_whose_windows_have_fully_expired() { + let rl = RateLimiter::new(); + + // A key whose only timestamp is older than the 24h ceiling. We can't sleep for a day, + // so backdate the Instant directly. + let ancient = Instant::now() + .checked_sub(Duration::from_secs(25 * 60 * 60)) + .expect("backdating an Instant by 25h"); + rl.windows + .lock() + .unwrap() + .insert("stale".to_string(), vec![ancient]); + + // ...alongside a key that is still inside its window. + assert!(rl.check("live", 5, MIN)); + assert_eq!(rl.windows.lock().unwrap().len(), 2); + + rl.prune(); + + let map = rl.windows.lock().unwrap(); + assert!( + !map.contains_key("stale"), + "prune() must drop keys whose timestamps have all expired" + ); + assert!( + map.contains_key("live"), + "prune() must keep keys that still have live timestamps" + ); + assert_eq!(map.len(), 1, "exactly one key should survive the prune"); + } + + #[test] + fn prune_does_not_reset_a_live_window() { + // The counterpart to the test above: pruning must reclaim memory, never quota. If + // 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)); + + rl.prune(); + + assert!( + !rl.check("k", 1, MIN), + "prune() must not clear a window that is still active" + ); + } + #[test] fn client_ip_takes_rightmost_forwarded_for_entry() { // The right-most entry is the hop our trusted proxy (Caddy) appended. diff --git a/backend/src/services/sse_tickets.rs b/backend/src/services/sse_tickets.rs index 2695d84..7476ba0 100644 --- a/backend/src/services/sse_tickets.rs +++ b/backend/src/services/sse_tickets.rs @@ -31,6 +31,13 @@ impl SseTicketStore { } } + /// Drop every outstanding ticket. Used by the e2e TRUNCATE endpoint: tickets are bound to a + /// session token hash, and TRUNCATE deletes the sessions out from under them, so anything left + /// here is a dangling reference to a user that no longer exists. + pub fn clear(&self) { + self.inner.lock().unwrap().clear(); + } + /// Mint a new ticket bound to the caller's session (identified by token hash). pub fn issue(&self, token_hash: String) -> String { let ticket = random_ticket(); diff --git a/backend/tests/common/mod.rs b/backend/tests/common/mod.rs new file mode 100644 index 0000000..c0a0a6b --- /dev/null +++ b/backend/tests/common/mod.rs @@ -0,0 +1,257 @@ +//! Shared fixtures for the DB-backed integration tests. +//! +//! Every helper here executes SQL that is **character-for-character identical** to what `src/` +//! actually runs (see the `// SRC:` markers). That is the whole point: a paraphrased query is a +//! query nobody runs, and a test that passes against a paraphrase proves nothing about production. + +#![allow(dead_code)] // each integration-test crate uses a different subset of these helpers + +use sqlx::PgPool; +use uuid::Uuid; + +/// Insert a bare, unreleased event (epoch 0, uploads open). +pub async fn seed_event(pool: &PgPool, slug: &str) -> Uuid { + sqlx::query_scalar("INSERT INTO event (slug, name) VALUES ($1, $2) RETURNING id") + .bind(slug) + .bind("Hochzeit") + .fetch_one(pool) + .await + .expect("seed event") +} + +/// Insert a guest with a zeroed byte total. +pub async fn seed_user(pool: &PgPool, event_id: Uuid, name: &str) -> Uuid { + sqlx::query_scalar( + "INSERT INTO \"user\" (event_id, display_name, recovery_pin_hash) + VALUES ($1, $2, 'x') RETURNING id", + ) + .bind(event_id) + .bind(name) + .fetch_one(pool) + .await + .expect("seed user") +} + +/// SRC: `handlers/host.rs::release_gallery` — the claim + epoch bump, verbatim. +/// Returns the POST-increment epoch, exactly as the handler consumes it. +pub async fn release_gallery(pool: &PgPool, slug: &str) -> Option { + let claimed: Option<(Uuid, String, i64)> = sqlx::query_as( + "UPDATE event + SET export_released_at = NOW(), + uploads_locked_at = COALESCE(uploads_locked_at, NOW()), + export_epoch = export_epoch + 1 + WHERE slug = $1 AND export_released_at IS NULL + RETURNING id, name, export_epoch", + ) + .bind(slug) + .fetch_optional(pool) + .await + .expect("release_gallery"); + + if let Some((event_id, _, epoch)) = claimed { + // The handler arms both jobs in the SAME transaction; for a single-connection fixture the + // sequencing is equivalent. + let mut conn = pool.acquire().await.expect("acquire"); + enqueue_types_at_epoch(&mut conn, event_id, epoch, &["zip", "html"]).await; + Some(epoch) + } else { + None + } +} + +/// SRC: `handlers/host.rs::open_event` — the one statement that retires an entire generation. +/// Returns rows affected. +pub async fn open_event(pool: &PgPool, slug: &str) -> u64 { + sqlx::query( + "UPDATE event + SET uploads_locked_at = NULL, + export_released_at = NULL, + export_epoch = export_epoch + 1 + WHERE slug = $1 AND (uploads_locked_at IS NOT NULL OR export_released_at IS NOT NULL)", + ) + .bind(slug) + .execute(pool) + .await + .expect("open_event") + .rows_affected() +} + +/// SRC: `services/export.rs::enqueue_types_at_epoch` — verbatim upsert. +pub async fn enqueue_types_at_epoch( + conn: &mut sqlx::PgConnection, + event_id: Uuid, + epoch: i64, + types: &[&str], +) { + for export_type in types { + sqlx::query( + "INSERT INTO export_job (event_id, type, status, progress_pct, epoch) + VALUES ($1, $2::export_type, 'pending', 0, $3) + ON CONFLICT (event_id, type) DO UPDATE + SET status = 'pending', progress_pct = 0, file_path = NULL, + error_message = NULL, completed_at = NULL, + epoch = EXCLUDED.epoch + WHERE export_job.status <> 'done' OR export_job.epoch <> EXCLUDED.epoch", + ) + .bind(event_id) + .bind(export_type) + .bind(epoch) + .execute(&mut *conn) + .await + .expect("enqueue_types_at_epoch"); + } +} + +/// SRC: `services/export.rs::claim_job` — verbatim. `true` = we won the generation. +pub async fn claim_job(pool: &PgPool, event_id: Uuid, export_type: &str, epoch: i64) -> bool { + sqlx::query( + "UPDATE export_job SET status = 'running' + WHERE event_id = $1 AND type = $2::export_type + AND epoch = $3 AND status = 'pending'", + ) + .bind(event_id) + .bind(export_type) + .bind(epoch) + .execute(pool) + .await + .expect("claim_job") + .rows_affected() + > 0 +} + +/// SRC: `services/export.rs::finalize_job` — verbatim. This IS the publish step. +pub async fn finalize_job( + pool: &PgPool, + event_id: Uuid, + export_type: &str, + epoch: i64, + file_path: &str, +) -> bool { + sqlx::query( + "UPDATE export_job + SET status = 'done', progress_pct = 100, file_path = $3, completed_at = NOW() + WHERE event_id = $1 AND type = $2::export_type + AND epoch = $4 AND status = 'running'", + ) + .bind(event_id) + .bind(export_type) + .bind(file_path) + .bind(epoch) + .execute(pool) + .await + .expect("finalize_job") + .rows_affected() + > 0 +} + +/// SRC: `services/export.rs::update_progress` — verbatim. Doubles as the worker's liveness check: +/// `false` means "your generation was retired, stop working". +pub async fn update_progress( + pool: &PgPool, + event_id: Uuid, + export_type: &str, + epoch: i64, + pct: i16, +) -> bool { + sqlx::query( + "UPDATE export_job SET progress_pct = $3 + WHERE event_id = $1 AND type = $2::export_type + AND epoch = $4 AND status = 'running'", + ) + .bind(event_id) + .bind(export_type) + .bind(pct) + .bind(epoch) + .execute(pool) + .await + .expect("update_progress") + .rows_affected() + > 0 +} + +/// SRC: `services/export.rs::invalidate_and_arm` — the ViewerOnly ZIP carry-forward, verbatim. +/// Returns `rows_affected() == 1`, which is what the production code branches on. +pub async fn carry_zip_forward(pool: &PgPool, event_id: Uuid, epoch: i64) -> bool { + sqlx::query( + "UPDATE export_job SET epoch = $2 + WHERE event_id = $1 AND type = 'zip'::export_type + AND status = 'done' AND epoch = $2 - 1", + ) + .bind(event_id) + .bind(epoch) + .execute(pool) + .await + .expect("carry_zip_forward") + .rows_affected() + == 1 +} + +/// SRC: `services/export.rs::invalidate_and_arm` — the epoch bump, verbatim. +pub async fn bump_epoch(pool: &PgPool, slug: &str) -> Option<(Uuid, String, i64)> { + sqlx::query_as( + "UPDATE event SET export_epoch = export_epoch + 1 + WHERE slug = $1 AND export_released_at IS NOT NULL + RETURNING id, name, export_epoch", + ) + .bind(slug) + .fetch_optional(pool) + .await + .expect("bump_epoch") +} + +/// The event's authoritative epoch. +pub async fn event_epoch(pool: &PgPool, event_id: Uuid) -> i64 { + sqlx::query_scalar("SELECT export_epoch FROM event WHERE id = $1") + .bind(event_id) + .fetch_one(pool) + .await + .expect("event_epoch") +} + +/// The raw job row, bypassing `export_current` — what the WORKER sees. +pub async fn job_row( + pool: &PgPool, + event_id: Uuid, + export_type: &str, +) -> Option<(String, i64, Option)> { + sqlx::query_as( + "SELECT status::text, epoch, file_path FROM export_job + WHERE event_id = $1 AND type = $2::export_type", + ) + .bind(event_id) + .bind(export_type) + .fetch_optional(pool) + .await + .expect("job_row") +} + +/// Does `export_current` expose this job at all? (The view itself, without the `status` filter — +/// it is what `handlers/admin.rs::export_status` reports to the host UI.) +pub async fn in_export_current(pool: &PgPool, event_id: Uuid, export_type: &str) -> bool { + sqlx::query_scalar::<_, i64>( + "SELECT COUNT(*) FROM export_current + WHERE event_id = $1 AND type = $2::export_type", + ) + .bind(event_id) + .bind(export_type) + .fetch_one(pool) + .await + .expect("in_export_current") + > 0 +} + +/// THE download predicate. SRC: `handlers/admin.rs::download_export` reads exactly this shape — +/// `SELECT c.file_path FROM export_current c WHERE ... AND c.status = 'done'`. If this returns +/// `Some`, a guest can download the keepsake; if `None`, they get a 404. +pub async fn downloadable(pool: &PgPool, event_id: Uuid, export_type: &str) -> Option { + sqlx::query_scalar( + "SELECT c.file_path FROM export_current c + WHERE c.event_id = $1 AND c.type = $2::export_type AND c.status = 'done'", + ) + .bind(event_id) + .bind(export_type) + .fetch_optional(pool) + .await + .expect("downloadable") + .flatten() +} diff --git a/backend/tests/export_epoch.rs b/backend/tests/export_epoch.rs new file mode 100644 index 0000000..3b492e5 --- /dev/null +++ b/backend/tests/export_epoch.rs @@ -0,0 +1,363 @@ +//! DB-backed integration tests for the export epoch state machine (migration 014). +//! +//! These run against a REAL Postgres: `#[sqlx::test]` creates a throwaway database per test and +//! runs `backend/migrations/` into it, so the schema, the enums, the `UNIQUE (event_id, type)` +//! constraint and the `export_current` view are the production ones — not a mock. +//! +//! THE INVARIANT, from migration 014: +//! +//! An export is downloadable IFF +//! event.export_released_at IS NOT NULL +//! AND export_job.epoch = event.export_epoch +//! AND export_job.status = 'done' +//! +//! Readiness is DERIVED (the `export_current` view), never stored. Every test below pins one leg of +//! that invariant with the exact SQL `src/` executes (see `tests/common/mod.rs`). + +mod common; + +use common::*; +use sqlx::PgPool; + +// ───────────────────────────────────────────────────────────────────────────── +// 1. Epoch monotonicity +// ───────────────────────────────────────────────────────────────────────────── + +/// Release, reopen and re-release each bump `export_epoch`, and `RETURNING export_epoch` hands the +/// caller the POST-increment value. +/// +/// PREVENTS: a worker born with the PRE-increment epoch. It would be inert from the instant it +/// started — every one of its writes is `epoch`-guarded, so `claim_job`/`finalize_job` would match +/// nothing, the job row would sit at `pending` 0% forever with no live worker, and the host's +/// download button would spin and then 404. The keepsake would never be built at all. +#[sqlx::test] +async fn release_returns_post_increment_epoch(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + assert_eq!(event_epoch(&pool, event_id).await, 0, "a fresh event starts at epoch 0"); + + let released = release_gallery(&pool, "wedding").await.expect("release claims the event"); + assert_eq!(released, 1, "RETURNING must give the epoch AFTER the +1, not before"); + assert_eq!(event_epoch(&pool, event_id).await, released, "worker's epoch == event's epoch"); + + // The jobs armed by the release carry exactly that epoch — this is what makes the worker's + // guarded writes match. + for t in ["zip", "html"] { + let (status, epoch, _) = job_row(&pool, event_id, t).await.expect("job armed"); + assert_eq!(status, "pending"); + assert_eq!(epoch, released, "{t} job must be armed at the epoch the worker was born with"); + } +} + +/// Epoch is strictly monotonic across the whole release/reopen/re-release cycle, and a second +/// release attempt while already released is rejected WITHOUT bumping. +/// +/// PREVENTS: epoch reuse. If a reopen could return the event to an epoch some old `done` row still +/// carries, a retired keepsake — one that a guest asked to be taken down from — would silently +/// become downloadable again. +#[sqlx::test] +async fn epoch_is_strictly_monotonic_across_reopen(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + + assert_eq!(release_gallery(&pool, "wedding").await, Some(1)); + + // A duplicate release is a no-op (`WHERE export_released_at IS NULL`) and must NOT bump. + assert_eq!(release_gallery(&pool, "wedding").await, None, "already released"); + assert_eq!(event_epoch(&pool, event_id).await, 1, "a rejected release must not move the epoch"); + + // Reopen retires the generation with ONE write. + assert_eq!(open_event(&pool, "wedding").await, 1); + assert_eq!(event_epoch(&pool, event_id).await, 2, "reopen bumps"); + + // And re-releasing bumps again — never back to 1. + assert_eq!(release_gallery(&pool, "wedding").await, Some(3), "re-release bumps again"); + assert_eq!(event_epoch(&pool, event_id).await, 3); +} + +// ───────────────────────────────────────────────────────────────────────────── +// 2. A retired-epoch worker is inert +// ───────────────────────────────────────────────────────────────────────────── + +/// A worker holding a retired epoch cannot write anything anybody can see: once the rows have been +/// re-armed at a newer epoch, its `update_progress` and `finalize_job` both match 0 rows, and +/// `export_current` never exposes its output. +/// +/// PREVENTS: the classic lost race — a slow worker from BEFORE a takedown finishing afterwards and +/// publishing an archive that still contains the photo a guest asked to have removed. "Please take +/// my photo out" is the one request that most needs to reach the keepsake, and the keepsake is the +/// artifact people keep forever. +#[sqlx::test] +async fn retired_epoch_worker_writes_are_no_ops(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let old_epoch = release_gallery(&pool, "wedding").await.unwrap(); + + // Worker A is born at epoch 1 and claims the ZIP. + assert!(claim_job(&pool, event_id, "zip", old_epoch).await, "worker A wins its claim"); + assert!(update_progress(&pool, event_id, "zip", old_epoch, 40).await, "still live at 40%"); + + // ── A takedown lands mid-export: `invalidate_and_arm(Affects::Both)` bumps and re-arms. ── + let (_, _, new_epoch) = bump_epoch(&pool, "wedding").await.expect("bump on a released event"); + assert_eq!(new_epoch, old_epoch + 1); + let mut conn = pool.acquire().await.unwrap(); + enqueue_types_at_epoch(&mut conn, event_id, new_epoch, &["zip", "html"]).await; + drop(conn); + + // Worker A is now INERT BY CONSTRUCTION. Every write is guarded on its own birth epoch. + assert!( + !update_progress(&pool, event_id, "zip", old_epoch, 90).await, + "the liveness check must report `false` so worker A stops grinding through the gallery" + ); + assert!( + !finalize_job(&pool, event_id, "zip", old_epoch, "exports/Gallery.1.zip").await, + "worker A's finalize MUST affect 0 rows — this is the write that would have published a \ + keepsake still containing the taken-down photo" + ); + + // The re-armed row is untouched by the loser: still pending at the LIVE epoch, waiting for the + // fresh worker. (If worker A had won, this row would read `done` at epoch 1.) + let (status, epoch, file_path) = job_row(&pool, event_id, "zip").await.unwrap(); + assert_eq!((status.as_str(), epoch), ("pending", new_epoch)); + assert_eq!(file_path, None, "the loser's file_path must never be recorded"); + + // And nothing is downloadable — not the stale archive, not anything. + assert_eq!(downloadable(&pool, event_id, "zip").await, None); +} + +/// The documented, deliberate nuance in `claim_job`: after a bare `open_event` (which writes +/// NOTHING to `export_job` — that is the point of the design), a worker at the old epoch still WINS +/// its claim and can still write `done`. That is wasted work, not incorrectness: retirement is +/// enforced at READ time. `export_current` must refuse to expose the row. +/// +/// PREVENTS: someone "optimising" `claim_job` into a cross-table `EXISTS (SELECT ... FROM event)` +/// guard — the exact unsound guard migration 014 removed (under READ COMMITTED, a blocked UPDATE +/// re-evaluates same-row predicates but answers other-table subqueries from a stale snapshot). +/// This test pins the read-time enforcement so the write-time guard is never re-added. +#[sqlx::test] +async fn reopen_retires_at_read_time_not_write_time(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let epoch = release_gallery(&pool, "wedding").await.unwrap(); + + assert!(claim_job(&pool, event_id, "zip", epoch).await); + + // Host reopens uploads. No export_job row is touched. + assert_eq!(open_event(&pool, "wedding").await, 1); + + // The in-flight worker's row-local writes still match — it was never told to stop. + assert!( + finalize_job(&pool, event_id, "zip", epoch, "exports/Gallery.1.zip").await, + "documented: the claim/finalize is guarded on the JOB row's epoch, not the event's" + ); + let (status, _, _) = job_row(&pool, event_id, "zip").await.unwrap(); + assert_eq!(status, "done", "the row really does say done"); + + // …and yet it is invisible. `export_current` requires the event to be released AND the epochs to + // match; the reopen broke both. A worker at a dead epoch writes a row nobody can see. + assert!(!in_export_current(&pool, event_id, "zip").await); + assert_eq!( + downloadable(&pool, event_id, "zip").await, + None, + "a reopened event must serve NO keepsake, however finished the job row looks" + ); +} + +// ───────────────────────────────────────────────────────────────────────────── +// 3. `export_current` exactness (table-driven) +// ───────────────────────────────────────────────────────────────────────────── + +/// The view is the ONE definition of "downloadable". Released + `done` + matching epoch ⇒ present; +/// break ANY single leg ⇒ absent. Nothing else may make it appear or disappear. +/// +/// PREVENTS, leg by leg: +/// * `released` — serving a keepsake for an event whose uploads are still open, i.e. an archive +/// missing every photo taken after the snapshot. +/// * `done` — handing out a half-written ZIP (a corrupt keepsake, downloaded once, kept forever). +/// * `epoch` — the retired-generation download: the 404-forever keepsake, or worse, the archive +/// still containing content that was taken down. +#[sqlx::test] +async fn export_current_is_exactly_the_invariant(pool: PgPool) { + // (name, released?, status, job epoch offset from the event epoch, expected visible) + let cases: &[(&str, bool, &str, i64, bool)] = &[ + ("released + done + current epoch", true, "done", 0, true), + ("NOT released (done, epoch matches)", false, "done", 0, false), + ("NOT done: pending", true, "pending", 0, false), + ("NOT done: running", true, "running", 0, false), + ("NOT done: failed", true, "failed", 0, false), + ("stale epoch (done, released)", true, "done", -1, false), + ("future epoch (done, released)", true, "done", 1, false), + ("migration-014 retired sentinel epoch -1", true, "done", -2, false), + ]; + + for (i, (name, released, status, offset, expect_visible)) in cases.iter().enumerate() { + let slug = format!("case{i}"); + let event_id = seed_event(&pool, &slug).await; + + // Get the event to a known epoch (1) either by releasing it, or — for the unreleased case — + // by releasing and reopening, which leaves it unreleased at a non-zero epoch. + let event_epoch_now = if *released { + release_gallery(&pool, &slug).await.unwrap() + } else { + release_gallery(&pool, &slug).await.unwrap(); + open_event(&pool, &slug).await; + event_epoch(&pool, event_id).await + }; + + // Plant a single ZIP job row in the exact state under test. `-2` encodes "the sentinel the + // migration stamps on retired rows", which must never equal a non-negative event epoch. + // (Clear the rows the release armed first — `UNIQUE (event_id, type)`.) + sqlx::query("DELETE FROM export_job WHERE event_id = $1") + .bind(event_id) + .execute(&pool) + .await + .expect("clear armed jobs"); + + let job_epoch = if *offset == -2 { -1 } else { event_epoch_now + offset }; + sqlx::query( + "INSERT INTO export_job (event_id, type, status, progress_pct, epoch, file_path) + VALUES ($1, 'zip', $2::export_status, 100, $3, 'exports/Gallery.zip')", + ) + .bind(event_id) + .bind(*status) + .bind(job_epoch) + .execute(&pool) + .await + .expect("plant job row"); + + let visible = downloadable(&pool, event_id, "zip").await.is_some(); + assert_eq!( + visible, *expect_visible, + "export_current exactness violated for case: {name} \ + (released={released}, status={status}, job_epoch={job_epoch}, event_epoch={event_epoch_now})" + ); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// 4. The ViewerOnly ZIP carry-forward +// ───────────────────────────────────────────────────────────────────────────── + +/// Branch A — the ZIP is `done` at the outgoing epoch: the carry-forward re-stamps it to the new +/// epoch (rows_affected = 1), so only the HTML viewer is rebuilt and the finished ZIP stays +/// downloadable throughout. +/// +/// PREVENTS: rebuilding a multi-GB archive because someone deleted a comment. The ZIP holds media, +/// not comments — a needless rebuild would 404 the photo download for minutes to change nothing +/// inside it. +#[sqlx::test] +async fn viewer_only_carries_a_done_zip_forward(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let e1 = release_gallery(&pool, "wedding").await.unwrap(); + + // Both halves finish at epoch 1 — the keepsake is live. + for t in ["zip", "html"] { + assert!(claim_job(&pool, event_id, t, e1).await); + assert!(finalize_job(&pool, event_id, t, e1, &format!("exports/{t}.{e1}.zip")).await); + } + let zip_file = downloadable(&pool, event_id, "zip").await.expect("zip is live"); + + // ── A comment is moderated: invalidate_and_arm(Affects::ViewerOnly). ── + let (_, _, e2) = bump_epoch(&pool, "wedding").await.unwrap(); + let carried = carry_zip_forward(&pool, event_id, e2).await; + assert!(carried, "a `done` ZIP at epoch-1 MUST be carried forward (rows_affected == 1)"); + + // Only the viewer is re-armed… + let mut conn = pool.acquire().await.unwrap(); + enqueue_types_at_epoch(&mut conn, event_id, e2, &["html"]).await; + drop(conn); + + // …and the ZIP is STILL DOWNLOADABLE, at the new epoch, pointing at the same, unrenamed file. + let (status, epoch, _) = job_row(&pool, event_id, "zip").await.unwrap(); + assert_eq!((status.as_str(), epoch), ("done", e2), "the ZIP row rode the epoch bump"); + assert_eq!( + downloadable(&pool, event_id, "zip").await, + Some(zip_file), + "the carried archive must never stop being served — same file, new epoch" + ); + + // The viewer, meanwhile, is correctly retired and pending a rebuild. + assert_eq!(job_row(&pool, event_id, "html").await.unwrap().0, "pending"); + assert_eq!(downloadable(&pool, event_id, "html").await, None); +} + +/// Branch B — THE BUG WE JUST FIXED. If the ZIP is still `pending`/`running` when the comment is +/// moderated (which is MINUTES for a real multi-GB gallery, and deleting a comment right after +/// release is an utterly ordinary thing to do), the carry-forward matches NOTHING +/// (rows_affected = 0) — so the caller must NOT assume it carried, and must re-arm the ZIP too. +/// +/// PREVENTS: the stranded ZIP. Blindly re-arming only the viewer would leave the ZIP row at the +/// retired epoch; the in-flight worker then finishes and writes `done` at an epoch `export_current` +/// no longer matches, nothing ever re-arms it, and `GET /export/zip` 404s FOREVER — a keepsake the +/// couple paid for that simply never appears, short of a reboot. +#[sqlx::test] +async fn viewer_only_carry_forward_matches_nothing_when_zip_unfinished(pool: PgPool) { + for zip_state in ["pending", "running"] { + let slug = format!("wedding-{zip_state}"); + let event_id = seed_event(&pool, &slug).await; + let e1 = release_gallery(&pool, &slug).await.unwrap(); + + // The ZIP worker is still going; only the viewer has finished. + if zip_state == "running" { + assert!(claim_job(&pool, event_id, "zip", e1).await); + } + assert!(claim_job(&pool, event_id, "html", e1).await); + assert!(finalize_job(&pool, event_id, "html", e1, "exports/Memories.1.zip").await); + + // ── The comment is moderated. ── + let (_, _, e2) = bump_epoch(&pool, &slug).await.unwrap(); + let carried = carry_zip_forward(&pool, event_id, e2).await; + + assert!( + !carried, + "a {zip_state} ZIP has nothing to carry forward — the UPDATE must affect 0 rows \ + (its `status = 'done'` predicate is the whole precondition)" + ); + + // The carry-forward's OWN result decides. It didn't match ⇒ rebuild the ZIP as well. + let types: &[&str] = if carried { &["html"] } else { &["zip", "html"] }; + let mut conn = pool.acquire().await.unwrap(); + enqueue_types_at_epoch(&mut conn, event_id, e2, types).await; + drop(conn); + + // THE ASSERTION THAT WOULD HAVE CAUGHT THE BUG: the ZIP must not be stranded at the dead + // epoch. It is re-armed at the live one, so a fresh worker will actually build it. + let (status, epoch, _) = job_row(&pool, event_id, "zip").await.unwrap(); + assert_eq!( + (status.as_str(), epoch), + ("pending", e2), + "the unfinished ZIP MUST be re-armed at the new epoch, not left stranded at {e1}" + ); + + // Even if the old in-flight worker now "finishes", it is inert and cannot resurrect itself. + assert!(!finalize_job(&pool, event_id, "zip", e1, "exports/Gallery.1.zip").await); + assert_eq!(downloadable(&pool, event_id, "zip").await, None); + } +} + +/// The re-arm upsert must never clobber the archive it just carried forward. +/// +/// `enqueue_types_at_epoch`'s `WHERE export_job.status <> 'done' OR export_job.epoch <> EXCLUDED.epoch` +/// is the "startup recovery must not clobber a good half" rule, expressed as the readiness predicate +/// itself. PREVENTS: boot recovery resetting a perfectly good, downloadable ZIP back to `pending` +/// and making the keepsake 404 while it needlessly rebuilds. +#[sqlx::test] +async fn enqueue_preserves_a_done_half_at_the_current_epoch(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let e1 = release_gallery(&pool, "wedding").await.unwrap(); + + // The ZIP finished; the HTML worker was killed mid-flight (crash) and sits at `running`. + assert!(claim_job(&pool, event_id, "zip", e1).await); + assert!(finalize_job(&pool, event_id, "zip", e1, "exports/Gallery.1.zip").await); + assert!(claim_job(&pool, event_id, "html", e1).await); + + // Boot recovery re-arms both types at the SAME epoch. + let mut conn = pool.acquire().await.unwrap(); + enqueue_types_at_epoch(&mut conn, event_id, e1, &["zip", "html"]).await; + drop(conn); + + // The good half survives untouched… + let (status, epoch, file_path) = job_row(&pool, event_id, "zip").await.unwrap(); + assert_eq!((status.as_str(), epoch), ("done", e1), "a done half at the live epoch is preserved"); + assert_eq!(file_path.as_deref(), Some("exports/Gallery.1.zip"), "file_path not nulled"); + assert!(downloadable(&pool, event_id, "zip").await.is_some()); + + // …and only the missing half is re-armed. + assert_eq!(job_row(&pool, event_id, "html").await.unwrap().0, "pending"); +} diff --git a/backend/tests/upload_concurrency.rs b/backend/tests/upload_concurrency.rs new file mode 100644 index 0000000..29cc182 --- /dev/null +++ b/backend/tests/upload_concurrency.rs @@ -0,0 +1,268 @@ +//! DB-backed integration tests for the two concurrency guards in the upload commit path +//! (`handlers/upload.rs`). Both are SQL — a `FOR SHARE` row lock and an atomic compare-and-increment +//! — and both are load-bearing for things a user can actually lose: a wedding photo, or the disk. +//! +//! `#[sqlx::test]` gives each test a fresh database with the real migrations applied. + +mod common; + +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; + +use chrono::{DateTime, Utc}; +use common::*; +use sqlx::PgPool; +use uuid::Uuid; + +/// SRC: `handlers/upload.rs:313-322` — the guarded quota increment, verbatim. +/// Returns `rows_affected()`; the handler aborts the whole upload tx when this is 0. +async fn quota_inc( + exec: impl sqlx::PgExecutor<'_>, + user_id: Uuid, + size: i64, + limit: i64, +) -> u64 { + sqlx::query( + "UPDATE \"user\" SET total_upload_bytes = total_upload_bytes + $2 + WHERE id = $1 AND total_upload_bytes + $2 <= $3", + ) + .bind(user_id) + .bind(size) + .bind(limit) + .execute(exec) + .await + .expect("quota_inc") + .rows_affected() +} + +async fn total_bytes(pool: &PgPool, user_id: Uuid) -> i64 { + sqlx::query_scalar("SELECT total_upload_bytes FROM \"user\" WHERE id = $1") + .bind(user_id) + .fetch_one(pool) + .await + .expect("total_bytes") +} + +// ───────────────────────────────────────────────────────────────────────────── +// 5. The atomic quota increment +// ───────────────────────────────────────────────────────────────────────────── + +/// Two attempts sized off ONE stale snapshot, each of which "fits" on its own, cannot both commit. +/// The predicate re-reads `total_upload_bytes` inside the UPDATE, so the second matches 0 rows. +/// +/// PREVENTS: one guest filling the disk. The handler's pre-flight quota check runs BEFORE the body is +/// streamed — minutes earlier, for a 500 MB video. If the commit trusted that snapshot, a guest could +/// start N uploads that each individually fit under the limit and land all N, blowing straight through +/// the quota and (in a 1 GB container) taking the event down for everyone. +#[sqlx::test] +async fn quota_two_attempts_from_one_stale_snapshot_cannot_both_commit(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let user_id = seed_user(&pool, event_id, "Gierige Gudrun").await; + + const LIMIT: i64 = 100; + const SIZE: i64 = 60; + + // THE STALE SNAPSHOT: the pre-flight check both uploads were admitted on. + let snapshot = total_bytes(&pool, user_id).await; + assert_eq!(snapshot, 0); + // Each upload, judged against that snapshot alone, fits: 0 + 60 <= 100. Twice. + assert!(snapshot + SIZE <= LIMIT); + + assert_eq!(quota_inc(&pool, user_id, SIZE, LIMIT).await, 1, "the first upload commits"); + assert_eq!( + quota_inc(&pool, user_id, SIZE, LIMIT).await, + 0, + "the second MUST affect 0 rows — it was admitted on a snapshot that is now a lie \ + (60 + 60 = 120 > 100). rows_affected() == 0 is what makes the handler abort." + ); + + assert_eq!(total_bytes(&pool, user_id).await, SIZE, "never 120 — the quota held"); +} + +/// The same, but genuinely CONCURRENT: two transactions that both read `total = 0`, then both try to +/// commit 60 bytes against a 100-byte limit. The second UPDATE blocks on the first's row lock and — +/// because every predicate is on the ROW BEING UPDATED — Postgres re-evaluates it against the +/// post-commit row (EPQ) rather than the statement's original snapshot. It matches nothing. +/// +/// PREVENTS: exactly the same disk-filling overrun, on the path it actually happens — two uploads +/// in flight at once, which is the normal case at a party. +#[sqlx::test] +async fn quota_guard_is_atomic_under_concurrent_transactions(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let user_id = seed_user(&pool, event_id, "Gierige Gudrun").await; + + const LIMIT: i64 = 100; + const SIZE: i64 = 60; + + let mut tx1 = pool.begin().await.unwrap(); + let mut tx2 = pool.begin().await.unwrap(); + + // Both transactions read the same snapshot and both would pass a naive `total + size <= limit` + // check done in Rust. + for tx in [&mut tx1, &mut tx2] { + let seen: i64 = sqlx::query_scalar("SELECT total_upload_bytes FROM \"user\" WHERE id = $1") + .bind(user_id) + .fetch_one(&mut **tx) + .await + .unwrap(); + assert_eq!(seen, 0, "both see an empty quota"); + } + + // tx1 takes the row lock and commits. + assert_eq!(quota_inc(&mut *tx1, user_id, SIZE, LIMIT).await, 1); + tx1.commit().await.unwrap(); + + // tx2's UPDATE was written against the stale snapshot but is evaluated against the row as it + // now stands. + assert_eq!( + quota_inc(&mut *tx2, user_id, SIZE, LIMIT).await, + 0, + "the loser MUST see 0 rows affected — this is the entire quota guarantee" + ); + tx2.rollback().await.unwrap(); + + assert_eq!(total_bytes(&pool, user_id).await, SIZE); + + // And an upload that legitimately fits in what's left still succeeds — the guard rejects + // overruns, not everything. + assert_eq!(quota_inc(&pool, user_id, 40, LIMIT).await, 1, "0 + 60 + 40 == 100, exactly at the limit"); + assert_eq!(total_bytes(&pool, user_id).await, LIMIT); + assert_eq!(quota_inc(&pool, user_id, 1, LIMIT).await, 0, "and one byte more is refused"); +} + +// ───────────────────────────────────────────────────────────────────────────── +// 6. The `FOR SHARE` upload lock vs. the release +// ───────────────────────────────────────────────────────────────────────────── + +/// SRC: `handlers/upload.rs:297-303` — the in-transaction re-check under a row lock, verbatim. +async fn lock_and_read_event( + tx: &mut sqlx::PgConnection, + event_id: Uuid, +) -> (Option>, Option>) { + sqlx::query_as("SELECT uploads_locked_at, export_released_at FROM event WHERE id = $1 FOR SHARE") + .bind(event_id) + .fetch_one(tx) + .await + .expect("FOR SHARE re-check") +} + +/// THE GUARD AGAINST SILENT, PERMANENT PHOTO LOSS. +/// +/// An upload holding `FOR SHARE` on the event row must BLOCK the `UPDATE event SET +/// export_released_at = NOW()` in `release_gallery` until it commits. Either the upload commits first +/// — and the release (hence the export snapshot) is strictly ordered after it, so the keepsake +/// CONTAINS the photo — or the release commits first and the upload observes the lock and rejects +/// (reversibly: the client keeps the blob and resumes after a reopen). +/// +/// PREVENTS: the lost wedding photo. Without this serialization: a guest starts a 500 MB video, the +/// pre-flight lock check passes, the host releases the gallery, the export workers snapshot the +/// uploads table, and THEN the upload commits. The photo appears in the live feed but is missing from +/// the downloaded keepsake, forever — nothing ever regenerates it and nobody ever notices. +#[sqlx::test] +async fn for_share_upload_lock_serializes_against_release(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let user_id = seed_user(&pool, event_id, "Fotograf Fritz").await; + + // ── The guest's upload transaction takes the share lock. ── + let mut upload_tx = pool.begin().await.unwrap(); + let (locked, released) = lock_and_read_event(&mut upload_tx, event_id).await; + assert!(locked.is_none() && released.is_none(), "uploads are open, so we proceed to commit"); + + // ── Concurrently, the host hits "Galerie freigeben". ── + let release_done = Arc::new(AtomicBool::new(false)); + let release_task = { + let pool = pool.clone(); + let release_done = release_done.clone(); + tokio::spawn(async move { + sqlx::query( + "UPDATE event + SET export_released_at = NOW(), + uploads_locked_at = COALESCE(uploads_locked_at, NOW()), + export_epoch = export_epoch + 1 + WHERE id = $1 AND export_released_at IS NULL", + ) + .bind(event_id) + .execute(&pool) + .await + .expect("release"); + release_done.store(true, Ordering::SeqCst); + }) + }; + + // The release MUST be stuck behind our `FOR SHARE` row lock. (`FOR SHARE` conflicts with the + // `FOR UPDATE` lock the UPDATE needs, so Postgres makes it wait — this is not a timing race, + // it is a lock-conflict guarantee; the sleep only gives it every chance to wrongly proceed.) + tokio::time::sleep(Duration::from_millis(750)).await; + assert!( + !release_done.load(Ordering::SeqCst), + "the release MUST block while an upload holds FOR SHARE — if it can slip past, the export \ + snapshot is taken while a photo is still committing and that photo is lost forever" + ); + + // The photo commits. It is now unambiguously part of the upload set. + let upload_id: Uuid = sqlx::query_scalar( + "INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes) + VALUES ($1, $2, 'originals/wedding/x.jpg', 'image/jpeg', 1234) RETURNING id", + ) + .bind(event_id) + .bind(user_id) + .fetch_one(&mut *upload_tx) + .await + .unwrap(); + upload_tx.commit().await.unwrap(); + + // Only now can the release proceed. + tokio::time::timeout(Duration::from_secs(5), release_task) + .await + .expect("the release must unblock once the upload commits") + .unwrap(); + + // THE PAYOFF: the export snapshot — the very query the ZIP worker runs — sees the photo. Order + // enforced by the lock: upload commit < release < snapshot. + let snapshot: Vec = sqlx::query_scalar( + "SELECT u.id FROM upload u + JOIN \"user\" usr ON usr.id = u.user_id + WHERE u.event_id = $1 AND u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE AND usr.is_banned = FALSE", + ) + .bind(event_id) + .fetch_all(&pool) + .await + .unwrap(); + assert_eq!(snapshot, vec![upload_id], "the released keepsake CONTAINS the in-flight photo"); +} + +/// The other side of the same lock: once the release has COMMITTED, the next upload's `FOR SHARE` +/// re-read sees `export_released_at` set and the handler rejects it with `UploadsLocked`. +/// +/// PREVENTS: the same lost photo, on the losing side of the race — a photo committing AFTER the +/// export snapshot would be in the live feed but missing from the keepsake. Rejecting is the correct +/// outcome, and it is reversible: `UploadsLocked` (not Forbidden) tells the client to keep the blob +/// and resume when the host reopens. +#[sqlx::test] +async fn upload_after_release_commits_sees_the_lock_and_is_rejected(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + + // Before the release, the re-check passes. + let mut tx = pool.begin().await.unwrap(); + let (locked, released) = lock_and_read_event(&mut tx, event_id).await; + assert!(locked.is_none() && released.is_none()); + tx.rollback().await.unwrap(); + + assert_eq!(release_gallery(&pool, "wedding").await, Some(1)); + + // After it, the identical re-check sees the release and the handler bails out. + let mut tx = pool.begin().await.unwrap(); + let (locked, released) = lock_and_read_event(&mut tx, event_id).await; + assert!(released.is_some(), "the FOR SHARE re-read MUST observe the committed release"); + assert!(locked.is_some(), "release locks uploads in the same statement (release ⇒ lock)"); + tx.rollback().await.unwrap(); + + // And a reopen makes it uploadable again — the rejection was reversible, not terminal. + assert_eq!(open_event(&pool, "wedding").await, 1); + let mut tx = pool.begin().await.unwrap(); + let (locked, released) = lock_and_read_event(&mut tx, event_id).await; + assert!(locked.is_none() && released.is_none(), "the guest can resume their upload"); + tx.rollback().await.unwrap(); +} diff --git a/e2e/page-objects/export-page.ts b/e2e/page-objects/export-page.ts index d6a63ca..d8dce62 100644 --- a/e2e/page-objects/export-page.ts +++ b/e2e/page-objects/export-page.ts @@ -1,18 +1,50 @@ import type { Page, Locator } from '@playwright/test'; +/** + * Page object for `/export` (see [frontend/src/routes/export/+page.svelte]). + * + * The two archive buttons are BOTH labelled exactly "Download" — the format is carried by + * the surrounding card ("ZIP-Archiv" / "HTML-Viewer"), not by the accessible name. Earlier + * versions of this file looked for `/zip.*herunter/i` and `/^herunterladen$/i`, which match + * nothing on this page; any test asserting `not.toBeVisible()` on those passed no matter + * what the page rendered. Locators here must match the real UI, so scope by card. + * + * "Herunterladen" IS a real label — but only inside the HTML-guide confirm modal, which is + * a different element entirely. + */ export class ExportPage { readonly page: Page; + /** Empty state shown while the host has not released the export yet. */ readonly notAvailableBanner: Locator; + /** Any "Download" button on the page (both cards). Useful for absence assertions. */ + readonly downloadButtons: Locator; readonly zipDownloadButton: Locator; readonly htmlDownloadButton: Locator; + /** Confirm button inside the "Hinweis zum HTML-Viewer" modal. */ readonly htmlGuideModalContinue: Locator; constructor(page: Page) { this.page = page; - this.notAvailableBanner = page.locator('[data-testid="export-not-available"]'); - this.zipDownloadButton = page.getByRole('button', { name: /zip.*herunter|herunter.*zip/i }).first(); - this.htmlDownloadButton = page.getByRole('button', { name: /html.*herunter|herunter.*html/i }).first(); - this.htmlGuideModalContinue = page.getByRole('button', { name: /^herunterladen$/i }); + this.notAvailableBanner = page.getByText('Export noch nicht verfügbar'); + this.downloadButtons = page.getByRole('button', { name: 'Download', exact: true }); + this.zipDownloadButton = this.cardButton('ZIP-Archiv'); + this.htmlDownloadButton = this.cardButton('HTML-Viewer'); + this.htmlGuideModalContinue = page + .getByRole('dialog') + .getByRole('button', { name: /^herunterladen$/i }); + } + + /** + * 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. + */ + private cardButton(heading: string): Locator { + return this.page + .locator('div.rounded-xl') + .filter({ has: this.page.getByRole('heading', { name: heading, exact: true }) }) + .getByRole('button', { name: 'Download', exact: true }); } async goto() { diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index 1073517..382d32a 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -23,7 +23,25 @@ export default defineConfig({ outputDir: './test-results', fullyParallel: false, // Single shared backend → tests TRUNCATE between, so don't run in parallel. forbidOnly: !!process.env.CI, - retries: process.env.CI ? 2 : 0, + // NO RETRIES — not even in CI. This is deliberate and it is the opposite of the usual advice. + // + // The standard case for retries is "the environment is flaky, the product isn't." That argument + // does not hold here. This backend's real bugs ARE races (the last several fixes were all export + // concurrency), and from the outside a race is indistinguishable from a flake — so a retry + // resolves that ambiguity, silently, in favour of "flake", every single time. + // + // The numbers make it concrete. A test that fails 3% of runs reports a bug roughly 1 run in 33. + // Under `retries: 2` it fails the build only when it fails three times in a row: ~1 in 37,000. + // We had exactly such a test, and it was reporting a REAL user-facing bug (a suggestion button + // that committed on mousedown and destroyed itself mid-click). Retries would have buried it + // forever, and buried it GREEN, so nobody would even have seen an amber. + // + // Worse, a retry does not re-run the failing conditions — it runs a CLEANER environment (the + // interfering background worker from the previous test has since finished). So the mechanism is + // specifically good at hiding exactly the cross-test contamination this suite is prone to. + // + // A flake here is a bug report. Treat it as one. + retries: 0, workers: 1, // One worker. Multi-worker needs per-worker isolated DBs (Phase 2+). reporter: [ ['list'], @@ -127,7 +145,12 @@ export default defineConfig({ { name: 'webkit-iphone', use: { ...devices['iPhone 14 Pro'] }, - grep: /@smoke/, + // The PRIMARY user of this app is a wedding guest opening a QR link in iOS Safari. Gating + // 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/**'], }, { name: 'firefox-android', diff --git a/e2e/specs/01-auth/recovery.spec.ts b/e2e/specs/01-auth/recovery.spec.ts index 23d697d..6a6ce2f 100644 --- a/e2e/specs/01-auth/recovery.spec.ts +++ b/e2e/specs/01-auth/recovery.spec.ts @@ -26,12 +26,39 @@ test.describe('Auth — /recover route', () => { await expect(recover.errorMessage).toContainText(/PIN ist falsch|falsch/i); }); - test('non-existent name returns the "not found" error', async ({ page }) => { + // An unknown name must be INDISTINGUISHABLE from a wrong PIN — same status, same message. + // + // This test used to assert the exact opposite: that an unknown name produced a distinct + // "nicht gefunden" error. That IS the account-enumeration oracle the F4 audit fix removed + // (recover now answers an unknown name with the same 401 "PIN ist falsch."), so reintroducing + // the vulnerability would have made this test pass and `recover-enumeration.spec.ts` fail — two + // tests asserting contradictory things about the same behaviour, one of them for the attacker. + // + // It also passed for a reason unrelated to the name: this test takes no `guest` fixture, so the + // per-test TRUNCATE leaves no event row at all, and recover 404s with "Event nicht gefunden." — + // which the old regex happily matched. + test('an unknown name is indistinguishable from a wrong PIN (no enumeration oracle)', async ({ + page, + guest, + }) => { + const handle = await guest('Known'); await clearAllStorage(page); const recover = new RecoverPage(page); + await recover.goto(); await recover.recover('Doesnt-Exist-' + Date.now(), '1234'); - await expect(recover.errorMessage).toContainText(/nicht gefunden|kein/i); + const unknownNameError = (await recover.errorMessage.textContent())?.trim(); + + await recover.goto(); + await recover.recover('Known', handle.pin === '9999' ? '0000' : '9999'); + const wrongPinError = (await recover.errorMessage.textContent())?.trim(); + + expect(unknownNameError).toBeTruthy(); + expect( + unknownNameError, + 'an unknown name must not be distinguishable from a wrong PIN — that is an account-enumeration oracle' + ).toBe(wrongPinError); + expect(unknownNameError).not.toMatch(/nicht gefunden|kein benutzer/i); }); test('lockout expires and counter resets (per recover handler)', async ({ api, guest, db }) => { diff --git a/e2e/specs/02-upload/quota.spec.ts b/e2e/specs/02-upload/quota.spec.ts new file mode 100644 index 0000000..0c0d12a --- /dev/null +++ b/e2e/specs/02-upload/quota.spec.ts @@ -0,0 +1,183 @@ +/** + * Storage-quota ENFORCEMENT — the branch that runs in production and, until now, in zero tests. + * + * `quota_enabled` and `storage_quota_enabled` both default to TRUE in production + * (upload.rs: `config::get_bool(.., "quota_enabled", true)`), but global-setup turns them off and + * the per-test TRUNCATE re-asserts them as 'false' before EVERY test. So the entire quota path was + * dead under test: the pre-check, the `QuotaExceeded` rejection, and — most importantly — the + * atomic increment + * + * UPDATE "user" SET total_upload_bytes = total_upload_bytes + $2 + * WHERE id = $1 AND total_upload_bytes + $2 <= $3 + * + * whose enforcement is `rows_affected() == 0`. That UPDATE is the ONLY thing stopping two + * concurrent uploads from the same guest (phone + laptop, or a double-tap) from both passing the + * stale pre-check and both committing. The five existing unit tests cover `quota_limit_bytes()` — + * the arithmetic that computes the number — which gave the appearance of coverage while the + * enforcement had none. + * + * Blast radius if it regresses: one guest silently fills the disk, and then NOBODY at the wedding + * can upload. Those photos don't exist anywhere else. + * + * These tests steer the limit via `quota_tolerance` (limit = floor(free_disk * tolerance / active)) + * rather than hoping a real disk happens to be nearly full. + */ +import { test, expect } from '../../fixtures/test'; +import { join } from 'node:path'; +import { readFileSync } from 'node:fs'; +import { uploadPausedMidStream } from '../../helpers/upload-client'; + +const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; +const SAMPLE_BYTES = readFileSync(join(process.cwd(), 'fixtures', 'media', 'sample.jpg')); +const SIZE = SAMPLE_BYTES.byteLength; + +interface Quota { + enabled: boolean; + used_bytes: number; + limit_bytes: number | null; + active_uploaders: number; + free_disk_bytes: number | null; +} + +const quotaOf = async (jwt: string): Promise => + (await fetch(BASE + '/api/v1/me/quota', { headers: { Authorization: `Bearer ${jwt}` } })).json(); + +function upload(jwt: string, name: string) { + const form = new FormData(); + form.append('file', new Blob([SAMPLE_BYTES], { type: 'image/jpeg' }), name); + form.append('content_type', 'image/jpeg'); + return fetch(BASE + '/api/v1/upload', { + method: 'POST', + headers: { Authorization: `Bearer ${jwt}` }, + body: form, + }); +} + +/** + * 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. + */ +async function setLimitTo( + api: any, + adminToken: string, + jwt: string, + targetBytes: number +): Promise { + const q = await quotaOf(jwt); + expect(q.free_disk_bytes, 'the disk must be readable, else quota fails OPEN and proves nothing').toBeTruthy(); + const active = Math.max(q.active_uploaders, 1); + const tolerance = (targetBytes * active) / (q.free_disk_bytes as number); + await api.patchConfig(adminToken, { quota_tolerance: tolerance.toExponential(12) }); + + const after = await quotaOf(jwt); + expect(after.enabled).toBe(true); + return after.limit_bytes as number; +} + +test.describe('Upload — storage quota enforcement', () => { + test.beforeEach(async ({ api, adminToken }) => { + await api.patchConfig(adminToken, { + quota_enabled: 'true', + storage_quota_enabled: 'true', + }); + }); + + test('an upload that would exceed the quota is rejected with 413 and stores nothing', async ({ + api, + adminToken, + guest, + }) => { + 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)); + expect(limit).toBeLessThan(SIZE); + + const res = await upload(g.jwt, 'too-big.jpg'); + expect(res.status, 'over-quota upload must be refused').toBe(413); + expect((await res.json()).error).toBe('quota_exceeded'); + + // Nothing was accounted, and nothing reached the feed. + expect((await quotaOf(g.jwt)).used_bytes).toBe(0); + const feed = await (await fetch(BASE + '/api/v1/feed', { headers: { Authorization: `Bearer ${g.jwt}` } })).json(); + expect(feed.uploads).toHaveLength(0); + }); + + test('an upload within the quota still succeeds (the guard is not simply "always no")', async ({ + api, + adminToken, + guest, + }) => { + const g = await guest('QuotaUnder'); + await setLimitTo(api, adminToken, g.jwt, SIZE * 4); + + expect((await upload(g.jwt, 'fine.jpg')).status).toBe(201); + expect((await quotaOf(g.jwt)).used_bytes).toBe(SIZE); + }); + + test('two CONCURRENT uploads cannot BOTH slip past the quota (atomic increment)', async ({ + api, + adminToken, + guest, + }) => { + const g = await guest('QuotaRacer'); + + // Room for exactly ONE file. + const limit = await setLimitTo(api, adminToken, g.jwt, Math.floor(SIZE * 1.5)); + expect(limit).toBeGreaterThanOrEqual(SIZE); + expect(limit).toBeLessThan(SIZE * 2); + + // Both uploads are held OPEN MID-BODY, and this is the entire point of the test. + // + // The handler loads the user row (and therefore snapshots `total_upload_bytes`) at its very + // FIRST line — before it streams a single byte of the body. So holding both requests inside + // the body loop guarantees both are carrying the same stale `total = 0`, and both pre-checks + // will conclude "this fits". Only the conditional UPDATE can then stop the second. + // + // Firing them with a plain `Promise.all` does NOT reproduce this: the first request completes + // and commits before the second even reads the user row, so the *pre-check* rejects the second + // and the test passes with the atomic guard deleted. (It did. That is how this test was caught + // being vacuous, and why it is written this way.) + const head = SAMPLE_BYTES.subarray(0, 32); + const tail = SAMPLE_BYTES.subarray(32); + const a = uploadPausedMidStream(g.jwt, head, tail, { filename: 'race-a.jpg' }); + const b = uploadPausedMidStream(g.jwt, head, tail, { filename: 'race-b.jpg' }); + + // Both are now past the pre-flight checks and sitting in the body loop, each holding total = 0. + await Promise.all([a.checkPassed, b.checkPassed]); + + a.finish(); + b.finish(); + const [ra, rb] = await Promise.all([a.response, b.response]); + const statuses = [ra.status, rb.status].sort(); + + expect( + statuses, + `exactly one upload may win the race; got ${JSON.stringify(statuses)} — if BOTH are 201 the atomic guard is gone and a single guest can overrun the disk, after which NOBODY at the party can upload` + ).toEqual([201, 413]); + + // The invariant that actually matters: accounting never exceeds the ceiling. + const q = await quotaOf(g.jwt); + expect(q.used_bytes).toBe(SIZE); + expect(q.used_bytes).toBeLessThanOrEqual(limit); + }); + + test('GET /me/quota reports the live usage the UI shows the guest', async ({ + api, + adminToken, + guest, + }) => { + // 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); + + const before = await quotaOf(g.jwt); + expect(before.enabled).toBe(true); + expect(before.used_bytes).toBe(0); + + expect((await upload(g.jwt, 'one.jpg')).status).toBe(201); + + const after = await quotaOf(g.jwt); + expect(after.used_bytes).toBe(SIZE); + expect(after.limit_bytes).toBeGreaterThan(after.used_bytes); + }); +}); diff --git a/e2e/specs/05-admin/authz-sweep.spec.ts b/e2e/specs/05-admin/authz-sweep.spec.ts new file mode 100644 index 0000000..ac6bcef --- /dev/null +++ b/e2e/specs/05-admin/authz-sweep.spec.ts @@ -0,0 +1,129 @@ +/** + * Table-driven privilege sweep over EVERY host- and admin-gated route. + * + * Before this file, exactly THREE authorization assertions existed in the whole suite (guest → + * /host/event/close, guest → GET /admin/config, host → GET /admin/config). Sixteen of nineteen + * privileged routes had no test proving a guest is turned away — including: + * + * POST /host/users/{id}/pin-reset → reset any guest's PIN, then log in as them via /recover. + * That is account takeover, and nothing guarded it. + * DELETE /host/upload/{id} → delete any guest's photo (unrecoverable after the party). + * POST /host/event/open → retire the released keepsake and reopen uploads to everyone. + * PATCH /admin/config → only GET was 403-tested; a guest WRITING config could + * disable quotas and rate limits outright. + * + * The point of a table is that adding a route to the router and forgetting to protect it should + * make a test fail. So this asserts the WHOLE privileged surface, not a sample of it. + * + * Deliberately NOT covered here: the read-only-ban model (a banned user keeps read access and can + * still download the keepsake) is BY DESIGN, and is asserted in 07-adversarial/authorization-deep. + */ +import { test, expect } from '../../fixtures/test'; + +const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + +type Method = 'GET' | 'POST' | 'PATCH' | 'DELETE'; +interface Route { + method: Method; + path: (victimId: string) => string; + name: string; + body?: unknown; + /** admin-only routes must also reject a HOST, not just a guest. */ + adminOnly?: boolean; +} + +const ROUTES: Route[] = [ + // ── Host surface ──────────────────────────────────────────────────────── + { method: 'GET', path: () => '/api/v1/host/event', name: 'GET /host/event' }, + { method: 'POST', path: () => '/api/v1/host/event/close', name: 'POST /host/event/close' }, + { method: 'POST', path: () => '/api/v1/host/event/open', name: 'POST /host/event/open' }, + { method: 'POST', path: () => '/api/v1/host/gallery/release', name: 'POST /host/gallery/release' }, + { method: 'POST', path: () => '/api/v1/host/export/rebuild', name: 'POST /host/export/rebuild' }, + { method: 'GET', path: () => '/api/v1/host/users', name: 'GET /host/users' }, + { method: 'POST', path: (v) => `/api/v1/host/users/${v}/ban`, name: 'POST /host/users/{id}/ban', body: {} }, + { method: 'POST', path: (v) => `/api/v1/host/users/${v}/unban`, name: 'POST /host/users/{id}/unban', body: {} }, + { method: 'PATCH', path: (v) => `/api/v1/host/users/${v}/role`, name: 'PATCH /host/users/{id}/role', body: { role: 'host' } }, + { method: 'POST', path: (v) => `/api/v1/host/users/${v}/pin-reset`, name: 'POST /host/users/{id}/pin-reset', body: {} }, + { method: 'GET', path: () => '/api/v1/host/pin-reset-requests', name: 'GET /host/pin-reset-requests' }, + { method: 'DELETE', path: (v) => `/api/v1/host/pin-reset-requests/${v}`, name: 'DELETE /host/pin-reset-requests/{id}' }, + { method: 'DELETE', path: (v) => `/api/v1/host/upload/${v}`, name: 'DELETE /host/upload/{id}' }, + { method: 'DELETE', path: (v) => `/api/v1/host/comment/${v}`, name: 'DELETE /host/comment/{id}' }, + + // ── Admin surface ─────────────────────────────────────────────────────── + { method: 'GET', path: () => '/api/v1/admin/stats', name: 'GET /admin/stats', adminOnly: true }, + { method: 'GET', path: () => '/api/v1/admin/config', name: 'GET /admin/config', adminOnly: true }, + { method: 'PATCH', path: () => '/api/v1/admin/config', name: 'PATCH /admin/config', adminOnly: true, body: { quota_enabled: 'false' } }, + { method: 'GET', path: () => '/api/v1/admin/export/jobs', name: 'GET /admin/export/jobs', adminOnly: true }, +]; + +async function call(route: Route, jwt: string, victimId: string): Promise { + const res = await fetch(BASE + route.path(victimId), { + method: route.method, + headers: { + Authorization: `Bearer ${jwt}`, + ...(route.body !== undefined ? { 'Content-Type': 'application/json' } : {}), + }, + ...(route.body !== undefined ? { body: JSON.stringify(route.body) } : {}), + }); + return res.status; +} + +test.describe('Authorization sweep — every privileged route', () => { + for (const route of ROUTES) { + test(`a GUEST is refused: ${route.name}`, async ({ guest }) => { + const attacker = await guest('AuthzAttacker'); + const victim = await guest('AuthzVictim'); + + const status = await call(route, attacker.jwt, victim.userId); + + // 403 exactly. NOT "some 4xx" — a 404 would mean the request got past the role gate and + // merely failed to find the resource, which is precisely the vacuity this suite has been + // bitten by: it would still pass with the RequireHost/RequireAdmin extractor deleted. + expect(status, `${route.name} must reject a guest with 403, got ${status}`).toBe(403); + }); + } + + for (const route of ROUTES.filter((r) => r.adminOnly)) { + test(`a HOST is refused: ${route.name}`, async ({ host, guest }) => { + const victim = await guest('AuthzVictim'); + + const status = await call(route, host.jwt, victim.userId); + + expect(status, `${route.name} is admin-only and must reject a host with 403, got ${status}`).toBe(403); + }); + } + + // The truncate endpoint wipes every table AND the media directory. It is test-mode only, but it + // is reachable over HTTP and gated solely by RequireAdmin — so it deserves the same proof. + test('a GUEST is refused: POST /admin/__truncate (wipes the whole event)', async ({ guest }) => { + const attacker = await guest('TruncateAttacker'); + const res = await fetch(`${BASE}/api/v1/admin/__truncate`, { + method: 'POST', + headers: { Authorization: `Bearer ${attacker.jwt}` }, + }); + expect(res.status).toBe(403); + }); + + test('a HOST is refused: POST /admin/__truncate', async ({ host }) => { + const res = await fetch(`${BASE}/api/v1/admin/__truncate`, { + method: 'POST', + headers: { Authorization: `Bearer ${host.jwt}` }, + }); + expect(res.status).toBe(403); + }); + + // An unauthenticated caller must not get further than an authenticated-but-unprivileged one. + test('an ANONYMOUS caller is refused every privileged route', async () => { + const victim = '00000000-0000-0000-0000-000000000000'; + for (const route of ROUTES) { + const res = await fetch(BASE + route.path(victim), { + method: route.method, + headers: route.body !== undefined ? { 'Content-Type': 'application/json' } : {}, + ...(route.body !== undefined ? { body: JSON.stringify(route.body) } : {}), + }); + expect([401, 403], `${route.name} must reject an anonymous caller, got ${res.status}`).toContain( + res.status + ); + } + }); +}); diff --git a/e2e/specs/06-export/export.spec.ts b/e2e/specs/06-export/export.spec.ts index 48d8a1d..44b71b7 100644 --- a/e2e/specs/06-export/export.spec.ts +++ b/e2e/specs/06-export/export.spec.ts @@ -16,8 +16,40 @@ test.describe('Export — release and download', () => { await signIn(page, g); const exportPage = new ExportPage(page); await exportPage.goto(); - // The page shouldn't show download buttons before release. - await expect(page.getByRole('button', { name: /^herunterladen$/i })).not.toBeVisible(); + + // POSITIVE anchor first. An absence-only assertion ("no download button") is green on a + // blank page, a 404, or an unhydrated shell — i.e. it would pass even with the buttons + // rendered, if the locator were wrong. Pin the empty state we actually expect. + await expect(exportPage.notAvailableBanner).toBeVisible({ timeout: 10_000 }); + + // And only THEN the absence: no download affordance exists before release. This uses the + // real button label ("Download"), so rendering a download button here turns it red. + await expect(exportPage.downloadButtons).toHaveCount(0); + }); + + test('/export shows enabled download buttons once released and the jobs are done', async ({ + page, + guest, + signIn, + db, + }) => { + const g = await guest('PostRelease'); + await db.setExportReleased(SLUG, true); + await db.fakeExportJob(SLUG, 'zip', 'done'); + await db.fakeExportJob(SLUG, 'html', 'done'); + + await signIn(page, g); + const exportPage = new ExportPage(page); + await exportPage.goto(); + + // The mirror of the test above: this is what proves the "before release" locators can + // actually SEE a download button when one exists. Without this, a typo'd locator makes + // the pre-release test unfalsifiable. + await expect(exportPage.notAvailableBanner).toHaveCount(0); + await expect(exportPage.zipDownloadButton).toBeVisible({ timeout: 10_000 }); + await expect(exportPage.zipDownloadButton).toBeEnabled(); + await expect(exportPage.htmlDownloadButton).toBeVisible(); + await expect(exportPage.htmlDownloadButton).toBeEnabled(); }); test('export status API reflects released flag', async ({ guest, db }) => { diff --git a/e2e/specs/07-adversarial/auth-tampering.spec.ts b/e2e/specs/07-adversarial/auth-tampering.spec.ts index 06480ae..9403889 100644 --- a/e2e/specs/07-adversarial/auth-tampering.spec.ts +++ b/e2e/specs/07-adversarial/auth-tampering.spec.ts @@ -110,7 +110,7 @@ test.describe('Adversarial — PIN brute-force', () => { expect(correct.status).toBe(429); }); - test('parallel wrong-PIN attempts may NOT all hit lockout (race-condition finding)', async ({ guest }) => { + test('parallel wrong-PIN attempts still lock the account (counter is not lost to the race)', async ({ guest }) => { const g = await guest('BruteParallel'); const wrong = g.pin === '0000' ? '1111' : '0000'; @@ -124,12 +124,22 @@ test.describe('Adversarial — PIN brute-force', () => { ) ); const statuses = attempts.map((r) => r.status); - expect(statuses.filter((s) => s === 200)).toHaveLength(0); - // Documented behavior: lockout counter may race so not every status is 429. - // Critical invariant: no attempt succeeded. - if (!statuses.some((s) => s === 429)) { - console.warn('[finding] PIN-attempt counter races under parallel requests — none hit lockout.'); - } + expect(statuses.filter((s) => s === 200), 'a wrong PIN must never authenticate').toHaveLength(0); + + // The in-flight requests all read `pin_locked_until` before any of them wrote it, so + // *which* of the 10 come back 429 is genuinely racy and can't be asserted. What is NOT + // racy — and is the property this test exists to guard — is the state left behind: + // `failed_pin_attempts` is incremented with an atomic `SET x = x + 1 ... RETURNING`, so + // 10 wrong PINs must push it past the 3-strike threshold and leave the account locked. + // + // We prove that with a follow-up request using the CORRECT pin: it must be refused with + // 429 (locked), not 200. Delete the lockout counter and this line goes 200 → red. + const correct = await fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: g.displayName, pin: g.pin }), + }); + expect(correct.status, 'after 10 wrong PINs the account must be locked, even for the right PIN').toBe(429); }); }); diff --git a/e2e/specs/07-adversarial/authorization-deep.spec.ts b/e2e/specs/07-adversarial/authorization-deep.spec.ts index 7a22943..d17f579 100644 --- a/e2e/specs/07-adversarial/authorization-deep.spec.ts +++ b/e2e/specs/07-adversarial/authorization-deep.spec.ts @@ -80,27 +80,39 @@ test.describe('Adversarial — deep authorization', () => { expect(row?.caption).toBe('original caption'); }); + // These two fire at a REAL upload, and demand exactly 403. + // + // They used to POST to the all-zeros UUID and accept `[403, 404]`. Both handlers check + // `user.is_banned` BEFORE they look the upload up (social.rs) — so the 404 came from the + // *lookup*, not the guard. Delete the ban check entirely and the request still 404s on the + // nonexistent upload, and both tests still passed. They were the only coverage of + // ban-blocks-like and ban-blocks-comment, and they guarded nothing. test('banned user cannot toggle a like', async ({ api, host, guest }) => { const target = await guest('BannedLike'); + const uploadId = await seedUpload(host.jwt, { caption: 'likeable' }); await api.banUser(host.jwt, target.userId); - const res = await fetch(`${BASE}/api/v1/upload/00000000-0000-0000-0000-000000000000/like`, { + const res = await fetch(`${BASE}/api/v1/upload/${uploadId}/like`, { method: 'POST', headers: { Authorization: `Bearer ${target.jwt}` }, }); - expect([403, 404]).toContain(res.status); + expect(res.status, 'a banned user must be Forbidden, not merely miss the resource').toBe(403); }); test('banned user cannot post a comment', async ({ api, host, guest }) => { const target = await guest('BannedComment'); + const uploadId = await seedUpload(host.jwt, { caption: 'commentable' }); await api.banUser(host.jwt, target.userId); - const res = await fetch(`${BASE}/api/v1/upload/00000000-0000-0000-0000-000000000000/comments`, { + const res = await fetch(`${BASE}/api/v1/upload/${uploadId}/comments`, { method: 'POST', headers: { Authorization: `Bearer ${target.jwt}`, 'Content-Type': 'application/json' }, body: JSON.stringify({ body: 'should be rejected' }), }); - expect([403, 404]).toContain(res.status); + expect(res.status, 'a banned user must be Forbidden, not merely miss the resource').toBe(403); + + // ...and nothing was written. + expect(await listComments(host.jwt, uploadId)).toHaveLength(0); }); test('banned user can still read the feed (read-only access preserved)', async ({ api, host, guest }) => { @@ -130,15 +142,40 @@ test.describe('Adversarial — deep authorization', () => { expect(stillWorks.status).toBe(200); }); - test('promote endpoint cannot be used to make oneself admin', async ({ host }) => { - const res = await fetch(`${BASE}/api/v1/host/users/${'00000000-0000-0000-0000-000000000000'}/role`, { + // Privilege escalation, tested against REAL targets. + // + // The old version PATCHed the all-zeros UUID and accepted `[400, 403, 404]`. The role whitelist + // rejects "admin" with a 400 before the target is ever looked up — so adding `"admin"` to the + // whitelist would make the request 404 on the nonexistent user instead, which was in the accepted + // list. It never promoted anyone, never targeted *oneself*, and could not detect escalation. + test('a host cannot promote a real guest to admin', async ({ api, host, guest, adminToken }) => { + const victim = await guest('EscalationTarget'); + + const res = await fetch(`${BASE}/api/v1/host/users/${victim.userId}/role`, { method: 'PATCH', headers: { Authorization: `Bearer ${host.jwt}`, 'Content-Type': 'application/json' }, body: JSON.stringify({ role: 'admin' }), }); - // 400 (invalid role for host-callable endpoint) or 403/404. - expect([400, 403, 404]).toContain(res.status); - // Critically, NOT 200/204. - expect([200, 204]).not.toContain(res.status); + expect(res.status).not.toBe(204); + expect(res.status).not.toBe(200); + + // The assertion that actually matters: nobody got promoted. + const users = await api.listUsers(adminToken); + const row = users.find((u: any) => u.id === victim.userId); + expect(row?.role, 'the guest must NOT have become an admin').not.toBe('admin'); + }); + + test('a host cannot promote THEMSELVES to admin', async ({ api, host, adminToken }) => { + const res = await fetch(`${BASE}/api/v1/host/users/${host.userId}/role`, { + method: 'PATCH', + headers: { Authorization: `Bearer ${host.jwt}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ role: 'admin' }), + }); + expect(res.status).not.toBe(204); + expect(res.status).not.toBe(200); + + const users = await api.listUsers(adminToken); + const me = users.find((u: any) => u.id === host.userId); + expect(me?.role, 'the host must NOT have self-promoted to admin').not.toBe('admin'); }); }); diff --git a/e2e/specs/07-adversarial/ddos.spec.ts b/e2e/specs/07-adversarial/ddos.spec.ts index 8779e4d..1b0c57b 100644 --- a/e2e/specs/07-adversarial/ddos.spec.ts +++ b/e2e/specs/07-adversarial/ddos.spec.ts @@ -5,6 +5,7 @@ */ import { test, expect } from '../../fixtures/test'; import { mintSseTicket } from '../../helpers/sse'; +import { seedUpload } from '../../helpers/seed'; const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; @@ -30,19 +31,50 @@ test.describe('Adversarial — small-scale abuse', () => { expect(statuses.some((s) => s === 201 || s === 409)).toBe(true); }); - test('10 MB comment body is rejected (multipart-less endpoint)', async ({ guest }) => { - const g = await guest('BigComment'); - const huge = 'A'.repeat(10 * 1024 * 1024); - const res = await fetch(`${BASE}/api/v1/upload/00000000-0000-0000-0000-000000000000/comments`, { + // The comment body cap lives in [backend/src/handlers/social.rs] `add_comment`: + // if text_chars == 0 || text_chars > 500 → 400 + // It must be exercised against a REAL upload: the handler looks the upload up (and + // 404s) *before* it reaches the length check, so posting to a non-existent id proves + // nothing about the cap. + async function postComment(jwt: string, uploadId: string, body: string) { + return fetch(`${BASE}/api/v1/upload/${uploadId}/comments`, { method: 'POST', - headers: { Authorization: `Bearer ${g.jwt}`, 'Content-Type': 'application/json' }, - body: JSON.stringify({ body: huge }), + headers: { Authorization: `Bearer ${jwt}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ body }), }); - // 400 (length cap), 404 (no such upload), 413 (payload too large), 429 (rate-limited), - // or 502 (Caddy rejected the body before it reached the backend) — all fine. - expect([400, 404, 413, 429, 502]).toContain(res.status); - // Not 200 — that would mean we accepted a 10 MB comment. - expect(res.status).not.toBe(200); + } + + test('comment body over the 500-char cap is rejected with 400', async ({ guest }) => { + const g = await guest('LongComment'); + const uploadId = await seedUpload(g.jwt); + + const res = await postComment(g.jwt, uploadId, 'A'.repeat(501)); + expect(res.status, '501 chars must be rejected by the length cap').toBe(400); + const json: any = await res.json().catch(() => ({})); + expect((json.message ?? '').toLowerCase()).toMatch(/500 zeichen/); + }); + + test('comment body exactly at the 500-char cap is accepted', async ({ guest }) => { + const g = await guest('MaxComment'); + const uploadId = await seedUpload(g.jwt); + + // The boundary must be inclusive — otherwise the "cap" is really 499 and the + // rejection test above would also pass on an off-by-one implementation. + const res = await postComment(g.jwt, uploadId, 'A'.repeat(500)); + expect(res.status, '500 chars is the documented maximum and must be accepted').toBe(201); + }); + + test('10 MB comment body never reaches the handler (body-size limit rejects it)', async ({ guest }) => { + const g = await guest('BigComment'); + const uploadId = await seedUpload(g.jwt); + const huge = 'A'.repeat(10 * 1024 * 1024); + + const res = await postComment(g.jwt, uploadId, huge); + // This asserts ONLY what it can prove: a 10 MB JSON body is refused somewhere on the + // path (Caddy's request-body limit → 502/413, or the backend's own body limit → 413, + // or the 500-char cap if it does get through → 400). The upload exists, so a 404 here + // would be a bug, and a 201 would mean we stored a 10 MB comment. + expect([400, 413, 502]).toContain(res.status); }); test('SSE: 10 concurrent streams from one user do not crash the server', async ({ guest }) => { diff --git a/e2e/specs/07-adversarial/file-upload-attacks.spec.ts b/e2e/specs/07-adversarial/file-upload-attacks.spec.ts index 8cbe49d..99b5607 100644 Binary files a/e2e/specs/07-adversarial/file-upload-attacks.spec.ts and b/e2e/specs/07-adversarial/file-upload-attacks.spec.ts differ diff --git a/e2e/specs/07-adversarial/ui-rendering.spec.ts b/e2e/specs/07-adversarial/ui-rendering.spec.ts index a82c572..9674374 100644 --- a/e2e/specs/07-adversarial/ui-rendering.spec.ts +++ b/e2e/specs/07-adversarial/ui-rendering.spec.ts @@ -26,14 +26,20 @@ test.describe('Adversarial — UI render escape', () => { }); await page.goto('/account'); - await page.waitForLoadState('domcontentloaded'); + + // Render guard FIRST. `domcontentloaded` fires before Svelte hydrates, so asserting + // the *absence* of a at that point passes on a page that never rendered the name + // at all — a false green on an XSS test. Prove the payload actually reached the DOM + // (as escaped text) before concluding anything about how it was rendered. + await expect(page.getByText(payload, { exact: false }).first()).toBeVisible({ timeout: 10_000 }); const fired = await page.evaluate(() => (window as any).__xssFired === true); expect(fired).toBe(false); - // tag inside the name should also not render as bold — Svelte escapes the entire string. - const boldCount = await page.locator('b:has-text("BOLD")').count(); - expect(boldCount).toBe(0); + // tag inside the name should also not render as bold — Svelte escapes the entire + // string. toHaveCount auto-retries, so this can't win by racing hydration. + await expect(page.locator('b:has-text("BOLD")')).toHaveCount(0); + await expect(page.locator('script:has-text("__xssFired")')).toHaveCount(0); }); test('rendering of a known SQL-injection-shaped name does not break the page', async ({ page, api }) => { diff --git a/e2e/specs/07-adversarial/xss-injection.spec.ts b/e2e/specs/07-adversarial/xss-injection.spec.ts index 8dacae0..cf0ba49 100644 --- a/e2e/specs/07-adversarial/xss-injection.spec.ts +++ b/e2e/specs/07-adversarial/xss-injection.spec.ts @@ -16,15 +16,29 @@ import { seedUpload, seedComment } from '../../helpers/seed'; const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; +/** + * Every payload sets `window.__x = 1` if it executes. The marker is deliberately terse: + * the join handler caps display names at 50 chars, and a payload that trips that cap is + * rejected at the API — which means it is NEVER stored and NEVER rendered, so the test + * that "nothing executed" proves nothing at all. Each payload below is < 50 chars, so it + * survives the join and actually reaches the render sink under test. + */ const XSS_PAYLOADS = [ - ``, - ``, - `">`, - ``, - `javascript:window.__xssFired=true`, - `click`, + ``, // 29 + ``, // 34 + `">`, // 29 + ``, // 47 + `javascript:window.__x=1`, // 23 + `c`, // 39 ]; +// Guard the invariant the payloads depend on: if the display-name cap ever changes, or a +// payload is edited past it, we want a loud failure here rather than six silent no-ops. +const NAME_MAX = 50; +for (const p of XSS_PAYLOADS) { + if (p.length > NAME_MAX) throw new Error(`XSS payload exceeds the ${NAME_MAX}-char display-name cap and would never be stored: ${p}`); +} + const SQLI_PAYLOADS = [ `'; DROP TABLE "user"; --`, `' OR 1=1 --`, @@ -35,19 +49,10 @@ const SQLI_PAYLOADS = [ test.describe('Adversarial — input injection (display name)', () => { for (const payload of XSS_PAYLOADS) { test(`name with XSS payload ${JSON.stringify(payload).slice(0, 40)} never executes`, async ({ api, page }) => { - // Payloads > 50 chars are rejected by the join handler — that's a valid defense. - // Only if the API accepts the payload do we proceed to assert it never executes - // when rendered. - let res; - try { - res = await api.join(payload); - } catch (e: any) { - if (/→ 400/.test(e.message ?? '')) { - // Defended at the API. No need to render. - return; - } - throw e; - } + // No try/catch escape hatch: every payload is short enough to be accepted, so a + // rejection here is a real failure (the payload would never be rendered, and the + // "nothing executed" assertions below would be vacuous). + const res = await api.join(payload); expect(res.jwt).toBeTruthy(); // Render the name in the account page by signing in. @@ -74,12 +79,12 @@ test.describe('Adversarial — input injection (display name)', () => { // so a "nothing fired" pass can't be because the name was never rendered. await expect(page.getByText(payload, { exact: false }).first()).toBeVisible({ timeout: 10_000 }); - const fired = await page.evaluate(() => (window as any).__xssFired === true); - expect(fired, 'window.__xssFired should never be set').toBe(false); + const fired = await page.evaluate(() => (window as any).__x === 1); + expect(fired, 'window.__x should never be set').toBe(false); expect(dialogs, 'no dialogs should appear').toHaveLength(0); // Inline script tag in the displayed name should be rendered as text, not parsed. - const scriptCount = await page.locator('script:has-text("window.__xssFired")').count(); + const scriptCount = await page.locator('script:has-text("window.__x")').count(); expect(scriptCount, 'no executable script tags rendered from name').toBe(0); }); } @@ -101,11 +106,11 @@ test.describe('Adversarial — stored XSS (caption)', () => { // Wait for the caption text to land in the DOM (escaped, as literal text). await expect(page.getByText('CAPMARK', { exact: false }).first()).toBeVisible({ timeout: 10_000 }); - expect(await page.evaluate(() => (window as any).__xssFired === true), 'caption XSS must not fire').toBe(false); + expect(await page.evaluate(() => (window as any).__x === 1), 'caption XSS must not fire').toBe(false); expect(dialogs, 'no dialogs from a caption').toHaveLength(0); // The payload must be inert text, not a live element / script. expect(await page.locator('img[onerror]').count(), 'no live onerror img from caption').toBe(0); - expect(await page.locator('script:has-text("__xssFired")').count(), 'no executable script from caption').toBe(0); + expect(await page.locator('script:has-text("window.__x")').count(), 'no executable script from caption').toBe(0); }); } }); @@ -114,8 +119,8 @@ test.describe('Adversarial — stored XSS (comment)', () => { // The two payloads that actually execute on render (script injection via innerHTML // does not) — enough to prove the comment body is escaped without a slow 6× lightbox loop. const COMMENT_PAYLOADS = [ - ``, - `">`, + ``, + `">`, ]; for (const payload of COMMENT_PAYLOADS) { test(`comment with XSS payload ${JSON.stringify(payload).slice(0, 40)} renders inert`, async ({ guest, page, signIn }) => { @@ -139,7 +144,7 @@ test.describe('Adversarial — stored XSS (comment)', () => { // Wait until the comment (marker) has rendered. await expect(lightbox.getByText('CMTMARK', { exact: false })).toBeVisible({ timeout: 10_000 }); - expect(await page.evaluate(() => (window as any).__xssFired === true), 'comment XSS must not fire').toBe(false); + expect(await page.evaluate(() => (window as any).__x === 1), 'comment XSS must not fire').toBe(false); expect(dialogs, 'no dialogs from a comment').toHaveLength(0); expect(await page.locator('img[onerror]').count(), 'no live onerror img from comment').toBe(0); }); diff --git a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts index 6539025..8a0a8e2 100644 --- a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts +++ b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts @@ -421,6 +421,103 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k expect(listing.some((n) => n.includes(keep)), 'the kept photo survives').toBe(true); }); + test('BANNING a guest after release removes their photos from the keepsake', async ({ + api, + host, + guest, + }) => { + // `ban_user` calls `invalidate_and_arm(Affects::Both)` — but every ban in the suite ran against + // an UNRELEASED event, where `invalidate_and_arm` returns early. So this regeneration path was + // dead code under test. + // + // The stakes are the whole point of a ban: the host bans someone for posting something abusive, + // and if the keepsake doesn't rebuild, that content stays in the archive every guest downloads, + // forever. The export query already filters `is_banned` — so the pipeline AGREES the content + // shouldn't be there; only the regeneration was missing. + const offender = await guest('Offender'); + const innocent = await guest('Innocent'); + const badPhoto = await seedUpload(offender.jwt, { caption: 'abusive' }); + const goodPhoto = await seedUpload(innocent.jwt, { caption: 'lovely' }); + + expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204); + await waitExportDone(host.jwt); + expect(await downloadZipEntries(host.jwt)).toHaveLength(2); + + await api.banUser(host.jwt, offender.userId); + + await waitExportDone(host.jwt); + const listing = await downloadZipEntries(host.jwt); + expect( + listing.some((n) => n.includes(badPhoto)), + "a banned guest's photo must not survive in the keepsake everyone downloads" + ).toBe(false); + expect(listing.some((n) => n.includes(goodPhoto)), 'everyone else keeps their photos').toBe(true); + expect(listing).toHaveLength(1); + }); + + test('UNBANNING a guest after release restores their photos to the keepsake', async ({ + api, + host, + guest, + }) => { + // The mirror image, and the one that LOSES data. A host bans someone by mistake (or bans, then + // reconsiders) — unban must put their photos back into the archive. If the regeneration is + // missing, that guest's photos are absent from the keepsake everyone keeps, and after the + // wedding they are gone: the export is the only copy anyone takes home. + const g = await guest('Forgiven'); + const photo = await seedUpload(g.jwt, { caption: 'restore me' }); + await seedUpload(host.jwt, { caption: 'host photo' }); + + await api.banUser(host.jwt, g.userId); + expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204); + await waitExportDone(host.jwt); + // Banned at release time → their photo is correctly absent. + expect(await downloadZipEntries(host.jwt)).toHaveLength(1); + + const unban = await post(`/api/v1/host/users/${g.userId}/unban`, host.jwt); + expect(unban.status).toBe(204); + + await waitExportDone(host.jwt); + const listing = await downloadZipEntries(host.jwt); + expect( + listing.some((n) => n.includes(photo)), + 'an unbanned guest must get their photos back in the keepsake — it is the only copy anyone keeps' + ).toBe(true); + expect(listing).toHaveLength(2); + }); + + test('a HOST comment takedown after release keeps the keepsake downloadable', async ({ + host, + guest, + }) => { + // `DELETE /host/comment/{id}` had ZERO test hits of any kind, and it is one of the two callers + // of the ViewerOnly carry-forward. A comment doesn't live in the ZIP (which holds media), so the + // ZIP must be CARRIED FORWARD, not rebuilt — but it must still be downloadable afterwards, and + // the viewer must lose the comment. + const g = await guest('Rude'); + const uploadId = await seedUpload(host.jwt, { caption: 'pic' }); + const cRes = await fetch(BASE + `/api/v1/upload/${uploadId}/comments`, { + method: 'POST', + headers: { Authorization: `Bearer ${g.jwt}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ body: 'unfreundlich' }), + }); + const commentId = (await cRes.json()).id; + + expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204); + await waitExportDone(host.jwt); + + const del = await fetch(BASE + `/api/v1/host/comment/${commentId}`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${host.jwt}` }, + }); + expect(del.status).toBe(204); + + // The keepsake must still be downloadable, with its media intact. + await waitExportDone(host.jwt); + expect(await downloadZipEntries(host.jwt)).toHaveLength(1); + expect(await zipJobEpoch()).toBe(await eventEpoch()); + }); + test('a comment deleted while the ZIP is still BUILDING does not strand the ZIP', async ({ host, guest, diff --git a/frontend/src/lib/avatar.test.ts b/frontend/src/lib/avatar.test.ts index 00b6673..bcb88cc 100644 --- a/frontend/src/lib/avatar.test.ts +++ b/frontend/src/lib/avatar.test.ts @@ -9,12 +9,36 @@ describe('avatarPalette', () => { }); it('is deterministic for the same name', () => { + // Same input, repeated calls AND a separately-constructed equal string — the palette + // must be a pure function of the name's characters, not of identity or call order. expect(avatarPalette('Alice')).toBe(avatarPalette('Alice')); + expect(avatarPalette('Alice')).toBe(avatarPalette('Ali' + 'ce')); + expect(avatarPalette('Zoë Müller')).toBe(avatarPalette('Zoë Müller')); }); it('returns a real palette entry (not neutral) for a non-empty name', () => { expect(avatarPalette('Bob')).toMatch(/bg-(blue|purple|green|amber|rose|teal)-100/); }); + + it('maps different names to different palette entries', () => { + // Without this, `name => name ? PALETTE[0] : NEUTRAL` (i.e. the hash loop deleted) + // passes every other test in this file — the palette would be a constant and every + // avatar in the app would render the same colour. + expect(avatarPalette('Alice')).not.toBe(avatarPalette('Bob')); + }); + + it('spreads names across the whole palette, not just one bucket', () => { + const names = [ + 'Alice', 'Bob', 'Carol', 'Dave', 'Erin', 'Frank', 'Grace', 'Heidi', + 'Ivan', 'Judy', 'Mallory', 'Niaj', 'Olivia', 'Peggy', 'Rupert', 'Sybil', + 'Trent', 'Victor', 'Walter', 'Xena' + ]; + const distinct = new Set(names.map((n) => avatarPalette(n))); + // 6 colours in the palette; 20 names must land on more than a couple of them. This + // catches a hash that collapses (e.g. always returns index 0, or ignores all but the + // first character in a way that clusters). + expect(distinct.size).toBeGreaterThanOrEqual(4); + }); }); describe('initials', () => { diff --git a/frontend/src/routes/feed/+page.svelte b/frontend/src/routes/feed/+page.svelte index 503b276..85774fc 100644 --- a/frontend/src/routes/feed/+page.svelte +++ b/frontend/src/routes/feed/+page.svelte @@ -214,6 +214,21 @@ onSseEvent('new-upload', (data) => { try { const upload: FeedUpload = JSON.parse(data); + // GRID view must NOT prepend live. Its rows are POSITIONAL windows + // (`uploads.slice(i * COLS, …)` in VirtualFeed), so inserting at the head shifts + // every tile by one slot: each row's keyed `{#each}` then sees a different set of + // ids and Svelte DESTROYS AND RECREATES the tile nodes. Two consequences at a + // party, where photos arrive continuously — a tap in flight is swallowed when its + // node is torn out, and the photo under the user's finger silently becomes a + // DIFFERENT photo, so they like or open one they never chose. + // + // The "neue Beiträge" pill already exists for exactly this: buffer, and let the + // user pull the new photos in when they are not mid-tap. List view is keyed by id + // at the top level and anchored, so its nodes survive a prepend — it stays live. + if (viewMode === 'grid') { + feedStale = true; + return; + } uploads = [upload, ...uploads]; } catch { /* ignore */ } }), diff --git a/frontend/src/routes/host/+page.svelte b/frontend/src/routes/host/+page.svelte index ff9d9b6..4d2fa7a 100644 --- a/frontend/src/routes/host/+page.svelte +++ b/frontend/src/routes/host/+page.svelte @@ -615,39 +615,56 @@ {:else if exportReady}

Keepsake ist bereit. - - - - Zum Download - + Zum Download

{:else}

Keepsake-Erstellung fehlgeschlagen.

- {/if} + + + {/if}