From f08d858281fc3c583bdb7caf000338472bae16a3 Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 1 Jul 2026 21:25:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20high=20=E2=80=94=20live=20auth?= =?UTF-8?q?z,=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'] + } + } } };