diff --git a/.changeset/handle-mode-through-approval.md b/.changeset/handle-mode-through-approval.md new file mode 100644 index 00000000..2936e07c --- /dev/null +++ b/.changeset/handle-mode-through-approval.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +The approval and account-chooser screens now honour the handle mode your app asked for, instead of falling back to the server default. + +**Affects:** Client app developers + +**Client app developers:** no action needed — `epds_handle_mode` is resolved exactly as before (the `/oauth/authorize` query parameter, then your OAuth client metadata, then the server default). Previously the resolved mode was lost on the way to the approval step, so a flow that asked for a chosen handle could still show a generated one there. The mode is now carried through `/oauth/epds-callback` and applied on the approval and chooser screens. Values that are not one of the recognised modes are ignored rather than forwarded. diff --git a/packages/auth-service/src/__tests__/callback-handle-mode.test.ts b/packages/auth-service/src/__tests__/callback-handle-mode.test.ts new file mode 100644 index 00000000..6d63e114 --- /dev/null +++ b/packages/auth-service/src/__tests__/callback-handle-mode.test.ts @@ -0,0 +1,239 @@ +import { randomBytes } from 'node:crypto' +import type { AddressInfo } from 'node:net' +import express from 'express' +import cookieParser from 'cookie-parser' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + verifyCallback, + type CallbackParams, + type HandleMode, +} from '@certified-app/shared' +import type { AuthServiceContext } from '../context.js' +import { createChooseHandleRouter } from '../routes/choose-handle.js' +import { createCompleteRouter } from '../routes/complete.js' + +const mocks = vi.hoisted(() => ({ + getDidByEmail: vi.fn(), + pingParRequest: vi.fn(), + resolveRecoveryEmail: vi.fn(), + resolveClientBranding: vi.fn(), +})) + +vi.mock('../lib/get-did-by-email.js', () => ({ + getDidByEmail: mocks.getDidByEmail, +})) + +vi.mock('../lib/ping-par-request.js', () => ({ + pingParRequest: mocks.pingParRequest, +})) + +vi.mock('../lib/resolve-recovery-email.js', () => ({ + resolveRecoveryEmail: mocks.resolveRecoveryEmail, +})) + +vi.mock('../lib/client-metadata.js', () => ({ + resolveClientBranding: mocks.resolveClientBranding, +})) + +const AUTH_FLOW_COOKIE = 'epds_auth_flow' +const realFetch = globalThis.fetch.bind(globalThis) + +function makeCtx(handleMode: HandleMode | null): AuthServiceContext { + return { + config: { + pdsPublicUrl: 'https://pds.example', + pdsHostname: 'pds.example', + epdsCallbackSecret: 'test-callback-secret', + trustedClients: [], + }, + db: { + getAuthFlow: vi.fn(() => ({ + flowId: 'flow-1', + requestUri: 'urn:ietf:params:oauth:request_uri:req-123', + clientId: 'https://app.example/client.json', + handleMode, + })), + deleteAuthFlow: vi.fn(), + }, + } as unknown as AuthServiceContext +} + +function makeAuth() { + return { + api: { + getSession: vi.fn(() => + Promise.resolve({ user: { email: 'Alice@example.com' } }), + ), + }, + } +} + +async function startApp( + ctx: AuthServiceContext, + auth: ReturnType, +): Promise<{ baseUrl: string; close: () => Promise }> { + const app = express() + app.disable('x-powered-by') + app.use(cookieParser()) + app.use(express.urlencoded({ extended: false })) + app.use(createCompleteRouter(ctx, auth)) + app.use(createChooseHandleRouter(ctx, auth)) + + 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() + }) + }), + } +} + +function parseRedirect(res: globalThis.Response): URL { + const location = res.headers.get('location') + if (!location) throw new Error('Missing redirect location') + return new URL(location) +} + +function normalizeFetchUrl(input: Parameters[0]): URL { + if (input instanceof URL) return input + if (typeof input === 'string') return new URL(input) + return new URL(input.url) +} + +function verifySignedCallbackUrl(url: URL): boolean { + const callbackParams: CallbackParams = { + request_uri: url.searchParams.get('request_uri') ?? '', + email: url.searchParams.get('email') ?? '', + approved: url.searchParams.get('approved') ?? '', + new_account: url.searchParams.get('new_account') ?? '', + ...(url.searchParams.has('handle') + ? { handle: url.searchParams.get('handle') ?? '' } + : {}), + // /auth/complete signs client_id too (so a dead-PAR clean exit can still + // reach the right client); omitting it here would fail verification. + ...(url.searchParams.has('client_id') + ? { client_id: url.searchParams.get('client_id') ?? '' } + : {}), + ...(url.searchParams.has('epds_handle_mode') + ? { epds_handle_mode: url.searchParams.get('epds_handle_mode') ?? '' } + : {}), + } + + return verifyCallback( + callbackParams, + url.searchParams.get('ts') ?? '', + url.searchParams.get('sig') ?? '', + 'test-callback-secret', + ) +} + +async function fetchCompleteRedirect(handleMode: HandleMode | null) { + const app = await startApp(makeCtx(handleMode), makeAuth()) + try { + const res = await fetch(`${app.baseUrl}/auth/complete`, { + redirect: 'manual', + headers: { cookie: `${AUTH_FLOW_COOKIE}=flow-1` }, + }) + + expect(res.status).toBe(303) + const url = parseRedirect(res) + expect(url.pathname).toBe('/oauth/epds-callback') + return url + } finally { + await app.close() + } +} + +describe('auth-service epds-callback handle mode threading', () => { + let priorEnv: { pdsInternalUrl?: string; internalSecret?: string } + + beforeEach(() => { + priorEnv = { + pdsInternalUrl: process.env.PDS_INTERNAL_URL, + internalSecret: process.env.EPDS_INTERNAL_SECRET, + } + process.env.PDS_INTERNAL_URL = 'http://pds.internal' // NOSONAR test-only internal mocked URL + process.env.EPDS_INTERNAL_SECRET = 'test-internal-secret' + mocks.getDidByEmail.mockReset() + mocks.pingParRequest.mockReset() + mocks.resolveRecoveryEmail.mockReset() + mocks.resolveClientBranding.mockReset() + mocks.pingParRequest.mockResolvedValue({ ok: true }) + mocks.resolveRecoveryEmail.mockResolvedValue(null) + mocks.resolveClientBranding.mockResolvedValue({ + customCss: null, + customFaviconUrl: null, + customFaviconUrlDark: null, + }) + vi.stubGlobal( + 'fetch', + vi.fn((input: Parameters[0], init?: RequestInit) => { + const url = normalizeFetchUrl(input) + if (url.hostname === '127.0.0.1') return realFetch(input, init) + return Promise.resolve({ + ok: true, + json: () => Promise.resolve({ exists: false }), + }) + }), + ) + }) + + afterEach(() => { + if (priorEnv.pdsInternalUrl === undefined) + delete process.env.PDS_INTERNAL_URL + else process.env.PDS_INTERNAL_URL = priorEnv.pdsInternalUrl + if (priorEnv.internalSecret === undefined) + delete process.env.EPDS_INTERNAL_SECRET + else process.env.EPDS_INTERNAL_SECRET = priorEnv.internalSecret + vi.unstubAllGlobals() + }) + + it('includes the stored canonical handle mode for random-mode new users', async () => { + mocks.getDidByEmail.mockResolvedValue(null) + const url = await fetchCompleteRedirect('random') + expect(url.searchParams.get('epds_handle_mode')).toBe('random') + expect(url.searchParams.has('handle')).toBe(false) + expect(verifySignedCallbackUrl(url)).toBe(true) + }) + + it('includes the stored canonical handle mode for existing users', async () => { + mocks.getDidByEmail.mockResolvedValue(randomBytes(16).toString('hex')) + const url = await fetchCompleteRedirect('picker-with-random') + expect(url.searchParams.get('epds_handle_mode')).toBe('picker-with-random') + }) + + it('includes the stored canonical handle mode for chosen-handle callbacks', async () => { + mocks.getDidByEmail.mockResolvedValue(null) + const app = await startApp(makeCtx('picker'), makeAuth()) + try { + const res = await fetch(`${app.baseUrl}/auth/choose-handle`, { + method: 'POST', + redirect: 'manual', + headers: { + cookie: `${AUTH_FLOW_COOKIE}=flow-1`, + 'content-type': 'application/x-www-form-urlencoded', + }, + body: new URLSearchParams({ handle: 'Alice1' }), + }) + + expect(res.status).toBe(303) + const url = parseRedirect(res) + expect(url.pathname).toBe('/oauth/epds-callback') + expect(url.searchParams.get('epds_handle_mode')).toBe('picker') + expect(url.searchParams.get('handle')).toBe('alice1') + } finally { + await app.close() + } + }) +}) diff --git a/packages/auth-service/src/routes/choose-handle.ts b/packages/auth-service/src/routes/choose-handle.ts index 73e502ca..a986ca87 100644 --- a/packages/auth-service/src/routes/choose-handle.ts +++ b/packages/auth-service/src/routes/choose-handle.ts @@ -404,6 +404,7 @@ export function createChooseHandleRouter( approved: '1', new_account: '1', handle: normalizedLocal, + epds_handle_mode: flow.handleMode ?? '', } if (flow.clientId) callbackParams.client_id = flow.clientId const { sig, ts } = signCallback( diff --git a/packages/auth-service/src/routes/complete.ts b/packages/auth-service/src/routes/complete.ts index 3c9d9cab..da5c1c44 100644 --- a/packages/auth-service/src/routes/complete.ts +++ b/packages/auth-service/src/routes/complete.ts @@ -57,6 +57,10 @@ const AUTH_FLOW_COOKIE = 'epds_auth_flow' * the same `params.handle ?? ''` shape; the sentinel is pinned by * tests in packages/shared/src/__tests__/crypto.test.ts. * + * `epds_handle_mode` is signed too rather than appended afterwards, so + * the browser cannot flip the chooser/consent presentation mode on the + * hop to pds-core without invalidating the signature. + * * Exported so it can be unit-tested without standing up the full * /auth/complete route. */ @@ -65,6 +69,7 @@ export function buildEpdsCallbackUrl(args: { flowClientId: string | null email: string isNewAccount: boolean + flowHandleMode?: string | null pdsPublicUrl: string epdsCallbackSecret: string }): string { @@ -75,11 +80,41 @@ export function buildEpdsCallbackUrl(args: { new_account: args.isNewAccount ? '1' : '0', } if (args.flowClientId) callbackParams.client_id = args.flowClientId + if (args.flowHandleMode) callbackParams.epds_handle_mode = args.flowHandleMode const { sig, ts } = signCallback(callbackParams, args.epdsCallbackSecret) const params = new URLSearchParams({ ...callbackParams, ts, sig }) return `${args.pdsPublicUrl}/oauth/epds-callback?${params.toString()}` } +async function resolveCompleteIdentity( + email: string, + flowId: string, + ctx: AuthServiceContext, + pdsUrl: string, + internalSecret: string, +): Promise<{ email: string; did: string | null }> { + const did = await getDidByEmail(email, pdsUrl, internalSecret) + if (did) return { email, did } + + // Recovery path: session email is a backup email, not a primary. Resolve + // the backup-email -> DID mapping (auth-service-owned) and then DID -> + // primary email via pds-core's internal API, so the downstream callback + // signs the user's real account email, not the recovery address. + const recovered = await resolveRecoveryEmail( + email, + ctx, + pdsUrl, + internalSecret, + ) + if (!recovered) return { email, did: null } + + logger.info( + { flowId, did: recovered.did }, + 'Recovery: translated backup email to primary email via DID', + ) + return { email: recovered.email, did: recovered.did } +} + export function createCompleteRouter( ctx: AuthServiceContext, // eslint-disable-next-line @typescript-eslint/no-explicit-any -- better-auth instance has no exported type @@ -127,7 +162,11 @@ export function createCompleteRouter( */ async function redirectNewUserRandomMode( res: Response, - flow: { requestUri: string; clientId: string | null }, + flow: { + requestUri: string + clientId: string | null + handleMode: string | null + }, email: string, flowId: string, ): Promise { @@ -143,6 +182,7 @@ export function createCompleteRouter( flowClientId: flow.clientId, email, isNewAccount: true, + flowHandleMode: flow.handleMode, pdsPublicUrl: ctx.config.pdsPublicUrl, epdsCallbackSecret: ctx.config.epdsCallbackSecret, }) @@ -204,31 +244,16 @@ export function createCompleteRouter( return } - let email = session.user.email.toLowerCase() + const sessionEmail = session.user.email.toLowerCase() // Step 4: Check whether this is a new user (no PDS account for email). - let did = await getDidByEmail(email, pdsUrl, internalSecret) - - // Recovery path: session email is a backup email, not a primary. Resolve - // the backup-email → DID mapping (auth-service-owned) and then DID → - // primary email via pds-core's internal API, so the downstream callback - // signs the user's real account email, not the recovery address. - if (!did) { - const recovered = await resolveRecoveryEmail( - email, - ctx, - pdsUrl, - internalSecret, - ) - if (recovered) { - logger.info( - { flowId, did: recovered.did }, - 'Recovery: translated backup email to primary email via DID', - ) - email = recovered.email - did = recovered.did - } - } + const { email, did } = await resolveCompleteIdentity( + sessionEmail, + flowId, + ctx, + pdsUrl, + internalSecret, + ) const isNewAccount = !did @@ -264,6 +289,7 @@ export function createCompleteRouter( flowClientId: flow.clientId, email, isNewAccount: false, + flowHandleMode: flow.handleMode, pdsPublicUrl: ctx.config.pdsPublicUrl, epdsCallbackSecret: ctx.config.epdsCallbackSecret, }) diff --git a/packages/pds-core/src/__tests__/epds-callback-authorize.test.ts b/packages/pds-core/src/__tests__/epds-callback-authorize.test.ts new file mode 100644 index 00000000..dad5103b --- /dev/null +++ b/packages/pds-core/src/__tests__/epds-callback-authorize.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from 'vitest' +import { buildPostCallbackAuthorizeUrl } from '../lib/epds-callback-authorize.js' + +describe('buildPostCallbackAuthorizeUrl', () => { + it('preserves a valid display-only epds_handle_mode on the final authorize redirect', () => { + const url = buildPostCallbackAuthorizeUrl({ + pdsUrl: 'https://pds.example', + requestUri: 'urn:ietf:params:oauth:request_uri:req-123', + clientId: 'https://app.example/client.json', + handleMode: 'random', + }) + + expect(url.pathname).toBe('/oauth/authorize') + expect(url.searchParams.get('request_uri')).toBe( + 'urn:ietf:params:oauth:request_uri:req-123', + ) + expect(url.searchParams.get('client_id')).toBe( + 'https://app.example/client.json', + ) + expect(url.searchParams.get('epds_handle_mode')).toBe('random') + }) + + it('drops invalid epds_handle_mode values from the final authorize redirect', () => { + const url = buildPostCallbackAuthorizeUrl({ + pdsUrl: 'https://pds.example', + requestUri: 'urn:ietf:params:oauth:request_uri:req-123', + clientId: 'https://app.example/client.json', + handleMode: 'garbage', + }) + + expect(url.searchParams.has('epds_handle_mode')).toBe(false) + }) +}) diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index 48ea356a..ec9281cf 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -67,6 +67,7 @@ import { createAuthUiGuard, parsePromptTokens } from './auth-ui-guard.js' import { loadDeviceAccountEmails } from './lib/device-accounts.js' import { handleCallbackError } from './lib/epds-callback-error.js' import { installTestHooks } from './lib/test-hooks.js' +import { buildPostCallbackAuthorizeUrl } from './lib/epds-callback-authorize.js' const logger = createLogger('pds-core') @@ -205,6 +206,7 @@ async function main() { const newAccountStr = req.query.new_account as string const handleParam = req.query.handle as string | undefined const clientIdParam = req.query.client_id as string | undefined + const handleModeParam = req.query.epds_handle_mode as string | undefined const signatureValid = verifyCallback( { request_uri: requestUri, @@ -213,6 +215,7 @@ async function main() { new_account: newAccountStr, handle: handleParam, client_id: clientIdParam, + epds_handle_mode: handleModeParam, }, ts, sig, @@ -592,9 +595,12 @@ async function main() { // - Checks checkConsentRequired() against actual OAuth scopes // - Auto-approves if no consent needed (SSO match, previously authorized scopes) // - Renders the upstream consent UI (consent-view.tsx) if consent is required - const authorizeUrl = new URL('/oauth/authorize', pdsUrl) - authorizeUrl.searchParams.set('request_uri', requestUri) - authorizeUrl.searchParams.set('client_id', clientId) + const authorizeUrl = buildPostCallbackAuthorizeUrl({ + pdsUrl, + requestUri, + clientId, + handleMode: req.query.epds_handle_mode, + }) res.setHeader('Cache-Control', 'no-store') res.redirect(303, authorizeUrl.toString()) @@ -775,19 +781,21 @@ async function main() { .map((s) => s.trim()) .filter(Boolean) + const resolveClientIdFromRequestUri = provider + ? async (requestUri: string) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- @atproto/oauth-provider requestManager not exported + const requestData = await (provider.requestManager as any).get( + requestUri, + ) + return requestData?.clientId as string | undefined + } + : undefined + installCssInjectionMiddleware(pds.app, stack, { trustedClients, resolveClientMetadata, getClientCss, - resolveClientIdFromRequestUri: provider - ? async (requestUri: string) => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- @atproto/oauth-provider requestManager not exported - const requestData = await (provider.requestManager as any).get( - requestUri, - ) - return requestData?.clientId as string | undefined - } - : undefined, + resolveClientIdFromRequestUri, logger, }) diff --git a/packages/pds-core/src/lib/epds-callback-authorize.ts b/packages/pds-core/src/lib/epds-callback-authorize.ts new file mode 100644 index 00000000..809f6324 --- /dev/null +++ b/packages/pds-core/src/lib/epds-callback-authorize.ts @@ -0,0 +1,33 @@ +import { VALID_HANDLE_MODES, type HandleMode } from '@certified-app/shared' + +export function resolveCallbackHandleMode( + value: unknown, +): HandleMode | undefined { + return typeof value === 'string' && + (VALID_HANDLE_MODES as readonly string[]).includes(value) + ? (value as HandleMode) + : undefined +} + +/** + * Builds the post-/oauth/epds-callback redirect back to stock /oauth/authorize. + * This differs from auth-service callback builders, which create signed + * auth-service -> pds-core /oauth/epds-callback URLs. It is extracted from the + * old inline construction so epds_handle_mode forwarding and sanitization can be + * unit-tested. + */ +export function buildPostCallbackAuthorizeUrl(opts: { + pdsUrl: string + requestUri: string + clientId: string + handleMode: unknown +}): URL { + const authorizeUrl = new URL('/oauth/authorize', opts.pdsUrl) + authorizeUrl.searchParams.set('request_uri', opts.requestUri) + authorizeUrl.searchParams.set('client_id', opts.clientId) + + const handleMode = resolveCallbackHandleMode(opts.handleMode) + if (handleMode) authorizeUrl.searchParams.set('epds_handle_mode', handleMode) + + return authorizeUrl +} diff --git a/packages/shared/src/__tests__/crypto.test.ts b/packages/shared/src/__tests__/crypto.test.ts index f56a8b65..cd2a57c1 100644 --- a/packages/shared/src/__tests__/crypto.test.ts +++ b/packages/shared/src/__tests__/crypto.test.ts @@ -131,6 +131,36 @@ describe('generateRandomHandle', () => { }) }) +function makeCallbackParams(overrides: Partial = {}) { + return { + request_uri: 'urn:ietf:params:oauth:request_uri:test', + email: 'alice@example.com', + approved: '1', + new_account: '1', + ...overrides, + } satisfies CallbackParams +} + +function expectSignedCallbackToVerify(callbackParams: CallbackParams) { + const secret = 'test-secret' + const { sig, ts } = signCallback(callbackParams, secret) + expect(verifyCallback(callbackParams, ts, sig, secret)).toBe(true) +} + +function expectTamperedHandleModeToFail(callbackParams: CallbackParams) { + const secret = 'test-secret' + const { sig, ts } = signCallback(callbackParams, secret) + expect(verifyCallback(callbackParams, ts, sig, secret)).toBe(true) + expect( + verifyCallback( + { ...callbackParams, epds_handle_mode: 'random' }, + ts, + sig, + secret, + ), + ).toBe(false) +} + describe('signCallback / verifyCallback', () => { const secret = 'test-secret-32bytes-padding-here' const params: CallbackParams = { @@ -182,6 +212,7 @@ describe('signCallback / verifyCallback', () => { params.new_account, '', // handle sentinel (absent) '', // client_id sentinel (absent) + '', // epds_handle_mode sentinel (absent) staleTs, ].join('\n') const { createHmac } = await import('node:crypto') @@ -189,6 +220,24 @@ describe('signCallback / verifyCallback', () => { expect(verifyCallback(params, staleTs, staleSig, secret)).toBe(false) }) + it.each([ + { name: 'with handle', handle: 'alice' }, + { name: 'without handle', handle: undefined }, + ])( + 'signs and verifies callback with epds_handle_mode $name', + ({ handle }) => { + expectSignedCallbackToVerify( + makeCallbackParams({ handle, epds_handle_mode: 'picker' }), + ) + }, + ) + + it('rejects tampered epds_handle_mode', () => { + expectTamperedHandleModeToFail( + makeCallbackParams({ epds_handle_mode: 'picker' }), + ) + }) + it('rejects future timestamp', async () => { const futureTs = (Math.floor(Date.now() / 1000) + 60).toString() const payload = [ @@ -198,6 +247,7 @@ describe('signCallback / verifyCallback', () => { params.new_account, '', // handle sentinel (absent) '', // client_id sentinel (absent) + '', // epds_handle_mode sentinel (absent) futureTs, ].join('\n') const { createHmac } = await import('node:crypto') diff --git a/packages/shared/src/crypto.ts b/packages/shared/src/crypto.ts index d1a40092..722eab9a 100644 --- a/packages/shared/src/crypto.ts +++ b/packages/shared/src/crypto.ts @@ -74,15 +74,18 @@ export interface CallbackParams { new_account: string handle?: string // only set for new account creation with chosen handle client_id?: string // OAuth client this flow belongs to; carried only so a clean-exit redirect from the catch block on /oauth/epds-callback can recover the client's redirect_uri when the upstream PAR row is gone. Signed so an attacker cannot redirect a victim's flow at a different OAuth client. + epds_handle_mode?: string } /** * Sign the epds-callback redirect parameters with HMAC-SHA256. * Returns the hex signature and the Unix timestamp (seconds) used. * - * Payload: request_uri, email, approved, new_account, handle (empty when absent), client_id (empty when absent), and ts, joined by newlines. + * Payload: request_uri, email, approved, new_account, handle (empty when absent), + * client_id (empty when absent), epds_handle_mode (empty when absent), and ts, + * joined by newlines. * A timestamp is included so signatures expire (see verifyCallback). - * handle and client_id use empty string as sentinel when absent so existing flows still produce valid signatures and so the payload shape stays stable across releases. + * handle, client_id and epds_handle_mode use empty string as sentinel when absent so existing flows still produce valid signatures and so the payload shape stays stable across releases. */ export function signCallback( params: CallbackParams, @@ -96,6 +99,7 @@ export function signCallback( params.new_account, params.handle ?? '', // empty string when absent params.client_id ?? '', // empty string when absent + params.epds_handle_mode ?? '', // empty string when absent ts, ].join('\n') const sig = crypto.createHmac('sha256', secret).update(payload).digest('hex') @@ -129,6 +133,7 @@ export function verifyCallback( params.new_account, params.handle ?? '', // empty string when absent — matches signCallback sentinel params.client_id ?? '', // empty string when absent — matches signCallback sentinel + params.epds_handle_mode ?? '', // empty string when absent — matches signCallback sentinel ts, ].join('\n') const expected = crypto