From 498b4e256b9db4cbfeace3758837f9c3814f1f4f Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 1 Jul 2026 21:25:39 +0200 Subject: [PATCH 1/5] =?UTF-8?q?fix(security):=20critical=20=E2=80=94=20rep?= =?UTF-8?q?air=20PIN=20reset=20+=20enforce=20real=20prod=20secrets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1: reset_user_pin wrote to a non-existent column (pin_failed_attempts); the real column is failed_pin_attempts, so every PIN reset 500'd. Fixed the column name; new e2e (pin-reset.spec.ts) proves a reset returns a usable PIN and the target can recover with it. C2: config.rs::validate_secrets now rejects placeholder-ish secrets (change_me/dev_secret/placeholder), enforces len>=32 in prod, and requires a real ADMIN_PASSWORD_HASH. docker-compose.yml sets APP_ENV=production so the guard actually runs. Corrected the false "fixed" claim in SECURITY-BACKLOG.md. .env.example documents the rule. Riders in these files (documented here since git can't split hunks): - host.rs also carries the event-scoped ban_user fix and the close_event/open_event no-op broadcast guard (medium). - docker-compose.yml also adds ORIGIN (H6), app/frontend healthchecks with Caddy waiting on health, and per-service memory limits (medium). Co-Authored-By: Claude Opus 4.8 --- .env.example | 4 ++ backend/src/config.rs | 103 ++++++++++++++++++++++++---- backend/src/handlers/host.rs | 21 ++++-- docker-compose.yml | 44 +++++++++++- docs/SECURITY-BACKLOG.md | 24 +++++-- e2e/fixtures/api-client.ts | 19 +++++ e2e/specs/04-host/pin-reset.spec.ts | 43 ++++++++++++ 7 files changed, 227 insertions(+), 31 deletions(-) create mode 100644 e2e/specs/04-host/pin-reset.spec.ts diff --git a/.env.example b/.env.example index 614e288..f0203d9 100644 --- a/.env.example +++ b/.env.example @@ -4,6 +4,10 @@ DOMAIN=my-event.example.com # ── App server ──────────────────────────────────────────────────────────────── APP_PORT=3000 +# Set to `production` in real deployments. This activates the secret guard that +# refuses to boot with placeholder JWT_SECRET / ADMIN_PASSWORD_HASH values. +# (docker-compose.yml already sets APP_ENV=production for the app service.) +APP_ENV=production # ── Database ────────────────────────────────────────────────────────────────── # Set a strong password and keep it in sync between DATABASE_URL and diff --git a/backend/src/config.rs b/backend/src/config.rs index 88363fc..bcc2d01 100644 --- a/backend/src/config.rs +++ b/backend/src/config.rs @@ -6,6 +6,47 @@ use anyhow::{anyhow, Context, Result}; /// we refuse to start with this value; otherwise we warn loudly. const DEV_JWT_SECRET_SENTINEL: &str = "dev_secret_do_not_use_in_production_32byteslong_aaaa"; +/// A secret is "placeholder-ish" if it's the shipped dev sentinel or still carries +/// the tell-tale scaffolding substrings from `.env.example`. Length alone is not +/// enough — the shipped `change_me_...` placeholder is >32 chars. +fn looks_placeholder(s: &str) -> bool { + let lower = s.to_ascii_lowercase(); + s == DEV_JWT_SECRET_SENTINEL + || lower.contains("change_me") + || lower.contains("dev_secret") + || lower.contains("placeholder") +} + +/// Enforce secret hygiene. In production every guard is hard-fail: a booting app +/// with a publicly-known signing key is worse than one that refuses to start. +/// Outside production the dev sentinel is tolerated (warned) so local dev is frictionless. +fn validate_secrets(is_prod: bool, jwt_secret: &str, admin_password_hash: &str) -> Result<()> { + if is_prod { + if looks_placeholder(jwt_secret) { + return Err(anyhow!( + "Refusing to start in production with a placeholder JWT_SECRET — \ + rotate it (openssl rand -hex 64)." + )); + } + if jwt_secret.len() < 32 { + return Err(anyhow!("JWT_SECRET must be at least 32 characters.")); + } + if admin_password_hash.is_empty() || looks_placeholder(admin_password_hash) { + return Err(anyhow!( + "Refusing to start in production without a real ADMIN_PASSWORD_HASH — \ + generate one (htpasswd -bnBC 12 '' | tr -d ':\\n')." + )); + } + } else if jwt_secret == DEV_JWT_SECRET_SENTINEL { + tracing::warn!( + "JWT_SECRET is the dev sentinel — fine for local development, NEVER ship this." + ); + } else if jwt_secret.len() < 32 { + return Err(anyhow!("JWT_SECRET must be at least 32 characters.")); + } + Ok(()) +} + #[derive(Clone, Debug)] pub struct AppConfig { pub database_url: String, @@ -25,19 +66,9 @@ impl AppConfig { let is_prod = app_env.eq_ignore_ascii_case("production"); let jwt_secret = std::env::var("JWT_SECRET").context("JWT_SECRET must be set")?; - if jwt_secret == DEV_JWT_SECRET_SENTINEL { - if is_prod { - return Err(anyhow!( - "Refusing to start in production with the well-known dev JWT_SECRET — \ - rotate it (openssl rand -hex 64)." - )); - } - tracing::warn!( - "JWT_SECRET is the dev sentinel — fine for local development, NEVER ship this." - ); - } else if jwt_secret.len() < 32 { - return Err(anyhow!("JWT_SECRET must be at least 32 characters.")); - } + let admin_password_hash = std::env::var("ADMIN_PASSWORD_HASH").unwrap_or_default(); + + validate_secrets(is_prod, &jwt_secret, &admin_password_hash)?; Ok(Self { database_url: std::env::var("DATABASE_URL") @@ -47,8 +78,7 @@ impl AppConfig { .unwrap_or_else(|_| "30".to_string()) .parse() .context("SESSION_EXPIRY_DAYS must be a number")?, - admin_password_hash: std::env::var("ADMIN_PASSWORD_HASH") - .unwrap_or_default(), + admin_password_hash, event_name: std::env::var("EVENT_NAME") .unwrap_or_else(|_| "EventSnap".to_string()), event_slug: std::env::var("EVENT_SLUG") @@ -63,3 +93,46 @@ impl AppConfig { }) } } + +#[cfg(test)] +mod tests { + use super::*; + + const REAL_SECRET: &str = "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6e7f8a9b0c1d2"; + const REAL_HASH: &str = "$2y$12$abcdefghijklmnopqrstuv.wxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012"; + + #[test] + fn prod_rejects_shipped_placeholder_secret() { + // The exact string shipped in `.env` — >32 chars, so it must be caught by + // the substring guard, not the length check. + let err = validate_secrets(true, "change_me_to_a_random_64_byte_hex_string", REAL_HASH); + assert!(err.is_err(), "placeholder JWT_SECRET must be rejected in prod"); + } + + #[test] + fn prod_rejects_dev_sentinel_and_short_secret() { + assert!(validate_secrets(true, DEV_JWT_SECRET_SENTINEL, REAL_HASH).is_err()); + assert!(validate_secrets(true, "tooshort", REAL_HASH).is_err()); + } + + #[test] + fn prod_rejects_missing_or_placeholder_admin_hash() { + assert!(validate_secrets(true, REAL_SECRET, "").is_err()); + assert!(validate_secrets(true, REAL_SECRET, "$2y$12$placeholder_replace_me").is_err()); + } + + #[test] + fn prod_accepts_real_secrets() { + assert!(validate_secrets(true, REAL_SECRET, REAL_HASH).is_ok()); + } + + #[test] + fn non_prod_tolerates_dev_sentinel() { + assert!(validate_secrets(false, DEV_JWT_SECRET_SENTINEL, "").is_ok()); + } + + #[test] + fn non_prod_still_rejects_short_non_sentinel_secret() { + assert!(validate_secrets(false, "tooshort", "").is_err()); + } +} diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index f9ea788..7994c9a 100644 --- a/backend/src/handlers/host.rs +++ b/backend/src/handlers/host.rs @@ -113,10 +113,11 @@ pub async fn ban_user( } sqlx::query( - "UPDATE \"user\" SET is_banned = TRUE, uploads_hidden = $2 WHERE id = $1", + "UPDATE \"user\" SET is_banned = TRUE, uploads_hidden = $2 WHERE id = $1 AND event_id = $3", ) .bind(user_id) .bind(body.hide_uploads) + .bind(auth.event_id) .execute(&state.pool) .await?; @@ -263,7 +264,7 @@ pub async fn reset_user_pin( sqlx::query( "UPDATE \"user\" SET recovery_pin_hash = $1, - pin_failed_attempts = 0, + failed_pin_attempts = 0, pin_locked_until = NULL WHERE id = $2", ) @@ -341,14 +342,18 @@ pub async fn close_event( State(state): State, RequireHost(_auth): RequireHost, ) -> Result { - sqlx::query( + let result = sqlx::query( "UPDATE event SET uploads_locked_at = NOW() WHERE slug = $1 AND uploads_locked_at IS NULL", ) .bind(&state.config.event_slug) .execute(&state.pool) .await?; - let _ = state.sse_tx.send(SseEvent::new("event-closed", "{}")); + // Only broadcast when this call actually flipped the lock — closing an + // already-closed event is a no-op and shouldn't spam listeners. + if result.rows_affected() > 0 { + let _ = state.sse_tx.send(SseEvent::new("event-closed", "{}")); + } Ok(StatusCode::NO_CONTENT) } @@ -357,14 +362,16 @@ pub async fn open_event( State(state): State, RequireHost(_auth): RequireHost, ) -> Result { - sqlx::query( - "UPDATE event SET uploads_locked_at = NULL WHERE slug = $1", + let result = sqlx::query( + "UPDATE event SET uploads_locked_at = NULL WHERE slug = $1 AND uploads_locked_at IS NOT NULL", ) .bind(&state.config.event_slug) .execute(&state.pool) .await?; - let _ = state.sse_tx.send(SseEvent::new("event-opened", "{}")); + if result.rows_affected() > 0 { + let _ = state.sse_tx.send(SseEvent::new("event-opened", "{}")); + } Ok(StatusCode::NO_CONTENT) } diff --git a/docker-compose.yml b/docker-compose.yml index 6919e38..57f1fa7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -14,6 +14,10 @@ services: interval: 5s timeout: 5s retries: 10 + deploy: + resources: + limits: + memory: 512M app: build: @@ -21,6 +25,10 @@ services: dockerfile: Dockerfile restart: unless-stopped env_file: .env + environment: + # Activates the production secret guard in config.rs — refuses to boot with + # placeholder JWT_SECRET / ADMIN_PASSWORD_HASH. + APP_ENV: production depends_on: db: condition: service_healthy @@ -28,6 +36,18 @@ services: - media_data:/media expose: - "3000" + healthcheck: + test: ["CMD-SHELL", "wget -q -O- http://localhost:3000/health || exit 1"] + interval: 10s + timeout: 5s + retries: 5 + start_period: 20s + deploy: + resources: + limits: + # Bounds a runaway ffmpeg transcode (large uploads, 2 workers) so it can't + # OOM the single box and take down Postgres. + memory: 1G frontend: build: @@ -35,10 +55,24 @@ services: dockerfile: Dockerfile restart: unless-stopped env_file: .env + environment: + # adapter-node behind Caddy TLS needs the public origin for CSRF checks on + # POST form actions — without it they fail only in production. + ORIGIN: "https://${DOMAIN}" depends_on: - app expose: - "3001" + healthcheck: + test: ["CMD-SHELL", "wget -q -O- http://localhost:3001/ >/dev/null 2>&1 || exit 1"] + interval: 10s + timeout: 5s + retries: 5 + start_period: 15s + deploy: + resources: + limits: + memory: 256M caddy: image: caddy:2-alpine @@ -50,8 +84,14 @@ services: - ./Caddyfile:/etc/caddy/Caddyfile:ro - caddy_data:/data depends_on: - - app - - frontend + app: + condition: service_healthy + frontend: + condition: service_healthy + deploy: + resources: + limits: + memory: 256M volumes: postgres_data: diff --git a/docs/SECURITY-BACKLOG.md b/docs/SECURITY-BACKLOG.md index f50685b..2282e1e 100644 --- a/docs/SECURITY-BACKLOG.md +++ b/docs/SECURITY-BACKLOG.md @@ -51,15 +51,25 @@ item below is tagged with its **current status in `main`**: ## ✅ Fixed in main since the audit (for the record) -These audit findings are present in `main` today (verified 2026-06-30): event-scoped social -handlers (cross-event authz), server-side MIME/ext allowlist on upload, recovery-PIN lockout -backoff, unspoofable client IP in the rate limiter, effective JWT production-secret guard, DB port -no longer publicly exposed, container healthchecks, bcrypt offloaded via `spawn_blocking`, -bounded compression concurrency (semaphore), bounded feed queries (`LIMIT ≤ 100`), and the -viewport-fit / reduced-motion / aria a11y pass. **New since the audit:** an image-decode -decompression-bomb cap (`image::Limits` 12000×12000 / 256 MiB) ported into +These audit findings are present in `main` today (verified 2026-06-30): server-side MIME/ext +allowlist on upload, recovery-PIN lockout backoff, DB port no longer publicly exposed, bcrypt +offloaded via `spawn_blocking`, bounded compression concurrency (semaphore), bounded feed queries +(`LIMIT ≤ 100`), and the viewport-fit / reduced-motion / aria a11y pass. An image-decode +decompression-bomb cap (`image::Limits` 12000×12000 / 256 MiB) lives in `backend/src/services/compression.rs`. +**Fixed in the 2026-07 review pass** (previously *claimed* fixed here but were not — corrected): +- **Effective JWT production-secret guard** — `APP_ENV=production` is now set for the app service, + and `config.rs::validate_secrets` rejects any placeholder-ish `JWT_SECRET`/`ADMIN_PASSWORD_HASH` + (not just the exact dev sentinel) and enforces `len ≥ 32` unconditionally in prod. +- **Unspoofable client IP in the rate limiter** — `client_ip` now takes the right-most + `X-Forwarded-For` entry (the hop Caddy appends), so a client-supplied left-most value is ignored. +- **Live role/ban re-check** — the auth extractor re-reads the user row and trusts the DB role and + `is_banned` flag rather than the JWT claim, revoking demoted/banned sessions immediately. +- **Container healthchecks** — `app` and `frontend` now have healthchecks and Caddy waits on + `service_healthy`. +- **Event-scoped `ban_user`** — the ban UPDATE is now scoped by `event_id` like its siblings. + ## 🅲 Consciously won't-fix at ~100-guest single-box scale Diminishing returns vs. the deployment's actual threat model. Revisit only if the scale or diff --git a/e2e/fixtures/api-client.ts b/e2e/fixtures/api-client.ts index 0bdde22..c86a61a 100644 --- a/e2e/fixtures/api-client.ts +++ b/e2e/fixtures/api-client.ts @@ -117,6 +117,25 @@ export class ApiClient { }); } + async unbanUser(token: string, userId: string, opts: { expectedStatus?: number | number[] } = {}) { + return this.request('POST', `/host/users/${userId}/unban`, { + token, + expectedStatus: opts.expectedStatus ?? [200, 204], + }); + } + + /** Reset another user's PIN. Returns the plaintext PIN the host must relay once. */ + async resetUserPin( + token: string, + userId: string, + opts: { expectedStatus?: number | number[] } = {} + ): Promise<{ status: number; body: { pin?: string } }> { + return this.request<{ pin?: string }>('POST', `/host/users/${userId}/pin-reset`, { + token, + expectedStatus: opts.expectedStatus ?? [200], + }); + } + async closeEvent(token: string) { return this.request('POST', '/host/event/close', { token, expectedStatus: [200, 204] }); } diff --git a/e2e/specs/04-host/pin-reset.spec.ts b/e2e/specs/04-host/pin-reset.spec.ts new file mode 100644 index 0000000..8feb3b6 --- /dev/null +++ b/e2e/specs/04-host/pin-reset.spec.ts @@ -0,0 +1,43 @@ +/** + * Regression for the review's C1: `reset_user_pin` wrote to a non-existent + * column (`pin_failed_attempts` vs the real `failed_pin_attempts`), so the + * endpoint 500'd every time and never returned a PIN — the feature had never + * worked against a real DB and no test caught it. These specs pin the contract: + * a successful reset returns a fresh 4-digit PIN, and the target can recover + * with it. + */ +import { test, expect } from '../../fixtures/test'; + +test.describe('Host — reset guest PIN (C1)', () => { + test('resetting a guest PIN returns a fresh 4-digit PIN', async ({ api, host, guest }) => { + const target = await guest('ResetMe'); + + const { status, body } = await api.resetUserPin(host.jwt, target.userId); + + expect(status).toBe(200); + expect(body.pin).toMatch(/^\d{4}$/); + }); + + test('the target can recover with the newly reset PIN (and not the old one)', async ({ + api, + host, + guest, + }) => { + const target = await guest('RecoverWithNewPin'); + + const { body } = await api.resetUserPin(host.jwt, target.userId); + const newPin = body.pin!; + + // New PIN works. + await api.recover(target.displayName, newPin, { expectedStatus: [200] }); + + // Old PIN no longer works (overwritten). 401 = wrong PIN. + if (target.pin !== newPin) { + await api.recover(target.displayName, target.pin, { expectedStatus: [401] }); + } + }); + + test('a host cannot reset their own PIN via this endpoint', async ({ api, host }) => { + await api.resetUserPin(host.jwt, host.userId, { expectedStatus: [400] }); + }); +}); From f08d858281fc3c583bdb7caf000338472bae16a3 Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 1 Jul 2026 21:25:50 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(security):=20high=20=E2=80=94=20live=20?= =?UTF-8?q?authz,=20XFF,=20SSE=20buffering,=20atomicity,=20pagination?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Caddyfile | 5 +- .../migrations/010_review_indexes.down.sql | 3 + backend/migrations/010_review_indexes.up.sql | 17 ++++++ backend/src/auth/middleware.rs | 20 +++++-- backend/src/handlers/feed.rs | 60 +++++++++++-------- backend/src/handlers/upload.rs | 56 +++++++++-------- backend/src/models/hashtag.rs | 32 ++++++---- backend/src/models/upload.rs | 24 +++++--- backend/src/services/rate_limiter.rs | 26 ++++++-- e2e/specs/04-host/moderation.spec.ts | 51 ++++++++++++++++ .../src/lib/components/LightboxModal.svelte | 14 ++++- frontend/svelte.config.js | 23 ++++++- 12 files changed, 248 insertions(+), 83 deletions(-) create mode 100644 backend/migrations/010_review_indexes.down.sql create mode 100644 backend/migrations/010_review_indexes.up.sql diff --git a/Caddyfile b/Caddyfile index d560ae4..3ceff7c 100644 --- a/Caddyfile +++ b/Caddyfile @@ -1,5 +1,8 @@ {$DOMAIN} { - encode zstd gzip + # Compress everything EXCEPT the SSE stream — gzip buffering delays + # "real-time" likes/comments until the ~30s keep-alive tick. + @compressible not path /api/v1/stream + encode @compressible zstd gzip # Site-wide security headers (defense-in-depth). HSTS is free since Caddy # already terminates TLS. nosniff also covers all of /media/*. diff --git a/backend/migrations/010_review_indexes.down.sql b/backend/migrations/010_review_indexes.down.sql new file mode 100644 index 0000000..d69f56b --- /dev/null +++ b/backend/migrations/010_review_indexes.down.sql @@ -0,0 +1,3 @@ +DROP INDEX IF EXISTS idx_comment_hashtag_hashtag; +DROP INDEX IF EXISTS idx_comment_user; +DROP INDEX IF EXISTS idx_upload_event_created_id; diff --git a/backend/migrations/010_review_indexes.up.sql b/backend/migrations/010_review_indexes.up.sql new file mode 100644 index 0000000..93d2717 --- /dev/null +++ b/backend/migrations/010_review_indexes.up.sql @@ -0,0 +1,17 @@ +-- Composite feed index with an id tiebreaker so keyset pagination +-- (ORDER BY created_at DESC, id DESC) stays index-covered and stable when +-- multiple uploads share a created_at timestamp. +CREATE INDEX idx_upload_event_created_id + ON upload(event_id, created_at DESC, id DESC) + WHERE deleted_at IS NULL; + +-- A user's own comments (moderation, "who commented"). The sibling +-- idx_upload_user already exists for uploads; comment(user_id) was missing. +CREATE INDEX idx_comment_user + ON comment(user_id) + WHERE deleted_at IS NULL; + +-- Hashtag filtering over comments — mirrors idx_upload_hashtag_hashtag, which +-- only covered upload_hashtag. +CREATE INDEX idx_comment_hashtag_hashtag + ON comment_hashtag(hashtag_id); diff --git a/backend/src/auth/middleware.rs b/backend/src/auth/middleware.rs index 1511141..17de8f8 100644 --- a/backend/src/auth/middleware.rs +++ b/backend/src/auth/middleware.rs @@ -1,4 +1,4 @@ -use axum::extract::{FromRequestParts, State}; +use axum::extract::FromRequestParts; use axum::http::request::Parts; use uuid::Uuid; @@ -43,6 +43,18 @@ impl FromRequestParts for AuthUser { .map_err(|e| AppError::Internal(e.into()))? .ok_or_else(|| AppError::Unauthorized("Sitzung nicht gefunden oder abgelaufen.".into()))?; + // Trust *live* DB state, not the JWT: a role/ban stored in the token would + // survive a demote/ban for the full session lifetime (up to 30d). Re-read + // the user row so a demoted host loses host powers and a banned user is + // locked out immediately on their next request. + let user = crate::models::user::User::find_by_id(&state.pool, claims.sub) + .await + .map_err(|e| AppError::Internal(e.into()))? + .ok_or_else(|| AppError::Unauthorized("Benutzer nicht gefunden.".into()))?; + if user.is_banned { + return Err(AppError::Forbidden("Dein Zugang wurde gesperrt.".into())); + } + // Update last_seen_at in the background (fire-and-forget). Failures are // non-fatal but worth surfacing — silent swallowing hides DB connection // pressure that would otherwise be the first symptom of a real problem. @@ -55,9 +67,9 @@ impl FromRequestParts for AuthUser { }); Ok(Self { - user_id: claims.sub, - event_id: claims.event_id, - role: claims.role, + user_id: user.id, + event_id: user.event_id, + role: user.role, token_hash, }) } diff --git a/backend/src/handlers/feed.rs b/backend/src/handlers/feed.rs index 0a6f21f..830a634 100644 --- a/backend/src/handlers/feed.rs +++ b/backend/src/handlers/feed.rs @@ -79,6 +79,17 @@ pub async fn feed( let limit = q.limit.unwrap_or(20).min(100); + // Resolve the cursor to a (created_at, id) position. The pair is compared as a + // tuple so ties on created_at break on id — keyset pagination on created_at + // alone would silently drop rows sharing a timestamp across a page boundary. + let (cursor_time, cursor_id) = match q.cursor { + Some(c) => match get_cursor_pos(&state.pool, c).await { + Some((t, id)) => (Some(t), Some(id)), + None => (None, None), + }, + None => (None, None), + }; + let rows = if let Some(hashtag) = &q.hashtag { let tag = hashtag.trim().trim_start_matches('#').to_lowercase(); sqlx::query_as::<_, FeedRow>( @@ -88,19 +99,14 @@ pub async fn feed( JOIN upload_hashtag uh ON uh.upload_id = v.id JOIN hashtag h ON h.id = uh.hashtag_id AND h.tag = $1 WHERE v.event_id = $2 - AND ($3::timestamptz IS NULL OR v.created_at < $3) - ORDER BY v.created_at DESC - LIMIT $4", + AND ($3::timestamptz IS NULL OR (v.created_at, v.id) < ($3, $4)) + ORDER BY v.created_at DESC, v.id DESC + LIMIT $5", ) .bind(&tag) .bind(auth.event_id) - .bind( - if let Some(cursor) = q.cursor { - get_cursor_time(&state.pool, cursor).await - } else { - None - }, - ) + .bind(cursor_time) + .bind(cursor_id) .bind(limit + 1) .fetch_all(&state.pool) .await? @@ -110,18 +116,13 @@ pub async fn feed( mime_type, caption, like_count, comment_count, created_at FROM v_feed WHERE event_id = $1 - AND ($2::timestamptz IS NULL OR created_at < $2) - ORDER BY created_at DESC - LIMIT $3", + AND ($2::timestamptz IS NULL OR (created_at, id) < ($2, $3)) + ORDER BY created_at DESC, id DESC + LIMIT $4", ) .bind(auth.event_id) - .bind( - if let Some(cursor) = q.cursor { - get_cursor_time(&state.pool, cursor).await - } else { - None - }, - ) + .bind(cursor_time) + .bind(cursor_id) .bind(limit + 1) .fetch_all(&state.pool) .await? @@ -178,15 +179,21 @@ pub async fn feed_delta( auth: AuthUser, Query(q): Query, ) -> Result, AppError> { + // Bounded like the paginated feed: a stale `since` could otherwise pull the + // entire event's uploads in one response. If a client hits the cap it should + // fall back to a full feed refresh rather than another delta. + const DELTA_LIMIT: i64 = 200; let rows = sqlx::query_as::<_, FeedRow>( "SELECT id, user_id, uploader_name, preview_path, thumbnail_path, mime_type, caption, like_count, comment_count, created_at FROM v_feed WHERE event_id = $1 AND created_at > $2 - ORDER BY created_at DESC", + ORDER BY created_at DESC, id DESC + LIMIT $3", ) .bind(auth.event_id) .bind(q.since) + .bind(DELTA_LIMIT) .fetch_all(&state.pool) .await?; @@ -249,14 +256,17 @@ pub async fn hashtags( )) } -async fn get_cursor_time(pool: &sqlx::PgPool, cursor_id: Uuid) -> Option> { - let row: Option<(DateTime,)> = - sqlx::query_as("SELECT created_at FROM upload WHERE id = $1") +/// Resolve a cursor id to its `(created_at, id)` position. Both are needed: +/// `created_at` alone isn't unique, so pagination must break ties on `id` to +/// avoid silently dropping rows that share a timestamp across a page boundary. +async fn get_cursor_pos(pool: &sqlx::PgPool, cursor_id: Uuid) -> Option<(DateTime, Uuid)> { + let row: Option<(DateTime, Uuid)> = + sqlx::query_as("SELECT created_at, id FROM upload WHERE id = $1") .bind(cursor_id) .fetch_optional(pool) .await .ok()?; - row.map(|r| r.0) + row } async fn get_liked_set( diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 841e6f9..f2043f6 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -190,25 +190,6 @@ pub async fn upload( } tokio::fs::write(&absolute_path, &data).await.map_err(|e| AppError::Internal(e.into()))?; - // Update user's total upload bytes - sqlx::query("UPDATE \"user\" SET total_upload_bytes = total_upload_bytes + $2 WHERE id = $1") - .bind(auth.user_id) - .bind(size) - .execute(&state.pool) - .await?; - - // Insert upload record - let upload = Upload::create( - &state.pool, - auth.event_id, - auth.user_id, - &relative_path, - &mime, - size, - caption.as_deref(), - ) - .await?; - // Process hashtags from caption and explicit CSV let mut tags: Vec = Vec::new(); if let Some(ref cap) = caption { @@ -225,10 +206,30 @@ pub async fn upload( tags.sort(); tags.dedup(); + // Quota accounting, the upload row, and its hashtag links must be atomic: a + // crash between the bytes increment and the insert would permanently charge + // bytes with no row to reclaim them (silent quota erosion / spurious lockout). + let mut tx = state.pool.begin().await?; + sqlx::query("UPDATE \"user\" SET total_upload_bytes = total_upload_bytes + $2 WHERE id = $1") + .bind(auth.user_id) + .bind(size) + .execute(&mut *tx) + .await?; + let upload = Upload::create( + &mut *tx, + auth.event_id, + auth.user_id, + &relative_path, + &mime, + size, + caption.as_deref(), + ) + .await?; for tag in &tags { - let h = Hashtag::upsert(&state.pool, auth.event_id, tag).await?; - Hashtag::link_to_upload(&state.pool, upload.id, h.id).await?; + let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?; + Hashtag::link_to_upload(&mut *tx, upload.id, h.id).await?; } + tx.commit().await?; // Spawn compression task state @@ -279,17 +280,20 @@ pub async fn edit_upload( return Err(AppError::Forbidden("Nur eigene Uploads bearbeiten.".into())); } + // Caption update + hashtag wipe-then-relink in one transaction, so a crash + // mid-relink can't leave the upload with its hashtags stripped. + let mut tx = state.pool.begin().await?; if let Some(ref caption) = body.caption { - Upload::update_caption(&state.pool, upload_id, Some(caption)).await?; + Upload::update_caption(&mut *tx, upload_id, Some(caption)).await?; } - if let Some(ref hashtags) = body.hashtags { - Hashtag::unlink_all_from_upload(&state.pool, upload_id).await?; + Hashtag::unlink_all_from_upload(&mut *tx, upload_id).await?; for tag in hashtags { - let h = Hashtag::upsert(&state.pool, auth.event_id, tag).await?; - Hashtag::link_to_upload(&state.pool, upload_id, h.id).await?; + let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?; + Hashtag::link_to_upload(&mut *tx, upload_id, h.id).await?; } } + tx.commit().await?; Ok(StatusCode::OK) } diff --git a/backend/src/models/hashtag.rs b/backend/src/models/hashtag.rs index 0414fbc..7cd13c9 100644 --- a/backend/src/models/hashtag.rs +++ b/backend/src/models/hashtag.rs @@ -10,7 +10,13 @@ pub struct Hashtag { impl Hashtag { /// Upsert a hashtag (insert if not exists, return existing if it does). - pub async fn upsert(pool: &PgPool, event_id: Uuid, tag: &str) -> Result { + /// + /// Takes any executor so callers can run it inside a transaction (atomic + /// upload/comment writes) or standalone against the pool. + pub async fn upsert<'e, E>(executor: E, event_id: Uuid, tag: &str) -> Result + where + E: sqlx::PgExecutor<'e>, + { let normalized = tag.trim().trim_start_matches('#').to_lowercase(); sqlx::query_as::<_, Self>( "INSERT INTO hashtag (event_id, tag) VALUES ($1, $2) @@ -19,33 +25,39 @@ impl Hashtag { ) .bind(event_id) .bind(&normalized) - .fetch_one(pool) + .fetch_one(executor) .await } - pub async fn link_to_upload( - pool: &PgPool, + pub async fn link_to_upload<'e, E>( + executor: E, upload_id: Uuid, hashtag_id: Uuid, - ) -> Result<(), sqlx::Error> { + ) -> Result<(), sqlx::Error> + where + E: sqlx::PgExecutor<'e>, + { sqlx::query( "INSERT INTO upload_hashtag (upload_id, hashtag_id) VALUES ($1, $2) ON CONFLICT DO NOTHING", ) .bind(upload_id) .bind(hashtag_id) - .execute(pool) + .execute(executor) .await?; Ok(()) } - pub async fn unlink_all_from_upload( - pool: &PgPool, + pub async fn unlink_all_from_upload<'e, E>( + executor: E, upload_id: Uuid, - ) -> Result<(), sqlx::Error> { + ) -> Result<(), sqlx::Error> + where + E: sqlx::PgExecutor<'e>, + { sqlx::query("DELETE FROM upload_hashtag WHERE upload_id = $1") .bind(upload_id) - .execute(pool) + .execute(executor) .await?; Ok(()) } diff --git a/backend/src/models/upload.rs b/backend/src/models/upload.rs index 1b085e1..243c527 100644 --- a/backend/src/models/upload.rs +++ b/backend/src/models/upload.rs @@ -36,15 +36,20 @@ pub struct UploadDto { } impl Upload { - pub async fn create( - pool: &PgPool, + /// Takes any executor so the caller can run it inside a transaction (atomic + /// quota + insert) or standalone against the pool. + pub async fn create<'e, E>( + executor: E, event_id: Uuid, user_id: Uuid, original_path: &str, mime_type: &str, original_size_bytes: i64, caption: Option<&str>, - ) -> Result { + ) -> Result + where + E: sqlx::PgExecutor<'e>, + { sqlx::query_as::<_, Self>( "INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes, caption) VALUES ($1, $2, $3, $4, $5, $6) @@ -56,7 +61,7 @@ impl Upload { .bind(mime_type) .bind(original_size_bytes) .bind(caption) - .fetch_one(pool) + .fetch_one(executor) .await } @@ -182,15 +187,18 @@ impl Upload { Ok(deleted) } - pub async fn update_caption( - pool: &PgPool, + pub async fn update_caption<'e, E>( + executor: E, id: Uuid, caption: Option<&str>, - ) -> Result<(), sqlx::Error> { + ) -> Result<(), sqlx::Error> + where + E: sqlx::PgExecutor<'e>, + { sqlx::query("UPDATE upload SET caption = $2 WHERE id = $1") .bind(id) .bind(caption) - .execute(pool) + .execute(executor) .await?; Ok(()) } diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index d29ae4d..cb9a041 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -71,14 +71,21 @@ impl RateLimiter { } } -/// Extract the client IP from X-Forwarded-For (Caddy sets this) or fall back -/// to a provided socket address string. +/// Extract the client IP from X-Forwarded-For or fall back to a provided socket +/// address string. +/// +/// We take the **right-most** entry, not the left-most. Caddy is the sole ingress +/// and the app port is only `expose`d (never published), so the last hop Caddy +/// 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. pub fn client_ip(headers: &axum::http::HeaderMap, fallback: &str) -> String { headers .get("x-forwarded-for") .and_then(|v| v.to_str().ok()) - .and_then(|s| s.split(',').next()) + .and_then(|s| s.rsplit(',').next()) .map(|s| s.trim().to_owned()) + .filter(|s| !s.is_empty()) .unwrap_or_else(|| fallback.to_owned()) } @@ -134,9 +141,18 @@ mod tests { } #[test] - fn client_ip_prefers_first_forwarded_for_entry() { + fn client_ip_takes_rightmost_forwarded_for_entry() { + // The right-most entry is the hop our trusted proxy (Caddy) appended. let mut h = HeaderMap::new(); - h.insert("x-forwarded-for", "203.0.113.7, 10.0.0.1".parse().unwrap()); + h.insert("x-forwarded-for", "10.0.0.1, 203.0.113.7".parse().unwrap()); + assert_eq!(client_ip(&h, "fallback"), "203.0.113.7"); + } + + #[test] + fn client_ip_ignores_spoofed_leftmost_entry() { + // A client prepending a fake IP to dodge throttles must not win. + let mut h = HeaderMap::new(); + h.insert("x-forwarded-for", "1.2.3.4, 9.9.9.9, 203.0.113.7".parse().unwrap()); assert_eq!(client_ip(&h, "fallback"), "203.0.113.7"); } diff --git a/e2e/specs/04-host/moderation.spec.ts b/e2e/specs/04-host/moderation.spec.ts index 0095958..153160b 100644 --- a/e2e/specs/04-host/moderation.spec.ts +++ b/e2e/specs/04-host/moderation.spec.ts @@ -45,3 +45,54 @@ test.describe('Host — moderation API', () => { expect(row?.role).toBe('host'); }); }); + +/** + * Regression for the review's H1: role & ban used to be trusted from the JWT and + * never re-checked against the DB, so a demoted/banned host kept full powers for + * the life of their token (up to 30d) and a banned host could even unban + * themselves. The auth extractor now re-reads the live user row, so these take + * effect on the *existing* session with no re-login. + */ +test.describe('Host — live role/ban revocation (H1)', () => { + test('a demoted host loses host powers on their existing session', async ({ + api, + adminToken, + guest, + }) => { + const u = await guest('DemoteMidSession'); + await api.setRole(adminToken, u.userId, 'host'); + // Fresh host token (role is encoded at mint time). + const { body } = await api.recover(u.displayName, u.pin); + const hostJwt = body.jwt; + + // Sanity: the token currently has host powers. + await api.listUsers(hostJwt); + + // Demote via admin — the host does NOT re-login. + await api.setRole(adminToken, u.userId, 'guest'); + + // Same token is now rejected because the middleware trusts the DB role. + await expect(api.listUsers(hostJwt)).rejects.toThrow(); + }); + + test('a banned host is locked out immediately on their existing session', async ({ + api, + adminToken, + host, + }) => { + // Sanity: host token works. + await api.listUsers(host.jwt); + + await api.banUser(adminToken, host.userId, false); + + // Banned users are rejected by the auth extractor before any handler runs. + await expect(api.listUsers(host.jwt)).rejects.toThrow(); + }); + + test('a banned host cannot unban themselves', async ({ api, adminToken, host }) => { + await api.banUser(adminToken, host.userId, false); + await expect( + api.unbanUser(host.jwt, host.userId, { expectedStatus: [204] }) + ).rejects.toThrow(); + }); +}); diff --git a/frontend/src/lib/components/LightboxModal.svelte b/frontend/src/lib/components/LightboxModal.svelte index 88f597d..7de599f 100644 --- a/frontend/src/lib/components/LightboxModal.svelte +++ b/frontend/src/lib/components/LightboxModal.svelte @@ -7,6 +7,7 @@ import { doubletap } from '$lib/actions/doubletap'; import { focusTrap } from '$lib/actions/focus-trap'; import { scrollLock } from '$lib/actions/scroll-lock'; + import { modalInert } from '$lib/actions/modal-inert'; import { toastError } from '$lib/toast-store'; import { vibrate } from '$lib/haptics'; import HeartBurst from './HeartBurst.svelte'; @@ -51,13 +52,19 @@ if (burstTimer) clearTimeout(burstTimer); }); + // Only refetch when a *different* upload is shown. The feed reassigns the + // `upload` prop object on every SSE like/comment count update; keying the + // effect off the memoized id avoids a refetch storm that would also clobber + // a just-posted optimistic comment. + const uploadId = $derived(upload.id); + $effect(() => { - loadComments(); + loadComments(uploadId); }); - async function loadComments() { + async function loadComments(id: string) { try { - comments = await api.get(`/upload/${upload.id}/comments`); + comments = await api.get(`/upload/${id}/comments`); } catch { // Background fetch — failure leaves the panel empty; reopening the lightbox retries. } @@ -106,6 +113,7 @@ aria-labelledby="lightbox-title" use:focusTrap={{ onclose }} use:scrollLock + use:modalInert >
diff --git a/frontend/svelte.config.js b/frontend/svelte.config.js index 45a528d..c7ae9de 100644 --- a/frontend/svelte.config.js +++ b/frontend/svelte.config.js @@ -6,7 +6,28 @@ const config = { runes: true }, kit: { - adapter: adapter() + adapter: adapter(), + // Content-Security-Policy — the highest-value header for this UGC app and the + // cheapest hardening of the localStorage-based auth (the whole model rests on + // never having an XSS). `mode: 'auto'` lets SvelteKit nonce/hash its own inline + // hydration script, so script-src stays free of 'unsafe-inline'. + csp: { + mode: 'auto', + directives: { + 'default-src': ['self'], + 'script-src': ['self'], + // Svelte emits inline style attributes (style: directives); allow them. + 'style-src': ['self', 'unsafe-inline'], + 'img-src': ['self', 'data:', 'blob:'], + 'media-src': ['self', 'blob:'], + 'font-src': ['self'], + 'connect-src': ['self'], + 'object-src': ['none'], + 'base-uri': ['self'], + 'form-action': ['self'], + 'frame-ancestors': ['none'] + } + } } }; From 91ff961850647e5ca3939129d2c9b9d5e7f6e15e Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 1 Jul 2026 21:25:58 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(security):=20medium/low=20=E2=80=94=20e?= =?UTF-8?q?vent-lock,=20atomic=20config,=20a11y,=20diashow,=20hygiene?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - social: likes/comments rejected on a closed event (e2e proven). - admin: patch_config validates-then-writes atomically; char-based length. - hashtag/comment models: atomic tag writes in edit + comment paths. - Modal: inert the background (modal-inert action) for screen readers. - sse.ts: idempotent visibilitychange listener (no duplicate reconnects). - diashow: videos advance on `ended` (transitions + page wiring). - docs/hygiene: README clone-case fix; removed stale committed .env.test and gitignored it; docker-compose.dev.yml tidy. Left documented as accepted-risk per plan: PIN-persist-after-logout, UUID-reachable hidden uploads, DECISION-media-auth.md. Co-Authored-By: Claude Opus 4.8 --- .env.test | 45 ------------------- .gitignore | 2 + README.md | 6 +-- backend/src/handlers/admin.rs | 13 +++++- backend/src/handlers/social.rs | 30 ++++++++++--- backend/src/models/comment.rs | 13 ++++-- docker-compose.dev.yml | 5 +++ e2e/specs/04-host/event-lock.spec.ts | 35 +++++++++++++++ frontend/src/lib/actions/modal-inert.ts | 29 ++++++++++++ frontend/src/lib/components/Modal.svelte | 42 +++++++++-------- .../lib/diashow/transitions/crossfade.svelte | 5 ++- frontend/src/lib/diashow/transitions/index.ts | 2 + .../lib/diashow/transitions/kenburns.svelte | 5 ++- frontend/src/lib/sse.ts | 38 +++++++++++----- frontend/src/routes/diashow/+page.svelte | 8 ++++ 15 files changed, 188 insertions(+), 90 deletions(-) delete mode 100644 .env.test create mode 100644 frontend/src/lib/actions/modal-inert.ts diff --git a/.env.test b/.env.test deleted file mode 100644 index b204567..0000000 --- a/.env.test +++ /dev/null @@ -1,45 +0,0 @@ -# ── Domain ──────────────────────────────────────────────────────────────────── -# Public domain Caddy will serve and obtain a TLS certificate for. -DOMAIN=my-event.example.com - -# ── App server ──────────────────────────────────────────────────────────────── -APP_PORT=3000 - -# ── Database ────────────────────────────────────────────────────────────────── -DATABASE_URL=postgres://eventsnap:secret@db:5432/eventsnap -POSTGRES_USER=eventsnap -POSTGRES_PASSWORD=secret -POSTGRES_DB=eventsnap - -# ── Authentication ──────────────────────────────────────────────────────────── -# Generate with: openssl rand -hex 64 -JWT_SECRET=change_me_to_a_random_64_byte_hex_string -SESSION_EXPIRY_DAYS=30 - -# Admin dashboard password (bcrypt hash). -# Generate with: htpasswd -bnBC 12 "" yourpassword | tr -d ':\n' -ADMIN_PASSWORD_HASH=$2y$12$placeholder_replace_me - -# ── Event ───────────────────────────────────────────────────────────────────── -EVENT_NAME=Max & Maria's Wedding -EVENT_SLUG=max-maria-2026 - -# ── Storage ─────────────────────────────────────────────────────────────────── -MEDIA_PATH=/media - -# ── Upload limits ───────────────────────────────────────────────────────────── -DEFAULT_MAX_IMAGE_SIZE_MB=20 -DEFAULT_MAX_VIDEO_SIZE_MB=500 - -# ── Rate limiting ───────────────────────────────────────────────────────────── -DEFAULT_UPLOAD_RATE_PER_HOUR=10 -DEFAULT_FEED_RATE_PER_MIN=60 -DEFAULT_EXPORT_RATE_PER_DAY=3 - -# ── Capacity ────────────────────────────────────────────────────────────────── -DEFAULT_ESTIMATED_GUEST_COUNT=100 -# Fraction of total storage that triggers the "low storage" warning (0.0–1.0) -DEFAULT_QUOTA_TOLERANCE=0.75 - -# ── Workers ─────────────────────────────────────────────────────────────────── -COMPRESSION_WORKER_CONCURRENCY=2 diff --git a/.gitignore b/.gitignore index 55d4381..d03afd4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,7 @@ # Environment secrets — never commit the real .env .env +# Stale local scratch copy of .env.example; nothing in the test stack reads it. +.env.test # Rust backend/target/ diff --git a/README.md b/README.md index 91f5cea..a3db1fe 100644 --- a/README.md +++ b/README.md @@ -94,9 +94,9 @@ eventsnap/ ### Deploy on a fresh VPS ```bash -# 1. Clone the repository -git clone https://git.mc02.dev/fabi/EventSnap.git -cd EventSnap +# 1. Clone the repository (into a lowercase dir, matching the paths used below) +git clone https://git.mc02.dev/fabi/EventSnap.git eventsnap +cd eventsnap # 2. Configure environment cp .env.example .env diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index 69f0753..987a1f0 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -146,6 +146,8 @@ pub async fn patch_config( let mut privacy_note_changed = false; + // Validate every key first so a bad value in the batch can't leave a partial + // update behind — validation must fully precede any write. for (key, value) in &body { let key_str = key.as_str(); if NUMERIC_KEYS.contains(&key_str) { @@ -164,7 +166,9 @@ pub async fn patch_config( } } } else if TEXT_KEYS.contains(&key_str) { - if value.len() > PRIVACY_NOTE_MAX_LEN { + // Count characters, not bytes — the message says "Zeichen" and a + // multi-byte grapheme shouldn't count against the limit multiple times. + if value.chars().count() > PRIVACY_NOTE_MAX_LEN { return Err(AppError::BadRequest(format!( "Wert für {key} ist zu lang (max. {PRIVACY_NOTE_MAX_LEN} Zeichen)." ))); @@ -177,16 +181,21 @@ pub async fn patch_config( "Unbekannter Konfigurationsschlüssel: {key}" ))); } + } + // Apply all writes in one transaction — the batch is all-or-nothing. + let mut tx = state.pool.begin().await?; + for (key, value) in &body { sqlx::query( "INSERT INTO config (key, value, updated_at) VALUES ($1, $2, NOW()) ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value, updated_at = NOW()", ) .bind(key) .bind(value) - .execute(&state.pool) + .execute(&mut *tx) .await?; } + tx.commit().await?; // Notify all clients that a publicly-readable config value changed so their stores // (e.g. the privacy note in My Account) refresh without a manual reload. diff --git a/backend/src/handlers/social.rs b/backend/src/handlers/social.rs index 79d83d0..168f6e0 100644 --- a/backend/src/handlers/social.rs +++ b/backend/src/handlers/social.rs @@ -12,6 +12,18 @@ use crate::models::hashtag::{self, Hashtag}; use crate::models::upload::Upload; use crate::state::AppState; +/// Reject the request when the event's uploads (and, by extension, social +/// interaction) are locked. Mirrors the guard in the upload handler. +async fn require_event_open(state: &AppState) -> Result<(), AppError> { + let event = crate::models::event::Event::find_by_slug(&state.pool, &state.config.event_slug) + .await? + .ok_or_else(|| AppError::NotFound("Event nicht gefunden.".into()))?; + if event.uploads_locked_at.is_some() { + return Err(AppError::Forbidden("Das Event ist geschlossen.".into())); + } + Ok(()) +} + pub async fn toggle_like( State(state): State, auth: AuthUser, @@ -31,6 +43,9 @@ pub async fn toggle_like( .await? .ok_or_else(|| AppError::NotFound("Upload nicht gefunden.".into()))?; + // A closed event freezes social interaction too, matching the upload handler. + require_event_open(&state).await?; + // Try to insert; if conflict, delete (toggle) let result = sqlx::query( "INSERT INTO \"like\" (upload_id, user_id) VALUES ($1, $2) @@ -120,6 +135,9 @@ pub async fn add_comment( .await? .ok_or_else(|| AppError::NotFound("Upload nicht gefunden.".into()))?; + // A closed event freezes social interaction too, matching the upload handler. + require_event_open(&state).await?; + let text = body.body.trim(); let text_chars = text.chars().count(); if text_chars == 0 || text_chars > 500 { @@ -128,20 +146,22 @@ pub async fn add_comment( )); } - let comment = Comment::create(&state.pool, upload_id, auth.user_id, text).await?; - - // Process hashtags in comment body + // Insert the comment and link its hashtags atomically, so a crash mid-loop + // can't leave a committed comment with only some of its tags indexed. let tags = hashtag::extract_hashtags(text); + let mut tx = state.pool.begin().await?; + let comment = Comment::create(&mut *tx, upload_id, auth.user_id, text).await?; for tag in &tags { - let h = Hashtag::upsert(&state.pool, auth.event_id, tag).await?; + let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?; sqlx::query( "INSERT INTO comment_hashtag (comment_id, hashtag_id) VALUES ($1, $2) ON CONFLICT DO NOTHING", ) .bind(comment.id) .bind(h.id) - .execute(&state.pool) + .execute(&mut *tx) .await?; } + tx.commit().await?; // Fresh count so feed clients can patch the single card in place instead of // refetching page 1 (mirrors v_feed.comment_count = COUNT(DISTINCT c.id); COUNT(*) diff --git a/backend/src/models/comment.rs b/backend/src/models/comment.rs index ec08e26..ac5d0cc 100644 --- a/backend/src/models/comment.rs +++ b/backend/src/models/comment.rs @@ -24,19 +24,24 @@ pub struct CommentDto { } impl Comment { - pub async fn create( - pool: &PgPool, + /// Takes any executor so the caller can insert the comment and link its + /// hashtags inside a single transaction. + pub async fn create<'e, E>( + executor: E, upload_id: Uuid, user_id: Uuid, body: &str, - ) -> Result { + ) -> Result + where + E: sqlx::PgExecutor<'e>, + { sqlx::query_as::<_, Self>( "INSERT INTO comment (upload_id, user_id, body) VALUES ($1, $2, $3) RETURNING *", ) .bind(upload_id) .bind(user_id) .bind(body) - .fetch_one(pool) + .fetch_one(executor) .await } diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 48b4937..2d341c3 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -6,3 +6,8 @@ services: db: ports: - "5432:5432" + app: + # Relax the production secret guard for local dev — the dev sentinel JWT_SECRET + # is tolerated (warned) rather than rejected. + environment: + APP_ENV: development diff --git a/e2e/specs/04-host/event-lock.spec.ts b/e2e/specs/04-host/event-lock.spec.ts index 4925a22..37d311b 100644 --- a/e2e/specs/04-host/event-lock.spec.ts +++ b/e2e/specs/04-host/event-lock.spec.ts @@ -33,4 +33,39 @@ test.describe('Host — event lock', () => { // Currently no UI consumes the event-closed SSE on /feed. Add this banner // and flip fixme to test once it lands. }); + + // Regression for the review: likes/comments used to ignore uploads_locked_at, + // so social writes still landed on a closed event. They now share the upload + // handler's lock guard. + test('a closed event rejects likes and comments', async ({ api, host, guest }) => { + const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + const g = await guest('SocialLocked'); + + // Upload while still open so there's a target to interact with. + const { uploadRaw } = await import('../../helpers/upload-client'); + const { readFileSync } = await import('node:fs'); + const { join } = await import('node:path'); + const sample = join(process.cwd(), 'fixtures', 'media', 'sample.jpg'); + const upRes = await uploadRaw(g.jwt, readFileSync(sample), { + filename: 'x.jpg', + contentType: 'image/jpeg', + }); + expect(upRes.status).toBe(201); + const { id } = await upRes.json(); + + await api.closeEvent(host.jwt); + + const likeRes = await fetch(`${BASE}/api/v1/upload/${id}/like`, { + method: 'POST', + headers: { Authorization: `Bearer ${g.jwt}` }, + }); + expect(likeRes.status).toBe(403); + + const commentRes = await fetch(`${BASE}/api/v1/upload/${id}/comments`, { + method: 'POST', + headers: { Authorization: `Bearer ${g.jwt}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ body: 'sollte blockiert sein' }), + }); + expect(commentRes.status).toBe(403); + }); }); diff --git a/frontend/src/lib/actions/modal-inert.ts b/frontend/src/lib/actions/modal-inert.ts new file mode 100644 index 0000000..8b9233f --- /dev/null +++ b/frontend/src/lib/actions/modal-inert.ts @@ -0,0 +1,29 @@ +// Make everything *outside* a modal's subtree inert while it's open, so screen +// readers and tab order can't reach the background (focus-trap only covers +// keyboard focus; `inert` also hides the content from assistive tech). +// +// Walks from the node up to and marks every sibling along the path as +// inert, then restores exactly those it changed on teardown. Applying it to a +// `display: contents` wrapper keeps the modal's own backdrop + dialog interactive. +export function modalInert(node: HTMLElement) { + const changed: HTMLElement[] = []; + + let el: HTMLElement | null = node; + while (el && el !== document.body) { + const parent: HTMLElement | null = el.parentElement; + if (!parent) break; + for (const sib of Array.from(parent.children)) { + if (sib !== el && sib instanceof HTMLElement && !sib.hasAttribute('inert')) { + sib.setAttribute('inert', ''); + changed.push(sib); + } + } + el = parent; + } + + return { + destroy() { + for (const sib of changed) sib.removeAttribute('inert'); + } + }; +} diff --git a/frontend/src/lib/components/Modal.svelte b/frontend/src/lib/components/Modal.svelte index da5ef79..1de7285 100644 --- a/frontend/src/lib/components/Modal.svelte +++ b/frontend/src/lib/components/Modal.svelte @@ -1,6 +1,7 @@ {#if open} - -