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 68e30b26..b9f2b747 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -8,6 +8,14 @@ services: env_file: .env environment: - PDS_PORT=3000 + # 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). + # 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 @@ -27,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/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/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 3fbc0672..23cc5bdf 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 - * welcome-page guard scenarios. See docs/design/session-reuse-bugs.md for - * the failure-mode taxonomy. + * Step definitions for features/session-reuse-bugs.feature — the + * 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 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 + * - 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 @@ -19,9 +22,10 @@ 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' // --------------------------------------------------------------------------- // 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) }, @@ -193,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 + // 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) + url.searchParams.set('login_hint_location', 'body') + await page.goto(url.toString()) + }, +) + Given( 'another user has a separate PDS account', async function (this: EpdsWorld) { @@ -332,7 +358,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( @@ -564,3 +598,134 @@ Given( ).toBe(pdsHost) }, ) + +// --------------------------------------------------------------------------- +// 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( + '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 trigger this scenario + // exercises. + const page = getPage(this) + const url = new URL('/api/oauth/login', testEnv.demoUrl) + url.searchParams.set('prompt', 'login') + await page.goto(url.toString()) + }, +) + +// 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 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 +}): 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-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 pickHandle(this) + 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/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 d931a764..e8410010 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,87 @@ 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. 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: + # - 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: 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 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) + # 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 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 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. + 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 + + # 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 + When the demo client starts a new OAuth flow with the test user's handle as a PAR-body login_hint + # 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/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 66% 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 46adde1d..df213699 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,12 @@ import { describe, expect, it, vi } from 'vitest' import { appendCookieClearHeaders, buildBounceUrl, - createWelcomePageGuard, + createAuthUiGuard, isGuardedPath, parseDeviceCookies, -} from '../welcome-page-guard.js' + parsePromptTokens, + promptHasLogin, +} from '../auth-ui-guard.js' const VALID_DEV = 'dev-0123456789abcdef0123456789abcdef' const VALID_SES = 'ses-fedcba9876543210fedcba9876543210' @@ -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() @@ -233,6 +299,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 +316,16 @@ 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). + // 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 return { @@ -256,6 +340,12 @@ function makeProvider(opts: { ), }, }, + requestManager: { + store: { + readRequest: vi.fn(opts.readRequest ?? (() => Promise.resolve(null))), + }, + }, + checkLoginRequired: vi.fn(opts.checkLoginRequired ?? (() => false)), } } @@ -298,12 +388,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, @@ -321,7 +411,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, @@ -334,7 +424,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, @@ -350,7 +440,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, @@ -385,7 +475,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, @@ -402,7 +492,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, @@ -425,7 +515,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, @@ -452,7 +542,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, @@ -475,7 +565,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, @@ -502,7 +592,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, @@ -528,7 +618,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, @@ -561,7 +651,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, @@ -593,7 +683,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, @@ -625,7 +715,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, @@ -656,7 +746,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, @@ -678,4 +768,182 @@ describe('createWelcomePageGuard', () => { expect.stringContaining('readDevice failed'), ) }) + + // --------------------------------------------------------------------------- + // 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 + // logic. Only `account.sub` and `account.preferred_username` are read. + function binding(sub: string, pu: string) { + return { + account: { sub, preferred_username: pu }, + } as unknown as { account: { sub: string; preferred_username: string } } + } + + // urn-prefixed request_uri so loadStoredPar actually attempts the read + // (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] & { + 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 = createAuthUiGuard({ + 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, + ) + return { provider, res, next } + } + + /** Asserts the guard responded 303 (bounce). */ + function expectBounce(res: ReturnType): void { + expect(res.status).toHaveBeenCalledWith(303) + } + + /** 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('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 + // 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('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([ + binding('did:plc:a', 'a.example'), + binding('did:plc:b', 'b.example'), + ]), + readRequest: () => Promise.resolve({ parameters: {} }), + checkLoginRequired: () => true, + }) + expectBounce(res) + }) + + // 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 = [ + binding('did:plc:stale', 'stale.example'), + binding('did:plc:fresh', 'fresh.example'), + ] + + 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: () => + Promise.resolve({ parameters: { login_hint: 'stale.example' } }), + checkLoginRequired: onlyStaleIsLoginRequired, + }) + expectBounce(res) + }) + + 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({ + bindings: () => Promise.resolve(TWO_BINDINGS), + readRequest: () => + Promise.resolve({ parameters: { login_hint: 'fresh.example' } }), + checkLoginRequired: onlyStaleIsLoginRequired, + }) + expectPassThrough(res, next) + }) + + it('passes through when at least one binding is fresh and there is no hint', async () => { + // Mixed freshness, no hint → chooser reaches a usable session. + const { res, next } = await signinViewCase({ + bindings: () => Promise.resolve(TWO_BINDINGS), + readRequest: () => Promise.resolve({ parameters: {} }), + checkLoginRequired: onlyStaleIsLoginRequired, + }) + expectPassThrough(res, next) + }) + + it('falls back to all bindings when login_hint matches none of them', async () => { + // 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, + }) + 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 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 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, + }) + expectPassThrough(res, next) + expect(errSpy).toHaveBeenCalledWith( + expect.any(Object), + expect.stringContaining('store.readRequest failed'), + ) + }) }) 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..09ac7902 --- /dev/null +++ b/packages/pds-core/src/__tests__/test-hooks.test.ts @@ -0,0 +1,243 @@ +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() + }) + }) + } +} + +/** 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 } = {} + + 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 = SECRET + process.env.EPDS_TEST_HOOKS = '1' + }) + + 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 { 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.NODE_ENV = 'production' + expect(() => { + setupApp() + }).toThrow(/production/i) + }) + + it('rejects requests without the internal secret', async () => { + 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 () => { + const { app } = setupApp() + 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 () => { + 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 () => { + 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 + 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 () => { + const { app, fakeUpdate } = setupApp({ updatedRows: 1 }) + const res = await postHook( + app, + { did: 'did:plc:a', deviceId: 'dev-deadbeef' }, + AUTH_HEADER, + ) + + expect(res.status).toBe(200) + expect(res.json.updated).toBe(1) + expect(fakeUpdate.inspect().wheres).toEqual([ + ['did', '=', 'did:plc:a'], + ['deviceId', '=', 'dev-deadbeef'], + ]) + }) + + it('returns 500 and logs when the underlying update throws', async () => { + 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( + 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. + 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) + expect(typeof res.json.updated).toBe('number') + }) +}) diff --git a/packages/pds-core/src/welcome-page-guard.ts b/packages/pds-core/src/auth-ui-guard.ts similarity index 55% rename from packages/pds-core/src/welcome-page-guard.ts rename to packages/pds-core/src/auth-ui-guard.ts index 269344ef..6aa1abde 100644 --- a/packages/pds-core/src/welcome-page-guard.ts +++ b/packages/pds-core/src/auth-ui-guard.ts @@ -1,27 +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 (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. + * 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 + * 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. * * 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. */ import type { NextFunction, Request, Response } from 'express' -import type { OAuthProvider } from '@atproto/oauth-provider' +import type { DeviceAccount, OAuthProvider } from '@atproto/oauth-provider' import { DEVICE_ID_BYTES_LENGTH, DEVICE_ID_PREFIX, @@ -29,7 +41,7 @@ import { SESSION_ID_PREFIX, } from '@atproto/oauth-provider' import type { Logger } from 'pino' -import { loadDeviceAccountEmails } from './lib/device-accounts.js' +import { loadDeviceBindings } from './lib/device-accounts.js' const DEVICE_ID_RE = new RegExp( `^${DEVICE_ID_PREFIX}[0-9a-f]{${DEVICE_ID_BYTES_LENGTH * 2}}$`, @@ -175,14 +187,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, @@ -229,18 +241,148 @@ 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, + logCtx: 'guard', + logger, + }) + 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 for the + // externally-reproducible scenarios under "Sign-in-view leaks". + const params = await loadStoredPar({ + provider, + requestUrl: req.url, logger, }) - if (!emails || emails.length === 0) { + if (promptHasLogin(params?.prompt)) { + 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. 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 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 + +/** 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 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 + 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 +} + +/** 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/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 42cc215e..5d1d1f6d 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -62,8 +62,9 @@ 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, parsePromptTokens } from './auth-ui-guard.js' import { loadDeviceAccountEmails } from './lib/device-accounts.js' +import { installTestHooks } from './lib/test-hooks.js' const logger = createLogger('pds-core') @@ -513,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 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( @@ -529,9 +542,26 @@ 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, + } + // 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 }) } } @@ -673,44 +703,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 @@ -721,7 +759,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 @@ -1088,6 +1126,8 @@ async function main() { res.json({ emails }) }) + 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/device-accounts.ts b/packages/pds-core/src/lib/device-accounts.ts index ddfdf91a..e3a4d553 100644 --- a/packages/pds-core/src/lib/device-accounts.ts +++ b/packages/pds-core/src/lib/device-accounts.ts @@ -1,10 +1,9 @@ /** - * 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, - * 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/lib/test-hooks.ts b/packages/pds-core/src/lib/test-hooks.ts new file mode 100644 index 00000000..4b5aca02 --- /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 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' +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' }) + } + }, + ) +}