Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .beads/issues.jsonl
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand Down
154 changes: 153 additions & 1 deletion e2e/step-definitions/auth.steps.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 7 additions & 0 deletions e2e/support/world.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
29 changes: 29 additions & 0 deletions features/passwordless-authentication.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/pds-core/src/chooser-enrichment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading