From 32dfe6874af043522e40b5a7c4d920e3441a5674 Mon Sep 17 00:00:00 2001 From: fabi Date: Tue, 11 Aug 2026 22:44:48 +0200 Subject: [PATCH] test(e2e): make nine red specs assert the contracts the code actually implements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The e2e suite had never been run during this audit. It failed 9 of 256; seven of those predated the audit's changes, established by building a stack from a clean HEAD worktree and running the same specs against it rather than guessing. Most were stale assertions rather than product defects: - quota.spec solved for a target limit using the observed uploader count, but the divisor is max(active, estimated_guest_count, 1) and that config seeds at 100 — so every limit it aimed for came out 100x small and every "within quota" upload 413'd. - rate-limit-shared-nat destructured `ticket` from a 429 body and fetched with `ticket=undefined`, turning the 429 under test into an unrelated 401. It also faked a release with no archive on disk, so the mint's pre-check 404'd and the per-day limiter was never reached; it now does a real release and asserts 200 rather than "not 429". - ddos allowed only [200,429] from ten concurrent streams, so it failed on the very defence it exercises: four tickets per session survive and the rest correctly 401. Now asserts exactly four, which a tightened cap or an inverted eviction order would catch. - auth-tampering asserted a throttled IP is refused EVEN with the correct password. That contract was deliberately removed — it let any phone on the venue NAT lock the operator out of their own admin panel, with a circular escape hatch. Inverted, plus a new check that a success does not refill an attacker's bucket. - moderation-ui assumed a ban leaves a comment "stuck on screen"; `list_for_upload` filters banned authors, so it is hidden from everyone including the host. Now pins the pair that matters — the ban hides it, and the host's permanent removal survives an unban — and the UI leg it used to own is restored as a separate test on a reachable comment. The export specs mint with `?kind=` now that a download ticket is bound to one archive, and four of them assert the mint's 404 rather than the download's: with the kind always known, the pre-check refuses up front instead of after charging a daily download for an archive that cannot be served. Co-Authored-By: Claude Opus 5 --- e2e/eslint.config.js | 17 ++- e2e/page-objects/account-page.ts | 7 +- .../01-auth/rate-limit-shared-nat.spec.ts | 114 ++++++++++++++++-- e2e/specs/02-upload/quota.spec.ts | 8 ++ e2e/specs/02-upload/rejection-visible.spec.ts | 21 +++- e2e/specs/04-host/moderation-ui.spec.ts | 113 ++++++++++++++--- e2e/specs/04-host/role-identity-reset.spec.ts | 4 +- .../06-export/archive-permissions.spec.ts | 2 +- e2e/specs/06-export/exif-orientation.spec.ts | 2 +- e2e/specs/06-export/export-leak.spec.ts | 2 +- e2e/specs/06-export/export-video.spec.ts | 2 +- e2e/specs/06-export/export.spec.ts | 22 ++-- .../viewer-caption-injection.spec.ts | 2 +- .../06-export/viewer-no-broken-tiles.spec.ts | 2 +- .../07-adversarial/auth-tampering.spec.ts | 51 +++++--- e2e/specs/07-adversarial/ddos.spec.ts | 22 +++- e2e/specs/09-mobile/sheet-escape.spec.ts | 2 +- .../export-reopen-rerelease.spec.ts | 34 ++++-- .../10-flow-review/upload-lock-code.spec.ts | 12 +- 19 files changed, 351 insertions(+), 88 deletions(-) diff --git a/e2e/eslint.config.js b/e2e/eslint.config.js index 6e81c02..d5ec870 100644 --- a/e2e/eslint.config.js +++ b/e2e/eslint.config.js @@ -42,6 +42,21 @@ export default ts.config( }, }, { - ignores: ['node_modules/', 'playwright-report/', 'test-results/', '*.config.js'], + ignores: [ + 'node_modules/', + 'playwright-report/', + 'test-results/', + '*.config.js', + // Standalone dev/load-test scripts, run directly with `node`. They are not part of the + // Playwright tsconfig project, so `projectService: true` cannot type them and every one of + // them failed with "was not found by the project service" — which meant `npm run lint` had + // been exiting non-zero on main, i.e. the e2e lint gate was not gating at all. + // + // Ignoring is the honest fix rather than widening the tsconfig: these are throwaway harness + // scripts, and the type-aware rules that justify the project service (no-floating-promises) + // exist to protect TEST code. + '*.mjs', + 'loadtest/', + ], } ); diff --git a/e2e/page-objects/account-page.ts b/e2e/page-objects/account-page.ts index b2da12a..dcd8f85 100644 --- a/e2e/page-objects/account-page.ts +++ b/e2e/page-objects/account-page.ts @@ -12,8 +12,11 @@ export class AccountPage { this.page = page; this.displayName = page.locator('[data-testid="account-display-name"]'); this.pinDisplay = page.locator('[data-testid="account-pin"]'); - this.leaveButton = page.getByRole('button', { name: /event verlassen/i }); - this.leaveConfirmButton = page.getByRole('button', { name: /^abmelden$/i }); + // Keyed on testids, not visible copy. The button was renamed "Event verlassen" -> + // "Abmelden" and these locators silently went stale for a week — the smoke spec that + // guards eight of nine UA projects runs through `leaveEvent()` below. + this.leaveButton = page.getByTestId('account-logout'); + this.leaveConfirmButton = page.getByTestId('confirm-sheet-confirm'); this.privacyNote = page.locator('[data-testid="privacy-note"]'); } diff --git a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts index 744188b..39dbde6 100644 --- a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts +++ b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts @@ -13,6 +13,7 @@ */ import { test, expect } from '../../fixtures/test'; import { BASE } from '../../helpers/env'; +import { seedUpload } from '../../helpers/seed'; test.describe('Rate limits — guests behind a shared NAT', () => { test('a dozen guests can all join from one IP, and 429s carry Retry-After', async ({ @@ -100,46 +101,139 @@ test.describe('Rate limits — guests behind a shared NAT', () => { expect((await read(b.jwt)).status, 'B must not inherit A’s exhausted bucket').toBe(200); }); + test('one guest sweeping /recover cannot lock the venue — or the host — out of PIN recovery', async ({ + api, + adminToken, + guest, + }) => { + // The sharpest version of this file's whole premise. `/recover` has a cross-name failure + // budget keyed on IP, meant to catch someone sweeping the public name list. Behind the venue + // NAT that budget is SHARED BY THE ENTIRE PARTY, and it used to be checked before the account + // was even looked up — so it refused a correct PIN. + // + // That is the host's problem specifically: hosts are promoted guests whose only credential is + // a 4-digit PIN, so /recover is their only way back in after losing a session. A guest posting + // invented names could deny it to everyone, indefinitely, for the price of ~2 requests/minute. + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + recover_rate_enabled: 'true', + // Raise the per-IP VOLUME ceiling out of the way. It defaults to 30/min, and the + // cross-name FAILURE budget under test is also 30 — so the sweep below would trip the + // volume limiter first and this test would pass for the wrong reason (a 429 that proves + // nothing about whether a correct PIN survives a spent failure budget). + recover_ip_rate_per_min: '500', + }); + + const victim = await guest('RecoverVictim'); + + // Burn the shared per-IP budget with names that do not exist — the cheapest sweep, and the + // one that needs no knowledge of the guest list at all. + for (let i = 0; i < 35; i++) { + await fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: `Ghost${i}-${Date.now()}`, pin: '0000' }), + }); + } + + // A real guest, on that same IP, with their REAL PIN, must still get in. + const res = await fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: 'RecoverVictim', pin: victim.pin }), + }); + expect( + res.status, + 'a correct PIN must survive a spent cross-name budget — otherwise any guest can lock the ' + + 'host out of the only login path they have' + ).toBe(200); + + // ...and the sweep is still answered as a sweep: a WRONG pin gets 429, not a bare 401, so the + // budget still does its job on the traffic it was built for. + const wrong = await fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: 'RecoverVictim', pin: '0001' }), + }); + expect(wrong.status, 'wrong PINs from an exhausted IP are still throttled').toBe(429); + }); + test('the export limit is per-user — one guest cannot spend the whole venue’s quota', async ({ api, adminToken, guest, host, - db, }) => { // The sharpest case: 3 downloads per DAY on an IP key meant the 4th guest to fetch // their keepsake was locked out until tomorrow. - await db.setExportReleased('e2e-test-event', true); + // + // A REAL release, not `setExportReleased`. `/export/ticket` pre-validates that the archive is + // actually servable and answers 404 without charging the limiter — deliberately, so a guest + // never spends one of their three daily downloads on an archive that cannot be served. With + // only the released FLAG set and no archive on disk, every mint here 404'd and the per-day + // limiter under test was never reached at all. + await seedUpload(host.jwt, { caption: 'for the keepsake' }); + expect( + ( + await fetch(`${BASE}/api/v1/host/gallery/release`, { + method: 'POST', + headers: { Authorization: `Bearer ${host.jwt}` }, + }) + ).status + ).toBe(204); + await expect + .poll( + async () => { + const s = await ( + await fetch(`${BASE}/api/v1/export/status`, { + headers: { Authorization: `Bearer ${host.jwt}` }, + }) + ).json(); + return s.released === true && s.zip?.status === 'done'; + }, + { timeout: 90_000, intervals: [500] } + ) + .toBe(true); + await api.patchConfig(adminToken, { rate_limits_enabled: 'true', export_rate_enabled: 'true', export_rate_per_day: '1', }); + // The per-day export limit is charged at the MINT, not at the download: the ticket endpoint is + // the authenticated chokepoint, while `/export/zip` authenticates by ticket alone so a resumed + // transfer doesn't spend another of the guest's daily allowance. So a throttled guest is + // refused with 429 at `/export/ticket` and never reaches the archive. + // + // This helper used to destructure `ticket` from that 429 body regardless, then fetch with + // `ticket=undefined` — turning the 429 under test into an unrelated 401 from the download + // endpoint. Surface the mint's refusal instead; that IS the throttle. const mintAndFetch = async (jwt: string) => { - const res = await fetch(`${BASE}/api/v1/export/ticket`, { + const minted = await fetch(`${BASE}/api/v1/export/ticket?kind=zip`, { method: 'POST', headers: { Authorization: `Bearer ${jwt}` }, }); - const { ticket } = await res.json(); + if (!minted.ok) return minted; + const { ticket } = await minted.json(); return fetch(`${BASE}/api/v1/export/zip?ticket=${encodeURIComponent(ticket)}`); }; const a = await guest('ExportFirst'); const b = await guest('ExportSecond'); - // A spends their single daily allowance. The archive itself may not exist (404) — - // what matters is that the limiter admitted the request rather than 429ing it. - expect((await mintAndFetch(a.jwt)).status).not.toBe(429); + // A spends their single daily allowance — on a real archive, so this is a genuine 200 rather + // than merely "not 429", which would have been satisfied by any error at all. + expect((await mintAndFetch(a.jwt)).status, 'A’s first download must succeed').toBe(200); expect((await mintAndFetch(a.jwt)).status, 'A’s second download is throttled').toBe(429); // B shares A's IP and must still get their keepsake. - expect((await mintAndFetch(b.jwt)).status, 'B must not be locked out by A’s download').not.toBe( - 429 + expect((await mintAndFetch(b.jwt)).status, 'B must not be locked out by A’s download').toBe( + 200 ); // And the host too, for good measure. - expect((await mintAndFetch(host.jwt)).status).not.toBe(429); + expect((await mintAndFetch(host.jwt)).status).toBe(200); }); }); diff --git a/e2e/specs/02-upload/quota.spec.ts b/e2e/specs/02-upload/quota.spec.ts index 34126eb..d51e82b 100644 --- a/e2e/specs/02-upload/quota.spec.ts +++ b/e2e/specs/02-upload/quota.spec.ts @@ -90,6 +90,14 @@ test.describe('Upload — storage quota enforcement', () => { await api.patchConfig(adminToken, { quota_enabled: 'true', storage_quota_enabled: 'true', + // The per-user ceiling divides the disk budget by + // `max(active_uploaders, estimated_guest_count, 1)` — the operator's expected headcount is a + // FLOOR on the divisor, so the ceiling settles early instead of sliding down all evening as + // guests arrive. It is seeded at 100, and `setLimitTo` below solves for a target using the + // OBSERVED uploader count, so every limit it aimed for came out 100x too small and every + // "within the quota" upload 413'd. Pin it to 1 so the divisor is the count the helper + // actually controls; the floor itself is exercised by the Rust unit tests. + estimated_guest_count: '1', }); }); diff --git a/e2e/specs/02-upload/rejection-visible.spec.ts b/e2e/specs/02-upload/rejection-visible.spec.ts index 39a51ac..1c9d346 100644 --- a/e2e/specs/02-upload/rejection-visible.spec.ts +++ b/e2e/specs/02-upload/rejection-visible.spec.ts @@ -55,13 +55,23 @@ test.describe('Upload — a rejected upload is surfaced', () => { await expect(queue, 'the upload queue must be rendered somewhere').toBeVisible({ timeout: 10_000, }); - // Both the status chip ("Gesperrt") and the server's reason ("Du bist gesperrt.") must - // render — the reason is the part that had no UI at all before. - await expect(page.getByText('Gesperrt', { exact: true })).toBeVisible(); + // The server's reason ("Du bist gesperrt.") must render — it had no UI at all before. await expect(page.getByText('Du bist gesperrt.')).toBeVisible(); + // The chip reads "Fehler", NOT "Gesperrt", and that is the fix rather than a regression. + // A ban used to come back as a generic `forbidden`, which purged the blob and moved the row + // to `blocked` — a terminal state with no retry button. So an unban restored everything + // except the photo that was actually in flight, which is the one the guest cares about. + // It is now a distinct `user_banned` code that PARKS the row (status `error`, blob kept, + // `parkedFor: 'unban'`) and resumes it when `user-shown` arrives. + await expect(page.getByText('Gesperrt', { exact: true })).toHaveCount(0); + // Positively, not just negatively: a chip that rendered empty would satisfy the line above. + await expect(page.getByText('Fehler', { exact: true }).first()).toBeVisible(); // 3. The badge must not read as success. It counted only pending/uploading before, so a // rejected item dropped it to 0 — indistinguishable from a completed upload. + // The row is now parked (`error` + `parkedFor: 'unban'`) rather than terminally `blocked`, + // so it is the parked count that must be exactly one — and critically the blob must still + // be there, since that is what an unban replays. await expect .poll( () => @@ -74,7 +84,10 @@ test.describe('Upload — a rejected upload is surfaced', () => { const all = tx.objectStore('queue').getAll(); all.onsuccess = () => resolve( - all.result.filter((r: { status: string }) => r.status === 'blocked').length + all.result.filter( + (r: { status: string; parkedFor?: string; blob?: Blob }) => + r.status === 'error' && r.parkedFor === 'unban' && !!r.blob + ).length ); all.onerror = () => reject(all.error); }; diff --git a/e2e/specs/04-host/moderation-ui.spec.ts b/e2e/specs/04-host/moderation-ui.spec.ts index 33a7cd0..bca27b0 100644 --- a/e2e/specs/04-host/moderation-ui.spec.ts +++ b/e2e/specs/04-host/moderation-ui.spec.ts @@ -108,31 +108,25 @@ test.describe('Host — moderation from the UI', () => { ).toBeVisible({ timeout: 10_000 }); }); - test('a host can remove the comment of a guest they have already banned', async ({ + test('a host removes a guest comment through the lightbox, via the confirm sheet', async ({ page, - api, host, guest, signIn, }) => { - // The deadlock this closes. Ban first, exactly as a host would react to abuse: from then - // on the author gets 403 on their own delete, so if the host has no removal affordance - // the comment is stuck on screen forever. - // The photo belongs to an innocent third party — a ban hides the banned user's OWN - // uploads, so if the comment sat on their own photo the whole card would vanish and - // there would be nothing left to moderate. - const victim = await guest('PhotoOwner'); + // The UI leg of host comment moderation, and the ONLY test that clicks it. The affordance is + // rendered solely by LightboxModal (`$isStaff` gates the trash button, and it routes through a + // ConfirmSheet rather than deleting on first tap). Without this, `pendingCommentDelete` could + // stop being wired to the sheet's onConfirm, or the staff gate could invert, and every + // remaining comment-moderation test would still pass — they all call the API directly. + // + // The author is NOT banned here, deliberately. A ban hides the comment from every reader + // including the host (see the next test), so a banned author's comment is unreachable in the + // UI by construction and cannot exercise this path. + const victim = await guest('LightboxPhotoOwner'); const uploadId = await seedUpload(victim.jwt); - const author = await guest('CommentOffender'); - const commentId = await seedComment(author.jwt, uploadId, 'unangebrachter Kommentar'); - await api.banUser(host.jwt, author.userId); - - // Confirm the deadlock really exists — the author cannot retract it themselves. - const selfDelete = await fetch(`${BASE}/api/v1/comment/${commentId}`, { - method: 'DELETE', - headers: { Authorization: `Bearer ${author.jwt}` }, - }); - expect(selfDelete.status, 'a banned author is blocked from their own delete').toBe(403); + const author = await guest('LightboxCommenter'); + await seedComment(author.jwt, uploadId, 'bitte entfernen'); await signIn(page, host); await page.goto('/feed'); @@ -141,9 +135,88 @@ test.describe('Host — moderation from the UI', () => { await expect(card).toBeVisible({ timeout: 15_000 }); await card.getByRole('button', { name: 'Bild vergrößern' }).click(); - const comment = page.getByText('unangebrachter Kommentar'); + const comment = page.getByText('bitte entfernen'); await expect(comment).toBeVisible({ timeout: 10_000 }); + + // "entfernen" (host removing someone else's) rather than "löschen" (deleting your own) — + // the aria-label distinguishes them and the host must get the host one. await page.getByRole('button', { name: 'Kommentar entfernen' }).first().click(); + // It must NOT delete on first tap; the comment is still there behind the sheet. + await expect(comment).toBeVisible(); + await page.getByTestId('confirm-sheet-confirm').click(); await expect(comment).toHaveCount(0, { timeout: 10_000 }); }); + + test('banning hides a comment for everyone, and the host can still delete it permanently', async ({ + page, + api, + host, + guest, + signIn, + }) => { + // This used to assert that the host could remove a banned author's comment FROM THE FEED, + // on the premise that a ban leaves the comment "stuck on screen forever". That premise no + // longer holds: `Comment::list_for_upload` filters `NOT u.is_banned`, so a ban hides the + // comment from every reader — host included — which is why there was nothing on screen to + // click. The export and hashtag-count queries already filtered banned authors, so this + // brought the live read path in line with them. + // + // But hiding is derived AT READ TIME, and a ban is reversible. Unbanning a guest — because + // the host was hasty, or the guest apologised — would republish the abusive comment. So the + // property worth pinning is the pair: the ban hides it immediately, and the host's permanent + // removal outlives the ban. + // The photo belongs to an innocent third party — a ban hides the banned user's OWN uploads, + // so if the comment sat on their own photo the whole card would vanish with it. + const victim = await guest('PhotoOwner'); + const uploadId = await seedUpload(victim.jwt); + const author = await guest('CommentOffender'); + const commentId = await seedComment(author.jwt, uploadId, 'unangebrachter Kommentar'); + + const listFor = async (jwt: string) => + (await ( + await fetch(`${BASE}/api/v1/upload/${uploadId}/comments`, { + headers: { Authorization: `Bearer ${jwt}` }, + }) + ).json()) as Array<{ id: string }>; + + expect( + (await listFor(host.jwt)).map((c) => c.id), + 'before the ban the comment is live' + ).toContain(commentId); + + await api.banUser(host.jwt, author.userId); + + // The author cannot retract it themselves — so removal has to be the host's to make. + const selfDelete = await fetch(`${BASE}/api/v1/comment/${commentId}`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${author.jwt}` }, + }); + expect(selfDelete.status, 'a banned author is blocked from their own delete').toBe(403); + + // Gone for the host and the photo's owner alike, with no further action. + expect((await listFor(host.jwt)).map((c) => c.id)).not.toContain(commentId); + expect((await listFor(victim.jwt)).map((c) => c.id)).not.toContain(commentId); + + // ...and gone from the rendered feed, which is what the host actually looks at. + await signIn(page, host); + await page.goto('/feed'); + const card = page.locator('article').filter({ hasText: victim.displayName }).first(); + await expect(card).toBeVisible({ timeout: 15_000 }); + await card.getByRole('button', { name: 'Bild vergrößern' }).click(); + await expect(page.getByText('unangebrachter Kommentar')).toHaveCount(0); + + // The permanent removal the host still needs: soft-delete survives an unban, so letting the + // guest back in does not republish what they were banned for. + const removed = await fetch(`${BASE}/api/v1/host/comment/${commentId}`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${host.jwt}` }, + }); + expect(removed.status, 'the host can delete a banned author’s comment outright').toBe(204); + + await api.unbanUser(host.jwt, author.userId); + expect( + (await listFor(host.jwt)).map((c) => c.id), + 'an unban must not resurrect a comment the host deleted' + ).not.toContain(commentId); + }); }); diff --git a/e2e/specs/04-host/role-identity-reset.spec.ts b/e2e/specs/04-host/role-identity-reset.spec.ts index ee8140f..e24f14e 100644 --- a/e2e/specs/04-host/role-identity-reset.spec.ts +++ b/e2e/specs/04-host/role-identity-reset.spec.ts @@ -39,9 +39,9 @@ test.describe('Role — follows the identity across a same-tab switch', () => { await expect(page.getByRole('button', { name: REMOVE })).toBeVisible(); await page.keyboard.press('Escape'); - // 2. Host leaves, in-app — no reload. This is the path "Event verlassen" takes. + // 2. Host leaves, in-app — no reload. This is the path the "Abmelden" button takes. await page.goto('/account'); - await page.getByRole('button', { name: /event verlassen/i }).click(); + await page.getByTestId('account-logout').click(); const confirm = page.getByTestId('confirm-sheet-confirm'); if (await confirm.isVisible().catch(() => false)) await confirm.click(); await page.waitForURL('**/join', { timeout: 10_000 }); diff --git a/e2e/specs/06-export/archive-permissions.spec.ts b/e2e/specs/06-export/archive-permissions.spec.ts index 557b1c0..9ca3e1a 100644 --- a/e2e/specs/06-export/archive-permissions.spec.ts +++ b/e2e/specs/06-export/archive-permissions.spec.ts @@ -59,7 +59,7 @@ test.describe('Export — the archives extract to readable files', () => { .toBe(true); for (const kind of ['zip', 'html'] as const) { - const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, { + const ticketRes = await fetch(`${BASE}/api/v1/export/ticket?kind=${kind}`, { method: 'POST', headers: bearer, }); diff --git a/e2e/specs/06-export/exif-orientation.spec.ts b/e2e/specs/06-export/exif-orientation.spec.ts index 56cccc9..7e9c958 100644 --- a/e2e/specs/06-export/exif-orientation.spec.ts +++ b/e2e/specs/06-export/exif-orientation.spec.ts @@ -80,7 +80,7 @@ test.describe('Export — EXIF orientation in the keepsake', () => { ) .toBe('done'); - const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, { + const ticketRes = await fetch(`${BASE}/api/v1/export/ticket?kind=html`, { method: 'POST', headers: bearer, }); diff --git a/e2e/specs/06-export/export-leak.spec.ts b/e2e/specs/06-export/export-leak.spec.ts index fadb8e3..bb848de 100644 --- a/e2e/specs/06-export/export-leak.spec.ts +++ b/e2e/specs/06-export/export-leak.spec.ts @@ -51,7 +51,7 @@ test.describe('Export — no public leak (CR2)', () => { // …but IS retrievable via the gated single-use ticket endpoint. This proves the // 404 above means "not public", not merely "no file was produced". - const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, { + const ticketRes = await fetch(`${BASE}/api/v1/export/ticket?kind=zip`, { method: 'POST', headers: bearer, }); diff --git a/e2e/specs/06-export/export-video.spec.ts b/e2e/specs/06-export/export-video.spec.ts index a7b9412..fcea2b1 100644 --- a/e2e/specs/06-export/export-video.spec.ts +++ b/e2e/specs/06-export/export-video.spec.ts @@ -53,7 +53,7 @@ test.describe('Export — video streaming (P4)', () => { .toBe('done'); // Download Memories.zip via the gated single-use ticket. - const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, { + const ticketRes = await fetch(`${BASE}/api/v1/export/ticket?kind=html`, { method: 'POST', headers: bearer, }); diff --git a/e2e/specs/06-export/export.spec.ts b/e2e/specs/06-export/export.spec.ts index 8c42c65..d2250fd 100644 --- a/e2e/specs/06-export/export.spec.ts +++ b/e2e/specs/06-export/export.spec.ts @@ -88,12 +88,14 @@ test.describe('Export — release and download', () => { // Browser downloads stream to disk via a top-level navigation, so the download // endpoint authenticates with a single-use ticket (no Bearer header). - async function mintTicket(jwt: string): Promise { - const res = await fetch(base + '/api/v1/export/ticket', { + /** The raw mint response — `/export/ticket` pre-validates that the archive is actually + * servable, so an unavailable keepsake is refused HERE rather than after charging one of the + * guest's three daily downloads. */ + async function mintTicketResponse(jwt: string, kind: 'zip' | 'html' = 'zip') { + return fetch(base + `/api/v1/export/ticket?kind=${kind}`, { method: 'POST', headers: { Authorization: `Bearer ${jwt}` }, }); - return (await res.json()).ticket; } test('ZIP download 404s for a `done` job at a RETIRED epoch', async ({ guest, db }) => { @@ -105,10 +107,12 @@ test.describe('Export — release and download', () => { await db.setExportReleased(SLUG, true); await db.fakeExportJob(SLUG, 'zip', 'done'); await db.setExportZipReady(SLUG, false); // retire the job to a dead epoch - const ticket = await mintTicket(g.jwt); - const res = await fetch(base + '/api/v1/export/zip?ticket=' + encodeURIComponent(ticket)); - expect(res.status).toBe(404); + // Refused at the MINT. This used to be asserted one step later, on the download, because the + // spec did not send `kind` and so skipped the pre-check entirely — now that a ticket is bound + // to an archive the kind is always known, and the guest is told the truth before a daily + // download is spent on an archive that cannot be served. + expect((await mintTicketResponse(g.jwt)).status).toBe(404); }); test('ZIP download 404s when the job is current but the file is missing on disk', async ({ @@ -122,9 +126,9 @@ test.describe('Export — release and download', () => { await db.setExportReleased(SLUG, true); await db.fakeExportJob(SLUG, 'zip', 'done'); await db.setExportZipReady(SLUG, true); - const ticket = await mintTicket(g.jwt); - const res = await fetch(base + '/api/v1/export/zip?ticket=' + encodeURIComponent(ticket)); - expect(res.status).toBe(404); + // Same as above: the pre-check resolves the file on disk, so a `done` job whose archive is + // missing is refused at the mint rather than 404ing mid-download. + expect((await mintTicketResponse(g.jwt)).status).toBe(404); }); }); diff --git a/e2e/specs/06-export/viewer-caption-injection.spec.ts b/e2e/specs/06-export/viewer-caption-injection.spec.ts index 9beabb7..47fcce2 100644 --- a/e2e/specs/06-export/viewer-caption-injection.spec.ts +++ b/e2e/specs/06-export/viewer-caption-injection.spec.ts @@ -69,7 +69,7 @@ test.describe('Export — a caption cannot brick the keepsake viewer', () => { ) .toBe('done'); - const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, { + const ticketRes = await fetch(`${BASE}/api/v1/export/ticket?kind=html`, { method: 'POST', headers: bearer, }); diff --git a/e2e/specs/06-export/viewer-no-broken-tiles.spec.ts b/e2e/specs/06-export/viewer-no-broken-tiles.spec.ts index 72da213..30a4b2a 100644 --- a/e2e/specs/06-export/viewer-no-broken-tiles.spec.ts +++ b/e2e/specs/06-export/viewer-no-broken-tiles.spec.ts @@ -61,7 +61,7 @@ test.describe('Export — the keepsake has no broken tiles', () => { ) .toBe('done'); - const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, { + const ticketRes = await fetch(`${BASE}/api/v1/export/ticket?kind=html`, { method: 'POST', headers: bearer, }); diff --git a/e2e/specs/07-adversarial/auth-tampering.spec.ts b/e2e/specs/07-adversarial/auth-tampering.spec.ts index a076434..ee2ea9a 100644 --- a/e2e/specs/07-adversarial/auth-tampering.spec.ts +++ b/e2e/specs/07-adversarial/auth-tampering.spec.ts @@ -133,10 +133,14 @@ test.describe('Adversarial — PIN brute-force', () => { }); statuses.push(r.status); } - expect(statuses.filter((s) => s === 200), 'a wrong PIN must never authenticate').toHaveLength( - 0 - ); - expect(statuses.some((s) => s === 429), 'the attacker must be throttled').toBe(true); + expect( + statuses.filter((s) => s === 200), + 'a wrong PIN must never authenticate' + ).toHaveLength(0); + expect( + statuses.some((s) => s === 429), + 'the attacker must be throttled' + ).toBe(true); expect( await db.isPinLocked(g.userId), @@ -255,29 +259,46 @@ test.describe('Adversarial — admin password brute-force', () => { expect(statuses.some((s) => s === 200)).toBe(false); }); - test('once throttled, even the CORRECT admin password is refused (it is an IP limit, not a password check)', async ({ - api, - adminToken, - }) => { - // This is the assertion that makes the test non-vacuous: it isolates the RATE LIMIT from the - // password logic. If the throttle were removed, the correct password would return 200 here. + test('the FAILURE bucket never refuses a correct admin password', async ({ api, adminToken }) => { + // This asserted the OPPOSITE — that a throttled IP is refused even with the right password — + // and that contract was deliberately removed, because at a real event it is a denial of + // service against the operator. Every guest at the venue shares one public IP behind NAT and + // `/admin/login` is a publicly linkable page, so a single tight IP bucket charged before the + // password check meant any phone in the room could keep it full and the host, on that same IP, + // could never spend a slot. The escape hatch was circular: `admin_login_rate_enabled` is only + // reachable through `PATCH /admin/config`, which needs the session being blocked. + // + // So the tight bucket is now charged ONLY on a wrong password. Brute force stays bounded (every + // guess costs a slot, per IP) while a valid credential is always honoured. A separate, generous + // per-IP ceiling bounds bcrypt CPU regardless of correctness — see ADMIN_LOGIN_CPU_CEILING. await api.patchConfig(adminToken, { rate_limits_enabled: 'true', admin_login_rate_enabled: 'true', }); - // Exhaust the window with wrong passwords until throttled. + // Exhaust the failure window with wrong passwords until throttled. let throttled = false; for (let i = 0; i < 10 && !throttled; i++) { throttled = (await tryLogin('wrong-' + i)).status === 429; } - expect(throttled, 'the IP should be throttled after a burst').toBe(true); + expect(throttled, 'a burst of WRONG passwords from one IP must be rate-limited').toBe(true); - // The right password, while throttled, must STILL be refused — the limiter is checked before - // the bcrypt verify, so a valid credential does not buy a way around a brute-force lockout. + // The limiter is real (above) and yet the operator gets in. That combination is the whole + // property: THIS bucket keys on failure, not on the IP alone. + // + // Deliberately not claimed here: "guests cannot lock the host out". They still can — the + // separate CPU ceiling below refuses any password, correct included. This test stays under + // that ceiling on purpose so the two are not conflated. expect( (await tryLogin(ADMIN_PASSWORD)).status, - 'a throttled IP is refused even with the correct password' + 'a burst of wrong guesses must not cost the operator their own admin panel' + ).toBe(200); + + // And guessing is still throttled AFTER a successful login — a correct password must not + // refill or bypass the attacker's bucket. + expect( + (await tryLogin('wrong-again')).status, + 'a successful login must not clear the failure bucket for wrong guesses' ).toBe(429); }); diff --git a/e2e/specs/07-adversarial/ddos.spec.ts b/e2e/specs/07-adversarial/ddos.spec.ts index c27b18c..730fa0d 100644 --- a/e2e/specs/07-adversarial/ddos.spec.ts +++ b/e2e/specs/07-adversarial/ddos.spec.ts @@ -101,10 +101,26 @@ test.describe('Adversarial — small-scale abuse', () => { }) ); const responses = await Promise.all(requests); - // All accepted (or some rate-limited — both fine). - for (const r of responses) { - expect([200, 429]).toContain(r.status); + // One session may hold only MAX_TICKETS_PER_SESSION (4) live tickets — enough for a guest with + // a couple of tabs open, deliberately not enough for a reconnect loop to accumulate. Minting a + // 5th evicts the oldest, so most of these ten tickets are already dead when their stream opens + // and the server answers 401. That IS the cap working; this used to allow only [200, 429] and + // so failed on the very defence it was written to exercise. + // + // What must hold is that the server sheds the flood deliberately rather than falling over: no + // 5xx, and the surviving tickets still get their stream. + const statuses = responses.map((r) => r.status); + for (const s of statuses) { + expect([200, 401, 429], `unexpected status from the stream flood: ${statuses}`).toContain(s); } + // EXACTLY four, not merely "at least one". The cap is a known constant, so asserting a + // bound this loose would still pass if it were tightened to 1 (a guest with two tabs loses + // their live feed) or if eviction kept the OLDEST ticket instead of the newest (every + // reconnect throws away the ticket it just minted — a permanently dead feed for that guest). + expect( + statuses.filter((s) => s === 200).length, + `exactly MAX_TICKETS_PER_SESSION streams should survive, got: ${statuses}` + ).toBe(4); // Tear them all down so the next test doesn't see leaked connections. controllers.forEach((c) => c.abort()); diff --git a/e2e/specs/09-mobile/sheet-escape.spec.ts b/e2e/specs/09-mobile/sheet-escape.spec.ts index efe02c8..6e2868a 100644 --- a/e2e/specs/09-mobile/sheet-escape.spec.ts +++ b/e2e/specs/09-mobile/sheet-escape.spec.ts @@ -44,7 +44,7 @@ test.describe('Mobile a11y — sheets dismiss on Escape', () => { // click reaches a live handler. See the test above. await expect(page.getByText(g.displayName)).toBeVisible(); - await page.getByRole('button', { name: /Event verlassen/i }).click(); + await page.getByTestId('account-logout').click(); const sheet = page.getByTestId('confirm-sheet'); await expect(sheet).toBeVisible(); 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 db9793e..7dc5d2c 100644 --- a/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts +++ b/e2e/specs/10-flow-review/export-reopen-rerelease.spec.ts @@ -80,8 +80,16 @@ async function exportStatus(jwt: string): Promise { return res.json(); } -async function mintTicket(jwt: string): Promise { - const res = await post('/api/v1/export/ticket', jwt); +/** The raw mint response. `/export/ticket` pre-validates that the archive is servable, so an + * unavailable keepsake is refused HERE — before one of the guest's three daily downloads is + * charged for an archive that cannot be served. */ +function mintTicketResponse(jwt: string, kind: 'zip' | 'html' = 'zip') { + return post(`/api/v1/export/ticket?kind=${kind}`, jwt); +} + +async function mintTicket(jwt: string, kind: 'zip' | 'html' = 'zip'): Promise { + // `kind` is REQUIRED: the ticket is bound to one archive (see TicketKind::Download). + const res = await post(`/api/v1/export/ticket?kind=${kind}`, jwt); return (await res.json()).ticket; } @@ -294,9 +302,11 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k ); expect(ev.released_at, 'reopen clears the release timestamp').toBeNull(); - const ticket = await mintTicket(host.jwt); - const dl = await fetch(BASE + '/api/v1/export/zip?ticket=' + encodeURIComponent(ticket)); - expect(dl.status, 'a reopened event serves no keepsake').toBe(404); + // Refused at the MINT, not one step later at the download: the ticket is bound to an archive, + // so the pre-check always knows which one to resolve and answers honestly up front. + expect((await mintTicketResponse(host.jwt)).status, 'a reopened event serves no keepsake').toBe( + 404 + ); }); test('open ‖ release churn always converges to a consistent, downloadable keepsake', async ({ @@ -387,11 +397,12 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k expect( res.status, - 'an upload whose body completed after the release MUST be rejected (403 uploads_locked). ' + - 'A 201 here means the server accepted a photo it will never put in the keepsake — silent, ' + - 'permanent data loss.' + 'an upload whose body completed after the release MUST be rejected (403). A 201 here means ' + + 'the server accepted a photo it will never put in the keepsake — silent, permanent data loss.' ).toBe(403); - expect((await res.json()).error).toBe('uploads_locked'); + // `gallery_released`, matching the pre-flight check: the release is what rejected this, and + // that code is the one that PARKS the blob instead of re-pushing it on the retry ladder. + expect((await res.json()).error).toBe('gallery_released'); // And the keepsake holds exactly the one upload that was genuinely committed before the release. await waitExportDone(host.jwt); @@ -679,10 +690,7 @@ test.describe('Flow re-review — reopen→re-release export integrity + stale-k expect(failed.zip.status).toBe('failed'); // Stuck: the keepsake is not downloadable and no amount of re-releasing helps. - const ticket = await mintTicket(host.jwt); - expect( - (await fetch(BASE + '/api/v1/export/zip?ticket=' + encodeURIComponent(ticket))).status - ).toBe(404); + expect((await mintTicketResponse(host.jwt)).status).toBe(404); // ("Galerie wurde bereits freigegeben." — this is the dead end the rebuild endpoint exists for.) expect((await post('/api/v1/host/gallery/release', host.jwt)).status).toBe(400); diff --git a/e2e/specs/10-flow-review/upload-lock-code.spec.ts b/e2e/specs/10-flow-review/upload-lock-code.spec.ts index a32fcdf..9536bc5 100644 --- a/e2e/specs/10-flow-review/upload-lock-code.spec.ts +++ b/e2e/specs/10-flow-review/upload-lock-code.spec.ts @@ -36,7 +36,15 @@ test.describe('Upload — locked event uses a distinct, reversible 403 (audit fi expect(ok.status, 'upload succeeds once the host reopens').toBeLessThan(300); }); - test('released gallery → 403 uploads_locked (also reversible via reopen)', async ({ host }) => { + test('released gallery → 403 gallery_released, the PARKING code (not uploads_locked)', async ({ + host, + }) => { + // `release ⇒ lock`, so a released gallery satisfies both conditions and the handler's check + // ORDER decides which code the guest gets. It must be the release one, and the difference is + // not cosmetic: `uploads_locked` charges a retry attempt and re-pushes the whole photo on the + // backoff ladder, against an answer that cannot change until a host acts. `gallery_released` + // parks it — blob kept, nothing re-sent, and the guest is told to ask the hosts to reopen. + // This asserted `uploads_locked` while the release branch was unreachable dead code. const rel = await fetch(`${BASE}/api/v1/host/gallery/release`, { method: 'POST', headers: { Authorization: `Bearer ${host.jwt}` }, @@ -49,6 +57,6 @@ test.describe('Upload — locked event uses a distinct, reversible 403 (audit fi }); expect(rejected.status).toBe(403); const body = await rejected.json(); - expect(body.error).toBe('uploads_locked'); + expect(body.error).toBe('gallery_released'); }); });