test(e2e): de-vacuum security tests; add quota, authz-sweep, keepsake-regen coverage

An audit found tests that pass on broken code. The dominant pattern: fire a security assertion at the all-zeros UUID and accept [403, 404] — the 404 comes from the resource LOOKUP, not the guard, so the guard can be deleted and the test still passes. Repaired to use real resources and demand exactly 403: - banned-user cannot like / comment (the only coverage of those ban invariants) - host cannot promote a real guest (or self) to admin — asserts nobody's role changed - IDOR comment-delete already used a real resource; kept Also inverted recovery.spec's "unknown name → nicht gefunden" test: that asserted the exact account-enumeration oracle the F4 fix removed, so restoring the vuln would have made it pass. Now: an unknown name must be byte-identical to a wrong PIN. New coverage for paths that ran in production but in zero tests: - quota.spec.ts: storage quota enforcement (413 over-limit, atomic increment under two uploads held mid-body so both carry a stale total=0 — the real race; a naive Promise.all version was itself vacuous and is documented as such). Proven to fail without the guard. - authz-sweep.spec.ts: table-driven guest→403 / host→403 over ALL 19 privileged routes + anonymous + __truncate. No live hole found; the whole surface is now locked. - ban / unban / host-comment-delete AFTER release regenerate the keepsake (data-loss paths that were dead under test); comment-delete mid-build doesn't strand the ZIP. Lower-severity de-vacuuming: 10 MB comment test hit Caddy's 502 before the real 500-char cap (now seeds a real upload, 501→400 / 500→201, mutation-verified); XSS name payloads shortened under the 50-char cap so they actually store+render; ui-rendering XSS test now proves the payload rendered before asserting no <b>; export page-object locators fixed to the real "Download" label with a positive empty-state anchor; avatar palette spread test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-15 07:26:40 +02:00
parent e5201a9889
commit 02971f3186
13 changed files with 681 additions and 67 deletions
--- a/e2e/specs/01-auth/recovery.spec.ts
+++ b/e2e/specs/01-auth/recovery.spec.ts
@@ -26,12 +26,39 @@ test.describe('Auth — /recover route', () => {
    await expect(recover.errorMessage).toContainText(/PIN ist falsch|falsch/i);
  });

