5f702f2b401e1503649b609e37555dfb1fa455a0
10 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
fabi
|
0c0eed885a |
fix(audit-1): media gating, per-user limits, moderation UI, upload integrity
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
|
||
|
fabi
|
ee554e7f38 |
style(backend): rustfmt the whole tree; gate cargo fmt --check in CI
The backend had never been run through rustfmt. Doing it in one mechanical pass (134 files) so no future functional diff is buried under formatting churn, then gating `cargo fmt --check` in checks.yml so it stays clean. Formatting only — no logic, SQL, or behaviour changed. Verified after the reformat: cargo test 56 passed, clippy --all-targets -D warnings clean, cargo fmt --check clean. This is the deferred cleanup noted when CI's Format step was first left out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
fabi
|
e5201a9889 |
test(backend): DB-backed tests for the risky SQL; test isolation; clippy cleanup
All 40 backend tests were pure-function tests — not one line of SQL ran under `cargo test`,
even though the riskiest code in the repo is SQL. Add 12 DB-backed `#[sqlx::test]` tests
(fresh throwaway DB per test, real migrations) pinning the invariants that code is
load-bearing for, each mutation-verified to fail on broken code:
export_epoch.rs (8): epoch is post-increment on release (a worker born pre-increment is
inert); epoch strictly monotonic across reopen; a retired-epoch worker's writes are
no-ops; export_current is EXACTLY the invariant (8-case table); the ViewerOnly
carry-forward carries a done ZIP forward AND matches nothing when the ZIP is unfinished
(the
|
||
|
fabi
|
41ac742af8 |
test+docs: tighten H1 assertions, cover secret/XFF edges, document boot & SSE-ban
Re-review follow-ups (non-blocking quality items): Tests: - moderation H1 specs now assert exact `→ 403` instead of bare .rejects.toThrow(), so a spurious 500 can no longer masquerade as "revocation worked". - config: added case-insensitivity (upper/mixed-case placeholder) and the len==32/31 boundary cases for validate_secrets. - rate_limiter: added the trailing-comma empty-entry case for client_ip (must fall back, not return ""). (Backend unit tests: 35 pass.) Docs: - README: note that with APP_ENV=production a placeholder .env makes the app refuse to boot and Caddy wait unhealthy — reason is in `docker compose logs app`. - SECURITY-BACKLOG + sse.rs comment: document that a mid-session ban does not tear down an already-open SSE stream (new tickets are blocked; low blast radius). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
fabi
|
f08d858281 |
fix(security): high — live authz, XFF, SSE buffering, atomicity, pagination
H1: auth extractor re-reads the live user row — trusts the DB role (not the
JWT claim) and rejects banned users mid-session. e2e proves a demoted
host loses powers and a banned host is locked out and can't self-unban.
H2: client_ip takes the right-most XFF hop (the one Caddy appends); spoofed
left-most entries are ignored, restoring IP-based throttles.
H3: Caddy excludes /api/v1/stream from `encode` so SSE isn't buffered.
H4: upload — quota increment + row insert + hashtag links now one txn.
H5: feed keyset pagination tiebroken on (created_at, id) + composite index
(migration 010); feed_delta bounded with LIMIT.
H7: LightboxModal keys its comment load off upload.id, ending the
SSE-driven refetch storm.
H8: CSP via SvelteKit kit.csp (svelte.config.js) hardens the localStorage
token model against injection.
Migration 010 also adds the latent comment/comment_hashtag indexes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
fabi
|
723a492d44 |
test(unit): add a unit-test layer (backend cargo + frontend vitest)
The suite was almost entirely e2e; pure logic had no direct coverage. Add fast, DB-free unit tests on both sides. Backend (cargo test — inline #[cfg(test)] modules): - rate_limiter: allow-up-to-max-then-block, per-key independence, sliding-window expiry, retry-after bounds, clear(), and client_ip X-Forwarded-For parsing (first entry / whitespace-trim / fallback). - sse_tickets: single-use consume (replay → None), unknown ticket, uniqueness + hex shape, prune keeps fresh tickets, and an expired ticket (injected past-TTL entry) consumes to None. - hashtag: fill the boundary gaps the 3 existing tests missed — stop-at-non-word, the 40-char cap (drops, doesn't truncate), no-dedup contract, and the German umlaut truncation limitation (pinned so a future Unicode fix is deliberate). Frontend (Vitest — new, standalone config that stubs $app/environment so server-safe module paths import cleanly under node): - avatar: avatarPalette (neutral for empty, deterministic, real palette entry) and initials (?, single word, two words, whitespace collapse). - data-mode-store: pickMediaUrl across original/saver modes and the preview→thumbnail→original fallback chain. - `npm run test:unit` script added. Backend: 20 passing. Frontend: 11 passing. svelte-check: 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
MechaCat02
|
05f76514a2 |
fix(backend): JWT jti, NUL-byte guard, dev-only truncate endpoint
Two bugs surfaced while running the new E2E suite, plus a small test hook: - jwt.rs: add a per-token `jti: Uuid` claim. Without it, two `create_token` calls in the same wall-clock second for the same (sub, role, event_id) produced identical JWT bytes — and identical sha256(token) hashes — which then collided on `session.token_hash UNIQUE` with a 500. Manifests in real use when an admin clicks "Anmelden" twice fast. - auth/handlers.rs: reject display names containing 0x00. Postgres rejects NUL in TEXT columns with `invalid byte sequence for encoding "UTF8"` and the request leaks back as a 500. Now returns 400 with a clean message. - handlers/test_admin.rs + main.rs: new POST /api/v1/admin/__truncate route, compiled in always but only **registered** when EVENTSNAP_TEST_MODE=1 is set on startup. Truncates every event-scoped table, reseeds config from migration defaults, wipes media on disk, and clears the in-memory rate limiter. RequireAdmin-gated so it's not anonymous even in test mode. In production builds (no env var) the route returns 404 — verified by the startup log message. - services/rate_limiter.rs: add `clear()` so the truncate handler can wipe the in-memory window map between tests. - Dockerfile: bump rust:1.87 → rust:1.88 (current dep tree needs it) and COPY ./migrations into the build context so the `sqlx::migrate!()` macro can resolve at compile time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
MechaCat02
|
141c918dd5 |
backend(infra): shared config helper, startup recovery, periodic maintenance
Foundations for the v0.16 features. No new endpoints here — those land in
the next commit on top of these.
- migrations 008 + 009: commit the load-bearing compression_status column
that was uncommitted on disk; add 009_feature_toggles seeding the master
+ per-endpoint rate-limit switches, the master + per-area quota switches,
and the admin-editable privacy_note.
- services/config.rs (new): get_str / get_i64 / get_usize / get_f64 / get_bool
consolidating the scattered helpers that lived in three handlers.
- services/maintenance.rs (new):
- startup_recovery() — resets compression_status='processing' and
export_job.status='running' rows orphaned by a previous crashed
instance, so users never see permanent "Wird vorbereitet…" spinners.
- spawn_periodic_tasks() — hourly cleanup of expired sessions (rows
were never pruned) + rate-limiter HashMap pruning (windows kept one
entry per IP forever).
- services/jobs.rs (new sketch): BackgroundJob trait + JobContext for
future jobs to plug into the same progress + SSE pipeline as
compression/export. Not wired yet — codifies the convention.
- services/compression.rs: 120s hard timeout + kill_on_drop on ffmpeg
so a malformed video can't hang and leak a worker semaphore permit.
- services/rate_limiter.rs: new prune() called from the periodic task.
- state.rs: SseEvent::new() constructor so event-type strings stay
consistent instead of being typed inline at every emit site.
- models/user.rs: UserRole::as_str() for /me/context serialization.
- models/upload.rs: soft_delete() now runs in a transaction and
decrements the uploader's total_upload_bytes (GREATEST(0, …) guard) —
fixes a quota drift where deleting reclaimed no quota.
- Cargo.toml + Cargo.lock: add `infer = "0.15"` (multipart MIME sniffing
used by the upload handler).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
MechaCat02
|
de0e395a9e |
feat: auto-retry uploads when rate limited (v0.13.0)
Backend: rate limiter gains check_with_retry() returning seconds until the next slot opens. Upload 429 responses include retry_after_secs in JSON and a Retry-After header. Frontend: upload queue catches 429 as RateLimitError, resets affected item to pending, schedules processQueue() for the server-reported delay, and shows a live countdown banner in the queue UI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
MechaCat02
|
989d88022a |
feat: implement rate limiting across all API endpoints
Add sliding-window in-memory RateLimiter service (Arc<Mutex<HashMap>>) with per-IP and per-user-id limits on all public endpoint classes: - POST /api/v1/join: 5/min per IP - GET /api/v1/feed: configurable per IP (feed_rate_per_min, default 60) - POST /api/v1/upload: configurable per user (upload_rate_per_hour, default 10) - GET /api/v1/export/zip|html: configurable per IP (export_rate_per_day, default 3) Limits are hot-reloadable via the config table. All 429 responses use German error messages. Client IP is read from X-Forwarded-For (Caddy). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |