From c2ca73abbcedacb0b7b0b2919bf68115b6f2495d Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 14:09:02 +0000 Subject: [PATCH 1/2] test(e2e): cover auth_flow TTL regression boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a second @otp-expiry scenario that backdates the auth_flow row via the existing /_internal/test/expire-auth-flow hook, then submits a still-valid OTP. After PR #154's reactive abort gate the OTP form pings /auth/ping before submitting; with the auth_flow row dead the ping reports `flow_expired`, the gate navigates to /auth/abort, and cleanExit serves its Tier-2 styled "Sign-in session expired" fallback page (the OAuth client redirect path needs the dead row's clientId, which is exactly what's missing here). The scenario asserts both signals — the ping reason (proving auth_flow specifically tripped, not PAR) and the abort fallback page — so a regression that, say, swaps which timer the gate honours would still be caught. Without this guardrail nothing in CI would notice if AUTH_FLOW_TTL_MS were quietly shortened back to the OTP TTL; the existing scenario only proves the 10-min-and-resend path works, not that the 60-min boundary is enforced. Co-Authored-By: Claude Opus 4.7 (1M context) --- e2e/step-definitions/auth.steps.ts | 154 ++++++++++++++++++- e2e/support/world.ts | 7 + features/passwordless-authentication.feature | 29 ++++ 3 files changed, 189 insertions(+), 1 deletion(-) diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index bca6225f..89343dba 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -1,5 +1,5 @@ import { Given, Then, When } from '@cucumber/cucumber' -import { expect } from '@playwright/test' +import { expect, type Route } from '@playwright/test' import { testEnv } from '../support/env.js' import type { EpdsWorld } from '../support/world.js' import { @@ -455,6 +455,158 @@ When( }, ) +When( + 'more than 60 minutes pass before the user submits the OTP', + async function (this: EpdsWorld) { + if (!testEnv.mailpitPass) return 'pending' + if (!testEnv.internalSecret) return 'pending' + if (!this.testEmail) { + throw new Error( + 'No test email set — the email-submit step must run first', + ) + } + + // Backdate the auth_flow row so getAuthFlow() filters it out as + // expired. The epds_auth_flow cookie is left alone: keeping it lets + // the abort gate's /auth/ping call differentiate "auth_flow expired" + // (cookie present but row gone -> reason: flow_expired) from + // "no_cookie", which is what discriminates this scenario from + // unrelated dead-session paths. + const flowResult = await callExpiryHook( + '/_internal/test/expire-auth-flow', + this.testEmail, + {}, + ) + if (flowResult.updated < 1) { + throw new Error( + `expire-auth-flow hook reported no rows updated — was the OAuth login flow started first?`, + ) + } + + // Arm a /auth/ping interceptor BEFORE the Verify click submits the + // OTP. The OTP form's reactive abort gate pings /auth/ping + // synchronously and bails to /auth/abort if the response carries a + // non-transient failure. Use page.route + route.fetch so we read the + // body off the wire ourselves, then forward it to the page — + // Playwright drops subresource bodies once the page navigates, and + // the navigation here happens immediately after this very ping, so + // listening on `page.on('response')` and calling resp.json() races + // the navigation and loses. + const page = getPage(this) + let resolvePing: (body: { ok: boolean; reason?: string }) => void = () => {} + let rejectPing: (err: unknown) => void = () => {} + const pingPromise = new Promise<{ ok: boolean; reason?: string }>( + (resolve, reject) => { + resolvePing = resolve + rejectPing = reject + }, + ) + + // Race the captured ping against an explicit timeout so the matching + // Then doesn't await forever if /auth/ping never fires (page JS broken, + // request blocked, etc.). 25s comfortably exceeds the form-load → Verify + // click → ping path; anything beyond that is a test failure, not slowness. + const PING_TIMEOUT_MS = 25_000 + const timeoutHandle: NodeJS.Timeout = setTimeout(() => { + rejectPing( + new Error( + `Timed out after ${PING_TIMEOUT_MS}ms waiting for /auth/ping response`, + ), + ) + }, PING_TIMEOUT_MS) + this.pendingPingBody = pingPromise.finally(() => { + clearTimeout(timeoutHandle) + }) + + const pingPattern = '**/auth/ping**' + let captured = false + const handler = async (route: Route) => { + let routeHandled = false + try { + const resp = await route.fetch() + const text = await resp.text() + if (!captured) { + captured = true + try { + resolvePing(JSON.parse(text) as { ok: boolean; reason?: string }) + } catch (parseErr) { + rejectPing( + new Error( + `/auth/ping body was not JSON: ${text.slice(0, 200)} (${String(parseErr)})`, + ), + ) + } + } + // Forward the response unmodified so the form's gate sees the + // exact body our test will assert on. + await route.fulfill({ response: resp, body: text }) + routeHandled = true + } catch (err) { + if (!captured) { + captured = true + rejectPing(err) + } + if (!routeHandled) { + await route.abort().catch(() => { + /* already handled — ignore */ + }) + } + } finally { + // First ping captured: drop the route so later steps (or + // long-running heartbeats from the abort fallback page) don't + // keep stacking through this interceptor. Doing this AFTER + // fulfill/abort avoids racing the in-flight handler. + if (captured) { + await page.unroute(pingPattern, handler).catch(() => { + /* already unrouted — ignore */ + }) + } + } + } + await page.route(pingPattern, handler) + }, +) + +Then( + 'the auth-complete page shows an {string} error', + async function (this: EpdsWorld, expected: string) { + const page = getPage(this) + await page.waitForURL('**/auth/complete', { timeout: 30_000 }) + await expect(page.locator('p.error')).toContainText(expected, { + timeout: 10_000, + }) + }, +) + +Then( + 'the OAuth flow aborts because auth_flow expired', + async function (this: EpdsWorld) { + if (!this.pendingPingBody) { + throw new Error( + 'No /auth/ping response was armed — an expiry step must run first', + ) + } + const pingBody = await this.pendingPingBody + if (pingBody.ok || pingBody.reason !== 'flow_expired') { + throw new Error( + `Expected /auth/ping to report flow_expired but got: ${JSON.stringify(pingBody)}`, + ) + } + + // /auth/abort reads the AUTH_FLOW_COOKIE to recover the OAuth + // client_id for a redirect back to the demo client. With the + // auth_flow row backdated, getAuthFlow() returns undefined, so + // cleanExit falls back to its Tier-2 styled page on the auth-service + // host rather than redirecting onward. That fallback IS the visible + // contract for this failure mode; assert it explicitly. + const page = getPage(this) + await page.waitForURL('**/auth/abort', { timeout: 30_000 }) + await expect(page.locator('h1')).toContainText('Sign-in session expired', { + timeout: 10_000, + }) + }, +) + Then( 'the verification form shows an {string} error', async function (this: EpdsWorld, expected: string) { diff --git a/e2e/support/world.ts b/e2e/support/world.ts index a7efe441..8350ce1e 100644 --- a/e2e/support/world.ts +++ b/e2e/support/world.ts @@ -80,6 +80,13 @@ export class EpdsWorld extends World { * scenario start, re-attached after resetBrowserContext. */ consoleCapture?: NodeJS.WritableStream + /** Body of the next /auth/ping response, captured by an expiry step + * before the abort gate fires. The matching assertion step awaits + * this to confirm which timer (auth_flow / PAR) tripped the abort. + * Captured eagerly because Playwright loses subresource bodies once + * the page navigates (which happens immediately after this ping). */ + pendingPingBody?: Promise<{ ok: boolean; reason?: string }> + get env() { return testEnv } diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index e13b9f5c..b0692184 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -489,6 +489,35 @@ Feature: Passwordless authentication via email OTP And the user enters the OTP code Then the browser lands back at the demo client with an auth error + # Regression boundary for the auth_flow lifetime. The auth_flow row + + # epds_auth_flow cookie are explicitly NOT tied to the OTP TTL — they + # live for 60 minutes (lib/auth-flow.ts) so a slow user who hits OTP + # expiry and clicks Resend can still complete on the original ticket. + # If someone shortens AUTH_FLOW_TTL_MS back to the OTP TTL we want to + # catch it here: past the 60-minute mark, even a freshly verified OTP + # must NOT recover the flow. + # + # Post-PR-154 the OTP form's reactive abort gate pings /auth/ping + # before submitting; with the auth_flow row backdated /auth/ping + # answers `flow_expired` and the gate navigates to /auth/abort. + # /auth/abort then reads AUTH_FLOW_COOKIE to recover the OAuth + # client_id for a redirect back — and because the same row is dead + # that lookup also fails, so cleanExit serves its Tier-2 styled + # "Sign-in session expired" fallback page on the auth-service host. + # We assert both signals: the ping reason (proving auth_flow + # specifically tripped, not PAR) and the abort fallback page. + @email @otp-expiry + Scenario: OAuth flow expires after the auth_flow TTL elapses + When the demo client initiates an OAuth login + Then the browser is redirected to the auth service login page + And the login page displays an email input form + When the user enters a unique test email and submits + Then an OTP email arrives in the mail trap for the test email + And the login page shows an OTP verification form + When more than 60 minutes pass before the user submits the OTP + And the user enters the OTP code + Then the OAuth flow aborts because auth_flow expired + # --- Brute force protection --- Scenario: OTP verification rejects wrong code From e873e7a3c868d69a17c14daa7d90f1350664e335 Mon Sep 17 00:00:00 2001 From: kzoeps Date: Thu, 7 May 2026 20:22:39 +0600 Subject: [PATCH 2/2] fix(pds-core): add email label spacing --- .beads/issues.jsonl | 1 + packages/pds-core/src/chooser-enrichment.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index 552a3d89..b7c1c108 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -261,6 +261,7 @@ {"id":"ePDS-ipm.7","title":"Update recovery.test.ts to use dynamic OTP length instead of hardcoded 8","description":"## Files\n- packages/auth-service/src/__tests__/recovery.test.ts (modify)\n\n## What to do\n\nThere are two test blocks that hardcode OTP length to 8:\n\n### Block 1 (lines 122-128)\n```ts\ndescribe('Recovery flow: OTP pattern (8 digits)', () =\u003e {\n it('OTP sent by better-auth is 8 digits (configured in emailOTP plugin)', () =\u003e {\n const OTP_LENGTH = 8\n const pattern = new RegExp(`^[0-9]{${OTP_LENGTH}}$`)\n // ...\n })\n})\n```\n\n### Block 2 (lines 136-142)\n```ts\nit('OTP entry form uses maxlength=8 and pattern [0-9]{8}', () =\u003e {\n const maxlength = 8\n const pattern = '[0-9]{8}'\n expect(maxlength).toBe(8)\n expect(pattern).toContain('8')\n})\n```\n\n### Changes needed\n\nFor both blocks, read the OTP length from `process.env.OTP_LENGTH` with default 8:\n```ts\nconst OTP_LENGTH = parseInt(process.env.OTP_LENGTH ?? '8', 10)\n```\n\nUpdate the describe/it descriptions to not hardcode '8':\n- `'Recovery flow: OTP pattern'` (drop '8 digits')\n- `'OTP sent by better-auth matches configured length'`\n- `'OTP entry form uses configured maxlength and pattern'`\n\nUpdate Block 2 assertions to use the dynamic value:\n```ts\nconst maxlength = OTP_LENGTH\nconst pattern = `[0-9]{${OTP_LENGTH}}`\nexpect(maxlength).toBe(OTP_LENGTH)\nexpect(pattern).toContain(String(OTP_LENGTH))\n```\n\n## Don't\n- Don't change any other test files\n- Don't import from better-auth internals — just read process.env","acceptance_criteria":"1. No hardcoded 8 remains in OTP-related test assertions or descriptions\n2. Both test blocks read OTP_LENGTH from process.env with default 8\n3. Test descriptions are updated to not mention '8 digits'\n4. pnpm test passes with default OTP_LENGTH (unset = 8)","status":"closed","priority":2,"issue_type":"task","assignee":"karma.gainforest.id","owner":"karma.gainforest.id","estimated_minutes":20,"created_at":"2026-03-11T22:24:40.027924118+06:00","created_by":"karma.gainforest.id","updated_at":"2026-03-11T22:26:23.690810039+06:00","closed_at":"2026-03-11T22:26:23.690810039+06:00","close_reason":"627028c Use dynamic OTP_LENGTH in recovery tests instead of hardcoded 8","labels":["scope:trivial"],"dependencies":[{"issue_id":"ePDS-ipm.7","depends_on_id":"ePDS-ipm","type":"parent-child","created_at":"2026-03-11T22:24:40.029555266+06:00","created_by":"karma.gainforest.id"}]} {"id":"ePDS-ipm.8","title":"Document OTP_LENGTH env var in .env.example","description":"## Files\n- packages/auth-service/.env.example (modify)\n\n## What to do\n\nAdd the OTP_LENGTH env var documentation. Place it in the '-- Auth-service only --' section, after the session settings block (after line 61, before the social login section):\n\n```\n# OTP code length — number of digits in the email verification code (default: 8)\n# Must be between 4 and 12. Applies to login, recovery, and account settings OTP flows.\n# OTP_LENGTH=8\n```\n\nThe variable is commented out (like other optional vars in this file) since the default of 8 is sensible.\n\n## Don't\n- Don't modify any other .env.example files (the root one, pds-core, or demo)\n- Don't change any existing lines","acceptance_criteria":"1. OTP_LENGTH is documented in packages/auth-service/.env.example\n2. It's in the auth-service-only section, near the session settings\n3. Comment explains the valid range (4-12) and default (8)\n4. The variable line is commented out (# OTP_LENGTH=8)\n5. No existing lines are modified","status":"closed","priority":3,"issue_type":"task","assignee":"karma.gainforest.id","owner":"karma.gainforest.id","estimated_minutes":10,"created_at":"2026-03-11T22:24:48.428248126+06:00","created_by":"karma.gainforest.id","updated_at":"2026-03-11T22:26:17.022688894+06:00","closed_at":"2026-03-11T22:26:17.022688894+06:00","close_reason":"b8d6f58 docs: document OTP_LENGTH env var in auth-service .env.example","labels":["scope:trivial"],"dependencies":[{"issue_id":"ePDS-ipm.8","depends_on_id":"ePDS-ipm","type":"parent-child","created_at":"2026-03-11T22:24:48.42974985+06:00","created_by":"karma.gainforest.id"}]} {"id":"ePDS-ix2","title":"Fix: recovery .link-btn has padding:0 — touch target too small on mobile (from ePDS-aq2.5)","description":"Review of ePDS-aq2.5 found: The .link-btn CSS rule in recovery.ts (line 636) sets padding: 0, which means the touch target for 'Back to sign in' and 'Resend Code' links is only the text height (~20px). WCAG 2.5.5 recommends a minimum 44×44px touch target.\n\nEvidence:\n- recovery.ts line 636: padding: 0; (inside .link-btn rule)\n- The same issue exists in login-page.ts (line 565: padding: 0)\n\nThe .link-btn elements appear in:\n1. .form-actions row: 'Back to sign in' link (line 246)\n2. .otp-links row: 'Resend Code' button and 'Back to sign in' link (lines 319, 321)\n\nThese are the only interactive elements in those rows, so small touch targets are a real usability issue on mobile.\n\nFix: Add padding: 8px 0 (or similar) to .link-btn to increase the touch target height to at least 36px, while keeping the visual appearance the same. This fix should be applied to both recovery.ts and login-page.ts for consistency. Note: this is a pre-existing issue in login-page.ts that was copied into recovery.ts.","status":"open","priority":3,"issue_type":"bug","owner":"karma.gainforest.id","created_at":"2026-03-04T15:59:41.944209123+06:00","created_by":"karma.gainforest.id","updated_at":"2026-03-04T15:59:41.944209123+06:00","dependencies":[{"issue_id":"ePDS-ix2","depends_on_id":"ePDS-aq2.5","type":"discovered-from","created_at":"2026-03-04T15:59:42.002820695+06:00","created_by":"karma.gainforest.id"}]} +{"id":"ePDS-j448","title":"Epic: E2E coverage for consent and chooser enrichment","description":"Add end-to-end coverage for the user-facing enrichment introduced on this branch. Success means real OAuth consent screens assert identity enrichment in random and picker modes, and the session-reuse chooser profile asserts non-random chooser rows keep handle and email visible. Constraints: do not add preview-route e2e coverage; do not add multi-account/exact-matching e2e coverage in this scope; avoid coupling random consent coverage to random account provisioning risk. Known risks: consent DOM is upstream-owned and can be fragile; keep assertions anchored on visible role/text and ePDS stable classes.","status":"open","priority":2,"issue_type":"epic","owner":"kzoepa@gmail.com","created_at":"2026-05-04T17:17:28.800992481+06:00","created_by":"kzoeps","updated_at":"2026-05-04T17:17:28.800992481+06:00","labels":["scope:medium"]} {"id":"ePDS-j825","title":"Fix: renderNoAccountPage() response missing HTTP status code (from ePDS-01p.3)","description":"Review of ePDS-01p.3 found: In account-settings.ts line 69, when no PDS account is found for the session email, the response is sent as 'res.type(\"html\").send(renderNoAccountPage())' without setting an HTTP status code. This defaults to 200 OK, which is semantically incorrect for an error page and may confuse clients, monitoring tools, or browser caching. The 503 branch (PDS unavailable) correctly uses res.status(503), but the no-account branch silently returns 200. Fix: add an appropriate status code — either 403 Forbidden (the session is valid but the user has no account) or 404 Not Found. Suggested: res.status(403).type(\"html\").send(renderNoAccountPage()). Evidence: account-settings.ts line 69 — no .status() call before .type('html').send(...).","status":"open","priority":2,"issue_type":"bug","owner":"kzoepa@gmail.com","created_at":"2026-03-19T16:12:47.885869077+06:00","created_by":"kzoeps","updated_at":"2026-03-19T16:12:47.885869077+06:00","dependencies":[{"issue_id":"ePDS-j825","depends_on_id":"ePDS-01p.3","type":"discovered-from","created_at":"2026-03-19T16:13:23.934385443+06:00","created_by":"kzoeps"}]} {"id":"ePDS-jbs","title":"Fix: dark mode body background overridden by light-mode rule ordering (from ePDS-wae.2)","description":"The CSS has two body background rules: (1) body { background: var(--color-contrast-0); } at line ~364 and (2) @media (prefers-color-scheme: light) { body { background: white; } } at line ~371. In dark mode, rule (1) correctly uses --color-contrast-0 which resolves to hsl(265 20% 6%) — a near-black. However, when AUTH_BACKGROUND_COLOR is set, the bgColorStyle injects a \u003cstyle\u003ebody { background: ... !important; }\u003c/style\u003e block. The !important correctly overrides both rules. But if a client's background_color is set via OAuth metadata (b.background_color), it is NOT injected into the bgColorStyle block — it is only set via --color-panel (which only affects the panel, not body). The body background_color from client OAuth metadata is silently ignored. Document this limitation or implement it.","status":"open","priority":2,"issue_type":"bug","owner":"karma.gainforest.id","created_at":"2026-03-03T20:16:17.449655765+06:00","created_by":"karma.gainforest.id","updated_at":"2026-03-03T20:16:17.449655765+06:00","dependencies":[{"issue_id":"ePDS-jbs","depends_on_id":"ePDS-wae.2","type":"discovered-from","created_at":"2026-03-03T20:17:56.359797404+06:00","created_by":"karma.gainforest.id"}]} {"id":"ePDS-jk0","title":"Bug: recovery GET requires request_uri query param but plan says cookie should be sufficient","description":"## Finding\n\n`packages/auth-service/src/routes/recovery.ts` lines 42-44 return HTTP 400 if `request_uri` is absent from the query string:\n\n```ts\nif (!requestUri) {\n res.status(400).send(renderError('Missing request_uri parameter'))\n return\n}\n```\n\nThe login page recovery link at `login-page.ts:368` currently passes `request_uri` and `client_id` as query params to work around this:\n\n```ts\n\u003ca href=\"/auth/recover?request_uri=${...}\\\u0026client_id=${...}\" ...\u003e\n```\n\nThis is the current working state, but it means:\n\n1. The recovery page is reachable only via the login page link (which includes the params). Direct navigation or a stale link without params returns 400.\n2. The plan to simplify the recovery link to just `/auth/recover` (change #5) would break the GET handler unless change #4 (read from cookie instead) is implemented first. These two changes are tightly coupled and must be implemented atomically.\n3. The fallback at lines 47-51 (`getAuthFlowByRequestUri`) only runs when `clientId` is missing but `requestUri` is present — it never fires when `requestUri` itself is absent.\n\n## Not a regression — this is the current design\n\nThis is not a regression from any recent change. It is a pre-existing coupling that the refactoring plan must address carefully. The plan correctly identifies this (change #4), but the implementation must ensure the GET handler is updated before or simultaneously with simplifying the login page link (change #5). If done in two separate commits, the intermediate state would break recovery.","status":"open","priority":2,"issue_type":"bug","owner":"karma.gainforest.id","created_at":"2026-03-18T17:11:00.440621594+06:00","created_by":"karma.gainforest.id","updated_at":"2026-03-18T17:11:00.440621594+06:00"} diff --git a/packages/pds-core/src/chooser-enrichment.ts b/packages/pds-core/src/chooser-enrichment.ts index 6a38778f..871a72f6 100644 --- a/packages/pds-core/src/chooser-enrichment.ts +++ b/packages/pds-core/src/chooser-enrichment.ts @@ -200,7 +200,7 @@ export function buildChooserEnrichmentScript(): string { label.className = 'epds-email-label'; label.style.cssText = 'min-width:0;white-space:nowrap;overflow:hidden;text-overflow:ellipsis;' - label.textContent = m.email; + label.textContent = ' ' + m.email; if (m.el.dataset) m.el.dataset.epdsEnriched = '1'; m.el.classList.add('epds-handle-label'); var wrap = m.el.parentElement;