From 8b396c806b749e8f8252514f8170eeefd920ea13 Mon Sep 17 00:00:00 2001 From: Mao Nakamoto <41178744+maonakamoto@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:04:42 +0200 Subject: [PATCH 1/3] test(e2e): wire three never-run specs into CI, and make the security one real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The e2e-local job runs 'tests/e2e/*journey.spec.ts'. Nine specs are not named *journey, so nothing ran them — anywhere, ever. I ran all of them against a locally seeded instance before wiring any, which is the only reason this PR is three specs and not nine. Wired (41 assertions that were running nowhere): security 4 tests — rewritten, see below notification-hrefs 21 assertions, green as-is user-admin-flows 16 assertions, green after one fix security.spec.ts was a STUB and wiring it as-written would have been worse than leaving it out: two of its four tests had comment-only bodies ('Would need to fill and submit form 6 times'), a third asserted only inside an isVisible() branch, and every one called test.skip() when redirected to login — which /it-hilfe/create always does signed out. In CI it would have reported a green 'security' check while exercising nothing. It now asserts the closed side of the authorization boundary: no admin page renders and no admin/money API answers 2xx to a signed-out request, and a refused response carries no user data. Proven by mutation — adding a public route to the protected list turns it red. user-admin-flows addressed routes as /de/x. The app 307-redirects those to /x, and when that races the client router Playwright aborts the navigation (net::ERR_ABORTED) — indistinguishable from a broken page. It cost three wrong hypotheses (concurrency, then state residue, then a fresh database) before I read the error. Now uses the canonical paths, which is where the redirect lands and what users actually see. NOT wired, with reasons rather than silence: marketplace, it-hilfe, appointments, payment-return and timecards fail against the current app — marketplace expects an h1 of 'Marketplace' and the text 'gebrauchte IT-Geräte', which is pre-rebrand Revamp-IT copy. dashboard-timecards skips its only test. Specs nothing runs rot; these did. Recorded in docs/AUDIT_BACKLOG_2026-08.md. Verified: 3 consecutive green runs of the bundle, typecheck, lint, docs gate. Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 10 ++ package.json | 1 + tests/e2e/security.spec.ts | 180 ++++++++++++++--------------- tests/e2e/user-admin-flows.spec.ts | 24 +++- 4 files changed, 116 insertions(+), 99 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 747b20103..942004f1b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -251,6 +251,16 @@ jobs: - name: Run Playwright E2E journeys run: npm run test:e2e:journeys -- --project=chromium --reporter=line + # Specs that are not named *journey and therefore fell outside the glob + # above — so nothing ran them, anywhere, ever. Run by name, not by + # pattern, so a file cannot silently drop out of coverage again. + # security 41 assertions: no admin page renders and no admin/ + # money API answers 2xx to a signed-out request + # notification-hrefs 21 assertions + # user-admin-flows 16 assertions + - name: Run Playwright E2E guards + run: npm run test:e2e:guards -- --project=chromium --reporter=line + - name: Upload E2E artifacts if: always() uses: actions/upload-artifact@v7 diff --git a/package.json b/package.json index 3363c58ed..5fb84a638 100644 --- a/package.json +++ b/package.json @@ -48,6 +48,7 @@ "test:e2e": "playwright test", "test:e2e:auth": "playwright test tests/e2e/auth-smoke.spec.ts -g 'credentials login'", "test:e2e:journeys": "playwright test tests/e2e/*journey.spec.ts", + "test:e2e:guards": "playwright test tests/e2e/security.spec.ts tests/e2e/notification-hrefs.spec.ts tests/e2e/user-admin-flows.spec.ts", "test:e2e:it-hilfe": "playwright test tests/e2e/it-hilfe.spec.ts", "test:e2e:it-hilfe:journey": "playwright test tests/e2e/it-hilfe-journey.spec.ts", "test:e2e:marketplace:journey": "playwright test tests/e2e/marketplace-checkout-journey.spec.ts", diff --git a/tests/e2e/security.spec.ts b/tests/e2e/security.spec.ts index fd852541b..9cf52b2f9 100644 --- a/tests/e2e/security.spec.ts +++ b/tests/e2e/security.spec.ts @@ -1,102 +1,96 @@ -import { test, expect } from '@playwright/test'; - - -test.describe('Security Features', () => { - test('XSS Prevention - IT-Hilfe title should sanitize script tags', async ({ page }) => { - // This test requires authentication - skip if not logged in - await page.goto(`/it-hilfe/create`); - - // Check if redirected to login - const currentUrl = page.url(); - if (currentUrl.includes('/auth/login') || currentUrl.includes('/api/auth')) { - test.skip(); - return; - } - - // Try to submit XSS payload - const titleInput = page.locator('input[name="title"], input[placeholder*="Titel"]'); - const descriptionInput = page.locator('textarea[name="description"], textarea[placeholder*="Beschreibung"]'); - - if (await titleInput.isVisible()) { - await titleInput.fill('Laptop Reparatur'); - await descriptionInput.fill('Normal description'); - - // Fill other required fields - // ... (would need to fill complete form) - - // After submission, verify the script tag is sanitized - // This would require actually submitting and checking the result - } - }); - - test('Input Validation - Invalid postal code should show error', async ({ page }) => { - await page.goto(`/it-hilfe/create`); - - const currentUrl = page.url(); - if (currentUrl.includes('/auth/login') || currentUrl.includes('/api/auth')) { - test.skip(); - return; +/** + * Authorization boundary — the closed side. + * + * This file replaces a stub. The previous version had four tests that between + * them asserted almost nothing: two had comment-only bodies ("Would need to + * fill and submit form 6 times"), a third only asserted inside an + * `if (await el.isVisible())` branch, and every one of them called + * `test.skip()` when redirected to login — which `/it-hilfe/create` always does + * when signed out. Wired into CI it would have reported a green "security" + * check while exercising nothing, which is worse than no check at all. + * + * What it asserts now is the property that actually matters and that a gate can + * genuinely fail on: **a signed-out visitor gets nothing.** Every admin page + * must redirect rather than render, and every admin/money API must answer 401 + * rather than 200. That is the closed side of the same boundary where a real + * privilege escalation was found in this codebase (money routes authorizing on + * a bare staff flag instead of the `finanzen` permission). + * + * Deliberately NOT authenticated: this runs with no session, so it needs no + * seeded users and cannot be skipped for want of credentials. Its whole job is + * to prove the door is shut. + */ + +import { test, expect } from '@playwright/test' +import { ADMIN_BLOCK_CHECK_ROUTES } from './helpers/inventory-routes' + +/** + * Admin/money APIs. A signed-out request must never receive a success body. + * 405 is acceptable for a POST-only route reached with GET — the request was + * refused before any handler logic ran. + */ +const PROTECTED_APIS = [ + '/api/invoices', + '/api/admin/users', + '/api/admin/refunds', + '/api/admin/permissions/requests', + '/api/payments/refund', +] + +test.describe('authorization boundary (signed out)', () => { + test('the route list is non-empty', () => { + // A sweep over zero routes passes trivially. Fail loudly instead. + expect(ADMIN_BLOCK_CHECK_ROUTES.length).toBeGreaterThan(5) + expect(PROTECTED_APIS.length).toBeGreaterThan(3) + }) + + test('no admin page renders to a signed-out visitor', async ({ page }) => { + // 37 routes in one test. Against a production build each is a fast redirect, + // but a dev server compiles every route on first visit, which blows through + // Playwright's 30s default. Budget for the slow case rather than sampling — + // a sweep that checks half the doors is not a sweep. + test.setTimeout(240_000) + + const leaked: string[] = [] + + for (const path of ADMIN_BLOCK_CHECK_ROUTES) { + const response = await page.goto(path, { waitUntil: 'domcontentloaded' }) + const status = response?.status() ?? 0 + const landedOn = new URL(page.url()).pathname + + // Acceptable: bounced to login/home, or refused outright. + const bounced = !landedOn.startsWith('/admin') + const refused = status === 401 || status === 403 || status === 404 + if (!bounced && !refused) leaked.push(`${path} → ${status} (stayed on ${landedOn})`) } - const postalCodeInput = page.locator('input[name="postalCode"], input[placeholder*="PLZ"]'); + expect(leaked).toEqual([]) + }) - if (await postalCodeInput.isVisible()) { - // Try 3-digit code (invalid) - await postalCodeInput.fill('123'); - await postalCodeInput.blur(); + test('no admin or money API answers 200 to a signed-out request', async ({ request }) => { + const leaked: string[] = [] - await page.waitForTimeout(500); - - // Try letters (invalid) - await postalCodeInput.fill('ABCD'); - await postalCodeInput.blur(); - - // Valid 4-digit code - await postalCodeInput.fill('8055'); - await postalCodeInput.blur(); - } - }); - - test('Rate Limiting - Should prevent rapid IT-Hilfe submissions', async ({ page, context }) => { - // This test would require: - // 1. Being logged in - // 2. Submitting multiple requests rapidly - // 3. Checking for rate limit error - - await page.goto(`/it-hilfe/create`); - - const currentUrl = page.url(); - if (currentUrl.includes('/auth/login') || currentUrl.includes('/api/auth')) { - test.skip(); - return; + for (const path of PROTECTED_APIS) { + const response = await request.get(path, { failOnStatusCode: false }) + const status = response.status() + // 401/403 = refused. 405 = wrong method on a POST-only route, also refused + // before any handler logic. Anything 2xx means the door was open. + if (status < 400) leaked.push(`${path} → ${status}`) } - // Would need to fill and submit form 6 times rapidly - // and check that 6th submission shows "Zu viele Anfragen" error - }); - - test('SSOT Validation - Canton dropdown should only show valid Swiss cantons', async ({ page }) => { - await page.goto(`/it-hilfe/create`); - - const currentUrl = page.url(); - if (currentUrl.includes('/auth/login') || currentUrl.includes('/api/auth')) { - test.skip(); - return; - } - - const cantonSelect = page.locator('select[name="canton"]'); - - if (await cantonSelect.isVisible()) { - // Get all options - const options = await cantonSelect.locator('option').allTextContents(); + expect(leaked).toEqual([]) + }) - // Verify Swiss cantons are present - expect(options).toContain('Zürich'); - expect(options).toContain('Bern'); - expect(options).toContain('Genf'); + test('a refused API response carries no user data', async ({ request }) => { + // A 401 that still serialises a row is the bug class that shipped a + // passwordHash inside a page response elsewhere in this fleet. Check the + // body, not just the status code. + const response = await request.get('/api/admin/users', { failOnStatusCode: false }) + const body = await response.text() - // Should have 26 cantons + 1 empty option - expect(options.length).toBeGreaterThanOrEqual(26); + expect(response.status()).toBeGreaterThanOrEqual(400) + for (const secret of ['password_hash', 'passwordHash', 'staff_permissions', '@']) { + expect(body).not.toContain(secret) } - }); -}); + }) +}) diff --git a/tests/e2e/user-admin-flows.spec.ts b/tests/e2e/user-admin-flows.spec.ts index 2556f07cf..dc1920aa3 100644 --- a/tests/e2e/user-admin-flows.spec.ts +++ b/tests/e2e/user-admin-flows.spec.ts @@ -1,3 +1,15 @@ +/** + * Routes are addressed WITHOUT the `/de` prefix on purpose. + * + * The app 307-redirects `/de/x` → `/x`, and when that server redirect races the + * client router Playwright aborts the navigation (`net::ERR_ABORTED`) — which + * looks exactly like a broken page but is a timing artefact of the redirect. + * It surfaced the moment this spec started running in CI: `/de/profil/techniker` + * failed while its identical sibling passed, on a freshly seeded database. + * The unprefixed path is where the redirect lands anyway, so this both removes + * the race and tests the URL users actually end up on. + */ + import { test, expect, type Page } from '@playwright/test' import { loginWithCredentials } from './helpers/auth' @@ -75,7 +87,7 @@ describeAuthenticatedFlows( test('technician profile editor loads', async () => { const page = getPage() - await page.goto('/de/profil/techniker') + await page.goto('/profil/techniker') await page.waitForLoadState('domcontentloaded') await expectNotLoginPage(page) expect(page.url()).toMatch(/techniker|profil/) @@ -83,18 +95,18 @@ describeAuthenticatedFlows( test('IT-Hilfe hub and browse load', async () => { const page = getPage() - await page.goto('/de/it-hilfe') + await page.goto('/it-hilfe') await page.waitForLoadState('domcontentloaded') await expect(page.locator('body')).toBeVisible() - await page.goto('/de/it-hilfe/anfragen') + await page.goto('/it-hilfe/anfragen') await page.waitForLoadState('domcontentloaded') await expectNotLoginPage(page) }) test('IT-Hilfe create form loads when authenticated', async () => { const page = getPage() - await page.goto('/de/it-hilfe/create') + await page.goto('/it-hilfe/create') await page.waitForLoadState('domcontentloaded') await expectNotLoginPage(page) expect(page.url()).toMatch(/create|anfragen|login/) @@ -102,7 +114,7 @@ describeAuthenticatedFlows( test('marketplace browse loads', async () => { const page = getPage() - await page.goto('/de/marketplace') + await page.goto('/marketplace') await page.waitForLoadState('domcontentloaded') await expect(page.locator('body')).toBeVisible() }) @@ -177,7 +189,7 @@ describeAuthenticatedFlows( test('technician profile editor loads', async () => { const page = getPage() - await page.goto('/de/profil/techniker') + await page.goto('/profil/techniker') await page.waitForLoadState('domcontentloaded') await expectNotLoginPage(page) }) From 0ed715f0763387ab331e934965e837cbad8c8b32 Mon Sep 17 00:00:00 2001 From: Mao Nakamoto <41178744+maonakamoto@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:05:43 +0200 Subject: [PATCH 2/3] docs: record which never-run E2E specs rotted, with the triage evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three are now wired; the other six are not, and the table says why rather than leaving a silent gap. marketplace expects pre-rebrand copy, and dashboard-timecards skips its only test. it-hilfe is the best next candidate — 10 of its 14 tests already pass. Co-Authored-By: Claude Fable 5 --- docs/AUDIT_BACKLOG_2026-08.md | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/docs/AUDIT_BACKLOG_2026-08.md b/docs/AUDIT_BACKLOG_2026-08.md index ad4193418..421452a45 100644 --- a/docs/AUDIT_BACKLOG_2026-08.md +++ b/docs/AUDIT_BACKLOG_2026-08.md @@ -61,9 +61,22 @@ issuing routes exist, redemption (`validateAndComputeDiscount`, - **Duplicate vocabularies**: two `SWISS_CANTONS` (names vs codes) both exported from `src/config`, two `DEVICE_CATEGORIES`, two `WORK_STATE_OPTIONS` with drifted labels. -- **E2E specs that no CI job runs**: `security.spec.ts`, - `user-admin-flows.spec.ts`, `payment-return.spec.ts`, and 6 more — only - `*journey.spec.ts` is wired. +- **E2E specs that no CI job runs** — RESOLVED IN PART. Three are now wired via + `npm run test:e2e:guards` (`security`, `notification-hrefs`, + `user-admin-flows` = 41 assertions). All nine were run against a locally + seeded instance first; the rest are **not** wired because they do not pass, + and the reason matters — specs nothing runs rot: + + | spec | result | why | + |---|---|---| + | `marketplace` | 9 failed / 2 passed | expects `h1` = "Marketplace" and "gebrauchte IT-Geräte" — pre-rebrand Revamp-IT copy | + | `it-hilfe` | 4 failed / 10 passed | mixed; worth salvaging, 10 tests already pass | + | `appointments` | 3 failed | session-email mismatch against seeded accounts | + | `payment-return` | 1 failed / 1 passed | — | + | `timecards` | 1 failed | — | + | `dashboard-timecards` | 1 skipped | skips its only test → inert if wired | + + `it-hilfe` is the best next candidate: two thirds of it already passes. - **`scripts/ship.sh`** duplicates `verify` with a different check list; delete it or make it call `verify`. - Duplicate scripts: `rollback.sh` vs `rollback-production.sh`, From 5223e5ce4ffb95da1bed47d24b69be61184f24b5 Mon Sep 17 00:00:00 2001 From: Mao Nakamoto <41178744+maonakamoto@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:21:41 +0200 Subject: [PATCH 3/3] fix(e2e): the signed-out security spec was running signed IN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI rejected the previous commit and it was right to. playwright.config.ts sets storageState globally, so every context — the page AND the request fixture — inherits a saved login unless a spec says otherwise. My 'authorization boundary (signed out)' tests therefore ran authenticated: /api/invoices, /api/admin/users and /api/admin/refunds all answered 200, and the spec reported the doors were open. They passed locally only because no saved-session file exists there. That is the worst kind of green: right answer, wrong reason, and dependent on which machine you run it on. Both specs now force an empty session explicitly. For security.spec.ts that line IS the test — a signed-out check carrying a session asserts nothing about the closed side of the boundary. Same root cause fixed notification-hrefs: it probes whether a ROUTE exists using a dummy UUID matching no record. Signed out, an auth redirect answers that without touching the database. Signed in, the admin page renders and correctly 404s on the missing record — so it reported 'notification deep link is broken' when nothing was broken. Co-Authored-By: Claude Fable 5 --- tests/e2e/notification-hrefs.spec.ts | 15 +++++++++++++++ tests/e2e/security.spec.ts | 16 ++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/tests/e2e/notification-hrefs.spec.ts b/tests/e2e/notification-hrefs.spec.ts index 83fb41a94..a4f3458ee 100644 --- a/tests/e2e/notification-hrefs.spec.ts +++ b/tests/e2e/notification-hrefs.spec.ts @@ -11,6 +11,21 @@ function buildNotificationHref(base: string): string { /** Routes that exist but require auth — redirect or 401 is OK; bare 404 is not. */ const ACCEPTABLE_STATUSES = new Set([200, 301, 302, 307, 308, 401, 403]) +/** + * Run signed out, explicitly. + * + * This spec asks "does the ROUTE exist", using a dummy UUID that matches no + * record. Signed out, an auth redirect or 401 answers that without ever + * touching the database. Signed IN, the admin page renders and then correctly + * 404s because the record is missing — so the test reports "deep link is + * broken" when nothing is broken. + * + * playwright.config.ts sets `storageState` globally, so a context inherits a + * saved login unless it says otherwise. That is exactly what happened in CI: + * job_application failed on a 404 while the route was perfectly fine. + */ +test.use({ storageState: { cookies: [], origins: [] } }) + test.describe('Notification bell deep links (RELATED_TYPE_HREFS)', () => { for (const [type, base] of Object.entries(RELATED_TYPE_HREFS)) { test(`${type} → ${base} resolves without HTTP 404`, async ({ request }) => { diff --git a/tests/e2e/security.spec.ts b/tests/e2e/security.spec.ts index 9cf52b2f9..ec57a2b86 100644 --- a/tests/e2e/security.spec.ts +++ b/tests/e2e/security.spec.ts @@ -24,6 +24,22 @@ import { test, expect } from '@playwright/test' import { ADMIN_BLOCK_CHECK_ROUTES } from './helpers/inventory-routes' +/** + * Force a genuinely empty session. THIS LINE IS THE TEST. + * + * playwright.config.ts sets `storageState` globally, so every context — the + * `page` AND the `request` fixture — silently inherits a saved login. Without + * this override these tests ran AUTHENTICATED while claiming to be signed out, + * and CI proved it: /api/invoices, /api/admin/users and /api/admin/refunds all + * answered 200. They passed locally only because no saved-session file existed + * there, which is the worst kind of green — right answer, wrong reason, + * environment-dependent. + * + * A signed-out test that quietly carries a session asserts nothing about the + * closed side of the boundary. Do not remove this. + */ +test.use({ storageState: { cookies: [], origins: [] } }) + /** * Admin/money APIs. A signed-out request must never receive a success body. * 405 is acceptable for a POST-only route reached with GET — the request was