diff --git a/.changeset/record-email-as-confirmed.md b/.changeset/record-email-as-confirmed.md new file mode 100644 index 00000000..efc2852d --- /dev/null +++ b/.changeset/record-email-as-confirmed.md @@ -0,0 +1,13 @@ +--- +'ePDS': patch +--- + +Apps you sign in to are now told that your email address is confirmed. + +**Affects:** End users, Client app developers, Operators + +**End users:** Apps no longer ask you to verify an address you have already confirmed with an emailed code. + +**Client app developers:** The `email_verified` claim is now `true` for accounts that signed in through the emailed-code flow, instead of always `false`. + +**Operators:** Deploy the auth service and the PDS together — the signed handover between them carries a new required field, and a mixed pair rejects sign-in until both are updated. An older auth service is rejected with an explicit `Missing or invalid email_verified parameter` rather than a generic signature error, so a mixed-version rollout is recognisable in the logs. Accounts predating this release are repaired the next time their owner signs in **through the emailed-code flow**; a sign-in that does not prove control of the address leaves them unconfirmed. To fix the rest, see "Backfilling Email Confirmation" in `docs/deployment.md`. diff --git a/AGENTS.md b/AGENTS.md index c7d73c39..9e7b83c3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -383,6 +383,14 @@ import { AuthServiceContext } from './context.js' schema and crashes. Leave unused tables/columns in place; they're harmless. - Do **not** directly read or modify `@atproto/pds` database tables — use `pds.ctx.accountManager.*` methods. + - **One documented exception:** enumerating every account in + `packages/pds-core/src/backfill-email-confirmed.ts`, because + `AccountManager` has no "list all accounts" call. Confined to that + operator-invoked script; the request path must never do it. See item 19 + of [`docs/design/pds-white-boxing.md`](docs/design/pds-white-boxing.md) + for the rationale and the breakage scenario — that document is the + catalogue of upstream-internal dependencies, and is the list to check + on every `@atproto/pds` bump. ## Security diff --git a/docs/deployment.md b/docs/deployment.md index 42939a72..d71afc85 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -246,3 +246,52 @@ For test or development environments, you can disable the invite code requirement entirely by setting `PDS_INVITE_REQUIRED=false` on the pds-core service. This allows anyone who can reach the PDS to create accounts, so it is **not recommended for production**. + +## Backfilling Email Confirmation + +Accounts created before email confirmation was recorded at sign-up report +`email_verified: false` to relying parties. New accounts are recorded as +confirmed automatically, and existing accounts are repaired the next time +their owner signs in **through the emailed-code flow** — repair requires a +sign-in that proves control of the address, so an authentication method that +does not (a passkey, say) leaves the account unconfirmed. This backfill is +therefore needed for accounts whose owners have not signed in since the +upgrade, and for any that sign in only by other means. + +Preview first: + +```bash +pnpm --filter @certified-app/pds-core backfill:email-confirmed --dry-run +``` + +Then run it for real: + +```bash +pnpm --filter @certified-app/pds-core backfill:email-confirmed +``` + +Run it with the same environment as the PDS service, so it targets that +deployment's account database. It is idempotent — already-confirmed accounts +are skipped, so re-running is safe. + +A trailing argument scopes the run to addresses containing it +(case-insensitive), which lets you work through a large deployment in batches +or repair a single account: + +```bash +pnpm --filter @certified-app/pds-core backfill:email-confirmed @gmail.com +pnpm --filter @certified-app/pds-core backfill:email-confirmed my.account@yahoo.com +``` + +With no argument, every account is considered. + +Accounts that cannot be confirmed are listed by DID at the end and the command +exits non-zero, so a scripted run does not report success after a partial +failure. + +**This is deliberately not automatic.** It marks every account that has an +email address but no confirmation timestamp. ePDS does not block upstream's +`com.atproto.server.createAccount` XRPC route, so if you provisioned any +accounts outside the normal sign-in flow, their addresses would be marked +confirmed without having been verified. Check the dry-run count before +committing. diff --git a/docs/design/pds-white-boxing.md b/docs/design/pds-white-boxing.md index 9fb8dcaa..71e8f4f2 100644 --- a/docs/design/pds-white-boxing.md +++ b/docs/design/pds-white-boxing.md @@ -57,7 +57,10 @@ The call assumes: - A real password string is **required** — passing `undefined` skips `registerAccount()` internally, leaving the `account` table empty and breaking `upsertDeviceAccount()` FK constraints -- Returns an `Account` object with a `.sub` property (the DID) +- Returns an `Account` object carrying the DID. **This has already broken + once:** the property was `.sub` up to `@atproto/pds` 0.4.x and is `.did` + as of 0.5.23. Call sites were updated in the upgrade; a future rename + would break them again. The `Account` type is not exported, so the return value is typed `any`. @@ -137,6 +140,45 @@ Methods accessed on the PDS-level account manager: `provider.accountManager` (OAuth-provider-level, manages OAuth sessions). The code assumes these are kept in sync by the upstream PDS. +### 19. Direct `account` table read in the email-confirmation backfill + +**File:** `packages/pds-core/src/backfill-email-confirmed.ts` + +```ts +const accounts: BackfillCandidate[] = await pds.ctx.accountManager.db.db + .selectFrom('account') + .select(['did', 'email', 'emailConfirmedAt']) + .execute() +``` + +This reaches past the `accountManager` methods catalogued in item 6 and +queries upstream's SQLite `account` table directly, via the nested +`.db.db` Kysely instance. It is the only place in the codebase that does +so, and it is the documented exception to AGENTS.md's "do not directly +read or modify `@atproto/pds` database tables" rule. + +**Why there is no supported alternative:** as of `@atproto/pds` 0.5.23, +`AccountManager` exposes `getAccount(handleOrDid)`, +`getAccounts(dids)` and `getAccountByEmail(email)` — every one of which +requires knowing the identifier up front. There is no "list all accounts" +query, and a backfill cannot know the DIDs in advance. Adding a wrapper +helper would relocate the same raw query rather than remove it. + +**Scope of the exposure:** confined to an operator-invoked one-off script. +pds-core's request path never does this, and the _writes_ still go through +the public `createEmailToken` / `confirmEmail` pair (see item 6). + +**Breakage scenario:** upstream renames the `account` table or the `did` / +`email` / `emailConfirmedAt` columns, or restructures `AccountDb` so +`accountManager.db.db` is no longer a Kysely instance. The backfill script +fails — loudly, at run time, in an operator's terminal rather than in a +user-facing path. Lowest-consequence failure of anything in this document, +but it fails with no type-checking safety net, since `.db.db` is untyped +at this depth. + +**If upstream ever adds an enumeration API**, switch to it and delete both +this entry and the exception in AGENTS.md's "Database" section. + ### 7. `provider.deviceManager.load()` **File:** `packages/pds-core/src/index.ts` diff --git a/packages/auth-service/src/__tests__/build-epds-callback-url.test.ts b/packages/auth-service/src/__tests__/build-epds-callback-url.test.ts index faf25ab1..b998b245 100644 --- a/packages/auth-service/src/__tests__/build-epds-callback-url.test.ts +++ b/packages/auth-service/src/__tests__/build-epds-callback-url.test.ts @@ -58,6 +58,7 @@ describe('buildEpdsCallbackUrl', () => { flowClientId: CLIENT_ID, email: EMAIL, isNewAccount: false, + emailVerified: true, pdsPublicUrl: PDS_PUBLIC_URL, epdsCallbackSecret: SECRET, }) @@ -72,6 +73,7 @@ describe('buildEpdsCallbackUrl', () => { flowClientId: CLIENT_ID, email: EMAIL, isNewAccount: false, + emailVerified: true, pdsPublicUrl: PDS_PUBLIC_URL, epdsCallbackSecret: SECRET, }) @@ -89,6 +91,7 @@ describe('buildEpdsCallbackUrl', () => { email: EMAIL, approved: '1', new_account: '0', + email_verified: '1', client_id: CLIENT_ID, } expect(verifyCallback(params, ts, sig, SECRET)).toBe(true) @@ -100,6 +103,7 @@ describe('buildEpdsCallbackUrl', () => { flowClientId: CLIENT_ID, email: EMAIL, isNewAccount: true, + emailVerified: true, pdsPublicUrl: PDS_PUBLIC_URL, epdsCallbackSecret: SECRET, }) @@ -110,6 +114,7 @@ describe('buildEpdsCallbackUrl', () => { email: EMAIL, approved: '1', new_account: '1', + email_verified: '1', client_id: CLIENT_ID, } expect( @@ -128,6 +133,7 @@ describe('buildEpdsCallbackUrl', () => { flowClientId: null, email: EMAIL, isNewAccount: false, + emailVerified: true, pdsPublicUrl: PDS_PUBLIC_URL, epdsCallbackSecret: SECRET, }) @@ -140,6 +146,7 @@ describe('buildEpdsCallbackUrl', () => { email: EMAIL, approved: '1', new_account: '0', + email_verified: '1', } expect( verifyCallback( @@ -160,6 +167,7 @@ describe('buildEpdsCallbackUrl', () => { flowClientId: CLIENT_ID, email: EMAIL, isNewAccount: true, + emailVerified: true, pdsPublicUrl: PDS_PUBLIC_URL, epdsCallbackSecret: SECRET, }) @@ -172,6 +180,7 @@ describe('buildEpdsCallbackUrl', () => { email: EMAIL, approved: '1', new_account: '1', + email_verified: '1', client_id: CLIENT_ID, } expect( @@ -190,6 +199,7 @@ describe('buildEpdsCallbackUrl', () => { flowClientId: CLIENT_ID, email: EMAIL, isNewAccount: false, + emailVerified: true, pdsPublicUrl: PDS_PUBLIC_URL, epdsCallbackSecret: SECRET, }) @@ -199,6 +209,7 @@ describe('buildEpdsCallbackUrl', () => { email: EMAIL, approved: '1', new_account: '0', + email_verified: '1', client_id: 'https://attacker.example/client-metadata.json', } expect( @@ -210,4 +221,41 @@ describe('buildEpdsCallbackUrl', () => { ), ).toBe(false) }) + it('forwards emailVerified into the signed callback, both ways', () => { + // The whole point of the field: pds-core records email + // confirmation from this and nothing else, so a flow that did not + // prove control of the address must say so. + for (const [emailVerified, expected] of [ + [true, '1'], + [false, '0'], + ] as const) { + const url = buildEpdsCallbackUrl({ + flowRequestUri: REQUEST_URI, + flowClientId: null, + email: EMAIL, + isNewAccount: true, + emailVerified, + pdsPublicUrl: PDS_PUBLIC_URL, + epdsCallbackSecret: SECRET, + }) + const q = paramsFromUrl(url) + expect(requiredParam(q, 'email_verified')).toBe(expected) + // And it must verify as signed — i.e. the value on the wire is + // the value inside the HMAC, not a decoration alongside it. + expect( + verifyCallback( + { + request_uri: REQUEST_URI, + email: EMAIL, + approved: '1', + new_account: '1', + email_verified: expected, + }, + requiredParam(q, 'ts'), + requiredParam(q, 'sig'), + SECRET, + ), + ).toBe(true) + } + }) }) diff --git a/packages/auth-service/src/__tests__/callback-handle-mode.test.ts b/packages/auth-service/src/__tests__/callback-handle-mode.test.ts index 6d63e114..41391b27 100644 --- a/packages/auth-service/src/__tests__/callback-handle-mode.test.ts +++ b/packages/auth-service/src/__tests__/callback-handle-mode.test.ts @@ -117,6 +117,9 @@ function verifySignedCallbackUrl(url: URL): boolean { email: url.searchParams.get('email') ?? '', approved: url.searchParams.get('approved') ?? '', new_account: url.searchParams.get('new_account') ?? '', + // Required, not spread conditionally: a callback that omitted it + // must fail verification rather than be read as unverified. + email_verified: url.searchParams.get('email_verified') ?? '', ...(url.searchParams.has('handle') ? { handle: url.searchParams.get('handle') ?? '' } : {}), diff --git a/packages/auth-service/src/routes/choose-handle.ts b/packages/auth-service/src/routes/choose-handle.ts index a986ca87..a98a8d59 100644 --- a/packages/auth-service/src/routes/choose-handle.ts +++ b/packages/auth-service/src/routes/choose-handle.ts @@ -67,6 +67,9 @@ export function createChooseHandleRouter( clientId: string | null } email: string + /** Whether this sign-in proved control of `email`; see + * CallbackParams.email_verified. */ + emailVerified: boolean } | null> { // Guard 1: auth_flow cookie const flowId = req.cookies[AUTH_FLOW_COOKIE] as string | undefined @@ -144,7 +147,16 @@ export function createChooseHandleRouter( return null } - return { flowId, flow, email: session.user.email.toLowerCase() } + // Carried alongside the email so the signed callback can state + // whether this sign-in actually proved control of that address. + // better-auth sets emailVerified when the emailed one-time code is + // verified. See CallbackParams.email_verified. + return { + flowId, + flow, + email: session.user.email.toLowerCase(), + emailVerified: session.user.emailVerified === true, + } } // --------------------------------------------------------------------------- @@ -245,7 +257,7 @@ export function createChooseHandleRouter( const result = await getFlowAndSession(req, res) if (!result) return - const { flowId, flow, email } = result + const { flowId, flow, email, emailVerified } = result // Guard: reject flows with handleMode='random' — they should skip the picker entirely if (flow.handleMode === 'random') { @@ -405,6 +417,7 @@ export function createChooseHandleRouter( new_account: '1', handle: normalizedLocal, epds_handle_mode: flow.handleMode ?? '', + email_verified: emailVerified ? '1' : '0', } 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 da5c1c44..5309c00a 100644 --- a/packages/auth-service/src/routes/complete.ts +++ b/packages/auth-service/src/routes/complete.ts @@ -70,6 +70,15 @@ export function buildEpdsCallbackUrl(args: { email: string isNewAccount: boolean flowHandleMode?: string | null + /** + * Whether this sign-in actually proved control of `email` — read + * from the better-auth session, which sets it when the emailed + * one-time code is verified. pds-core records email confirmation + * from this and nothing else, so it must reflect what the + * authenticating flow really established, not what the current + * flow happens to be. See CallbackParams.email_verified. + */ + emailVerified: boolean pdsPublicUrl: string epdsCallbackSecret: string }): string { @@ -78,6 +87,7 @@ export function buildEpdsCallbackUrl(args: { email: args.email, approved: '1', new_account: args.isNewAccount ? '1' : '0', + email_verified: args.emailVerified ? '1' : '0', } if (args.flowClientId) callbackParams.client_id = args.flowClientId if (args.flowHandleMode) callbackParams.epds_handle_mode = args.flowHandleMode @@ -92,9 +102,9 @@ async function resolveCompleteIdentity( ctx: AuthServiceContext, pdsUrl: string, internalSecret: string, -): Promise<{ email: string; did: string | null }> { +): Promise<{ email: string; did: string | null; viaRecovery: boolean }> { const did = await getDidByEmail(email, pdsUrl, internalSecret) - if (did) return { email, did } + if (did) return { email, did, viaRecovery: false } // Recovery path: session email is a backup email, not a primary. Resolve // the backup-email -> DID mapping (auth-service-owned) and then DID -> @@ -106,13 +116,16 @@ async function resolveCompleteIdentity( pdsUrl, internalSecret, ) - if (!recovered) return { email, did: null } + if (!recovered) return { email, did: null, viaRecovery: false } logger.info( { flowId, did: recovered.did }, 'Recovery: translated backup email to primary email via DID', ) - return { email: recovered.email, did: recovered.did } + // Reported so the caller can withhold the email-verified claim: the + // returned address is the account's primary, which this sign-in proved + // nothing about. + return { email: recovered.email, did: recovered.did, viaRecovery: true } } export function createCompleteRouter( @@ -169,6 +182,7 @@ export function createCompleteRouter( }, email: string, flowId: string, + emailVerified: boolean, ): Promise { const ping = await pingParRequest(flow.requestUri, pdsUrl, internalSecret) if (!ping.ok) { @@ -183,6 +197,7 @@ export function createCompleteRouter( email, isNewAccount: true, flowHandleMode: flow.handleMode, + emailVerified, pdsPublicUrl: ctx.config.pdsPublicUrl, epdsCallbackSecret: ctx.config.epdsCallbackSecret, }) @@ -246,8 +261,14 @@ export function createCompleteRouter( const sessionEmail = session.user.email.toLowerCase() + // Whether this sign-in proved control of the address the user + // actually authenticated with. better-auth sets emailVerified when + // the emailed one-time code is verified, so it is the authoritative + // answer for `sessionEmail`. + const sessionEmailVerified: boolean = session.user.emailVerified === true + // Step 4: Check whether this is a new user (no PDS account for email). - const { email, did } = await resolveCompleteIdentity( + const { email, did, viaRecovery } = await resolveCompleteIdentity( sessionEmail, flowId, ctx, @@ -255,13 +276,19 @@ export function createCompleteRouter( internalSecret, ) + // Recovery rebinds `email` from the proved backup address to the + // account's primary. The user proved control of the backup, not the + // primary, so recovering access is not evidence about the address + // being signed here — withhold the claim. + const emailVerified = sessionEmailVerified && !viaRecovery + const isNewAccount = !did if (isNewAccount && flow.handleMode === 'random') { // Step 5a: skip the handle picker, let pds-core call // generateRandomHandle() (signalled by the absent `handle` // field in the signed callback). - await redirectNewUserRandomMode(res, flow, email, flowId) + await redirectNewUserRandomMode(res, flow, email, flowId, emailVerified) return } @@ -290,6 +317,7 @@ export function createCompleteRouter( email, isNewAccount: false, flowHandleMode: flow.handleMode, + emailVerified, pdsPublicUrl: ctx.config.pdsPublicUrl, epdsCallbackSecret: ctx.config.epdsCallbackSecret, }) diff --git a/packages/pds-core/package.json b/packages/pds-core/package.json index 4fd06ad0..afe6e420 100644 --- a/packages/pds-core/package.json +++ b/packages/pds-core/package.json @@ -10,7 +10,9 @@ "scripts": { "build": "tsc --build", "dev": "tsx watch src/index.ts", - "start": "node dist/index.js" + "start": "node dist/index.js", + "backfill:email-confirmed": "node dist/backfill-email-confirmed.js", + "backfill:email-confirmed:dev": "tsx src/backfill-email-confirmed.ts" }, "dependencies": { "@atproto/crypto": "^0.4.5", diff --git a/packages/pds-core/src/__tests__/email-confirmed.test.ts b/packages/pds-core/src/__tests__/email-confirmed.test.ts new file mode 100644 index 00000000..af0f3e26 --- /dev/null +++ b/packages/pds-core/src/__tests__/email-confirmed.test.ts @@ -0,0 +1,380 @@ +import { describe, expect, it, vi } from 'vitest' + +import { + backfillEmailConfirmedAt, + confirmAccountEmail, + formatBackfillReport, + markEmailConfirmed, + matchesEmailFilter, + needsEmailConfirmation, + parseDryRun, + parseEmailFilter, + type EmailConfirmingAccountManager, +} from '../lib/email-confirmed.js' + +const DID = 'did:plc:7iza6de2dwap2sbkpav7c6c6' +const EMAIL = 'alice@example.test' + +/** + * Fake AccountManager recording the mint/redeem pair. Typed against + * the real interface — no casts needed, so a signature change in + * `EmailConfirmingAccountManager` breaks these tests rather than + * silently passing. + */ +function makeAccountManager( + opts: { failOn?: 'createEmailToken' | 'confirmEmail' } = {}, +) { + const calls: { + minted: { did: string; purpose: string }[] + redeemed: { did: string; email: string; token: string }[] + } = { minted: [], redeemed: [] } + + let counter = 0 + + const accountManager: EmailConfirmingAccountManager = { + createEmailToken: (did, purpose) => { + if (opts.failOn === 'createEmailToken') { + return Promise.reject(new Error('db down')) + } + calls.minted.push({ did, purpose }) + return Promise.resolve(`TOKEN-${++counter}`) + }, + confirmEmail: (did, email, token) => { + if (opts.failOn === 'confirmEmail') { + return Promise.reject(new Error('token rejected')) + } + // Upstream rejects a mismatch between this address and the + // account's current one; recorded so tests can assert the + // proved address is what reaches it. + calls.redeemed.push({ did, email, token }) + return Promise.resolve() + }, + } + + return { accountManager, calls } +} + +describe('confirmAccountEmail', () => { + it('redeems the token it just minted, for the same DID', async () => { + const { accountManager, calls } = makeAccountManager() + + await confirmAccountEmail(accountManager, DID, EMAIL) + + expect(calls.minted).toEqual([{ did: DID, purpose: 'confirm_email' }]) + // The token must be the minted one — redeeming anything else + // would leave the freshly created token row behind. + expect(calls.redeemed).toEqual([ + { did: DID, email: EMAIL, token: 'TOKEN-1' }, + ]) + }) + + it('propagates a minting failure without attempting to redeem', async () => { + const { accountManager, calls } = makeAccountManager({ + failOn: 'createEmailToken', + }) + + await expect( + confirmAccountEmail(accountManager, DID, EMAIL), + ).rejects.toThrow('db down') + expect(calls.redeemed).toEqual([]) + }) + + it('propagates a redemption failure', async () => { + const { accountManager } = makeAccountManager({ failOn: 'confirmEmail' }) + + await expect( + confirmAccountEmail(accountManager, DID, EMAIL), + ).rejects.toThrow('token rejected') + }) +}) + +describe('markEmailConfirmed', () => { + it('confirms the account and stays silent on success', async () => { + const { accountManager, calls } = makeAccountManager() + const logger = { error: vi.fn() } + + await markEmailConfirmed({ accountManager, did: DID, email: EMAIL, logger }) + + expect(calls.redeemed).toEqual([ + { did: DID, email: EMAIL, token: 'TOKEN-1' }, + ]) + expect(logger.error).not.toHaveBeenCalled() + }) + + it('swallows failures so sign-in is never blocked, logging at error', async () => { + const { accountManager } = makeAccountManager({ failOn: 'confirmEmail' }) + const logger = { error: vi.fn() } + + // Must resolve, not reject — the user has already proven ownership + // of the address, so bookkeeping failure must not fail their sign-in. + await expect( + markEmailConfirmed({ accountManager, did: DID, email: EMAIL, logger }), + ).resolves.toBeUndefined() + + expect(logger.error).toHaveBeenCalledTimes(1) + const [context] = logger.error.mock.calls[0] + expect(context).toMatchObject({ did: DID }) + expect((context as { err: Error }).err.message).toBe('token rejected') + }) +}) + +describe('needsEmailConfirmation', () => { + it('is true for an account with an address and no confirmation', () => { + expect( + needsEmailConfirmation({ email: 'a@x.test', emailConfirmedAt: null }), + ).toBe(true) + }) + + it('is false once confirmed, so re-runs and sign-ins skip the work', () => { + expect( + needsEmailConfirmation({ + email: 'a@x.test', + emailConfirmedAt: '2026-08-04T10:00:00.000Z', + }), + ).toBe(false) + }) + + it('is false when there is no address to confirm', () => { + // The column is NOT NULL in the PDS schema, so "no address" + // arrives as the empty string; null is covered as insurance + // against that constraint being relaxed upstream. + expect(needsEmailConfirmation({ email: '', emailConfirmedAt: null })).toBe( + false, + ) + expect( + needsEmailConfirmation({ email: null, emailConfirmedAt: null }), + ).toBe(false) + }) + + it('is false for a missing account', () => { + expect(needsEmailConfirmation(null)).toBe(false) + }) +}) + +describe('backfillEmailConfirmedAt', () => { + const UNCONFIRMED = [ + { did: 'did:plc:aaa', email: 'a@x.test', emailConfirmedAt: null }, + { did: 'did:plc:bbb', email: 'b@x.test', emailConfirmedAt: null }, + ] + const CONFIRMED = { + did: 'did:plc:ccc', + email: 'c@x.test', + emailConfirmedAt: '2026-08-04T10:00:00.000Z', + } + const NO_EMAIL = { did: 'did:plc:ddd', email: '', emailConfirmedAt: null } + + it('confirms only the accounts that need it', async () => { + const { accountManager, calls } = makeAccountManager() + + const result = await backfillEmailConfirmedAt({ + accountManager, + accounts: [...UNCONFIRMED, CONFIRMED, NO_EMAIL], + }) + + expect(result).toMatchObject({ candidates: 2, updated: 2, failed: 0 }) + expect(calls.redeemed.map((r) => r.did)).toEqual([ + 'did:plc:aaa', + 'did:plc:bbb', + ]) + }) + + it('confirms only accounts matching the email filter', async () => { + const { accountManager, calls } = makeAccountManager() + + const result = await backfillEmailConfirmedAt({ + accountManager, + accounts: [ + { did: 'did:plc:aaa', email: 'a@gmail.com', emailConfirmedAt: null }, + { did: 'did:plc:bbb', email: 'b@yahoo.com', emailConfirmedAt: null }, + ], + emailFilter: '@gmail.com', + }) + + expect(result).toMatchObject({ candidates: 1, updated: 1, failed: 0 }) + expect(calls.redeemed.map((r) => r.did)).toEqual(['did:plc:aaa']) + }) + + it('reports candidates without writing when dryRun is set', async () => { + const { accountManager, calls } = makeAccountManager() + + const result = await backfillEmailConfirmedAt({ + accountManager, + accounts: [...UNCONFIRMED, CONFIRMED], + dryRun: true, + }) + + expect(result).toEqual({ + candidates: 2, + updated: 0, + failed: 0, + failures: [], + dryRun: true, + }) + expect(calls.minted).toEqual([]) + }) + + it('is a no-op when every account is already confirmed', async () => { + const { accountManager, calls } = makeAccountManager() + + const result = await backfillEmailConfirmedAt({ + accountManager, + accounts: [CONFIRMED], + }) + + expect(result).toMatchObject({ candidates: 0, updated: 0, failed: 0 }) + expect(calls.minted).toEqual([]) + }) + + it('keeps going after a failure and reports which accounts failed', async () => { + // One unconfirmable account must not strand the rest of the run, + // and the operator needs the DIDs to chase them up. + let attempt = 0 + const accountManager: EmailConfirmingAccountManager = { + createEmailToken: () => Promise.resolve('TOKEN'), + confirmEmail: () => { + attempt++ + return attempt === 1 + ? Promise.reject(new Error('account deactivated')) + : Promise.resolve() + }, + } + + const result = await backfillEmailConfirmedAt({ + accountManager, + accounts: UNCONFIRMED, + }) + + expect(result).toMatchObject({ candidates: 2, updated: 1, failed: 1 }) + expect(result.failures).toEqual([ + { did: 'did:plc:aaa', error: 'account deactivated' }, + ]) + }) +}) + +describe('matchesEmailFilter', () => { + it('matches everything when no filter is given', () => { + // "no filter" must mean all accounts, never none — otherwise a + // mistyped invocation would do nothing and look like a clean run. + expect(matchesEmailFilter('a@x.test', undefined)).toBe(true) + expect(matchesEmailFilter('a@x.test', '')).toBe(true) + }) + + it('matches on a domain substring, case-insensitively', () => { + expect(matchesEmailFilter('someone@gmail.com', '@gmail.com')).toBe(true) + expect(matchesEmailFilter('Someone@GMAIL.com', '@gmail.com')).toBe(true) + expect(matchesEmailFilter('someone@yahoo.com', '@gmail.com')).toBe(false) + }) + + it('matches a single full address', () => { + expect( + matchesEmailFilter('my.account@yahoo.com', 'my.account@yahoo.com'), + ).toBe(true) + expect( + matchesEmailFilter('other.account@yahoo.com', 'my.account@yahoo.com'), + ).toBe(false) + }) + + it('never matches an account with no address once a filter is set', () => { + expect(matchesEmailFilter('', '@gmail.com')).toBe(false) + expect(matchesEmailFilter(null, '@gmail.com')).toBe(false) + }) +}) + +describe('parseEmailFilter', () => { + it('is undefined when only flags are passed', () => { + expect(parseEmailFilter(['node', 'backfill.ts'])).toBeUndefined() + expect( + parseEmailFilter(['node', 'backfill.ts', '--dry-run']), + ).toBeUndefined() + }) + + it('takes the first non-flag argument, in any position', () => { + expect(parseEmailFilter(['node', 'backfill.ts', '@gmail.com'])).toBe( + '@gmail.com', + ) + expect( + parseEmailFilter(['node', 'backfill.ts', '--dry-run', '@gmail.com']), + ).toBe('@gmail.com') + expect( + parseEmailFilter(['node', 'backfill.ts', '@gmail.com', '--dry-run']), + ).toBe('@gmail.com') + }) +}) + +describe('parseDryRun', () => { + it('is off unless --dry-run is passed', () => { + expect(parseDryRun(['node', 'backfill.ts'])).toBe(false) + // A near-miss must not be treated as the flag: writing when the + // operator meant to preview is the one unrecoverable mistake here. + expect(parseDryRun(['node', 'backfill.ts', '--dry'])).toBe(false) + expect(parseDryRun(['node', 'backfill.ts', 'dry-run'])).toBe(false) + }) + + it('is on when --dry-run is passed, in any position', () => { + expect(parseDryRun(['node', 'backfill.ts', '--dry-run'])).toBe(true) + expect(parseDryRun(['--dry-run', 'node', 'backfill.ts'])).toBe(true) + }) +}) + +describe('formatBackfillReport', () => { + const LOCATION = '/data/account.sqlite' + const base = { failed: 0, failures: [], dryRun: false } + + it('reports the candidate count and names the db on a dry run', () => { + const line = formatBackfillReport( + { ...base, candidates: 3, updated: 0, dryRun: true }, + LOCATION, + ) + + expect(line).toBe( + `[dry run] 3 account(s) in ${LOCATION} would be marked email-confirmed.`, + ) + }) + + it('reports what was actually written on a real run', () => { + const line = formatBackfillReport( + { ...base, candidates: 3, updated: 3 }, + LOCATION, + ) + + expect(line).toBe(`Marked 3 account(s) in ${LOCATION} as email-confirmed.`) + }) + + it('makes a no-op run unambiguous rather than silent', () => { + expect( + formatBackfillReport({ ...base, candidates: 0, updated: 0 }, LOCATION), + ).toContain('0 account(s)') + }) + + it('names the filter so a scoped run is distinguishable from a full one', () => { + // "0 account(s)" is ambiguous otherwise: nothing left to do, or a + // filter that matched nothing? + expect( + formatBackfillReport( + { ...base, candidates: 0, updated: 0, dryRun: true }, + LOCATION, + '@gmail.com', + ), + ).toContain('matching "@gmail.com"') + expect( + formatBackfillReport({ ...base, candidates: 2, updated: 2 }, LOCATION), + ).not.toContain('matching') + }) + + it('names the accounts that failed so the operator can chase them', () => { + const line = formatBackfillReport( + { + candidates: 2, + updated: 1, + failed: 1, + failures: [{ did: 'did:plc:aaa', error: 'account deactivated' }], + dryRun: false, + }, + LOCATION, + ) + + expect(line).toContain('Marked 1 account(s)') + expect(line).toContain('1 account(s) could not be confirmed') + expect(line).toContain('did:plc:aaa: account deactivated') + }) +}) diff --git a/packages/pds-core/src/backfill-email-confirmed.ts b/packages/pds-core/src/backfill-email-confirmed.ts new file mode 100644 index 00000000..1defcee2 --- /dev/null +++ b/packages/pds-core/src/backfill-email-confirmed.ts @@ -0,0 +1,80 @@ +/** + * One-off operator script: record email confirmation for ePDS + * accounts created before that was done at sign-up. + * + * Run it against a deployment's account database, with the same + * environment the server runs with: + * + * pnpm --filter @certified-app/pds-core backfill:email-confirmed --dry-run + * pnpm --filter @certified-app/pds-core backfill:email-confirmed + * + * A trailing argument scopes the run to addresses containing it + * (case-insensitive), so an operator can work through a deployment in + * batches or fix up a single account: + * + * pnpm --filter @certified-app/pds-core backfill:email-confirmed --dry-run @gmail.com + * pnpm --filter @certified-app/pds-core backfill:email-confirmed my.account@yahoo.com + * + * Not wired into startup on purpose — see the rationale on + * `backfillEmailConfirmedAt`. + * + * Builds the PDS via the same `PDS.create()` the server uses so the + * database location, migrations and connection settings come from the + * deployment's own config. `create()` wires up the context without + * binding a port; `start()` is never called, so nothing is served. + * + * Confirmation itself goes through `accountManager.createEmailToken` / + * `confirmEmail`, per AGENTS.md's account-manager boundary. Enumerating + * the accounts to consider is the one read this script does directly, + * under the documented exception to that rule — see "Database" in + * AGENTS.md, and item 19 of docs/design/pds-white-boxing.md for what + * breaks it on an upstream bump. + */ +import { PDS, envToCfg, envToSecrets, readEnv } from '@atproto/pds' +import { createLogger } from '@certified-app/shared' +import { + backfillEmailConfirmedAt, + formatBackfillReport, + parseDryRun, + parseEmailFilter, + type BackfillCandidate, +} from './lib/email-confirmed.js' + +const logger = createLogger('pds-core:backfill-email-confirmed') + +async function main(): Promise { + const dryRun = parseDryRun(process.argv) + const emailFilter = parseEmailFilter(process.argv) + + const env = readEnv() + const cfg = envToCfg(env) + const secrets = envToSecrets(env) + + const pds = await PDS.create(cfg, secrets) + try { + const accounts: BackfillCandidate[] = await pds.ctx.accountManager.db.db + .selectFrom('account') + .select(['did', 'email', 'emailConfirmedAt']) + .execute() + + const result = await backfillEmailConfirmedAt({ + accountManager: pds.ctx.accountManager, + accounts, + emailFilter, + dryRun, + }) + process.stdout.write( + formatBackfillReport(result, cfg.db.accountDbLoc, emailFilter) + '\n', + ) + // Surface partial failure to the shell so a scripted run does not + // report success when some accounts could not be confirmed. + if (result.failed > 0) process.exitCode = 1 + } finally { + await pds.destroy() + } +} + +main().catch((err: unknown) => { + logger.error({ err }, 'Email-confirmation backfill failed') + process.exit(1) +}) diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index 7f4043f6..a101b8d2 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -72,6 +72,10 @@ import { createUpstreamFaviconMiddleware } from './upstream-favicon.js' import { createAuthUiGuard, parsePromptTokens } from './auth-ui-guard.js' import { loadDeviceAccountEmails } from './lib/device-accounts.js' import { handleCallbackError } from './lib/epds-callback-error.js' +import { + markEmailConfirmed, + needsEmailConfirmation, +} from './lib/email-confirmed.js' import { installTestHooks } from './lib/test-hooks.js' import { buildPostCallbackAuthorizeUrl } from './lib/epds-callback-authorize.js' @@ -218,9 +222,33 @@ async function main() { const approvedStr = req.query.approved as string const newAccountStr = req.query.new_account as string + // Whether auth-service is claiming this sign-in proved control of + // `email`. Part of the HMAC payload, so it cannot be forged or + // stripped: a callback without it fails verifyCallback outright. + const emailVerifiedStr = req.query.email_verified 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 + + // Reject a missing or malformed email_verified before signature + // verification. The signature check would catch it anyway — the + // field is inside the HMAC — but it would surface as "Invalid + // callback signature", which sends an operator hunting for a + // secret mismatch during what is actually a mixed-version rollout + // (an auth-service too old to send the field). Say what is + // actually wrong instead. + if (emailVerifiedStr !== '0' && emailVerifiedStr !== '1') { + logger.warn( + { emailVerified: emailVerifiedStr }, + 'epds-callback missing or invalid email_verified — auth-service too old, or a hand-built callback', + ) + res.status(400).json({ + error: + 'Missing or invalid email_verified parameter (expected "0" or "1")', + }) + return + } + const signatureValid = verifyCallback( { request_uri: requestUri, @@ -230,6 +258,7 @@ async function main() { handle: handleParam, client_id: clientIdParam, epds_handle_mode: handleModeParam, + email_verified: emailVerifiedStr, }, ts, sig, @@ -486,6 +515,45 @@ async function main() { } await provider.accountManager.upsertDeviceAccount(deviceId, account.did) + // Step 4b: Record that the email is confirmed, but only when + // auth-service explicitly claims this sign-in proved control of + // `email`. Upstream only records confirmation via confirmEmail()'s + // token flow, so without this the address stays marked unverified + // forever. + // + // The claim is read from the signed callback rather than inferred + // from "a valid callback arrived". Only the authenticating service + // knows *how* the user authenticated: today that is always an + // emailed one-time code, but a passkey or similar flow would + // legitimately send a signed callback carrying `email` merely to + // locate the account, having proved nothing about that address. + // Inferring verification here would assert `email_verified: true` + // to relying parties on no evidence, and would engage the + // email-change verification gate on an unproven address. + // + // Keyed on whether the account is *confirmed*, not on whether it + // is new: a returning user whose earlier confirmation failed + // (or who predates this code) is repaired on their next sign-in + // rather than waiting for the backfill script. Already-confirmed + // accounts skip it, so the common case costs no writes. + // Best-effort: never fails the sign-in. + const emailProven = emailVerifiedStr === '1' + if ( + emailProven && + (!existingAccount || needsEmailConfirmation(existingAccount)) + ) { + await markEmailConfirmed({ + accountManager: pds.ctx.accountManager, + did: account.did, + // The HMAC-signed address whose control was proved — not the + // account's stored address. Upstream compares the two and + // rejects a mismatch, so an address changed since the flow + // began is not recorded as confirmed on this evidence. + email, + logger, + }) + } + // Step 5: Determine whether to skip consent on sign-up. // Consent is skipped only when ALL of these hold: // a) This is a brand-new account (not an existing user) diff --git a/packages/pds-core/src/lib/email-confirmed.ts b/packages/pds-core/src/lib/email-confirmed.ts new file mode 100644 index 00000000..986a96a9 --- /dev/null +++ b/packages/pds-core/src/lib/email-confirmed.ts @@ -0,0 +1,294 @@ +/** + * Marks ePDS accounts as email-confirmed. + * + * Every ePDS account is created by /oauth/epds-callback, which only + * runs after auth-service has verified a one-time code sent to that + * address. The email is therefore *already* verified by the time the + * account row exists — but the PDS `account` table's + * `emailConfirmedAt` column stays null, because upstream only ever + * populates it from `confirmEmail()`, which consumes a + * `confirm_email` token that the OTP flow never mints. + * + * Leaving it null has two observable consequences: + * 1. `email_verified` is false in every OIDC/token claim, because + * upstream's oauth-store derives it as `emailConfirmedAt != null`. + * Relying parties see a verified address reported as unverified. + * 2. Upstream's `requestEmailUpdate` only demands a confirmation + * token when `emailConfirmedAt` is set, so a null value skips + * the verification gate on email change entirely. + * + * ## Why mint-then-redeem rather than writing the column + * + * AGENTS.md: "Do not directly read or modify `@atproto/pds` database + * tables — use `pds.ctx.accountManager.*` methods." `createEmailToken` + * and `confirmEmail` are both public `AccountManager` methods, and + * together they are exactly the supported route to a confirmed email: + * `confirmEmail` validates the token, deletes it, and sets + * `emailConfirmedAt` in a single transaction, so no token row is left + * behind. + * + * `createEmailToken` only inserts the row and returns the token — + * upstream's XRPC handlers do the mailing separately, so nothing is + * sent to the user here. That matters: the user already proved + * ownership of this address via the OTP, and a second unexpected mail + * would be worse than the bug being fixed. + * + * Lives in its own module so the flow can be unit-tested against a + * fake account manager without booting a real PDS, matching the + * extraction pattern used by the other lib/ modules. + */ +import type { Logger } from 'pino' +import type { DidString } from '@atproto/syntax' + +/** + * The slice of `AccountManager` this module needs. Structurally + * compatible with the real class, so the call sites pass + * `pds.ctx.accountManager` directly and the fakes in tests stay + * honest without importing PDS internals. + */ +export interface EmailConfirmingAccountManager { + createEmailToken: ( + did: DidString, + purpose: 'confirm_email', + ) => Promise + confirmEmail: ( + did: DidString, + email: string, + token: string, + ) => Promise +} + +/** + * Confirm a single account's email address. + * + * Mints a `confirm_email` token and immediately redeems it. Both + * halves are public `AccountManager` operations; the token never + * leaves this function and is deleted by `confirmEmail` as part of + * the same transaction that records the confirmation. + * + * `email` is the address whose control was actually proved. Upstream + * compares it against the account's current address and throws + * `InvalidEmail` if they differ, so an address that changed between + * our check and this call is rejected rather than silently recorded + * as confirmed. + */ +export async function confirmAccountEmail( + accountManager: EmailConfirmingAccountManager, + did: DidString, + email: string, +): Promise { + const token = await accountManager.createEmailToken(did, 'confirm_email') + await accountManager.confirmEmail(did, email, token) +} + +/** + * Best-effort variant used on the sign-in hot path. + * + * The email genuinely *is* verified at this point, so recording that + * fact is correct — but it is bookkeeping, not part of the sign-in + * contract. If it fails (SQLite busy, disk error) the user has still + * proven ownership of the address, so failing their sign-in over it + * would be a strictly worse outcome than a stale + * `email_verified: false` claim. The next sign-in that proves control + * of the address retries — callers skip already-confirmed accounts, + * not already-*seen* ones. Self-healing therefore depends on the owner + * signing in through the emailed-code flow again; an account whose + * owner only ever uses another method stays unconfirmed until the + * operator backfill runs. + * + * Logged at `error`, not `warn`: swallowing the exception keeps the + * user signed in, but the account is left claiming an unverified + * address to every relying party until some later sign-in happens to + * succeed — and the operator has no other signal that it happened. + * Self-healing is not the same as harmless, so this should reach + * error-level alerting rather than sit quietly in the logs. + */ +export async function markEmailConfirmed(opts: { + accountManager: EmailConfirmingAccountManager + did: DidString + /** The address whose control this sign-in proved. */ + email: string + logger: Pick +}): Promise { + try { + await confirmAccountEmail(opts.accountManager, opts.did, opts.email) + } catch (err) { + opts.logger.error( + { err, did: opts.did }, + 'Failed to record email confirmation after OTP-verified sign-in', + ) + } +} + +/** An account as far as the backfill is concerned. */ +export interface BackfillCandidate { + did: string + email?: string | null + emailConfirmedAt?: string | null +} + +/** True when this account has a real address that is not yet confirmed. */ +export function needsEmailConfirmation( + account: Pick | null, +): boolean { + if (!account) return false + // An account with no address has nothing to confirm — upstream + // reports `email_verified` as undefined rather than false for those. + if (!account.email) return false + return !account.emailConfirmedAt +} + +/** + * Case-insensitive substring match on the address, so an operator can + * scope a backfill run to a domain (`@gmail.com`) or a single account + * (`my.account@yahoo.com`) instead of the whole table. + * + * An empty or absent filter matches everything — "no filter given" + * must mean "all accounts", never "none", or a mistyped invocation + * would silently do nothing and look like a clean run. + */ +export function matchesEmailFilter( + email: string | null | undefined, + filter?: string, +): boolean { + if (!filter) return true + if (!email) return false + return email.toLowerCase().includes(filter.toLowerCase()) +} + +/** + * The first non-flag argument, treated as the address filter. + * Returns undefined when the operator passed only flags. + */ +export function parseEmailFilter(argv: readonly string[]): string | undefined { + // argv[0] is the node binary and argv[1] the script path; anything + // further that is not a flag is the filter. + return argv.slice(2).find((a) => !a.startsWith('-')) +} + +export interface BackfillResult { + /** Accounts found needing confirmation. */ + candidates: number + /** Accounts confirmed successfully. */ + updated: number + /** Accounts that failed to confirm; see `failures`. */ + failed: number + /** DIDs that failed, with the reason, for the operator to chase. */ + failures: { did: string; error: string }[] + /** True when no write was attempted. */ + dryRun: boolean +} + +/** + * One-off backfill for accounts created before this fix landed, whose + * email was verified by OTP but never recorded as confirmed. + * + * Deliberately NOT run automatically at startup. Whether a null + * `emailConfirmedAt` means "verified via OTP but never recorded" or + * "genuinely never verified" depends on how a given deployment was + * operated — ePDS does not block upstream's + * `com.atproto.server.createAccount` XRPC route, so an operator who + * provisioned accounts by other means must not have those addresses + * silently promoted to verified. Only the operator knows which case + * applies, so this is exposed as a script they choose to run. + * + * Confirms one account at a time via the public API rather than a + * single set-based UPDATE. That is more round-trips, but it keeps to + * the account-manager boundary and lets one bad row be reported + * without abandoning the rest of the run. + * + * Idempotent — already-confirmed accounts are skipped, so re-running + * is a no-op. Use `dryRun` to report the candidate count without + * writing. + */ +export async function backfillEmailConfirmedAt(opts: { + accountManager: EmailConfirmingAccountManager + /** Every account to consider, typically the full account list. */ + accounts: readonly BackfillCandidate[] + /** + * Optional case-insensitive substring match on the address, so a run + * can be scoped to a domain or a single account. Absent means all. + */ + emailFilter?: string + dryRun?: boolean +}): Promise { + const dryRun = opts.dryRun ?? false + const candidates = opts.accounts.filter( + (a) => + needsEmailConfirmation(a) && + matchesEmailFilter(a.email, opts.emailFilter), + ) + + if (dryRun || candidates.length === 0) { + return { + candidates: candidates.length, + updated: 0, + failed: 0, + failures: [], + dryRun, + } + } + + let updated = 0 + const failures: { did: string; error: string }[] = [] + for (const account of candidates) { + try { + // needsEmailConfirmation() already excluded blank addresses, so + // this only narrows the type. Upstream compares the address we + // pass against the account's current one and rejects a mismatch, + // so a row whose email changed between the scan above and this + // call is reported as a failure rather than confirmed. + if (!account.email) continue + await confirmAccountEmail( + opts.accountManager, + account.did as DidString, + account.email, + ) + updated++ + } catch (err) { + // One unconfirmable account must not strand the rest of the + // run; collect and report instead. + failures.push({ + did: account.did, + error: err instanceof Error ? err.message : String(err), + }) + } + } + + return { + candidates: candidates.length, + updated, + failed: failures.length, + failures, + dryRun, + } +} + +/** True when the operator asked to preview rather than write. */ +export function parseDryRun(argv: readonly string[]): boolean { + return argv.includes('--dry-run') +} + +/** + * The line the backfill script prints on completion. Split out from + * the script so the wording is covered by tests — the script itself + * is an entry point and never imported by one. + */ +export function formatBackfillReport( + result: BackfillResult, + location: string, + emailFilter?: string, +): string { + // Name the filter in the output: a scoped run and an + // everything-matched run otherwise look identical, and an operator + // reading "0 account(s)" needs to know whether that means "none left + // to do" or "your filter matched nothing". + const scope = emailFilter ? `${location} matching "${emailFilter}"` : location + if (result.dryRun) { + return `[dry run] ${result.candidates} account(s) in ${scope} would be marked email-confirmed.` + } + const base = `Marked ${result.updated} account(s) in ${scope} as email-confirmed.` + if (result.failed === 0) return base + const detail = result.failures.map((f) => ` ${f.did}: ${f.error}`).join('\n') + return `${base}\n${result.failed} account(s) could not be confirmed:\n${detail}` +} diff --git a/packages/shared/src/__tests__/crypto.test.ts b/packages/shared/src/__tests__/crypto.test.ts index cd2a57c1..a2e288e8 100644 --- a/packages/shared/src/__tests__/crypto.test.ts +++ b/packages/shared/src/__tests__/crypto.test.ts @@ -137,6 +137,8 @@ function makeCallbackParams(overrides: Partial = {}) { email: 'alice@example.com', approved: '1', new_account: '1', + // Required, so it has no sentinel — see CallbackParams.email_verified. + email_verified: '1', ...overrides, } satisfies CallbackParams } @@ -168,6 +170,7 @@ describe('signCallback / verifyCallback', () => { email: 'user@example.com', approved: '1', new_account: '0', + email_verified: '1', } it('produces a hex signature and numeric timestamp string', () => { @@ -213,6 +216,7 @@ describe('signCallback / verifyCallback', () => { '', // handle sentinel (absent) '', // client_id sentinel (absent) '', // epds_handle_mode sentinel (absent) + params.email_verified, staleTs, ].join('\n') const { createHmac } = await import('node:crypto') @@ -248,6 +252,7 @@ describe('signCallback / verifyCallback', () => { '', // handle sentinel (absent) '', // client_id sentinel (absent) '', // epds_handle_mode sentinel (absent) + params.email_verified, futureTs, ].join('\n') const { createHmac } = await import('node:crypto') @@ -274,6 +279,7 @@ describe('signCallback / verifyCallback with handle', () => { email: 'alice@example.com', approved: '1', new_account: '1', + email_verified: '1', handle: 'alice.pds.example.com', } const { sig, ts } = signCallback(params, secret) @@ -287,6 +293,7 @@ describe('signCallback / verifyCallback with handle', () => { email: 'alice@example.com', approved: '1', new_account: '1', + email_verified: '1', } const { sig, ts } = signCallback(params, secret) expect(verifyCallback(params, ts, sig, secret)).toBe(true) @@ -299,6 +306,7 @@ describe('signCallback / verifyCallback with handle', () => { email: 'alice@example.com', approved: '1', new_account: '1', + email_verified: '1', } // Sign without handle, then verify that adding a handle breaks the signature. // This proves handle is included in the HMAC payload without relying on two @@ -321,6 +329,7 @@ describe('signCallback / verifyCallback with handle', () => { email: 'alice@example.com', approved: '1', new_account: '1', + email_verified: '1', } const withUndefined: CallbackParams = { ...baseParams, handle: undefined } const { sig, ts } = signCallback(baseParams, secret) @@ -339,6 +348,7 @@ describe('signCallback / verifyCallback with handle', () => { email: 'alice@example.com', approved: '1', new_account: '1', + email_verified: '1', handle: 'alice.pds.example.com', } const { sig, ts } = signCallback(params, secret) @@ -360,6 +370,7 @@ describe('signCallback / verifyCallback with client_id', () => { email: 'alice@example.com', approved: '1', new_account: '0', + email_verified: '1', client_id: 'https://demo.example.com/client-metadata.json', } const { sig, ts } = signCallback(params, secret) @@ -372,6 +383,7 @@ describe('signCallback / verifyCallback with client_id', () => { email: 'alice@example.com', approved: '1', new_account: '0', + email_verified: '1', client_id: 'https://demo.example.com/client-metadata.json', } const { sig, ts } = signCallback(params, secret) @@ -392,6 +404,7 @@ describe('signCallback / verifyCallback with client_id', () => { email: 'alice@example.com', approved: '1', new_account: '0', + email_verified: '1', } const withUndefined: CallbackParams = { ...baseParams, @@ -403,12 +416,54 @@ describe('signCallback / verifyCallback with client_id', () => { expect(verifyCallback(baseParams, ts2, sig2, secret)).toBe(true) }) + const emailVerifiedParams: CallbackParams = { + request_uri: 'urn:ietf:params:oauth:request_uri:test', + email: 'alice@example.com', + approved: '1', + new_account: '0', + email_verified: '1', + } + + it('a caller that omits email_verified is rejected rather than read as verified', () => { + // The field is required, not sentinel-defaulted like handle / + // client_id. A future sign-in flow whose author forgets to set it + // signs a different payload and fails at the trust boundary — the + // one failure mode we can afford, versus silently asserting + // verification that never happened. + const omitted = { + request_uri: emailVerifiedParams.request_uri, + email: emailVerifiedParams.email, + approved: emailVerifiedParams.approved, + new_account: emailVerifiedParams.new_account, + } as unknown as CallbackParams + const { sig, ts } = signCallback(omitted, secret) + + expect(verifyCallback(emailVerifiedParams, ts, sig, secret)).toBe(false) + }) + + it('email_verified is covered by the signature, so it cannot be flipped in transit', () => { + // pds-core records email confirmation from this field alone. If it + // were outside the HMAC, anyone holding a callback URL could + // upgrade '0' to '1' and have an unproven address marked verified. + const unverified: CallbackParams = { + ...emailVerifiedParams, + email_verified: '0', + } + const { sig, ts } = signCallback(unverified, secret) + + expect(verifyCallback(unverified, ts, sig, secret)).toBe(true) + expect( + verifyCallback({ ...unverified, email_verified: '1' }, ts, sig, secret), + ).toBe(false) + }) + it('a sig produced WITH a client_id does not verify WITHOUT one', () => { const withClient: CallbackParams = { request_uri: 'urn:ietf:params:oauth:request_uri:test', email: 'alice@example.com', approved: '1', new_account: '0', + email_verified: '1', client_id: 'https://demo.example.com/client-metadata.json', } const { sig, ts } = signCallback(withClient, secret) @@ -417,6 +472,7 @@ describe('signCallback / verifyCallback with client_id', () => { email: withClient.email, approved: withClient.approved, new_account: withClient.new_account, + email_verified: withClient.email_verified, } expect(verifyCallback(withoutClient, ts, sig, secret)).toBe(false) }) diff --git a/packages/shared/src/crypto.ts b/packages/shared/src/crypto.ts index 722eab9a..d9b466ea 100644 --- a/packages/shared/src/crypto.ts +++ b/packages/shared/src/crypto.ts @@ -75,6 +75,28 @@ export interface CallbackParams { 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 + /** + * '1' when the sign-in that produced this callback actually proved + * control of `email`; '0' otherwise. + * + * pds-core records email confirmation on the PDS account from this + * flag alone. It exists because only the authenticating service + * knows *how* the user authenticated: today every callback follows + * an emailed one-time code, but a future passkey or similar flow + * would legitimately send a signed callback carrying `email` merely + * to locate the account, with nobody having proved control of that + * address. Without an explicit claim, pds-core would have to infer + * verification from the mere existence of a valid signature and + * would mark such an address confirmed — asserting + * `email_verified: true` to relying parties on no evidence. + * + * Deliberately REQUIRED rather than optional: the signed payload is + * positional, so a caller that omits it produces a different + * payload and its callback is rejected as an invalid signature. A + * future flow that forgets to set it therefore fails loudly at the + * trust boundary instead of silently claiming verification. + */ + email_verified: string } /** @@ -82,10 +104,15 @@ export interface CallbackParams { * 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), epds_handle_mode (empty when absent), and ts, - * joined by newlines. + * client_id (empty when absent), epds_handle_mode (empty when absent), + * email_verified, and ts, joined by newlines. * A timestamp is included so signatures expire (see verifyCallback). - * 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. + * handle, client_id and epds_handle_mode use empty string as sentinel when absent, so a flow that does not set them still produces a valid signature. + * email_verified has no sentinel — it is required, so a caller that omits it signs a different payload and is rejected rather than being read as verified. + * Adding a field to this payload is a breaking change for the handover: both + * signer and verifier must be deployed together, since either side alone + * computes a different HMAC. See the rollout note in the changeset that + * introduced email_verified. */ export function signCallback( params: CallbackParams, @@ -100,6 +127,7 @@ export function signCallback( params.handle ?? '', // empty string when absent params.client_id ?? '', // empty string when absent params.epds_handle_mode ?? '', // empty string when absent + params.email_verified, // required — no sentinel, see the note above ts, ].join('\n') const sig = crypto.createHmac('sha256', secret).update(payload).digest('hex') @@ -134,6 +162,7 @@ export function verifyCallback( 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 + params.email_verified, // required — matches signCallback, no sentinel ts, ].join('\n') const expected = crypto