From 0ed97f45cffcce818c8d524708eaf2ae4186a1d3 Mon Sep 17 00:00:00 2001 From: fabi Date: Thu, 2 Jul 2026 22:45:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(review-2):=20address=20re-review=20?= =?UTF-8?q?=E2=80=94=20close=20two=20blocker=20regressions=20+=20harden=20?= =?UTF-8?q?tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-2 re-review (full suite green) found two real defects the passing tests missed, both now fixed and covered: B1 — banned users could still edit/delete their OWN content: edit_upload, delete_upload, delete_comment skipped the is_banned guard that upload/ like/comment got (round-2 removed the global extractor block and missed these three). Added the guard (using auth.is_banned — no extra query). The moderation ban test now asserts 403 on DELETE upload, PATCH upload, and DELETE comment for a banned owner (+ upload survives) — it previously only exercised POST /upload, which is why the gap shipped green. B2 — H3 live-eviction was dead code: 'user-hidden' and 'comment-deleted' were missing from KNOWN_EVENTS, so EventSource never listened and the handlers never fired. Added both. New browser-driven sse-eviction test asserts a hidden user's card actually evicts from an open feed with no reload — the backend-only SseListener checks couldn't catch this. Minor items folded in: - admin dashboard bounces a mid-session-demoted admin to /feed via live /me/context (mirrors the host page) instead of a dead error screen. - feed "new posts" pill cleared on any full refresh (pull-to-refresh no longer strands it). - corrected the stale sse.rs comment (banned users may hold a read-only stream). Verified: backend 35 unit tests; svelte-check 0 errors; e2e 04-host + comment-ui + export-leak 13/13 on chromium (incl. the two new regression tests). Co-Authored-By: Claude Opus 4.8 --- backend/src/handlers/social.rs | 4 +++ backend/src/handlers/sse.rs | 8 ++--- backend/src/handlers/upload.rs | 8 +++++ e2e/specs/04-host/moderation.spec.ts | 47 ++++++++++++++++++++++---- e2e/specs/04-host/sse-eviction.spec.ts | 24 +++++++++++++ frontend/src/lib/sse.ts | 2 ++ frontend/src/routes/admin/+page.svelte | 18 ++++++++-- frontend/src/routes/feed/+page.svelte | 4 +++ 8 files changed, 102 insertions(+), 13 deletions(-) diff --git a/backend/src/handlers/social.rs b/backend/src/handlers/social.rs index 6044c8b..c7a6621 100644 --- a/backend/src/handlers/social.rs +++ b/backend/src/handlers/social.rs @@ -199,6 +199,10 @@ pub async fn delete_comment( auth: AuthUser, Path(comment_id): Path, ) -> Result { + // Banned users keep read access but cannot mutate (USER_JOURNEYS §10). + if auth.is_banned { + return Err(AppError::Forbidden("Du bist gesperrt.".into())); + } let comment = Comment::find_by_id(&state.pool, comment_id) .await? .ok_or_else(|| AppError::NotFound("Kommentar nicht gefunden.".into()))?; diff --git a/backend/src/handlers/sse.rs b/backend/src/handlers/sse.rs index ed63d05..394017b 100644 --- a/backend/src/handlers/sse.rs +++ b/backend/src/handlers/sse.rs @@ -48,10 +48,10 @@ pub async fn stream( .consume(&q.ticket) .ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?; - // NOTE: this authenticates via ticket→session, not the `AuthUser` extractor, so - // it does not re-check `is_banned`. A user banned mid-stream keeps receiving - // events until the connection drops; they cannot reconnect (issue_ticket uses - // AuthUser, which rejects banned users). Documented in docs/SECURITY-BACKLOG.md. + // NOTE: this authenticates via ticket→session, not the `AuthUser` extractor. The + // SSE stream is read-only, and under the read-only-ban model (USER_JOURNEYS §10) + // banned users retain read access — so both minting a ticket and holding a stream + // open are intentionally allowed for banned users; only writes are blocked. Session::find_by_token_hash(&state.pool, &token_hash) .await .map_err(|e| AppError::Internal(e.into()))? diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 7d73f3d..29f05c9 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -272,6 +272,10 @@ pub async fn edit_upload( Path(upload_id): Path, Json(body): Json, ) -> Result { + // Banned users keep read access but cannot mutate (USER_JOURNEYS §10). + if auth.is_banned { + return Err(AppError::Forbidden("Du bist gesperrt.".into())); + } let upload = Upload::find_by_id_and_event(&state.pool, upload_id, auth.event_id) .await? .ok_or_else(|| AppError::NotFound("Upload nicht gefunden.".into()))?; @@ -303,6 +307,10 @@ pub async fn delete_upload( auth: AuthUser, Path(upload_id): Path, ) -> Result { + // Banned users keep read access but cannot mutate (USER_JOURNEYS §10). + if auth.is_banned { + return Err(AppError::Forbidden("Du bist gesperrt.".into())); + } let upload = Upload::find_by_id_and_event(&state.pool, upload_id, auth.event_id) .await? .ok_or_else(|| AppError::NotFound("Upload nicht gefunden.".into()))?; diff --git a/e2e/specs/04-host/moderation.spec.ts b/e2e/specs/04-host/moderation.spec.ts index bcb4c17..a1a3e53 100644 --- a/e2e/specs/04-host/moderation.spec.ts +++ b/e2e/specs/04-host/moderation.spec.ts @@ -4,6 +4,7 @@ * coverage of the buttons lives in a separate UI-focused spec. */ import { test, expect } from '../../fixtures/test'; +import { seedUpload, seedComment } from '../../helpers/seed'; test.describe('Host — moderation API', () => { test('ban with hide_uploads=true sets the right flags', async ({ api, host, guest }) => { @@ -104,20 +105,52 @@ test.describe('Host — live role/ban revocation (H1)', () => { test('a banned user keeps read access but is blocked from writes', async ({ api, host, guest }) => { const base = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; const target = await guest('BannedRW'); + const auth = (jwt: string) => ({ Authorization: `Bearer ${jwt}` }); + + // Seed content the target OWNS *before* the ban, so the delete/edit paths get + // past the ownership check and it's genuinely the ban guard being exercised. + const ownUpload = await seedUpload(target.jwt, { caption: 'mine' }); + const ownComment = await seedComment(target.jwt, ownUpload, 'my comment'); + await api.banUser(host.jwt, target.userId, false); // Reads still succeed. - const read = await fetch(base + '/api/v1/me/context', { - headers: { Authorization: `Bearer ${target.jwt}` }, - }); + const read = await fetch(base + '/api/v1/me/context', { headers: auth(target.jwt) }); expect(read.status).toBe(200); - // Writes are rejected. - const write = await fetch(base + '/api/v1/upload', { + // Every write is rejected — cover ALL guest-reachable mutations, not just + // /upload. delete_upload / delete_comment / edit_upload previously skipped the + // ban guard (a banned owner could still delete/edit their own content). + const create = await fetch(base + '/api/v1/upload', { method: 'POST', - headers: { Authorization: `Bearer ${target.jwt}` }, + headers: auth(target.jwt), body: new FormData(), }); - expect(write.status).toBe(403); + expect(create.status).toBe(403); + + const delUpload = await fetch(base + `/api/v1/upload/${ownUpload}`, { + method: 'DELETE', + headers: auth(target.jwt), + }); + expect(delUpload.status).toBe(403); + + const editUpload = await fetch(base + `/api/v1/upload/${ownUpload}`, { + method: 'PATCH', + headers: { ...auth(target.jwt), 'Content-Type': 'application/json' }, + body: JSON.stringify({ caption: 'edited while banned' }), + }); + expect(editUpload.status).toBe(403); + + const delComment = await fetch(base + `/api/v1/comment/${ownComment}`, { + method: 'DELETE', + headers: auth(target.jwt), + }); + expect(delComment.status).toBe(403); + + // The upload survived every blocked write (still fetchable via the feed). + const feed = await fetch(base + '/api/v1/feed', { headers: auth(host.jwt) }); + const body = await feed.json(); + const rows = body.uploads ?? body.items ?? body; + expect(Array.isArray(rows) && rows.some((u: any) => u.id === ownUpload)).toBe(true); }); }); diff --git a/e2e/specs/04-host/sse-eviction.spec.ts b/e2e/specs/04-host/sse-eviction.spec.ts index 5fef7dc..a7f698c 100644 --- a/e2e/specs/04-host/sse-eviction.spec.ts +++ b/e2e/specs/04-host/sse-eviction.spec.ts @@ -48,4 +48,28 @@ test.describe('Host — live SSE eviction (H3)', () => { (e) => e.data.user_id === target.userId ); }); + + // Frontend regression: the broadcasts above are inert if the client never + // registers the event name (the KNOWN_EVENTS gap that shipped both eviction + // handlers as dead code). Drive a real browser feed and assert LIVE eviction — + // this fails if 'user-hidden' is missing from KNOWN_EVENTS, unlike the + // backend-only SseListener checks above. + test('a hidden user is evicted from an open feed without reload (frontend)', async ({ + page, + api, + host, + guest, + signIn, + }) => { + const viewer = await guest('LiveEvictViewer'); + const target = await guest('LiveEvictTarget'); + await seedUpload(target.jwt, { caption: 'evict-me-live-xyz' }); + + await signIn(page, viewer); // lands on the event-wide /feed + await expect(page.getByText('evict-me-live-xyz').first()).toBeVisible(); + + // Host hides the target — the viewer's feed must drop the card via SSE, no reload. + await api.banUser(host.jwt, target.userId, true); + await expect(page.getByText('evict-me-live-xyz')).toHaveCount(0, { timeout: 15_000 }); + }); }); diff --git a/frontend/src/lib/sse.ts b/frontend/src/lib/sse.ts index a5ca62f..1fc2fa7 100644 --- a/frontend/src/lib/sse.ts +++ b/frontend/src/lib/sse.ts @@ -38,6 +38,8 @@ const KNOWN_EVENTS = [ 'upload-deleted', 'like-update', 'new-comment', + 'comment-deleted', + 'user-hidden', 'event-closed', 'event-opened', 'event-updated', diff --git a/frontend/src/routes/admin/+page.svelte b/frontend/src/routes/admin/+page.svelte index 78cf12b..85f3dfe 100644 --- a/frontend/src/routes/admin/+page.svelte +++ b/frontend/src/routes/admin/+page.svelte @@ -158,8 +158,22 @@ onMount(async () => { const token = getToken(); - const role = getRole(); - if (!token || role !== 'admin') { + if (!token) { + goto('/admin/login'); + return; + } + // Trust the *live* role, not the JWT claim: a mid-session demote leaves a + // stale 'admin' in the token, but the backend now 403s every admin call. + // Bounce a demoted admin to the feed instead of leaving them on a dashboard + // that errors on load (mirrors the host page). + try { + const ctx = await api.get<{ role: string }>('/me/context'); + if (ctx.role !== 'admin') { + goto('/feed'); + return; + } + } catch { + // Expired/invalid session (api.ts cleared it) — send them to re-auth. goto('/admin/login'); return; } diff --git a/frontend/src/routes/feed/+page.svelte b/frontend/src/routes/feed/+page.svelte index d8039a4..4b731c5 100644 --- a/frontend/src/routes/feed/+page.svelte +++ b/frontend/src/routes/feed/+page.svelte @@ -317,6 +317,10 @@ } async function loadFeed(refresh = false) { + // Any full refresh (pill tap, pull-to-refresh, filter change) resyncs page 1, + // so the "new posts" pill is no longer relevant — clear it here rather than + // only in the pill's own onclick, or a pull-to-refresh leaves it stranded. + if (refresh) feedStale = false; try { const params = new URLSearchParams(); if (!refresh && nextCursor) params.set('cursor', nextCursor);