fix(review-2): critical — repair comment posting + close export data leak
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 <noreply@anthropic.com>
This commit is contained in:
@@ -32,6 +32,9 @@ EVENT_SLUG=max-maria-2026
|
|||||||
|
|
||||||
# ── Storage ───────────────────────────────────────────────────────────────────
|
# ── Storage ───────────────────────────────────────────────────────────────────
|
||||||
MEDIA_PATH=/media
|
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 ─────────────────────────────────────────────────────────────
|
# ── Upload limits ─────────────────────────────────────────────────────────────
|
||||||
DEFAULT_MAX_IMAGE_SIZE_MB=20
|
DEFAULT_MAX_IMAGE_SIZE_MB=20
|
||||||
|
|||||||
@@ -20,15 +20,17 @@ FROM alpine:3.21
|
|||||||
|
|
||||||
RUN apk add --no-cache ca-certificates ffmpeg
|
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
|
# Run as a non-root user. Pre-create and chown the media + export mount paths so
|
||||||
# named volume inherits the non-root ownership (Docker seeds an empty named volume
|
# the fresh named volumes inherit the non-root ownership (Docker seeds an empty
|
||||||
# from the image directory, preserving its uid/gid) and uploads can be written.
|
# 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
|
RUN addgroup -S app && adduser -S app -G app
|
||||||
|
|
||||||
WORKDIR /app
|
WORKDIR /app
|
||||||
COPY --from=builder /app/target/release/eventsnap-backend ./
|
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
|
USER app
|
||||||
|
|
||||||
EXPOSE 3000
|
EXPOSE 3000
|
||||||
|
|||||||
@@ -56,7 +56,13 @@ pub struct AppConfig {
|
|||||||
pub event_name: String,
|
pub event_name: String,
|
||||||
pub event_slug: String,
|
pub event_slug: String,
|
||||||
pub media_path: PathBuf,
|
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,
|
pub app_port: u16,
|
||||||
|
/// Number of concurrent media compression workers (read once at boot).
|
||||||
|
pub compression_concurrency: usize,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl AppConfig {
|
impl AppConfig {
|
||||||
@@ -86,10 +92,18 @@ impl AppConfig {
|
|||||||
media_path: PathBuf::from(
|
media_path: PathBuf::from(
|
||||||
std::env::var("MEDIA_PATH").unwrap_or_else(|_| "/media".to_string()),
|
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")
|
app_port: std::env::var("APP_PORT")
|
||||||
.unwrap_or_else(|_| "3000".to_string())
|
.unwrap_or_else(|_| "3000".to_string())
|
||||||
.parse()
|
.parse()
|
||||||
.context("APP_PORT must be a number")?,
|
.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),
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -87,15 +87,17 @@ pub fn spawn_export_jobs(
|
|||||||
event_name: String,
|
event_name: String,
|
||||||
pool: PgPool,
|
pool: PgPool,
|
||||||
media_path: PathBuf,
|
media_path: PathBuf,
|
||||||
|
export_path: PathBuf,
|
||||||
sse_tx: broadcast::Sender<SseEvent>,
|
sse_tx: broadcast::Sender<SseEvent>,
|
||||||
) {
|
) {
|
||||||
let pool2 = pool.clone();
|
let pool2 = pool.clone();
|
||||||
let media_path2 = media_path.clone();
|
let media_path2 = media_path.clone();
|
||||||
|
let export_path2 = export_path.clone();
|
||||||
let sse_tx2 = sse_tx.clone();
|
let sse_tx2 = sse_tx.clone();
|
||||||
let event_name2 = event_name.clone();
|
let event_name2 = event_name.clone();
|
||||||
|
|
||||||
tokio::spawn(async move {
|
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:#}");
|
tracing::error!("ZIP export failed for event {event_id}: {e:#}");
|
||||||
mark_failed(&pool, event_id, "zip", &e.to_string()).await;
|
mark_failed(&pool, event_id, "zip", &e.to_string()).await;
|
||||||
}
|
}
|
||||||
@@ -104,7 +106,7 @@ pub fn spawn_export_jobs(
|
|||||||
|
|
||||||
tokio::spawn(async move {
|
tokio::spawn(async move {
|
||||||
if let Err(e) =
|
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:#}");
|
tracing::error!("HTML export failed for event {event_id}: {e:#}");
|
||||||
mark_failed(&pool2, event_id, "html", &e.to_string()).await;
|
mark_failed(&pool2, event_id, "html", &e.to_string()).await;
|
||||||
@@ -119,6 +121,7 @@ async fn run_zip_export(
|
|||||||
event_id: Uuid,
|
event_id: Uuid,
|
||||||
pool: &PgPool,
|
pool: &PgPool,
|
||||||
media_path: &Path,
|
media_path: &Path,
|
||||||
|
export_path: &Path,
|
||||||
sse_tx: &broadcast::Sender<SseEvent>,
|
sse_tx: &broadcast::Sender<SseEvent>,
|
||||||
) -> Result<()> {
|
) -> Result<()> {
|
||||||
mark_running(pool, event_id, "zip").await;
|
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 uploads = query_uploads(pool, event_id).await?;
|
||||||
let total = uploads.len().max(1) as f32;
|
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?;
|
tokio::fs::create_dir_all(&exports_dir).await?;
|
||||||
|
|
||||||
let tmp_path = exports_dir.join("Gallery.zip.tmp");
|
let tmp_path = exports_dir.join("Gallery.zip.tmp");
|
||||||
@@ -193,6 +197,7 @@ async fn run_html_export(
|
|||||||
event_name: &str,
|
event_name: &str,
|
||||||
pool: &PgPool,
|
pool: &PgPool,
|
||||||
media_path: &Path,
|
media_path: &Path,
|
||||||
|
export_path: &Path,
|
||||||
sse_tx: &broadcast::Sender<SseEvent>,
|
sse_tx: &broadcast::Sender<SseEvent>,
|
||||||
) -> Result<()> {
|
) -> Result<()> {
|
||||||
mark_running(pool, event_id, "html").await;
|
mark_running(pool, event_id, "html").await;
|
||||||
@@ -205,7 +210,8 @@ async fn run_html_export(
|
|||||||
|
|
||||||
update_progress(pool, event_id, "html", 5).await;
|
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?;
|
tokio::fs::create_dir_all(&exports_dir).await?;
|
||||||
|
|
||||||
// 2. Create temp directory for media processing
|
// 2. Create temp directory for media processing
|
||||||
|
|||||||
@@ -34,6 +34,9 @@ services:
|
|||||||
condition: service_healthy
|
condition: service_healthy
|
||||||
volumes:
|
volumes:
|
||||||
- media_data:/media
|
- 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:
|
expose:
|
||||||
- "3000"
|
- "3000"
|
||||||
healthcheck:
|
healthcheck:
|
||||||
@@ -96,4 +99,5 @@ services:
|
|||||||
volumes:
|
volumes:
|
||||||
postgres_data:
|
postgres_data:
|
||||||
media_data:
|
media_data:
|
||||||
|
exports_data:
|
||||||
caddy_data:
|
caddy_data:
|
||||||
|
|||||||
53
e2e/specs/03-feed/comment-ui.spec.ts
Normal file
53
e2e/specs/03-feed/comment-ui.spec.ts
Normal file
@@ -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);
|
||||||
|
});
|
||||||
|
});
|
||||||
22
e2e/specs/06-export/export-leak.spec.ts
Normal file
22
e2e/specs/06-export/export-leak.spec.ts
Normal file
@@ -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);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -2,6 +2,7 @@
|
|||||||
import { onDestroy } from 'svelte';
|
import { onDestroy } from 'svelte';
|
||||||
import type { FeedUpload } from '$lib/types';
|
import type { FeedUpload } from '$lib/types';
|
||||||
import { api } from '$lib/api';
|
import { api } from '$lib/api';
|
||||||
|
import { onSseEvent } from '$lib/sse';
|
||||||
import { getUserId } from '$lib/auth';
|
import { getUserId } from '$lib/auth';
|
||||||
import { dataMode, pickMediaUrl } from '$lib/data-mode-store';
|
import { dataMode, pickMediaUrl } from '$lib/data-mode-store';
|
||||||
import { doubletap } from '$lib/actions/doubletap';
|
import { doubletap } from '$lib/actions/doubletap';
|
||||||
@@ -48,8 +49,18 @@
|
|||||||
burstTimer = setTimeout(() => (heartBurst = false), 700);
|
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(() => {
|
onDestroy(() => {
|
||||||
if (burstTimer) clearTimeout(burstTimer);
|
if (burstTimer) clearTimeout(burstTimer);
|
||||||
|
unsubCommentDeleted();
|
||||||
});
|
});
|
||||||
|
|
||||||
// Only refetch when a *different* upload is shown. The feed reassigns the
|
// Only refetch when a *different* upload is shown. The feed reassigns the
|
||||||
@@ -74,7 +85,7 @@
|
|||||||
if (!newComment.trim()) return;
|
if (!newComment.trim()) return;
|
||||||
loading = true;
|
loading = true;
|
||||||
try {
|
try {
|
||||||
const comment = await api.post<CommentDto>(`/upload/${upload.id}/comment`, {
|
const comment = await api.post<CommentDto>(`/upload/${upload.id}/comments`, {
|
||||||
body: newComment.trim()
|
body: newComment.trim()
|
||||||
});
|
});
|
||||||
comments = [...comments, comment];
|
comments = [...comments, comment];
|
||||||
|
|||||||
Reference in New Issue
Block a user