diff --git a/Caddyfile b/Caddyfile index 129bb28..e4fd381 100644 --- a/Caddyfile +++ b/Caddyfile @@ -5,21 +5,24 @@ @hashed_assets path_regexp hashed /_app/immutable/.*\.[a-f0-9]{8,}\.(js|css|woff2)$ header @hashed_assets Cache-Control "public, max-age=31536000, immutable" - # Media previews and thumbnails - @previews path /media/previews/* /media/thumbnails/* - header @previews Cache-Control "public, max-age=3600" - - # Original media files (private — only host can download) - @originals path /media/originals/* - header @originals Cache-Control "private, max-age=86400" - # API — never cache @api path /api/* header @api Cache-Control "no-store" - # Route API and media requests to the Rust backend - reverse_proxy /api/* app:3000 - reverse_proxy /media/* app:3000 + # Media is served by the authenticated gateway at /media/{kind}/{id}, which + # sets its own Cache-Control (private) and security headers per response — no + # Caddy-side cache rules (the old /media/previews|originals matchers were for + # the retired static mount). + + # Route API and media requests to the Rust backend. Set X-Real-IP from the + # real TCP peer and overwrite any client-supplied value so the backend's + # rate-limit keys can't be spoofed via a forged X-Forwarded-For. + reverse_proxy /api/* app:3000 { + header_up X-Real-IP {remote_host} + } + reverse_proxy /media/* app:3000 { + header_up X-Real-IP {remote_host} + } # Everything else goes to SvelteKit frontend reverse_proxy frontend:3001 diff --git a/backend/Dockerfile b/backend/Dockerfile index 3953c2f..2cbdf47 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -18,10 +18,18 @@ RUN touch src/main.rs && cargo build --release # --- Runtime stage --- FROM alpine:3.21 -RUN apk add --no-cache ca-certificates ffmpeg +# Run as an unprivileged user (defense-in-depth). The media volume mounts at +# /media; creating it here as `app` means a fresh named volume inherits app +# ownership and is writable without running as root. (An existing root-owned +# volume from a prior deploy needs a one-time `chown -R app:app` — see deploy notes.) +RUN apk add --no-cache ca-certificates ffmpeg \ + && addgroup -S app && adduser -S -G app app \ + && mkdir -p /media && chown app:app /media WORKDIR /app COPY --from=builder /app/target/release/eventsnap-backend ./ +RUN chown -R app:app /app +USER app EXPOSE 3000 CMD ["./eventsnap-backend"] diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index 2c3c378..78695d7 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -271,6 +271,22 @@ pub async fn admin_login( // a long-running guess campaign. 5 attempts / minute / IP is plenty for // honest typos. let ip = client_ip(&headers, "unknown"); + + // Hard, non-disableable floor: admin_login is the one credential endpoint + // whose brute-force protection must survive the DB rate-limit master toggle + // being turned off. 30 attempts / 5 min / IP is generous for typos but caps + // sustained guessing regardless of config. + if !state.rate_limiter.check( + format!("admin_login_floor:{ip}"), + 30, + Duration::from_secs(300), + ) { + return Err(AppError::TooManyRequests( + "Zu viele Anmeldeversuche. Bitte warte kurz und versuche es erneut.".into(), + None, + )); + } + let rate_limits_on = config::get_bool(&state.pool, "rate_limits_enabled", true).await; let admin_rate_on = config::get_bool(&state.pool, "admin_login_rate_enabled", true).await; if rate_limits_on && admin_rate_on diff --git a/backend/src/main.rs b/backend/src/main.rs index 141362b..a5dc886 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -131,11 +131,24 @@ async fn main() -> Result<()> { // there is no raw static mount. The gateway verifies an HMAC signature and // consults the DB (deleted / ban-hidden / type), so private artifacts and // the export archives are never reachable by guessing a path. + // Trace spans log the request *path* only, never the query string — signed + // media URLs carry a replayable `?sig=` capability that must not land in + // access logs. + let trace_layer = TraceLayer::new_for_http().make_span_with( + |req: &axum::http::Request| { + tracing::info_span!( + "request", + method = %req.method(), + path = %req.uri().path(), + ) + }, + ); + let router = Router::new() .route("/health", get(|| async { "ok" })) .merge(api) .route("/media/{kind}/{id}", get(handlers::media::serve)) - .layer(TraceLayer::new_for_http()) + .layer(trace_layer) .with_state(state); let listener = tokio::net::TcpListener::bind(("0.0.0.0", config.app_port)).await?; diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index 8442016..057d8cb 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -54,10 +54,16 @@ impl CompressionWorker { }); } Err(e) => { + // Log the detailed error (incl. paths) server-side only; the + // SSE channel is broadcast to every client, so send a generic + // message — never leak absolute filesystem paths. tracing::error!("compression failed for upload {upload_id}: {e:#}"); let _ = worker.sse_tx.send(SseEvent { event_type: "upload-error".to_string(), - data: serde_json::json!({ "upload_id": upload_id, "error": e.to_string() }).to_string(), + data: serde_json::json!({ + "upload_id": upload_id, + "error": "Verarbeitung fehlgeschlagen." + }).to_string(), }); let _ = Upload::set_compression_status(&worker.pool, upload_id, "failed").await; } diff --git a/backend/src/services/media_fs.rs b/backend/src/services/media_fs.rs index d521355..24e9074 100644 --- a/backend/src/services/media_fs.rs +++ b/backend/src/services/media_fs.rs @@ -37,8 +37,13 @@ pub async fn unlink_media(media_path: &Path, paths: &DeletedPaths) { /// DB-row-driven — it never walks the filesystem, so it can never remove a file /// belonging to a live upload. pub async fn reap_deleted(pool: &PgPool, media_path: &Path) { - let rows: Vec = match sqlx::query_as::<_, DeletedPaths>( - "SELECT original_path AS original, preview_path AS preview, thumbnail_path AS thumbnail + // Snapshot the exact rows (id + paths) we are about to unlink, and delete by + // those ids — NOT by re-evaluating the `deleted_at < now - 1 day` predicate. + // A row that crosses the 1-day line *during* the unlink loop would otherwise + // be hard-deleted by the second predicate without ever having its files + // unlinked → a permanent orphan. + let rows: Vec<(uuid::Uuid, String, Option, Option)> = match sqlx::query_as( + "SELECT id, original_path, preview_path, thumbnail_path FROM upload WHERE deleted_at IS NOT NULL AND deleted_at < NOW() - INTERVAL '1 day'", ) @@ -56,16 +61,21 @@ pub async fn reap_deleted(pool: &PgPool, media_path: &Path) { return; } - for paths in &rows { - unlink_media(media_path, paths).await; + let mut ids = Vec::with_capacity(rows.len()); + for (id, original, preview, thumbnail) in &rows { + unlink_media(media_path, &DeletedPaths { + original: original.clone(), + preview: preview.clone(), + thumbnail: thumbnail.clone(), + }) + .await; + ids.push(*id); } - match sqlx::query( - "DELETE FROM upload - WHERE deleted_at IS NOT NULL AND deleted_at < NOW() - INTERVAL '1 day'", - ) - .execute(pool) - .await + match sqlx::query("DELETE FROM upload WHERE id = ANY($1)") + .bind(&ids) + .execute(pool) + .await { Ok(r) => tracing::info!("media reaper: removed {} deleted upload(s)", r.rows_affected()), Err(e) => tracing::warn!(error = ?e, "media reaper: row delete failed"), diff --git a/backend/src/services/rate_limiter.rs b/backend/src/services/rate_limiter.rs index 733640a..8b0769f 100644 --- a/backend/src/services/rate_limiter.rs +++ b/backend/src/services/rate_limiter.rs @@ -71,13 +71,55 @@ 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 for rate-limit keys. +/// +/// Prefers `X-Real-IP`, which our reverse proxy (Caddy) overwrites with the real +/// TCP peer (`header_up X-Real-IP {remote_host}`) — a client cannot spoof it. +/// Falls back to the **rightmost** `X-Forwarded-For` token (the entry the proxy +/// appended), never the leftmost: the leftmost is fully client-controlled, and +/// trusting it let an attacker rotate the key to bypass the join cap, the +/// recover throttle, and the admin-login floor. pub fn client_ip(headers: &axum::http::HeaderMap, fallback: &str) -> String { + if let Some(ip) = headers + .get("x-real-ip") + .and_then(|v| v.to_str().ok()) + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + { + return ip.to_owned(); + } headers .get("x-forwarded-for") .and_then(|v| v.to_str().ok()) - .and_then(|s| s.split(',').next()) + .and_then(|s| s.split(',').next_back()) .map(|s| s.trim().to_owned()) + .filter(|s| !s.is_empty()) .unwrap_or_else(|| fallback.to_owned()) } + +#[cfg(test)] +mod tests { + use super::client_ip; + use axum::http::HeaderMap; + + #[test] + fn prefers_x_real_ip() { + let mut h = HeaderMap::new(); + h.insert("x-real-ip", "9.9.9.9".parse().unwrap()); + h.insert("x-forwarded-for", "1.1.1.1, 9.9.9.9".parse().unwrap()); + assert_eq!(client_ip(&h, "fb"), "9.9.9.9"); + } + + #[test] + fn xff_uses_rightmost_not_spoofable_leftmost() { + // Attacker prepends a forged entry; the proxy-appended real peer is last. + let mut h = HeaderMap::new(); + h.insert("x-forwarded-for", "1.2.3.4, 203.0.113.7".parse().unwrap()); + assert_eq!(client_ip(&h, "fb"), "203.0.113.7"); + } + + #[test] + fn falls_back_when_no_headers() { + assert_eq!(client_ip(&HeaderMap::new(), "fb"), "fb"); + } +} diff --git a/docs/SECURITY-BACKLOG.md b/docs/SECURITY-BACKLOG.md new file mode 100644 index 0000000..0070285 --- /dev/null +++ b/docs/SECURITY-BACKLOG.md @@ -0,0 +1,51 @@ +# Security & Hardening Backlog + +Tracks the deliberately-deferred items from the 2026-06-27 audit and its review passes. The +Critical→Medium findings and the cheap "bucket 🅰" LOWs are fixed on +`fix/audit-2026-06-27-critical-medium`. What remains is recorded here so the decisions are +explicit, not forgotten. + +## 🅱 Worth a tracked ticket (real, not one-liners) + +- **Moderation UI gap** — the backend `DELETE /host/upload/{id}` and `DELETE /host/comment/{id}` + endpoints have **no frontend caller**, so a host cannot remove a guest's content from the UI. + This is a functional hole, not polish. Needs a host-facing "remove" action wired to those + endpoints (a `ContextSheet` action gated on host role). +- **Feed reactivity** — a single global SSE event (`like-update`/`new-comment`/`upload-processed`) + triggers `loadFeed(true)`, which full-replaces the list with the first 20, collapsing a + scrolled feed; and owner-deleted uploads aren't broadcast to other clients (only host deletes + are). Patch the affected item from the SSE payload instead of full-reloading; emit + `upload-deleted` from the owner `delete_upload` path too. +- **Media HMAC domain separation** — the signed-media tokens reuse `jwt_secret` as the HMAC key. + Correct today (different message structure), but a dedicated derived key + (`HKDF(jwt_secret, "media-url")`) would isolate the domains so a future change to one can't + weaken the other. +- **Quota mount-detection + low-disk guard** — `compute_storage_quota` picks the disk via + `starts_with` (root is a wildcard prefix → can match the wrong device) and there's no hard + min-free-space precheck when the quota is disabled. Use longest-prefix match; add an + unconditional 507/429 when free space is critically low. + +## 🅲 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 +tenancy model changes. + +- Rate-limiter `HashMap` key LRU/cap (attacker-chosen `recover:{ip}:{name}` keys accumulate up to + the 24h prune ceiling) — bounded and pruned; not worth an LRU. +- "Last host" / host↔host role-churn guard — operational, low blast radius. +- Performance micro-indexes (`idx_like_user_upload`, comment pagination index) — current queries + are sub-ms at this row count. +- Optimistic-like in-flight guard, ownership-snapshot-at-mount, assorted copy tweaks — UX polish. + +## By-design notes (documented, not bugs) + +- **Banned user retains ≤24h media access via already-held signed URLs.** The media gateway + authorizes by *signature + uploader visibility*, not requester identity, and signed URLs are + time-boxed (24h, bucketed). A banned/revoked-session user cannot mint new URLs (every API call + 401/403s) but can replay URLs they already hold until expiry — only for content they already + saw. Accepted: tightening this would require per-request identity on every `` load, which + the ``-can't-send-a-Bearer constraint precludes. Shorten the TTL in `media_token.rs` if a + stricter bound is ever needed. +- **Two DB queries per *cold* media fetch** (`upload` row + uploader row). Mitigated by browser + caching and the stable bucketed URL (so warm fetches don't hit the backend at all). Could be one + JOIN if it ever shows up in profiling. diff --git a/frontend/src/routes/feed/+page.svelte b/frontend/src/routes/feed/+page.svelte index a7fe915..2cc9597 100644 --- a/frontend/src/routes/feed/+page.svelte +++ b/frontend/src/routes/feed/+page.svelte @@ -461,6 +461,7 @@ { diff --git a/frontend/src/routes/upload/+page.svelte b/frontend/src/routes/upload/+page.svelte index a5be70b..16fbeb6 100644 --- a/frontend/src/routes/upload/+page.svelte +++ b/frontend/src/routes/upload/+page.svelte @@ -195,6 +195,7 @@ bind:value={caption} maxlength={MAX_CAPTION_LENGTH} data-testid="upload-caption" + aria-label="Beschreibung" placeholder="Beschreibung hinzufügen… (#hashtags möglich)" rows="4" class="w-full resize-none rounded-xl border border-gray-200 bg-gray-50 px-4 py-3 text-sm text-gray-900