-  test('non-existent name returns the "not found" error', async ({ page }) => {
+  // An unknown name must be INDISTINGUISHABLE from a wrong PIN — same status, same message.
+  //
+  // This test used to assert the exact opposite: that an unknown name produced a distinct
+  // "nicht gefunden" error. That IS the account-enumeration oracle the F4 audit fix removed
+  // (recover now answers an unknown name with the same 401 "PIN ist falsch."), so reintroducing
+  // the vulnerability would have made this test pass and `recover-enumeration.spec.ts` fail — two
+  // tests asserting contradictory things about the same behaviour, one of them for the attacker.
+  //
+  // It also passed for a reason unrelated to the name: this test takes no `guest` fixture, so the
+  // per-test TRUNCATE leaves no event row at all, and recover 404s with "Event nicht gefunden." —
+  // which the old regex happily matched.
+  test('an unknown name is indistinguishable from a wrong PIN (no enumeration oracle)', async ({
+    page,
+    guest,
+  }) => {
+    const handle = await guest('Known');
    await clearAllStorage(page);
    const recover = new RecoverPage(page);
+
    await recover.goto();
    await recover.recover('Doesnt-Exist-' + Date.now(), '1234');
-    await expect(recover.errorMessage).toContainText(/nicht gefunden|kein/i);
+    const unknownNameError = (await recover.errorMessage.textContent())?.trim();
+
+    await recover.goto();
+    await recover.recover('Known', handle.pin === '9999' ? '0000' : '9999');
+    const wrongPinError = (await recover.errorMessage.textContent())?.trim();
+
+    expect(unknownNameError).toBeTruthy();
+    expect(
+      unknownNameError,
+      'an unknown name must not be distinguishable from a wrong PIN — that is an account-enumeration oracle'
+    ).toBe(wrongPinError);
+    expect(unknownNameError).not.toMatch(/nicht gefunden|kein benutzer/i);
  });

  test('lockout expires and counter resets (per recover handler)', async ({ api, guest, db }) => {
--- a/e2e/specs/02-upload/quota.spec.ts
+++ b/e2e/specs/02-upload/quota.spec.ts
@@ -0,0 +1,183 @@
+/**
+ * Storage-quota ENFORCEMENT — the branch that runs in production and, until now, in zero tests.
+ *
+ * `quota_enabled` and `storage_quota_enabled` both default to TRUE in production
+ * (upload.rs: `config::get_bool(.., "quota_enabled", true)`), but global-setup turns them off and
+ * the per-test TRUNCATE re-asserts them as 'false' before EVERY test. So the entire quota path was
+ * dead under test: the pre-check, the `QuotaExceeded` rejection, and — most importantly — the
+ * atomic increment
+ *
+ *     UPDATE "user" SET total_upload_bytes = total_upload_bytes + $2
+ *      WHERE id = $1 AND total_upload_bytes + $2 <= $3
+ *
+ * whose enforcement is `rows_affected() == 0`. That UPDATE is the ONLY thing stopping two
+ * concurrent uploads from the same guest (phone + laptop, or a double-tap) from both passing the
+ * stale pre-check and both committing. The five existing unit tests cover `quota_limit_bytes()` —
+ * the arithmetic that computes the number — which gave the appearance of coverage while the
+ * enforcement had none.
+ *
+ * Blast radius if it regresses: one guest silently fills the disk, and then NOBODY at the wedding
+ * can upload. Those photos don't exist anywhere else.
+ *
+ * These tests steer the limit via `quota_tolerance` (limit = floor(free_disk * tolerance / active))
+ * rather than hoping a real disk happens to be nearly full.
+ */
+import { test, expect } from '../../fixtures/test';
+import { join } from 'node:path';
+import { readFileSync } from 'node:fs';
+import { uploadPausedMidStream } from '../../helpers/upload-client';
+
+const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101';
+const SAMPLE_BYTES = readFileSync(join(process.cwd(), 'fixtures', 'media', 'sample.jpg'));
+const SIZE = SAMPLE_BYTES.byteLength;
+
+interface Quota {
+  enabled: boolean;
+  used_bytes: number;
+  limit_bytes: number | null;
+  active_uploaders: number;
+  free_disk_bytes: number | null;
+}
+
+const quotaOf = async (jwt: string): Promise<Quota> =>
+  (await fetch(BASE + '/api/v1/me/quota', { headers: { Authorization: `Bearer ${jwt}` } })).json();
+
+function upload(jwt: string, name: string) {
+  const form = new FormData();
+  form.append('file', new Blob([SAMPLE_BYTES], { type: 'image/jpeg' }), name);
+  form.append('content_type', 'image/jpeg');
+  return fetch(BASE + '/api/v1/upload', {
+    method: 'POST',
+    headers: { Authorization: `Bearer ${jwt}` },
+    body: form,
+  });
+}
+
+/**
+ * Pick a `quota_tolerance` that makes the per-user ceiling land on `targetBytes`.
+ * limit = floor(free_disk * tolerance / max(active, 1))  ⇒  tolerance = target * active / free.
+ */
+async function setLimitTo(
+  api: any,
+  adminToken: string,
+  jwt: string,
+  targetBytes: number
+): Promise<number> {
+  const q = await quotaOf(jwt);
+  expect(q.free_disk_bytes, 'the disk must be readable, else quota fails OPEN and proves nothing').toBeTruthy();
+  const active = Math.max(q.active_uploaders, 1);
+  const tolerance = (targetBytes * active) / (q.free_disk_bytes as number);
+  await api.patchConfig(adminToken, { quota_tolerance: tolerance.toExponential(12) });
+
+  const after = await quotaOf(jwt);
+  expect(after.enabled).toBe(true);
+  return after.limit_bytes as number;
+}
+
+test.describe('Upload — storage quota enforcement', () => {
+  test.beforeEach(async ({ api, adminToken }) => {
+    await api.patchConfig(adminToken, {
+      quota_enabled: 'true',
+      storage_quota_enabled: 'true',
+    });
+  });
+
+  test('an upload that would exceed the quota is rejected with 413 and stores nothing', async ({
+    api,
+    adminToken,
+    guest,
+  }) => {
+    const g = await guest('QuotaOver');
+    // Ceiling below one file: the very first upload must be refused.
+    const limit = await setLimitTo(api, adminToken, g.jwt, Math.floor(SIZE / 2));
+    expect(limit).toBeLessThan(SIZE);
+
+    const res = await upload(g.jwt, 'too-big.jpg');
+    expect(res.status, 'over-quota upload must be refused').toBe(413);
+    expect((await res.json()).error).toBe('quota_exceeded');
+
+    // Nothing was accounted, and nothing reached the feed.
+    expect((await quotaOf(g.jwt)).used_bytes).toBe(0);
+    const feed = await (await fetch(BASE + '/api/v1/feed', { headers: { Authorization: `Bearer ${g.jwt}` } })).json();
+    expect(feed.uploads).toHaveLength(0);
+  });
+
+  test('an upload within the quota still succeeds (the guard is not simply "always no")', async ({
+    api,
+    adminToken,
+    guest,
+  }) => {
+    const g = await guest('QuotaUnder');
+    await setLimitTo(api, adminToken, g.jwt, SIZE * 4);
+
+    expect((await upload(g.jwt, 'fine.jpg')).status).toBe(201);
+    expect((await quotaOf(g.jwt)).used_bytes).toBe(SIZE);
+  });
+
+  test('two CONCURRENT uploads cannot BOTH slip past the quota (atomic increment)', async ({
+    api,
+    adminToken,
+    guest,
+  }) => {
+    const g = await guest('QuotaRacer');
+
+    // Room for exactly ONE file.
+    const limit = await setLimitTo(api, adminToken, g.jwt, Math.floor(SIZE * 1.5));
+    expect(limit).toBeGreaterThanOrEqual(SIZE);
+    expect(limit).toBeLessThan(SIZE * 2);
+
+    // Both uploads are held OPEN MID-BODY, and this is the entire point of the test.
+    //
+    // The handler loads the user row (and therefore snapshots `total_upload_bytes`) at its very
+    // FIRST line — before it streams a single byte of the body. So holding both requests inside
+    // the body loop guarantees both are carrying the same stale `total = 0`, and both pre-checks
+    // will conclude "this fits". Only the conditional UPDATE can then stop the second.
+    //
+    // Firing them with a plain `Promise.all` does NOT reproduce this: the first request completes
+    // and commits before the second even reads the user row, so the *pre-check* rejects the second
+    // and the test passes with the atomic guard deleted. (It did. That is how this test was caught
+    // being vacuous, and why it is written this way.)
+    const head = SAMPLE_BYTES.subarray(0, 32);
+    const tail = SAMPLE_BYTES.subarray(32);
+    const a = uploadPausedMidStream(g.jwt, head, tail, { filename: 'race-a.jpg' });
+    const b = uploadPausedMidStream(g.jwt, head, tail, { filename: 'race-b.jpg' });
+
+    // Both are now past the pre-flight checks and sitting in the body loop, each holding total = 0.
+    await Promise.all([a.checkPassed, b.checkPassed]);
+
+    a.finish();
+    b.finish();
+    const [ra, rb] = await Promise.all([a.response, b.response]);
+    const statuses = [ra.status, rb.status].sort();
+
+    expect(
+      statuses,
+      `exactly one upload may win the race; got ${JSON.stringify(statuses)} — if BOTH are 201 the atomic guard is gone and a single guest can overrun the disk, after which NOBODY at the party can upload`
+    ).toEqual([201, 413]);
+
+    // The invariant that actually matters: accounting never exceeds the ceiling.
+    const q = await quotaOf(g.jwt);
+    expect(q.used_bytes).toBe(SIZE);
+    expect(q.used_bytes).toBeLessThanOrEqual(limit);
+  });
+
+  test('GET /me/quota reports the live usage the UI shows the guest', async ({
+    api,
+    adminToken,
+    guest,
+  }) => {
+    // Zero test hits before this — and it is the source of the "X von Y MB genutzt" widget.
+    const g = await guest('QuotaWidget');
+    await setLimitTo(api, adminToken, g.jwt, SIZE * 10);
+
+    const before = await quotaOf(g.jwt);
+    expect(before.enabled).toBe(true);
+    expect(before.used_bytes).toBe(0);
+
+    expect((await upload(g.jwt, 'one.jpg')).status).toBe(201);
+
+    const after = await quotaOf(g.jwt);
+    expect(after.used_bytes).toBe(SIZE);
+    expect(after.limit_bytes).toBeGreaterThan(after.used_bytes);
+  });
+});
--- a/e2e/specs/05-admin/authz-sweep.spec.ts
+++ b/e2e/specs/05-admin/authz-sweep.spec.ts
@@ -0,0 +1,129 @@
+/**
+ * Table-driven privilege sweep over EVERY host- and admin-gated route.
+ *
+ * Before this file, exactly THREE authorization assertions existed in the whole suite (guest →
+ * /host/event/close, guest → GET /admin/config, host → GET /admin/config). Sixteen of nineteen
+ * privileged routes had no test proving a guest is turned away — including:
+ *
+ *   POST /host/users/{id}/pin-reset  → reset any guest's PIN, then log in as them via /recover.
+ *                                      That is account takeover, and nothing guarded it.
+ *   DELETE /host/upload/{id}         → delete any guest's photo (unrecoverable after the party).
+ *   POST /host/event/open            → retire the released keepsake and reopen uploads to everyone.
+ *   PATCH /admin/config              → only GET was 403-tested; a guest WRITING config could
+ *                                      disable quotas and rate limits outright.
+ *
+ * The point of a table is that adding a route to the router and forgetting to protect it should
+ * make a test fail. So this asserts the WHOLE privileged surface, not a sample of it.
+ *
+ * Deliberately NOT covered here: the read-only-ban model (a banned user keeps read access and can
+ * still download the keepsake) is BY DESIGN, and is asserted in 07-adversarial/authorization-deep.
+ */
+import { test, expect } from '../../fixtures/test';
+
+const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101';
+
+type Method = 'GET' | 'POST' | 'PATCH' | 'DELETE';
+interface Route {
+  method: Method;
+  path: (victimId: string) => string;
+  name: string;
+  body?: unknown;
+  /** admin-only routes must also reject a HOST, not just a guest. */
+  adminOnly?: boolean;
+}
+
+const ROUTES: Route[] = [
+  // ── Host surface ────────────────────────────────────────────────────────
+  { method: 'GET', path: () => '/api/v1/host/event', name: 'GET /host/event' },
+  { method: 'POST', path: () => '/api/v1/host/event/close', name: 'POST /host/event/close' },
+  { method: 'POST', path: () => '/api/v1/host/event/open', name: 'POST /host/event/open' },
+  { method: 'POST', path: () => '/api/v1/host/gallery/release', name: 'POST /host/gallery/release' },
+  { method: 'POST', path: () => '/api/v1/host/export/rebuild', name: 'POST /host/export/rebuild' },
+  { method: 'GET', path: () => '/api/v1/host/users', name: 'GET /host/users' },
+  { method: 'POST', path: (v) => `/api/v1/host/users/${v}/ban`, name: 'POST /host/users/{id}/ban', body: {} },
+  { method: 'POST', path: (v) => `/api/v1/host/users/${v}/unban`, name: 'POST /host/users/{id}/unban', body: {} },
+  { method: 'PATCH', path: (v) => `/api/v1/host/users/${v}/role`, name: 'PATCH /host/users/{id}/role', body: { role: 'host' } },
+  { method: 'POST', path: (v) => `/api/v1/host/users/${v}/pin-reset`, name: 'POST /host/users/{id}/pin-reset', body: {} },
+  { method: 'GET', path: () => '/api/v1/host/pin-reset-requests', name: 'GET /host/pin-reset-requests' },
+  { method: 'DELETE', path: (v) => `/api/v1/host/pin-reset-requests/${v}`, name: 'DELETE /host/pin-reset-requests/{id}' },
+  { method: 'DELETE', path: (v) => `/api/v1/host/upload/${v}`, name: 'DELETE /host/upload/{id}' },
+  { method: 'DELETE', path: (v) => `/api/v1/host/comment/${v}`, name: 'DELETE /host/comment/{id}' },
+
+  // ── Admin surface ───────────────────────────────────────────────────────
+  { method: 'GET', path: () => '/api/v1/admin/stats', name: 'GET /admin/stats', adminOnly: true },
+  { method: 'GET', path: () => '/api/v1/admin/config', name: 'GET /admin/config', adminOnly: true },
+  { method: 'PATCH', path: () => '/api/v1/admin/config', name: 'PATCH /admin/config', adminOnly: true, body: { quota_enabled: 'false' } },
+  { method: 'GET', path: () => '/api/v1/admin/export/jobs', name: 'GET /admin/export/jobs', adminOnly: true },
+];
+
+async function call(route: Route, jwt: string, victimId: string): Promise<number> {
+  const res = await fetch(BASE + route.path(victimId), {
+    method: route.method,
+    headers: {
+      Authorization: `Bearer ${jwt}`,
+      ...(route.body !== undefined ? { 'Content-Type': 'application/json' } : {}),
+    },
+    ...(route.body !== undefined ? { body: JSON.stringify(route.body) } : {}),
+  });
+  return res.status;
+}
+
+test.describe('Authorization sweep — every privileged route', () => {
+  for (const route of ROUTES) {
+    test(`a GUEST is refused: ${route.name}`, async ({ guest }) => {
+      const attacker = await guest('AuthzAttacker');
+      const victim = await guest('AuthzVictim');
+
+      const status = await call(route, attacker.jwt, victim.userId);
+
+      // 403 exactly. NOT "some 4xx" — a 404 would mean the request got past the role gate and
+      // merely failed to find the resource, which is precisely the vacuity this suite has been
+      // bitten by: it would still pass with the RequireHost/RequireAdmin extractor deleted.
+      expect(status, `${route.name} must reject a guest with 403, got ${status}`).toBe(403);
+    });
+  }
+
+  for (const route of ROUTES.filter((r) => r.adminOnly)) {
+    test(`a HOST is refused: ${route.name}`, async ({ host, guest }) => {
+      const victim = await guest('AuthzVictim');
+
+      const status = await call(route, host.jwt, victim.userId);
+
+      expect(status, `${route.name} is admin-only and must reject a host with 403, got ${status}`).toBe(403);
+    });
+  }
+
+  // The truncate endpoint wipes every table AND the media directory. It is test-mode only, but it
+  // is reachable over HTTP and gated solely by RequireAdmin — so it deserves the same proof.
+  test('a GUEST is refused: POST /admin/__truncate (wipes the whole event)', async ({ guest }) => {
+    const attacker = await guest('TruncateAttacker');
+    const res = await fetch(`${BASE}/api/v1/admin/__truncate`, {
+      method: 'POST',
+      headers: { Authorization: `Bearer ${attacker.jwt}` },
+    });
+    expect(res.status).toBe(403);
+  });
+
+  test('a HOST is refused: POST /admin/__truncate', async ({ host }) => {
+    const res = await fetch(`${BASE}/api/v1/admin/__truncate`, {
+      method: 'POST',
+      headers: { Authorization: `Bearer ${host.jwt}` },
+    });
+    expect(res.status).toBe(403);
+  });
+
+  // An unauthenticated caller must not get further than an authenticated-but-unprivileged one.
+  test('an ANONYMOUS caller is refused every privileged route', async () => {
+    const victim = '00000000-0000-0000-0000-000000000000';
+    for (const route of ROUTES) {
+      const res = await fetch(BASE + route.path(victim), {
+        method: route.method,
+        headers: route.body !== undefined ? { 'Content-Type': 'application/json' } : {},
+        ...(route.body !== undefined ? { body: JSON.stringify(route.body) } : {}),
+      });
+      expect([401, 403], `${route.name} must reject an anonymous caller, got ${res.status}`).toContain(
+        res.status
+      );
+    }
+  });
+});
--- a/e2e/specs/06-export/export.spec.ts
+++ b/e2e/specs/06-export/export.spec.ts
@@ -16,8 +16,40 @@ test.describe('Export — release and download', () => {
    await signIn(page, g);
    const exportPage = new ExportPage(page);
    await exportPage.goto();
-    // The page shouldn't show download buttons before release.
-    await expect(page.getByRole('button', { name: /^herunterladen$/i })).not.toBeVisible();
+
+    // POSITIVE anchor first. An absence-only assertion ("no download button") is green on a
+    // blank page, a 404, or an unhydrated shell — i.e. it would pass even with the buttons
+    // rendered, if the locator were wrong. Pin the empty state we actually expect.
+    await expect(exportPage.notAvailableBanner).toBeVisible({ timeout: 10_000 });
+
+    // And only THEN the absence: no download affordance exists before release. This uses the
+    // real button label ("Download"), so rendering a download button here turns it red.
+    await expect(exportPage.downloadButtons).toHaveCount(0);
+  });
+
+  test('/export shows enabled download buttons once released and the jobs are done', async ({
+    page,
+    guest,
+    signIn,
+    db,
+  }) => {
+    const g = await guest('PostRelease');
+    await db.setExportReleased(SLUG, true);
+    await db.fakeExportJob(SLUG, 'zip', 'done');
+    await db.fakeExportJob(SLUG, 'html', 'done');
+
+    await signIn(page, g);
+    const exportPage = new ExportPage(page);
+    await exportPage.goto();
+
+    // The mirror of the test above: this is what proves the "before release" locators can
+    // actually SEE a download button when one exists. Without this, a typo'd locator makes
+    // the pre-release test unfalsifiable.
+    await expect(exportPage.notAvailableBanner).toHaveCount(0);
+    await expect(exportPage.zipDownloadButton).toBeVisible({ timeout: 10_000 });
+    await expect(exportPage.zipDownloadButton).toBeEnabled();
+    await expect(exportPage.htmlDownloadButton).toBeVisible();
+    await expect(exportPage.htmlDownloadButton).toBeEnabled();
  });

  test('export status API reflects released flag', async ({ guest, db }) => {
--- a/e2e/specs/07-adversarial/auth-tampering.spec.ts
+++ b/e2e/specs/07-adversarial/auth-tampering.spec.ts
@@ -110,7 +110,7 @@ test.describe('Adversarial — PIN brute-force', () => {
    expect(correct.status).toBe(429);
  });

-  test('parallel wrong-PIN attempts may NOT all hit lockout (race-condition finding)', async ({ guest }) => {
+  test('parallel wrong-PIN attempts still lock the account (counter is not lost to the race)', async ({ guest }) => {
    const g = await guest('BruteParallel');
    const wrong = g.pin === '0000' ? '1111' : '0000';

@@ -124,12 +124,22 @@ test.describe('Adversarial — PIN brute-force', () => {
      )
    );
    const statuses = attempts.map((r) => r.status);
-    expect(statuses.filter((s) => s === 200)).toHaveLength(0);
-    // Documented behavior: lockout counter may race so not every status is 429.
-    // Critical invariant: no attempt succeeded.
-    if (!statuses.some((s) => s === 429)) {
-      console.warn('[finding] PIN-attempt counter races under parallel requests — none hit lockout.');
-    }
+    expect(statuses.filter((s) => s === 200), 'a wrong PIN must never authenticate').toHaveLength(0);
+
+    // The in-flight requests all read `pin_locked_until` before any of them wrote it, so
+    // *which* of the 10 come back 429 is genuinely racy and can't be asserted. What is NOT
+    // racy — and is the property this test exists to guard — is the state left behind:
+    // `failed_pin_attempts` is incremented with an atomic `SET x = x + 1 ... RETURNING`, so
+    // 10 wrong PINs must push it past the 3-strike threshold and leave the account locked.
+    //
+    // We prove that with a follow-up request using the CORRECT pin: it must be refused with
+    // 429 (locked), not 200. Delete the lockout counter and this line goes 200 → red.
+    const correct = await fetch(`${BASE}/api/v1/recover`, {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({ display_name: g.displayName, pin: g.pin }),
+    });
+    expect(correct.status, 'after 10 wrong PINs the account must be locked, even for the right PIN').toBe(429);
  });
 });

--- a/e2e/specs/07-adversarial/authorization-deep.spec.ts
+++ b/e2e/specs/07-adversarial/authorization-deep.spec.ts
@@ -80,27 +80,39 @@ test.describe('Adversarial — deep authorization', () => {
    expect(row?.caption).toBe('original caption');
  });

+  // These two fire at a REAL upload, and demand exactly 403.
+  //
+  // They used to POST to the all-zeros UUID and accept `[403, 404]`. Both handlers check
+  // `user.is_banned` BEFORE they look the upload up (social.rs) — so the 404 came from the
+  // *lookup*, not the guard. Delete the ban check entirely and the request still 404s on the
+  // nonexistent upload, and both tests still passed. They were the only coverage of
+  // ban-blocks-like and ban-blocks-comment, and they guarded nothing.
  test('banned user cannot toggle a like', async ({ api, host, guest }) => {
    const target = await guest('BannedLike');
+    const uploadId = await seedUpload(host.jwt, { caption: 'likeable' });
    await api.banUser(host.jwt, target.userId);

-    const res = await fetch(`${BASE}/api/v1/upload/00000000-0000-0000-0000-000000000000/like`, {
+    const res = await fetch(`${BASE}/api/v1/upload/${uploadId}/like`, {
      method: 'POST',
      headers: { Authorization: `Bearer ${target.jwt}` },
    });
-    expect([403, 404]).toContain(res.status);
+    expect(res.status, 'a banned user must be Forbidden, not merely miss the resource').toBe(403);
  });

  test('banned user cannot post a comment', async ({ api, host, guest }) => {
    const target = await guest('BannedComment');
+    const uploadId = await seedUpload(host.jwt, { caption: 'commentable' });
    await api.banUser(host.jwt, target.userId);

-    const res = await fetch(`${BASE}/api/v1/upload/00000000-0000-0000-0000-000000000000/comments`, {
+    const res = await fetch(`${BASE}/api/v1/upload/${uploadId}/comments`, {
      method: 'POST',
      headers: { Authorization: `Bearer ${target.jwt}`, 'Content-Type': 'application/json' },
      body: JSON.stringify({ body: 'should be rejected' }),
    });
-    expect([403, 404]).toContain(res.status);
+    expect(res.status, 'a banned user must be Forbidden, not merely miss the resource').toBe(403);
+
+    // ...and nothing was written.
+    expect(await listComments(host.jwt, uploadId)).toHaveLength(0);
  });

  test('banned user can still read the feed (read-only access preserved)', async ({ api, host, guest }) => {
@@ -130,15 +142,40 @@ test.describe('Adversarial — deep authorization', () => {
    expect(stillWorks.status).toBe(200);
  });

-  test('promote endpoint cannot be used to make oneself admin', async ({ host }) => {
-    const res = await fetch(`${BASE}/api/v1/host/users/${'00000000-0000-0000-0000-000000000000'}/role`, {
+  // Privilege escalation, tested against REAL targets.
+  //
+  // The old version PATCHed the all-zeros UUID and accepted `[400, 403, 404]`. The role whitelist
+  // rejects "admin" with a 400 before the target is ever looked up — so adding `"admin"` to the
+  // whitelist would make the request 404 on the nonexistent user instead, which was in the accepted
+  // list. It never promoted anyone, never targeted *oneself*, and could not detect escalation.
+  test('a host cannot promote a real guest to admin', async ({ api, host, guest, adminToken }) => {
+    const victim = await guest('EscalationTarget');
+
+    const res = await fetch(`${BASE}/api/v1/host/users/${victim.userId}/role`, {
      method: 'PATCH',
      headers: { Authorization: `Bearer ${host.jwt}`, 'Content-Type': 'application/json' },
      body: JSON.stringify({ role: 'admin' }),
    });
-    // 400 (invalid role for host-callable endpoint) or 403/404.
-    expect([400, 403, 404]).toContain(res.status);
-    // Critically, NOT 200/204.
-    expect([200, 204]).not.toContain(res.status);
+    expect(res.status).not.toBe(204);
+    expect(res.status).not.toBe(200);
+
+    // The assertion that actually matters: nobody got promoted.
+    const users = await api.listUsers(adminToken);
+    const row = users.find((u: any) => u.id === victim.userId);
+    expect(row?.role, 'the guest must NOT have become an admin').not.toBe('admin');
+  });
+
+  test('a host cannot promote THEMSELVES to admin', async ({ api, host, adminToken }) => {
+    const res = await fetch(`${BASE}/api/v1/host/users/${host.userId}/role`, {
+      method: 'PATCH',
+      headers: { Authorization: `Bearer ${host.jwt}`, 'Content-Type': 'application/json' },
+      body: JSON.stringify({ role: 'admin' }),
+    });
+    expect(res.status).not.toBe(204);
+    expect(res.status).not.toBe(200);
+
+    const users = await api.listUsers(adminToken);
+    const me = users.find((u: any) => u.id === host.userId);
+    expect(me?.role, 'the host must NOT have self-promoted to admin').not.toBe('admin');
  });
 });
--- a/e2e/specs/07-adversarial/ddos.spec.ts
+++ b/e2e/specs/07-adversarial/ddos.spec.ts
@@ -5,6 +5,7 @@
 */
 import { test, expect } from '../../fixtures/test';
 import { mintSseTicket } from '../../helpers/sse';
+import { seedUpload } from '../../helpers/seed';

 const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101';

@@ -30,19 +31,50 @@ test.describe('Adversarial — small-scale abuse', () => {
    expect(statuses.some((s) => s === 201 || s === 409)).toBe(true);
  });

-  test('10 MB comment body is rejected (multipart-less endpoint)', async ({ guest }) => {
-    const g = await guest('BigComment');
-    const huge = 'A'.repeat(10 * 1024 * 1024);
-    const res = await fetch(`${BASE}/api/v1/upload/00000000-0000-0000-0000-000000000000/comments`, {
+  // The comment body cap lives in [backend/src/handlers/social.rs] `add_comment`:
+  //   if text_chars == 0 || text_chars > 500 → 400
+  // It must be exercised against a REAL upload: the handler looks the upload up (and
+  // 404s) *before* it reaches the length check, so posting to a non-existent id proves
+  // nothing about the cap.
+  async function postComment(jwt: string, uploadId: string, body: string) {
+    return fetch(`${BASE}/api/v1/upload/${uploadId}/comments`, {
      method: 'POST',
-      headers: { Authorization: `Bearer ${g.jwt}`, 'Content-Type': 'application/json' },
-      body: JSON.stringify({ body: huge }),
+      headers: { Authorization: `Bearer ${jwt}`, 'Content-Type': 'application/json' },
+      body: JSON.stringify({ body }),
    });
-    // 400 (length cap), 404 (no such upload), 413 (payload too large), 429 (rate-limited),
-    // or 502 (Caddy rejected the body before it reached the backend) — all fine.
-    expect([400, 404, 413, 429, 502]).toContain(res.status);
-    // Not 200 — that would mean we accepted a 10 MB comment.
-    expect(res.status).not.toBe(200);
+  }
+
+  test('comment body over the 500-char cap is rejected with 400', async ({ guest }) => {
+    const g = await guest('LongComment');
+    const uploadId = await seedUpload(g.jwt);
+
+    const res = await postComment(g.jwt, uploadId, 'A'.repeat(501));
+    expect(res.status, '501 chars must be rejected by the length cap').toBe(400);
+    const json: any = await res.json().catch(() => ({}));
+    expect((json.message ?? '').toLowerCase()).toMatch(/500 zeichen/);
+  });
+
+  test('comment body exactly at the 500-char cap is accepted', async ({ guest }) => {
+    const g = await guest('MaxComment');
+    const uploadId = await seedUpload(g.jwt);
+
+    // The boundary must be inclusive — otherwise the "cap" is really 499 and the
+    // rejection test above would also pass on an off-by-one implementation.
+    const res = await postComment(g.jwt, uploadId, 'A'.repeat(500));
+    expect(res.status, '500 chars is the documented maximum and must be accepted').toBe(201);
+  });
+
+  test('10 MB comment body never reaches the handler (body-size limit rejects it)', async ({ guest }) => {
+    const g = await guest('BigComment');
+    const uploadId = await seedUpload(g.jwt);
+    const huge = 'A'.repeat(10 * 1024 * 1024);
+
+    const res = await postComment(g.jwt, uploadId, huge);
+    // This asserts ONLY what it can prove: a 10 MB JSON body is refused somewhere on the
+    // path (Caddy's request-body limit → 502/413, or the backend's own body limit → 413,
+    // or the 500-char cap if it does get through → 400). The upload exists, so a 404 here
+    // would be a bug, and a 201 would mean we stored a 10 MB comment.
+    expect([400, 413, 502]).toContain(res.status);
  });

  test('SSE: 10 concurrent streams from one user do not crash the server', async ({ guest }) => {
--- a/e2e/specs/07-adversarial/file-upload-attacks.spec.ts
+++ b/e2e/specs/07-adversarial/file-upload-attacks.spec.ts
--- a/e2e/specs/07-adversarial/ui-rendering.spec.ts
+++ b/e2e/specs/07-adversarial/ui-rendering.spec.ts
@@ -26,14 +26,20 @@ test.describe('Adversarial — UI render escape', () => {
    });

    await page.goto('/account');
-    await page.waitForLoadState('domcontentloaded');
+
+    // Render guard FIRST. `domcontentloaded` fires before Svelte hydrates, so asserting
+    // the *absence* of a <b> at that point passes on a page that never rendered the name
+    // at all — a false green on an XSS test. Prove the payload actually reached the DOM
+    // (as escaped text) before concluding anything about how it was rendered.
+    await expect(page.getByText(payload, { exact: false }).first()).toBeVisible({ timeout: 10_000 });

    const fired = await page.evaluate(() => (window as any).__xssFired === true);
    expect(fired).toBe(false);

-    // <b> tag inside the name should also not render as bold — Svelte escapes the entire string.
-    const boldCount = await page.locator('b:has-text("BOLD")').count();
-    expect(boldCount).toBe(0);
+    // <b> tag inside the name should also not render as bold — Svelte escapes the entire
+    // string. toHaveCount auto-retries, so this can't win by racing hydration.
+    await expect(page.locator('b:has-text("BOLD")')).toHaveCount(0);
+    await expect(page.locator('script:has-text("__xssFired")')).toHaveCount(0);
  });

  test('rendering of a known SQL-injection-shaped name does not break the page', async ({ page, api }) => {
--- a/e2e/specs/07-adversarial/xss-injection.spec.ts
+++ b/e2e/specs/07-adversarial/xss-injection.spec.ts
@@ -16,15 +16,29 @@ import { seedUpload, seedComment } from '../../helpers/seed';

 const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101';

+/**
+ * Every payload sets `window.__x = 1` if it executes. The marker is deliberately terse:
+ * the join handler caps display names at 50 chars, and a payload that trips that cap is
+ * rejected at the API — which means it is NEVER stored and NEVER rendered, so the test
+ * that "nothing executed" proves nothing at all. Each payload below is < 50 chars, so it
+ * survives the join and actually reaches the render sink under test.
+ */
 const XSS_PAYLOADS = [
-  `<script>window.__xssFired=true</script>`,
-  `<img src=x onerror="window.__xssFired=true">`,
-  `"><svg onload="window.__xssFired=true">`,
-  `<iframe src="javascript:window.parent.__xssFired=true"></iframe>`,
-  `javascript:window.__xssFired=true`,
-  `<a href="javascript:window.__xssFired=true">click</a>`,
+  `<script>window.__x=1</script>`, // 29
+  `<img src=x onerror="window.__x=1">`, // 34
+  `"><svg onload="window.__x=1">`, // 29
+  `<iframe src="javascript:parent.__x=1"></iframe>`, // 47
+  `javascript:window.__x=1`, // 23
+  `<a href="javascript:window.__x=1">c</a>`, // 39
 ];

+// Guard the invariant the payloads depend on: if the display-name cap ever changes, or a
+// payload is edited past it, we want a loud failure here rather than six silent no-ops.
+const NAME_MAX = 50;
+for (const p of XSS_PAYLOADS) {
+  if (p.length > NAME_MAX) throw new Error(`XSS payload exceeds the ${NAME_MAX}-char display-name cap and would never be stored: ${p}`);
+}
+
 const SQLI_PAYLOADS = [
  `'; DROP TABLE "user"; --`,
  `' OR 1=1 --`,
@@ -35,19 +49,10 @@ const SQLI_PAYLOADS = [
 test.describe('Adversarial — input injection (display name)', () => {
  for (const payload of XSS_PAYLOADS) {
    test(`name with XSS payload ${JSON.stringify(payload).slice(0, 40)} never executes`, async ({ api, page }) => {
-      // Payloads > 50 chars are rejected by the join handler — that's a valid defense.
-      // Only if the API accepts the payload do we proceed to assert it never executes
-      // when rendered.
-      let res;
-      try {
-        res = await api.join(payload);
-      } catch (e: any) {
-        if (/→ 400/.test(e.message ?? '')) {
-          // Defended at the API. No need to render.
-          return;
-        }
-        throw e;
-      }
+      // No try/catch escape hatch: every payload is short enough to be accepted, so a
+      // rejection here is a real failure (the payload would never be rendered, and the
+      // "nothing executed" assertions below would be vacuous).
+      const res = await api.join(payload);
      expect(res.jwt).toBeTruthy();

      // Render the name in the account page by signing in.
@@ -74,12 +79,12 @@ test.describe('Adversarial — input injection (display name)', () => {
      // so a "nothing fired" pass can't be because the name was never rendered.
      await expect(page.getByText(payload, { exact: false }).first()).toBeVisible({ timeout: 10_000 });

-      const fired = await page.evaluate(() => (window as any).__xssFired === true);
-      expect(fired, 'window.__xssFired should never be set').toBe(false);
+      const fired = await page.evaluate(() => (window as any).__x === 1);
+      expect(fired, 'window.__x should never be set').toBe(false);
      expect(dialogs, 'no dialogs should appear').toHaveLength(0);

      // Inline script tag in the displayed name should be rendered as text, not parsed.
-      const scriptCount = await page.locator('script:has-text("window.__xssFired")').count();
+      const scriptCount = await page.locator('script:has-text("window.__x")').count();
      expect(scriptCount, 'no executable script tags rendered from name').toBe(0);
    });
  }
@@ -101,11 +106,11 @@ test.describe('Adversarial — stored XSS (caption)', () => {
      // Wait for the caption text to land in the DOM (escaped, as literal text).
      await expect(page.getByText('CAPMARK', { exact: false }).first()).toBeVisible({ timeout: 10_000 });

-      expect(await page.evaluate(() => (window as any).__xssFired === true), 'caption XSS must not fire').toBe(false);
+      expect(await page.evaluate(() => (window as any).__x === 1), 'caption XSS must not fire').toBe(false);
      expect(dialogs, 'no dialogs from a caption').toHaveLength(0);
      // The payload must be inert text, not a live element / script.
      expect(await page.locator('img[onerror]').count(), 'no live onerror img from caption').toBe(0);
-      expect(await page.locator('script:has-text("__xssFired")').count(), 'no executable script from caption').toBe(0);
+      expect(await page.locator('script:has-text("window.__x")').count(), 'no executable script from caption').toBe(0);
    });
  }
 });
@@ -114,8 +119,8 @@ test.describe('Adversarial — stored XSS (comment)', () => {
  // The two payloads that actually execute on render (script injection via innerHTML
  // does not) — enough to prove the comment body is escaped without a slow 6× lightbox loop.
  const COMMENT_PAYLOADS = [
-    `<img src=x onerror="window.__xssFired=true">`,
-    `"><svg onload="window.__xssFired=true">`,
+    `<img src=x onerror="window.__x=1">`,
+    `"><svg onload="window.__x=1">`,
  ];
  for (const payload of COMMENT_PAYLOADS) {
    test(`comment with XSS payload ${JSON.stringify(payload).slice(0, 40)} renders inert`, async ({ guest, page, signIn }) => {
@@ -139,7 +144,7 @@ test.describe('Adversarial — stored XSS (comment)', () => {
      // Wait until the comment (marker) has rendered.
      await expect(lightbox.getByText('CMTMARK', { exact: false })).toBeVisible({ timeout: 10_000 });

-      expect(await page.evaluate(() => (window as any).__xssFired === true), 'comment XSS must not fire').toBe(false);
+      expect(await page.evaluate(() => (window as any).__x === 1), 'comment XSS must not fire').toBe(false);
      expect(dialogs, 'no dialogs from a comment').toHaveLength(0);
      expect(await page.locator('img[onerror]').count(), 'no live onerror img from comment').toBe(0);
    });
--- a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts
+++ b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts
@@ -421,6 +421,103 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k
    expect(listing.some((n) => n.includes(keep)), 'the kept photo survives').toBe(true);
  });

+  test('BANNING a guest after release removes their photos from the keepsake', async ({
+    api,
+    host,
+    guest,
+  }) => {
+    // `ban_user` calls `invalidate_and_arm(Affects::Both)` — but every ban in the suite ran against
+    // an UNRELEASED event, where `invalidate_and_arm` returns early. So this regeneration path was
+    // dead code under test.
+    //
+    // The stakes are the whole point of a ban: the host bans someone for posting something abusive,
+    // and if the keepsake doesn't rebuild, that content stays in the archive every guest downloads,
+    // forever. The export query already filters `is_banned` — so the pipeline AGREES the content
+    // shouldn't be there; only the regeneration was missing.
+    const offender = await guest('Offender');
+    const innocent = await guest('Innocent');
+    const badPhoto = await seedUpload(offender.jwt, { caption: 'abusive' });
+    const goodPhoto = await seedUpload(innocent.jwt, { caption: 'lovely' });
+
+    expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204);
+    await waitExportDone(host.jwt);
+    expect(await downloadZipEntries(host.jwt)).toHaveLength(2);
+
+    await api.banUser(host.jwt, offender.userId);
+
+    await waitExportDone(host.jwt);
+    const listing = await downloadZipEntries(host.jwt);
+    expect(
+      listing.some((n) => n.includes(badPhoto)),
+      "a banned guest's photo must not survive in the keepsake everyone downloads"
+    ).toBe(false);
+    expect(listing.some((n) => n.includes(goodPhoto)), 'everyone else keeps their photos').toBe(true);
+    expect(listing).toHaveLength(1);
+  });
+
+  test('UNBANNING a guest after release restores their photos to the keepsake', async ({
+    api,
+    host,
+    guest,
+  }) => {
+    // The mirror image, and the one that LOSES data. A host bans someone by mistake (or bans, then
+    // reconsiders) — unban must put their photos back into the archive. If the regeneration is
+    // missing, that guest's photos are absent from the keepsake everyone keeps, and after the
+    // wedding they are gone: the export is the only copy anyone takes home.
+    const g = await guest('Forgiven');
+    const photo = await seedUpload(g.jwt, { caption: 'restore me' });
+    await seedUpload(host.jwt, { caption: 'host photo' });
+
+    await api.banUser(host.jwt, g.userId);
+    expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204);
+    await waitExportDone(host.jwt);
+    // Banned at release time → their photo is correctly absent.
+    expect(await downloadZipEntries(host.jwt)).toHaveLength(1);
+
+    const unban = await post(`/api/v1/host/users/${g.userId}/unban`, host.jwt);
+    expect(unban.status).toBe(204);
+
+    await waitExportDone(host.jwt);
+    const listing = await downloadZipEntries(host.jwt);
+    expect(
+      listing.some((n) => n.includes(photo)),
+      'an unbanned guest must get their photos back in the keepsake — it is the only copy anyone keeps'
+    ).toBe(true);
+    expect(listing).toHaveLength(2);
+  });
+
+  test('a HOST comment takedown after release keeps the keepsake downloadable', async ({
+    host,
+    guest,
+  }) => {
+    // `DELETE /host/comment/{id}` had ZERO test hits of any kind, and it is one of the two callers
+    // of the ViewerOnly carry-forward. A comment doesn't live in the ZIP (which holds media), so the
+    // ZIP must be CARRIED FORWARD, not rebuilt — but it must still be downloadable afterwards, and
+    // the viewer must lose the comment.
+    const g = await guest('Rude');
+    const uploadId = await seedUpload(host.jwt, { caption: 'pic' });
+    const cRes = await fetch(BASE + `/api/v1/upload/${uploadId}/comments`, {
+      method: 'POST',
+      headers: { Authorization: `Bearer ${g.jwt}`, 'Content-Type': 'application/json' },
+      body: JSON.stringify({ body: 'unfreundlich' }),
+    });
+    const commentId = (await cRes.json()).id;
+
+    expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(204);
+    await waitExportDone(host.jwt);
+
+    const del = await fetch(BASE + `/api/v1/host/comment/${commentId}`, {
+      method: 'DELETE',
+      headers: { Authorization: `Bearer ${host.jwt}` },
+    });
+    expect(del.status).toBe(204);
+
+    // The keepsake must still be downloadable, with its media intact.
+    await waitExportDone(host.jwt);
+    expect(await downloadZipEntries(host.jwt)).toHaveLength(1);
+    expect(await zipJobEpoch()).toBe(await eventEpoch());
+  });
+
  test('a comment deleted while the ZIP is still BUILDING does not strand the ZIP', async ({
    host,
    guest,