From c86e23710295d701a33e948baa683709d4970ae6 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 17:33:39 +0000 Subject: [PATCH 01/11] test(e2e): reproduce sign-in-view leaks past welcome-page guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-route welcome-page guard intercepts the upstream stock welcome page when a request resolves to a device with zero bound accounts. But upstream renders a second authentication UI — its sign-in-view (handle + password form) — whenever every binding upstream considers has loginRequired: true. ePDS accounts are passwordless, so any path into that form is unusable; the guard must treat it identically to the welcome page. Three independent triggers force every binding loginRequired: Row 5 — stored PAR `parameters.prompt === 'login'` Row 6 — every binding's `account_device.updated_at` exceeds 7d Row 9 — `login_hint` resolves to a binding that's individually stale even though other bindings on the device are fresh Add scenarios for all three to features/session-reuse-bugs.feature under a new "Sign-in-view leaks" section, and update the feature's docstring to describe both authentication UIs the guard must suppress. The new "no upstream password field is rendered anywhere on the page" assertion pins the contract: a single `input[type="password"]` on the landing page is a leak. Row 5 reproduces against a fresh docker-compose stack today: post-OTP, the browser lands on pds-core's /oauth/authorize, the guard passes through, and upstream's sign-in-view renders. Rows 6 and 9 require a pds-core /_internal/test/expire-device-account hook (mirroring auth-service's expire-otp / expire-auth-flow hooks) that doesn't exist yet — those step bodies will fail at runtime until the hook lands. Drive-by: the existing returning-user step (and two of the new step bodies) called `page.fill('#code', otp)` against the auth-service login page's hidden #code input. Replace with the existing fillOtp helper, which fills the visible per-digit .otp-box inputs that feed the hidden field via JS (and auto-submits the verify form on the last digit). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../session-reuse-bugs.steps.ts | 182 +++++++++++++++++- features/session-reuse-bugs.feature | 89 +++++++-- 2 files changed, 254 insertions(+), 17 deletions(-) diff --git a/e2e/step-definitions/session-reuse-bugs.steps.ts b/e2e/step-definitions/session-reuse-bugs.steps.ts index 3fbc0672..e04c0bd9 100644 --- a/e2e/step-definitions/session-reuse-bugs.steps.ts +++ b/e2e/step-definitions/session-reuse-bugs.steps.ts @@ -1,13 +1,16 @@ /** - * Step definitions for features/session-reuse-bugs.feature — the Layer 2 + * Step definitions for features/session-reuse-bugs.feature — the * welcome-page guard scenarios. See docs/design/session-reuse-bugs.md for * the failure-mode taxonomy. * * These steps exercise the pre-route guard in pds-core that intercepts * /oauth/authorize and /account* before upstream's signin handler can - * render the stock welcome page. The guard bounces to auth-service when - * the dev-id/ses-id cookie pair is missing, malformed, or resolves to a - * device with zero bound accounts. + * render its authentication UIs (stock welcome page or sign-in-view). + * The guard bounces to auth-service when: + * - the dev-id/ses-id cookie pair is missing or malformed (rows 1–3) + * - the cookie pair resolves to a device with zero bound accounts (row 4) + * - the stored PAR carries `prompt=login` (row 5) + * - every relevant binding's auth age exceeds 7 days (rows 6 / 9) * * All scenarios require a docker-compose topology where auth-service is a * sibling subdomain of pds-core so device-session cookies are domain-scoped @@ -22,6 +25,7 @@ import { getPage } from '../support/utils.js' import { createAccountViaOAuth } from '../support/flows.js' import { sharedBrowser } from '../support/hooks.js' import { clearMailpit, extractOtp, waitForEmail } from '../support/mailpit.js' +import { fillOtp } from '../support/otp.js' // --------------------------------------------------------------------------- // Background: returning user completes an OAuth sign-in, leaving valid @@ -55,8 +59,7 @@ Given( }) const message = await waitForEmail(`to:${email}`) const otp = await extractOtp(message.ID) - await page.fill('#code', otp) - await page.click('#form-verify-otp .btn-primary') + await fillOtp(page, otp) await page.waitForURL('**/welcome', { timeout: 30_000 }) await clearMailpit(email) }, @@ -564,3 +567,170 @@ Given( ).toBe(pdsHost) }, ) + +// --------------------------------------------------------------------------- +// Sign-in-view leaks (rows 5/6/9). Distinct from the welcome-page leaks +// above: cookies + bindings are valid, but every binding upstream would +// consider has `loginRequired: true`, so upstream falls through to its +// sign-in-view (handle + password form). ePDS accounts are passwordless, so +// any path into that form is unusable. The guard must bounce these to +// auth-service for a fresh OTP round. +// --------------------------------------------------------------------------- + +When( + 'the demo client starts a new OAuth flow with prompt=login', + async function (this: EpdsWorld) { + // /api/oauth/login on the demo accepts ?prompt=login and propagates + // it to BOTH the PAR body AND the authorize-URL query string + // (packages/demo/src/app/api/oauth/login/route.ts:146,162). + // Cookies are preserved (the device must already be bound from the + // Background) so once the auth-service-side OTP completes and the + // pds-core epds-callback redirects back to /oauth/authorize, the + // stored PAR still carries `prompt=login` — the row-5 trigger. + const page = getPage(this) + const url = new URL('/api/oauth/login', testEnv.demoUrl) + url.searchParams.set('prompt', 'login') + await page.goto(url.toString()) + }, +) + +When('the user completes the OTP flow', async function (this: EpdsWorld) { + if (!testEnv.mailpitPass) return 'pending' + if (!this.testEmail) { + throw new Error( + 'world.testEmail missing — Background step must establish a returning user first', + ) + } + // Drive auth-service's email/OTP form to completion. Same shape as the + // Background's returning-user sign-in but invoked mid-scenario after + // the prompt=login redirect lands the user there. + const page = getPage(this) + await page.fill('#email', this.testEmail) + await page.click('button[type=submit]') + await expect(page.locator('#step-otp.active')).toBeVisible({ + timeout: 30_000, + }) + const message = await waitForEmail(`to:${this.testEmail}`) + const otp = await extractOtp(message.ID) + await fillOtp(page, otp) +}) + +Then( + 'no upstream password field is rendered anywhere on the page', + async function (this: EpdsWorld) { + // Negative assertion: ePDS accounts are passwordless, so a password + // input on any screen we land on is a leak. Upstream's sign-in-view + // renders ; auth-service's OTP form does not. + // Counting password inputs across the whole document keeps the + // assertion robust against future structural changes. + const page = getPage(this) + const passwordCount = await page.locator('input[type="password"]').count() + expect( + passwordCount, + `Found ${passwordCount} password input(s) on ${page.url()} — ePDS is passwordless, this is the upstream sign-in-view leak`, + ).toBe(0) + }, +) + +// Backdate `account_device.updated_at` past upstream's authenticationMaxAge +// (7d) via a pds-core /_internal/test hook gated on EPDS_TEST_HOOKS=1. The +// hook accepts a target DID and optional deviceId; omitting deviceId +// backdates every device row for that DID (sufficient for row 6's +// single-account device, while row 9 passes both did + deviceId for +// surgical targeting). +async function expireDeviceAccount(opts: { + did: string + deviceId?: string +}): Promise { + if (!testEnv.internalSecret) { + throw new Error( + 'E2E_INTERNAL_SECRET is unset — cannot call /_internal/test/expire-device-account', + ) + } + const url = new URL('/_internal/test/expire-device-account', testEnv.pdsUrl) + const res = await fetch(url, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-EPDS-Internal-Secret': testEnv.internalSecret, + }, + body: JSON.stringify(opts), + }) + if (!res.ok) { + const body = await res.text() + throw new Error(`expire-device-account hook failed: ${res.status} ${body}`) + } +} + +Given( + "the device's account_device row has been backdated past 7 days", + async function (this: EpdsWorld) { + if (!this.userDid) { + throw new Error('world.userDid missing — Background step must run first') + } + await expireDeviceAccount({ did: this.userDid }) + }, +) + +Given('the device has two bound accounts', async function (this: EpdsWorld) { + if (!testEnv.mailpitPass) return 'pending' + if (!this.userDid) { + throw new Error('world.userDid missing — Background step must run first') + } + // Drive a second OAuth sign-in within the SAME browser context (no + // resetBrowserContext — the dev-id/ses-id cookies from the Background + // sign-in must survive). accountManager.upsertDeviceAccount(deviceId, sub) + // on the existing dev-id then binds the new account to the same device + // row, leaving the first binding intact. + // + // Identity extraction matches createAccountViaOAuth's regex pattern + // (e2e/support/flows.ts:165-173) — kept inline rather than reusing + // that helper because it writes to world.userDid/userHandle/testEmail + // which we explicitly do NOT want to clobber here. + const page = getPage(this) + const otherEmail = `row9-other-${Date.now()}@example.com` + await page.goto(testEnv.demoTrustedUrl) + await page.fill('#email', otherEmail) + await page.click('button[type=submit]') + await expect(page.locator('#step-otp.active')).toBeVisible({ + timeout: 30_000, + }) + const message = await waitForEmail(`to:${otherEmail}`) + const otp = await extractOtp(message.ID) + await fillOtp(page, otp) + await page.waitForURL('**/welcome', { timeout: 30_000 }) + + const bodyText = await page.locator('body').innerText() + const didMatch = /did:[a-z0-9:]+/i.exec(bodyText) + if (!didMatch) { + throw new Error('Could not find DID on welcome page for second user') + } + const handleMatch = /@([\w.-]+)/.exec(bodyText) + this.otherUserEmail = otherEmail + this.otherUserDid = didMatch[0] + this.otherUserHandle = handleMatch ? handleMatch[1] : undefined +}) + +Given( + "the test user's account_device row has been backdated past 7 days", + async function (this: EpdsWorld) { + if (!this.userDid) { + throw new Error('world.userDid missing — Background step must run first') + } + await expireDeviceAccount({ did: this.userDid }) + }, +) + +Given( + "the other user's account_device row is fresh", + function (this: EpdsWorld) { + // No-op: the second sign-in driven by "the device has two bound + // accounts" leaves the other user's updatedAt at "now". This step + // exists to make the precondition explicit in the feature file. + if (!this.otherUserDid) { + throw new Error( + 'world.otherUserDid missing — "the device has two bound accounts" step must run first', + ) + } + }, +) diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index d931a764..d7fafc5f 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -1,15 +1,18 @@ @session-reuse @docker-only @email -Feature: Session-reuse resilience against stale device cookies - When a user's upstream @atproto/oauth-provider device cookies - (dev-id / ses-id) no longer match the server's device-session state, - the auth-service used to redirect them anyway. That landed the user on - pds-core's stock welcome page ("Authenticate / Create new account / - Sign in / Cancel") instead of the enriched ePDS account picker or the - email/OTP form. - - ePDS must recover gracefully from every variety of cookie/server - divergence by falling back to the auth-service email/OTP form and - clearing the stale cookies so the next visit starts clean. +Feature: Welcome-page guard suppresses upstream's authentication UI + Upstream @atproto/oauth-provider has two authentication UIs that ePDS + must never render: its stock welcome page ("Authenticate / Create new + account / Sign in / Cancel") and its sign-in-view (handle + password + form). Both are unreachable by design — ePDS accounts are passwordless + and authentication goes through auth-service's OTP flow. + + The pre-route guard in pds-core (welcome-page-guard.ts) intercepts + every guarded GET and bounces to auth-service whenever upstream would + otherwise render either UI. ePDS must recover gracefully from every + variety of cookie/server divergence — and from every PAR-parameter + combination that would otherwise force upstream into its sign-in + form — by falling back to the auth-service email/OTP form and + clearing stale cookies so the next visit starts clean. See docs/design/session-reuse-bugs.md for the full failure-mode taxonomy. This feature covers the externally-reproducible cases. @@ -140,3 +143,67 @@ Feature: Session-reuse resilience against stale device cookies When the demo client starts a new OAuth flow with the other user's handle as login_hint Then the login page renders directly at the OTP verification step And the OTP form will submit the other user's email + + # --------------------------------------------------------------------------- + # Sign-in-view leaks (rows 5/6/9 of the failure-mode taxonomy). Distinct + # from the welcome-page leaks above: here cookies and bindings are valid, + # but every binding upstream would consider has `loginRequired: true`, so + # upstream falls through to its sign-in-view (handle + password form). ePDS + # accounts are passwordless, so any path into that form is unusable. The + # guard must bounce these to auth-service for a fresh OTP round. + # + # Three independent triggers force every binding into loginRequired: + # Row 5 — stored PAR `parameters.prompt === 'login'` + # Row 6 — every binding's `account_device.updated_at` is older than + # upstream's authenticationMaxAge (7 days) + # Row 9 — `login_hint` resolves to a binding that's individually stale + # even though other bindings on the device are fresh; upstream + # pre-selects the hinted account and clicking it lands on + # sign-in-view + # --------------------------------------------------------------------------- + + Scenario: Row 5 — prompt=login forces every binding loginRequired + # The demo's "Force re-authentication" checkbox sets prompt=login on + # both the auth-service redirect query AND the PAR body. Auth-service's + # `shouldReuseSession` honours the query and serves the OTP form rather + # than redirecting to pds-core (session-reuse.ts:158, isForceLoginPrompt). + # The user completes OTP, and pds-core's epds-callback redirects to + # pds-core's own /oauth/authorize?request_uri=... with the now-fresh + # device cookies. At THAT hop the welcome-page guard fires: + # - cookie pair is valid (just set by the callback) + # - the device has one binding (the user just authenticated) + # - the stored PAR `parameters.prompt` is still 'login' (the demo also + # set it in the PAR body) + # Today the guard passes through; upstream's authorize() reads the + # stored PAR, marks the only session loginRequired, and the frontend + # renders sign-in-view. The guard must instead bounce back to + # auth-service for another OTP round. + When the demo client starts a new OAuth flow with prompt=login + And the user completes the OTP flow + Then the browser lands on the auth-service email-and-OTP form + And no upstream password field is rendered anywhere on the page + + # Reproducing row 6 needs server-side white-box access to backdate + # `account_device.updated_at` past upstream's authenticationMaxAge + # (7 days). A pds-core /_internal/test/expire-device-account hook + # provides this, gated on EPDS_TEST_HOOKS=1 && NODE_ENV !== 'production' + # (mirroring auth-service's expire-otp / expire-auth-flow hooks). + Scenario: Row 6 — every binding's auth age is older than 7 days + Given the device's account_device row has been backdated past 7 days + When the demo client starts a new OAuth flow + Then the browser lands on the auth-service email-and-OTP form + And no upstream password field is rendered anywhere on the page + + # Row 9 needs two bindings on the same device — one stale, one fresh — + # plus a login_hint that resolves to the stale one. Built on the same + # /_internal/test/expire-device-account hook used for row 6, plus a + # second OAuth sign-in driven within the SAME browser context (so the + # second account's upsertDeviceAccount binds to the existing dev-id + # rather than creating a fresh device). + Scenario: Row 9 — login_hint resolves to a stale binding on a multi-account device + Given the device has two bound accounts + And the test user's account_device row has been backdated past 7 days + And the other user's account_device row is fresh + When the demo client starts a new OAuth flow with the test user's handle as login_hint + Then the browser lands on the auth-service email-and-OTP form + And no upstream password field is rendered anywhere on the page From f5baf3c4faf723fca93c09308ca5861fd6c9dba8 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 17:42:18 +0000 Subject: [PATCH 02/11] test(pds-core): add expire-device-account hook for sign-in-view repros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a /_internal/test/expire-device-account hook to pds-core so the e2e suite can reproduce rows 6 and 9 of the welcome-page-guard scenario taxonomy without waiting out upstream's 7-day authenticationMaxAge. Hook design mirrors auth-service's /_internal/test/expire-otp: - Mounted only when EPDS_TEST_HOOKS=1 is set on the core service. - Refuses to start if NODE_ENV=production (defence in depth on top of the gate flag). - Authenticates via the existing x-internal-secret header (verifyInternalSecret in @certified-app/shared). - Body: {did, deviceId?}. Backdates account_device.updatedAt to 8 days ago for matching rows. Omitting deviceId backdates every device row for the DID (row 6's single-account device); passing both keys narrows to the targeted (deviceId, did) pair (row 9's multi-account device). Wires up the matching e2e steps: - "the device's account_device row has been backdated past 7 days" - "the device has two bound accounts" (drives a second sign-up in the same browser context so upsertDeviceAccount binds to the existing device, then picks the second user's handle) - "the test user's account_device row has been backdated past 7 days" + "the other user's account_device row is fresh" - When-step "the demo client starts a new OAuth flow" reused verbatim for row 6; row 9 reuses the existing login_hint variant. All three sign-in-view scenarios now reproduce against a fresh docker-compose stack: - Row 5 lands on upstream's sign-in-view (handle + password form). - Row 6 lands on upstream's chooser with the single stale binding; clicking it would yield sign-in-view (still a leak — the chooser has no other path). - Row 9 lands on upstream's chooser with both bindings; the hinted (stale) account leads to sign-in-view on click while the other (fresh) one auto-SSOs — equally a leak for the hinted user. Drive-by: feature comment for the sign-in-view section now notes the chooser-then-sign-in-view variant explicitly, so future readers don't assume sign-in-view is always rendered up front. Co-Authored-By: Claude Opus 4.7 (1M context) --- docker-compose.yml | 7 ++ .../session-reuse-bugs.steps.ts | 5 +- features/session-reuse-bugs.feature | 8 +- packages/pds-core/package.json | 2 + packages/pds-core/src/index.ts | 78 +++++++++++++++++++ pnpm-lock.yaml | 6 ++ 6 files changed, 101 insertions(+), 5 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 68e30b26..6d2e8a6c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -8,6 +8,13 @@ services: env_file: .env environment: - PDS_PORT=3000 + # Enable /_internal/test/* hooks used by the e2e suite (e.g. + # backdating account_device.updatedAt past upstream's 7-day + # authenticationMaxAge so checkLoginRequired returns true). Mounted + # only here and on the Railway pr-base preview env; production + # deployments leave it unset. The router throws at startup if + # NODE_ENV=production. + - EPDS_TEST_HOOKS=${EPDS_TEST_HOOKS:-1} volumes: - pds-data:/data restart: unless-stopped diff --git a/e2e/step-definitions/session-reuse-bugs.steps.ts b/e2e/step-definitions/session-reuse-bugs.steps.ts index e04c0bd9..6d418173 100644 --- a/e2e/step-definitions/session-reuse-bugs.steps.ts +++ b/e2e/step-definitions/session-reuse-bugs.steps.ts @@ -22,7 +22,7 @@ import { expect } from '@playwright/test' import type { EpdsWorld } from '../support/world.js' import { testEnv } from '../support/env.js' import { getPage } from '../support/utils.js' -import { createAccountViaOAuth } from '../support/flows.js' +import { createAccountViaOAuth, pickHandle } from '../support/flows.js' import { sharedBrowser } from '../support/hooks.js' import { clearMailpit, extractOtp, waitForEmail } from '../support/mailpit.js' import { fillOtp } from '../support/otp.js' @@ -652,7 +652,7 @@ async function expireDeviceAccount(opts: { method: 'POST', headers: { 'Content-Type': 'application/json', - 'X-EPDS-Internal-Secret': testEnv.internalSecret, + 'x-internal-secret': testEnv.internalSecret, }, body: JSON.stringify(opts), }) @@ -698,6 +698,7 @@ Given('the device has two bound accounts', async function (this: EpdsWorld) { const message = await waitForEmail(`to:${otherEmail}`) const otp = await extractOtp(message.ID) await fillOtp(page, otp) + await pickHandle(this) await page.waitForURL('**/welcome', { timeout: 30_000 }) const bodyText = await page.locator('body').innerText() diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index d7fafc5f..a02246f2 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -148,9 +148,11 @@ Feature: Welcome-page guard suppresses upstream's authentication UI # Sign-in-view leaks (rows 5/6/9 of the failure-mode taxonomy). Distinct # from the welcome-page leaks above: here cookies and bindings are valid, # but every binding upstream would consider has `loginRequired: true`, so - # upstream falls through to its sign-in-view (handle + password form). ePDS - # accounts are passwordless, so any path into that form is unusable. The - # guard must bounce these to auth-service for a fresh OTP round. + # upstream's only remaining path is to render its sign-in-view (handle + + # password form). The chooser may render first as an intermediate step, + # but every account on it leads to sign-in-view on click — equally a leak. + # ePDS accounts are passwordless, so any path into that form is unusable. + # The guard must bounce these to auth-service for a fresh OTP round. # # Three independent triggers force every binding into loginRequired: # Row 5 — stored PAR `parameters.prompt === 'login'` diff --git a/packages/pds-core/package.json b/packages/pds-core/package.json index 7dccdbfd..031c5053 100644 --- a/packages/pds-core/package.json +++ b/packages/pds-core/package.json @@ -15,11 +15,13 @@ "@atproto/pds": "^0.4.209", "@certified-app/shared": "workspace:*", "@did-plc/lib": "^0.0.4", + "better-sqlite3": "^12.0.0", "dotenv": "^16.3.1", "express": "^4.18.2", "serialize-javascript": "^7.0.5" }, "devDependencies": { + "@types/better-sqlite3": "^7.6.8", "@types/express": "^4.17.21", "@types/node": "^20.11.0", "@types/serialize-javascript": "^5.0.4", diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index 42cc215e..7edcc3fe 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -27,6 +27,7 @@ import { randomBytes } from 'node:crypto' import * as path from 'node:path' import { PDS, envToCfg, envToSecrets, readEnv } from '@atproto/pds' import { readFileSync } from 'node:fs' +import Database from 'better-sqlite3' /* v8 ignore next 3 -- module-level init, only testable via e2e */ const atprotoPdsPkg: { version: string } = JSON.parse( readFileSync(require.resolve('@atproto/pds/package.json'), 'utf8'), @@ -1088,6 +1089,83 @@ async function main() { res.json({ emails }) }) + // ========================================================================= + // Test-only hooks for the e2e suite. Mounted only when EPDS_TEST_HOOKS=1 + // and refused outright when NODE_ENV=production. Mirrors auth-service's + // /_internal/test/expire-otp pattern: a narrow UPDATE that backdates a + // single timestamp to reproduce time-dependent behaviour without waiting + // out the wall-clock TTL. Used by features/session-reuse-bugs.feature + // rows 6 and 9 to age account_device.updatedAt past upstream's + // authenticationMaxAge (7 days) so checkLoginRequired returns true for + // the targeted binding(s). + // ========================================================================= + + if (process.env.EPDS_TEST_HOOKS === '1') { + if (process.env.NODE_ENV === 'production') { + throw new Error( + 'EPDS_TEST_HOOKS=1 is set but NODE_ENV=production — refusing to mount test-only endpoints', + ) + } + const dataDir = process.env.PDS_DATA_DIRECTORY + if (!dataDir) { + throw new Error( + 'EPDS_TEST_HOOKS=1 but PDS_DATA_DIRECTORY is unset — cannot locate account.sqlite', + ) + } + const accountDbPath = `${dataDir.replace(/\/$/, '')}/account.sqlite` + logger.warn( + { accountDbPath }, + 'Test hooks ENABLED — /_internal/test/* routes are live (EPDS_TEST_HOOKS=1)', + ) + + pds.app.post( + '/_internal/test/expire-device-account', + express.json(), + (req, res) => { + if (!verifyInternalSecret(req.headers['x-internal-secret'])) { + res.status(401).json({ error: 'Unauthorized' }) + return + } + const did = ((req.body?.did as string) || '').trim() + const deviceId = + typeof req.body?.deviceId === 'string' + ? req.body.deviceId.trim() + : undefined + if (!did) { + res.status(400).json({ error: 'Missing did' }) + return + } + // 8 days ago — comfortably past upstream's 7-day authenticationMaxAge + // so checkLoginRequired returns true on every backdated row. + const past = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000).toISOString() + const db = new Database(accountDbPath) + try { + const stmt = deviceId + ? db.prepare( + 'UPDATE account_device SET updatedAt = ? WHERE did = ? AND deviceId = ?', + ) + : db.prepare('UPDATE account_device SET updatedAt = ? WHERE did = ?') + const result = deviceId + ? stmt.run(past, did, deviceId) + : stmt.run(past, did) + logger.warn( + { did, deviceId, updated: result.changes, past }, + 'Backdated account_device.updatedAt', + ) + res.json({ updated: result.changes }) + } catch (err) { + logger.error( + { err, did, deviceId }, + 'Failed to backdate account_device.updatedAt', + ) + res.status(500).json({ error: 'Internal server error' }) + } finally { + db.close() + } + }, + ) + } + // ========================================================================= // TLS check - used by Caddy on-demand TLS to verify handle ownership // ========================================================================= diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8b8edc4c..4752a9bb 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -155,6 +155,9 @@ importers: '@did-plc/lib': specifier: ^0.0.4 version: 0.0.4 + better-sqlite3: + specifier: ^12.0.0 + version: 12.6.2 dotenv: specifier: ^16.3.1 version: 16.6.1 @@ -165,6 +168,9 @@ importers: specifier: ^7.0.5 version: 7.0.5 devDependencies: + '@types/better-sqlite3': + specifier: ^7.6.8 + version: 7.6.13 '@types/express': specifier: ^4.17.21 version: 4.17.25 From 0f2b19d40b3c3bb38ca74d5f6521fb1521ee4162 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 18:54:01 +0000 Subject: [PATCH 03/11] refactor(pds-core): reuse PDS Kysely handle in expire-device-account hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original implementation in f5baf3c opened a fresh better-sqlite3 connection to account.sqlite. That introduces a second handle on the same WAL file as PDS's own accountManager, which led to opaque visibility issues during e2e: rows backdated by the hook weren't always visible to subsequent guard reads on the same request. Switch the hook to reuse pds.ctx.accountManager.db.db (the Kysely instance PDS itself uses for upsertDeviceAccount and friends). One handle, one WAL view, no two-connection coordination problems. Drops the better-sqlite3 + @types/better-sqlite3 deps from pds-core entirely — they were only ever pulled in for the test hook. Also tidies away the now-unused PDS_DATA_DIRECTORY lookup, since we no longer need to locate account.sqlite on disk. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/pds-core/package.json | 2 -- packages/pds-core/src/index.ts | 44 ++++++++++++++++------------------ pnpm-lock.yaml | 6 ----- 3 files changed, 20 insertions(+), 32 deletions(-) diff --git a/packages/pds-core/package.json b/packages/pds-core/package.json index 031c5053..7dccdbfd 100644 --- a/packages/pds-core/package.json +++ b/packages/pds-core/package.json @@ -15,13 +15,11 @@ "@atproto/pds": "^0.4.209", "@certified-app/shared": "workspace:*", "@did-plc/lib": "^0.0.4", - "better-sqlite3": "^12.0.0", "dotenv": "^16.3.1", "express": "^4.18.2", "serialize-javascript": "^7.0.5" }, "devDependencies": { - "@types/better-sqlite3": "^7.6.8", "@types/express": "^4.17.21", "@types/node": "^20.11.0", "@types/serialize-javascript": "^5.0.4", diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index 7edcc3fe..79fabc91 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -27,7 +27,6 @@ import { randomBytes } from 'node:crypto' import * as path from 'node:path' import { PDS, envToCfg, envToSecrets, readEnv } from '@atproto/pds' import { readFileSync } from 'node:fs' -import Database from 'better-sqlite3' /* v8 ignore next 3 -- module-level init, only testable via e2e */ const atprotoPdsPkg: { version: string } = JSON.parse( readFileSync(require.resolve('@atproto/pds/package.json'), 'utf8'), @@ -1106,22 +1105,14 @@ async function main() { 'EPDS_TEST_HOOKS=1 is set but NODE_ENV=production — refusing to mount test-only endpoints', ) } - const dataDir = process.env.PDS_DATA_DIRECTORY - if (!dataDir) { - throw new Error( - 'EPDS_TEST_HOOKS=1 but PDS_DATA_DIRECTORY is unset — cannot locate account.sqlite', - ) - } - const accountDbPath = `${dataDir.replace(/\/$/, '')}/account.sqlite` logger.warn( - { accountDbPath }, 'Test hooks ENABLED — /_internal/test/* routes are live (EPDS_TEST_HOOKS=1)', ) pds.app.post( '/_internal/test/expire-device-account', express.json(), - (req, res) => { + async (req, res) => { if (!verifyInternalSecret(req.headers['x-internal-secret'])) { res.status(401).json({ error: 'Unauthorized' }) return @@ -1137,30 +1128,35 @@ async function main() { } // 8 days ago — comfortably past upstream's 7-day authenticationMaxAge // so checkLoginRequired returns true on every backdated row. - const past = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000).toISOString() - const db = new Database(accountDbPath) + const past = new Date( + Date.now() - 8 * 24 * 60 * 60 * 1000, + ).toISOString() try { - const stmt = deviceId - ? db.prepare( - 'UPDATE account_device SET updatedAt = ? WHERE did = ? AND deviceId = ?', - ) - : db.prepare('UPDATE account_device SET updatedAt = ? WHERE did = ?') - const result = deviceId - ? stmt.run(past, did, deviceId) - : stmt.run(past, did) + // Reuse the PDS accountManager's own Kysely instance — same handle + // PDS uses for upsertDeviceAccount, so there are no two-connection + // WAL-visibility surprises. + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- account_device shape not exported by @atproto/pds + const db = pds.ctx.accountManager.db.db as any + let q = db + .updateTable('account_device') + .set({ updatedAt: past }) + .where('did', '=', did) + if (deviceId) { + q = q.where('deviceId', '=', deviceId) + } + const result = await q.executeTakeFirst() + const updated = Number(result?.numUpdatedRows ?? 0) logger.warn( - { did, deviceId, updated: result.changes, past }, + { did, deviceId, updated, past }, 'Backdated account_device.updatedAt', ) - res.json({ updated: result.changes }) + res.json({ updated }) } catch (err) { logger.error( { err, did, deviceId }, 'Failed to backdate account_device.updatedAt', ) res.status(500).json({ error: 'Internal server error' }) - } finally { - db.close() } }, ) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4752a9bb..8b8edc4c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -155,9 +155,6 @@ importers: '@did-plc/lib': specifier: ^0.0.4 version: 0.0.4 - better-sqlite3: - specifier: ^12.0.0 - version: 12.6.2 dotenv: specifier: ^16.3.1 version: 16.6.1 @@ -168,9 +165,6 @@ importers: specifier: ^7.0.5 version: 7.0.5 devDependencies: - '@types/better-sqlite3': - specifier: ^7.6.8 - version: 7.6.13 '@types/express': specifier: ^4.17.21 version: 4.17.25 From ee7813aa58f8942911311aef3190d58055b14970 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 18:54:23 +0000 Subject: [PATCH 04/11] fix(pds-core): bounce sign-in-view leaks past the welcome-page guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the rows 5/6/9 gap in the welcome-page-guard taxonomy (see #131). The guard previously short-circuited only on rows 1–4 (empty/malformed/stale device cookies, zero bindings) — i.e. the upstream "welcome page". It missed the upstream sign-in-view (handle + password form), which renders whenever every binding upstream considers has loginRequired: true. Three independent triggers force upstream into that state: Row 5 — stored PAR parameters.prompt === 'login' Row 6 — every binding's account_device.updatedAt is older than upstream's authenticationMaxAge (default 7 days) Row 9 — login_hint matches a binding which is itself stale, even when other bindings on the device are fresh ePDS accounts are passwordless (random unguessable password set at creation, never exposed), so any path into the upstream sign-in-view is a contract violation — the user gets a form they cannot submit. After the existing empty-bindings bounce, the guard now also: 1. Reads the stored PAR via provider.requestManager.store.readRequest and bounces when parameters.prompt === 'login'. 2. Computes upstream's candidate-binding set by mirroring its matchesHint logic (sub or preferred_username only) and bounces when every candidate has provider.checkLoginRequired === true. Both bounces go to auth-service with the existing prompt=login bounce URL, which forces the OTP flow. Also widens the e2e landing-assertion locator to accept both auth-service login-page initial-step variants (#step-email when there's no hint; #step-otp when login_hint resolves to a known email). Both paths land on the same auth-service login page — the assertion just needs to confirm "on auth-service, not pds-core". Unit tests extended to cover both bounce paths via stubbed provider.requestManager.store.readRequest and provider.checkLoginRequired. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../session-reuse-bugs.steps.ts | 10 +- .../src/__tests__/welcome-page-guard.test.ts | 23 +++ packages/pds-core/src/welcome-page-guard.ts | 164 +++++++++++++++++- 3 files changed, 190 insertions(+), 7 deletions(-) diff --git a/e2e/step-definitions/session-reuse-bugs.steps.ts b/e2e/step-definitions/session-reuse-bugs.steps.ts index 6d418173..32fcdc10 100644 --- a/e2e/step-definitions/session-reuse-bugs.steps.ts +++ b/e2e/step-definitions/session-reuse-bugs.steps.ts @@ -335,7 +335,15 @@ Then( 'the browser lands on the auth-service email-and-OTP form', async function (this: EpdsWorld) { const page = getPage(this) - await expect(page.locator('#email')).toBeVisible({ timeout: 30_000 }) + // Wait for one of the auth-service login-page steps to be visible. + // initialStep is 'email' on no-hint flows and 'otp' when a login_hint + // resolves to an email — both land on the same login-page route, just + // with a different step displayed initially. Either is a valid "lands + // on the login page" outcome; the assertion is "we're on auth-service, + // not pds-core". + await expect( + page.locator('#step-email:not(.hidden), #step-otp.active'), + ).toBeVisible({ timeout: 30_000 }) const url = new URL(page.url()) const authHost = new URL(testEnv.authUrl).host expect( diff --git a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts b/packages/pds-core/src/__tests__/welcome-page-guard.test.ts index 46adde1d..cbfeb25f 100644 --- a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts +++ b/packages/pds-core/src/__tests__/welcome-page-guard.test.ts @@ -233,6 +233,14 @@ type FakeProvider = { readDevice: (id: string) => Promise<{ sessionId: string } | null> } } + requestManager: { + store: { + readRequest: ( + id: string, + ) => Promise<{ parameters?: Record } | null> + } + } + checkLoginRequired: (binding: unknown) => boolean } /** Build a FakeProvider whose deviceStore returns a row matching the @@ -242,6 +250,15 @@ function makeProvider(opts: { bindings?: () => Promise sessionId?: string | null readDevice?: () => Promise<{ sessionId: string } | null> + // Stored PAR for the request_uri on the test URL. Returned shape mirrors + // what `(provider.requestManager as any).store.readRequest(id)` produces: + // a `{ parameters: { prompt?, login_hint?, ... } }` envelope. + readRequest?: ( + id: string, + ) => Promise<{ parameters?: Record } | null> + // Per-binding loginRequired predicate. Default: false (everything fresh). + // Tests for rows 6/9 supply a custom predicate. + checkLoginRequired?: (binding: unknown) => boolean }): FakeProvider { const ses = opts.sessionId === undefined ? VALID_SES : opts.sessionId return { @@ -256,6 +273,12 @@ function makeProvider(opts: { ), }, }, + requestManager: { + store: { + readRequest: vi.fn(opts.readRequest ?? (() => Promise.resolve(null))), + }, + }, + checkLoginRequired: vi.fn(opts.checkLoginRequired ?? (() => false)), } } diff --git a/packages/pds-core/src/welcome-page-guard.ts b/packages/pds-core/src/welcome-page-guard.ts index 269344ef..143a6257 100644 --- a/packages/pds-core/src/welcome-page-guard.ts +++ b/packages/pds-core/src/welcome-page-guard.ts @@ -21,7 +21,11 @@ * See docs/design/session-reuse-bugs.md for the full failure-mode taxonomy. */ import type { NextFunction, Request, Response } from 'express' -import type { OAuthProvider } from '@atproto/oauth-provider' +import type { + DeviceAccount, + DeviceId, + OAuthProvider, +} from '@atproto/oauth-provider' import { DEVICE_ID_BYTES_LENGTH, DEVICE_ID_PREFIX, @@ -29,7 +33,6 @@ import { SESSION_ID_PREFIX, } from '@atproto/oauth-provider' import type { Logger } from 'pino' -import { loadDeviceAccountEmails } from './lib/device-accounts.js' const DEVICE_ID_RE = new RegExp( `^${DEVICE_ID_PREFIX}[0-9a-f]{${DEVICE_ID_BYTES_LENGTH * 2}}$`, @@ -229,18 +232,167 @@ export function createWelcomePageGuard(opts: { // etc.), or a device with zero bound accounts (migration-005 1h // TTL purge for remember=0 rows), would otherwise sail past and // let upstream render its stock welcome page. Both cases come - // back as `null` or `[]` from the shared helper — bounce on - // either. - const emails = await loadDeviceAccountEmails({ + // back as `null` or `[]` from the helper — bounce on either. + const bindings = await loadDeviceBindings({ provider, deviceId: parsed.deviceId, sessionId: parsed.sessionId, logger, }) - if (!emails || emails.length === 0) { + if (!bindings || bindings.length === 0) { + bounceOrPass() + return + } + + // At this point bindings exist, so upstream won't render the welcome + // page. But it may still render its sign-in-view (handle + password + // form) when every binding it would consider has loginRequired: true + // — see oauth-provider.ts:622-624. ePDS accounts are passwordless, + // so any path into that form is unusable. Three independent triggers + // force upstream into that state: + // + // - stored PAR `parameters.prompt === 'login'` (forces every + // session loginRequired regardless of auth age) + // - every binding's auth age exceeds upstream's + // authenticationMaxAge (default 7d, applied per-binding) + // - login_hint matches one binding which is itself stale, even + // when other bindings on the device are fresh (upstream + // pre-selects the hinted account; clicking falls through to + // sign-in-view) + // + // Read the stored PAR parameters and compute upstream's + // candidate-binding set; bounce when every candidate would be + // loginRequired. See features/session-reuse-bugs.feature rows + // 5/6/9 for the externally-reproducible test cases. + const params = await loadStoredPar({ + provider, + requestUrl: req.url, + logger, + }) + if (params?.prompt === 'login') { + bounceOrPass() + return + } + const candidates = filterCandidateBindings(bindings, params?.login_hint) + if (candidates.every((b) => provider.checkLoginRequired(b))) { bounceOrPass() return } next() } } + +// --------------------------------------------------------------------------- +// Helpers used only by the guard middleware. Kept in this file rather than +// the shared `lib/device-accounts.ts` because (a) the guard needs the full +// DeviceAccount[] (with updatedAt + account.preferred_username) whereas the +// `/_internal/device-accounts` consumer only needs emails, and (b) the +// stored-PAR / login_hint logic is unique to the guard. +// --------------------------------------------------------------------------- + +type DeviceStoreLike = { + readDevice: (deviceId: DeviceId) => Promise<{ sessionId: string } | null> +} + +type StoredPar = { + prompt?: string + login_hint?: string +} + +// Local alias so the helpers' signatures read clearly. Upstream's +// DeviceAccount has the fields we need (`account` for the hint match, +// `updatedAt` for checkLoginRequired). +type Binding = DeviceAccount + +/** Returns the full bindings list for a (deviceId, sessionId) pair, or null + * on any miss (malformed ids, missing device row, ses-id mismatch, or any + * underlying error). Mirrors loadDeviceAccountEmails' miss semantics — never + * returns partial data. */ +async function loadDeviceBindings(opts: { + provider: OAuthProvider + deviceId: string + sessionId: string + logger?: Partial> +}): Promise { + const { provider, deviceId, sessionId, logger } = opts + if (!DEVICE_ID_RE.test(deviceId)) return null + if (!SESSION_ID_RE.test(sessionId)) return null + + const deviceStore = ( + provider.deviceManager as unknown as { store: DeviceStoreLike } + ).store + try { + const data = await deviceStore.readDevice(deviceId as DeviceId) + if (!data || data.sessionId !== sessionId) return null + } catch (err) { + logger?.error?.({ err, deviceId }, 'guard: readDevice failed') + return null + } + + try { + return await provider.accountManager.listDeviceAccounts( + deviceId as DeviceId, + ) + } catch (err) { + logger?.error?.({ err, deviceId }, 'guard: listDeviceAccounts failed') + return null + } +} + +/** Read the stored PAR parameters for the request_uri on the current URL. + * Returns null when the URL has no request_uri, when the lookup fails, or + * when stored data is shaped unexpectedly. Used to read `prompt` and + * `login_hint` for the row-5/9 bounce decisions. */ +async function loadStoredPar(opts: { + provider: OAuthProvider + requestUrl: string + logger?: Partial> +}): Promise { + const { provider, requestUrl, logger } = opts + let requestUri: string | null + try { + requestUri = new URL(requestUrl, 'https://placeholder').searchParams.get( + 'request_uri', + ) + } catch { + return null + } + if (!requestUri) return null + const REQUEST_URI_PREFIX = 'urn:ietf:params:oauth:request_uri:' + if (!requestUri.startsWith(REQUEST_URI_PREFIX)) return null + const requestId = decodeURIComponent( + requestUri.slice(REQUEST_URI_PREFIX.length), + ) + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- requestManager.store not in upstream types + const store = (provider.requestManager as any).store + const stored = await store.readRequest(requestId) + const params = stored?.parameters + if (!params || typeof params !== 'object') return null + return { + prompt: typeof params.prompt === 'string' ? params.prompt : undefined, + login_hint: + typeof params.login_hint === 'string' ? params.login_hint : undefined, + } + } catch (err) { + logger?.error?.({ err, requestId }, 'guard: store.readRequest failed') + return null + } +} + +/** Apply upstream's `matchesHint` semantics to narrow the binding list to + * the candidates upstream would consider. When no hint is set OR the hint + * matches no binding, all bindings are candidates (chooser-like). When the + * hint matches exactly one binding (sub or preferred_username), that's the + * only candidate. Mirrors oauth-provider.ts:1100-1108. */ +function filterCandidateBindings( + bindings: Binding[], + loginHint: string | undefined, +): Binding[] { + if (!loginHint) return bindings + const matched = bindings.filter( + ({ account }) => + account.sub === loginHint || account.preferred_username === loginHint, + ) + return matched.length === 1 ? matched : bindings +} From 7eb6e59ac243744537a58e1491b0a7b7bf4b1f75 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 19:04:11 +0000 Subject: [PATCH 05/11] test(e2e): row 9 needs the login_hint in the PAR body, not the URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous step impl set login_hint as a query param on the demo's /api/oauth/login redirect, which makes the demo append it to the /oauth/authorize URL — but NOT to the PAR body. Upstream's matchesHint runs against parameters loaded from the request_uri (i.e. the stored PAR), so a URL-only hint never reaches the candidate-filtering code that row 9 depends on. The realistic third-party OAuth-client pattern that exposes the row 9 sign-in-view leak puts login_hint in the PAR body. Switch the step to login_hint_location=body so the stored PAR carries the hint and the guard can filter the candidate-binding list down to the single stale binding (which then trips the every-candidate-loginRequired check and bounces). Renamed the step to "as a PAR-body login_hint" so the Gherkin reads unambiguously and doesn't shadow the existing URL-hint step (used by the unrelated "Login hint matches a bound account" scenario, which must stay on the URL-hint path). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../session-reuse-bugs.steps.ts | 23 +++++++++++++++++++ features/session-reuse-bugs.feature | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/e2e/step-definitions/session-reuse-bugs.steps.ts b/e2e/step-definitions/session-reuse-bugs.steps.ts index 32fcdc10..29bcb24b 100644 --- a/e2e/step-definitions/session-reuse-bugs.steps.ts +++ b/e2e/step-definitions/session-reuse-bugs.steps.ts @@ -196,6 +196,29 @@ When( }, ) +When( + "the demo client starts a new OAuth flow with the test user's handle as a PAR-body login_hint", + async function (this: EpdsWorld) { + if (!this.userHandle) { + throw new Error( + 'world.userHandle missing — Background step must run first', + ) + } + // login_hint_location=body puts the hint in the PAR body rather than the + // /oauth/authorize redirect URL — this matches the third-party + // OAuth-client pattern that drives upstream's matchesHint logic against + // `parameters.login_hint` from the stored PAR. Without this, upstream's + // request-manager-loaded parameters carry no hint and the guard's row 9 + // bounce condition can never fire (because filterCandidateBindings sees + // every binding as a candidate, not just the stale one). + const page = getPage(this) + const url = new URL('/api/oauth/login', testEnv.demoUrl) + url.searchParams.set('login_hint', this.userHandle) + url.searchParams.set('login_hint_location', 'body') + await page.goto(url.toString()) + }, +) + Given( 'another user has a separate PDS account', async function (this: EpdsWorld) { diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index a02246f2..d914ee8c 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -206,6 +206,6 @@ Feature: Welcome-page guard suppresses upstream's authentication UI Given the device has two bound accounts And the test user's account_device row has been backdated past 7 days And the other user's account_device row is fresh - When the demo client starts a new OAuth flow with the test user's handle as login_hint + When the demo client starts a new OAuth flow with the test user's handle as a PAR-body login_hint Then the browser lands on the auth-service email-and-OTP form And no upstream password field is rendered anywhere on the page From 59611831250dad1f621476b883dfb8d0abeb927e Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 19:12:46 +0000 Subject: [PATCH 06/11] refactor: address PR #129 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Threads addressed: - "completes the OTP flow" / "do we already have a step for this?" (e2e/step-definitions/session-reuse-bugs.steps.ts:628,650): the monolithic step duplicated the existing canonical sequence used elsewhere in the suite. Replace it in row 5 with the established composition: "enters the test email" → "OTP email arrives" → "enters the OTP code", and drop the bespoke step body entirely. - "this is bloating main()" (packages/pds-core/src/index.ts:1102): extract the EPDS_TEST_HOOKS=1 wiring (the expire-device-account route + its production-mode guard + the secret check) into a new lib/test-hooks.ts. main() now installs it via a single installTestHooks() call, matching the pattern used for the other middleware modules. - "guard is now misnamed" (welcome-page-guard.ts:248): expand the file's top docstring to describe both UIs the guard now suppresses (welcome page rows 1–4; sign-in-view rows 5/6/9) and explain why the filename is kept stable. The function/file names stay as-is to avoid churning every import site for no behavioural gain — the docstring carries the current scope. The fourth thread (welcome-page-guard.ts:281, "isn't replicating upstream's render decision brittle?") is a real architectural question; left for separate discussion in the PR rather than papered over in code. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../session-reuse-bugs.steps.ts | 21 ----- features/session-reuse-bugs.feature | 4 +- packages/pds-core/src/index.ts | 75 +--------------- packages/pds-core/src/lib/test-hooks.ts | 85 +++++++++++++++++++ packages/pds-core/src/welcome-page-guard.ts | 39 ++++++--- 5 files changed, 118 insertions(+), 106 deletions(-) create mode 100644 packages/pds-core/src/lib/test-hooks.ts diff --git a/e2e/step-definitions/session-reuse-bugs.steps.ts b/e2e/step-definitions/session-reuse-bugs.steps.ts index 29bcb24b..fd0e3109 100644 --- a/e2e/step-definitions/session-reuse-bugs.steps.ts +++ b/e2e/step-definitions/session-reuse-bugs.steps.ts @@ -625,27 +625,6 @@ When( }, ) -When('the user completes the OTP flow', async function (this: EpdsWorld) { - if (!testEnv.mailpitPass) return 'pending' - if (!this.testEmail) { - throw new Error( - 'world.testEmail missing — Background step must establish a returning user first', - ) - } - // Drive auth-service's email/OTP form to completion. Same shape as the - // Background's returning-user sign-in but invoked mid-scenario after - // the prompt=login redirect lands the user there. - const page = getPage(this) - await page.fill('#email', this.testEmail) - await page.click('button[type=submit]') - await expect(page.locator('#step-otp.active')).toBeVisible({ - timeout: 30_000, - }) - const message = await waitForEmail(`to:${this.testEmail}`) - const otp = await extractOtp(message.ID) - await fillOtp(page, otp) -}) - Then( 'no upstream password field is rendered anywhere on the page', async function (this: EpdsWorld) { diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index d914ee8c..059e9eda 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -181,7 +181,9 @@ Feature: Welcome-page guard suppresses upstream's authentication UI # renders sign-in-view. The guard must instead bounce back to # auth-service for another OTP round. When the demo client starts a new OAuth flow with prompt=login - And the user completes the OTP flow + And the user enters the test email on the login page + Then an OTP email arrives in the mail trap + When the user enters the OTP code Then the browser lands on the auth-service email-and-OTP form And no upstream password field is rendered anywhere on the page diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index 79fabc91..cde5262e 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -64,6 +64,7 @@ import { createChooserEnrichmentMiddleware } from './chooser-enrichment.js' import { createUpstreamFaviconMiddleware } from './upstream-favicon.js' import { createWelcomePageGuard } from './welcome-page-guard.js' import { loadDeviceAccountEmails } from './lib/device-accounts.js' +import { installTestHooks } from './lib/test-hooks.js' const logger = createLogger('pds-core') @@ -1088,79 +1089,7 @@ async function main() { res.json({ emails }) }) - // ========================================================================= - // Test-only hooks for the e2e suite. Mounted only when EPDS_TEST_HOOKS=1 - // and refused outright when NODE_ENV=production. Mirrors auth-service's - // /_internal/test/expire-otp pattern: a narrow UPDATE that backdates a - // single timestamp to reproduce time-dependent behaviour without waiting - // out the wall-clock TTL. Used by features/session-reuse-bugs.feature - // rows 6 and 9 to age account_device.updatedAt past upstream's - // authenticationMaxAge (7 days) so checkLoginRequired returns true for - // the targeted binding(s). - // ========================================================================= - - if (process.env.EPDS_TEST_HOOKS === '1') { - if (process.env.NODE_ENV === 'production') { - throw new Error( - 'EPDS_TEST_HOOKS=1 is set but NODE_ENV=production — refusing to mount test-only endpoints', - ) - } - logger.warn( - 'Test hooks ENABLED — /_internal/test/* routes are live (EPDS_TEST_HOOKS=1)', - ) - - pds.app.post( - '/_internal/test/expire-device-account', - express.json(), - async (req, res) => { - if (!verifyInternalSecret(req.headers['x-internal-secret'])) { - res.status(401).json({ error: 'Unauthorized' }) - return - } - const did = ((req.body?.did as string) || '').trim() - const deviceId = - typeof req.body?.deviceId === 'string' - ? req.body.deviceId.trim() - : undefined - if (!did) { - res.status(400).json({ error: 'Missing did' }) - return - } - // 8 days ago — comfortably past upstream's 7-day authenticationMaxAge - // so checkLoginRequired returns true on every backdated row. - const past = new Date( - Date.now() - 8 * 24 * 60 * 60 * 1000, - ).toISOString() - try { - // Reuse the PDS accountManager's own Kysely instance — same handle - // PDS uses for upsertDeviceAccount, so there are no two-connection - // WAL-visibility surprises. - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- account_device shape not exported by @atproto/pds - const db = pds.ctx.accountManager.db.db as any - let q = db - .updateTable('account_device') - .set({ updatedAt: past }) - .where('did', '=', did) - if (deviceId) { - q = q.where('deviceId', '=', deviceId) - } - const result = await q.executeTakeFirst() - const updated = Number(result?.numUpdatedRows ?? 0) - logger.warn( - { did, deviceId, updated, past }, - 'Backdated account_device.updatedAt', - ) - res.json({ updated }) - } catch (err) { - logger.error( - { err, did, deviceId }, - 'Failed to backdate account_device.updatedAt', - ) - res.status(500).json({ error: 'Internal server error' }) - } - }, - ) - } + installTestHooks({ pds, app: pds.app, logger }) // ========================================================================= // TLS check - used by Caddy on-demand TLS to verify handle ownership diff --git a/packages/pds-core/src/lib/test-hooks.ts b/packages/pds-core/src/lib/test-hooks.ts new file mode 100644 index 00000000..8272de4b --- /dev/null +++ b/packages/pds-core/src/lib/test-hooks.ts @@ -0,0 +1,85 @@ +/** + * E2E test-only hooks. Mounted only when EPDS_TEST_HOOKS=1 and refused + * outright when NODE_ENV=production. Mirrors auth-service's /_internal/test/* + * pattern: narrow UPDATEs that backdate a single timestamp to reproduce + * time-dependent behaviour without waiting out the wall-clock TTL. + * + * Currently exposes: + * POST /_internal/test/expire-device-account + * Body: {did, deviceId?} + * Backdates `account_device.updatedAt` 8 days into the past for the + * matching row(s). Used by features/session-reuse-bugs.feature rows 6 + * and 9 to age bindings past upstream's authenticationMaxAge (7d) so + * checkLoginRequired returns true for the targeted binding(s). + */ +import express, { type Application } from 'express' +import type { PDS } from '@atproto/pds' +import { verifyInternalSecret } from '@certified-app/shared' +import type { Logger } from 'pino' + +export function installTestHooks(opts: { + pds: PDS + app: Application + logger: Pick +}): void { + const { pds, app, logger } = opts + if (process.env.EPDS_TEST_HOOKS !== '1') return + if (process.env.NODE_ENV === 'production') { + throw new Error( + 'EPDS_TEST_HOOKS=1 is set but NODE_ENV=production — refusing to mount test-only endpoints', + ) + } + logger.warn( + 'Test hooks ENABLED — /_internal/test/* routes are live (EPDS_TEST_HOOKS=1)', + ) + + app.post( + '/_internal/test/expire-device-account', + express.json(), + async (req, res) => { + if (!verifyInternalSecret(req.headers['x-internal-secret'])) { + res.status(401).json({ error: 'Unauthorized' }) + return + } + const did = ((req.body?.did as string) || '').trim() + const deviceId = + typeof req.body?.deviceId === 'string' + ? req.body.deviceId.trim() + : undefined + if (!did) { + res.status(400).json({ error: 'Missing did' }) + return + } + // 8 days ago — comfortably past upstream's 7-day authenticationMaxAge + // so checkLoginRequired returns true on every backdated row. + const past = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000).toISOString() + try { + // Reuse the PDS accountManager's own Kysely instance — same handle + // PDS uses for upsertDeviceAccount, so there are no two-connection + // WAL-visibility surprises. + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- account_device shape not exported by @atproto/pds + const db = pds.ctx.accountManager.db.db as any + let q = db + .updateTable('account_device') + .set({ updatedAt: past }) + .where('did', '=', did) + if (deviceId) { + q = q.where('deviceId', '=', deviceId) + } + const result = await q.executeTakeFirst() + const updated = Number(result?.numUpdatedRows ?? 0) + logger.warn( + { did, deviceId, updated, past }, + 'Backdated account_device.updatedAt', + ) + res.json({ updated }) + } catch (err) { + logger.error( + { err, did, deviceId }, + 'Failed to backdate account_device.updatedAt', + ) + res.status(500).json({ error: 'Internal server error' }) + } + }, + ) +} diff --git a/packages/pds-core/src/welcome-page-guard.ts b/packages/pds-core/src/welcome-page-guard.ts index 143a6257..c4a7b18d 100644 --- a/packages/pds-core/src/welcome-page-guard.ts +++ b/packages/pds-core/src/welcome-page-guard.ts @@ -1,22 +1,39 @@ /** * Pre-route middleware that short-circuits any request which would cause - * upstream @atproto/oauth-provider to render its three-button welcome page - * ("Authenticate / Create new account / Sign in / Cancel"). + * upstream @atproto/oauth-provider to render either of its two stock + * authentication UIs: * - * That page is unreachable from ePDS by design — every entry point should - * either show the enriched chooser (when the device has bound accounts) - * or fall back to auth-service's email/OTP form. So whenever the current - * request resolves to a device with zero bound accounts (partial cookie - * pair, stale pair, migration-005 TTL purge, fixation race, etc.), we - * respond with a 303 redirect to auth-service and clear the stale - * device-session cookies. + * 1. The three-button welcome page ("Authenticate / Create new account / + * Sign in / Cancel") — rendered when the device has zero bound + * accounts (rows 1–4 of the failure-mode taxonomy: partial cookie + * pair, stale pair, migration-005 TTL purge, fixation race, etc.). + * + * 2. The sign-in-view (handle + password form) — rendered when bindings + * exist but every binding upstream considers has loginRequired: true + * (rows 5/6/9: stored PAR prompt=login; all bindings older than + * authenticationMaxAge; login_hint pre-selecting an individually + * stale binding). + * + * Both UIs are unreachable from ePDS by design — every entry point should + * either show the enriched chooser (when the device has fresh bound + * accounts) or fall back to auth-service's email/OTP form. ePDS accounts + * are passwordless, so the password form in particular is a contract + * violation: the user gets a form they cannot submit. + * + * The filename and function name retain "welcome-page" for stability — + * the welcome page was the original failure mode this guard handled, and + * a rename would churn every import site for no behavioural gain. The + * scope expanded organically as we discovered upstream's other + * unreachable UI; treat the names as historical rather than current. * * Upstream's DeviceManager.hasSession/getCookies has a side effect — it * deletes the device row on a partial cookie pair — so we re-parse the * cookies ourselves using the exported Zod schemas rather than calling * upstream. We then query account bindings via the public - * accountManager.listDeviceAccounts API. If bindings exist, we call - * next() and let upstream proceed; if not, we bounce. + * accountManager.listDeviceAccounts API. If bindings exist we additionally + * mirror upstream's matchesHint + checkLoginRequired logic to detect the + * sign-in-view path; on any bounce condition we 303 to auth-service and + * clear stale device-session cookies, otherwise we let upstream proceed. * * See docs/design/session-reuse-bugs.md for the full failure-mode taxonomy. */ From 13019a62381fb94859f911720c7a1790066df4fc Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 19:38:41 +0000 Subject: [PATCH 07/11] test(pds-core): cover sign-in-view bounce branches + extract shared device-bindings helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CI-driven changes: 1. Sonar flagged a 16-line duplicated block in welcome-page-guard.ts — loadDeviceBindings duplicated lib/device-accounts.ts's cookie-pair validation + listDeviceAccounts call. Extract loadDeviceBindings as a shared export from lib/device-accounts.ts; have both consumers call it. loadDeviceAccountEmails is now a thin projection over the shared helper. The logCtx string parameter keeps log lines from the two call sites distinguishable. 2. Coveralls flagged uncovered new lines on welcome-page-guard.ts (the row 5/6/9 bounce branches) and lib/test-hooks.ts (the entire file). Add focused unit tests: - welcome-page-guard.test.ts: rows 5/6/9 each get a "must bounce" test, plus three pass-through cases (row 7 hint→fresh; mixed freshness with no hint; hint matching no binding) and one fail-open case (store.readRequest throws → pass through with logged error). - test-hooks.test.ts: new suite covering installTestHooks. EPDS_TEST_HOOKS unset → 404 (route not mounted); NODE_ENV=production → throws at install; missing/wrong x-internal-secret → 401; missing did → 400; happy paths for both DID-only and (DID, deviceId) bodies; underlying query throwing → 500 with logged error; bigint numUpdatedRows coerced to Number. All 828 tests pass; format, lint, typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../pds-core/src/__tests__/test-hooks.test.ts | 311 ++++++++++++++++++ .../src/__tests__/welcome-page-guard.test.ts | 240 ++++++++++++++ packages/pds-core/src/lib/device-accounts.ts | 73 ++-- packages/pds-core/src/welcome-page-guard.ts | 57 +--- 4 files changed, 606 insertions(+), 75 deletions(-) create mode 100644 packages/pds-core/src/__tests__/test-hooks.test.ts diff --git a/packages/pds-core/src/__tests__/test-hooks.test.ts b/packages/pds-core/src/__tests__/test-hooks.test.ts new file mode 100644 index 00000000..58afd778 --- /dev/null +++ b/packages/pds-core/src/__tests__/test-hooks.test.ts @@ -0,0 +1,311 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import express from 'express' +import { installTestHooks } from '../lib/test-hooks.js' + +// Minimal Kysely-like update query mock: chains `.set().where()*.executeTakeFirst()` +// and records what was called for assertions. Enough for the hook — +// `installTestHooks` only ever calls these four chain methods. +function makeFakeUpdate() { + const wheres: Array<[string, string, unknown]> = [] + let setVals: Record = {} + let result: { numUpdatedRows: number | bigint } = { numUpdatedRows: 0 } + const chain = { + set(vals: Record) { + setVals = vals + return chain + }, + where(col: string, op: string, val: unknown) { + wheres.push([col, op, val]) + return chain + }, + executeTakeFirst() { + return Promise.resolve(result) + }, + } + return { + chain, + setUpdatedRows(n: number | bigint) { + result = { numUpdatedRows: n } + }, + inspect() { + return { setVals, wheres } + }, + } +} + +function makeFakePds(opts: { + fakeUpdate: ReturnType + failOnExecute?: boolean +}) { + return { + ctx: { + accountManager: { + db: { + db: { + updateTable: (table: string) => { + expect(table).toBe('account_device') + if (opts.failOnExecute) { + return { + set: () => ({ + where: () => ({ + executeTakeFirst: () => + Promise.reject(new Error('db down')), + }), + }), + } + } + return opts.fakeUpdate.chain + }, + }, + }, + }, + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any +} + +async function postHook( + app: express.Express, + body: Record, + headers: Record = {}, +): Promise<{ status: number; json: Record }> { + const server = app.listen(0) + const port = await new Promise((resolve, reject) => { + server.once('error', reject) + server.once('listening', () => { + const addr = server.address() + if (typeof addr === 'object' && addr) resolve(addr.port) + else reject(new Error('Failed to resolve ephemeral port')) + }) + }) + server.unref() + try { + const res = await fetch( + `http://127.0.0.1:${port}/_internal/test/expire-device-account`, + { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...headers }, + body: JSON.stringify(body), + }, + ) + const json = (await res.json().catch(() => ({}))) as Record + return { status: res.status, json } + } finally { + await new Promise((resolve) => { + server.close(() => { + resolve() + }) + }) + } +} + +describe('installTestHooks — expire-device-account', () => { + let priorEnv: { hooks?: string; secret?: string; node?: string } = {} + + beforeEach(() => { + priorEnv = { + hooks: process.env.EPDS_TEST_HOOKS, + secret: process.env.EPDS_INTERNAL_SECRET, + node: process.env.NODE_ENV, + } + delete process.env.NODE_ENV + process.env.EPDS_INTERNAL_SECRET = 'test-secret-1234' + }) + + afterEach(() => { + if (priorEnv.hooks === undefined) delete process.env.EPDS_TEST_HOOKS + else process.env.EPDS_TEST_HOOKS = priorEnv.hooks + if (priorEnv.secret === undefined) delete process.env.EPDS_INTERNAL_SECRET + else process.env.EPDS_INTERNAL_SECRET = priorEnv.secret + if (priorEnv.node === undefined) delete process.env.NODE_ENV + else process.env.NODE_ENV = priorEnv.node + }) + + it('does nothing when EPDS_TEST_HOOKS is unset', async () => { + delete process.env.EPDS_TEST_HOOKS + const fakeUpdate = makeFakeUpdate() + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger: { warn: vi.fn(), error: vi.fn() }, + }) + + const res = await postHook( + app, + { did: 'did:plc:a' }, + { 'x-internal-secret': 'test-secret-1234' }, + ) + // Route was never mounted, so the request 404s before any auth check. + expect(res.status).toBe(404) + }) + + it('refuses to install when NODE_ENV=production', () => { + process.env.EPDS_TEST_HOOKS = '1' + process.env.NODE_ENV = 'production' + const fakeUpdate = makeFakeUpdate() + const app = express() + expect(() => { + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger: { warn: vi.fn(), error: vi.fn() }, + }) + }).toThrow(/production/i) + }) + + it('rejects requests without the internal secret', async () => { + process.env.EPDS_TEST_HOOKS = '1' + const fakeUpdate = makeFakeUpdate() + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger: { warn: vi.fn(), error: vi.fn() }, + }) + + const res = await postHook(app, { did: 'did:plc:a' }) + expect(res.status).toBe(401) + }) + + it('rejects requests with the wrong secret', async () => { + process.env.EPDS_TEST_HOOKS = '1' + const fakeUpdate = makeFakeUpdate() + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger: { warn: vi.fn(), error: vi.fn() }, + }) + + const res = await postHook( + app, + { did: 'did:plc:a' }, + { 'x-internal-secret': 'wrong' }, + ) + expect(res.status).toBe(401) + }) + + it('rejects requests missing the did', async () => { + process.env.EPDS_TEST_HOOKS = '1' + const fakeUpdate = makeFakeUpdate() + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger: { warn: vi.fn(), error: vi.fn() }, + }) + + const res = await postHook( + app, + {}, + { 'x-internal-secret': 'test-secret-1234' }, + ) + expect(res.status).toBe(400) + expect(String(res.json.error)).toMatch(/did/i) + }) + + it('backdates every device row for the did when no deviceId is given', async () => { + process.env.EPDS_TEST_HOOKS = '1' + const fakeUpdate = makeFakeUpdate() + fakeUpdate.setUpdatedRows(2) + const logger = { warn: vi.fn(), error: vi.fn() } + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger, + }) + + const res = await postHook( + app, + { did: 'did:plc:a' }, + { 'x-internal-secret': 'test-secret-1234' }, + ) + + expect(res.status).toBe(200) + expect(res.json.updated).toBe(2) + const { setVals, wheres } = fakeUpdate.inspect() + // updatedAt is set to an ISO string ~8 days in the past. + const updatedAt = setVals.updatedAt as string + expect(typeof updatedAt).toBe('string') + const past = new Date(updatedAt).getTime() + const eightDaysAgo = Date.now() - 8 * 24 * 60 * 60 * 1000 + expect(Math.abs(past - eightDaysAgo)).toBeLessThan(60_000) + // Only one WHERE clause: did. No deviceId narrowing. + expect(wheres).toEqual([['did', '=', 'did:plc:a']]) + }) + + it('narrows by deviceId when both keys are provided', async () => { + process.env.EPDS_TEST_HOOKS = '1' + const fakeUpdate = makeFakeUpdate() + fakeUpdate.setUpdatedRows(1) + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger: { warn: vi.fn(), error: vi.fn() }, + }) + + const res = await postHook( + app, + { did: 'did:plc:a', deviceId: 'dev-deadbeef' }, + { 'x-internal-secret': 'test-secret-1234' }, + ) + + expect(res.status).toBe(200) + expect(res.json.updated).toBe(1) + const { wheres } = fakeUpdate.inspect() + expect(wheres).toEqual([ + ['did', '=', 'did:plc:a'], + ['deviceId', '=', 'dev-deadbeef'], + ]) + }) + + it('returns 500 and logs when the underlying update throws', async () => { + process.env.EPDS_TEST_HOOKS = '1' + const fakeUpdate = makeFakeUpdate() + const logger = { warn: vi.fn(), error: vi.fn() } + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate, failOnExecute: true }), + app, + logger, + }) + + const res = await postHook( + app, + { did: 'did:plc:a' }, + { 'x-internal-secret': 'test-secret-1234' }, + ) + + expect(res.status).toBe(500) + expect(logger.error).toHaveBeenCalledWith( + expect.objectContaining({ did: 'did:plc:a' }), + expect.stringContaining('Failed to backdate'), + ) + }) + + it('coerces bigint numUpdatedRows to a regular Number', async () => { + // Kysely's actual return type is `bigint` on better-sqlite3 driver. + process.env.EPDS_TEST_HOOKS = '1' + const fakeUpdate = makeFakeUpdate() + fakeUpdate.setUpdatedRows(BigInt(3)) + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate }), + app, + logger: { warn: vi.fn(), error: vi.fn() }, + }) + + const res = await postHook( + app, + { did: 'did:plc:a' }, + { 'x-internal-secret': 'test-secret-1234' }, + ) + + expect(res.status).toBe(200) + expect(res.json.updated).toBe(3) + expect(typeof res.json.updated).toBe('number') + }) +}) diff --git a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts b/packages/pds-core/src/__tests__/welcome-page-guard.test.ts index cbfeb25f..0bd6f10a 100644 --- a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts +++ b/packages/pds-core/src/__tests__/welcome-page-guard.test.ts @@ -701,4 +701,244 @@ describe('createWelcomePageGuard', () => { expect.stringContaining('readDevice failed'), ) }) + + // --------------------------------------------------------------------------- + // Sign-in-view leak coverage (rows 5 / 6 / 9 of the failure-mode taxonomy + // in features/session-reuse-bugs.feature). Each row is the same shape: + // valid cookies + non-empty bindings + a request_uri, but every binding + // upstream would consider has loginRequired === true. The guard must + // bounce instead of letting upstream render the password form. + // --------------------------------------------------------------------------- + + // Helper: build a binding fixture good enough for the guard's matchesHint + // logic. Only `account.sub` and `account.preferred_username` are read. + function binding(opts: { sub: string; pu: string }) { + return { + account: { sub: opts.sub, preferred_username: opts.pu }, + } as unknown as { account: { sub: string; preferred_username: string } } + } + + // The request_uri that the guard's loadStoredPar will look up. The + // urn-prefix matters — anything else makes the guard treat the URL as + // "no request_uri" and skip the stored-PAR read entirely. + const REQUEST_URI = 'urn:ietf:params:oauth:request_uri:req-abc' + + it('row 5: bounces when stored PAR has prompt === "login"', async () => { + const provider = makeProvider({ + bindings: () => + Promise.resolve([binding({ sub: 'did:plc:a', pu: 'a.example' })]), + // Even though the binding itself is fresh (default checkLoginRequired + // returns false), the prompt=login branch must bounce first. + readRequest: () => Promise.resolve({ parameters: { prompt: 'login' } }), + }) + const mw = createWelcomePageGuard({ + authHostname: AUTH, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + provider: provider as any, + cookieDomain: null, + }) + const res = makeRes() + await mw( + makeReq({ + url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, + cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, + }), + res, + vi.fn(), + ) + expect(res.status).toHaveBeenCalledWith(303) + // Sanity: checkLoginRequired must NOT have been consulted — the + // prompt=login check short-circuits ahead of it. + expect(provider.checkLoginRequired).not.toHaveBeenCalled() + }) + + it('row 6: bounces when every binding is loginRequired and there is no hint', async () => { + const provider = makeProvider({ + bindings: () => + Promise.resolve([ + binding({ sub: 'did:plc:a', pu: 'a.example' }), + binding({ sub: 'did:plc:b', pu: 'b.example' }), + ]), + // Empty stored PAR — no prompt=login, no login_hint. With every + // binding stale, the chooser would render and clicking either + // account would land on sign-in-view. + readRequest: () => Promise.resolve({ parameters: {} }), + checkLoginRequired: () => true, + }) + const mw = createWelcomePageGuard({ + authHostname: AUTH, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + provider: provider as any, + cookieDomain: null, + }) + const res = makeRes() + await mw( + makeReq({ + url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, + cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, + }), + res, + vi.fn(), + ) + expect(res.status).toHaveBeenCalledWith(303) + }) + + it('row 9: bounces when login_hint matches the only stale binding', async () => { + const stale = binding({ sub: 'did:plc:stale', pu: 'stale.example' }) + const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) + const provider = makeProvider({ + bindings: () => Promise.resolve([stale, fresh]), + readRequest: () => + Promise.resolve({ parameters: { login_hint: 'stale.example' } }), + // Only the stale binding is loginRequired. With the hint narrowing + // the candidate set to just `stale`, the every-loginRequired check + // succeeds and the guard bounces — even though `fresh` exists. + checkLoginRequired: (b: unknown) => + (b as { account: { sub: string } }).account.sub === 'did:plc:stale', + }) + const mw = createWelcomePageGuard({ + authHostname: AUTH, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + provider: provider as any, + cookieDomain: null, + }) + const res = makeRes() + await mw( + makeReq({ + url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, + cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, + }), + res, + vi.fn(), + ) + expect(res.status).toHaveBeenCalledWith(303) + }) + + it('passes through when login_hint matches a fresh binding (row 7)', async () => { + // Hint resolves to a single fresh binding → upstream's chooser / + // SSO redirect path is reachable. Guard must NOT bounce. + const stale = binding({ sub: 'did:plc:stale', pu: 'stale.example' }) + const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) + const provider = makeProvider({ + bindings: () => Promise.resolve([stale, fresh]), + readRequest: () => + Promise.resolve({ parameters: { login_hint: 'fresh.example' } }), + checkLoginRequired: (b: unknown) => + (b as { account: { sub: string } }).account.sub === 'did:plc:stale', + }) + const mw = createWelcomePageGuard({ + authHostname: AUTH, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + provider: provider as any, + cookieDomain: null, + }) + const res = makeRes() + const next = vi.fn() + await mw( + makeReq({ + url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, + cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, + }), + res, + next, + ) + expect(next).toHaveBeenCalledOnce() + expect(res.status).not.toHaveBeenCalled() + }) + + it('passes through when at least one binding is fresh and there is no hint', async () => { + // No prompt=login, no login_hint, mixed binding freshness → chooser + // reaches a usable session. Guard must pass through. + const stale = binding({ sub: 'did:plc:stale', pu: 'stale.example' }) + const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) + const provider = makeProvider({ + bindings: () => Promise.resolve([stale, fresh]), + readRequest: () => Promise.resolve({ parameters: {} }), + checkLoginRequired: (b: unknown) => + (b as { account: { sub: string } }).account.sub === 'did:plc:stale', + }) + const mw = createWelcomePageGuard({ + authHostname: AUTH, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + provider: provider as any, + cookieDomain: null, + }) + const res = makeRes() + const next = vi.fn() + await mw( + makeReq({ + url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, + cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, + }), + res, + next, + ) + expect(next).toHaveBeenCalledOnce() + expect(res.status).not.toHaveBeenCalled() + }) + + it('falls back to all bindings when login_hint matches none of them', async () => { + // matchesHint resolves to an empty set, so per upstream's logic + // every binding becomes a candidate. With at least one fresh, the + // guard passes through. + const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) + const provider = makeProvider({ + bindings: () => Promise.resolve([fresh]), + readRequest: () => + Promise.resolve({ parameters: { login_hint: 'unknown.example' } }), + checkLoginRequired: () => false, + }) + const mw = createWelcomePageGuard({ + authHostname: AUTH, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + provider: provider as any, + cookieDomain: null, + }) + const res = makeRes() + const next = vi.fn() + await mw( + makeReq({ + url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, + cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, + }), + res, + next, + ) + expect(next).toHaveBeenCalledOnce() + }) + + it('passes through when readRequest fails — fail-open on the PAR-read path', async () => { + // store.readRequest throwing means we don't know whether prompt=login + // is set, but bindings exist and (default) none are loginRequired. + // Failing closed here would 303 every flow whose PAR happens to be + // unreachable. Pass through and let upstream handle it. + const provider = makeProvider({ + bindings: () => + Promise.resolve([binding({ sub: 'did:plc:a', pu: 'a.example' })]), + readRequest: () => Promise.reject(new Error('store down')), + }) + const logger = { error: vi.fn() } + const mw = createWelcomePageGuard({ + authHostname: AUTH, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + provider: provider as any, + cookieDomain: null, + logger, + }) + const res = makeRes() + const next = vi.fn() + await mw( + makeReq({ + url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, + cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, + }), + res, + next, + ) + expect(next).toHaveBeenCalledOnce() + expect(logger.error).toHaveBeenCalledWith( + expect.any(Object), + expect.stringContaining('store.readRequest failed'), + ) + }) }) diff --git a/packages/pds-core/src/lib/device-accounts.ts b/packages/pds-core/src/lib/device-accounts.ts index ddfdf91a..c941783e 100644 --- a/packages/pds-core/src/lib/device-accounts.ts +++ b/packages/pds-core/src/lib/device-accounts.ts @@ -3,8 +3,7 @@ * /_internal/device-accounts endpoint. * * Validates a (dev-id, ses-id) cookie pair against the live device row, - * then asks `accountManager.listDeviceAccounts` for the bindings. Returns - * the bound account emails (lowercased) or null when validation fails. + * then asks `accountManager.listDeviceAccounts` for the bindings. * * "Validation fails" means: either id is syntactically malformed, the * device row is missing, the device row's active sessionId does not match @@ -12,7 +11,11 @@ * return null — never partial data — so callers don't accidentally trust * a stale half-state. */ -import type { DeviceId, OAuthProvider } from '@atproto/oauth-provider' +import type { + DeviceAccount, + DeviceId, + OAuthProvider, +} from '@atproto/oauth-provider' import { DEVICE_ID_BYTES_LENGTH, DEVICE_ID_PREFIX, @@ -28,29 +31,28 @@ const SESSION_ID_RE = new RegExp( `^${SESSION_ID_PREFIX}[0-9a-f]{${SESSION_ID_BYTES_LENGTH * 2}}$`, ) -/** See welcome-page-guard.ts — same minimal contract; duplicated here so - * this module doesn't depend on the guard. */ type DeviceStoreLike = { readDevice: (deviceId: DeviceId) => Promise<{ sessionId: string } | null> } -export type LoadDeviceAccountEmailsOpts = { +export type LoadDeviceBindingsOpts = { provider: OAuthProvider deviceId: string sessionId: string + /** Free-form prefix used in error log messages so multiple call sites + * remain distinguishable in logs. */ + logCtx: string logger?: Partial> } -/** Validate the cookie pair and return the lowercased emails of every - * account bound to the device, or `null` if the pair is malformed, - * unknown, or its ses-id doesn't match the device row. - * - * Lowercases emails to mirror `/_internal/account-by-email`'s normalised - * lookup so callers can compare a resolved login_hint email directly. */ -export async function loadDeviceAccountEmails( - opts: LoadDeviceAccountEmailsOpts, -): Promise { - const { provider, deviceId, sessionId, logger } = opts +/** Validate the cookie pair against the device row + return every binding + * for the device. Returns null on any miss (malformed ids, missing device + * row, ses-id mismatch, underlying error) — never partial data. Both the + * guard middleware and the /_internal/device-accounts endpoint use this. */ +export async function loadDeviceBindings( + opts: LoadDeviceBindingsOpts, +): Promise { + const { provider, deviceId, sessionId, logCtx, logger } = opts if (!DEVICE_ID_RE.test(deviceId)) return null if (!SESSION_ID_RE.test(sessionId)) return null @@ -61,23 +63,42 @@ export async function loadDeviceAccountEmails( const data = await deviceStore.readDevice(deviceId as DeviceId) if (!data || data.sessionId !== sessionId) return null } catch (err) { - logger?.error?.({ err, deviceId }, 'device-accounts: readDevice failed') + logger?.error?.({ err, deviceId }, `${logCtx}: readDevice failed`) return null } try { - const bindings = await provider.accountManager.listDeviceAccounts( + return await provider.accountManager.listDeviceAccounts( deviceId as DeviceId, ) - return bindings - .map((b) => b.account.email) - .filter((e): e is string => typeof e === 'string' && e.length > 0) - .map((e) => e.toLowerCase()) } catch (err) { - logger?.error?.( - { err, deviceId }, - 'device-accounts: listDeviceAccounts failed', - ) + logger?.error?.({ err, deviceId }, `${logCtx}: listDeviceAccounts failed`) return null } } + +export type LoadDeviceAccountEmailsOpts = { + provider: OAuthProvider + deviceId: string + sessionId: string + logger?: Partial> +} + +/** Validate the cookie pair and return the lowercased emails of every + * account bound to the device, or `null` on any miss. + * + * Lowercases emails to mirror `/_internal/account-by-email`'s normalised + * lookup so callers can compare a resolved login_hint email directly. */ +export async function loadDeviceAccountEmails( + opts: LoadDeviceAccountEmailsOpts, +): Promise { + const bindings = await loadDeviceBindings({ + ...opts, + logCtx: 'device-accounts', + }) + if (!bindings) return null + return bindings + .map((b) => b.account.email) + .filter((e): e is string => typeof e === 'string' && e.length > 0) + .map((e) => e.toLowerCase()) +} diff --git a/packages/pds-core/src/welcome-page-guard.ts b/packages/pds-core/src/welcome-page-guard.ts index c4a7b18d..662c27fb 100644 --- a/packages/pds-core/src/welcome-page-guard.ts +++ b/packages/pds-core/src/welcome-page-guard.ts @@ -38,11 +38,7 @@ * See docs/design/session-reuse-bugs.md for the full failure-mode taxonomy. */ import type { NextFunction, Request, Response } from 'express' -import type { - DeviceAccount, - DeviceId, - OAuthProvider, -} from '@atproto/oauth-provider' +import type { DeviceAccount, OAuthProvider } from '@atproto/oauth-provider' import { DEVICE_ID_BYTES_LENGTH, DEVICE_ID_PREFIX, @@ -50,6 +46,7 @@ import { SESSION_ID_PREFIX, } from '@atproto/oauth-provider' import type { Logger } from 'pino' +import { loadDeviceBindings } from './lib/device-accounts.js' const DEVICE_ID_RE = new RegExp( `^${DEVICE_ID_PREFIX}[0-9a-f]{${DEVICE_ID_BYTES_LENGTH * 2}}$`, @@ -254,6 +251,7 @@ export function createWelcomePageGuard(opts: { provider, deviceId: parsed.deviceId, sessionId: parsed.sessionId, + logCtx: 'guard', logger, }) if (!bindings || bindings.length === 0) { @@ -300,17 +298,13 @@ export function createWelcomePageGuard(opts: { } // --------------------------------------------------------------------------- -// Helpers used only by the guard middleware. Kept in this file rather than -// the shared `lib/device-accounts.ts` because (a) the guard needs the full -// DeviceAccount[] (with updatedAt + account.preferred_username) whereas the -// `/_internal/device-accounts` consumer only needs emails, and (b) the -// stored-PAR / login_hint logic is unique to the guard. +// Helpers used only by the guard middleware. The cookie-pair-validating +// `loadDeviceBindings` lives in `lib/device-accounts.ts` so this file and +// the /_internal/device-accounts endpoint share the same miss semantics. +// What's left here is the stored-PAR / login_hint logic that's unique to +// the guard. // --------------------------------------------------------------------------- -type DeviceStoreLike = { - readDevice: (deviceId: DeviceId) => Promise<{ sessionId: string } | null> -} - type StoredPar = { prompt?: string login_hint?: string @@ -321,41 +315,6 @@ type StoredPar = { // `updatedAt` for checkLoginRequired). type Binding = DeviceAccount -/** Returns the full bindings list for a (deviceId, sessionId) pair, or null - * on any miss (malformed ids, missing device row, ses-id mismatch, or any - * underlying error). Mirrors loadDeviceAccountEmails' miss semantics — never - * returns partial data. */ -async function loadDeviceBindings(opts: { - provider: OAuthProvider - deviceId: string - sessionId: string - logger?: Partial> -}): Promise { - const { provider, deviceId, sessionId, logger } = opts - if (!DEVICE_ID_RE.test(deviceId)) return null - if (!SESSION_ID_RE.test(sessionId)) return null - - const deviceStore = ( - provider.deviceManager as unknown as { store: DeviceStoreLike } - ).store - try { - const data = await deviceStore.readDevice(deviceId as DeviceId) - if (!data || data.sessionId !== sessionId) return null - } catch (err) { - logger?.error?.({ err, deviceId }, 'guard: readDevice failed') - return null - } - - try { - return await provider.accountManager.listDeviceAccounts( - deviceId as DeviceId, - ) - } catch (err) { - logger?.error?.({ err, deviceId }, 'guard: listDeviceAccounts failed') - return null - } -} - /** Read the stored PAR parameters for the request_uri on the current URL. * Returns null when the URL has no request_uri, when the lookup fails, or * when stored data is shaped unexpectedly. Used to read `prompt` and From 236dd393e06e20ecace51d909b78c53a53ca6948 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 19:44:38 +0000 Subject: [PATCH 08/11] test(pds-core): factor out test scaffolding to drop Sonar duplication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sonar flagged 123 dup lines on the previous push (107 in welcome-page-guard.test.ts, 16 in test-hooks.test.ts) — an artefact of 9-line and 11-line scenario blocks repeating the same setup boilerplate across 7 and 9 tests respectively. - welcome-page-guard.test.ts: extract a `signinViewCase()` helper that builds the (provider, mw, req, res, next) tuple from a single options bag, plus `expectBounce()` / `expectPassThrough()` tiny assertions. Each test body collapses to "supply the inputs that vary, assert the outcome". - test-hooks.test.ts: extract a `setupApp()` helper that returns {app, fakeUpdate, logger} ready to drive. Hoist the secret + auth header into module-level constants. The describe() beforeEach now also sets EPDS_TEST_HOOKS=1 so the per-test "process.env.EPDS_TEST_HOOKS = '1'" boilerplate is gone. 828 tests still pass; format, lint, typecheck clean. Behaviour unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../pds-core/src/__tests__/test-hooks.test.ts | 160 ++++------- .../src/__tests__/welcome-page-guard.test.ts | 259 +++++++----------- 2 files changed, 139 insertions(+), 280 deletions(-) diff --git a/packages/pds-core/src/__tests__/test-hooks.test.ts b/packages/pds-core/src/__tests__/test-hooks.test.ts index 58afd778..09ac7902 100644 --- a/packages/pds-core/src/__tests__/test-hooks.test.ts +++ b/packages/pds-core/src/__tests__/test-hooks.test.ts @@ -99,6 +99,34 @@ async function postHook( } } +/** Build an express app with installTestHooks applied to a default + * fake-pds + fake-update fixture. Centralises the boilerplate every test + * used to repeat. Tests that need to inspect the underlying update or + * override the failure mode pass extra options here. */ +function setupApp(opts?: { + failOnExecute?: boolean + updatedRows?: number | bigint +}): { + app: express.Express + fakeUpdate: ReturnType + logger: { warn: ReturnType; error: ReturnType } +} { + const fakeUpdate = makeFakeUpdate() + if (opts?.updatedRows !== undefined) + fakeUpdate.setUpdatedRows(opts.updatedRows) + const logger = { warn: vi.fn(), error: vi.fn() } + const app = express() + installTestHooks({ + pds: makeFakePds({ fakeUpdate, failOnExecute: opts?.failOnExecute }), + app, + logger, + }) + return { app, fakeUpdate, logger } +} + +const SECRET = 'test-secret-1234' +const AUTH_HEADER = { 'x-internal-secret': SECRET } + describe('installTestHooks — expire-device-account', () => { let priorEnv: { hooks?: string; secret?: string; node?: string } = {} @@ -109,7 +137,8 @@ describe('installTestHooks — expire-device-account', () => { node: process.env.NODE_ENV, } delete process.env.NODE_ENV - process.env.EPDS_INTERNAL_SECRET = 'test-secret-1234' + process.env.EPDS_INTERNAL_SECRET = SECRET + process.env.EPDS_TEST_HOOKS = '1' }) afterEach(() => { @@ -123,61 +152,27 @@ describe('installTestHooks — expire-device-account', () => { it('does nothing when EPDS_TEST_HOOKS is unset', async () => { delete process.env.EPDS_TEST_HOOKS - const fakeUpdate = makeFakeUpdate() - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger: { warn: vi.fn(), error: vi.fn() }, - }) - - const res = await postHook( - app, - { did: 'did:plc:a' }, - { 'x-internal-secret': 'test-secret-1234' }, - ) + const { app } = setupApp() // Route was never mounted, so the request 404s before any auth check. + const res = await postHook(app, { did: 'did:plc:a' }, AUTH_HEADER) expect(res.status).toBe(404) }) it('refuses to install when NODE_ENV=production', () => { - process.env.EPDS_TEST_HOOKS = '1' process.env.NODE_ENV = 'production' - const fakeUpdate = makeFakeUpdate() - const app = express() expect(() => { - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger: { warn: vi.fn(), error: vi.fn() }, - }) + setupApp() }).toThrow(/production/i) }) it('rejects requests without the internal secret', async () => { - process.env.EPDS_TEST_HOOKS = '1' - const fakeUpdate = makeFakeUpdate() - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger: { warn: vi.fn(), error: vi.fn() }, - }) - + const { app } = setupApp() const res = await postHook(app, { did: 'did:plc:a' }) expect(res.status).toBe(401) }) it('rejects requests with the wrong secret', async () => { - process.env.EPDS_TEST_HOOKS = '1' - const fakeUpdate = makeFakeUpdate() - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger: { warn: vi.fn(), error: vi.fn() }, - }) - + const { app } = setupApp() const res = await postHook( app, { did: 'did:plc:a' }, @@ -187,48 +182,21 @@ describe('installTestHooks — expire-device-account', () => { }) it('rejects requests missing the did', async () => { - process.env.EPDS_TEST_HOOKS = '1' - const fakeUpdate = makeFakeUpdate() - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger: { warn: vi.fn(), error: vi.fn() }, - }) - - const res = await postHook( - app, - {}, - { 'x-internal-secret': 'test-secret-1234' }, - ) + const { app } = setupApp() + const res = await postHook(app, {}, AUTH_HEADER) expect(res.status).toBe(400) expect(String(res.json.error)).toMatch(/did/i) }) it('backdates every device row for the did when no deviceId is given', async () => { - process.env.EPDS_TEST_HOOKS = '1' - const fakeUpdate = makeFakeUpdate() - fakeUpdate.setUpdatedRows(2) - const logger = { warn: vi.fn(), error: vi.fn() } - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger, - }) - - const res = await postHook( - app, - { did: 'did:plc:a' }, - { 'x-internal-secret': 'test-secret-1234' }, - ) + const { app, fakeUpdate } = setupApp({ updatedRows: 2 }) + const res = await postHook(app, { did: 'did:plc:a' }, AUTH_HEADER) expect(res.status).toBe(200) expect(res.json.updated).toBe(2) const { setVals, wheres } = fakeUpdate.inspect() // updatedAt is set to an ISO string ~8 days in the past. const updatedAt = setVals.updatedAt as string - expect(typeof updatedAt).toBe('string') const past = new Date(updatedAt).getTime() const eightDaysAgo = Date.now() - 8 * 24 * 60 * 60 * 1000 expect(Math.abs(past - eightDaysAgo)).toBeLessThan(60_000) @@ -237,47 +205,24 @@ describe('installTestHooks — expire-device-account', () => { }) it('narrows by deviceId when both keys are provided', async () => { - process.env.EPDS_TEST_HOOKS = '1' - const fakeUpdate = makeFakeUpdate() - fakeUpdate.setUpdatedRows(1) - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger: { warn: vi.fn(), error: vi.fn() }, - }) - + const { app, fakeUpdate } = setupApp({ updatedRows: 1 }) const res = await postHook( app, { did: 'did:plc:a', deviceId: 'dev-deadbeef' }, - { 'x-internal-secret': 'test-secret-1234' }, + AUTH_HEADER, ) expect(res.status).toBe(200) expect(res.json.updated).toBe(1) - const { wheres } = fakeUpdate.inspect() - expect(wheres).toEqual([ + expect(fakeUpdate.inspect().wheres).toEqual([ ['did', '=', 'did:plc:a'], ['deviceId', '=', 'dev-deadbeef'], ]) }) it('returns 500 and logs when the underlying update throws', async () => { - process.env.EPDS_TEST_HOOKS = '1' - const fakeUpdate = makeFakeUpdate() - const logger = { warn: vi.fn(), error: vi.fn() } - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate, failOnExecute: true }), - app, - logger, - }) - - const res = await postHook( - app, - { did: 'did:plc:a' }, - { 'x-internal-secret': 'test-secret-1234' }, - ) + const { app, logger } = setupApp({ failOnExecute: true }) + const res = await postHook(app, { did: 'did:plc:a' }, AUTH_HEADER) expect(res.status).toBe(500) expect(logger.error).toHaveBeenCalledWith( @@ -288,21 +233,8 @@ describe('installTestHooks — expire-device-account', () => { it('coerces bigint numUpdatedRows to a regular Number', async () => { // Kysely's actual return type is `bigint` on better-sqlite3 driver. - process.env.EPDS_TEST_HOOKS = '1' - const fakeUpdate = makeFakeUpdate() - fakeUpdate.setUpdatedRows(BigInt(3)) - const app = express() - installTestHooks({ - pds: makeFakePds({ fakeUpdate }), - app, - logger: { warn: vi.fn(), error: vi.fn() }, - }) - - const res = await postHook( - app, - { did: 'did:plc:a' }, - { 'x-internal-secret': 'test-secret-1234' }, - ) + const { app } = setupApp({ updatedRows: BigInt(3) }) + const res = await postHook(app, { did: 'did:plc:a' }, AUTH_HEADER) expect(res.status).toBe(200) expect(res.json.updated).toBe(3) diff --git a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts b/packages/pds-core/src/__tests__/welcome-page-guard.test.ts index 0bd6f10a..adcd0364 100644 --- a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts +++ b/packages/pds-core/src/__tests__/welcome-page-guard.test.ts @@ -704,239 +704,166 @@ describe('createWelcomePageGuard', () => { // --------------------------------------------------------------------------- // Sign-in-view leak coverage (rows 5 / 6 / 9 of the failure-mode taxonomy - // in features/session-reuse-bugs.feature). Each row is the same shape: - // valid cookies + non-empty bindings + a request_uri, but every binding - // upstream would consider has loginRequired === true. The guard must - // bounce instead of letting upstream render the password form. + // in features/session-reuse-bugs.feature). Every test below shares the + // same wiring — fresh cookies + non-empty bindings + a request_uri-bearing + // URL — and varies only in the stored PAR `prompt` / `login_hint` values + // and which bindings have loginRequired=true. The signinViewCase helper + // captures that wiring; each test just supplies the inputs and assertion. // --------------------------------------------------------------------------- // Helper: build a binding fixture good enough for the guard's matchesHint // logic. Only `account.sub` and `account.preferred_username` are read. - function binding(opts: { sub: string; pu: string }) { + function binding(sub: string, pu: string) { return { - account: { sub: opts.sub, preferred_username: opts.pu }, + account: { sub, preferred_username: pu }, } as unknown as { account: { sub: string; preferred_username: string } } } - // The request_uri that the guard's loadStoredPar will look up. The - // urn-prefix matters — anything else makes the guard treat the URL as - // "no request_uri" and skip the stored-PAR read entirely. + // urn-prefixed request_uri so loadStoredPar actually attempts the read + // (anything else makes it short-circuit and skip the row-5/9 logic). const REQUEST_URI = 'urn:ietf:params:oauth:request_uri:req-abc' - it('row 5: bounces when stored PAR has prompt === "login"', async () => { - const provider = makeProvider({ - bindings: () => - Promise.resolve([binding({ sub: 'did:plc:a', pu: 'a.example' })]), - // Even though the binding itself is fresh (default checkLoginRequired - // returns false), the prompt=login branch must bounce first. - readRequest: () => Promise.resolve({ parameters: { prompt: 'login' } }), - }) + type SigninViewCaseOpts = Parameters[0] & { + loggerError?: (...args: unknown[]) => void + } + /** Run the guard against the standard sign-in-view scenario fixture and + * return the (provider, res, next) trio for assertions. Centralises the + * repetitive setup that Sonar flagged as duplication. */ + async function signinViewCase(opts: SigninViewCaseOpts): Promise<{ + provider: FakeProvider + res: ReturnType + next: ReturnType + }> { + const { loggerError, ...providerOpts } = opts + const provider = makeProvider(providerOpts) + const logger = loggerError ? { error: loggerError } : undefined const mw = createWelcomePageGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, cookieDomain: null, + logger, }) const res = makeRes() + const next = vi.fn() await mw( makeReq({ url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, }), res, - vi.fn(), + next, ) + return { provider, res, next } + } + + /** Asserts the guard responded 303 (bounce). */ + function expectBounce(res: ReturnType): void { expect(res.status).toHaveBeenCalledWith(303) - // Sanity: checkLoginRequired must NOT have been consulted — the - // prompt=login check short-circuits ahead of it. + } + + /** Asserts the guard called next() and didn't touch res.status. */ + function expectPassThrough( + res: ReturnType, + next: ReturnType, + ): void { + expect(next).toHaveBeenCalledOnce() + expect(res.status).not.toHaveBeenCalled() + } + + it('row 5: bounces when stored PAR has prompt === "login"', async () => { + const { provider, res } = await signinViewCase({ + bindings: () => Promise.resolve([binding('did:plc:a', 'a.example')]), + // Even though the binding itself is fresh (checkLoginRequired + // defaults to false), the prompt=login branch must bounce first — + // and short-circuit ahead of the per-binding check. + readRequest: () => Promise.resolve({ parameters: { prompt: 'login' } }), + }) + expectBounce(res) expect(provider.checkLoginRequired).not.toHaveBeenCalled() }) it('row 6: bounces when every binding is loginRequired and there is no hint', async () => { - const provider = makeProvider({ + const { res } = await signinViewCase({ bindings: () => Promise.resolve([ - binding({ sub: 'did:plc:a', pu: 'a.example' }), - binding({ sub: 'did:plc:b', pu: 'b.example' }), + binding('did:plc:a', 'a.example'), + binding('did:plc:b', 'b.example'), ]), - // Empty stored PAR — no prompt=login, no login_hint. With every - // binding stale, the chooser would render and clicking either - // account would land on sign-in-view. readRequest: () => Promise.resolve({ parameters: {} }), checkLoginRequired: () => true, }) - const mw = createWelcomePageGuard({ - authHostname: AUTH, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - provider: provider as any, - cookieDomain: null, - }) - const res = makeRes() - await mw( - makeReq({ - url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, - cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, - }), - res, - vi.fn(), - ) - expect(res.status).toHaveBeenCalledWith(303) + expectBounce(res) }) + // Picker for the row-9-shaped fixture: hint narrows to one binding, + // others stay fresh. Returns a checkLoginRequired predicate that + // marks only `did:plc:stale` as loginRequired. + const onlyStaleIsLoginRequired = (b: unknown): boolean => + (b as { account: { sub: string } }).account.sub === 'did:plc:stale' + const TWO_BINDINGS = [ + binding('did:plc:stale', 'stale.example'), + binding('did:plc:fresh', 'fresh.example'), + ] + it('row 9: bounces when login_hint matches the only stale binding', async () => { - const stale = binding({ sub: 'did:plc:stale', pu: 'stale.example' }) - const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) - const provider = makeProvider({ - bindings: () => Promise.resolve([stale, fresh]), + const { res } = await signinViewCase({ + bindings: () => Promise.resolve(TWO_BINDINGS), readRequest: () => Promise.resolve({ parameters: { login_hint: 'stale.example' } }), - // Only the stale binding is loginRequired. With the hint narrowing - // the candidate set to just `stale`, the every-loginRequired check - // succeeds and the guard bounces — even though `fresh` exists. - checkLoginRequired: (b: unknown) => - (b as { account: { sub: string } }).account.sub === 'did:plc:stale', + checkLoginRequired: onlyStaleIsLoginRequired, }) - const mw = createWelcomePageGuard({ - authHostname: AUTH, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - provider: provider as any, - cookieDomain: null, - }) - const res = makeRes() - await mw( - makeReq({ - url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, - cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, - }), - res, - vi.fn(), - ) - expect(res.status).toHaveBeenCalledWith(303) + expectBounce(res) }) it('passes through when login_hint matches a fresh binding (row 7)', async () => { - // Hint resolves to a single fresh binding → upstream's chooser / - // SSO redirect path is reachable. Guard must NOT bounce. - const stale = binding({ sub: 'did:plc:stale', pu: 'stale.example' }) - const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) - const provider = makeProvider({ - bindings: () => Promise.resolve([stale, fresh]), + // Hint resolves to a single fresh binding → SSO/chooser path is + // reachable; the guard must NOT bounce. + const { res, next } = await signinViewCase({ + bindings: () => Promise.resolve(TWO_BINDINGS), readRequest: () => Promise.resolve({ parameters: { login_hint: 'fresh.example' } }), - checkLoginRequired: (b: unknown) => - (b as { account: { sub: string } }).account.sub === 'did:plc:stale', - }) - const mw = createWelcomePageGuard({ - authHostname: AUTH, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - provider: provider as any, - cookieDomain: null, + checkLoginRequired: onlyStaleIsLoginRequired, }) - const res = makeRes() - const next = vi.fn() - await mw( - makeReq({ - url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, - cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, - }), - res, - next, - ) - expect(next).toHaveBeenCalledOnce() - expect(res.status).not.toHaveBeenCalled() + expectPassThrough(res, next) }) it('passes through when at least one binding is fresh and there is no hint', async () => { - // No prompt=login, no login_hint, mixed binding freshness → chooser - // reaches a usable session. Guard must pass through. - const stale = binding({ sub: 'did:plc:stale', pu: 'stale.example' }) - const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) - const provider = makeProvider({ - bindings: () => Promise.resolve([stale, fresh]), + // Mixed freshness, no hint → chooser reaches a usable session. + const { res, next } = await signinViewCase({ + bindings: () => Promise.resolve(TWO_BINDINGS), readRequest: () => Promise.resolve({ parameters: {} }), - checkLoginRequired: (b: unknown) => - (b as { account: { sub: string } }).account.sub === 'did:plc:stale', + checkLoginRequired: onlyStaleIsLoginRequired, }) - const mw = createWelcomePageGuard({ - authHostname: AUTH, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - provider: provider as any, - cookieDomain: null, - }) - const res = makeRes() - const next = vi.fn() - await mw( - makeReq({ - url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, - cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, - }), - res, - next, - ) - expect(next).toHaveBeenCalledOnce() - expect(res.status).not.toHaveBeenCalled() + expectPassThrough(res, next) }) it('falls back to all bindings when login_hint matches none of them', async () => { - // matchesHint resolves to an empty set, so per upstream's logic - // every binding becomes a candidate. With at least one fresh, the - // guard passes through. - const fresh = binding({ sub: 'did:plc:fresh', pu: 'fresh.example' }) - const provider = makeProvider({ - bindings: () => Promise.resolve([fresh]), + // matchesHint → empty set → upstream treats all bindings as candidates; + // with at least one fresh, the guard passes through. + const { next } = await signinViewCase({ + bindings: () => + Promise.resolve([binding('did:plc:fresh', 'fresh.example')]), readRequest: () => Promise.resolve({ parameters: { login_hint: 'unknown.example' } }), checkLoginRequired: () => false, }) - const mw = createWelcomePageGuard({ - authHostname: AUTH, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - provider: provider as any, - cookieDomain: null, - }) - const res = makeRes() - const next = vi.fn() - await mw( - makeReq({ - url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, - cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, - }), - res, - next, - ) expect(next).toHaveBeenCalledOnce() }) it('passes through when readRequest fails — fail-open on the PAR-read path', async () => { // store.readRequest throwing means we don't know whether prompt=login - // is set, but bindings exist and (default) none are loginRequired. + // is set, but bindings exist and none are loginRequired by default. // Failing closed here would 303 every flow whose PAR happens to be - // unreachable. Pass through and let upstream handle it. - const provider = makeProvider({ - bindings: () => - Promise.resolve([binding({ sub: 'did:plc:a', pu: 'a.example' })]), + // unreachable; pass through and let upstream handle it. + const errSpy = vi.fn() + const { res, next } = await signinViewCase({ + bindings: () => Promise.resolve([binding('did:plc:a', 'a.example')]), readRequest: () => Promise.reject(new Error('store down')), + loggerError: errSpy, }) - const logger = { error: vi.fn() } - const mw = createWelcomePageGuard({ - authHostname: AUTH, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - provider: provider as any, - cookieDomain: null, - logger, - }) - const res = makeRes() - const next = vi.fn() - await mw( - makeReq({ - url: `/oauth/authorize?request_uri=${encodeURIComponent(REQUEST_URI)}`, - cookieHeader: `dev-id=${VALID_DEV}; ses-id=${VALID_SES}`, - }), - res, - next, - ) - expect(next).toHaveBeenCalledOnce() - expect(logger.error).toHaveBeenCalledWith( + expectPassThrough(res, next) + expect(errSpy).toHaveBeenCalledWith( expect.any(Object), expect.stringContaining('store.readRequest failed'), ) From ac6b8c764e656df307a7f5799bd5f905b94529f8 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 19:55:38 +0000 Subject: [PATCH 09/11] =?UTF-8?q?refactor(pds-core):=20rename=20welcome-pa?= =?UTF-8?q?ge-guard=20=E2=86=92=20auth-ui-guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard's scope grew past the welcome page to also cover upstream's sign-in-view (rows 5/6/9 of the failure-mode taxonomy). The welcome-page name no longer captures what the guard actually does — it suppresses BOTH stock authentication UIs that ePDS must never surface. Renames: - packages/pds-core/src/welcome-page-guard.ts → auth-ui-guard.ts - packages/pds-core/src/__tests__/welcome-page-guard.test.ts → auth-ui-guard.test.ts - createWelcomePageGuard() → createAuthUiGuard() - inner welcomePageGuard middleware → authUiGuard - install/error log strings updated to "Auth-UI guard …" Updates references in: - packages/pds-core/src/index.ts (import + install block + section header + fail-closed error messages) - packages/pds-core/src/cookie-domain.ts (two doc comments) - packages/pds-core/src/lib/device-accounts.ts (top docstring) - packages/auth-service/src/routes/login-page.ts (one inline comment about the orphan-cookie bounce path) - docs/design/{cross-client-session-reuse,pds-white-boxing}.md The branch name "welcome-guard-inadequate" stays as-is — it's an ephemeral throwaway. The describe(...) test labels are updated naturally by the symbol rename. Behaviour unchanged. 828 tests pass; format, lint, typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/design/cross-client-session-reuse.md | 2 +- docs/design/pds-white-boxing.md | 2 +- .../auth-service/src/routes/login-page.ts | 2 +- ...ge-guard.test.ts => auth-ui-guard.test.ts} | 38 +++++++------- ...welcome-page-guard.ts => auth-ui-guard.ts} | 10 +--- packages/pds-core/src/cookie-domain.ts | 4 +- packages/pds-core/src/index.ts | 50 +++++++++++-------- packages/pds-core/src/lib/device-accounts.ts | 2 +- 8 files changed, 56 insertions(+), 54 deletions(-) rename packages/pds-core/src/__tests__/{welcome-page-guard.test.ts => auth-ui-guard.test.ts} (97%) rename packages/pds-core/src/{welcome-page-guard.ts => auth-ui-guard.ts} (97%) diff --git a/docs/design/cross-client-session-reuse.md b/docs/design/cross-client-session-reuse.md index 765c70a8..da2179e8 100644 --- a/docs/design/cross-client-session-reuse.md +++ b/docs/design/cross-client-session-reuse.md @@ -446,7 +446,7 @@ The auto-skip predicate is, with all conditions required: 1. `client_id` is on `PDS_OAUTH_TRUSTED_CLIENTS`. 2. Client metadata advertises `epds_skip_chooser_on_match: true` (per-client opt-in; default off). -3. The welcome-page-guard's existing checks pass: +3. The auth-ui-guard's existing checks pass: - `dev-id`/`ses-id` cookies parse and ses-id matches the device row's active sessionId. - `accountManager.listDeviceAccounts(deviceId)` returns at least diff --git a/docs/design/pds-white-boxing.md b/docs/design/pds-white-boxing.md index c061c87f..9fb8dcaa 100644 --- a/docs/design/pds-white-boxing.md +++ b/docs/design/pds-white-boxing.md @@ -321,7 +321,7 @@ pds-core back to the auth-service in a loop. ### 18. Welcome-page guard pre-routes `/oauth/authorize` and `/account*` -**File:** `packages/pds-core/src/welcome-page-guard.ts`, wired in +**File:** `packages/pds-core/src/auth-ui-guard.ts`, wired in `packages/pds-core/src/index.ts` A pre-route Express middleware intercepts `GET /oauth/authorize` and diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 310f1318..b4435ee0 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -235,7 +235,7 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { // DeviceManager cannot hydrate from. Emit Max-Age=0 clears for both // cookies in both host-only and shared-parent-domain scopes so the // next OAuth flow gets a clean slate — otherwise the orphan half - // keeps bouncing through pds-core's welcome-page-guard every time. + // keeps bouncing through pds-core's auth-ui-guard every time. const orphan = hasOrphanDeviceCookie(sessionReuseReq) if (orphan.isOrphan) { const cookieDomain = deriveSharedCookieDomain( diff --git a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts b/packages/pds-core/src/__tests__/auth-ui-guard.test.ts similarity index 97% rename from packages/pds-core/src/__tests__/welcome-page-guard.test.ts rename to packages/pds-core/src/__tests__/auth-ui-guard.test.ts index adcd0364..7e9b5428 100644 --- a/packages/pds-core/src/__tests__/welcome-page-guard.test.ts +++ b/packages/pds-core/src/__tests__/auth-ui-guard.test.ts @@ -3,10 +3,10 @@ import { describe, expect, it, vi } from 'vitest' import { appendCookieClearHeaders, buildBounceUrl, - createWelcomePageGuard, + createAuthUiGuard, isGuardedPath, parseDeviceCookies, -} from '../welcome-page-guard.js' +} from '../auth-ui-guard.js' const VALID_DEV = 'dev-0123456789abcdef0123456789abcdef' const VALID_SES = 'ses-fedcba9876543210fedcba9876543210' @@ -321,12 +321,12 @@ function makeRes(): Response & { return r as any } -describe('createWelcomePageGuard', () => { +describe('createAuthUiGuard', () => { const AUTH = 'auth.pds.example' it('passes non-GET requests through', async () => { const provider = makeProvider({}) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -344,7 +344,7 @@ describe('createWelcomePageGuard', () => { it('passes unguarded paths through', async () => { const provider = makeProvider({}) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -357,7 +357,7 @@ describe('createWelcomePageGuard', () => { }) it('passes through when provider is null (OAuth disabled)', async () => { - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, provider: null, cookieDomain: null, @@ -373,7 +373,7 @@ describe('createWelcomePageGuard', () => { it('bounces with cookie clears when cookies are missing', async () => { const provider = makeProvider({ bindings: () => Promise.resolve([]) }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -408,7 +408,7 @@ describe('createWelcomePageGuard', () => { // "Missing request_uri" — worse than letting upstream render. The // guard explicitly opts out of this case. const provider = makeProvider({}) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -425,7 +425,7 @@ describe('createWelcomePageGuard', () => { it('passes /account* through when bindings are zero but there is no request_uri', async () => { const provider = makeProvider({ bindings: () => Promise.resolve([]) }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -448,7 +448,7 @@ describe('createWelcomePageGuard', () => { it('bounces when cookies parse but bindings are empty', async () => { const provider = makeProvider({ bindings: () => Promise.resolve([]) }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -475,7 +475,7 @@ describe('createWelcomePageGuard', () => { const provider = makeProvider({ bindings: () => Promise.resolve([{ some: 'binding' }]), }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -498,7 +498,7 @@ describe('createWelcomePageGuard', () => { const provider = makeProvider({ bindings: () => Promise.reject(new Error('db down')), }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -525,7 +525,7 @@ describe('createWelcomePageGuard', () => { const err = new Error('db down') const provider = makeProvider({ bindings: () => Promise.reject(err) }) const logger = { error: vi.fn() } - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -551,7 +551,7 @@ describe('createWelcomePageGuard', () => { // triggers ERR_INVALID_URL). The guard must treat an unparseable URL // as "no OAuth context" and call next() rather than crash the request. const provider = makeProvider({}) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -584,7 +584,7 @@ describe('createWelcomePageGuard', () => { bindings: () => Promise.resolve([{ some: 'binding' }]), sessionId: ACTIVE_SES, }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -616,7 +616,7 @@ describe('createWelcomePageGuard', () => { bindings: () => Promise.resolve([{ some: 'binding' }]), sessionId: null, }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -648,7 +648,7 @@ describe('createWelcomePageGuard', () => { bindings: () => Promise.resolve([{ some: 'binding' }]), sessionId: ACTIVE_SES, }) - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -679,7 +679,7 @@ describe('createWelcomePageGuard', () => { readDevice: () => Promise.reject(err), }) const logger = { error: vi.fn() } - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, @@ -737,7 +737,7 @@ describe('createWelcomePageGuard', () => { const { loggerError, ...providerOpts } = opts const provider = makeProvider(providerOpts) const logger = loggerError ? { error: loggerError } : undefined - const mw = createWelcomePageGuard({ + const mw = createAuthUiGuard({ authHostname: AUTH, // eslint-disable-next-line @typescript-eslint/no-explicit-any provider: provider as any, diff --git a/packages/pds-core/src/welcome-page-guard.ts b/packages/pds-core/src/auth-ui-guard.ts similarity index 97% rename from packages/pds-core/src/welcome-page-guard.ts rename to packages/pds-core/src/auth-ui-guard.ts index 662c27fb..40e7c7a8 100644 --- a/packages/pds-core/src/welcome-page-guard.ts +++ b/packages/pds-core/src/auth-ui-guard.ts @@ -20,12 +20,6 @@ * are passwordless, so the password form in particular is a contract * violation: the user gets a form they cannot submit. * - * The filename and function name retain "welcome-page" for stability — - * the welcome page was the original failure mode this guard handled, and - * a rename would churn every import site for no behavioural gain. The - * scope expanded organically as we discovered upstream's other - * unreachable UI; treat the names as historical rather than current. - * * Upstream's DeviceManager.hasSession/getCookies has a side effect — it * deletes the device row on a partial cookie pair — so we re-parse the * cookies ourselves using the exported Zod schemas rather than calling @@ -192,14 +186,14 @@ export function appendCookieClearHeaders( /** Create the Express middleware. `cookieDomain` may be null when the * auth-service and pds-core don't share a common parent domain — in * that case there's no domain-scoped cookie to clear. */ -export function createWelcomePageGuard(opts: { +export function createAuthUiGuard(opts: { authHostname: string provider: OAuthProvider | null cookieDomain: string | null logger?: Partial> }) { const { authHostname, provider, cookieDomain, logger } = opts - return async function welcomePageGuard( + return async function authUiGuard( req: Request, res: Response, next: NextFunction, diff --git a/packages/pds-core/src/cookie-domain.ts b/packages/pds-core/src/cookie-domain.ts index 946d2e89..01bcf705 100644 --- a/packages/pds-core/src/cookie-domain.ts +++ b/packages/pds-core/src/cookie-domain.ts @@ -64,7 +64,7 @@ export const DEVICE_COOKIE_NAMES = new Set([ * - Returns the input unchanged if it is a clearing cookie (`Max-Age=0` * or a past `Expires`). Browsers only clear a host-only cookie if the * clearing Set-Cookie itself carries no `Domain=`; auto-scoping a - * clear would silently neuter callers (e.g. welcome-page-guard) that + * clear would silently neuter callers (e.g. auth-ui-guard) that * intentionally emit BOTH a host-only clear and a Domain-scoped clear * to evict cookies in both scopes. Without this guard the host-only * variant of a stale device cookie can never be removed once the @@ -93,7 +93,7 @@ export function rewriteSetCookie(value: string, domain: string): string { * than or equal to zero (0), let expiry-time be the earliest * representable date") or a past `Expires=` date. Numeric `Max-Age` * is the canonical form upstream uses for host-only/Domain-scoped - * clears in welcome-page-guard. + * clears in auth-ui-guard. */ function isClearingCookie(value: string): boolean { if (/;\s*Max-Age\s*=\s*-?0+\b/i.test(value)) return true diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index cde5262e..b9f96f35 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -62,7 +62,7 @@ import { } from './cookie-domain.js' import { createChooserEnrichmentMiddleware } from './chooser-enrichment.js' import { createUpstreamFaviconMiddleware } from './upstream-favicon.js' -import { createWelcomePageGuard } from './welcome-page-guard.js' +import { createAuthUiGuard } from './auth-ui-guard.js' import { loadDeviceAccountEmails } from './lib/device-accounts.js' import { installTestHooks } from './lib/test-hooks.js' @@ -674,44 +674,52 @@ async function main() { } // ========================================================================= - // Welcome-page guard: never let upstream render the three-button page + // Auth-UI guard: never let upstream render the welcome page or sign-in-view // ========================================================================= // - // The stock @atproto/oauth-provider welcome page (Authenticate / Create - // new account / Sign in / Cancel) appears whenever upstream ends up with - // a device that has zero bound accounts — due to partial cookie pairs, - // stale pairs, fixation-race device deletions, or the migration-005 1h - // TTL purge of remember=0 bindings. ePDS must never surface that page; - // users should always land on the email/OTP form or the enriched + // The stock @atproto/oauth-provider has two authentication UIs that ePDS + // must never surface: + // + // 1. Welcome page (Authenticate / Create new account / Sign in / Cancel) — + // rendered when upstream ends up with a device that has zero bound + // accounts (partial cookie pairs, stale pairs, fixation-race device + // deletions, or the migration-005 1h TTL purge of remember=0 bindings). + // + // 2. Sign-in-view (handle + password form) — rendered when bindings exist + // but every binding upstream considers has loginRequired: true (forced + // `prompt=login`, all bindings older than authenticationMaxAge, or + // login_hint pre-selecting an individually stale binding). + // + // ePDS users should always land on the email/OTP form or the enriched // account picker. See docs/design/session-reuse-bugs.md. // // The guard intercepts /oauth/authorize and /account* before upstream's - // own middleware, checks for a valid cookie pair and non-empty bindings, - // and bounces to auth-service with stale cookies cleared when either - // check fails. All other requests pass through unchanged. + // own middleware and bounces to auth-service with stale cookies cleared + // whenever upstream would render either UI. All other requests pass + // through unchanged. - const welcomePageGuardMiddleware = createWelcomePageGuard({ + const authUiGuardMiddleware = createAuthUiGuard({ authHostname, provider: provider ?? null, cookieDomain, logger, }) - pds.app.use(welcomePageGuardMiddleware) + pds.app.use(authUiGuardMiddleware) // Fail closed: the guard has to run BEFORE upstream's OAuth / account - // middleware, otherwise it can never intercept the stock welcome - // page. If the Express `_router.stack` we rely on isn't exposed - // (Express 5, future pds-core swap), refuse to start rather than - // silently run the service with the guard defeated — the whole - // security value of this PR depends on the splice succeeding. + // middleware, otherwise it can never intercept the stock UIs. If the + // Express `_router.stack` we rely on isn't exposed (Express 5, future + // pds-core swap), refuse to start rather than silently run the service + // with the guard defeated — the whole security value of this guard + // depends on the splice succeeding. if (!stack) { throw new Error( - 'Welcome-page guard install failed: Express _router.stack is unavailable — refusing to start pds-core with an inert guard', + 'Auth-UI guard install failed: Express _router.stack is unavailable — refusing to start pds-core with an inert guard', ) } const guardLayer = stack.pop() if (!guardLayer) { throw new Error( - 'Welcome-page guard install failed: middleware layer missing from stack after pop', + 'Auth-UI guard install failed: middleware layer missing from stack after pop', ) } let guardIdx = 0 @@ -722,7 +730,7 @@ async function main() { } } stack.splice(guardIdx, 0, guardLayer) - logger.info('Welcome-page guard installed') + logger.info('Auth-UI guard installed') // ========================================================================= // CSS injection for trusted OAuth clients diff --git a/packages/pds-core/src/lib/device-accounts.ts b/packages/pds-core/src/lib/device-accounts.ts index c941783e..e3a4d553 100644 --- a/packages/pds-core/src/lib/device-accounts.ts +++ b/packages/pds-core/src/lib/device-accounts.ts @@ -1,5 +1,5 @@ /** - * Device-bound account lookup shared between welcome-page-guard and the + * Device-bound account lookup shared between auth-ui-guard and the * /_internal/device-accounts endpoint. * * Validates a (dev-id, ses-id) cookie pair against the live device row, From 3e57080b4315c60e32a18b6e994b952a9508b559 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 22:07:32 +0000 Subject: [PATCH 10/11] fix(pds-core): strip prompt=login from stored PAR after a successful OTP cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manual repro on the PR branch surfaced a regression that the row 5 e2e didn't catch. With the new auth-ui-guard in place, prompt=login flows were stuck in an infinite OTP loop: 1. Demo posts PAR with prompt=login. 2. Auth-service shouldReuseSession returns false (sees prompt=login), serves OTP form. User completes OTP. 3. /auth/complete signs an epds-callback URL. 4. epds-callback authenticates the user, upserts the device-account binding, redirects to pds-core /oauth/authorize?request_uri=... 5. The auth-ui-guard reads the stored PAR. parameters.prompt is STILL 'login' — it was never consumed. The guard bounces back to auth-service. 6. Auth-service serves OTP again. 7. Goto 5. The user-visible symptom: complete OTP, get a fresh OTP form again, and again, forever. The original e2e for row 5 only asserted "after the bounce, the browser is on the auth-service email-and-OTP form" — which is still satisfied at every iteration of the loop. Fix: epds-callback already mutates the stored PAR in Step 6 to inject login_hint after the user is freshly authenticated. Strip prompt='login' at the same hop. Other prompt values ('consent', 'select_account', etc.) stay untouched — only 'login' is loop-forming. By the time the mutation happens, the user IS freshly authenticated; the prompt's contract is satisfied. The mutation is gated by epds-callback's existing HMAC-signed callback chain (auth-service signs after server-side OTP verification, pds-core verifies the signature before any account ops). The same gate already protects the login_hint injection — we're piggybacking on it. Test changes: - features/session-reuse-bugs.feature: rows 5/6/9 now drive the full recovery sequence (post-bounce OTP cycle) and assert the user reaches the demo's signed-in welcome page. Drops the redundant "no upstream password field" follow-up — landing on the welcome page proves the user got there, full stop. - Renamed shared step "the browser is redirected back to the demo client with a valid session" → "the demo client's welcome page confirms the user is signed in" so the Gherkin matches what the impl actually verifies (URL is /welcome AND body contains "You are signed in." AND session_id cookie is set). Three feature files touched. The old name described the redirect but not the positive landing-page check that was already present in the implementation. - Removed the "no upstream password field is rendered anywhere on the page" step impl from session-reuse-bugs.steps.ts; no longer referenced. All e2e session-reuse scenarios pass; 828 unit tests pass; format, lint, typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- e2e/step-definitions/auth.steps.ts | 2 +- .../session-reuse-bugs.steps.ts | 17 --------- features/account-recovery.feature | 2 +- features/passwordless-authentication.feature | 4 +- features/session-reuse-bugs.feature | 29 ++++++++++++--- packages/pds-core/src/index.ts | 37 ++++++++++++++----- 6 files changed, 54 insertions(+), 37 deletions(-) diff --git a/e2e/step-definitions/auth.steps.ts b/e2e/step-definitions/auth.steps.ts index 610aff3c..856d7c88 100644 --- a/e2e/step-definitions/auth.steps.ts +++ b/e2e/step-definitions/auth.steps.ts @@ -233,7 +233,7 @@ Then( ) Then( - 'the browser is redirected back to the demo client with a valid session', + "the demo client's welcome page confirms the user is signed in", async function (this: EpdsWorld) { await assertDemoClientSession(this) }, diff --git a/e2e/step-definitions/session-reuse-bugs.steps.ts b/e2e/step-definitions/session-reuse-bugs.steps.ts index fd0e3109..531fb078 100644 --- a/e2e/step-definitions/session-reuse-bugs.steps.ts +++ b/e2e/step-definitions/session-reuse-bugs.steps.ts @@ -625,23 +625,6 @@ When( }, ) -Then( - 'no upstream password field is rendered anywhere on the page', - async function (this: EpdsWorld) { - // Negative assertion: ePDS accounts are passwordless, so a password - // input on any screen we land on is a leak. Upstream's sign-in-view - // renders ; auth-service's OTP form does not. - // Counting password inputs across the whole document keeps the - // assertion robust against future structural changes. - const page = getPage(this) - const passwordCount = await page.locator('input[type="password"]').count() - expect( - passwordCount, - `Found ${passwordCount} password input(s) on ${page.url()} — ePDS is passwordless, this is the upstream sign-in-view leak`, - ).toBe(0) - }, -) - // Backdate `account_device.updated_at` past upstream's authenticationMaxAge // (7d) via a pds-core /_internal/test hook gated on EPDS_TEST_HOOKS=1. The // hook accepts a target DID and optional deviceId; omitting deviceId diff --git a/features/account-recovery.feature b/features/account-recovery.feature index ed3edb1f..4ded73d1 100644 --- a/features/account-recovery.feature +++ b/features/account-recovery.feature @@ -30,7 +30,7 @@ Feature: Account recovery via backup emails And the user enters the backup email on the recovery page Then an OTP email arrives in the mail trap for the backup email When the user enters the recovery OTP - Then the browser is redirected back to the demo client with a valid session + Then the demo client's welcome page confirms the user is signed in Scenario: Recovery with non-existent email shows same UI (anti-enumeration) Given the demo client initiates OAuth with the test email as login_hint diff --git a/features/passwordless-authentication.feature b/features/passwordless-authentication.feature index d85e53bb..995f74e6 100644 --- a/features/passwordless-authentication.feature +++ b/features/passwordless-authentication.feature @@ -34,7 +34,7 @@ Feature: Passwordless authentication via email OTP Then an OTP email arrives in the mail trap And the email subject contains "ePDS Demo" When the user enters the OTP code - Then the browser is redirected back to the demo client with a valid session + Then the demo client's welcome page confirms the user is signed in @email Scenario: Returning user who has already approved skips consent @@ -44,7 +44,7 @@ Feature: Passwordless authentication via email OTP Then an OTP email arrives in the mail trap And the email subject contains "ePDS Demo" When the user enters the OTP code - Then the browser is redirected back to the demo client with a valid session + Then the demo client's welcome page confirms the user is signed in # --- Session reuse across OAuth clients (HYPER-268) --- # diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index 059e9eda..8981d33b 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -184,8 +184,13 @@ Feature: Welcome-page guard suppresses upstream's authentication UI And the user enters the test email on the login page Then an OTP email arrives in the mail trap When the user enters the OTP code - Then the browser lands on the auth-service email-and-OTP form - And no upstream password field is rendered anywhere on the page + # The end-to-end recovery: after the (single) OTP cycle the user + # lands on /welcome. Earlier iterations of the fix bounced the + # post-OTP /oauth/authorize hop AGAIN because the stored PAR still + # carried prompt=login, looping forever. The epds-callback hop + # strips that field after a successful OTP, so there is exactly + # one OTP cycle, one bounce-suppression, then /welcome. + Then the demo client's welcome page confirms the user is signed in # Reproducing row 6 needs server-side white-box access to backdate # `account_device.updated_at` past upstream's authenticationMaxAge @@ -195,8 +200,15 @@ Feature: Welcome-page guard suppresses upstream's authentication UI Scenario: Row 6 — every binding's auth age is older than 7 days Given the device's account_device row has been backdated past 7 days When the demo client starts a new OAuth flow - Then the browser lands on the auth-service email-and-OTP form - And no upstream password field is rendered anywhere on the page + # End-to-end recovery: the auth-ui-guard bounces the initial + # /oauth/authorize hop to auth-service, the user completes a fresh + # OTP cycle, and lands on /welcome. The "no password field" + # assertion runs at /welcome — it would catch a regression where + # the user lands on the upstream sign-in-view as the FINAL state. + And the user enters the test email on the login page + Then an OTP email arrives in the mail trap + When the user enters the OTP code + Then the demo client's welcome page confirms the user is signed in # Row 9 needs two bindings on the same device — one stale, one fresh — # plus a login_hint that resolves to the stale one. Built on the same @@ -209,5 +221,10 @@ Feature: Welcome-page guard suppresses upstream's authentication UI And the test user's account_device row has been backdated past 7 days And the other user's account_device row is fresh When the demo client starts a new OAuth flow with the test user's handle as a PAR-body login_hint - Then the browser lands on the auth-service email-and-OTP form - And no upstream password field is rendered anywhere on the page + # End-to-end recovery: the auth-ui-guard bounces to auth-service + # with the email already resolved from the PAR-body login_hint, so + # auth-service serves the OTP step directly (no email entry). After + # the OTP cycle the user lands on /welcome. + Then an OTP email arrives in the mail trap + When the user enters the OTP code + Then the demo client's welcome page confirms the user is signed in diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index b9f96f35..36467b53 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -514,13 +514,25 @@ async function main() { } } - // Step 6: Set login_hint in the stored PAR parameters so the stock - // authorize UI auto-selects this account's session and skips account - // selection (going straight to consent or auto-approve). - // The oauth-provider UI checks `selected` which is true when - // login_hint matches the account AND prompt !== 'select_account'. - // prompt is already 'consent' (forced by the provider for - // unauthenticated clients). + // Step 6: Mutate the stored PAR parameters before redirecting to the + // stock /oauth/authorize endpoint: + // + // - Set `login_hint` to the freshly-authenticated DID so the stock + // authorize UI auto-selects this account's session and skips + // account selection. The oauth-provider UI checks `selected`, + // which is true when login_hint matches the account AND + // prompt !== 'select_account'. (prompt is already 'consent', + // forced by the provider for unauthenticated clients.) + // + // - Strip `prompt: 'login'` if present. The auth-ui-guard at + // /oauth/authorize bounces requests whose stored PAR carries + // prompt=login (row 5 of the failure-mode taxonomy), so leaving + // it set after a successful OTP cycle would loop forever: + // authenticate → bounce → authenticate → bounce. By the time + // this hop fires, the user IS freshly authenticated; the + // prompt's contract is satisfied. Other prompt values + // ('consent', 'select_account', etc.) stay untouched — only + // 'login' is loop-forming. if (did) { const REQUEST_URI_PREFIX = 'urn:ietf:params:oauth:request_uri:' const requestId = decodeURIComponent( @@ -530,9 +542,14 @@ async function main() { const store = (provider.requestManager as any).store const storedRequest = await store.readRequest(requestId) if (storedRequest?.parameters) { - await store.updateRequest(requestId, { - parameters: { ...storedRequest.parameters, login_hint: did }, - }) + const nextParams: Record = { + ...storedRequest.parameters, + login_hint: did, + } + if (nextParams.prompt === 'login') { + delete nextParams.prompt + } + await store.updateRequest(requestId, { parameters: nextParams }) } } From 14e5033026ecebb21acdc23211d96c755cedb1e0 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 30 Apr 2026 23:04:19 +0000 Subject: [PATCH 11/11] fix(pds-core): tokenise prompt, opt-out test hooks by default, drop private-taxonomy refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes from PR #129 review feedback: 1. **OIDC prompt is space-delimited.** The exact-string check `params?.prompt === 'login'` (and the matching strip in epds-callback) missed valid multi-token values like `prompt="login consent"`. A third-party client sending such a prompt could bypass the auth-ui-guard's bounce condition, reopening the leak this PR set out to close. Per OIDC Core 1.0 §3.1.2.1 prompt is a space-delimited list, so the fix is to tokenise. Adds `parsePromptTokens()` and `promptHasLogin()` helpers to `auth-ui-guard.ts`, used by: - the guard's bounce condition (`promptHasLogin(params?.prompt)`); - epds-callback's strip after a successful OTP, which now removes only the `login` token and preserves the rest (so a hypothetical `prompt="login consent"` becomes `prompt="consent"` rather than silently dropping consent). 2. **Test hooks default off.** `docker-compose.yml` previously set `EPDS_TEST_HOOKS=${EPDS_TEST_HOOKS:-1}`, defaulting test-only mutation routes ON in any non-production environment. The production failsafe (`NODE_ENV=production` throws at startup) is still there, but the wider default-on stance violated security-by-default. Both core and auth services now default to `:-0`; opt in for e2e runs by setting `EPDS_TEST_HOOKS=1` in `.env` or the shell. Comments in `docker-compose.yml` and `.env.example` updated. 3. **Drop private-taxonomy references.** Comments and test labels used "row 5/6/9" / "rows 1–4" referring to the failure-mode taxonomy in #131. Those numbers mean nothing to anyone reading the code without that issue open. Replaced with descriptive labels — e.g. `it('bounces when the stored PAR forces re-authentication via prompt=login', ...)` — so test output and in-source comments are self-explanatory. The Gherkin scenarios in `features/session-reuse-bugs.feature` are renamed similarly (e.g. `Scenario: Forced re-authentication via prompt=login`). Adds 10 new unit tests (838 total, was 828) covering the tokeniser edge cases and a multi-token prompt bounce. Whole session-reuse e2e profile passes (19/19). Includes the previously-missing changeset: .changeset/sign-in-no-longer-dead-ends-on-password-form.md (End-user-only audience; the bug fix is transparent to client app developers and operators.) Closes the two CodeRabbit-flagged threads on PR #129. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...in-no-longer-dead-ends-on-password-form.md | 9 ++ .env.example | 17 +-- docker-compose.yml | 24 ++-- .../session-reuse-bugs.steps.ts | 39 ++++--- features/session-reuse-bugs.feature | 87 +++++++------- .../src/__tests__/auth-ui-guard.test.ts | 108 +++++++++++++++--- packages/pds-core/src/auth-ui-guard.ts | 38 ++++-- packages/pds-core/src/index.ts | 36 ++++-- packages/pds-core/src/lib/test-hooks.ts | 6 +- 9 files changed, 243 insertions(+), 121 deletions(-) create mode 100644 .changeset/sign-in-no-longer-dead-ends-on-password-form.md diff --git a/.changeset/sign-in-no-longer-dead-ends-on-password-form.md b/.changeset/sign-in-no-longer-dead-ends-on-password-form.md new file mode 100644 index 00000000..c45bee9c --- /dev/null +++ b/.changeset/sign-in-no-longer-dead-ends-on-password-form.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Sign-in no longer hits a dead-end on the password form + +**Affects:** End users + +**End users:** if you saw a "handle and password" form during sign-in with no way to enter a code, that path is gone. The email-code form will be shown instead, and after entering the code you'll be signed in normally. diff --git a/.env.example b/.env.example index 80805169..e03a37f0 100644 --- a/.env.example +++ b/.env.example @@ -69,14 +69,15 @@ PDS_ADMIN_PASSWORD= # Set to 'development' for dev mode (disables secure cookies, etc.) # NODE_ENV=development -# Enable test-only HTTP hooks on auth-service used by the e2e suite -# (e.g. POST /_internal/test/expire-otp, which backdates the verification -# row's expiresAt so the "OTP expired after 10 minutes" scenario can run -# in seconds). Set to 1 for the Railway pr-base preview env. Leave unset -# for production — the router throws at startup if EPDS_TEST_HOOKS=1 and -# NODE_ENV=production. docker-compose.yml supplies its own default of 1 -# via `${EPDS_TEST_HOOKS:-1}`, so this stays commented in the template -# to keep non-compose deployments off by default. +# Enable test-only HTTP hooks on auth-service and pds-core used by the +# e2e suite (e.g. POST /_internal/test/expire-otp, which backdates the +# verification row's expiresAt so the "OTP expired after 10 minutes" +# scenario can run in seconds). Set to 1 for the Railway pr-base preview +# env AND for any local docker-compose run that drives the e2e suite — +# docker-compose.yml defaults it OFF (`${EPDS_TEST_HOOKS:-0}`) so the +# stack matches a production deployment unless you explicitly opt in. +# Leave unset for production — the router throws at startup if +# EPDS_TEST_HOOKS=1 and NODE_ENV=production. # EPDS_TEST_HOOKS=1 # ============================================================================ diff --git a/docker-compose.yml b/docker-compose.yml index 6d2e8a6c..b9f2b747 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -8,13 +8,14 @@ services: env_file: .env environment: - PDS_PORT=3000 - # Enable /_internal/test/* hooks used by the e2e suite (e.g. + # Optional /_internal/test/* hooks used by the e2e suite (e.g. # backdating account_device.updatedAt past upstream's 7-day - # authenticationMaxAge so checkLoginRequired returns true). Mounted - # only here and on the Railway pr-base preview env; production - # deployments leave it unset. The router throws at startup if - # NODE_ENV=production. - - EPDS_TEST_HOOKS=${EPDS_TEST_HOOKS:-1} + # authenticationMaxAge so checkLoginRequired returns true). + # Defaults OFF so the local docker-compose stack matches a + # production deployment by default; opt in for e2e runs by setting + # EPDS_TEST_HOOKS=1 in your .env or shell. The router additionally + # throws at startup if NODE_ENV=production, regardless of this var. + - EPDS_TEST_HOOKS=${EPDS_TEST_HOOKS:-0} volumes: - pds-data:/data restart: unless-stopped @@ -34,11 +35,12 @@ services: env_file: .env environment: - AUTH_PORT=3001 - # Enable /_internal/test/* hooks used by the e2e suite (e.g. forcing - # OTP expiry without a 10-minute wait). Mounted only here and on the - # Railway pr-base preview env; production deployments leave it unset. - # The router throws at startup if NODE_ENV=production. - - EPDS_TEST_HOOKS=${EPDS_TEST_HOOKS:-1} + # Optional /_internal/test/* hooks used by the e2e suite (e.g. forcing + # OTP expiry without a 10-minute wait). Defaults OFF — opt in for e2e + # runs by setting EPDS_TEST_HOOKS=1 in your .env or shell. The router + # additionally throws at startup if NODE_ENV=production, regardless + # of this var. + - EPDS_TEST_HOOKS=${EPDS_TEST_HOOKS:-0} volumes: - auth-data:/data restart: unless-stopped diff --git a/e2e/step-definitions/session-reuse-bugs.steps.ts b/e2e/step-definitions/session-reuse-bugs.steps.ts index 531fb078..23cc5bdf 100644 --- a/e2e/step-definitions/session-reuse-bugs.steps.ts +++ b/e2e/step-definitions/session-reuse-bugs.steps.ts @@ -1,16 +1,16 @@ /** * Step definitions for features/session-reuse-bugs.feature — the - * welcome-page guard scenarios. See docs/design/session-reuse-bugs.md for - * the failure-mode taxonomy. + * auth-ui-guard scenarios. See docs/design/session-reuse-bugs.md for the + * full failure-mode taxonomy. * * These steps exercise the pre-route guard in pds-core that intercepts * /oauth/authorize and /account* before upstream's signin handler can * render its authentication UIs (stock welcome page or sign-in-view). * The guard bounces to auth-service when: - * - the dev-id/ses-id cookie pair is missing or malformed (rows 1–3) - * - the cookie pair resolves to a device with zero bound accounts (row 4) - * - the stored PAR carries `prompt=login` (row 5) - * - every relevant binding's auth age exceeds 7 days (rows 6 / 9) + * - the dev-id/ses-id cookie pair is missing or malformed + * - the cookie pair resolves to a device with zero bound accounts + * - the stored PAR forces re-authentication (prompt contains 'login') + * - every relevant binding's auth age exceeds 7 days * * All scenarios require a docker-compose topology where auth-service is a * sibling subdomain of pds-core so device-session cookies are domain-scoped @@ -208,9 +208,9 @@ When( // /oauth/authorize redirect URL — this matches the third-party // OAuth-client pattern that drives upstream's matchesHint logic against // `parameters.login_hint` from the stored PAR. Without this, upstream's - // request-manager-loaded parameters carry no hint and the guard's row 9 - // bounce condition can never fire (because filterCandidateBindings sees - // every binding as a candidate, not just the stale one). + // request-manager-loaded parameters carry no hint and the guard's + // hint-narrowed bounce condition can never fire — filterCandidateBindings + // would see every binding as a candidate, not just the stale one. const page = getPage(this) const url = new URL('/api/oauth/login', testEnv.demoUrl) url.searchParams.set('login_hint', this.userHandle) @@ -600,12 +600,12 @@ Given( ) // --------------------------------------------------------------------------- -// Sign-in-view leaks (rows 5/6/9). Distinct from the welcome-page leaks -// above: cookies + bindings are valid, but every binding upstream would -// consider has `loginRequired: true`, so upstream falls through to its -// sign-in-view (handle + password form). ePDS accounts are passwordless, so -// any path into that form is unusable. The guard must bounce these to -// auth-service for a fresh OTP round. +// Sign-in-view leaks. Distinct from the welcome-page leaks above: cookies +// and bindings are valid, but every binding upstream would consider has +// `loginRequired: true`, so upstream falls through to its sign-in-view +// (handle + password form). ePDS accounts are passwordless, so any path +// into that form is unusable. The guard must bounce these to auth-service +// for a fresh OTP round. // --------------------------------------------------------------------------- When( @@ -617,7 +617,8 @@ When( // Cookies are preserved (the device must already be bound from the // Background) so once the auth-service-side OTP completes and the // pds-core epds-callback redirects back to /oauth/authorize, the - // stored PAR still carries `prompt=login` — the row-5 trigger. + // stored PAR still carries `prompt=login` — the trigger this scenario + // exercises. const page = getPage(this) const url = new URL('/api/oauth/login', testEnv.demoUrl) url.searchParams.set('prompt', 'login') @@ -628,9 +629,9 @@ When( // Backdate `account_device.updated_at` past upstream's authenticationMaxAge // (7d) via a pds-core /_internal/test hook gated on EPDS_TEST_HOOKS=1. The // hook accepts a target DID and optional deviceId; omitting deviceId -// backdates every device row for that DID (sufficient for row 6's -// single-account device, while row 9 passes both did + deviceId for -// surgical targeting). +// backdates every device row for that DID (sufficient when the device has +// only one binding; the multi-account-device scenario passes both did and +// deviceId for surgical targeting). async function expireDeviceAccount(opts: { did: string deviceId?: string diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index 8981d33b..e8410010 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -145,78 +145,77 @@ Feature: Welcome-page guard suppresses upstream's authentication UI And the OTP form will submit the other user's email # --------------------------------------------------------------------------- - # Sign-in-view leaks (rows 5/6/9 of the failure-mode taxonomy). Distinct - # from the welcome-page leaks above: here cookies and bindings are valid, - # but every binding upstream would consider has `loginRequired: true`, so - # upstream's only remaining path is to render its sign-in-view (handle + - # password form). The chooser may render first as an intermediate step, - # but every account on it leads to sign-in-view on click — equally a leak. - # ePDS accounts are passwordless, so any path into that form is unusable. - # The guard must bounce these to auth-service for a fresh OTP round. + # Sign-in-view leaks. Distinct from the welcome-page leaks above: here + # cookies and bindings are valid, but every binding upstream would consider + # has `loginRequired: true`, so upstream's only remaining path is to render + # its sign-in-view (handle + password form). The chooser may render first + # as an intermediate step, but every account on it leads to sign-in-view on + # click — equally a leak. ePDS accounts are passwordless, so any path into + # that form is unusable. The guard must bounce these to auth-service for a + # fresh OTP round. # # Three independent triggers force every binding into loginRequired: - # Row 5 — stored PAR `parameters.prompt === 'login'` - # Row 6 — every binding's `account_device.updated_at` is older than - # upstream's authenticationMaxAge (7 days) - # Row 9 — `login_hint` resolves to a binding that's individually stale - # even though other bindings on the device are fresh; upstream - # pre-selects the hinted account and clicking it lands on - # sign-in-view + # - the stored PAR contains `prompt=login` (forced re-authentication); + # - every binding's `account_device.updated_at` is older than + # upstream's authenticationMaxAge (7 days); + # - `login_hint` resolves to a binding that's individually stale even + # though other bindings on the device are fresh, and upstream + # pre-selects the hinted account so clicking lands on sign-in-view. # --------------------------------------------------------------------------- - Scenario: Row 5 — prompt=login forces every binding loginRequired + Scenario: Forced re-authentication via prompt=login # The demo's "Force re-authentication" checkbox sets prompt=login on # both the auth-service redirect query AND the PAR body. Auth-service's # `shouldReuseSession` honours the query and serves the OTP form rather # than redirecting to pds-core (session-reuse.ts:158, isForceLoginPrompt). # The user completes OTP, and pds-core's epds-callback redirects to # pds-core's own /oauth/authorize?request_uri=... with the now-fresh - # device cookies. At THAT hop the welcome-page guard fires: + # device cookies. At THAT hop the auth-ui-guard fires: # - cookie pair is valid (just set by the callback) # - the device has one binding (the user just authenticated) # - the stored PAR `parameters.prompt` is still 'login' (the demo also # set it in the PAR body) - # Today the guard passes through; upstream's authorize() reads the - # stored PAR, marks the only session loginRequired, and the frontend - # renders sign-in-view. The guard must instead bounce back to - # auth-service for another OTP round. + # Without the fix the guard would pass through; upstream's authorize() + # reads the stored PAR, marks the only session loginRequired, and the + # frontend renders sign-in-view. The guard bounces back to auth-service + # and the epds-callback hop strips the `login` token from the stored + # PAR so the next /oauth/authorize hop can complete normally. When the demo client starts a new OAuth flow with prompt=login And the user enters the test email on the login page Then an OTP email arrives in the mail trap When the user enters the OTP code - # The end-to-end recovery: after the (single) OTP cycle the user - # lands on /welcome. Earlier iterations of the fix bounced the - # post-OTP /oauth/authorize hop AGAIN because the stored PAR still - # carried prompt=login, looping forever. The epds-callback hop - # strips that field after a successful OTP, so there is exactly - # one OTP cycle, one bounce-suppression, then /welcome. + # The end-to-end recovery: after the (single) OTP cycle the user lands + # on /welcome. Earlier iterations of the fix re-bounced the post-OTP + # /oauth/authorize hop because the stored PAR still carried + # prompt=login, looping forever. The epds-callback hop strips that + # token after a successful OTP, so there is exactly one OTP cycle, + # one bounce-suppression, then /welcome. Then the demo client's welcome page confirms the user is signed in - # Reproducing row 6 needs server-side white-box access to backdate - # `account_device.updated_at` past upstream's authenticationMaxAge - # (7 days). A pds-core /_internal/test/expire-device-account hook - # provides this, gated on EPDS_TEST_HOOKS=1 && NODE_ENV !== 'production' - # (mirroring auth-service's expire-otp / expire-auth-flow hooks). - Scenario: Row 6 — every binding's auth age is older than 7 days + # Reproducing the all-bindings-stale case needs server-side white-box + # access to backdate `account_device.updated_at` past upstream's + # authenticationMaxAge (7 days). A pds-core + # /_internal/test/expire-device-account hook provides this, gated on + # EPDS_TEST_HOOKS=1 && NODE_ENV !== 'production' (mirroring auth-service's + # expire-otp / expire-auth-flow hooks). + Scenario: Every binding's auth age exceeds 7 days Given the device's account_device row has been backdated past 7 days When the demo client starts a new OAuth flow # End-to-end recovery: the auth-ui-guard bounces the initial - # /oauth/authorize hop to auth-service, the user completes a fresh - # OTP cycle, and lands on /welcome. The "no password field" - # assertion runs at /welcome — it would catch a regression where - # the user lands on the upstream sign-in-view as the FINAL state. + # /oauth/authorize hop to auth-service, the user completes a fresh OTP + # cycle, and lands on /welcome. And the user enters the test email on the login page Then an OTP email arrives in the mail trap When the user enters the OTP code Then the demo client's welcome page confirms the user is signed in - # Row 9 needs two bindings on the same device — one stale, one fresh — - # plus a login_hint that resolves to the stale one. Built on the same - # /_internal/test/expire-device-account hook used for row 6, plus a - # second OAuth sign-in driven within the SAME browser context (so the - # second account's upsertDeviceAccount binds to the existing dev-id - # rather than creating a fresh device). - Scenario: Row 9 — login_hint resolves to a stale binding on a multi-account device + # The hint-narrowed case needs two bindings on the same device — one + # stale, one fresh — plus a login_hint that resolves to the stale one. + # Built on the same /_internal/test/expire-device-account hook used by + # the all-stale scenario above, plus a second OAuth sign-in driven within + # the SAME browser context (so the second account's upsertDeviceAccount + # binds to the existing dev-id rather than creating a fresh device). + Scenario: login_hint narrows to a stale binding on a multi-account device Given the device has two bound accounts And the test user's account_device row has been backdated past 7 days And the other user's account_device row is fresh diff --git a/packages/pds-core/src/__tests__/auth-ui-guard.test.ts b/packages/pds-core/src/__tests__/auth-ui-guard.test.ts index 7e9b5428..df213699 100644 --- a/packages/pds-core/src/__tests__/auth-ui-guard.test.ts +++ b/packages/pds-core/src/__tests__/auth-ui-guard.test.ts @@ -6,6 +6,8 @@ import { createAuthUiGuard, isGuardedPath, parseDeviceCookies, + parsePromptTokens, + promptHasLogin, } from '../auth-ui-guard.js' const VALID_DEV = 'dev-0123456789abcdef0123456789abcdef' @@ -196,6 +198,70 @@ function makeResStub(): Response & { _calls: string[][] } { return res as any } +describe('parsePromptTokens', () => { + it('returns an empty Set for non-string input', () => { + expect(parsePromptTokens(undefined).size).toBe(0) + expect(parsePromptTokens(null).size).toBe(0) + expect(parsePromptTokens(123).size).toBe(0) + expect(parsePromptTokens({}).size).toBe(0) + }) + + it('returns an empty Set for an empty / whitespace-only string', () => { + expect(parsePromptTokens('').size).toBe(0) + expect(parsePromptTokens(' ').size).toBe(0) + }) + + it('splits a single-token value', () => { + expect([...parsePromptTokens('login')]).toEqual(['login']) + }) + + it('splits a space-delimited multi-token value', () => { + // Per OIDC Core 1.0 §3.1.2.1, prompt is space-delimited. The order of + // tokens within a Set is insertion order, so an array roundtrip is + // stable. + expect([...parsePromptTokens('login consent')]).toEqual([ + 'login', + 'consent', + ]) + }) + + it('collapses repeated whitespace and ignores empty segments', () => { + expect([...parsePromptTokens(' login consent ')]).toEqual([ + 'login', + 'consent', + ]) + }) +}) + +describe('promptHasLogin', () => { + it('is false for absent / non-string / unrelated prompt values', () => { + expect(promptHasLogin(undefined)).toBe(false) + expect(promptHasLogin(null)).toBe(false) + expect(promptHasLogin('')).toBe(false) + expect(promptHasLogin('consent')).toBe(false) + expect(promptHasLogin('select_account')).toBe(false) + }) + + it('is true when login is the only token', () => { + expect(promptHasLogin('login')).toBe(true) + }) + + it('is true when login appears alongside other tokens (in any order)', () => { + expect(promptHasLogin('login consent')).toBe(true) + expect(promptHasLogin('consent login')).toBe(true) + expect(promptHasLogin('login select_account consent')).toBe(true) + }) + + it('is false for substrings that do not match the login token exactly', () => { + // Defence against "login" appearing inside a longer token. OIDC has no + // such tokens defined today, but the matcher should still be exact — + // future spec extensions could add e.g. `login_required` and we want + // to be wrong-by-design rather than wrong-by-coincidence. + expect(promptHasLogin('logincreate')).toBe(false) + expect(promptHasLogin('relogin')).toBe(false) + }) +}) + describe('appendCookieClearHeaders', () => { it('clears dev-id and ses-id (plus :hash sidecars) host-only when no cookie domain given', () => { const res = makeResStub() @@ -257,7 +323,8 @@ function makeProvider(opts: { id: string, ) => Promise<{ parameters?: Record } | null> // Per-binding loginRequired predicate. Default: false (everything fresh). - // Tests for rows 6/9 supply a custom predicate. + // Sign-in-view leak tests supply a custom predicate to mark specific + // bindings stale. checkLoginRequired?: (binding: unknown) => boolean }): FakeProvider { const ses = opts.sessionId === undefined ? VALID_SES : opts.sessionId @@ -703,12 +770,11 @@ describe('createAuthUiGuard', () => { }) // --------------------------------------------------------------------------- - // Sign-in-view leak coverage (rows 5 / 6 / 9 of the failure-mode taxonomy - // in features/session-reuse-bugs.feature). Every test below shares the - // same wiring — fresh cookies + non-empty bindings + a request_uri-bearing - // URL — and varies only in the stored PAR `prompt` / `login_hint` values - // and which bindings have loginRequired=true. The signinViewCase helper - // captures that wiring; each test just supplies the inputs and assertion. + // Sign-in-view leak coverage. Every test below shares the same wiring — + // fresh cookies + non-empty bindings + a request_uri-bearing URL — and + // varies only in the stored PAR `prompt` / `login_hint` values and which + // bindings have loginRequired=true. The signinViewCase helper captures + // that wiring; each test just supplies the inputs and assertion. // --------------------------------------------------------------------------- // Helper: build a binding fixture good enough for the guard's matchesHint @@ -720,7 +786,7 @@ describe('createAuthUiGuard', () => { } // urn-prefixed request_uri so loadStoredPar actually attempts the read - // (anything else makes it short-circuit and skip the row-5/9 logic). + // (anything else makes it short-circuit and skip the prompt/hint logic). const REQUEST_URI = 'urn:ietf:params:oauth:request_uri:req-abc' type SigninViewCaseOpts = Parameters[0] & { @@ -771,7 +837,7 @@ describe('createAuthUiGuard', () => { expect(res.status).not.toHaveBeenCalled() } - it('row 5: bounces when stored PAR has prompt === "login"', async () => { + it('bounces when the stored PAR forces re-authentication via prompt=login', async () => { const { provider, res } = await signinViewCase({ bindings: () => Promise.resolve([binding('did:plc:a', 'a.example')]), // Even though the binding itself is fresh (checkLoginRequired @@ -783,7 +849,19 @@ describe('createAuthUiGuard', () => { expect(provider.checkLoginRequired).not.toHaveBeenCalled() }) - it('row 6: bounces when every binding is loginRequired and there is no hint', async () => { + it('bounces when login appears alongside other prompt tokens', async () => { + // Per OIDC Core §3.1.2.1, prompt is space-delimited. A third-party + // client sending prompt="login consent" must trip the same bounce as + // a bare prompt="login" — the earlier exact-string check missed this. + const { res } = await signinViewCase({ + bindings: () => Promise.resolve([binding('did:plc:a', 'a.example')]), + readRequest: () => + Promise.resolve({ parameters: { prompt: 'login consent' } }), + }) + expectBounce(res) + }) + + it('bounces when every binding is loginRequired and there is no hint', async () => { const { res } = await signinViewCase({ bindings: () => Promise.resolve([ @@ -796,9 +874,9 @@ describe('createAuthUiGuard', () => { expectBounce(res) }) - // Picker for the row-9-shaped fixture: hint narrows to one binding, - // others stay fresh. Returns a checkLoginRequired predicate that - // marks only `did:plc:stale` as loginRequired. + // Fixture for the hint-narrowed cases: a device with two bindings, + // one stale and one fresh. The checkLoginRequired predicate marks + // only `did:plc:stale` as loginRequired. const onlyStaleIsLoginRequired = (b: unknown): boolean => (b as { account: { sub: string } }).account.sub === 'did:plc:stale' const TWO_BINDINGS = [ @@ -806,7 +884,7 @@ describe('createAuthUiGuard', () => { binding('did:plc:fresh', 'fresh.example'), ] - it('row 9: bounces when login_hint matches the only stale binding', async () => { + it('bounces when login_hint narrows to a stale binding among otherwise-fresh bindings', async () => { const { res } = await signinViewCase({ bindings: () => Promise.resolve(TWO_BINDINGS), readRequest: () => @@ -816,7 +894,7 @@ describe('createAuthUiGuard', () => { expectBounce(res) }) - it('passes through when login_hint matches a fresh binding (row 7)', async () => { + it('passes through when login_hint matches a fresh binding', async () => { // Hint resolves to a single fresh binding → SSO/chooser path is // reachable; the guard must NOT bounce. const { res, next } = await signinViewCase({ diff --git a/packages/pds-core/src/auth-ui-guard.ts b/packages/pds-core/src/auth-ui-guard.ts index 40e7c7a8..6aa1abde 100644 --- a/packages/pds-core/src/auth-ui-guard.ts +++ b/packages/pds-core/src/auth-ui-guard.ts @@ -5,14 +5,15 @@ * * 1. The three-button welcome page ("Authenticate / Create new account / * Sign in / Cancel") — rendered when the device has zero bound - * accounts (rows 1–4 of the failure-mode taxonomy: partial cookie + * accounts (partial cookie * pair, stale pair, migration-005 TTL purge, fixation race, etc.). * * 2. The sign-in-view (handle + password form) — rendered when bindings - * exist but every binding upstream considers has loginRequired: true - * (rows 5/6/9: stored PAR prompt=login; all bindings older than - * authenticationMaxAge; login_hint pre-selecting an individually - * stale binding). + * exist but every binding upstream considers has loginRequired: true. + * Three triggers: the stored PAR has prompt=login; every binding's + * auth age exceeds authenticationMaxAge (default 7d); a login_hint + * pre-selects an individually stale binding among otherwise-fresh + * bindings on the same device. * * Both UIs are unreachable from ePDS by design — every entry point should * either show the enriched chooser (when the device has fresh bound @@ -271,14 +272,14 @@ export function createAuthUiGuard(opts: { // // Read the stored PAR parameters and compute upstream's // candidate-binding set; bounce when every candidate would be - // loginRequired. See features/session-reuse-bugs.feature rows - // 5/6/9 for the externally-reproducible test cases. + // loginRequired. See features/session-reuse-bugs.feature for the + // externally-reproducible scenarios under "Sign-in-view leaks". const params = await loadStoredPar({ provider, requestUrl: req.url, logger, }) - if (params?.prompt === 'login') { + if (promptHasLogin(params?.prompt)) { bounceOrPass() return } @@ -312,7 +313,8 @@ type Binding = DeviceAccount /** Read the stored PAR parameters for the request_uri on the current URL. * Returns null when the URL has no request_uri, when the lookup fails, or * when stored data is shaped unexpectedly. Used to read `prompt` and - * `login_hint` for the row-5/9 bounce decisions. */ + * `login_hint` for the bounce decisions on flows where the stored PAR + * forces re-authentication or hints at a stale binding. */ async function loadStoredPar(opts: { provider: OAuthProvider requestUrl: string @@ -366,3 +368,21 @@ function filterCandidateBindings( ) return matched.length === 1 ? matched : bindings } + +/** Tokenise an OIDC `prompt` parameter value into its space-delimited set. + * Per OpenID Connect Core 1.0 §3.1.2.1, `prompt` is a space-delimited list + * of values (e.g. `"login consent"`), not a single literal — so an exact + * string check would miss `login` when it appears alongside other tokens. + * Returns an empty Set for null/undefined/non-string input. */ +export function parsePromptTokens(value: unknown): Set { + if (typeof value !== 'string') return new Set() + return new Set(value.split(/\s+/).filter(Boolean)) +} + +/** True when the given OIDC prompt value contains the `login` token. Both + * the auth-ui-guard and epds-callback's PAR mutation use this — they must + * agree on what counts as "forced login" so the guard's bounce condition + * and the callback's prompt-strip apply to exactly the same requests. */ +export function promptHasLogin(value: unknown): boolean { + return parsePromptTokens(value).has('login') +} diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index 36467b53..5d1d1f6d 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -62,7 +62,7 @@ import { } from './cookie-domain.js' import { createChooserEnrichmentMiddleware } from './chooser-enrichment.js' import { createUpstreamFaviconMiddleware } from './upstream-favicon.js' -import { createAuthUiGuard } from './auth-ui-guard.js' +import { createAuthUiGuard, parsePromptTokens } from './auth-ui-guard.js' import { loadDeviceAccountEmails } from './lib/device-accounts.js' import { installTestHooks } from './lib/test-hooks.js' @@ -524,15 +524,15 @@ async function main() { // prompt !== 'select_account'. (prompt is already 'consent', // forced by the provider for unauthenticated clients.) // - // - Strip `prompt: 'login'` if present. The auth-ui-guard at - // /oauth/authorize bounces requests whose stored PAR carries - // prompt=login (row 5 of the failure-mode taxonomy), so leaving - // it set after a successful OTP cycle would loop forever: - // authenticate → bounce → authenticate → bounce. By the time - // this hop fires, the user IS freshly authenticated; the - // prompt's contract is satisfied. Other prompt values - // ('consent', 'select_account', etc.) stay untouched — only - // 'login' is loop-forming. + // - Strip the `login` token from `prompt` if present. The + // auth-ui-guard at /oauth/authorize bounces requests whose + // stored PAR carries prompt=login, so leaving it set after a + // successful OTP cycle would loop forever: authenticate → + // bounce → authenticate → bounce. By the time this hop fires, + // the user IS freshly authenticated; the forced-login + // contract is satisfied. Other prompt tokens ('consent', + // 'select_account', etc.) stay untouched — only 'login' is + // loop-forming. if (did) { const REQUEST_URI_PREFIX = 'urn:ietf:params:oauth:request_uri:' const requestId = decodeURIComponent( @@ -546,8 +546,20 @@ async function main() { ...storedRequest.parameters, login_hint: did, } - if (nextParams.prompt === 'login') { - delete nextParams.prompt + // Strip the 'login' token from the prompt parameter, leaving any + // other tokens (e.g. 'consent') intact. Per OIDC Core §3.1.2.1 + // prompt is space-delimited; a third-party client could send + // 'login consent', and an exact-string strip would miss 'login' + // in that case and re-trigger the guard's bounce after every + // OTP cycle. + if (typeof nextParams.prompt === 'string') { + const remaining = parsePromptTokens(nextParams.prompt) + remaining.delete('login') + if (remaining.size === 0) { + delete nextParams.prompt + } else { + nextParams.prompt = [...remaining].join(' ') + } } await store.updateRequest(requestId, { parameters: nextParams }) } diff --git a/packages/pds-core/src/lib/test-hooks.ts b/packages/pds-core/src/lib/test-hooks.ts index 8272de4b..4b5aca02 100644 --- a/packages/pds-core/src/lib/test-hooks.ts +++ b/packages/pds-core/src/lib/test-hooks.ts @@ -8,9 +8,9 @@ * POST /_internal/test/expire-device-account * Body: {did, deviceId?} * Backdates `account_device.updatedAt` 8 days into the past for the - * matching row(s). Used by features/session-reuse-bugs.feature rows 6 - * and 9 to age bindings past upstream's authenticationMaxAge (7d) so - * checkLoginRequired returns true for the targeted binding(s). + * matching row(s). Used by the e2e suite to age bindings past + * upstream's authenticationMaxAge (7d) so checkLoginRequired returns + * true for the targeted binding(s). */ import express, { type Application } from 'express' import type { PDS } from '@atproto/pds'