fix(audit-followup): close regressions/gaps from the persona-audit re-review

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 <noreply@anthropic.com>
This commit is contained in:
fabi
2026-07-13 21:19:51 +02:00
parent c647ddfa6b
commit 811c724685
6 changed files with 157 additions and 16 deletions

View File

@@ -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

View File

@@ -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);

View File

@@ -15,6 +15,12 @@ export interface EventState {
export const eventState = writable<EventState>({ 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<void> {
const seq = stateSeq;
try {
const ctx = await api.get<MeContextDto>('/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<void> {
/** 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 });
}

View File

@@ -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<void> {
// 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;
}

View File

@@ -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<EventStatus>('/host/event'),
api.get<UserSummary[]>('/host/users'),
api.get<PinResetRequest[]>('/host/pin-reset-requests'),
api.get<ExportStatusDto>('/export/status')
api.get<PinResetRequest[]>('/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). */