diff --git a/e2e/page-objects/export-page.ts b/e2e/page-objects/export-page.ts index d6a63ca..d8dce62 100644 --- a/e2e/page-objects/export-page.ts +++ b/e2e/page-objects/export-page.ts @@ -1,18 +1,50 @@ import type { Page, Locator } from '@playwright/test'; +/** + * Page object for `/export` (see [frontend/src/routes/export/+page.svelte]). + * + * The two archive buttons are BOTH labelled exactly "Download" — the format is carried by + * the surrounding card ("ZIP-Archiv" / "HTML-Viewer"), not by the accessible name. Earlier + * versions of this file looked for `/zip.*herunter/i` and `/^herunterladen$/i`, which match + * nothing on this page; any test asserting `not.toBeVisible()` on those passed no matter + * what the page rendered. Locators here must match the real UI, so scope by card. + * + * "Herunterladen" IS a real label — but only inside the HTML-guide confirm modal, which is + * a different element entirely. + */ export class ExportPage { readonly page: Page; + /** Empty state shown while the host has not released the export yet. */ readonly notAvailableBanner: Locator; + /** Any "Download" button on the page (both cards). Useful for absence assertions. */ + readonly downloadButtons: Locator; readonly zipDownloadButton: Locator; readonly htmlDownloadButton: Locator; + /** Confirm button inside the "Hinweis zum HTML-Viewer" modal. */ readonly htmlGuideModalContinue: Locator; constructor(page: Page) { this.page = page; - this.notAvailableBanner = page.locator('[data-testid="export-not-available"]'); - this.zipDownloadButton = page.getByRole('button', { name: /zip.*herunter|herunter.*zip/i }).first(); - this.htmlDownloadButton = page.getByRole('button', { name: /html.*herunter|herunter.*html/i }).first(); - this.htmlGuideModalContinue = page.getByRole('button', { name: /^herunterladen$/i }); + this.notAvailableBanner = page.getByText('Export noch nicht verfügbar'); + this.downloadButtons = page.getByRole('button', { name: 'Download', exact: true }); + this.zipDownloadButton = this.cardButton('ZIP-Archiv'); + this.htmlDownloadButton = this.cardButton('HTML-Viewer'); + this.htmlGuideModalContinue = page + .getByRole('dialog') + .getByRole('button', { name: /^herunterladen$/i }); + } + + /** + * The "Download" button inside the card whose heading is `heading`. + * + * Scoped to the card element (`div.rounded-xl`) rather than "any div containing the + * heading" — the latter also matches the page wrapper, which contains BOTH cards' buttons. + */ + private cardButton(heading: string): Locator { + return this.page + .locator('div.rounded-xl') + .filter({ has: this.page.getByRole('heading', { name: heading, exact: true }) }) + .getByRole('button', { name: 'Download', exact: true }); } async goto() { diff --git a/e2e/specs/01-auth/recovery.spec.ts b/e2e/specs/01-auth/recovery.spec.ts index 23d697d..6a6ce2f 100644 --- 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 }) => { diff --git a/e2e/specs/02-upload/quota.spec.ts b/e2e/specs/02-upload/quota.spec.ts new file mode 100644 index 0000000..0c0d12a --- /dev/null +++ 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 => + (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 { + 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); + }); +}); diff --git a/e2e/specs/05-admin/authz-sweep.spec.ts b/e2e/specs/05-admin/authz-sweep.spec.ts new file mode 100644 index 0000000..ac6bcef --- /dev/null +++ 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 { + 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 + ); + } + }); +}); diff --git a/e2e/specs/06-export/export.spec.ts b/e2e/specs/06-export/export.spec.ts index 48d8a1d..44b71b7 100644 --- 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 }) => { diff --git a/e2e/specs/07-adversarial/auth-tampering.spec.ts b/e2e/specs/07-adversarial/auth-tampering.spec.ts index 06480ae..9403889 100644 --- 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); }); }); diff --git a/e2e/specs/07-adversarial/authorization-deep.spec.ts b/e2e/specs/07-adversarial/authorization-deep.spec.ts index 7a22943..d17f579 100644 --- 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'); }); }); diff --git a/e2e/specs/07-adversarial/ddos.spec.ts b/e2e/specs/07-adversarial/ddos.spec.ts index 8779e4d..1b0c57b 100644 --- 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 }) => { diff --git a/e2e/specs/07-adversarial/file-upload-attacks.spec.ts b/e2e/specs/07-adversarial/file-upload-attacks.spec.ts index 8cbe49d..99b5607 100644 Binary files a/e2e/specs/07-adversarial/file-upload-attacks.spec.ts and b/e2e/specs/07-adversarial/file-upload-attacks.spec.ts differ diff --git a/e2e/specs/07-adversarial/ui-rendering.spec.ts b/e2e/specs/07-adversarial/ui-rendering.spec.ts index a82c572..9674374 100644 --- 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 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); - // 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); + // 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 }) => { diff --git a/e2e/specs/07-adversarial/xss-injection.spec.ts b/e2e/specs/07-adversarial/xss-injection.spec.ts index 8dacae0..cf0ba49 100644 --- 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 = [ - ``, - ``, - `">`, - ``, - `javascript:window.__xssFired=true`, - `click`, + ``, // 29 + ``, // 34 + `">`, // 29 + ``, // 47 + `javascript:window.__x=1`, // 23 + `c`, // 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 = [ - ``, - `">`, + ``, + `">`, ]; 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); }); 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 6539025..8a0a8e2 100644 --- 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, diff --git a/frontend/src/lib/avatar.test.ts b/frontend/src/lib/avatar.test.ts index 00b6673..bcb88cc 100644 --- a/frontend/src/lib/avatar.test.ts +++ b/frontend/src/lib/avatar.test.ts @@ -9,12 +9,36 @@ describe('avatarPalette', () => { }); it('is deterministic for the same name', () => { + // Same input, repeated calls AND a separately-constructed equal string — the palette + // must be a pure function of the name's characters, not of identity or call order. expect(avatarPalette('Alice')).toBe(avatarPalette('Alice')); + expect(avatarPalette('Alice')).toBe(avatarPalette('Ali' + 'ce')); + expect(avatarPalette('Zoë Müller')).toBe(avatarPalette('Zoë Müller')); }); it('returns a real palette entry (not neutral) for a non-empty name', () => { expect(avatarPalette('Bob')).toMatch(/bg-(blue|purple|green|amber|rose|teal)-100/); }); + + it('maps different names to different palette entries', () => { + // Without this, `name => name ? PALETTE[0] : NEUTRAL` (i.e. the hash loop deleted) + // passes every other test in this file — the palette would be a constant and every + // avatar in the app would render the same colour. + expect(avatarPalette('Alice')).not.toBe(avatarPalette('Bob')); + }); + + it('spreads names across the whole palette, not just one bucket', () => { + const names = [ + 'Alice', 'Bob', 'Carol', 'Dave', 'Erin', 'Frank', 'Grace', 'Heidi', + 'Ivan', 'Judy', 'Mallory', 'Niaj', 'Olivia', 'Peggy', 'Rupert', 'Sybil', + 'Trent', 'Victor', 'Walter', 'Xena' + ]; + const distinct = new Set(names.map((n) => avatarPalette(n))); + // 6 colours in the palette; 20 names must land on more than a couple of them. This + // catches a hash that collapses (e.g. always returns index 0, or ignores all but the + // first character in a way that clusters). + expect(distinct.size).toBeGreaterThanOrEqual(4); + }); }); describe('initials', () => {