From db7c4459d7de6d247e1ba219dc5e185c32bf9fc5 Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 15 Jul 2026 19:48:15 +0200 Subject: [PATCH 1/2] fix(upload): editing a caption after release now regenerates the keepsake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit edit_upload updated the caption/hashtags and nothing else. A caption lives in the HTML viewer keepsake (the ZIP holds media only — export.rs), so after release the downloadable viewer kept showing the OLD caption forever while the live feed showed the new one. Editing stays allowed while locked/released — like comments and likes, the lock freezes new uploads only (USER_JOURNEYS §9.3) — so the fix is to regenerate, not forbid: the edit and an invalidate_and_arm(ViewerOnly) share one transaction (same atomicity as delete_upload), then start_regen after commit. ViewerOnly carries the finished ZIP forward untouched since the media didn't change; when the gallery isn't released, invalidate_and_arm returns None and this is a no-op. Mutation-verified: without it, an edit doesn't bump the epoch and the caption test fails. Also (test isolation): the compression worker now carries a generation counter that TRUNCATE bumps. A worker queued on the concurrency semaphore when a truncate wiped media would otherwise wake in the NEXT test, fail to find its file, and broadcast upload-error/upload-deleted into that test's SSE stream — corrupting any test asserting on toasts or feed. It now abandons itself when the generation moved. No-op in production (TRUNCATE is the only caller). Export workers were already inert across a truncate (epoch guard on a fresh-UUID event). Co-Authored-By: Claude Opus 4.8 --- backend/src/handlers/test_admin.rs | 7 +++++++ backend/src/handlers/upload.rs | 18 ++++++++++++++++++ backend/src/services/compression.rs | 23 +++++++++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/backend/src/handlers/test_admin.rs b/backend/src/handlers/test_admin.rs index b6c40ce..917ccbf 100644 --- a/backend/src/handlers/test_admin.rs +++ b/backend/src/handlers/test_admin.rs @@ -98,6 +98,13 @@ pub async fn truncate_all( // surviving ticket is a dangling reference to a user that no longer exists. state.sse_tickets.clear(); + // Invalidate any in-flight/queued compression task spawned by the previous test. Without this a + // task still waiting on the concurrency semaphore wakes AFTER this wipe, fails to find its + // (now-deleted) file, and broadcasts upload-error/upload-deleted into the NEXT test's SSE + // stream. (Export workers are already inert across a truncate: they are epoch-guarded on the + // event row, and truncate gives the event a fresh random UUID, so their writes match nothing.) + state.compression.bump_generation(); + Ok(StatusCode::NO_CONTENT) } diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 0edcc5a..e0fdbd2 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -416,6 +416,15 @@ pub async fn edit_upload( // Caption update + hashtag wipe-then-relink in one transaction, so a crash // mid-relink can't leave the upload with its hashtags stripped. + // + // Editing is intentionally allowed while uploads are locked or the gallery is released — like + // comments and likes, the lock freezes *new uploads* only (USER_JOURNEYS §9.3). But a caption + // is embedded in the HTML viewer keepsake (the ZIP holds media only — see export.rs), so an + // edit AFTER release must regenerate the viewer, or the downloadable keepsake keeps showing the + // old caption forever while the live feed shows the new one. Same atomicity as delete_upload: + // the edit and its invalidation share one tx so a dropped handler can't leave them disagreeing. + // `Affects::ViewerOnly` carries the finished ZIP forward (the media didn't change); when the + // gallery isn't released, `invalidate_and_arm` returns None and this is a no-op. let mut tx = state.pool.begin().await?; if let Some(ref caption) = body.caption { Upload::update_caption(&mut *tx, upload_id, Some(caption)).await?; @@ -427,7 +436,16 @@ pub async fn edit_upload( Hashtag::link_to_upload(&mut *tx, upload_id, h.id).await?; } } + let regen = crate::services::export::invalidate_and_arm( + &mut tx, + &state.config.event_slug, + crate::services::export::Affects::ViewerOnly, + ) + .await?; tx.commit().await?; + if let Some(r) = regen { + crate::handlers::host::start_regen(&state, r); + } Ok(StatusCode::OK) } diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index 44813c0..29fcb1b 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -1,4 +1,5 @@ use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; use anyhow::{Context, Result}; @@ -15,6 +16,10 @@ pub struct CompressionWorker { pool: PgPool, media_path: PathBuf, sse_tx: broadcast::Sender, + /// Bumped whenever the underlying data is reset out from under in-flight work (only the e2e + /// TRUNCATE does this today). A task captures the value at spawn and abandons itself if it has + /// changed by the time it runs — see `process`. + generation: Arc, } impl CompressionWorker { @@ -24,14 +29,32 @@ impl CompressionWorker { pool, media_path, sse_tx, + generation: Arc::new(AtomicU64::new(0)), } } + /// Invalidate all in-flight and queued compression work. Called by the e2e TRUNCATE endpoint: + /// truncating deletes the upload rows and wipes `media/`, so a worker that was queued on the + /// semaphore when the wipe happened would otherwise wake in the NEXT test, fail to find its + /// file, and broadcast `upload-error` / `upload-deleted` into that test's live SSE stream — + /// corrupting any test that asserts on toasts or feed contents. Bumping the generation makes + /// those stale tasks return silently instead. A no-op in production (never called there). + pub fn bump_generation(&self) { + self.generation.fetch_add(1, Ordering::SeqCst); + } + /// Spawn a background task to process an uploaded file. pub fn process(&self, upload_id: Uuid, original_path: String, mime_type: String) { let worker = self.clone(); + let born_at = worker.generation.load(Ordering::SeqCst); tokio::spawn(async move { let _permit = worker.semaphore.acquire().await; + // The data this task was queued against may have been reset while it waited for a permit + // (e2e TRUNCATE). If so, its file and row are gone; doing anything — including + // broadcasting a failure — would leak into an unrelated test. Abandon quietly. + if worker.generation.load(Ordering::SeqCst) != born_at { + return; + } match worker.do_process(upload_id, &original_path, &mime_type).await { Ok(_) => { tracing::info!("compression completed for upload {upload_id}"); From c48d43f5b3b01140d30d955ae6c92b3de255fd66 Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 15 Jul 2026 19:48:31 +0200 Subject: [PATCH 2/2] test(e2e): cover the untested security lifecycles (PIN reset, logout-all, ban gating, caption) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the coverage gaps the audit flagged and I verified were real: pin-reset-lifecycle.spec.ts (new): the whole "I forgot my PIN" journey (§4) had only its 403 gate tested. Now: the always-204 non-enumeration contract (unknown name looks identical to known); dedup (ON CONFLICT); admins are EXCLUDED from the queue; the 3-per-15-min throttle; and a host reset REVOKES the target's sessions (the pre-reset JWT 401s afterward) and clears the request. Sessions are validated per-request against the DB, so the revoke is observable. logout-everywhere.spec.ts (new): DELETE /sessions ("sign out everywhere") had ZERO tests. Proves it revokes ALL of the caller's sessions across two devices (both tokens 401 after), not just the current one, and doesn't touch another user's sessions. media-gating: the file claimed delete AND ban-hide both revoke preview access, but only delete was exercised. Add the ban case — a banned uploader's gated preview 404s, same as a takedown. (Thumbnail shares the identical find_by_id_visible gate; the seed fixture produces no thumbnail derivative, so preview is the honest thing to assert.) export caption test: an edit after release regenerates the viewer (epoch bumps) while the ZIP is carried forward, not rebuilt — guards the fix in the previous commit. Helpers: api.listPinResetRequests; db.countSessionsForUser / countPinResetRequestsForUser. Co-Authored-By: Claude Opus 4.8 --- e2e/fixtures/api-client.ts | 5 + e2e/fixtures/db.ts | 20 +++ e2e/specs/01-auth/logout-everywhere.spec.ts | 58 +++++++++ e2e/specs/01-auth/pin-reset-lifecycle.spec.ts | 119 ++++++++++++++++++ e2e/specs/07-adversarial/media-gating.spec.ts | 38 ++++++ .../export-reopen-rerelease.spec.ts | 37 ++++++ 6 files changed, 277 insertions(+) create mode 100644 e2e/specs/01-auth/logout-everywhere.spec.ts create mode 100644 e2e/specs/01-auth/pin-reset-lifecycle.spec.ts diff --git a/e2e/fixtures/api-client.ts b/e2e/fixtures/api-client.ts index 238ae0b..c0b35b0 100644 --- a/e2e/fixtures/api-client.ts +++ b/e2e/fixtures/api-client.ts @@ -137,6 +137,11 @@ export class ApiClient { }); } + async listPinResetRequests(token: string): Promise { + const { body } = await this.request('GET', '/host/pin-reset-requests', { token }); + return body; + } + async closeEvent(token: string) { return this.request('POST', '/host/event/close', { token, expectedStatus: [200, 204] }); } diff --git a/e2e/fixtures/db.ts b/e2e/fixtures/db.ts index f881632..6a001fc 100644 --- a/e2e/fixtures/db.ts +++ b/e2e/fixtures/db.ts @@ -59,6 +59,26 @@ export const db = { }); }, + async countSessionsForUser(userId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ count: string }>( + `SELECT COUNT(*)::text AS count FROM session WHERE user_id = $1`, + [userId] + ); + return Number(r.rows[0].count); + }); + }, + + async countPinResetRequestsForUser(userId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ count: string }>( + `SELECT COUNT(*)::text AS count FROM pin_reset_request WHERE user_id = $1`, + [userId] + ); + return Number(r.rows[0].count); + }); + }, + async setExportReleased(slug: string, released: boolean) { await withClient((c) => c.query(`UPDATE event SET export_released_at = $2 WHERE slug = $1`, [ diff --git a/e2e/specs/01-auth/logout-everywhere.spec.ts b/e2e/specs/01-auth/logout-everywhere.spec.ts new file mode 100644 index 0000000..9451c00 --- /dev/null +++ b/e2e/specs/01-auth/logout-everywhere.spec.ts @@ -0,0 +1,58 @@ +/** + * "Sign out everywhere" — DELETE /api/v1/sessions. A security control (revoke every device after a + * lost/stolen phone) that had ZERO coverage. Sessions are validated per-request against the DB + * (auth middleware resolves the token hash to a live session row), so revocation is observable: a + * revoked token must stop authenticating. + */ +import { test, expect } from '../../fixtures/test'; + +const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + +const ctx = (jwt: string) => + fetch(`${BASE}/api/v1/me/context`, { headers: { Authorization: `Bearer ${jwt}` } }); + +test.describe('Auth — sign out everywhere', () => { + test('DELETE /sessions revokes ALL of the caller\'s sessions, not just the current one', async ({ + api, + guest, + db, + }) => { + // Two devices for one guest: the join session, plus a second session from a recover login. + const g = await guest('MultiDevice'); + const second = await api.recover(g.displayName, g.pin); + const secondJwt = second.body.jwt; + + // Both tokens work, and there really are two distinct sessions. + expect((await ctx(g.jwt)).status).toBe(200); + expect((await ctx(secondJwt)).status).toBe(200); + expect(await db.countSessionsForUser(g.userId)).toBeGreaterThanOrEqual(2); + + // Sign out everywhere, authenticated as device one. + const res = await fetch(`${BASE}/api/v1/sessions`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${g.jwt}` }, + }); + expect(res.status).toBe(204); + + // BOTH devices are now logged out — the whole point of the control. If it only killed the + // caller's own session, device two would still be live and a stolen phone would keep access. + expect((await ctx(g.jwt)).status, 'the calling device is signed out').toBe(401); + expect((await ctx(secondJwt)).status, 'the OTHER device is signed out too').toBe(401); + expect(await db.countSessionsForUser(g.userId)).toBe(0); + }); + + test('one user signing out everywhere does not touch another user\'s sessions', async ({ + guest, + }) => { + const a = await guest('SignsOut'); + const b = await guest('StaysIn'); + + await fetch(`${BASE}/api/v1/sessions`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${a.jwt}` }, + }); + + expect((await ctx(a.jwt)).status).toBe(401); + expect((await ctx(b.jwt)).status, "another user's session must be untouched").toBe(200); + }); +}); diff --git a/e2e/specs/01-auth/pin-reset-lifecycle.spec.ts b/e2e/specs/01-auth/pin-reset-lifecycle.spec.ts new file mode 100644 index 0000000..7579a06 --- /dev/null +++ b/e2e/specs/01-auth/pin-reset-lifecycle.spec.ts @@ -0,0 +1,119 @@ +/** + * USER_JOURNEYS §4 — the "I forgot my PIN" in-app request lifecycle, end to end. + * + * This whole journey had ZERO functional coverage (only the 403 gate, from the authz sweep). The + * invariants below are all security-relevant and none of them were tested: + * - the always-204 non-enumeration contract (an unknown name must look identical to a known one) + * - the per-IP+name throttle (3 / 15 min) + * - admins are EXCLUDED from the request queue (they recover via password, not PIN) + * - a host reset REVOKES the target's sessions (the forgotten/compromised device is logged out) + */ +import { test, expect } from '../../fixtures/test'; + +const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + +const requestReset = (displayName: string) => + fetch(`${BASE}/api/v1/recover/request`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: displayName }), + }); + +test.describe('PIN reset — in-app request lifecycle', () => { + test('a request for a real guest queues exactly one request the host can see', async ({ + api, + host, + guest, + db, + }) => { + const g = await guest('ForgetfulFrida'); + + expect((await requestReset('ForgetfulFrida')).status).toBe(204); + + // The host sees it... + const list = await api.listPinResetRequests(host.jwt); + expect(list.some((r: any) => r.user_id === g.userId)).toBe(true); + // ...and it's deduped: a second request does not stack a duplicate (ON CONFLICT DO NOTHING). + expect((await requestReset('ForgetfulFrida')).status).toBe(204); + expect(await db.countPinResetRequestsForUser(g.userId)).toBe(1); + }); + + test('an UNKNOWN name is indistinguishable from a known one (204, no queue, no enumeration)', async ({ + host, + api, + guest, + }) => { + // Same status code and no error body as the real path — the only observable difference must be + // in the host's queue, which the requester cannot see. + const known = await guest('KnownKarl'); + expect((await requestReset('KnownKarl')).status).toBe(204); + expect((await requestReset('no-such-guest-' + Date.now())).status).toBe(204); + + // The unknown name queued nothing; the known one queued exactly one. + const list = await api.listPinResetRequests(host.jwt); + expect(list).toHaveLength(1); + expect(list[0].user_id).toBe(known.userId); + }); + + test('an ADMIN never gets a reset request queued (admins recover via password)', async ({ + api, + host, + db, + adminToken, + }) => { + // The admin user exists (adminToken logged them in). Their display name is "Admin" by + // convention; request a reset for it and confirm the queue stays empty — the INSERT filters + // `role <> 'admin'`, so queuing a reset for an admin would be a privilege-relevant leak. + const admin = (await api.listUsers(host.jwt)).find((u: any) => u.role === 'admin'); + expect(admin, 'an admin user must exist').toBeTruthy(); + + expect((await requestReset(admin.display_name)).status).toBe(204); + + expect(await db.countPinResetRequestsForUser(admin.id)).toBe(0); + expect(await api.listPinResetRequests(host.jwt)).toHaveLength(0); + }); + + test('requests are throttled to 3 per 15 min per IP+name', async ({ api, adminToken, guest }) => { + await api.patchConfig(adminToken, { rate_limits_enabled: 'true' }); + await guest('ThrottleTarget'); + + const statuses: number[] = []; + for (let i = 0; i < 5; i++) statuses.push((await requestReset('ThrottleTarget')).status); + + // First 3 accepted, the rest throttled — proving the limiter is real and keyed. + expect(statuses.filter((s) => s === 204)).toHaveLength(3); + expect(statuses.filter((s) => s === 429).length).toBeGreaterThan(0); + + // Restore so the next test's fixtures aren't affected (rate limits share the backend). + await api.patchConfig(adminToken, { rate_limits_enabled: 'false' }); + }); + + test('a host PIN reset revokes the targets sessions and clears the request', async ({ + api, + host, + guest, + db, + }) => { + const g = await guest('CompromisedCarl'); + await requestReset('CompromisedCarl'); + + // The guest has a live session (the join minted one) and a queued request. + expect(await db.countSessionsForUser(g.userId)).toBeGreaterThan(0); + expect(await db.countPinResetRequestsForUser(g.userId)).toBe(1); + + // Host resets the PIN. + await api.resetUserPin(host.jwt, g.userId); + + // The old device is logged out: the guest's pre-reset JWT no longer authenticates. This is the + // security point of a reset — sessions are token-bound, so without the revoke the compromised + // device would stay logged in with full access despite the new PIN. + const stale = await fetch(`${BASE}/api/v1/me/context`, { + headers: { Authorization: `Bearer ${g.jwt}` }, + }); + expect(stale.status, 'the pre-reset session must be revoked').toBe(401); + expect(await db.countSessionsForUser(g.userId)).toBe(0); + + // And the pending request was resolved. + expect(await db.countPinResetRequestsForUser(g.userId)).toBe(0); + }); +}); diff --git a/e2e/specs/07-adversarial/media-gating.spec.ts b/e2e/specs/07-adversarial/media-gating.spec.ts index 84f11ae..47b2f04 100644 --- a/e2e/specs/07-adversarial/media-gating.spec.ts +++ b/e2e/specs/07-adversarial/media-gating.spec.ts @@ -56,4 +56,42 @@ test.describe('Media gating — moderation revokes preview access (F2)', () => { const afterDelete = await fetch(`${BASE}/api/v1/upload/${id}/preview`); expect(afterDelete.status, 'moderation must revoke preview access').toBe(404); }); + + test('the preview is revoked when the UPLOADER is banned (not just on delete)', async ({ + host, + api, + guest, + }) => { + test.setTimeout(30_000); + // The header of this file claims delete AND ban-hide both revoke access, but only delete was + // ever exercised. A ban hides the user's content everywhere (the visibility check filters + // `is_banned` inside `find_by_id_visible`, which gates preview AND thumbnail identically), and + // its whole point is that a direct-URL holder loses the image — so it must 404 the gated + // preview too, exactly like a delete. + const offender = await guest('BannedUploader'); + const id = await seedUpload(offender.jwt, { caption: 'to be hidden' }); + + // Wait for the compression worker to produce the preview. + await expect + .poll( + async () => { + const row = (await api.getFeed(host.jwt)).uploads?.find((u: any) => u.id === id); + return row?.preview_url; + }, + { timeout: 20_000, intervals: [500] } + ) + .toBe(`/api/v1/upload/${id}/preview`); + + // Served while the uploader is in good standing. + expect((await fetch(`${BASE}/api/v1/upload/${id}/preview`)).status).toBe(200); + + // Ban the uploader (default: hide their uploads). + await api.banUser(host.jwt, offender.userId); + + // Must now 404 — the direct-URL holder loses the image, same as a takedown. + expect( + (await fetch(`${BASE}/api/v1/upload/${id}/preview`)).status, + "a banned uploader's preview must be revoked" + ).toBe(404); + }); }); diff --git a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts index 8a0a8e2..7257124 100644 --- a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts +++ b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts @@ -518,6 +518,43 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k expect(await zipJobEpoch()).toBe(await eventEpoch()); }); + test('editing a caption AFTER release regenerates the viewer but carries the ZIP forward', async ({ + host, + }) => { + // edit_upload used to update the caption and NOTHING else — no keepsake invalidation. A caption + // lives in the HTML viewer (the ZIP holds media only), so after release the downloadable viewer + // kept showing the OLD caption forever while the live feed showed the new one. Editing is + // intentionally allowed post-release (like comments), so the fix is to regenerate, not forbid: + // invalidate_and_arm(ViewerOnly) — rebuild the viewer, carry the finished ZIP forward untouched. + const uploadId = await seedUpload(host.jwt, { caption: 'tippfehlr' }); + expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204); + await waitExportDone(host.jwt); + + const zipEpochBefore = await zipJobEpoch(); + const eventEpochBefore = await eventEpoch(); + expect(zipEpochBefore).toBe(eventEpochBefore); + + // The edit is allowed (not blocked by the release lock) ... + const res = await fetch(BASE + `/api/v1/upload/${uploadId}`, { + method: 'PATCH', + headers: { Authorization: `Bearer ${host.jwt}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ caption: 'korrigiert' }), + }); + expect(res.status).toBe(200); + + // ... and it bumped the epoch (the viewer must rebuild) ... + await expect.poll(async () => await eventEpoch(), { timeout: 10_000 }).toBeGreaterThan(eventEpochBefore); + + // ... while the ZIP was CARRIED FORWARD, not rebuilt: its row rides the new epoch (the media + // didn't change, so a full ZIP rebuild would be wasted work). + await waitExportDone(host.jwt); + expect(await zipJobEpoch(), 'the ZIP must be carried forward to the new epoch').toBe( + await eventEpoch() + ); + // The keepsake is still downloadable with its single photo intact. + expect(await downloadZipEntries(host.jwt)).toHaveLength(1); + }); + test('a comment deleted while the ZIP is still BUILDING does not strand the ZIP', async ({ host, guest,