From 811c724685d0e27eac9a05da10f1fccc5b1350c4 Mon Sep 17 00:00:00 2001 From: fabi Date: Mon, 13 Jul 2026 21:19:51 +0200 Subject: [PATCH] fix(audit-followup): close regressions/gaps from the persona-audit re-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial re-review of the persona-audit commit surfaced two HIGH regressions I'd introduced plus MED/LOW gaps. All fixed: HIGH - Auth privilege escalation: reads are sessionStorage-first, but guest `setAuth` wrote only localStorage — a resident admin token shadowed a new guest login (guest ran as admin on a shared device). setAuth/setAdminAuth now DISPLACE the other store so exactly one identity is resident. Adds unit + e2e regression guards. - Host dashboard: `exportGenerating` used `||`, so a one-half export failure stuck on "wird erstellt…" forever and never showed the re-release hint. Changed to `&&`. MEDIUM - Locked-upload auto-resume was unreliable (`event-opened` has no reconnect replay and SSE is down on non-feed pages) — now also resumes on `feed-delta`, which fires on every SSE reconnect. - Queue-cap eviction could drop a recoverable locked item (parked as `error` with a live blob) — now evicts only `done`/`blocked`. - Host page async-`onMount` leaked SSE handlers if unmounted mid-load — added a `destroyed` guard before registration. LOW - An unparseable 403 body now keeps the blob (reversible) instead of purging it. - `refreshEventState` is sequence-guarded so a late close-refresh can't clobber a reopen. - `/export/status` moved out of the all-or-nothing `reload()` so its failure can't blank the whole host dashboard. Verified: svelte-check 0 errors, 36 frontend unit tests (incl. new displacement guards). Co-Authored-By: Claude Opus 4.8 --- .../admin-guest-displacement.spec.ts | 51 +++++++++++++++++++ frontend/src/lib/auth.test.ts | 38 +++++++++++++- frontend/src/lib/auth.ts | 12 +++++ frontend/src/lib/event-state-store.ts | 12 +++++ frontend/src/lib/upload-queue.ts | 38 ++++++++++---- frontend/src/routes/host/+page.svelte | 22 ++++++-- 6 files changed, 157 insertions(+), 16 deletions(-) create mode 100644 e2e/specs/07-adversarial/admin-guest-displacement.spec.ts diff --git a/e2e/specs/07-adversarial/admin-guest-displacement.spec.ts b/e2e/specs/07-adversarial/admin-guest-displacement.spec.ts new file mode 100644 index 0000000..b628298 --- /dev/null +++ b/e2e/specs/07-adversarial/admin-guest-displacement.spec.ts @@ -0,0 +1,51 @@ +/** + * Regression guard — privilege escalation via mixed auth storage. + * + * The admin JWT lives in sessionStorage; guest JWTs in localStorage; reads are + * sessionStorage-first. So a guest login MUST displace any resident admin token — otherwise + * a leftover admin session on a shared "event laptop" would shadow the guest's own token and + * silently hand the next person who joins full admin rights. This exercises the real UI flow. + */ +import { test, expect } from '../../fixtures/test'; +import { JoinPage } from '../../page-objects'; + +function roleOf(page: import('@playwright/test').Page) { + return page.evaluate(() => { + const t = sessionStorage.getItem('eventsnap_jwt') ?? localStorage.getItem('eventsnap_jwt'); + if (!t) return null; + try { + return JSON.parse(atob(t.split('.')[1])).role; + } catch { + return null; + } + }); +} + +test.describe('AuthZ — guest join displaces a resident admin session', () => { + test('joining as a guest with an admin token resident in sessionStorage yields GUEST rights', async ({ + page, + api, + }) => { + // Simulate an admin who logged in on a shared device and walked away without closing the + // tab — their token sits in sessionStorage. + const adminJwt = await api.adminLogin(); + await page.goto('/'); + await page.evaluate((jwt) => sessionStorage.setItem('eventsnap_jwt', jwt), adminJwt); + + // A different person now joins as a guest in that same tab. + const join = new JoinPage(page); + await join.goto(); + await join.joinAs('DisplacementGuest'); + await join.continueToFeed(); + + // The effective identity must be the GUEST — not the leftover admin. + expect(await roleOf(page)).toBe('guest'); + // The admin token must have been cleared from sessionStorage by the guest login. + expect(await page.evaluate(() => sessionStorage.getItem('eventsnap_jwt'))).toBeNull(); + + // And the guest must NOT be able to reach the admin dashboard. + await page.goto('/admin'); + await page.waitForURL(/admin\/login|join|feed/, { timeout: 5_000 }); + expect(page.url()).not.toMatch(/\/admin(\/)?$/); + }); +}); diff --git a/frontend/src/lib/auth.test.ts b/frontend/src/lib/auth.test.ts index febef54..d22442c 100644 --- a/frontend/src/lib/auth.test.ts +++ b/frontend/src/lib/auth.test.ts @@ -5,7 +5,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; // jsdom localStorage is actually used (the shared mock stubs it to false). vi.mock('$app/environment', () => ({ browser: true, dev: false, building: false, version: 'test' })); -import { setAuth, getToken, getPin, getExpiry, getRole, clearAuth, clearPin } from './auth'; +import { setAuth, setAdminAuth, getToken, getPin, getExpiry, getRole, getUserId, clearAuth, clearPin } from './auth'; /** Build a JWT-shaped string (header.payload.sig) with the given claims. */ function makeJwt(claims: object): string { @@ -13,7 +13,10 @@ function makeJwt(claims: object): string { return `${seg({ alg: 'HS256', typ: 'JWT' })}.${seg(claims)}.sig`; } -beforeEach(() => localStorage.clear()); +beforeEach(() => { + localStorage.clear(); + sessionStorage.clear(); +}); describe('auth — token storage', () => { it('setAuth then getToken/getPin round-trips', () => { @@ -42,6 +45,37 @@ describe('auth — token storage', () => { }); }); +describe('auth — admin (sessionStorage) vs guest (localStorage) identity displacement', () => { + // Regression guard for a privilege-escalation: reads are sessionStorage-first, so a guest + // login MUST displace any resident admin token (else the guest silently runs as admin on a + // shared device), and symmetrically an admin login must displace a resident guest token. + it('a guest login displaces a resident admin session (no privilege escalation)', () => { + setAdminAuth(makeJwt({ role: 'admin' }), 'admin-uid', 'Admin'); + expect(getRole()).toBe('admin'); + + setAuth(makeJwt({ role: 'guest' }), '1234', 'guest-uid', 'Guest'); + expect(getRole()).toBe('guest'); // NOT admin + expect(getUserId()).toBe('guest-uid'); + expect(sessionStorage.getItem('eventsnap_jwt')).toBeNull(); // admin token cleared + }); + + it('an admin login displaces a resident guest session but keeps the guest PIN', () => { + setAuth(makeJwt({ role: 'guest' }), '1234', 'guest-uid', 'Guest'); + setAdminAuth(makeJwt({ role: 'admin' }), 'admin-uid', 'Admin'); + expect(getRole()).toBe('admin'); + expect(getUserId()).toBe('admin-uid'); + expect(localStorage.getItem('eventsnap_jwt')).toBeNull(); // guest token cleared + expect(getPin()).toBe('1234'); // PIN preserved for later recovery + }); + + it('clearAuth wipes both stores', () => { + setAdminAuth(makeJwt({ role: 'admin' }), 'admin-uid', 'Admin'); + clearAuth(); + expect(getToken()).toBeNull(); + expect(sessionStorage.getItem('eventsnap_jwt')).toBeNull(); + }); +}); + describe('auth — JWT claim decode', () => { it('getExpiry decodes the exp claim (seconds → ms Date)', () => { const exp = 2_000_000_000; // far-future unix seconds diff --git a/frontend/src/lib/auth.ts b/frontend/src/lib/auth.ts index a048146..1400e96 100644 --- a/frontend/src/lib/auth.ts +++ b/frontend/src/lib/auth.ts @@ -67,6 +67,12 @@ export function getExpiry(): Date | null { export function setAuth(jwt: string, pin: string | null, userId: string, displayName?: string): void { if (!browser) return; + // DISPLACE any resident admin session: reads are sessionStorage-first, so a leftover + // admin token there would otherwise shadow this guest login and hand the guest admin + // rights on a shared device. Clear the sessionStorage identity so exactly one is resident. + sessionStorage.removeItem(TOKEN_KEY); + sessionStorage.removeItem(USER_ID_KEY); + sessionStorage.removeItem(DISPLAY_NAME_KEY); localStorage.setItem(TOKEN_KEY, jwt); if (pin) { localStorage.setItem(PIN_KEY, pin); @@ -85,6 +91,12 @@ export function setAuth(jwt: string, pin: string | null, userId: string, display */ export function setAdminAuth(jwt: string, userId: string, displayName?: string): void { if (!browser) return; + // DISPLACE any resident guest session so exactly one identity is resident (symmetric with + // setAuth). Keep the guest PIN — it's deliberately preserved for later recovery, and the + // admin has none. Reads are sessionStorage-first, so the admin token now wins cleanly. + localStorage.removeItem(TOKEN_KEY); + localStorage.removeItem(USER_ID_KEY); + localStorage.removeItem(DISPLAY_NAME_KEY); sessionStorage.setItem(TOKEN_KEY, jwt); sessionStorage.setItem(USER_ID_KEY, userId); if (displayName) sessionStorage.setItem(DISPLAY_NAME_KEY, displayName); diff --git a/frontend/src/lib/event-state-store.ts b/frontend/src/lib/event-state-store.ts index 53d56ce..643ef02 100644 --- a/frontend/src/lib/event-state-store.ts +++ b/frontend/src/lib/event-state-store.ts @@ -15,6 +15,12 @@ export interface EventState { export const eventState = writable({ uploadsLocked: false, galleryReleased: false }); +// Monotonic sequence bumped by every synchronous state transition (markClosed/markOpened). +// `refreshEventState` captures it before its async fetch and only applies the result if no +// newer transition happened meanwhile — so a slow `/me/context` reflecting a pre-reopen +// snapshot can't clobber a subsequent `event-opened` with stale `locked=true`. +let stateSeq = 0; + /** True when uploads are closed for any reason (event locked or gallery released). */ export function uploadsClosed(s: EventState): boolean { return s.uploadsLocked || s.galleryReleased; @@ -22,8 +28,12 @@ export function uploadsClosed(s: EventState): boolean { /** Refresh from the server. Non-fatal on failure — the next SSE event reconciles. */ export async function refreshEventState(): Promise { + const seq = stateSeq; try { const ctx = await api.get('/me/context'); + // A close/reopen landed while this was in flight — its result is now authoritative; + // don't overwrite it with our possibly-stale snapshot. + if (seq !== stateSeq) return; eventState.set({ uploadsLocked: ctx.uploads_locked, galleryReleased: ctx.gallery_released @@ -35,10 +45,12 @@ export async function refreshEventState(): Promise { /** Apply an `event-closed` SSE event (uploads just locked). */ export function markClosed(): void { + stateSeq++; eventState.update((s) => ({ ...s, uploadsLocked: true })); } /** Apply an `event-opened` SSE event (uploads reopened → release also cleared). */ export function markOpened(): void { + stateSeq++; eventState.set({ uploadsLocked: false, galleryReleased: false }); } diff --git a/frontend/src/lib/upload-queue.ts b/frontend/src/lib/upload-queue.ts index 99624cd..780a29e 100644 --- a/frontend/src/lib/upload-queue.ts +++ b/frontend/src/lib/upload-queue.ts @@ -55,18 +55,26 @@ function bindOnline(): void { bindOnline(); // Resume the queue when the host reopens the event. A queued upload that hit a locked/ -// released event kept its blob and parked as a retryable `error` (LockedError); the -// `event-opened` SSE flips it back to pending and re-drains, so a photo staged during a -// lock isn't lost across a reopen. One-way import (sse.ts never imports this module). +// released event kept its blob and parked as a retryable `error` (LockedError); flipping it +// back to pending and re-draining recovers it so a photo staged during a lock isn't lost. +// We resume on TWO signals: +// - `event-opened`: the live reopen, when the SSE happens to be connected. +// - `feed-delta`: fired on every SSE (re)connect (see sse.ts `deltaFetchAndFan`). Because +// `event-opened` has no server-side replay and the SSE is disconnected on non-feed pages +// / backgrounded tabs, a reopen is often MISSED live — this catches up on the next +// reconnect. Retrying while still locked just re-parks the item (bounded, one per event). +// One-way import (sse.ts never imports this module). let sseBound = false; function bindSse(): void { if (sseBound || typeof window === 'undefined') return; - onSseEvent('event-opened', () => { + const resume = () => { void (async () => { await requeueRetriable(); await processQueue(); })(); - }); + }; + onSseEvent('event-opened', resume); + onSseEvent('feed-delta', resume); sseBound = true; } bindSse(); @@ -266,12 +274,15 @@ export async function addToQueue( if (dup) return 'duplicate'; // Cap the queue so stuck/blocked blobs can't grow IndexedDB without bound. When full, - // evict the oldest terminal item (done/error/blocked) to make room; if none exist, - // refuse and tell the caller so it can surface a "queue full" message (silent loss of a - // photo the user believed was queued is the worst outcome here). + // evict the oldest item whose blob is already gone or unrecoverable — ONLY `done` (blob + // deleted on success) or `blocked` (blob purged, terminal). NEVER evict `error`: those are + // retryable and still hold a live blob (a network blip, or a locked/released upload that + // resumes on reopen) — evicting one would silently lose a photo the fix deliberately kept. + // If nothing is evictable, refuse and tell the caller so it can surface a "queue full" + // message rather than dropping a photo the user believed was queued. if (mine.length >= MAX_QUEUE_ITEMS) { const evictable = get(queueItems).find( - (i) => i.userId === userId && (i.status === 'done' || i.status === 'error' || i.status === 'blocked') + (i) => i.userId === userId && (i.status === 'done' || i.status === 'blocked') ); if (evictable) { await database.delete(STORE_NAME, evictable.id); @@ -486,7 +497,14 @@ async function uploadItem(id: string): Promise { // A REVERSIBLE lock (event closed / gallery released) is tagged // `uploads_locked` by the backend — keep the blob and park it retryable so // a host reopen resumes it, instead of purging it like a permanent 4xx. - if (body?.error === 'uploads_locked') { + // Also treat ANY 403 we can't positively identify as permanent (`forbidden` + // = banned) as reversible: an unparseable 403 body (proxy/WAF/captive + // portal) must NOT purge the blob — losing a photo is the worst outcome, and + // 403 is the reversible-lock status here. + if ( + body?.error === 'uploads_locked' || + (xhr.status === 403 && body?.error !== 'forbidden') + ) { reject(new LockedError(body?.message || 'Event ist geschlossen.')); break; } diff --git a/frontend/src/routes/host/+page.svelte b/frontend/src/routes/host/+page.svelte index 3405fe4..ffe4c53 100644 --- a/frontend/src/routes/host/+page.svelte +++ b/frontend/src/routes/host/+page.svelte @@ -56,9 +56,12 @@ let exportReady = $derived( exportInfo?.zip?.status === 'done' && exportInfo?.html?.status === 'done' ); + // The keepsake needs BOTH halves, so EITHER failing means the whole thing failed — use + // `&&` (not `||`), else a one-half failure stays stuck on "wird erstellt…" forever and the + // host never sees the "re-release" recovery hint. let exportGenerating = $derived( !!exportInfo?.released && !exportReady && - (exportInfo?.zip?.status !== 'failed' || exportInfo?.html?.status !== 'failed') + exportInfo?.zip?.status !== 'failed' && exportInfo?.html?.status !== 'failed' ); let exportProgress = $derived( Math.min(exportInfo?.zip?.progress_pct ?? 0, exportInfo?.html?.progress_pct ?? 0) @@ -148,6 +151,12 @@ } await reload(); + // The awaits above mean the component can already be destroyed by the time we get + // here (user navigated away mid-load). Svelte doesn't cancel an async onMount, so + // without this guard onDestroy would have run with an empty `sseOff` and we'd leak + // these handlers into the module-global SSE map forever. + if (destroyed) return; + // Live updates so the dashboard doesn't need a manual reload: // - pin-reset-requested: a guest just asked for a reset → refresh the badge/list. // - pin-reset: some host resolved a reset → drop it from the list (two-host race: @@ -161,7 +170,9 @@ ]; }); + let destroyed = false; onDestroy(() => { + destroyed = true; for (const off of sseOff) off(); }); @@ -169,17 +180,20 @@ loading = true; error = null; try { - [event, users, pinResetRequests, exportInfo] = await Promise.all([ + [event, users, pinResetRequests] = await Promise.all([ api.get('/host/event'), api.get('/host/users'), - api.get('/host/pin-reset-requests'), - api.get('/export/status') + api.get('/host/pin-reset-requests') ]); } catch (e: unknown) { error = e instanceof Error ? e.message : 'Fehler beim Laden.'; } finally { loading = false; } + // Export status is a secondary widget — fetch it OUTSIDE the all-or-nothing block above + // (and error-swallowing) so a transient /export/status failure can't blank the whole + // dashboard (users, moderation, settings). + void refreshExportStatus(); } /** Refetch just the pending PIN-reset requests (live badge; avoids a full-page reload). */