From 899346c9e736881e7a024ec4778901045ff40415 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 13:40:43 +0000 Subject: [PATCH 1/9] fix(auth-service): land on email form when prompt=login overrides login_hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub issue #138. The "Another account" rebind injected into the account chooser by pds-core navigates back to /oauth/authorize with the original login_hint preserved AND prompt=login appended. The session-reuse decision honoured prompt=login (skipped the chooser redirect, fell through to the login page), but the initialStep selector only looked at hint presence — so the resolved email forced initialStep='otp' and the user landed on the OTP form for the *previous* account instead of the email entry form they had asked for. Gate initialStep, otpAlreadySent, and the email pre-fill on isForceLoginPrompt(req) so prompt=login consistently produces a fresh email form regardless of which login_hint was carried through. E2E repro added in features/session-reuse-bugs.feature. --- .../another-account-reaches-email-form.md | 11 ++++++++++ features/session-reuse-bugs.feature | 15 ++++++++++++++ .../auth-service/src/routes/login-page.ts | 20 +++++++++++++++---- 3 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 .changeset/another-account-reaches-email-form.md diff --git a/.changeset/another-account-reaches-email-form.md b/.changeset/another-account-reaches-email-form.md new file mode 100644 index 00000000..b2177a5e --- /dev/null +++ b/.changeset/another-account-reaches-email-form.md @@ -0,0 +1,11 @@ +--- +'ePDS': patch +--- + +"Use a different account" on the chooser now reliably takes you to the email form, not the code step for the previous account. + +**Affects:** End users, Client app developers + +**End users:** when you click "Another account" on the account chooser to sign in as someone else, you now always land on a fresh email entry form. Previously, if the app that started the sign-in had pre-filled an account hint, the page jumped straight to the verification-code step for the *previous* account — leaving you stuck typing a code for an account you were trying to leave. + +**Client app developers:** no integration changes required. The fix tightens how `auth-service`'s `/oauth/authorize` page combines the standard OIDC `prompt=login` signal with a resolved `login_hint`: when both are present, the email step is rendered with no pre-fill, and any "code already sent" claim is suppressed so the next OTP send is treated as a fresh send. Apps that pass `login_hint` continue to land on the OTP step on a normal first visit; the change only affects the "force re-auth" path triggered by `prompt=login`. diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index e8410010..54903b3d 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -138,6 +138,21 @@ Feature: Welcome-page guard suppresses upstream's authentication UI When the demo client starts a new OAuth flow with the test user's handle as login_hint Then the browser lands on the ePDS enriched account picker + # GitHub issue #138: when the OAuth flow that reached the chooser carried + # a login_hint, the "Another account" rebind preserves it on the + # auth-service URL and adds prompt=login. shouldReuseSession honours + # prompt=login and falls through to the login page, but the initialStep + # selector at routes/login-page.ts only looks at hint presence — so the + # resolved email forces initialStep='otp' and the user lands on the OTP + # form for the *previous* account instead of the email entry form. The + # email form is the only correct outcome: prompt=login is the user's + # explicit "start over" signal. + Scenario: "Another account" with login_hint must reach the email form + When the demo client starts a new OAuth flow with the test user's handle as login_hint + Then the browser lands on the ePDS enriched account picker + When the user clicks "Another account" on the enriched account picker + Then the browser is on the auth service email form + Scenario: Login hint resolves to an unbound account — skip chooser Given another user has a separate PDS account When the demo client starts a new OAuth flow with the other user's handle as login_hint diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 62a3e959..18d5fdaa 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -58,6 +58,7 @@ import { buildPdsAuthorizeRedirect, deriveSharedCookieDomain, hasOrphanDeviceCookie, + isForceLoginPrompt, readDeviceSessionCookies, shouldReuseSession, } from '../lib/session-reuse.js' @@ -369,13 +370,22 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { // handle in the PAR body but don't duplicate it on the redirect URL) // The hint was already resolved above for the session-reuse decision; we // reuse `resolvedEmail` here rather than re-fetching. + // + // GitHub issue #138: prompt=login is the user's explicit "start fresh" + // signal — emitted by pds-core's "Another account" rebind injected into + // the chooser. The rebind preserves the original login_hint on the URL + // (so the OAuth flow can resume cleanly afterwards), but the user has + // just told us they want to sign in as a different account, so we must + // surface the email form regardless of the hint. const hasLoginHint = !!resolvedEmail - const initialStep = hasLoginHint ? 'otp' : 'email' + const forceLogin = isForceLoginPrompt(sessionReuseReq) + const initialStep = hasLoginHint && !forceLogin ? 'otp' : 'email' // Pillar 3 — Idempotency (Option A): when this is a duplicate GET for an // existing flow (e.g. browser extension, StayFocusd), tell the client-side - // script that OTP was already sent so it skips the auto-send. - const otpAlreadySent = hasLoginHint && !!existingFlow + // script that OTP was already sent so it skips the auto-send. Skipped on + // prompt=login: a forced re-auth must always re-send. + const otpAlreadySent = hasLoginHint && !forceLogin && !!existingFlow logger.info( { @@ -391,7 +401,9 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { // Use the resolved email (not the raw loginHint) for pre-filling forms. // This ensures handle-based hints get resolved to the correct email. - const emailHint = resolvedEmail ?? '' + // On prompt=login the user is opting out of the hinted account (issue + // #138), so suppress the pre-fill and start with an empty email box. + const emailHint = forceLogin ? '' : (resolvedEmail ?? '') res.type('html').send( renderLoginPage({ From 404063c5004adb09e8dd2f5257e86bbd9d454535 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 13:51:09 +0000 Subject: [PATCH 2/9] fix(auth-service): short-circuit login_hint resolution on prompt=login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #141 review feedback: - Compute forceLogin up-front so fetchParLoginHint, resolveLoginHint and fetchDeviceAccountEmails are skipped on the "Another account" path. Those internal-API round trips were pure overhead — the result was unused for both rendering (initialStep / emailHint) and for shouldReuseSession (which already short-circuits on isForceLoginPrompt). - Tighten the issue #138 E2E repro: assert #email is empty and #step-otp.active is hidden, not just that the email form is reachable. This pins down the full set of behaviours described in the issue. - Drop the Client app developers section from the changeset — the fix requires no integration changes, and per the writing-changesets skill an audience-specific section is only worthwhile if the reader needs to adapt their code. --- .../another-account-reaches-email-form.md | 6 +-- e2e/step-definitions/session-reuse.steps.ts | 19 +++++++ features/session-reuse-bugs.feature | 2 + .../auth-service/src/routes/login-page.ts | 54 ++++++++++--------- 4 files changed, 52 insertions(+), 29 deletions(-) diff --git a/.changeset/another-account-reaches-email-form.md b/.changeset/another-account-reaches-email-form.md index b2177a5e..182147d2 100644 --- a/.changeset/another-account-reaches-email-form.md +++ b/.changeset/another-account-reaches-email-form.md @@ -4,8 +4,6 @@ "Use a different account" on the chooser now reliably takes you to the email form, not the code step for the previous account. -**Affects:** End users, Client app developers +**Affects:** End users -**End users:** when you click "Another account" on the account chooser to sign in as someone else, you now always land on a fresh email entry form. Previously, if the app that started the sign-in had pre-filled an account hint, the page jumped straight to the verification-code step for the *previous* account — leaving you stuck typing a code for an account you were trying to leave. - -**Client app developers:** no integration changes required. The fix tightens how `auth-service`'s `/oauth/authorize` page combines the standard OIDC `prompt=login` signal with a resolved `login_hint`: when both are present, the email step is rendered with no pre-fill, and any "code already sent" claim is suppressed so the next OTP send is treated as a fresh send. Apps that pass `login_hint` continue to land on the OTP step on a normal first visit; the change only affects the "force re-auth" path triggered by `prompt=login`. +**End users:** when you click "Another account" on the account chooser to sign in as someone else, you now always land on a fresh email entry form. Previously, if the app that started the sign-in had pre-filled an account hint, the page jumped straight to the verification-code step for the _previous_ account — leaving you stuck typing a code for an account you were trying to leave. diff --git a/e2e/step-definitions/session-reuse.steps.ts b/e2e/step-definitions/session-reuse.steps.ts index 3de2152a..53d62b2e 100644 --- a/e2e/step-definitions/session-reuse.steps.ts +++ b/e2e/step-definitions/session-reuse.steps.ts @@ -418,6 +418,25 @@ Then( }, ) +/** + * Strict-form assertions used by the "Another account + login_hint" repro + * for issue #138: the email form must not carry the previous account's + * email pre-fill, and the OTP step must not be the active step (the + * regression rendered the page with both #email and #step-otp.active). + */ +Then('the email input is empty', async function (this: EpdsWorld) { + const page = getPage(this) + await expect(page.locator('#email')).toHaveValue('') +}) + +Then( + 'the OTP verification step is not active', + async function (this: EpdsWorld) { + const page = getPage(this) + await expect(page.locator('#step-otp.active')).toBeHidden() + }, +) + /** * Negative assertion for the auto-approve path: the second OAuth flow * must complete without the consent screen ever appearing. Checked by diff --git a/features/session-reuse-bugs.feature b/features/session-reuse-bugs.feature index 54903b3d..c35d3a4a 100644 --- a/features/session-reuse-bugs.feature +++ b/features/session-reuse-bugs.feature @@ -152,6 +152,8 @@ Feature: Welcome-page guard suppresses upstream's authentication UI Then the browser lands on the ePDS enriched account picker When the user clicks "Another account" on the enriched account picker Then the browser is on the auth service email form + And the email input is empty + And the OTP verification step is not active Scenario: Login hint resolves to an unbound account — skip chooser Given another user has a separate PDS account diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index 18d5fdaa..b9bd3bb8 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -173,6 +173,15 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { query: req.query as Record, } + // GitHub issue #138: prompt=login is the user's explicit "start fresh" + // signal — emitted by pds-core's "Another account" rebind injected into + // the chooser. On this path the resolved hint is never used for + // rendering (initialStep is forced to email and the prefill is + // suppressed) and shouldReuseSession bypasses its hint check, so the + // PAR/handle/device-account internal API round trips below would all be + // pure overhead. Compute it here so they can be skipped. + const forceLogin = isForceLoginPrompt(sessionReuseReq) + // Resolve the login_hint up-front so we can decide whether the // device session is a match before redirecting to pds-core. The // resolution result is also reused below for the email/OTP form. @@ -183,25 +192,27 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { const internalSecret = process.env.EPDS_INTERNAL_SECRET ?? '' let effectiveLoginHint = loginHint ?? null - if (!effectiveLoginHint && requestUri) { + if (!forceLogin && !effectiveLoginHint && requestUri) { effectiveLoginHint = await fetchParLoginHint( pdsInternalUrl, requestUri, internalSecret, ) } - const resolvedEmail = effectiveLoginHint - ? await resolveLoginHint( - effectiveLoginHint, - pdsInternalUrl, - internalSecret, - ) - : null + const resolvedEmail = + !forceLogin && effectiveLoginHint + ? await resolveLoginHint( + effectiveLoginHint, + pdsInternalUrl, + internalSecret, + ) + : null // Only fetch device-bound emails when we actually need them: the // cookie pair is present AND a hint resolved. Otherwise the existing // cookie-only reuse logic stands and the round trip to pds-core is - // pure overhead. + // pure overhead. forceLogin already short-circuited resolvedEmail to + // null, so the cookie-only branch covers the prompt=login path here. let deviceBoundEmails: string[] | null | undefined const cookiePair = readDeviceSessionCookies(sessionReuseReq) if (resolvedEmail && cookiePair) { @@ -369,23 +380,16 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { // c) Only in the stored PAR request (third-party apps that put the // handle in the PAR body but don't duplicate it on the redirect URL) // The hint was already resolved above for the session-reuse decision; we - // reuse `resolvedEmail` here rather than re-fetching. - // - // GitHub issue #138: prompt=login is the user's explicit "start fresh" - // signal — emitted by pds-core's "Another account" rebind injected into - // the chooser. The rebind preserves the original login_hint on the URL - // (so the OAuth flow can resume cleanly afterwards), but the user has - // just told us they want to sign in as a different account, so we must - // surface the email form regardless of the hint. + // reuse `resolvedEmail` here rather than re-fetching. On prompt=login + // (issue #138) `resolvedEmail` is forced to null up front so hasLoginHint + // is false and the email step always wins. const hasLoginHint = !!resolvedEmail - const forceLogin = isForceLoginPrompt(sessionReuseReq) - const initialStep = hasLoginHint && !forceLogin ? 'otp' : 'email' + const initialStep = hasLoginHint ? 'otp' : 'email' // Pillar 3 — Idempotency (Option A): when this is a duplicate GET for an // existing flow (e.g. browser extension, StayFocusd), tell the client-side - // script that OTP was already sent so it skips the auto-send. Skipped on - // prompt=login: a forced re-auth must always re-send. - const otpAlreadySent = hasLoginHint && !forceLogin && !!existingFlow + // script that OTP was already sent so it skips the auto-send. + const otpAlreadySent = hasLoginHint && !!existingFlow logger.info( { @@ -401,9 +405,9 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { // Use the resolved email (not the raw loginHint) for pre-filling forms. // This ensures handle-based hints get resolved to the correct email. - // On prompt=login the user is opting out of the hinted account (issue - // #138), so suppress the pre-fill and start with an empty email box. - const emailHint = forceLogin ? '' : (resolvedEmail ?? '') + // resolvedEmail is already null on the prompt=login path (issue #138), + // so the email box starts empty for "Another account". + const emailHint = resolvedEmail ?? '' res.type('html').send( renderLoginPage({ From 28c3a0da5560bb0e751d1333ae83a29b6a3c9ea8 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 14:27:15 +0000 Subject: [PATCH 3/9] test(auth-service): cover prompt=login branch of GET /oauth/authorize Adds three route-level tests for the issue #138 fix: 1. Email step rendered with empty input on prompt=login + login_hint 2. Hint-resolution + device-account internal API calls are NOT made when prompt=login is present (pins the optimisation that the e2e repro can't see) 3. Without prompt=login, login_hint resolution still happens normally (regression guard so the optimisation doesn't grow into a leak) Lifts login-page.ts unit-test coverage from ~33% to ~79% line. Uses the existing fetch+ephemeral-port pattern from test-hooks.test.ts rather than introducing supertest. Lives in its own file so vi.mock of the resolver modules doesn't bleed into the existing pure-helper tests in login-page.test.ts. --- .../__tests__/login-page-prompt-login.test.ts | 211 ++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 packages/auth-service/src/__tests__/login-page-prompt-login.test.ts diff --git a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts new file mode 100644 index 00000000..3d9e0dcb --- /dev/null +++ b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts @@ -0,0 +1,211 @@ +/** + * Route-level coverage for the prompt=login branch of GET /oauth/authorize. + * + * GitHub issue #138: pds-core's "Another account" rebind navigates back to + * auth-service with the original `login_hint` preserved AND `prompt=login` + * appended. The login-page handler must: + * + * 1. Render the email step (not the OTP step) regardless of the hint. + * 2. Leave the `#email` input empty (no pre-fill from the previous account). + * 3. Skip the three internal-API round trips that would normally resolve + * the hint (`fetchParLoginHint`, `resolveLoginHint`, + * `fetchDeviceAccountEmails`) — none of their results are used on this + * path, so calling them is pure overhead. + * + * Mirrors the e2e scenario at + * `features/session-reuse-bugs.feature:148`. The e2e test catches a + * regression of (1) and (2); this test pins (3) — a regression that + * re-introduced the network calls would silently revert the optimisation + * without any user-visible effect, so e2e wouldn't notice. + * + * Lives in its own file because the `vi.mock` calls below replace the + * shared resolver modules wholesale, and we don't want that bleed into the + * existing unit tests in login-page.test.ts. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import express from 'express' +import cookieParser from 'cookie-parser' +import * as fs from 'node:fs' +import * as path from 'node:path' +import * as os from 'node:os' +import type { AddressInfo } from 'node:net' +import { EpdsDb, _seedClientMetadataCacheForTest } from '@certified-app/shared' +import { csrfProtection } from '../middleware/csrf.js' +import { createLoginPageRouter } from '../routes/login-page.js' +import { AuthServiceContext, type AuthServiceConfig } from '../context.js' + +// Hoisted spies — `vi.mock` runs before module imports, so the mock factory +// must capture them via `vi.hoisted` rather than referencing top-level +// `const`s that haven't been initialised yet. +const mocks = vi.hoisted(() => ({ + fetchParLoginHint: vi.fn(), + resolveLoginHint: vi.fn(), + fetchDeviceAccountEmails: vi.fn(), +})) + +vi.mock('../lib/resolve-login-hint.js', () => ({ + fetchParLoginHint: mocks.fetchParLoginHint, + resolveLoginHint: mocks.resolveLoginHint, +})) + +vi.mock('../lib/fetch-device-accounts.js', () => ({ + fetchDeviceAccountEmails: mocks.fetchDeviceAccountEmails, +})) + +function makeConfig(dbPath: string): AuthServiceConfig { + return { + hostname: 'auth.test.local', + port: 0, + sessionSecret: 'test-session-secret', + csrfSecret: 'test-csrf-secret', + epdsCallbackSecret: 'test-callback-secret', + pdsHostname: 'test.local', + pdsPublicUrl: 'https://test.local', + email: { + provider: 'smtp', + smtpHost: 'localhost', + smtpPort: 1025, + from: 'noreply@test.local', + fromName: 'ePDS Test', + }, + dbLocation: dbPath, + otpLength: 6, + otpCharset: 'numeric', + trustedClients: [], + } +} + +async function startApp(ctx: AuthServiceContext): Promise<{ + baseUrl: string + close: () => Promise +}> { + const app = express() + app.use(cookieParser()) + app.use(csrfProtection(ctx.config.csrfSecret)) + app.use(createLoginPageRouter(ctx)) + const server = app.listen(0) + await new Promise((resolve, reject) => { + server.once('error', reject) + server.once('listening', () => { + resolve() + }) + }) + server.unref() + const port = (server.address() as AddressInfo).port + return { + baseUrl: `http://127.0.0.1:${port}`, + close: () => + new Promise((resolve) => { + server.close(() => { + resolve() + }) + }), + } +} + +describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { + let db: EpdsDb + let dbPath: string + let ctx: AuthServiceContext + let app: { baseUrl: string; close: () => Promise } + + beforeEach(async () => { + dbPath = path.join( + os.tmpdir(), + `prompt-login-${Date.now()}-${Math.random()}.db`, + ) + db = new EpdsDb(dbPath) + // Avoid an outbound fetch when the handler resolves client metadata. + _seedClientMetadataCacheForTest('https://app.example.com', { + client_name: 'Test App', + }) + const config = makeConfig(dbPath) + ctx = new AuthServiceContext(config) + // Replace the constructor-created db (built against the same path) so + // the test's seed/inspection share state with the handler. + Object.defineProperty(ctx, 'db', { value: db, configurable: true }) + app = await startApp(ctx) + mocks.fetchParLoginHint.mockReset() + mocks.resolveLoginHint.mockReset() + mocks.fetchDeviceAccountEmails.mockReset() + }) + + afterEach(async () => { + await app.close() + ctx.destroy() + try { + fs.unlinkSync(dbPath) + } catch { + /* db may already be gone */ + } + }) + + it('renders the email step with empty input on prompt=login + login_hint', async () => { + // Even if a stale hint resolution were to slip through, force its + // result to be a non-empty email so a regression that ignored + // forceLogin would be obviously visible as a pre-filled box. + mocks.fetchParLoginHint.mockResolvedValue('previous@example.com') + mocks.resolveLoginHint.mockResolvedValue('previous@example.com') + + const url = + app.baseUrl + + '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:promptlogin' + + '&client_id=' + + encodeURIComponent('https://app.example.com') + + '&login_hint=previous%40example.com' + + '&prompt=login' + const res = await fetch(url) + expect(res.status).toBe(200) + const html = await res.text() + + // (1) Email step is the rendered step, not the OTP step. + expect(html).toMatch(/
/) + expect(html).not.toMatch(/class="step-otp active"/) + + // (2) Email input is empty (no pre-fill from the previous account). + expect(html).toMatch(/]*value=""[^>]*>/) + }) + + it('skips internal-API round trips when prompt=login is present', async () => { + mocks.fetchParLoginHint.mockResolvedValue('previous@example.com') + mocks.resolveLoginHint.mockResolvedValue('previous@example.com') + + const url = + app.baseUrl + + '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:promptloginskip' + + '&login_hint=previous%40example.com' + + '&prompt=login' + const res = await fetch(url) + expect(res.status).toBe(200) + + // The hint resolution chain must be untouched — its result is unused + // on the prompt=login path AND shouldReuseSession bypasses the hint + // check, so calling these is pure overhead on the PDS internal API. + expect(mocks.fetchParLoginHint).not.toHaveBeenCalled() + expect(mocks.resolveLoginHint).not.toHaveBeenCalled() + expect(mocks.fetchDeviceAccountEmails).not.toHaveBeenCalled() + }) + + it('still resolves login_hint when prompt=login is absent (regression guard)', async () => { + // Without prompt=login, the hint must still be resolved so the OTP + // step can pre-fill the email — the optimisation only applies to the + // forced-reauth path. + mocks.resolveLoginHint.mockResolvedValue('user@example.com') + + const url = + app.baseUrl + + '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:nopromptlogin' + + '&login_hint=user%40example.com' + const res = await fetch(url) + expect(res.status).toBe(200) + const html = await res.text() + + expect(mocks.resolveLoginHint).toHaveBeenCalledWith( + 'user@example.com', + expect.any(String), + expect.any(String), + ) + // OTP step is the active step (hasLoginHint true, !forceLogin). + expect(html).toMatch(/class="step-otp active"/) + }) +}) From 6f4a65d5731c67411dcb91c64e51141f178ca045 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 14:33:56 +0000 Subject: [PATCH 4/9] test(auth-service): silence sonar hotspots in prompt-login route test Address two SonarCloud hotspots flagged on the new test file: - Replace Math.random() with randomBytes(4) for the tmp DB filename suffix. Date.now() collision avoidance doesn't actually need a cryptographic source here, but using randomBytes silences sonar's "weak PRNG" warning and matches the import already in login-page.test.ts. - Disable express's x-powered-by response header on the in-process test app to silence sonar's "framework version disclosure" hotspot. --- .../src/__tests__/login-page-prompt-login.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts index 3d9e0dcb..16e3addc 100644 --- a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts +++ b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts @@ -25,6 +25,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import express from 'express' import cookieParser from 'cookie-parser' +import { randomBytes } from 'node:crypto' import * as fs from 'node:fs' import * as path from 'node:path' import * as os from 'node:os' @@ -80,6 +81,12 @@ async function startApp(ctx: AuthServiceContext): Promise<{ close: () => Promise }> { const app = express() + // Silence sonar's "framework version disclosure" hotspot that fires + // on any vanilla express() instance. This is an in-process test + // server bound to 127.0.0.1 on an ephemeral port — the header is + // only visible to the test runner — but disabling it keeps the + // signal clean. + app.disable('x-powered-by') app.use(cookieParser()) app.use(csrfProtection(ctx.config.csrfSecret)) app.use(createLoginPageRouter(ctx)) @@ -112,7 +119,7 @@ describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { beforeEach(async () => { dbPath = path.join( os.tmpdir(), - `prompt-login-${Date.now()}-${Math.random()}.db`, + `prompt-login-${Date.now()}-${randomBytes(4).toString('hex')}.db`, ) db = new EpdsDb(dbPath) // Avoid an outbound fetch when the handler resolves client metadata. From 1ce864e0270341acf0044149bc1f1270882e4fb4 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 14:45:32 +0000 Subject: [PATCH 5/9] fix(auth-service): honour space-delimited and array prompt values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #141 review feedback (copilot, @aspiers). The original isForceLoginPrompt() check `p === 'login'` only matched the exact single-token literal. Per OIDC Core 1.0 §3.1.2.1, prompt is a space-delimited list (e.g. `prompt=login consent`), and Express's req.query parser surfaces repeated keys as arrays (`?prompt=login&prompt=consent` -> `['login','consent']`). Either shape would have re-introduced the issue #138 behaviour for clients that pass them. pds-core's auth-ui-guard already had a correct parsePromptTokens() implementation (PR #129). Lift it to @certified-app/shared as oidc-prompt.ts so both packages consume the same canonical version, and rewrite isForceLoginPrompt to delegate to promptHasLogin. Keep the auth-ui-guard names as thin re-exports of the shared functions so existing imports (and the auth-ui-guard.test.ts test file) don't churn. Tighten the prompt=login test in login-page-prompt-login.test.ts: the original assertion that fetchDeviceAccountEmails wasn't called passed trivially because the request didn't carry a dev-id/ses-id cookie pair, and that call is gated on cookie presence anyway. Send a real cookie pair so the assertion now genuinely catches a regression that re-introduced device-email fetching on the prompt=login path. Verified via local sabotage: stripping the forceLogin gates causes 2/3 tests to fail as expected. Add session-reuse.test.ts coverage for the new behaviours: prompt as space-delimited list, prompt as array, and the negative case (list of non-login tokens). --- .../__tests__/login-page-prompt-login.test.ts | 15 ++++++- .../src/__tests__/session-reuse.test.ts | 33 +++++++++++++- .../auth-service/src/lib/session-reuse.ts | 12 +++-- packages/pds-core/src/auth-ui-guard.ts | 35 +++++++-------- packages/shared/src/index.ts | 1 + packages/shared/src/oidc-prompt.ts | 44 +++++++++++++++++++ 6 files changed, 115 insertions(+), 25 deletions(-) create mode 100644 packages/shared/src/oidc-prompt.ts diff --git a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts index 16e3addc..a079a110 100644 --- a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts +++ b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts @@ -176,18 +176,29 @@ describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { it('skips internal-API round trips when prompt=login is present', async () => { mocks.fetchParLoginHint.mockResolvedValue('previous@example.com') mocks.resolveLoginHint.mockResolvedValue('previous@example.com') - + // Make the assertion meaningful for fetchDeviceAccountEmails by sending + // a dev-id/ses-id cookie pair: the handler only fetches device-bound + // emails when BOTH a cookie pair is present AND `resolvedEmail` is + // truthy. Without the cookie pair, the assertion would pass trivially + // even if a regression reintroduced hint resolution. const url = app.baseUrl + '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:promptloginskip' + '&login_hint=previous%40example.com' + '&prompt=login' - const res = await fetch(url) + const res = await fetch(url, { + headers: { + cookie: + 'dev-id=dev-test-skip-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; ses-id=ses-test-skip-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb', + }, + }) expect(res.status).toBe(200) // The hint resolution chain must be untouched — its result is unused // on the prompt=login path AND shouldReuseSession bypasses the hint // check, so calling these is pure overhead on the PDS internal API. + // With cookies present, a regression that left fetchDeviceAccountEmails + // unguarded by forceLogin would call it; this assertion catches that. expect(mocks.fetchParLoginHint).not.toHaveBeenCalled() expect(mocks.resolveLoginHint).not.toHaveBeenCalled() expect(mocks.fetchDeviceAccountEmails).not.toHaveBeenCalled() diff --git a/packages/auth-service/src/__tests__/session-reuse.test.ts b/packages/auth-service/src/__tests__/session-reuse.test.ts index 8630473f..baa03af6 100644 --- a/packages/auth-service/src/__tests__/session-reuse.test.ts +++ b/packages/auth-service/src/__tests__/session-reuse.test.ts @@ -180,10 +180,39 @@ describe('isForceLoginPrompt (HYPER-268)', () => { expect(isForceLoginPrompt(makeReq())).toBe(false) }) - it('returns false when prompt is an array', () => { + // OIDC Core 1.0 §3.1.2.1: `prompt` is a space-delimited list. The + // single-token cases above must continue to work, AND multi-token / + // repeated-key forms that include `login` must also be honoured — + // otherwise a client passing `prompt=login consent` (or two `prompt=` + // query keys) would re-trigger #138 even after the per-step gate + // landed. + it('returns true when prompt is a space-delimited list containing login', () => { + expect( + isForceLoginPrompt(makeReq({ query: { prompt: 'login consent' } })), + ).toBe(true) + expect( + isForceLoginPrompt(makeReq({ query: { prompt: 'consent login' } })), + ).toBe(true) + }) + + it('returns true when prompt is an array containing login', () => { + // Express's req.query parser surfaces repeated query keys as arrays: + // `?prompt=login&prompt=consent` becomes `['login', 'consent']`. expect(isForceLoginPrompt(makeReq({ query: { prompt: ['login'] } }))).toBe( - false, + true, ) + expect( + isForceLoginPrompt(makeReq({ query: { prompt: ['consent', 'login'] } })), + ).toBe(true) + }) + + it('returns false when prompt is a list of other tokens', () => { + expect( + isForceLoginPrompt(makeReq({ query: { prompt: 'consent none' } })), + ).toBe(false) + expect( + isForceLoginPrompt(makeReq({ query: { prompt: ['consent', 'none'] } })), + ).toBe(false) }) }) diff --git a/packages/auth-service/src/lib/session-reuse.ts b/packages/auth-service/src/lib/session-reuse.ts index 6f3d5961..4b25c75f 100644 --- a/packages/auth-service/src/lib/session-reuse.ts +++ b/packages/auth-service/src/lib/session-reuse.ts @@ -10,6 +10,7 @@ * then either auto-selects a matching session (flow 1 with login_hint) * or renders the account chooser (flow 2). */ +import { promptHasLogin } from '@certified-app/shared' /** Shape of the request data the helpers consume. Intentionally narrow * so we can unit-test without an Express instance. Mirrors the fields @@ -117,10 +118,15 @@ export function hasOrphanDeviceCookie(req: SessionReuseRequest): { * #AuthRequest): "The Authorization Server SHOULD prompt the End-User * for reauthentication". The capture-phase rebind of upstream's * "Another account" button injected into the chooser in pds-core uses - * this to opt out of session reuse. */ + * this to opt out of session reuse. + * + * Delegates to the shared `promptHasLogin` so this check matches + * pds-core's auth-ui-guard exactly: both the space-delimited token + * case (`prompt=login consent`) and the array-of-strings case that + * Express produces from repeated query keys (`?prompt=login&prompt=consent`) + * are honoured. */ export function isForceLoginPrompt(req: SessionReuseRequest): boolean { - const p = req.query.prompt - return typeof p === 'string' && p === 'login' + return promptHasLogin(req.query.prompt) } /** Optional context for the hint-vs-bindings check that gates Flow 1 diff --git a/packages/pds-core/src/auth-ui-guard.ts b/packages/pds-core/src/auth-ui-guard.ts index 6aa1abde..a053b24b 100644 --- a/packages/pds-core/src/auth-ui-guard.ts +++ b/packages/pds-core/src/auth-ui-guard.ts @@ -41,8 +41,25 @@ import { SESSION_ID_PREFIX, } from '@atproto/oauth-provider' import type { Logger } from 'pino' +import { + parsePromptTokens as parsePromptTokensShared, + promptHasLogin as promptHasLoginShared, +} from '@certified-app/shared' import { loadDeviceBindings } from './lib/device-accounts.js' +/** Re-export of the shared implementation. The canonical home is + * `@certified-app/shared`; this re-export exists so internal callers + * in pds-core (and the existing `auth-ui-guard.test.ts` import site) + * don't have to be touched on every refactor. New callers should + * import from `@certified-app/shared` directly. */ +export const parsePromptTokens = parsePromptTokensShared +/** Re-export of the shared implementation. The canonical home is + * `@certified-app/shared`; this re-export exists so internal callers + * in pds-core (and the existing `auth-ui-guard.test.ts` import site) + * don't have to be touched on every refactor. New callers should + * import from `@certified-app/shared` directly. */ +export const promptHasLogin = promptHasLoginShared + const DEVICE_ID_RE = new RegExp( `^${DEVICE_ID_PREFIX}[0-9a-f]{${DEVICE_ID_BYTES_LENGTH * 2}}$`, ) @@ -368,21 +385,3 @@ 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/shared/src/index.ts b/packages/shared/src/index.ts index 8b906e15..120fb4f0 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -71,6 +71,7 @@ export type { PreviewCheck, PreviewValidationResult, } from './preview-validation.js' +export { parsePromptTokens, promptHasLogin } from './oidc-prompt.js' export { getEpdsVersion } from './version.js' export { makeSafeFetch } from './safe-fetch.js' export type { SafeFetchOptions } from './safe-fetch.js' diff --git a/packages/shared/src/oidc-prompt.ts b/packages/shared/src/oidc-prompt.ts new file mode 100644 index 00000000..81107aca --- /dev/null +++ b/packages/shared/src/oidc-prompt.ts @@ -0,0 +1,44 @@ +/** + * Helpers for parsing the OIDC `prompt` parameter. + * + * 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. An + * exact string check (`p === 'login'`) misses every multi-token value + * that includes `login`. Both pds-core's auth-ui-guard and + * auth-service's session-reuse layer must agree on what counts as + * "the client asked for forced re-authentication", so the parsing + * lives here in shared rather than being duplicated per package. + * + * Express's `req.query` parser also surfaces repeated query keys as + * arrays (`?prompt=login&prompt=consent` → `['login', 'consent']`), so + * we accept both string and string-array shapes. + */ + +/** Tokenise an OIDC `prompt` parameter value into its space-delimited + * set. Returns an empty Set for null/undefined/non-string-or-array + * input. Array shapes (from repeated query keys) are joined with a + * space before tokenising so `?prompt=login&prompt=consent` produces + * the same set as `prompt=login%20consent`. */ +export function parsePromptTokens(value: unknown): Set { + let raw: string + if (typeof value === 'string') { + raw = value + } else if ( + Array.isArray(value) && + value.every((v): v is string => typeof v === 'string') + ) { + raw = value.join(' ') + } else { + return new Set() + } + return new Set(raw.split(/\s+/).filter(Boolean)) +} + +/** True when the given OIDC prompt value contains the `login` token. + * Both the auth-ui-guard and the auth-service login-page route use + * this — they must agree on what counts as "forced login" so the + * guard's bounce condition and the auth-service's session-reuse / + * initial-step decisions apply to exactly the same requests. */ +export function promptHasLogin(value: unknown): boolean { + return parsePromptTokens(value).has('login') +} From 83e8012d00759ad300499dae311df96421b2ab41 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 15:02:43 +0000 Subject: [PATCH 6/9] test(auth-service): close the dual-handle SQLite leak in prompt=login test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #141 review feedback (copilot). The original beforeEach created its own `EpdsDb` against `dbPath`, then `Object.defineProperty`-overwrote `ctx.db` to point at it. The constructor-created instance was orphaned: never closed, holding a file descriptor + a SQLite write lock + the WAL/SHM companion files until process GC. Same dual-handle WAL antipattern that motivated 0f2b19d's switch from a second better-sqlite3 connection to reusing PDS's Kysely instance — two writers on one file is opaque trouble. The original "share state with the handler" rationale didn't apply: no test in the file actually seeds or inspects via the test-side db handle. Drop it. AuthServiceContext opens its own EpdsDb; we use ctx.db (transitively) for everything. One handle per test, closed cleanly by ctx.destroy(). Also tidy the afterEach: replace the empty-catch unlink with fs.rmSync(force:true), and clean up the WAL/SHM companions so the tmp directory isn't left with stale -wal/-shm files when WAL hasn't checkpointed by close time. --- .../__tests__/login-page-prompt-login.test.ts | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts index a079a110..98e4d1f8 100644 --- a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts +++ b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts @@ -30,7 +30,7 @@ import * as fs from 'node:fs' import * as path from 'node:path' import * as os from 'node:os' import type { AddressInfo } from 'node:net' -import { EpdsDb, _seedClientMetadataCacheForTest } from '@certified-app/shared' +import { _seedClientMetadataCacheForTest } from '@certified-app/shared' import { csrfProtection } from '../middleware/csrf.js' import { createLoginPageRouter } from '../routes/login-page.js' import { AuthServiceContext, type AuthServiceConfig } from '../context.js' @@ -111,7 +111,6 @@ async function startApp(ctx: AuthServiceContext): Promise<{ } describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { - let db: EpdsDb let dbPath: string let ctx: AuthServiceContext let app: { baseUrl: string; close: () => Promise } @@ -121,16 +120,16 @@ describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { os.tmpdir(), `prompt-login-${Date.now()}-${randomBytes(4).toString('hex')}.db`, ) - db = new EpdsDb(dbPath) // Avoid an outbound fetch when the handler resolves client metadata. _seedClientMetadataCacheForTest('https://app.example.com', { client_name: 'Test App', }) - const config = makeConfig(dbPath) - ctx = new AuthServiceContext(config) - // Replace the constructor-created db (built against the same path) so - // the test's seed/inspection share state with the handler. - Object.defineProperty(ctx, 'db', { value: db, configurable: true }) + // AuthServiceContext opens its own EpdsDb against config.dbLocation; + // we use ctx.db throughout (rather than constructing a parallel + // instance and overwriting) so there's exactly one open SQLite handle + // per test — `ctx.destroy()` in afterEach closes it cleanly and + // releases the WAL/SHM companion files for the unlink to remove. + ctx = new AuthServiceContext(makeConfig(dbPath)) app = await startApp(ctx) mocks.fetchParLoginHint.mockReset() mocks.resolveLoginHint.mockReset() @@ -140,10 +139,12 @@ describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { afterEach(async () => { await app.close() ctx.destroy() - try { - fs.unlinkSync(dbPath) - } catch { - /* db may already be gone */ + // Best-effort cleanup of the temp DB and its WAL/SHM companions. + // SQLite in WAL mode writes alongside the main file; rmSync(force) + // tolerates the missing companions when WAL was checkpointed before + // close, and avoids the empty try/catch antipattern. + for (const suffix of ['', '-wal', '-shm']) { + fs.rmSync(dbPath + suffix, { force: true }) } }) From dfee0f8c2d97d4691651ce74910333af817b29bd Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 15:50:30 +0000 Subject: [PATCH 7/9] fix(pds-core,auth-service): use epds_skip_par_hint signal for "Another account" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the prior approach that gated rendering on prompt=login with a dedicated signal carrying the actual user intent. The prior approach broke the @session-reuse e2e scenario "login_hint narrows to a stale binding on a multi-account device": pds-core's auth-ui-guard appends prompt=login on its sign-in-view bounce while still expecting auth-service to honour the PAR's login_hint and serve the OTP step. Conflating prompt=login with the "Another account" rebind made auth-service skip hint resolution on that bounce too, so no OTP email was sent and the user got stuck on an empty email form. Solution: - pds-core's chooser-enrichment "Another account" rebind now sets epds_skip_par_hint=1 (in addition to the existing prompt=login for shouldReuseSession bypass) and drops any URL login_hint. Comment block updated to spell out why prompt=login alone is ambiguous and the skip flag is needed. - auth-service's GET /oauth/authorize gates fetchParLoginHint on the absence of epds_skip_par_hint=1. Once the PAR hint is suppressed, resolvedEmail ends up null and the spec-correct rendering decision (hasLoginHint -> initialStep) drops out: email form for issue #138, OTP form for the auth-ui-guard bounce. - Drop the forceEmailForm override and the unused isForceLoginPrompt import — no longer needed once the input to the rendering decision is correctly suppressed. Unit tests in login-page-prompt-login.test.ts updated: - "renders the email step on the Another account rebind path" asserts fetchParLoginHint is NOT called when epds_skip_par_hint=1. - "honours PAR login_hint when prompt=login arrives without the skip flag" pins the auth-ui-guard bounce path so a future simplification doesn't re-conflate the two signals. Verified locally: - 20/20 session-reuse e2e (was 19/20 — the stale-binding scenario now passes again). - 897/897 unit tests, lint/format/typecheck clean. --- .../__tests__/login-page-prompt-login.test.ts | 104 ++++++++---------- .../auth-service/src/routes/login-page.ts | 49 ++++----- packages/pds-core/src/chooser-enrichment.ts | 10 ++ 3 files changed, 81 insertions(+), 82 deletions(-) diff --git a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts index 98e4d1f8..28561c03 100644 --- a/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts +++ b/packages/auth-service/src/__tests__/login-page-prompt-login.test.ts @@ -1,26 +1,26 @@ /** - * Route-level coverage for the prompt=login branch of GET /oauth/authorize. + * Route-level coverage for the "Another account" rebind branch of + * GET /oauth/authorize. * - * GitHub issue #138: pds-core's "Another account" rebind navigates back to - * auth-service with the original `login_hint` preserved AND `prompt=login` - * appended. The login-page handler must: + * GitHub issue #138: pds-core's "Another account" rebind navigates back + * to auth-service with `prompt=login` AND `epds_skip_par_hint=1` + * appended (and any URL `login_hint` stripped). `epds_skip_par_hint=1` + * tells auth-service to ignore any login_hint stored in the PAR — the + * user clicked an opt-out, so the RP's hint must not influence rendering. + * With no hint resolving, `resolvedEmail` is null and the email step + * falls out from the standard rendering decision. * - * 1. Render the email step (not the OTP step) regardless of the hint. - * 2. Leave the `#email` input empty (no pre-fill from the previous account). - * 3. Skip the three internal-API round trips that would normally resolve - * the hint (`fetchParLoginHint`, `resolveLoginHint`, - * `fetchDeviceAccountEmails`) — none of their results are used on this - * path, so calling them is pure overhead. - * - * Mirrors the e2e scenario at - * `features/session-reuse-bugs.feature:148`. The e2e test catches a - * regression of (1) and (2); this test pins (3) — a regression that - * re-introduced the network calls would silently revert the optimisation - * without any user-visible effect, so e2e wouldn't notice. + * `prompt=login` ALONE (without the skip flag) must NOT suppress hint + * resolution — pds-core's auth-ui-guard sign-in-view bounce appends + * prompt=login while still expecting the hint to be honoured (the user + * wants that account; upstream's password sign-in form is just + * unreachable in a passwordless deployment). The third test below pins + * this so a future "simplification" that conflates the two signals is + * caught. * * Lives in its own file because the `vi.mock` calls below replace the - * shared resolver modules wholesale, and we don't want that bleed into the - * existing unit tests in login-page.test.ts. + * shared resolver modules wholesale, and we don't want that bleed into + * the existing unit tests in login-page.test.ts. */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import express from 'express' @@ -148,67 +148,59 @@ describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { } }) - it('renders the email step with empty input on prompt=login + login_hint', async () => { - // Even if a stale hint resolution were to slip through, force its - // result to be a non-empty email so a regression that ignored - // forceLogin would be obviously visible as a pre-filled box. + it('renders the email step on the "Another account" rebind path', async () => { + // pds-core's "Another account" rebind sets epds_skip_par_hint=1 + // and strips URL login_hint. Mock fetchParLoginHint to return a + // value anyway so a regression that ignored the skip flag would + // visibly pre-fill the email box with the previous user. mocks.fetchParLoginHint.mockResolvedValue('previous@example.com') mocks.resolveLoginHint.mockResolvedValue('previous@example.com') const url = app.baseUrl + - '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:promptlogin' + + '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:rebind' + '&client_id=' + encodeURIComponent('https://app.example.com') + - '&login_hint=previous%40example.com' + - '&prompt=login' + '&prompt=login' + + '&epds_skip_par_hint=1' const res = await fetch(url) expect(res.status).toBe(200) const html = await res.text() - // (1) Email step is the rendered step, not the OTP step. + // PAR hint resolution is skipped: fetchParLoginHint must NOT be called. + expect(mocks.fetchParLoginHint).not.toHaveBeenCalled() + // Email step rendered, OTP step not active, input empty. expect(html).toMatch(/
/) expect(html).not.toMatch(/class="step-otp active"/) - - // (2) Email input is empty (no pre-fill from the previous account). expect(html).toMatch(/]*value=""[^>]*>/) }) - it('skips internal-API round trips when prompt=login is present', async () => { - mocks.fetchParLoginHint.mockResolvedValue('previous@example.com') - mocks.resolveLoginHint.mockResolvedValue('previous@example.com') - // Make the assertion meaningful for fetchDeviceAccountEmails by sending - // a dev-id/ses-id cookie pair: the handler only fetches device-bound - // emails when BOTH a cookie pair is present AND `resolvedEmail` is - // truthy. Without the cookie pair, the assertion would pass trivially - // even if a regression reintroduced hint resolution. + it('honours PAR login_hint when prompt=login arrives without the skip flag', async () => { + // pds-core's auth-ui-guard sign-in-view bounce appends prompt=login + // (no epds_skip_par_hint) and expects auth-service to resolve any + // PAR login_hint and serve the OTP step. A regression that + // conflated prompt=login with the rebind semantics would re-break + // the @session-reuse e2e scenario "login_hint narrows to a stale + // binding on a multi-account device". + mocks.fetchParLoginHint.mockResolvedValue('hinted@example.com') + mocks.resolveLoginHint.mockResolvedValue('hinted@example.com') + const url = app.baseUrl + - '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:promptloginskip' + - '&login_hint=previous%40example.com' + + '/oauth/authorize?request_uri=urn:ietf:params:oauth:request_uri:guardbounce' + '&prompt=login' - const res = await fetch(url, { - headers: { - cookie: - 'dev-id=dev-test-skip-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; ses-id=ses-test-skip-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb', - }, - }) + const res = await fetch(url) expect(res.status).toBe(200) + const html = await res.text() - // The hint resolution chain must be untouched — its result is unused - // on the prompt=login path AND shouldReuseSession bypasses the hint - // check, so calling these is pure overhead on the PDS internal API. - // With cookies present, a regression that left fetchDeviceAccountEmails - // unguarded by forceLogin would call it; this assertion catches that. - expect(mocks.fetchParLoginHint).not.toHaveBeenCalled() - expect(mocks.resolveLoginHint).not.toHaveBeenCalled() - expect(mocks.fetchDeviceAccountEmails).not.toHaveBeenCalled() + expect(mocks.fetchParLoginHint).toHaveBeenCalled() + expect(mocks.resolveLoginHint).toHaveBeenCalled() + expect(html).toMatch(/class="step-otp active"/) }) it('still resolves login_hint when prompt=login is absent (regression guard)', async () => { - // Without prompt=login, the hint must still be resolved so the OTP - // step can pre-fill the email — the optimisation only applies to the - // forced-reauth path. + // Without prompt=login at all, the hint must resolve normally and + // the OTP step pre-fills with the email. mocks.resolveLoginHint.mockResolvedValue('user@example.com') const url = @@ -224,7 +216,7 @@ describe('GET /oauth/authorize prompt=login handling (issue #138)', () => { expect.any(String), expect.any(String), ) - // OTP step is the active step (hasLoginHint true, !forceLogin). + // OTP step is the active step (hasLoginHint true). expect(html).toMatch(/class="step-otp active"/) }) }) diff --git a/packages/auth-service/src/routes/login-page.ts b/packages/auth-service/src/routes/login-page.ts index b9bd3bb8..53503472 100644 --- a/packages/auth-service/src/routes/login-page.ts +++ b/packages/auth-service/src/routes/login-page.ts @@ -58,7 +58,6 @@ import { buildPdsAuthorizeRedirect, deriveSharedCookieDomain, hasOrphanDeviceCookie, - isForceLoginPrompt, readDeviceSessionCookies, shouldReuseSession, } from '../lib/session-reuse.js' @@ -173,18 +172,19 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { query: req.query as Record, } - // GitHub issue #138: prompt=login is the user's explicit "start fresh" - // signal — emitted by pds-core's "Another account" rebind injected into - // the chooser. On this path the resolved hint is never used for - // rendering (initialStep is forced to email and the prefill is - // suppressed) and shouldReuseSession bypasses its hint check, so the - // PAR/handle/device-account internal API round trips below would all be - // pure overhead. Compute it here so they can be skipped. - const forceLogin = isForceLoginPrompt(sessionReuseReq) - // Resolve the login_hint up-front so we can decide whether the // device session is a match before redirecting to pds-core. The // resolution result is also reused below for the email/OTP form. + // + // pds-core's "Another account" rebind sets epds_skip_par_hint=1 + // (issue #138) — the user clicked an opt-out, so any login_hint + // stored in the PAR by the RP at OAuth init must be ignored. The + // rebind also strips URL login_hint, so on that path effectiveLoginHint + // ends up null and the spec-correct decision below renders the email + // form. This skip flag does NOT affect prompt=login alone: pds-core's + // sign-in-view bounce also sets prompt=login (without the skip flag) + // and expects the hint to resolve normally so the OTP step renders. + const skipParHint = req.query.epds_skip_par_hint === '1' const pdsInternalUrl = ensurePdsUrl( process.env.PDS_INTERNAL_URL, ctx.config.pdsPublicUrl, @@ -192,27 +192,25 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { const internalSecret = process.env.EPDS_INTERNAL_SECRET ?? '' let effectiveLoginHint = loginHint ?? null - if (!forceLogin && !effectiveLoginHint && requestUri) { + if (!skipParHint && !effectiveLoginHint && requestUri) { effectiveLoginHint = await fetchParLoginHint( pdsInternalUrl, requestUri, internalSecret, ) } - const resolvedEmail = - !forceLogin && effectiveLoginHint - ? await resolveLoginHint( - effectiveLoginHint, - pdsInternalUrl, - internalSecret, - ) - : null + const resolvedEmail = effectiveLoginHint + ? await resolveLoginHint( + effectiveLoginHint, + pdsInternalUrl, + internalSecret, + ) + : null // Only fetch device-bound emails when we actually need them: the // cookie pair is present AND a hint resolved. Otherwise the existing // cookie-only reuse logic stands and the round trip to pds-core is - // pure overhead. forceLogin already short-circuited resolvedEmail to - // null, so the cookie-only branch covers the prompt=login path here. + // pure overhead. let deviceBoundEmails: string[] | null | undefined const cookiePair = readDeviceSessionCookies(sessionReuseReq) if (resolvedEmail && cookiePair) { @@ -380,9 +378,10 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { // c) Only in the stored PAR request (third-party apps that put the // handle in the PAR body but don't duplicate it on the redirect URL) // The hint was already resolved above for the session-reuse decision; we - // reuse `resolvedEmail` here rather than re-fetching. On prompt=login - // (issue #138) `resolvedEmail` is forced to null up front so hasLoginHint - // is false and the email step always wins. + // reuse `resolvedEmail` here rather than re-fetching. On the "Another + // account" rebind path (issue #138) `epds_skip_par_hint=1` upstream + // suppresses both URL and PAR hint resolution, so resolvedEmail is null + // and the email step falls out without a special case here. const hasLoginHint = !!resolvedEmail const initialStep = hasLoginHint ? 'otp' : 'email' @@ -405,8 +404,6 @@ export function createLoginPageRouter(ctx: AuthServiceContext): Router { // Use the resolved email (not the raw loginHint) for pre-filling forms. // This ensures handle-based hints get resolved to the correct email. - // resolvedEmail is already null on the prompt=login path (issue #138), - // so the email box starts empty for "Another account". const emailHint = resolvedEmail ?? '' res.type('html').send( diff --git a/packages/pds-core/src/chooser-enrichment.ts b/packages/pds-core/src/chooser-enrichment.ts index dd6935ee..6a38778f 100644 --- a/packages/pds-core/src/chooser-enrichment.ts +++ b/packages/pds-core/src/chooser-enrichment.ts @@ -109,6 +109,14 @@ export function buildChooserEnrichmentScript(): string { // request_uri / client_id / scope etc. so the OAuth flow resumes // after the new account signs in. // + // Adds epds_skip_par_hint=1 — an ePDS-private signal that tells + // auth-service "ignore the login_hint stored in the PAR for this + // request" (issue #138). The user clicked "Another account", so + // they're overriding any hint the RP supplied at OAuth init: with + // no hint resolved, the spec-correct rendering decision is the + // email form. URL login_hint is also dropped; the PAR-body hint + // can't be mutated from here, hence the explicit skip flag. + // // Returns '' when there is no request_uri in the current URL // (standalone /account navigation, bookmark, direct URL) — auth-service // rejects /oauth/authorize without request_uri with a 400, so letting @@ -118,6 +126,8 @@ export function buildChooserEnrichmentScript(): string { var params = new URLSearchParams(window.location.search || ''); if (!params.has('request_uri')) return ''; params.set('prompt', 'login'); + params.set('epds_skip_par_hint', '1'); + params.delete('login_hint'); return authOrigin + '/oauth/authorize?' + params.toString(); } From 15814d7fd7a3b746990e52bcfa23298d0a02a063 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 16:16:46 +0000 Subject: [PATCH 8/9] test(e2e): retry PDS /health on the env-up Given against transient blips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CI e2e workflow already polls /health for up to 5 minutes per service before invoking the suite, so by the time `pnpm test:e2e` runs the deploy is reachable. But a single transient Railway-edge or undici DNS blip 12 seconds later on the first scenario's `Given the ePDS test environment is running` step kills an entire 9-minute e2e run — observed on PR #141 commit dfee0f8. Budget 6 attempts × 2s = ~12s of retries on transient failures (undici TypeError, non-OK status). Real outages still surface once the budget is spent. Reproduced from CI logs: pre-step health check passed all 5 services at 15:52:47, scenario 1 fetch threw `TypeError: fetch failed` at 15:52:59, all 60 subsequent scenarios passed. --- e2e/step-definitions/common.steps.ts | 37 ++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/e2e/step-definitions/common.steps.ts b/e2e/step-definitions/common.steps.ts index b2b835d0..155d69c3 100644 --- a/e2e/step-definitions/common.steps.ts +++ b/e2e/step-definitions/common.steps.ts @@ -11,13 +11,40 @@ import type { EpdsWorld } from '../support/world.js' import { testEnv } from '../support/env.js' import { getPage } from '../support/utils.js' +/** + * Health-check the PDS with a short retry budget. The CI workflow already + * polls /health for 5 minutes before invoking the e2e suite, so any + * failure here is *post* deploy-readiness — typically a transient Railway + * edge / undici DNS blip that resolves in seconds. Without retries, a + * single such blip on the first scenario fails an entire 9-minute e2e + * run; with retries, the rest of the suite runs unimpaired. + * + * Total budget: 6 attempts × 2s backoff = ~12s. Real outages still + * surface — the suite cannot make meaningful progress past this Given + * without a live PDS, so a hard failure is the right outcome once the + * budget is spent. + */ Given('the ePDS test environment is running', async function (this: EpdsWorld) { - const res = await fetch(`${testEnv.pdsUrl}/health`) - if (!res.ok) { - throw new Error( - `PDS health check failed: ${res.status} at ${testEnv.pdsUrl}/health`, - ) + const url = `${testEnv.pdsUrl}/health` + const maxAttempts = 6 + let lastErr: unknown + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + const res = await fetch(url) + if (res.ok) return + lastErr = new Error(`PDS health check failed: ${res.status} at ${url}`) + } catch (err) { + lastErr = err + } + if (attempt < maxAttempts) { + await new Promise((resolve) => setTimeout(resolve, 2000)) + } } + throw new Error( + `PDS health check failed after ${maxAttempts} attempts at ${url}: ${ + lastErr instanceof Error ? lastErr.message : String(lastErr) + }`, + ) }) /** From 592a7a6e97cece9fb0ede9629398ea87a28b9a7a Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 1 May 2026 16:43:11 +0000 Subject: [PATCH 9/9] fix(auth-service,e2e): clear cleanup interval on context destroy + bound retry fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #141 review feedback (copilot, coderabbit). - AuthServiceContext's setInterval was never cleared in destroy(), so vitest workers constructing one context per test would accumulate timers across tests. Store the handle, clearInterval() in destroy(), and unref() it so a short-lived test process isn't held alive by the timer alone if destroy() is somehow missed. - The /health retry helper added in 15814d7 had no per-attempt fetch timeout, so a hung connection could blow past the documented ~12s budget. Add AbortSignal.timeout(2s) per attempt; the worst-case wall-clock is now bounded at 6 × (2s fetch + 2s backoff) = ~24s. --- e2e/step-definitions/common.steps.ts | 17 +++++++++++------ packages/auth-service/src/context.ts | 10 ++++++++-- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/e2e/step-definitions/common.steps.ts b/e2e/step-definitions/common.steps.ts index 155d69c3..a1d41884 100644 --- a/e2e/step-definitions/common.steps.ts +++ b/e2e/step-definitions/common.steps.ts @@ -19,25 +19,30 @@ import { getPage } from '../support/utils.js' * single such blip on the first scenario fails an entire 9-minute e2e * run; with retries, the rest of the suite runs unimpaired. * - * Total budget: 6 attempts × 2s backoff = ~12s. Real outages still - * surface — the suite cannot make meaningful progress past this Given - * without a live PDS, so a hard failure is the right outcome once the - * budget is spent. + * Each attempt has its own AbortSignal.timeout(2s) so a hung connection + * cannot blow past the documented wall-clock budget of 6 × (2s fetch + + * 2s backoff) = ~24s. Real outages still surface — the suite cannot make + * meaningful progress past this Given without a live PDS, so a hard + * failure is the right outcome once the budget is spent. */ Given('the ePDS test environment is running', async function (this: EpdsWorld) { const url = `${testEnv.pdsUrl}/health` const maxAttempts = 6 + const perAttemptTimeoutMs = 2000 + const backoffMs = 2000 let lastErr: unknown for (let attempt = 1; attempt <= maxAttempts; attempt++) { try { - const res = await fetch(url) + const res = await fetch(url, { + signal: AbortSignal.timeout(perAttemptTimeoutMs), + }) if (res.ok) return lastErr = new Error(`PDS health check failed: ${res.status} at ${url}`) } catch (err) { lastErr = err } if (attempt < maxAttempts) { - await new Promise((resolve) => setTimeout(resolve, 2000)) + await new Promise((resolve) => setTimeout(resolve, backoffMs)) } } throw new Error( diff --git a/packages/auth-service/src/context.ts b/packages/auth-service/src/context.ts index a2b02f38..65f6d787 100644 --- a/packages/auth-service/src/context.ts +++ b/packages/auth-service/src/context.ts @@ -51,14 +51,18 @@ export class AuthServiceContext { public readonly db: EpdsDb public readonly emailSender: EmailSender public readonly config: AuthServiceConfig + private readonly cleanupInterval: ReturnType constructor(config: AuthServiceConfig) { this.config = config this.db = new EpdsDb(config.dbLocation) this.emailSender = new EmailSender(config.email, config.trustedClients) - // Cleanup expired tokens every 5 minutes - setInterval( + // Cleanup expired tokens every 5 minutes. unref() so a process that + // owns this context (e.g. a vitest worker constructing a context per + // test) isn't held alive by the timer alone — destroy() still + // explicitly clears it for prompt teardown of the cleanup callback. + this.cleanupInterval = setInterval( () => { const flows = this.db.cleanupExpiredAuthFlows() if (flows > 0) { @@ -69,9 +73,11 @@ export class AuthServiceContext { }, 5 * 60 * 1000, ) + this.cleanupInterval.unref() } destroy(): void { + clearInterval(this.cleanupInterval) this.db.close() } }