From dba4d3f93224697ffbe65d59bdc06c02af8ec671 Mon Sep 17 00:00:00 2001 From: fabi Date: Thu, 2 Jul 2026 22:19:32 +0200 Subject: [PATCH] =?UTF-8?q?fix(review-2):=20critical=20=E2=80=94=20repair?= =?UTF-8?q?=20comment=20posting=20+=20close=20export=20data=20leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR1: the lightbox comment button POSTed to /upload/{id}/comment (singular); no such route exists, so every UI-submitted comment 404'd and was lost. Fixed to /comments (plural). The prior e2e passed because its seed helper POSTs the API directly — added comment-ui.spec.ts which drives the real component so this can't regress silently again. CR2: export archives (Gallery.zip / Memories.zip / HTML viewer) were written under media_path, which is a public ServeDir — so GET /media/exports/ Gallery.zip served the entire event (every photo, caption, comment, uploader name) to any anonymous visitor at a guessable URL, bypassing the ticket + release gate. Moved exports to a separate EXPORT_PATH (=/exports) on its own volume (Dockerfile chown, compose volume, gated handler reads the new path). export-leak.spec.ts asserts /media/exports/Gallery.zip → 404. Riders (git can't split hunks): LightboxModal also gains H3 live-eviction on comment-deleted/upload-deleted; config.rs also wires compression_concurrency from boot config (medium). Co-Authored-By: Claude Opus 4.8 --- .env.example | 3 ++ backend/Dockerfile | 10 ++-- backend/src/config.rs | 14 +++++ backend/src/services/export.rs | 14 +++-- docker-compose.yml | 4 ++ e2e/specs/03-feed/comment-ui.spec.ts | 53 +++++++++++++++++++ e2e/specs/06-export/export-leak.spec.ts | 22 ++++++++ .../src/lib/components/LightboxModal.svelte | 13 ++++- 8 files changed, 124 insertions(+), 9 deletions(-) create mode 100644 e2e/specs/03-feed/comment-ui.spec.ts create mode 100644 e2e/specs/06-export/export-leak.spec.ts diff --git a/.env.example b/.env.example index f0203d9..ce778bf 100644 --- a/.env.example +++ b/.env.example @@ -32,6 +32,9 @@ EVENT_SLUG=max-maria-2026 # ── Storage ─────────────────────────────────────────────────────────────────── MEDIA_PATH=/media +# Export archives (Gallery.zip / Memories.zip). MUST be outside MEDIA_PATH — +# /media is publicly served, so exports here would be downloadable without auth. +EXPORT_PATH=/exports # ── Upload limits ───────────────────────────────────────────────────────────── DEFAULT_MAX_IMAGE_SIZE_MB=20 diff --git a/backend/Dockerfile b/backend/Dockerfile index 3dc6159..4af576f 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -20,15 +20,17 @@ FROM alpine:3.21 RUN apk add --no-cache ca-certificates ffmpeg -# Run as a non-root user. Pre-create and chown the media mount path so the fresh -# named volume inherits the non-root ownership (Docker seeds an empty named volume -# from the image directory, preserving its uid/gid) and uploads can be written. +# Run as a non-root user. Pre-create and chown the media + export mount paths so +# the fresh named volumes inherit the non-root ownership (Docker seeds an empty +# named volume from the image directory, preserving its uid/gid) and uploads + +# export archives can be written. Exports live OUTSIDE /media on purpose so the +# public media ServeDir can't reach them. RUN addgroup -S app && adduser -S app -G app WORKDIR /app COPY --from=builder /app/target/release/eventsnap-backend ./ -RUN mkdir -p /media && chown -R app:app /app /media +RUN mkdir -p /media /exports && chown -R app:app /app /media /exports USER app EXPOSE 3000 diff --git a/backend/src/config.rs b/backend/src/config.rs index d35e8a7..3cffe02 100644 --- a/backend/src/config.rs +++ b/backend/src/config.rs @@ -56,7 +56,13 @@ pub struct AppConfig { pub event_name: String, pub event_slug: String, pub media_path: PathBuf, + /// Where export archives are written. MUST be outside `media_path` — that + /// directory is served by a public `ServeDir`, so a predictable archive name + /// under it would leak the whole gallery to anonymous visitors. + pub export_path: PathBuf, pub app_port: u16, + /// Number of concurrent media compression workers (read once at boot). + pub compression_concurrency: usize, } impl AppConfig { @@ -86,10 +92,18 @@ impl AppConfig { media_path: PathBuf::from( std::env::var("MEDIA_PATH").unwrap_or_else(|_| "/media".to_string()), ), + export_path: PathBuf::from( + std::env::var("EXPORT_PATH").unwrap_or_else(|_| "/exports".to_string()), + ), app_port: std::env::var("APP_PORT") .unwrap_or_else(|_| "3000".to_string()) .parse() .context("APP_PORT must be a number")?, + compression_concurrency: std::env::var("COMPRESSION_WORKER_CONCURRENCY") + .ok() + .and_then(|v| v.parse().ok()) + .filter(|&n| n >= 1) + .unwrap_or(2), }) } } diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index 0665f16..ad908de 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -87,15 +87,17 @@ pub fn spawn_export_jobs( event_name: String, pool: PgPool, media_path: PathBuf, + export_path: PathBuf, sse_tx: broadcast::Sender, ) { let pool2 = pool.clone(); let media_path2 = media_path.clone(); + let export_path2 = export_path.clone(); let sse_tx2 = sse_tx.clone(); let event_name2 = event_name.clone(); tokio::spawn(async move { - if let Err(e) = run_zip_export(event_id, &pool, &media_path, &sse_tx).await { + if let Err(e) = run_zip_export(event_id, &pool, &media_path, &export_path, &sse_tx).await { tracing::error!("ZIP export failed for event {event_id}: {e:#}"); mark_failed(&pool, event_id, "zip", &e.to_string()).await; } @@ -104,7 +106,7 @@ pub fn spawn_export_jobs( tokio::spawn(async move { if let Err(e) = - run_html_export(event_id, &event_name2, &pool2, &media_path2, &sse_tx2).await + run_html_export(event_id, &event_name2, &pool2, &media_path2, &export_path2, &sse_tx2).await { tracing::error!("HTML export failed for event {event_id}: {e:#}"); mark_failed(&pool2, event_id, "html", &e.to_string()).await; @@ -119,6 +121,7 @@ async fn run_zip_export( event_id: Uuid, pool: &PgPool, media_path: &Path, + export_path: &Path, sse_tx: &broadcast::Sender, ) -> Result<()> { mark_running(pool, event_id, "zip").await; @@ -126,7 +129,8 @@ async fn run_zip_export( let uploads = query_uploads(pool, event_id).await?; let total = uploads.len().max(1) as f32; - let exports_dir = media_path.join("exports"); + // Written OUTSIDE media_path: the public /media ServeDir must never reach these. + let exports_dir = export_path.to_path_buf(); tokio::fs::create_dir_all(&exports_dir).await?; let tmp_path = exports_dir.join("Gallery.zip.tmp"); @@ -193,6 +197,7 @@ async fn run_html_export( event_name: &str, pool: &PgPool, media_path: &Path, + export_path: &Path, sse_tx: &broadcast::Sender, ) -> Result<()> { mark_running(pool, event_id, "html").await; @@ -205,7 +210,8 @@ async fn run_html_export( update_progress(pool, event_id, "html", 5).await; - let exports_dir = media_path.join("exports"); + // Written OUTSIDE media_path: the public /media ServeDir must never reach these. + let exports_dir = export_path.to_path_buf(); tokio::fs::create_dir_all(&exports_dir).await?; // 2. Create temp directory for media processing diff --git a/docker-compose.yml b/docker-compose.yml index 57f1fa7..6baec40 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -34,6 +34,9 @@ services: condition: service_healthy volumes: - media_data:/media + # Export archives live OUTSIDE /media so the public media ServeDir can't + # serve them — downloads go only through the ticket-gated handler. + - exports_data:/exports expose: - "3000" healthcheck: @@ -96,4 +99,5 @@ services: volumes: postgres_data: media_data: + exports_data: caddy_data: diff --git a/e2e/specs/03-feed/comment-ui.spec.ts b/e2e/specs/03-feed/comment-ui.spec.ts new file mode 100644 index 0000000..f0d118e --- /dev/null +++ b/e2e/specs/03-feed/comment-ui.spec.ts @@ -0,0 +1,53 @@ +/** + * Regression for the review's CR1: the LightboxModal posted comments to + * `/upload/{id}/comment` (singular) while the only route is `/comments` (plural), + * so every comment submitted through the UI 404'd and was silently lost. The + * earlier "comment → SSE" spec passed by posting via a fetch helper, bypassing + * the component — a false green. This drives the real component end-to-end. + */ +import { test, expect } from '../../fixtures/test'; +import { seedUpload } from '../../helpers/seed'; + +const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + +test.describe('Comments — UI round-trip (CR1)', () => { + test('a comment typed in the lightbox persists to the backend', async ({ + page, + guest, + signIn, + }) => { + const author = await guest('CommentAuthor'); + const commenter = await guest('Commenter'); + const uploadId = await seedUpload(author.jwt, { caption: 'Comment target' }); + + await signIn(page, commenter); + await page.goto('/feed'); + + // Open the lightbox. Only one upload exists, so the first open-button is it. + const imageButton = page.getByRole('button', { name: 'Bild vergrößern' }).first(); + await expect(imageButton).toBeVisible({ timeout: 15_000 }); + await imageButton.click(); + + const lightbox = page.locator('[role="dialog"][aria-labelledby="lightbox-title"]'); + await expect(lightbox).toBeVisible(); + + const text = `Wunderschönes Foto ${Date.now()}`; + await lightbox.getByPlaceholder(/kommentar/i).fill(text); + await lightbox.getByRole('button', { name: /senden/i }).click(); + + // The component appends the comment only on a 2xx — with the old singular path + // it threw and nothing appeared. Assert it's visible in the panel... + await expect(lightbox.getByText(text)).toBeVisible(); + + // ...and that it actually persisted server-side (the crux CR1 broke). + await expect + .poll(async () => { + const res = await fetch(`${BASE}/api/v1/upload/${uploadId}/comments`, { + headers: { Authorization: `Bearer ${commenter.jwt}` }, + }); + const body = await res.json(); + return Array.isArray(body) && body.some((c: { body: string }) => c.body === text); + }) + .toBe(true); + }); +}); diff --git a/e2e/specs/06-export/export-leak.spec.ts b/e2e/specs/06-export/export-leak.spec.ts new file mode 100644 index 0000000..ff1a21c --- /dev/null +++ b/e2e/specs/06-export/export-leak.spec.ts @@ -0,0 +1,22 @@ +/** + * Regression for the review's CR2: export archives (Gallery.zip / Memories.zip) + * were written under media_path/exports, and /media is a public ServeDir — so + * anyone could GET /media/exports/Gallery.zip and download the whole gallery, + * bypassing the ticket + export_*_ready gate. Exports now live OUTSIDE media_path + * and are reachable only via the gated /api/v1/export/{zip,html} handlers (covered + * by export.spec.ts). Here we assert the public path is dead. + */ +import { test, expect } from '../../fixtures/test'; + +const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + +test.describe('Export — no public leak (CR2)', () => { + test('archives are not reachable through the public /media path', async () => { + for (const name of ['Gallery.zip', 'Memories.zip']) { + const res = await fetch(`${BASE}/media/exports/${name}`); + // 404 (not 200): a 200 here would mean the whole-gallery archive is + // downloadable without any auth — the CR2 data-exposure regression. + expect(res.status).toBe(404); + } + }); +}); diff --git a/frontend/src/lib/components/LightboxModal.svelte b/frontend/src/lib/components/LightboxModal.svelte index 7de599f..1c81612 100644 --- a/frontend/src/lib/components/LightboxModal.svelte +++ b/frontend/src/lib/components/LightboxModal.svelte @@ -2,6 +2,7 @@ import { onDestroy } from 'svelte'; import type { FeedUpload } from '$lib/types'; import { api } from '$lib/api'; + import { onSseEvent } from '$lib/sse'; import { getUserId } from '$lib/auth'; import { dataMode, pickMediaUrl } from '$lib/data-mode-store'; import { doubletap } from '$lib/actions/doubletap'; @@ -48,8 +49,18 @@ burstTimer = setTimeout(() => (heartBurst = false), 700); } + // Drop a comment live when it's deleted elsewhere (host moderation or the + // author on another device), so the open panel doesn't show a ghost comment. + const unsubCommentDeleted = onSseEvent('comment-deleted', (data) => { + try { + const { comment_id } = JSON.parse(data) as { comment_id: string }; + comments = comments.filter((c) => c.id !== comment_id); + } catch { /* ignore */ } + }); + onDestroy(() => { if (burstTimer) clearTimeout(burstTimer); + unsubCommentDeleted(); }); // Only refetch when a *different* upload is shown. The feed reassigns the @@ -74,7 +85,7 @@ if (!newComment.trim()) return; loading = true; try { - const comment = await api.post(`/upload/${upload.id}/comment`, { + const comment = await api.post(`/upload/${upload.id}/comments`, { body: newComment.trim() }); comments = [...comments, comment];