From 0c0eed885a1191afb7c94d9a4a7fdf0778b743da Mon Sep 17 00:00:00 2001 From: fabi Date: Tue, 28 Jul 2026 19:14:37 +0200 Subject: [PATCH] fix(audit-1): media gating, per-user limits, moderation UI, upload integrity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First audit round: a blocker and eight high-severity findings across the media access gate, the guest-facing rate limiters, host moderation, and the upload pipeline — plus the backup commands and the e2e/production divergences that hid several of them. Squashed from 7 commits, original messages preserved below. ──────── fix(export): let the keepsake download through X-Frame-Options on iOS The keepsake download navigates a hidden, same-origin iframe (deliberately: a top-level navigation to a 404/429 would unload the PWA). Caddy stamped a site-wide `X-Frame-Options: DENY` that also covered the proxied `/api/*`. Blink hands a `Content-Disposition: attachment` response to the download manager at the network layer, so Chromium never noticed. WebKit enforces XFO on the frame navigation first and aborts the load — so on iOS Safari, the app's primary platform, tapping Download did nothing at all, silently. Carve the two export endpoints out to SAMEORIGIN, which still blocks cross-origin framing. Implemented as two disjoint matchers rather than an override: Caddy applies the FIRST `header` directive outermost, so it wins on write and a later, more specific `header` is silently ignored (verified against the running test stack). Also close the test gap that let this ship: - `06-export` ran on chromium-desktop only; add it to `webkit-iphone`, the only engine that enforces XFO on the download frame. - No test in the suite ever clicked a download button — every archive assertion used Node `fetch`, which has no frame and no XFO enforcement. Add a spec that clicks it and awaits a real `download` event. Verified falsifiable: with the blanket DENY reinstated it fails and reports the WebKit refusal as the cause. - Fix `ExportPage`'s card-scoped locators, which matched nothing: the cards carry `class="card p-5"` (a Tailwind `@apply` component class), never the `rounded-xl` the page object looked for. This had left the "shows enabled download buttons" test red on main. ──────── fix(media): close the percent-escape bypass of the media gate `/media/%70reviews/{id}.jpg` served a taken-down photo to anyone, unauthenticated. Verified against the running stack: the literal path 404s, the escaped one returned 200 with the full image. Same for displays, thumbnails and originals, and any escaped byte in any position works. Cause: the block was four `nest_service("/media/previews", 404)` route matches sitting above a `ServeDir` on `/media`. axum matches on the RAW path (matchit does no percent-decoding), while `ServeDir` percent-decodes when it resolves the file. So `%70reviews` missed every blocker, fell through to the ServeDir, and was decoded back to `previews/` on disk — reaching the bytes with no soft-delete and no ban-hide check. That defeats a host takedown, which is the entire point of the gate. Remove the `/media` route tree outright instead of racing the decoder. Nothing needs it: every media URL the backend emits is already a gated `/api/v1/upload/{id}/{original,preview,display,thumbnail}` alias (handlers::feed), the frontend contains zero `/media/` references, and the `/media` in config.rs/disk.rs is the filesystem path while `media/` in export.rs is a path inside the zip. `/media/**` now 404s regardless of encoding. The route's own comment already said it "serves nothing" — it wasn't a backstop, it was the vector. Caddy keeps proxying /media/* deliberately: the app 404s it, and forwarding means the e2e gating specs exercise the app's refusal exactly as production would rather than being masked by the SvelteKit 404 page. Extend the gating spec with the encoded variants — asserting only the literal spelling is what let this sit undetected. ──────── fix(rate-limit): key the guest-facing limiters per user, not per IP At a venue every guest is behind one NAT, so an IP-keyed limiter hands the whole party a single bucket. On a fresh deploy 12 guests arriving together meant 5 joined and 7 were turned away, with no Retry-After telling them when to retry. `/feed` (60/min) and `/export` (3 per DAY — the fourth guest to fetch their keepsake locked out until tomorrow) had the same defect. `feed_delta` was already keyed per-user and its comment states the exact rationale ("so one client can't starve others behind a shared NAT"); this makes its siblings match. - feed:{ip} -> feed:{user_id} (auth was already in scope) - export:{ip} -> export:{user_id} (resolved from the download ticket's session, which was previously looked up and discarded) - join:{ip}: pre-auth, so there is no user to key on. Split in two — a loose per-IP ceiling that only bounds raw volume (new `join_ip_rate_per_min`, default 60, migration 017), plus the real 5/60s anti-spam bucket keyed per (ip, name), mirroring the existing `recover:{ip}:{name}`. admin_login / recover / pin_reset_req stay IP-keyed on purpose and are now commented as such: they guard credential guessing, where a per-user or per-name key would just hand an attacker a fresh bucket per guess. Retry-After: the machinery existed but 7 of 8 sites called `check()` and hard-coded `None`, so a throttled client was told to back off but never for how long. Delete the bool `check()` wrapper entirely so `check_with_retry` is the only entry point and the delay cannot be discarded by accident. Also surface it for the PIN lockout, where the deadline was already known. Fix the "unknown" fallback while here: every client_ip() caller passed that literal, so any request without X-Forwarded-For — anything reaching the app directly rather than through Caddy — shared ONE global bucket. Serve with connect-info and use the peer address. Tests: the reseed forces every limiter toggle off before each test, which is why this whole class was invisible. Add 01-auth/rate-limit-shared-nat, which enables them and asserts 12 guests share an IP without collision, that one guest hammering their own name IS still throttled (so the fix re-keys rather than removes the limit), and that feed/export buckets are per-user. Retarget the ddos join test at the new per-IP ceiling — it asserted the defect. Also seed `admin_login_rate_enabled` (read by the handler, seeded by no migration and no reseed) and register `join_ip_rate_per_min` in the admin config allowlist. Unrelated pre-existing red test fixed: 01-auth/join asserted a "Willkommen!" heading the wedding redesign removed. ──────── feat(moderation): let a host remove a guest's photo or comment from the UI `DELETE /host/upload/{id}` and `DELETE /host/comment/{id}` were complete on the backend — transactional, SSE-broadcasting, audit-logged — and had zero frontend callers. The feed context sheet offered "Löschen" only when `target.user_id === myUserId`, so the only lever a host actually had against an unwanted photo was banning the uploader. That is both disproportionate and ineffective. A ban doesn't retract what was already posted, and it makes things strictly worse for comments: the ban check runs BEFORE the ownership check on the guest delete route, so banning an abusive author leaves their comment on screen and permanently undeletable by them. With no host affordance, nobody could remove it at all. - feed: hosts/admins get "Beitrag entfernen" on other people's posts, routed to the host endpoint (the guest route 403s anything the caller doesn't own) with moderation-specific confirm copy. Own-post "Löschen" is unchanged. - lightbox: same for comments, via /host/comment/{id}. - Ban semantics are deliberately untouched (USER_JOURNEYS §10 — banned users keep read access and cannot write). The deadlock is broken by giving the host a way in, not by loosening the ban. Live role (this had to come first). `getRole()` decodes the JWT claim, but the token is never reissued — the backend slides the session row forward and treats the DB row as authoritative. The claim is therefore frozen for the token's lifetime: up to 30 days. A guest promoted at the party saw no Host-Dashboard and no moderation actions until they signed out and back in, even though `/me/context` had been returning their real role on every page load and 4 of its 6 call sites dropped the field on the floor. Add `role-store.ts`: seeded from the claim so there's no flash of the wrong nav, then corrected by every `/me/context` response. Point the ad-hoc `getRole()` callers at it (account, upload, host, admin, and the new feed gate). The host and admin dashboards now derive `myRole` reactively, so a demotion disables their controls immediately instead of at next login. Tests: 04-host/moderation-ui drives the real UI — host removes a guest photo and it's gone from /feed server-side; a plain guest is offered nothing on someone else's post (the mirror that keeps the first test honest); a promoted guest gains the dashboard on reload while their token still carries `role: guest`; and a host removes the comment of an already-banned guest, asserting first that the author's own delete 403s so the deadlock is real. ──────── fix(upload): stop destroying originals, apply EXIF orientation, surface rejections Three defects in the same pipeline, each of which loses a photo or misrepresents one. 1. A transient error destroyed the guest's only copy. `process`'s error arm unconditionally `remove_file`d the original. Every failure routed there: `create_dir_all`, both derivative `save_with_format` calls (disk full is the canonical case, and it arrives exactly when many guests upload at once), a panic inside the image codec, or a momentary DB-pool exhaustion. The row is only SOFT-deleted, so the bytes were the sole unrecoverable part — and they were the part we deleted. The author already knew this was wrong next door: `backfill_missing_display` says it "must NEVER soft-delete an upload that already has a working preview". Retry up to 3 times with backoff (re-checking the e2e generation guard after each sleep), and on final failure keep the refund + soft-delete but leave the original on disk, logging its path. A failed upload is now recoverable instead of gone. 2. Every portrait photo was stored sideways. Phones don't rotate sensor data — they record the camera orientation in EXIF and store the pixels as shot. `decode()` returns those raw pixels and the JPEG re-encode writes no EXIF, so the 800px preview, the 2048px diashow display and the keepsake were all rotated 90°, while "Original anzeigen" rendered upright because the original keeps its tag. That asymmetry is why it reads as a viewer bug. There was no EXIF handling anywhere in the repo and no exif crate. Read the tag via `into_decoder()` (which carries the decode Limits through, so the decompression-bomb cap is untouched) and apply it. Missing/malformed tags fall back to NoTransforms — most images have none. Existing derivatives are already baked wrong, so migration 018 adds `derivatives_rev` and `backfill_missing_display` becomes `backfill_stale_derivatives`: it now also picks up anything below the current rev and regenerates it once from the original, which still carries its EXIF. Videos are marked current in the migration — ffmpeg already honours the rotation matrix. Bump DERIVATIVES_REV for any future change that invalidates derivatives. 3. A rejected upload vanished without a word. `UploadQueue.svelte` — 162 lines holding the ONLY renderer of an item's error text, the only "Erneut" retry button and the only rate-limit countdown — was never imported anywhere, so `retryItem`, `removeItem` and `clearCompleted` were unreachable at runtime. On a terminal rejection the store purged the blob and wrote a clear German reason into `entry.error` "so the UI shows a clear reason". There was no such UI. And `uploadBadgeCount` counted only pending/uploading, so the badge decremented exactly as if the upload had succeeded. Mount the queue on /upload, toast the reason immediately (the flow sends the user to /feed straight after staging, so the list alone would still miss them), and count blocked/error in the badge so a failure can't read as success. Tests: 02-upload/exif-orientation uploads a 40x20 fixture tagged Orientation=6 and asserts both derivatives come back PORTRAIT, with a sanity check that the source really is stored landscape. 02-upload/rejection-visible bans the uploader between staging and sending, then asserts the toast, the queue row with the server's reason, and that the item is still counted. Note: 02-upload/quota's 4 failures are pre-existing and unrelated — see the next commit. ──────── docs(backup): make the backup commands work; fix the e2e/prod divergences Backup. Both documented commands failed on the shipped stack, and the sentence explaining them was wrong too: - `pg_dump $DATABASE_URL` — `DATABASE_URL` is only ever in the compose environment, never an operator's shell, and it points at `db:5432`, which is compose-internal DNS. The app image has no postgres client either. - `> /media/backups/…` — `/media` is a named volume mounted inside the app container, not a host path, and nothing ever creates a `backups` subdirectory. - `rsync /opt/eventsnap/media/` — that path does not exist anywhere. - "a single path to back up" — false, and dangerously so: exports were moved to their own `exports_data` volume precisely so a keepsake (which contains every photo in the event) can't be served off the media tree. Backing up only `media_data` silently loses every generated keepsake. Rewritten as three commands — db via `docker compose exec -T db pg_dump`, and one `docker run … tar` per volume — all verified against the running stack. The volume mounts use `/src`, not `/media`: I hit the footgun while testing this. Docker pre-populates an EMPTY volume from the image's own directory and chowns it to match, so `-v media_data:/media alpine` tars alpine's cdrom/floppy/usb, writes them into the volume, and leaves it root-owned so the non-root app can no longer write. Mounting where the image has nothing avoids all of it. Documented inline so the next person doesn't rediscover it. Also correct the architecture notes: `/media/*` no longer routes to the backend (that static tree was removed as a gating bypass), and `exports_data` was missing from the volume list — the one volume an operator most needs to know about. e2e stack: add the `EXPORT_PATH` + `/exports` volume it was missing. The file says "mirrors production layout"; without these, exports landed on the container's writable layer at the default path, so export-leak and export-video wrote real archives into ephemeral storage and the "exports live outside media" invariant was never actually exercised. Pre-existing red test, unrelated to the audit: all four 02-upload/quota tests have been failing since 4464147 "stop /me/quota leaking raw disk to guests" (2026-07-19), which post-dates the spec's last edit. `setLimitTo` calibrated `quota_tolerance` from `free_disk_bytes` read through the GUEST's token — a field that commit deliberately zeroes for non-staff. Dividing by it yields a NaN tolerance, so every test in the block died in the helper. Read the calibration inputs through a staff token and keep reading the ceiling back through the guest, whose limit is the thing under test. ──────── test(e2e): document why WebKit can't run the client-queue upload tests Three 02-upload tests have been failing on `webkit-iphone` on main — `02-upload` was already in that project's testMatch, so this is pre-existing red, not something the audit work introduced. Root cause is the harness, not the app. Playwright's Linux WebKit build cannot store Blobs in IndexedDB at all: `put()` fails with "UnknownError: Error preparing Blob/File data to be stored in object store". Confirmed it is not about how Playwright delivers files — a Blob constructed in-page with `new Blob([bytes])` fails identically, while Chromium stores both that and a `setInputFiles` File without complaint. That breaks every test driving the composer (FAB → UploadSheet → /upload → submit), because `handleSubmit` awaits `addToQueue`, which persists the file before it can navigate. The symptom is a submit button stuck on "Wird hochgeladen…" and a timeout waiting for /feed — which reads like an app hang and cost real time to run down. Skip those four (the three above plus the new rejection-visible) on WebKit only, behind a named helper carrying the full explanation, so the next person gets the answer instead of the investigation. Deliberately narrow: WebKit still runs every API-driven upload test, all of 01-auth, 03-feed and 06-export — including the keepsake download, which only WebKit can meaningfully verify. Chromium continues to run all 14. Worth being explicit, since these tests exist to protect iOS: real Safari supports Blobs in IndexedDB, so this is NOT evidence that the offline upload queue is broken on the platform. It does mean that guarantee is currently unverifiable in CI and rests on Chromium coverage plus manual device testing. Co-Authored-By: Claude Opus 5 --- Caddyfile | 31 +++- PROJECT.md | 19 ++- README.md | 51 +++++- backend/migrations/017_join_ip_rate.down.sql | 1 + backend/migrations/017_join_ip_rate.up.sql | 18 +++ .../migrations/018_derivatives_rev.down.sql | 2 + backend/migrations/018_derivatives_rev.up.sql | 18 +++ backend/src/auth/handlers.rs | 86 +++++++--- backend/src/handlers/admin.rs | 43 ++--- backend/src/handlers/feed.rs | 22 +-- backend/src/handlers/test_admin.rs | 24 ++- backend/src/main.rs | 67 ++++---- backend/src/models/upload.rs | 15 ++ backend/src/services/compression.rs | 145 +++++++++++++---- backend/src/services/rate_limiter.rs | 59 ++++--- e2e/Caddyfile.test | 9 +- e2e/docker-compose.test.yml | 10 ++ e2e/fixtures/db.ts | 21 +++ e2e/helpers/webkit.ts | 29 ++++ e2e/page-objects/export-page.ts | 13 +- e2e/playwright.config.ts | 14 +- e2e/specs/01-auth/join.spec.ts | 6 +- .../01-auth/rate-limit-shared-nat.spec.ts | 144 +++++++++++++++++ e2e/specs/02-upload/burst-queue.spec.ts | 5 + e2e/specs/02-upload/exif-orientation.spec.ts | 80 ++++++++++ e2e/specs/02-upload/gallery-path.spec.ts | 3 + e2e/specs/02-upload/quota.spec.ts | 22 ++- e2e/specs/02-upload/rejection-visible.spec.ts | 87 ++++++++++ e2e/specs/04-host/moderation-ui.spec.ts | 149 ++++++++++++++++++ e2e/specs/06-export/download-iframe.spec.ts | 103 ++++++++++++ e2e/specs/07-adversarial/ddos.spec.ts | 12 +- e2e/specs/07-adversarial/media-gating.spec.ts | 17 ++ .../src/lib/components/LightboxModal.svelte | 17 +- frontend/src/lib/event-state-store.ts | 4 + frontend/src/lib/role-store.ts | 34 ++++ frontend/src/lib/ui-store.ts | 16 +- frontend/src/lib/upload-queue.ts | 6 + frontend/src/routes/+layout.svelte | 4 + frontend/src/routes/account/+page.svelte | 16 +- frontend/src/routes/admin/+page.svelte | 6 +- frontend/src/routes/feed/+page.svelte | 49 ++++-- frontend/src/routes/host/+page.svelte | 6 +- frontend/src/routes/upload/+page.svelte | 15 +- 43 files changed, 1276 insertions(+), 222 deletions(-) create mode 100644 backend/migrations/017_join_ip_rate.down.sql create mode 100644 backend/migrations/017_join_ip_rate.up.sql create mode 100644 backend/migrations/018_derivatives_rev.down.sql create mode 100644 backend/migrations/018_derivatives_rev.up.sql create mode 100644 e2e/helpers/webkit.ts create mode 100644 e2e/specs/01-auth/rate-limit-shared-nat.spec.ts create mode 100644 e2e/specs/02-upload/exif-orientation.spec.ts create mode 100644 e2e/specs/02-upload/rejection-visible.spec.ts create mode 100644 e2e/specs/04-host/moderation-ui.spec.ts create mode 100644 e2e/specs/06-export/download-iframe.spec.ts create mode 100644 frontend/src/lib/role-store.ts diff --git a/Caddyfile b/Caddyfile index a8bebb8..138ea14 100644 --- a/Caddyfile +++ b/Caddyfile @@ -9,17 +9,31 @@ header { Strict-Transport-Security "max-age=31536000; includeSubDomains" X-Content-Type-Options "nosniff" - X-Frame-Options "DENY" Referrer-Policy "strict-origin-when-cross-origin" } + # X-Frame-Options: DENY everywhere EXCEPT the keepsake download endpoints, which + # are navigated in a HIDDEN, SAME-ORIGIN iframe so a 404/429 can't unload the PWA + # (see frontend/src/routes/export/+page.svelte). WebKit enforces XFO *before* + # honouring Content-Disposition, so a blanket DENY makes the download silently do + # nothing on iOS Safari — the app's primary platform. SAMEORIGIN still blocks + # cross-origin framing. + # + # Split into two disjoint matchers rather than an override: Caddy applies the + # FIRST header directive outermost, so it wins on write — a later, more specific + # `header` would be silently ignored. + @framable path /api/v1/export/zip /api/v1/export/html + @not_framable not path /api/v1/export/zip /api/v1/export/html + header @framable X-Frame-Options "SAMEORIGIN" + header @not_framable X-Frame-Options "DENY" + # SvelteKit frontend — static assets with long-lived cache (content-hashed filenames) @hashed_assets path_regexp hashed /_app/immutable/.*\.[a-f0-9]{8,}\.(js|css|woff2)$ header @hashed_assets Cache-Control "public, max-age=31536000, immutable" - # Preview/thumbnail images. These are now served by the app through a - # visibility-checked alias (/api/v1/upload/{id}/{preview,thumbnail}) so moderation - # can revoke access; direct /media/previews|thumbnails is 404-blocked at the app. + # Preview/thumbnail images. These are served by the app through a visibility-checked + # alias (/api/v1/upload/{id}/{preview,thumbnail}) so moderation can revoke access; + # the app serves no /media route at all, so there is no direct path to the bytes. # Privately cacheable for a short window (the app sets the same header; this is the # edge carve-out from the blanket no-store below). Kept short so a moderated image # stops being served to a direct-URL holder promptly. @@ -33,7 +47,14 @@ } header @api Cache-Control "no-store" - # Route API and media requests to the Rust backend + # Route API and media requests to the Rust backend. + # + # The app serves no /media route at all (see the note in backend/src/main.rs) — media + # bytes are reachable only through the visibility-checked /api/v1/upload aliases, so + # /media/* forwards to a plain 404. The proxy line is kept deliberately: it means the + # edge faithfully hands /media to the app, so if a future change ever re-introduces a + # static media route the e2e gating specs see it here exactly as production would, + # instead of being masked by the SvelteKit 404 page. reverse_proxy /api/* app:3000 reverse_proxy /media/* app:3000 diff --git a/PROJECT.md b/PROJECT.md index 7095f83..034a906 100644 --- a/PROJECT.md +++ b/PROJECT.md @@ -1133,16 +1133,19 @@ eventsnap/ ### Backup Strategy -```bash -# Daily (e.g. as a separate Compose service or cron on the VPS) -pg_dump $DATABASE_URL | gzip > /media/backups/db_$(date +%Y-%m-%d).sql.gz +Three artefacts in three places: the database, the `media_data` volume +(originals + derivatives), and the **separate** `exports_data` volume. See +[README.md](README.md#backup) for the exact commands. -# Weekly: rsync /media volume to Hetzner Storage Box -rsync -az /opt/eventsnap/media/ \ - user@u123456.your-storagebox.de:backup/eventsnap/ -``` +Everything runs through `docker compose` / `docker run`, because `DATABASE_URL` +and the `/media` and `/exports` paths only exist inside the compose network — +they are not host paths, and `DATABASE_URL` is never exported into an operator's +shell. -The `/media` volume contains originals, previews, thumbnails, generated exports, and DB backups — a single volume to back up. +Export archives are deliberately outside `MEDIA_PATH` (`EXPORT_PATH=/exports`): a +keepsake contains every photo in the event, and keeping it off the media tree is +what stops it being reachable except through the ticket-gated handler. A backup +of the media volume alone silently loses every generated keepsake. --- diff --git a/README.md b/README.md index f4f1751..73d880c 100644 --- a/README.md +++ b/README.md @@ -162,23 +162,58 @@ See [.env.example](.env.example) for the full list with descriptions and default └────────┘ ``` -- `/api/*` and `/media/*` → Rust backend +- `/api/*` → Rust backend - Everything else → SvelteKit frontend (`adapter-node`) -- Named volumes: `postgres_data`, `media_data`, `caddy_data` +- Named volumes: `postgres_data`, `media_data`, `exports_data`, `caddy_data` + +Media is **not** served as static files. Every image goes through a +visibility-checked alias (`/api/v1/upload/{id}/{preview,display,thumbnail,original}`) +so a host takedown or a ban actually revokes access to the bytes. --- ## Backup -```bash -# Database snapshot -pg_dump $DATABASE_URL | gzip > /media/backups/db_$(date +%Y-%m-%d).sql.gz +There are **three** things to back up, and they live in three different places. +`DATABASE_URL` and the container paths (`/media`, `/exports`) are meaningful only +*inside* the compose network — they are not host paths, and `DATABASE_URL` is +never exported into an operator's shell — so every command below runs through +`docker compose` from the repo directory. -# Weekly offsite sync (Hetzner Storage Box or similar) -rsync -az /opt/eventsnap/media/ user@storagebox.example.com:backup/eventsnap/ +```bash +# 1. Database snapshot. Runs pg_dump inside the db container (the app image has no +# postgres client), reading credentials from the compose environment. +mkdir -p ./backups +docker compose exec -T db \ + sh -c 'pg_dump -U "$POSTGRES_USER" "$POSTGRES_DB"' \ + | gzip > ./backups/db_$(date +%Y-%m-%d).sql.gz + +# 2. Uploaded media (originals + derivatives) out of the named volume. +# NOTE the mountpoint is /src, not /media: if the volume is ever empty, Docker +# pre-populates a fresh mount from the image's own directory, and alpine ships a +# /media containing cdrom/floppy/usb. Mounting somewhere the image has nothing +# avoids silently tarring (and polluting the volume with) those. +docker run --rm \ + -v eventsnap_media_data:/src:ro -v "$PWD/backups":/backup \ + alpine tar czf /backup/media_$(date +%Y-%m-%d).tar.gz -C /src . + +# 3. Export archives — a SEPARATE volume (see the security note below). +docker run --rm \ + -v eventsnap_exports_data:/src:ro -v "$PWD/backups":/backup \ + alpine tar czf /backup/exports_$(date +%Y-%m-%d).tar.gz -C /src . + +# Weekly offsite sync of the three artefacts above. +rsync -az ./backups/ user@storagebox.example.com:backup/eventsnap/ ``` -The `/media` volume holds originals, previews, thumbnails, exports, and DB backups — a single path to back up. +Volume names are prefixed with the compose project name — `eventsnap_` if you run +from a directory called `eventsnap`. Confirm yours with `docker volume ls`. + +> **Exports are deliberately NOT under `/media`.** They live on their own +> `exports_data` volume (`EXPORT_PATH=/exports`) because a keepsake archive +> contains every photo in the event; keeping it outside the media tree is what +> stops it being reachable except through the ticket-gated download handler. +> Backing up only the media volume therefore loses every generated keepsake. --- diff --git a/backend/migrations/017_join_ip_rate.down.sql b/backend/migrations/017_join_ip_rate.down.sql new file mode 100644 index 0000000..86d16c5 --- /dev/null +++ b/backend/migrations/017_join_ip_rate.down.sql @@ -0,0 +1 @@ +DELETE FROM config WHERE key IN ('join_ip_rate_per_min', 'admin_login_rate_enabled'); diff --git a/backend/migrations/017_join_ip_rate.up.sql b/backend/migrations/017_join_ip_rate.up.sql new file mode 100644 index 0000000..135e49f --- /dev/null +++ b/backend/migrations/017_join_ip_rate.up.sql @@ -0,0 +1,18 @@ +-- Per-IP flood ceiling for /join, and the `admin_login_rate_enabled` toggle that +-- every prior migration forgot to seed. +-- +-- Rationale: /join was throttled at 5 requests per 60s keyed on the client IP. At a +-- venue every guest is behind one NAT, so the whole party shared a single bucket — +-- 12 guests scanning the QR code within a few seconds meant 5 got in and 7 were +-- turned away. The handler now keys the real anti-spam bucket per (ip, name), the +-- same shape as `recover:{ip}:{name}`, and keeps only a loose per-IP ceiling to bound +-- raw volume. 60/min comfortably covers a whole wedding arriving at once while still +-- capping a flood from a single source. +-- +-- `admin_login_rate_enabled` is read by auth::handlers::admin_login with a code +-- default of `true`, but no migration ever inserted it, so it was invisible to the +-- admin config UI and to the e2e reseed. Seed it explicitly. +INSERT INTO config (key, value) VALUES + ('join_ip_rate_per_min', '60'), + ('admin_login_rate_enabled', 'true') +ON CONFLICT (key) DO NOTHING; diff --git a/backend/migrations/018_derivatives_rev.down.sql b/backend/migrations/018_derivatives_rev.down.sql new file mode 100644 index 0000000..9e2db5a --- /dev/null +++ b/backend/migrations/018_derivatives_rev.down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS idx_upload_derivatives_rev; +ALTER TABLE upload DROP COLUMN IF EXISTS derivatives_rev; diff --git a/backend/migrations/018_derivatives_rev.up.sql b/backend/migrations/018_derivatives_rev.up.sql new file mode 100644 index 0000000..1625e97 --- /dev/null +++ b/backend/migrations/018_derivatives_rev.up.sql @@ -0,0 +1,18 @@ +-- Track which revision of the derivative pipeline produced an upload's preview/display. +-- +-- Rev 1 applies the EXIF orientation tag. Everything generated before it decoded the raw +-- sensor pixels and re-encoded to JPEG (which writes no EXIF), so every portrait phone photo +-- was stored sideways in the feed preview, the diashow display and the keepsake — while the +-- untouched original still rendered upright. +-- +-- Existing rows default to 0 so the startup backfill can find and re-generate them exactly +-- once; bump the constant in services/compression.rs if the pipeline ever changes again. +ALTER TABLE upload ADD COLUMN IF NOT EXISTS derivatives_rev SMALLINT NOT NULL DEFAULT 0; + +-- Only image derivatives are affected — video thumbnails are extracted by ffmpeg, which +-- already honours the rotation matrix. Mark them current so the backfill skips them. +UPDATE upload SET derivatives_rev = 1 WHERE mime_type NOT LIKE 'image/%'; + +CREATE INDEX IF NOT EXISTS idx_upload_derivatives_rev + ON upload (derivatives_rev) + WHERE deleted_at IS NULL; diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index f5ed12a..74f54f0 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -1,11 +1,12 @@ use std::time::Duration; use axum::Json; -use axum::extract::State; +use axum::extract::{ConnectInfo, State}; use axum::http::{HeaderMap, StatusCode}; use chrono::Utc; use rand::Rng; use serde::{Deserialize, Serialize}; +use std::net::SocketAddr; use uuid::Uuid; use crate::auth::jwt; @@ -33,22 +34,32 @@ pub struct JoinResponse { pub async fn join( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result<(StatusCode, Json), AppError> { - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let join_rate_on = config::get_bool(&state.config_cache, "join_rate_enabled", true).await; - if rate_limits_on - && join_rate_on - && !state - .rate_limiter - .check(format!("join:{ip}"), 5, Duration::from_secs(60)) - { - return Err(AppError::TooManyRequests( - "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, - )); + + // Coarse per-IP flood ceiling. `/join` is pre-auth so there is no user to key on, and + // at a venue EVERY guest arrives from one public IP — a tight per-IP bucket meant the + // 6th person through the door was turned away by the 5 ahead of them. So the per-IP + // limit here only bounds raw volume; the real anti-spam bucket is per-name below. + // Cheap enough to run before validation, which keeps a flood of malformed bodies from + // being free. + if rate_limits_on && join_rate_on { + let ip_ceiling = config::get_usize(&state.config_cache, "join_ip_rate_per_min", 60).await; + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("join_ip:{ip}"), + ip_ceiling, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), + Some(retry_after_secs), + )); + } } let display_name = body.display_name.trim(); @@ -66,6 +77,23 @@ pub async fn join( )); } + // Per-guest bucket, keyed like the `recover:{ip}:{name}` limiter below. This carries + // the original 5/60s anti-spam intent, but one guest retrying can no longer consume + // the allowance of everyone else sharing the venue's NAT. + if rate_limits_on && join_rate_on { + let name_key = display_name.to_lowercase(); + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("join:{ip}:{name_key}"), + 5, + Duration::from_secs(60), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), + Some(retry_after_secs), + )); + } + } + let event = Event::find_or_create( &state.pool, &state.config.event_slug, @@ -149,6 +177,7 @@ fn dummy_pin_hash() -> &'static str { pub async fn recover( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result, AppError> { @@ -159,19 +188,19 @@ pub async fn recover( // burn through 3 wrong PINs and lock the victim for 15 minutes — repeated // every 15 minutes, indefinitely. 5 attempts per 15 minutes per (IP, name) // softens that into a real cost. - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let recover_rate_on = config::get_bool(&state.config_cache, "recover_rate_enabled", true).await; if rate_limits_on && recover_rate_on { let name_key = display_name.to_lowercase(); - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("recover:{ip}:{name_key}"), 5, Duration::from_secs(15 * 60), ) { return Err(AppError::TooManyRequests( "Zu viele Versuche. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } @@ -200,9 +229,12 @@ pub async fn recover( // is effectively permanently fragile. if let Some(locked_until) = user.pin_locked_until { if Utc::now() < locked_until { + // The exact deadline is known, so surface it as Retry-After instead of + // making the client guess at the "15 Minuten" in the copy. + let retry_after_secs = (locked_until - Utc::now()).num_seconds().max(1) as u64; return Err(AppError::TooManyRequests( "Zu viele Versuche. Bitte warte 15 Minuten.".into(), - None, + Some(retry_after_secs), )); } // Lockout window expired — wipe the counter and the timestamp. @@ -274,6 +306,7 @@ pub struct AdminLoginResponse { pub async fn admin_login( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result, AppError> { @@ -287,19 +320,23 @@ pub async fn admin_login( // verify) but with no IP-level limit a determined attacker can still mount // a long-running guess campaign. 5 attempts / minute / IP is plenty for // honest typos. - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let admin_rate_on = config::get_bool(&state.config_cache, "admin_login_rate_enabled", true).await; + // Stays keyed by IP on purpose: this guards a single shared credential, so a per-user + // or per-name key would just hand an attacker a fresh bucket per guess. if rate_limits_on && admin_rate_on - && !state - .rate_limiter - .check(format!("admin_login:{ip}"), 5, Duration::from_secs(60)) + && let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("admin_login:{ip}"), + 5, + Duration::from_secs(60), + ) { return Err(AppError::TooManyRequests( "Zu viele Anmeldeversuche. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } @@ -390,22 +427,23 @@ pub struct PinResetRequestBody { /// feed already exposes. pub async fn request_pin_reset( State(state): State, + ConnectInfo(peer): ConnectInfo, headers: HeaderMap, Json(body): Json, ) -> Result { let display_name = body.display_name.trim(); - let ip = client_ip(&headers, "unknown"); + let ip = client_ip(&headers, &peer.ip().to_string()); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; if rate_limits_on { let name_key = display_name.to_lowercase(); - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("pin_reset_req:{ip}:{name_key}"), 3, Duration::from_secs(15 * 60), ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index fb519c1..f29fe36 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -3,13 +3,13 @@ use std::time::Duration; use axum::Json; use axum::extract::{Query, State}; -use axum::http::{HeaderMap, StatusCode}; +use axum::http::StatusCode; use serde::{Deserialize, Serialize}; +use uuid::Uuid; use crate::auth::middleware::RequireAdmin; use crate::error::AppError; use crate::services::config; -use crate::services::rate_limiter::client_ip; use crate::state::AppState; // ── DTOs ───────────────────────────────────────────────────────────────────── @@ -120,6 +120,10 @@ pub async fn patch_config( ("upload_rate_per_hour", true, 1.0, 100_000.0), ("feed_rate_per_min", true, 1.0, 100_000.0), ("export_rate_per_day", true, 1.0, 100_000.0), + // Loose per-IP ceiling on /join. The real anti-spam bucket is per (ip, name); this + // only bounds raw volume from one source, so it must stay well above the size of a + // party arriving at once (see migration 017). + ("join_ip_rate_per_min", true, 1.0, 100_000.0), ("quota_tolerance", false, 0.0, 1.0), ("estimated_guest_count", true, 1.0, 1_000_000.0), ]; @@ -320,25 +324,26 @@ pub async fn export_ticket( } /// Validate a download ticket (single-use) and confirm its session still exists. -async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result<(), AppError> { +/// Resolve a single-use download ticket to the user who minted it. The caller needs the +/// id to key the export rate limit per-user (see `enforce_export_rate`). +async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result { let token_hash = state .sse_tickets .consume(ticket) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; - crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) + let session = crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash) .await .map_err(|e| AppError::Internal(e.into()))? .ok_or_else(|| AppError::Unauthorized("Sitzung nicht gefunden.".into()))?; - Ok(()) + Ok(session.user_id) } pub async fn download_zip( State(state): State, Query(q): Query, - headers: HeaderMap, ) -> Result { - authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, &headers).await?; + let user_id = authenticate_download_ticket(&state, &q.ticket).await?; + enforce_export_rate(&state, user_id).await?; let path = resolve_export_file(&state, "zip", "Der ZIP-Export ist noch nicht verfügbar.").await?; @@ -389,10 +394,9 @@ async fn resolve_export_file( pub async fn download_html( State(state): State, Query(q): Query, - headers: HeaderMap, ) -> Result { - authenticate_download_ticket(&state, &q.ticket).await?; - enforce_export_rate(&state, &headers).await?; + let user_id = authenticate_download_ticket(&state, &q.ticket).await?; + enforce_export_rate(&state, user_id).await?; let path = resolve_export_file(&state, "html", "Der HTML-Export ist noch nicht verfügbar.").await?; @@ -476,21 +480,24 @@ pub async fn export_status( /// Centralised guard for the export rate limit. Same pattern as upload/feed: master /// switch + per-endpoint switch + numeric value, all stored in `config` and read on /// each request. -async fn enforce_export_rate(state: &AppState, headers: &HeaderMap) -> Result<(), AppError> { +async fn enforce_export_rate(state: &AppState, user_id: Uuid) -> Result<(), AppError> { let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let export_rate_on = config::get_bool(&state.config_cache, "export_rate_enabled", true).await; if !(rate_limits_on && export_rate_on) { return Ok(()); } - let ip = client_ip(headers, "unknown"); let limit = config::get_usize(&state.config_cache, "export_rate_per_day", 3).await; - if !state - .rate_limiter - .check(format!("export:{ip}"), limit, Duration::from_secs(86400)) - { + // Keyed per-user. This was the worst of the IP-keyed limiters: 3 downloads per DAY + // shared across every guest behind the venue's public IP, so the fourth person to + // fetch their keepsake was locked out until the next day. + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("export:{user_id}"), + limit, + Duration::from_secs(86400), + ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } Ok(()) diff --git a/backend/src/handlers/feed.rs b/backend/src/handlers/feed.rs index 3382539..2be8f5e 100644 --- a/backend/src/handlers/feed.rs +++ b/backend/src/handlers/feed.rs @@ -2,7 +2,6 @@ use std::time::Duration; use axum::Json; use axum::extract::{Query, State}; -use axum::http::HeaderMap; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -10,7 +9,6 @@ use uuid::Uuid; use crate::auth::middleware::AuthUser; use crate::error::AppError; use crate::services::config; -use crate::services::rate_limiter::client_ip; use crate::state::AppState; #[derive(Deserialize)] @@ -61,21 +59,23 @@ struct FeedRow { pub async fn feed( State(state): State, auth: AuthUser, - headers: HeaderMap, Query(q): Query, ) -> Result, AppError> { - let ip = client_ip(&headers, "unknown"); let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let feed_rate_on = config::get_bool(&state.config_cache, "feed_rate_enabled", true).await; if rate_limits_on && feed_rate_on { let rate_limit = config::get_usize(&state.config_cache, "feed_rate_per_min", 60).await; - if !state - .rate_limiter - .check(format!("feed:{ip}"), rate_limit, Duration::from_secs(60)) - { + // Keyed per-user, exactly like `feed_delta` below: at a venue every guest shares + // one public IP, so an IP key gave the whole party a single 60/min bucket and the + // fastest scroller starved everyone else. + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( + format!("feed:{}", auth.user_id), + rate_limit, + Duration::from_secs(60), + ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } @@ -225,14 +225,14 @@ pub async fn feed_delta( let feed_rate_on = config::get_bool(&state.config_cache, "feed_rate_enabled", true).await; if rate_limits_on && feed_rate_on { let rate_limit = config::get_usize(&state.config_cache, "feed_rate_per_min", 60).await; - if !state.rate_limiter.check( + if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( format!("feed_delta:{}", auth.user_id), rate_limit, Duration::from_secs(60), ) { return Err(AppError::TooManyRequests( "Zu viele Anfragen. Bitte warte kurz und versuche es erneut.".into(), - None, + Some(retry_after_secs), )); } } diff --git a/backend/src/handlers/test_admin.rs b/backend/src/handlers/test_admin.rs index 7e44c7c..c6f4202 100644 --- a/backend/src/handlers/test_admin.rs +++ b/backend/src/handlers/test_admin.rs @@ -13,9 +13,10 @@ use crate::auth::middleware::RequireAdmin; use crate::error::AppError; use crate::state::AppState; -/// Truncates every event-scoped table, wipes media on disk, and reseeds the -/// `config` table from migration defaults. Requires an admin JWT — even with -/// `EVENTSNAP_TEST_MODE=1` it cannot be hit anonymously. +/// Truncates every event-scoped table, wipes media on disk, and reseeds the `config` +/// table: numeric values from the migration defaults, but every feature toggle forced +/// OFF (production seeds them ON — see the note at the reseed below). Requires an admin +/// JWT — even with `EVENTSNAP_TEST_MODE=1` it cannot be hit anonymously. pub async fn truncate_all( State(state): State, RequireAdmin(_auth): RequireAdmin, @@ -40,8 +41,19 @@ pub async fn truncate_all( .execute(&state.pool) .await?; - // Reseed config — mirrors migrations 005, 009 and 015. Kept in sync by hand - // because pulling SQL out of the migration files at runtime is fragile. + // Reseed config. The NUMERIC values mirror migrations 005/015/016; the BOOLEAN + // toggles deliberately do NOT — migration 009 seeds every one of them `true` + // (production), and this forces them `false` so the suite isn't fighting rate limits + // and quotas it isn't testing. + // + // Be aware of what that costs: this runs as an auto-fixture before EVERY test, so no + // test starts from production's config unless it explicitly turns a toggle back on + // (02-upload/rate-limit, 07-adversarial/ddos, 01-auth/rate-limit-nat, …). That blind + // spot is exactly why an entire class of per-IP limiter bugs went unnoticed: the + // limiters were simply off. When adding a limiter or quota, add a spec that enables it. + // + // Kept in sync by hand because pulling SQL out of the migration files at runtime is + // fragile — if you add a config key in a migration, add it here too. sqlx::query( r#"INSERT INTO config (key, value) VALUES ('max_image_size_mb', '20'), @@ -49,6 +61,7 @@ pub async fn truncate_all( ('upload_rate_per_hour', '100'), ('feed_rate_per_min', '60'), ('export_rate_per_day', '3'), + ('join_ip_rate_per_min', '60'), ('quota_tolerance', '0.75'), ('estimated_guest_count', '100'), ('compression_concurrency', '2'), @@ -57,6 +70,7 @@ pub async fn truncate_all( ('feed_rate_enabled', 'false'), ('export_rate_enabled', 'false'), ('join_rate_enabled', 'false'), + ('admin_login_rate_enabled', 'false'), ('quota_enabled', 'false'), ('storage_quota_enabled', 'false'), ('upload_count_quota_enabled', 'false'), diff --git a/backend/src/main.rs b/backend/src/main.rs index fd5b3a8..569ee17 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -2,7 +2,6 @@ use anyhow::Result; use axum::Router; use axum::extract::DefaultBodyLimit; use axum::routing::{delete, get, patch, post}; -use tower_http::services::ServeDir; use tower_http::trace::TraceLayer; use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt}; @@ -45,10 +44,12 @@ async fn main() -> Result<()> { let state = AppState::new(pool.clone(), config.clone()); - // Backfill the big-screen display derivative for image uploads processed before it - // existed (v0.17.x). Fire-and-forget behind the compression semaphore; each upload - // falls back to its original in the diashow until its display is generated. - state.compression.backfill_missing_display().await; + // Regenerate image derivatives an older pipeline produced: the big-screen display for + // uploads processed before it existed (v0.17.x), and anything predating the current + // DERIVATIVES_REV (rev 1 applies the EXIF orientation, without which every portrait + // phone photo is stored sideways). Fire-and-forget behind the compression semaphore; + // originals are never touched, so a failure just retries on the next start. + state.compression.backfill_stale_derivatives().await; // Re-spawn exports for events that were released but whose keepsake never finished // (crash mid-export). Needs the media/export paths + SSE sender, so it runs here @@ -229,45 +230,39 @@ async fn main() -> Result<()> { api }; - // Serve media files from disk - let media_service = ServeDir::new(&config.media_path); - + // NOTE: media is deliberately NOT served over HTTP. + // + // Files live under `media_path` so the compression worker and the export job can read + // them off disk, but nothing may pull them straight from `/media/**` — that bypasses + // the visibility checks (soft-delete + ban-hide) that make a host takedown stick. + // Every legitimate fetch goes through `/api/v1/upload/{id}/{original,preview,display, + // thumbnail}`, which filter via `find_visible_media`; those are the only media URLs the + // backend ever emits (see `handlers::feed`). + // + // This used to be a `ServeDir` on `/media` with four `nest_service` blockers on the + // subtrees above it. That was bypassable: axum routes on the RAW path while `ServeDir` + // percent-decodes afterwards, so `/media/%70reviews/{id}.jpg` missed every blocker, + // fell through to the `ServeDir`, and was decoded back to `previews/` on disk — serving + // a taken-down photo to anyone, unauthenticated. Any single escaped byte worked, in all + // four subtrees. Deleting the route removes the vector outright rather than racing the + // decoder; `/media/**` now 404s regardless of encoding. let router = Router::new() .route("/health", get(|| async { "ok" })) .merge(api) - // Block direct HTTP access to ALL media subtrees. The files live under - // `media_path` (so the compression worker and export can read them off disk) but - // must NOT be pullable straight from `/media/**` — that bypasses the visibility - // checks (soft-delete + ban-hide) in the gated handlers. Every legitimate fetch - // goes through `/api/v1/upload/{id}/{original,preview,thumbnail}`, which filter - // via `find_visible_media`. The more specific nests take precedence over the - // `/media` ServeDir below (which, with all three subtrees blocked, now serves - // nothing — kept as a backstop). - .nest_service( - "/media/originals", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/previews", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/displays", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/thumbnails", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service("/media", media_service) .layer(TraceLayer::new_for_http()) .with_state(state); let listener = tokio::net::TcpListener::bind(("0.0.0.0", config.app_port)).await?; tracing::info!("listening on {}", listener.local_addr()?); - axum::serve(listener, router) - .with_graceful_shutdown(shutdown_signal()) - .await?; + // `into_make_service_with_connect_info` is required by the pre-auth handlers, which + // extract `ConnectInfo` to use the peer address as the rate-limit key when + // X-Forwarded-For is absent. Without it those extractors fail at runtime. + axum::serve( + listener, + router.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(shutdown_signal()) + .await?; Ok(()) } diff --git a/backend/src/models/upload.rs b/backend/src/models/upload.rs index 3c47cb2..bcfb153 100644 --- a/backend/src/models/upload.rs +++ b/backend/src/models/upload.rs @@ -148,6 +148,21 @@ impl Upload { Ok(()) } + /// Stamp which revision of the derivative pipeline produced this row's preview/display, + /// so the startup backfill can find rows generated by an older one exactly once. + pub async fn set_derivatives_rev( + pool: &PgPool, + id: Uuid, + rev: i16, + ) -> Result<(), sqlx::Error> { + sqlx::query("UPDATE upload SET derivatives_rev = $2 WHERE id = $1") + .bind(id) + .bind(rev) + .execute(pool) + .await?; + Ok(()) + } + pub async fn set_thumbnail_path( pool: &PgPool, id: Uuid, diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index eeb8491..ff74358 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; use anyhow::{Context, Result}; +use image::ImageDecoder; use sqlx::PgPool; use tokio::sync::{Semaphore, broadcast}; use uuid::Uuid; @@ -48,6 +49,16 @@ impl CompressionWorker { self.generation.fetch_add(1, Ordering::SeqCst); } + /// How many times `do_process` is attempted before an upload is given up on. The + /// give-up path is user-visible (the photo disappears), so transient infrastructure + /// errors must not reach it. + const MAX_PROCESS_ATTEMPTS: u32 = 3; + + /// Revision of the image-derivative pipeline. Bump this whenever a change makes existing + /// previews/displays wrong, so `backfill_stale_derivatives` regenerates them once on the + /// next start. Rev 1 = EXIF orientation is applied. + const DERIVATIVES_REV: i16 = 1; + /// Spawn a background task to process an uploaded file. pub fn process(&self, upload_id: Uuid, original_path: String, mime_type: String) { let worker = self.clone(); @@ -60,10 +71,35 @@ impl CompressionWorker { if worker.generation.load(Ordering::SeqCst) != born_at { return; } - match worker - .do_process(upload_id, &original_path, &mime_type) - .await - { + // Retry before giving up. Most failures here are transient and self-clearing — + // an ENOSPC spike while several guests upload at once, a momentary DB-pool + // exhaustion, a panic inside the image codec — and the give-up path is + // user-visible data loss, so it is worth a few seconds to avoid entering it. + let mut attempt = 1u32; + let outcome = loop { + match worker + .do_process(upload_id, &original_path, &mime_type) + .await + { + Ok(v) => break Ok(v), + Err(e) if attempt < Self::MAX_PROCESS_ATTEMPTS => { + tracing::warn!( + error = ?e, %upload_id, attempt, + "compression attempt failed; retrying" + ); + tokio::time::sleep(std::time::Duration::from_secs(2u64.pow(attempt))) + .await; + attempt += 1; + // The data may have been reset while we slept (e2e TRUNCATE). + if worker.generation.load(Ordering::SeqCst) != born_at { + return; + } + } + Err(e) => break Err(e), + } + }; + + match outcome { Ok(_) => { tracing::info!("compression completed for upload {upload_id}"); let _ = worker.sse_tx.send(SseEvent { @@ -72,21 +108,31 @@ impl CompressionWorker { }); } Err(e) => { - tracing::error!("compression failed for upload {upload_id}: {e:#}"); - // Auto-cleanup: a failed transcode would otherwise leave a - // permanently broken feed card, silently charge the uploader's - // quota, and orphan the original on disk. Refund + soft-delete - // (one tx, so v_feed excludes it), remove the orphan file, then - // tell the uploader (upload-error toast) and evict the card - // everywhere (upload-deleted, already handled by the feed). + tracing::error!( + "compression failed for upload {upload_id} after {attempt} attempt(s): {e:#}" + ); + // Refund + soft-delete (one tx, so v_feed excludes it) so a failed + // transcode doesn't leave a permanently broken feed card or silently + // charge the uploader's quota. Then tell the uploader (upload-error + // toast) and evict the card everywhere (upload-deleted). + // + // The ORIGINAL IS DELIBERATELY KEPT. This path used to `remove_file` it + // unconditionally, which meant any transient error — a disk-full blip + // while saving a derivative, a pool hiccup, a panic in the image codec — + // irreversibly destroyed the guest's only copy of a photo they can never + // retake. The row is only soft-deleted, so keeping the bytes makes the + // upload fully recoverable; the file is orphaned rather than lost, and + // the path is logged so it can be found. `backfill_stale_derivatives` + // already refuses to destroy data on error for exactly this reason. let _ = Upload::set_compression_status(&worker.pool, upload_id, "failed").await; if let Err(del) = Upload::soft_delete(&worker.pool, upload_id).await { tracing::warn!(error = ?del, %upload_id, "failed to soft-delete after compression failure"); } - let orphan = worker.media_path.join(&original_path); - if let Err(rm) = tokio::fs::remove_file(&orphan).await { - tracing::warn!(error = ?rm, path = %orphan.display(), "failed to remove orphaned original"); - } + tracing::warn!( + %upload_id, + path = %worker.media_path.join(&original_path).display(), + "original retained for recovery after compression failure" + ); let _ = worker.sse_tx.send(SseEvent { event_type: "upload-error".to_string(), data: serde_json::json!({ "upload_id": upload_id, "error": e.to_string() }) @@ -117,6 +163,7 @@ impl CompressionWorker { .await?; Upload::set_preview_path(&self.pool, upload_id, &preview_rel).await?; Upload::set_display_path(&self.pool, upload_id, &display_rel).await?; + Upload::set_derivatives_rev(&self.pool, upload_id, Self::DERIVATIVES_REV).await?; tracing::info!("preview + display generated for upload {upload_id}"); } else if mime_type.starts_with("video/") { let thumb_rel = self.generate_video_thumbnail(upload_id, &original).await?; @@ -172,7 +219,24 @@ impl CompressionWorker { limits.max_image_height = Some(12_000); limits.max_alloc = Some(256 * 1024 * 1024); reader.limits(limits); - let img = reader.decode().context("failed to decode image")?; + + // Apply the EXIF orientation. Phones do not rotate the sensor data — they record + // the physical camera orientation in a tag and store the pixels as shot. `decode()` + // hands back those raw pixels, and the JPEG re-encode below writes no EXIF at all, + // so skipping this stores EVERY portrait photo sideways in the feed preview, the + // 2048px diashow display and the keepsake — while the untouched original still + // renders upright, which is why it looks like a viewer bug rather than a pipeline + // one. `into_decoder` carries the limits set above through to the decoder, so the + // decompression-bomb guard is unaffected. + let mut decoder = reader.into_decoder().context("failed to decode image")?; + // A missing or malformed tag is not a failure: most images simply have none. + let orientation = decoder + .orientation() + .unwrap_or(image::metadata::Orientation::NoTransforms); + let mut img = + image::DynamicImage::from_decoder(decoder).context("failed to decode image")?; + img.apply_orientation(orientation); + let img = img; // Preview: max 800px, preserving aspect ratio (data-saver feed). img.resize( @@ -221,33 +285,41 @@ impl CompressionWorker { )) } - /// One-time backfill: existing image uploads processed before the display derivative - /// existed have a preview but no `display_path`. Regenerate both derivatives for them - /// (decode is cheap and idempotent) and set the path. Unlike the failure path in - /// `process`, a backfill error is logged and skipped — it must NEVER soft-delete an - /// upload that already has a working preview. Fire-and-forget from startup. - pub async fn backfill_missing_display(&self) { + /// Regenerate image derivatives that an older pipeline produced. Fire-and-forget from + /// startup; picks up two cases, both of which leave the ORIGINAL untouched: + /// + /// - uploads processed before the `display` derivative existed (preview but no + /// `display_path`), and + /// - uploads whose derivatives predate `DERIVATIVES_REV` — currently rev 1, which applies + /// the EXIF orientation. Everything generated before it is stored sideways for any + /// portrait phone photo. + /// + /// Unlike the failure path in `process`, a backfill error is logged and skipped — it must + /// NEVER destroy or soft-delete an upload that already has a working preview. + pub async fn backfill_stale_derivatives(&self) { let rows = sqlx::query_as::<_, (Uuid, String, String)>( "SELECT id, original_path, mime_type FROM upload - WHERE display_path IS NULL AND preview_path IS NOT NULL - AND deleted_at IS NULL AND mime_type LIKE 'image/%'", + WHERE deleted_at IS NULL AND mime_type LIKE 'image/%' + AND original_path IS NOT NULL + AND ( + (display_path IS NULL AND preview_path IS NOT NULL) + OR derivatives_rev < $1 + )", ) + .bind(Self::DERIVATIVES_REV) .fetch_all(&self.pool) .await; let rows = match rows { Ok(r) => r, Err(e) => { - tracing::warn!(error = ?e, "display backfill query failed"); + tracing::warn!(error = ?e, "derivative backfill query failed"); return; } }; if rows.is_empty() { return; } - tracing::info!( - "backfilling display derivative for {} upload(s)", - rows.len() - ); + tracing::info!("regenerating derivatives for {} upload(s)", rows.len()); for (id, original_path, mime_type) in rows { let worker = self.clone(); tokio::spawn(async move { @@ -260,12 +332,19 @@ impl CompressionWorker { Ok((preview_rel, display_rel)) => { let _ = Upload::set_preview_path(&worker.pool, id, &preview_rel).await; let _ = Upload::set_display_path(&worker.pool, id, &display_rel).await; - tracing::info!("display backfilled for upload {id}"); + let _ = Upload::set_derivatives_rev( + &worker.pool, + id, + Self::DERIVATIVES_REV, + ) + .await; + tracing::info!("derivatives regenerated for upload {id}"); } Err(e) => { - // Leave the existing preview intact; the diashow falls back to the - // original for this upload until a later successful pass. - tracing::warn!(error = ?e, %id, "display backfill failed; leaving as-is"); + // Leave the existing derivatives and the original intact; this row is + // simply retried on the next start. The rev stays behind, which is the + // marker that it still needs doing. + tracing::warn!(error = ?e, %id, "derivative backfill failed; leaving as-is"); } } }); diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index bc2f247..ca0df1f 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -17,13 +17,14 @@ impl RateLimiter { } } - /// Returns `true` if the request is allowed, `false` if rate-limited. - pub fn check(&self, key: impl Into, max: usize, window: Duration) -> bool { - self.check_with_retry(key, max, window).is_ok() - } - /// Returns `Ok(())` if allowed, `Err(retry_after_secs)` if rate-limited. /// `retry_after_secs` is how long until the oldest slot in the window expires. + /// + /// This is deliberately the ONLY entry point. There used to be a `check()` wrapper + /// returning a plain bool, and 7 of the 8 call sites used it and then hard-coded + /// `None` for the response's `Retry-After` — so a throttled client was told to back + /// off but never for how long. Forcing every caller through the `Result` makes the + /// retry delay impossible to discard by accident. pub fn check_with_retry( &self, key: impl Into, @@ -84,6 +85,11 @@ impl RateLimiter { /// appends is the real client. A client can prepend arbitrary spoofed values to /// the left of XFF to dodge throttles — those are ignored here. This assumes /// exactly one trusted proxy (Caddy); revisit if that changes. +/// +/// Pass the peer address as `fallback`, never a constant. Every caller used to pass +/// the literal `"unknown"`, so any request that arrived without XFF — i.e. anything +/// reaching the app directly rather than through Caddy — shared ONE bucket with every +/// other such request, turning the limiter into a self-inflicted global throttle. pub fn client_ip(headers: &axum::http::HeaderMap, fallback: &str) -> String { headers .get("x-forwarded-for") @@ -104,29 +110,35 @@ mod tests { #[test] fn allows_up_to_max_then_blocks() { let rl = RateLimiter::new(); - assert!(rl.check("k", 3, MIN)); - assert!(rl.check("k", 3, MIN)); - assert!(rl.check("k", 3, MIN)); - assert!(!rl.check("k", 3, MIN), "the 4th request must be blocked"); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!(rl.check_with_retry("k", 3, MIN).is_ok()); + assert!( + rl.check_with_retry("k", 3, MIN).is_err(), + "the 4th request must be blocked" + ); } #[test] fn keys_are_independent() { let rl = RateLimiter::new(); - assert!(rl.check("a", 1, MIN)); - assert!(!rl.check("a", 1, MIN)); - assert!(rl.check("b", 1, MIN), "a different key has its own window"); + assert!(rl.check_with_retry("a", 1, MIN).is_ok()); + assert!(rl.check_with_retry("a", 1, MIN).is_err()); + assert!( + rl.check_with_retry("b", 1, MIN).is_ok(), + "a different key has its own window" + ); } #[test] fn window_slides_and_allows_again_after_expiry() { let rl = RateLimiter::new(); let w = Duration::from_millis(40); - assert!(rl.check("k", 1, w)); - assert!(!rl.check("k", 1, w)); + assert!(rl.check_with_retry("k", 1, w).is_ok()); + assert!(rl.check_with_retry("k", 1, w).is_err()); std::thread::sleep(Duration::from_millis(55)); assert!( - rl.check("k", 1, w), + rl.check_with_retry("k", 1, w).is_ok(), "the slot should expire once the window passes" ); } @@ -191,10 +203,13 @@ mod tests { #[test] fn clear_resets_every_window() { let rl = RateLimiter::new(); - assert!(rl.check("k", 1, MIN)); - assert!(!rl.check("k", 1, MIN)); + assert!(rl.check_with_retry("k", 1, MIN).is_ok()); + assert!(rl.check_with_retry("k", 1, MIN).is_err()); rl.clear(); - assert!(rl.check("k", 1, MIN), "clear() must free the window"); + assert!( + rl.check_with_retry("k", 1, MIN).is_ok(), + "clear() must free the window" + ); } /// `prune()` is a memory-leak guard: without it a long-lived process keeps one HashMap @@ -216,7 +231,7 @@ mod tests { .insert("stale".to_string(), vec![ancient]); // ...alongside a key that is still inside its window. - assert!(rl.check("live", 5, MIN)); + assert!(rl.check_with_retry("live", 5, MIN).is_ok()); assert_eq!(rl.windows.lock().unwrap().len(), 2); rl.prune(); @@ -239,13 +254,13 @@ mod tests { // prune() dropped live keys, every background sweep would hand attackers a fresh // budget. let rl = RateLimiter::new(); - assert!(rl.check("k", 1, MIN)); - assert!(!rl.check("k", 1, MIN)); + assert!(rl.check_with_retry("k", 1, MIN).is_ok()); + assert!(rl.check_with_retry("k", 1, MIN).is_err()); rl.prune(); assert!( - !rl.check("k", 1, MIN), + rl.check_with_retry("k", 1, MIN).is_err(), "prune() must not clear a window that is still active" ); } diff --git a/e2e/Caddyfile.test b/e2e/Caddyfile.test index 02fb85c..e481ea7 100644 --- a/e2e/Caddyfile.test +++ b/e2e/Caddyfile.test @@ -12,10 +12,17 @@ # Mirror prod's security headers (minus HSTS, which is HTTPS-only). header { X-Content-Type-Options "nosniff" - X-Frame-Options "DENY" Referrer-Policy "strict-origin-when-cross-origin" } + # Mirror prod's export carve-out: the keepsake download targets a hidden + # same-origin iframe, and WebKit enforces XFO before Content-Disposition. + # Two disjoint matchers, not an override — see the comment in ../Caddyfile. + @framable path /api/v1/export/zip /api/v1/export/html + @not_framable not path /api/v1/export/zip /api/v1/export/html + header @framable X-Frame-Options "SAMEORIGIN" + header @not_framable X-Frame-Options "DENY" + reverse_proxy /api/* app:3000 reverse_proxy /media/* app:3000 reverse_proxy /health app:3000 diff --git a/e2e/docker-compose.test.yml b/e2e/docker-compose.test.yml index adf77d5..15a0440 100644 --- a/e2e/docker-compose.test.yml +++ b/e2e/docker-compose.test.yml @@ -42,11 +42,20 @@ services: EVENT_NAME: E2E Test Event APP_PORT: '3000' MEDIA_PATH: /media + # Exports MUST live outside MEDIA_PATH — see the note on the volume below and + # config.rs::validate. Omitting this left exports on the container's writable + # layer at the /exports default, so the test stack diverged from the prod layout + # it claims to mirror, and export-leak/export-video wrote real archives into + # ephemeral storage. + EXPORT_PATH: /exports SESSION_EXPIRY_DAYS: '30' EVENTSNAP_TEST_MODE: '1' # ENABLES /admin/__truncate — never set in prod RUST_LOG: eventsnap_backend=info,tower_http=warn volumes: - media_data:/media + # Separate volume, exactly as in production: a keepsake archive contains every + # photo in the event, so it is kept off the media tree. + - exports_data:/exports expose: - '3000' @@ -75,3 +84,4 @@ services: volumes: media_data: + exports_data: diff --git a/e2e/fixtures/db.ts b/e2e/fixtures/db.ts index 1dafdb0..cdbe6e2 100644 --- a/e2e/fixtures/db.ts +++ b/e2e/fixtures/db.ts @@ -54,6 +54,27 @@ export const db = { ); }, + async compressionStatus(uploadId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ compression_status: string }>( + `SELECT compression_status FROM upload WHERE id = $1`, + [uploadId] + ); + return r.rows[0]?.compression_status ?? null; + }); + }, + + /** Which revision of the derivative pipeline produced this row's preview/display. */ + async derivativesRev(uploadId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ derivatives_rev: number }>( + `SELECT derivatives_rev FROM upload WHERE id = $1`, + [uploadId] + ); + return r.rows[0]?.derivatives_rev ?? null; + }); + }, + async countUploadsForUser(userId: string): Promise { return withClient(async (c) => { const r = await c.query<{ count: string }>( diff --git a/e2e/helpers/webkit.ts b/e2e/helpers/webkit.ts new file mode 100644 index 0000000..9c98cd6 --- /dev/null +++ b/e2e/helpers/webkit.ts @@ -0,0 +1,29 @@ +import { test } from '@playwright/test'; + +/** + * Skip a test that depends on persisting a Blob/File in IndexedDB when running on + * Playwright's WebKit. + * + * The client upload queue (`frontend/src/lib/upload-queue.ts`) stores the file itself in + * IndexedDB so a backgrounded or reloaded phone can resume the upload. Playwright's Linux + * WebKit build cannot store Blobs in IndexedDB at all — `put()` fails with + * "UnknownError: Error preparing Blob/File data to be stored in object store". Verified to + * be the harness, not the app: a Blob constructed in-page with `new Blob([bytes])` fails + * exactly the same way, while Chromium stores both that and a `setInputFiles` File fine. + * Real iOS Safari supports Blobs in IndexedDB, so this is NOT evidence of a bug on the + * platform these tests exist to protect. + * + * Any test that drives the composer (FAB → UploadSheet → /upload → submit) hits this, + * because `handleSubmit` awaits `addToQueue`, which throws before it can navigate. + * + * This is deliberately narrow. WebKit still runs every API-driven upload test, the whole of + * 01-auth, 03-feed and 06-export — including the keepsake download, which only WebKit can + * meaningfully verify. If Playwright's WebKit ever gains IndexedDB Blob support, delete this + * helper and the four call sites. + */ +export function skipIfNoIdbBlobs(browserName: string) { + test.skip( + browserName === 'webkit', + "Playwright's Linux WebKit cannot store Blobs in IndexedDB (harness limitation, not an iOS one) — the client upload queue can't be exercised there" + ); +} diff --git a/e2e/page-objects/export-page.ts b/e2e/page-objects/export-page.ts index d8dce62..687f2f6 100644 --- a/e2e/page-objects/export-page.ts +++ b/e2e/page-objects/export-page.ts @@ -37,12 +37,19 @@ export class ExportPage { /** * The "Download" button inside the card whose heading is `heading`. * - * Scoped to the card element (`div.rounded-xl`) rather than "any div containing the - * heading" — the latter also matches the page wrapper, which contains BOTH cards' buttons. + * Scoped to the card element rather than "any div containing the heading" — the latter + * also matches the page wrapper, which contains BOTH cards' buttons. + * + * The scope class is `div.card` (see the ZIP/HTML cards in + * frontend/src/routes/export/+page.svelte). It was previously `div.rounded-xl`, which + * matched NOTHING: `.card` is a Tailwind `@apply` component class + * (frontend/src/lib/styles/components.css) so the DOM only ever carries `class="card p-5"` + * — and the utility it applies is `rounded-2xl` anyway. Both card-scoped locators were + * therefore dead, which is why the "shows enabled download buttons" test was red. */ private cardButton(heading: string): Locator { return this.page - .locator('div.rounded-xl') + .locator('div.card') .filter({ has: this.page.getByRole('heading', { name: heading, exact: true }) }) .getByRole('button', { name: 'Download', exact: true }); } diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index a39125b..15c6a7e 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -146,8 +146,18 @@ export default defineConfig({ // that on `@smoke` — which exists on exactly two specs — meant the entire iOS guarantee was // one happy path and one join test. Every other UA here is a secondary browser and a smoke // check is proportionate; WebKit is not. Give it the core journeys the guest actually walks: - // join/recover, upload, and browse the feed. - testMatch: ['**/__smoke/**', '**/01-auth/**', '**/02-upload/**', '**/03-feed/**'], + // join/recover, upload, browse the feed — and take the keepsake home. + // + // 06-export is here because WebKit is the ONLY engine that enforces X-Frame-Options on the + // hidden download iframe. Excluding it is what let a site-wide `XFO: DENY` ship a keepsake + // download that silently did nothing on iOS. See 06-export/download-iframe.spec.ts. + testMatch: [ + '**/__smoke/**', + '**/01-auth/**', + '**/02-upload/**', + '**/03-feed/**', + '**/06-export/**', + ], }, { name: 'firefox-android', diff --git a/e2e/specs/01-auth/join.spec.ts b/e2e/specs/01-auth/join.spec.ts index 58a2940..c5eadb9 100644 --- a/e2e/specs/01-auth/join.spec.ts +++ b/e2e/specs/01-auth/join.spec.ts @@ -13,7 +13,11 @@ test.describe('Auth — join flow', () => { const join = new JoinPage(page); await join.goto(); - await expect(page.getByRole('heading', { name: 'Willkommen!' })).toBeVisible(); + // The join form's landing state. There is no "Willkommen!" heading — the wedding + // redesign (f243bfe) split it into a "Willkommen bei" lead-in plus the event name as + // the

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