diff --git a/.claude/settings.local.json b/.claude/settings.local.json new file mode 100644 index 00000000000..72a3e34f7ac --- /dev/null +++ b/.claude/settings.local.json @@ -0,0 +1,8 @@ +{ + "permissions": { + "allow": [ + "Bash(node:*)", + "Bash(head -5 grep -r \"require\\\\|import\" /Users/dschomburg/Code/fxa/node_modules/@simplewebauthn/server/esm/registration/verifyRegistrationResponse.js)" + ] + } +} diff --git a/libs/accounts/passkey/src/lib/passkey.challenge.manager.in.spec.ts b/libs/accounts/passkey/src/lib/passkey.challenge.manager.in.spec.ts index 9aca2519d88..214d6e82f7b 100644 --- a/libs/accounts/passkey/src/lib/passkey.challenge.manager.in.spec.ts +++ b/libs/accounts/passkey/src/lib/passkey.challenge.manager.in.spec.ts @@ -10,6 +10,7 @@ import { StatsDService } from '@fxa/shared/metrics/statsd'; import { PasskeyChallengeManager } from './passkey.challenge.manager'; import { PasskeyConfig } from './passkey.config'; import { PASSKEY_CHALLENGE_REDIS } from './passkey.provider'; +import { generateRandomChallenge } from './webauthn-adapter'; const mockLogger = { log: jest.fn(), @@ -73,9 +74,10 @@ afterAll(async () => { }); describe('PasskeyChallengeManager (integration)', () => { - describe('generateRegistrationChallenge', () => { + describe('storeRegistrationChallenge', () => { it('stores challenge with uid and type=registration', async () => { - const challenge = await manager.generateRegistrationChallenge('deadbeef'); + const challenge = generateRandomChallenge(); + await manager.storeRegistrationChallenge(challenge, 'deadbeef'); const stored = await manager.consumeRegistrationChallenge( challenge, @@ -90,7 +92,8 @@ describe('PasskeyChallengeManager (integration)', () => { describe('generateAuthenticationChallenge', () => { it('stores challenge with no uid and type=authentication', async () => { - const challenge = await manager.generateAuthenticationChallenge(); + const challenge = generateRandomChallenge(); + await manager.storeAuthenticationChallenge(challenge); const stored = await manager.consumeAuthenticationChallenge(challenge); @@ -102,7 +105,8 @@ describe('PasskeyChallengeManager (integration)', () => { describe('generateUpgradeChallenge', () => { it('stores challenge with uid and type=upgrade', async () => { - const challenge = await manager.generateUpgradeChallenge('cafebabe'); + const challenge = generateRandomChallenge(); + await manager.storeUpgradeChallenge(challenge, 'cafebabe'); const stored = await manager.consumeUpgradeChallenge( challenge, @@ -117,7 +121,8 @@ describe('PasskeyChallengeManager (integration)', () => { describe('consumeChallenge', () => { it('returns null on second consume (single-use enforcement)', async () => { - const challenge = await manager.generateRegistrationChallenge('deadbeef'); + const challenge = generateRandomChallenge(); + await manager.storeRegistrationChallenge(challenge, 'deadbeef'); await manager.consumeRegistrationChallenge(challenge, 'deadbeef'); @@ -150,8 +155,8 @@ describe('PasskeyChallengeManager (integration)', () => { const moduleRef = await buildTestModule(redis, shortConfig, mockLogger); const shortManager = moduleRef.get(PasskeyChallengeManager); - const challenge = - await shortManager.generateRegistrationChallenge('deadbeef'); + const challenge = generateRandomChallenge(); + await shortManager.storeRegistrationChallenge(challenge, 'deadbeef'); // Wait longer than the TTL to ensure the challenge expires await new Promise((resolve) => setTimeout(resolve, 1100)); @@ -166,7 +171,8 @@ describe('PasskeyChallengeManager (integration)', () => { describe('deleteChallenge', () => { it('removes the key from Redis', async () => { - const challenge = await manager.generateRegistrationChallenge('deadbeef'); + const challenge = generateRandomChallenge(); + await manager.storeRegistrationChallenge(challenge, 'deadbeef'); await manager.deleteChallenge('registration', challenge, 'deadbeef'); diff --git a/libs/accounts/passkey/src/lib/passkey.challenge.manager.spec.ts b/libs/accounts/passkey/src/lib/passkey.challenge.manager.spec.ts index ed0cb85b77f..dbdd49a7b12 100644 --- a/libs/accounts/passkey/src/lib/passkey.challenge.manager.spec.ts +++ b/libs/accounts/passkey/src/lib/passkey.challenge.manager.spec.ts @@ -7,6 +7,7 @@ import { StoredChallenge, } from './passkey.challenge.manager'; import { PasskeyConfig } from './passkey.config'; +import { generateRandomChallenge } from './webauthn-adapter'; const mockRedis = { set: jest.fn(), @@ -56,20 +57,14 @@ describe('PasskeyChallengeManager', () => { ); }); - describe('generateRegistrationChallenge', () => { - it('returns a base64url-encoded challenge string', async () => { - mockRedis.set.mockResolvedValue('OK'); - const result = await manager.generateRegistrationChallenge('deadbeef'); - - expect(result).toBe(MOCK_CHALLENGE); - }); - + describe('storeRegistrationChallenge', () => { it('calls redis.set with the correct key and TTL', async () => { mockRedis.set.mockResolvedValue('OK'); - await manager.generateRegistrationChallenge('deadbeef'); + const challenge = generateRandomChallenge(); + await manager.storeRegistrationChallenge(challenge, 'deadbeef'); expect(mockRedis.set).toHaveBeenCalledWith( - `passkey:challenge:registration:${MOCK_CHALLENGE}:deadbeef`, + `passkey:challenge:registration:${challenge}:deadbeef`, expect.any(String), 'EX', CHALLENGE_TIMEOUT_MS / 1000 @@ -78,7 +73,8 @@ describe('PasskeyChallengeManager', () => { it('increments statsd counter for generated challenges', async () => { mockRedis.set.mockResolvedValue('OK'); - await manager.generateRegistrationChallenge('deadbeef'); + const challenge = generateRandomChallenge(); + await manager.storeRegistrationChallenge(challenge, 'deadbeef'); expect(mockStatsd.increment).toHaveBeenCalledWith( 'passkey.challenge.generated', @@ -91,14 +87,15 @@ describe('PasskeyChallengeManager', () => { const dateNowSpy = jest.spyOn(Date, 'now').mockReturnValue(fakeNow); mockRedis.set.mockResolvedValue('OK'); - await manager.generateRegistrationChallenge('deadbeef'); + const challenge = generateRandomChallenge(); + await manager.storeRegistrationChallenge(challenge, 'deadbeef'); dateNowSpy.mockRestore(); const [, rawJson] = mockRedis.set.mock.calls[0]; const stored: StoredChallenge = JSON.parse(rawJson); - expect(stored.challenge).toBe(MOCK_CHALLENGE); + expect(stored.challenge).toBe(challenge); expect(stored.type).toBe('registration'); expect(stored.uid).toBe('deadbeef'); expect(stored.createdAt).toBe(fakeNow); @@ -107,8 +104,9 @@ describe('PasskeyChallengeManager', () => { it('throws if redis.set rejects', async () => { mockRedis.set.mockRejectedValue(new Error('Redis connection lost')); + const challenge = generateRandomChallenge(); await expect( - manager.generateRegistrationChallenge('deadbeef') + manager.storeRegistrationChallenge(challenge, 'deadbeef') ).rejects.toThrow('Redis connection lost'); }); }); @@ -116,12 +114,13 @@ describe('PasskeyChallengeManager', () => { describe('generateAuthenticationChallenge', () => { it('stores the challenge with type=authentication and no uid', async () => { mockRedis.set.mockResolvedValue('OK'); - await manager.generateAuthenticationChallenge(); + const challenge = generateRandomChallenge(); + await manager.storeAuthenticationChallenge(challenge); const [key, rawJson] = mockRedis.set.mock.calls[0]; const stored: StoredChallenge = JSON.parse(rawJson); - expect(key).toBe(`passkey:challenge:authentication:${MOCK_CHALLENGE}`); + expect(key).toBe(`passkey:challenge:authentication:${challenge}`); expect(stored.type).toBe('authentication'); expect(stored.uid).toBeUndefined(); }); @@ -130,12 +129,13 @@ describe('PasskeyChallengeManager', () => { describe('generateUpgradeChallenge', () => { it('stores the challenge with type=upgrade and uid', async () => { mockRedis.set.mockResolvedValue('OK'); - await manager.generateUpgradeChallenge('cafebabe'); + const challenge = generateRandomChallenge(); + await manager.storeUpgradeChallenge(challenge, 'cafebabe'); const [key, rawJson] = mockRedis.set.mock.calls[0]; const stored: StoredChallenge = JSON.parse(rawJson); - expect(key).toBe(`passkey:challenge:upgrade:${MOCK_CHALLENGE}:cafebabe`); + expect(key).toBe(`passkey:challenge:upgrade:${challenge}:cafebabe`); expect(stored.type).toBe('upgrade'); expect(stored.uid).toBe('cafebabe'); }); diff --git a/libs/accounts/passkey/src/lib/passkey.challenge.manager.ts b/libs/accounts/passkey/src/lib/passkey.challenge.manager.ts index fbcd851c541..8eee412e3ee 100644 --- a/libs/accounts/passkey/src/lib/passkey.challenge.manager.ts +++ b/libs/accounts/passkey/src/lib/passkey.challenge.manager.ts @@ -2,7 +2,6 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { randomBytes } from 'crypto'; import { Injectable, Inject, LoggerService } from '@nestjs/common'; import { LOGGER_PROVIDER } from '@fxa/shared/log'; import { StatsD, StatsDService } from '@fxa/shared/metrics/statsd'; @@ -117,8 +116,11 @@ export class PasskeyChallengeManager { * @param uid - Hex-encoded uid of the user. * @returns Base64url-encoded 32-byte challenge string. */ - async generateRegistrationChallenge(uid: string): Promise { - return this.generateChallenge('registration', uid); + async storeRegistrationChallenge( + challenge: string, + uid: string + ): Promise { + await this.storeChallenge('registration', challenge, uid); } /** @@ -126,8 +128,8 @@ export class PasskeyChallengeManager { * * @returns Base64url-encoded 32-byte challenge string. */ - async generateAuthenticationChallenge(): Promise { - return this.generateChallenge('authentication'); + async storeAuthenticationChallenge(challenge: string): Promise { + await this.storeChallenge('authentication', challenge); } /** @@ -136,8 +138,8 @@ export class PasskeyChallengeManager { * @param uid - Hex-encoded uid of the user. * @returns Base64url-encoded 32-byte challenge string. */ - async generateUpgradeChallenge(uid: string): Promise { - return this.generateChallenge('upgrade', uid); + async storeUpgradeChallenge(challenge: string, uid: string): Promise { + await this.storeChallenge('upgrade', challenge, uid); } /** @@ -241,11 +243,11 @@ export class PasskeyChallengeManager { } } - private async generateChallenge( + private async storeChallenge( type: ChallengeType, + challenge: string, uid?: string - ): Promise { - const challenge = randomBytes(32).toString('base64url'); + ): Promise { const now = Date.now(); const timeout = this.config.challengeTimeout; const ttlSeconds = Math.ceil(timeout / 1000); @@ -263,8 +265,6 @@ export class PasskeyChallengeManager { this.log?.debug?.('passkey.challenge.generated', { type, uid }); this.statsd?.increment('passkey.challenge.generated', { type }); - - return challenge; } private buildKey( diff --git a/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts b/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts index 8bb2ed7c3f8..1bb32e2d1df 100644 --- a/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts +++ b/libs/accounts/passkey/src/lib/passkey.security.in.spec.ts @@ -11,6 +11,9 @@ */ import { faker } from '@faker-js/faker'; +import Redis from 'ioredis'; +import { Test } from '@nestjs/testing'; +import { AppError } from '@fxa/accounts/errors'; import { AccountDatabase, AccountDbProvider, @@ -20,14 +23,16 @@ import { import { AccountManager } from '@fxa/shared/account/account'; import { LOGGER_PROVIDER } from '@fxa/shared/log'; import { StatsDService } from '@fxa/shared/metrics/statsd'; -import { Test } from '@nestjs/testing'; -import Redis from 'ioredis'; import { PasskeyManager } from './passkey.manager'; import { PasskeyChallengeManager } from './passkey.challenge.manager'; import { PasskeyConfig } from './passkey.config'; -import { AppError } from '@fxa/accounts/errors'; import { PASSKEY_CHALLENGE_REDIS } from './passkey.provider'; import { findPasskeyByCredentialId, insertPasskey } from './passkey.repository'; +import { generateRandomChallenge } from './webauthn-adapter'; +import { PasskeyService } from './passkey.service'; +import type { RegistrationResponseJSON } from '@simplewebauthn/server'; +import { encodeCBOR, type CBORType } from '@levischuck/tiny-cbor'; +import { createHash, generateKeyPairSync, randomBytes } from 'node:crypto'; const mockLogger = { log: jest.fn(), @@ -56,10 +61,12 @@ describe('Passkey Security Tests', () => { { provide: AccountDbProvider, useValue: db }, { provide: PasskeyConfig, - useValue: Object.assign(new PasskeyConfig(), { + useValue: new PasskeyConfig({ rpId: 'accounts.example.com', allowedOrigins: ['https://accounts.example.com'], maxPasskeysPerUser: 10, + enabled: true, + challengeTimeout: 30_000, }), }, { provide: LOGGER_PROVIDER, useValue: mockLogger }, @@ -164,10 +171,12 @@ describe('Passkey Security Tests', () => { beforeAll(async () => { redis = new Redis({ host: 'localhost' }); - const config = Object.assign(new PasskeyConfig(), { + const config = new PasskeyConfig({ + enabled: true, rpId: 'localhost', allowedOrigins: ['http://localhost'], challengeTimeout: 1000 * 60 * 5, // 5 minutes + maxPasskeysPerUser: 2, }); const moduleRef = await Test.createTestingModule({ @@ -199,8 +208,8 @@ describe('Passkey Security Tests', () => { const challenges: string[] = []; for (let i = 0; i < 100; i++) { - const challenge = - await challengeManager.generateRegistrationChallenge(fakeUid()); + const challenge = generateRandomChallenge(); + await challengeManager.storeRegistrationChallenge(challenge, fakeUid()); challenges.push(challenge); } @@ -216,8 +225,8 @@ describe('Passkey Security Tests', () => { // https://www.w3.org/TR/webauthn-3/#sctn-registering-a-new-credential it('consumeRegistrationChallenge physically deletes the Redis key on first use', async () => { const uid = fakeUid(); - const challenge = - await challengeManager.generateRegistrationChallenge(uid); + const challenge = generateRandomChallenge(); + await challengeManager.storeRegistrationChallenge(challenge, uid); await challengeManager.consumeRegistrationChallenge(challenge, uid); @@ -230,8 +239,8 @@ describe('Passkey Security Tests', () => { // https://www.w3.org/TR/webauthn-3/#sctn-cryptographic-challenges it('a registration challenge is rejected when presented as an authentication challenge and remains unconsumed', async () => { const uid = fakeUid(); - const challenge = - await challengeManager.generateRegistrationChallenge(uid); + const challenge = generateRandomChallenge(); + await challengeManager.storeRegistrationChallenge(challenge, uid); expect( await challengeManager.consumeAuthenticationChallenge(challenge) @@ -245,4 +254,204 @@ describe('Passkey Security Tests', () => { expect(correct?.type).toBe('registration'); }); }); + + describe('PasskeyService Registration Flow', () => { + const serviceConfig = new PasskeyConfig({ + rpId: 'localhost', + allowedOrigins: ['http://localhost'], + maxPasskeysPerUser: 10, + enabled: true, + challengeTimeout: 30_000, + }); + + let serviceDb: AccountDatabase; + let serviceRedis: Redis.Redis; + let serviceAccountManager: AccountManager; + let passkeyService: PasskeyService; + + /** + * Builds a cryptographically valid WebAuthn "none"-attestation registration response + * for the given challenge. Generates a fresh EC P-256 key pair each call. + * + * Uses the same CBOR library (@levischuck/tiny-cbor) that @simplewebauthn/server uses + * internally, so the encoded structures are accepted without modification. + */ + function buildValidRegistrationResponse( + challenge: string + ): RegistrationResponseJSON { + const { publicKey } = generateKeyPairSync('ec', { namedCurve: 'P-256' }); + const jwk = publicKey.export({ format: 'jwk' }) as { + x: string; + y: string; + }; + const x = Buffer.from(jwk.x, 'base64url'); + const y = Buffer.from(jwk.y, 'base64url'); + + // COSE EC2 public key (RFC 9052 §7.1) + const coseKeyMap = new Map([ + [1, 2], + [3, -7], + [-1, 1], + [-2, x], + [-3, y], + ]); + const coseKey = Buffer.from(encodeCBOR(coseKeyMap)); + + const credentialId = randomBytes(32); + + // Authenticator data layout (WebAuthn §6.1) + const rpIdHash = createHash('sha256').update(serviceConfig.rpId).digest(); + const flags = Buffer.from([0x45]); // UP | UV | AT + const signCount = Buffer.alloc(4); + const aaguid = Buffer.alloc(16); + const credIdLen = Buffer.alloc(2); + credIdLen.writeUInt16BE(credentialId.length); + + const authenticatorData = Buffer.concat([ + rpIdHash, + flags, + signCount, + aaguid, + credIdLen, + credentialId, + coseKey, + ]); + + const clientDataJSON = Buffer.from( + JSON.stringify({ + type: 'webauthn.create', + challenge, + origin: 'http://localhost', + crossOrigin: false, + }) + ).toString('base64url'); + + const attestObjMap = new Map([ + ['fmt', 'none'], + ['attStmt', new Map()], + ['authData', new Uint8Array(authenticatorData)], + ]); + const attestationObject = Buffer.from(encodeCBOR(attestObjMap)).toString( + 'base64url' + ); + + return { + id: credentialId.toString('base64url'), + rawId: credentialId.toString('base64url'), + response: { clientDataJSON, attestationObject }, + type: 'public-key', + clientExtensionResults: {}, + }; + } + + beforeAll(async () => { + try { + serviceDb = await testAccountDatabaseSetup([ + 'accounts', + 'emails', + 'passkeys', + ]); + serviceAccountManager = new AccountManager(serviceDb); + serviceRedis = new Redis({ host: 'localhost' }); + + const moduleRef = await Test.createTestingModule({ + providers: [ + PasskeyService, + PasskeyManager, + PasskeyChallengeManager, + { provide: AccountDbProvider, useValue: serviceDb }, + { provide: PasskeyConfig, useValue: serviceConfig }, + { provide: PASSKEY_CHALLENGE_REDIS, useValue: serviceRedis }, + { provide: LOGGER_PROVIDER, useValue: mockLogger }, + { provide: StatsDService, useValue: { increment: jest.fn() } }, + ], + }).compile(); + + passkeyService = moduleRef.get(PasskeyService); + } catch (error) { + console.warn('⚠️ Integration tests require database infrastructure.'); + console.warn( + '⚠️ Run "yarn start infrastructure" to enable these tests.' + ); + throw error; + } + }); + + afterAll(async () => { + if (serviceDb) await serviceDb.destroy(); + if (serviceRedis) await serviceRedis.quit(); + }); + + // WebAuthn §7.1: a valid attestation response with a matching challenge must + // be accepted and the credential stored in the database. + // https://www.w3.org/TR/webauthn-3/#sctn-registering-a-new-credential + it('accepts a valid registration response paired with its challenge', async () => { + const email = faker.internet.email(); + const uidHex = await serviceAccountManager.createAccountStub( + email, + 1, + 'en-US' + ); + const uid = Buffer.from(uidHex, 'hex'); + + const options = await passkeyService.generateRegistrationChallenge( + uid, + email + ); + const challenge = options.challenge; + + const response = buildValidRegistrationResponse(challenge); + + const passkey = + await passkeyService.createPasskeyFromRegistrationResponse( + uid, + response, + challenge + ); + + expect(passkey).toMatchObject({ + uid, + credentialId: expect.any(Buffer), + publicKey: expect.any(Buffer), + }); + }); + + // WebAuthn §7.1 step 11: a tampered or structurally invalid attestation must + // be rejected even when the challenge is genuine. + // https://www.w3.org/TR/webauthn-3/#sctn-registering-a-new-credential + it('rejects a bogus registration response paired with a valid challenge', async () => { + const email = faker.internet.email(); + const uidHex = await serviceAccountManager.createAccountStub( + email, + 1, + 'en-US' + ); + const uid = Buffer.from(uidHex, 'hex'); + + const options = await passkeyService.generateRegistrationChallenge( + uid, + email + ); + const challenge = options.challenge; + + const bogusResponse: RegistrationResponseJSON = { + id: 'bogus-credential-id', + rawId: 'bogus-credential-id', + response: { + clientDataJSON: Buffer.from('not-valid-json').toString('base64url'), + attestationObject: Buffer.from('not-cbor-data').toString('base64url'), + }, + type: 'public-key', + clientExtensionResults: {}, + }; + + await expect( + passkeyService.createPasskeyFromRegistrationResponse( + uid, + bogusResponse, + challenge + ) + ).rejects.toMatchObject(AppError.passkeyRegistrationFailed()); + }); + }); }); diff --git a/libs/accounts/passkey/src/lib/passkey.service.spec.ts b/libs/accounts/passkey/src/lib/passkey.service.spec.ts index 3e1e1396b78..a8a2db0ab03 100644 --- a/libs/accounts/passkey/src/lib/passkey.service.spec.ts +++ b/libs/accounts/passkey/src/lib/passkey.service.spec.ts @@ -15,20 +15,14 @@ import { PasskeyService } from './passkey.service'; import { PasskeyManager } from './passkey.manager'; import { PasskeyChallengeManager } from './passkey.challenge.manager'; import { AppError } from '@fxa/accounts/errors'; - -jest.mock('./webauthn-adapter', () => ({ - generateRegistrationOptions: jest.fn(), - verifyRegistrationResponse: jest.fn(), - generateAuthenticationOptions: jest.fn(), - verifyAuthenticationResponse: jest.fn(), -})); - import * as webauthnAdapter from './webauthn-adapter'; -const mockGenerateRegistrationOptions = - webauthnAdapter.generateRegistrationOptions as jest.Mock; -const mockVerifyRegistrationResponse = - webauthnAdapter.verifyRegistrationResponse as jest.Mock; +jest.mock('./webauthn-adapter'); + +// const mockGenerateRegistrationOptions = +// webauthnAdapter.generateRegistrationOptions as jest.Mock; +// const mockVerifyRegistrationResponse = +// webauthnAdapter.verifyRegistrationResponse as jest.Mock; describe('PasskeyService', () => { let service: PasskeyService; @@ -85,9 +79,9 @@ describe('PasskeyService', () => { }; const mockChallengeManager = { - generateRegistrationChallenge: jest.fn(), + storeRegistrationChallenge: jest.fn(), consumeRegistrationChallenge: jest.fn(), - generateAuthenticationChallenge: jest.fn(), + storeAuthenticationChallenge: jest.fn(), consumeAuthenticationChallenge: jest.fn(), }; @@ -144,10 +138,12 @@ describe('PasskeyService', () => { beforeEach(() => { mockManager.checkPasskeyCount.mockResolvedValue(undefined); - mockChallengeManager.generateRegistrationChallenge.mockResolvedValue( + mockChallengeManager.storeRegistrationChallenge.mockResolvedValue( MOCK_CHALLENGE ); - mockGenerateRegistrationOptions.mockResolvedValue(mockWebAuthnOptions); + ( + webauthnAdapter.generateRegistrationOptions as jest.Mock + ).mockResolvedValue(mockWebAuthnOptions); }); it('returns PublicKeyCredentialCreationOptionsJSON from the adapter', async () => { @@ -165,7 +161,7 @@ describe('PasskeyService', () => { const [checkCallOrder] = mockManager.checkPasskeyCount.mock.invocationCallOrder; const [generateChallengeCallOrder] = - mockChallengeManager.generateRegistrationChallenge.mock + mockChallengeManager.storeRegistrationChallenge.mock .invocationCallOrder; expect(checkCallOrder).toBeLessThan(generateChallengeCallOrder); }); @@ -174,15 +170,14 @@ describe('PasskeyService', () => { await service.generateRegistrationChallenge(MOCK_UID, MOCK_USER_NAME); expect( - mockChallengeManager.generateRegistrationChallenge - ).toHaveBeenCalledWith(MOCK_UID.toString('hex')); + mockChallengeManager.storeRegistrationChallenge + ).toHaveBeenCalledWith(MOCK_CHALLENGE, MOCK_UID.toString('hex')); - expect(mockGenerateRegistrationOptions).toHaveBeenCalledWith( + expect(webauthnAdapter.generateRegistrationOptions).toHaveBeenCalledWith( mockConfig, expect.objectContaining({ uid: MOCK_UID, email: MOCK_USER_NAME, - challenge: MOCK_CHALLENGE, }) ); }); @@ -209,7 +204,9 @@ describe('PasskeyService', () => { createdAt: Date.now() - 1000, expiresAt: Date.now() + 299000, }); - mockVerifyRegistrationResponse.mockResolvedValue({ + ( + webauthnAdapter.verifyRegistrationResponse as jest.Mock + ).mockResolvedValue({ verified: true, data: mockVerifiedData, }); @@ -230,11 +227,13 @@ describe('PasskeyService', () => { message: 'Passkey challenge not found', code: 404, }); - expect(mockVerifyRegistrationResponse).not.toHaveBeenCalled(); + expect(webauthnAdapter.verifyRegistrationResponse).not.toHaveBeenCalled(); }); it('throws passkeyRegistrationFailed AppError when adapter returns verified: false', async () => { - mockVerifyRegistrationResponse.mockResolvedValue({ verified: false }); + ( + webauthnAdapter.verifyRegistrationResponse as jest.Mock + ).mockResolvedValue({ verified: false }); await expect( service.createPasskeyFromRegistrationResponse( MOCK_UID, @@ -249,9 +248,9 @@ describe('PasskeyService', () => { }); it('throws passkeyRegistrationFailed AppError when adapter throws', async () => { - mockVerifyRegistrationResponse.mockRejectedValue( - new Error('Invalid attestation format') - ); + ( + webauthnAdapter.verifyRegistrationResponse as jest.Mock + ).mockRejectedValue(new Error('Invalid attestation format')); await expect( service.createPasskeyFromRegistrationResponse( MOCK_UID, @@ -299,7 +298,9 @@ describe('PasskeyService', () => { }); it('sets backupEligible=1 and backupState=1 when flags are true', async () => { - mockVerifyRegistrationResponse.mockResolvedValue({ + ( + webauthnAdapter.verifyRegistrationResponse as jest.Mock + ).mockResolvedValue({ verified: true, data: { ...mockVerifiedData, backupEligible: true, backupState: true }, }); @@ -362,7 +363,9 @@ describe('PasskeyService', () => { transports: string[], aaguid: Buffer = MOCK_AAGUID_ZEROS ): Promise { - mockVerifyRegistrationResponse.mockResolvedValue({ + ( + webauthnAdapter.verifyRegistrationResponse as jest.Mock + ).mockResolvedValue({ verified: true, data: { ...mockVerifiedData, transports, aaguid }, }); @@ -461,7 +464,7 @@ describe('PasskeyService', () => { }; beforeEach(() => { - mockChallengeManager.generateAuthenticationChallenge.mockResolvedValue( + mockChallengeManager.storeAuthenticationChallenge.mockResolvedValue( MOCK_CHALLENGE ); ( @@ -477,7 +480,7 @@ describe('PasskeyService', () => { it('generates a challenge via ChallengeManager', async () => { await service.generateAuthenticationChallenge(); expect( - mockChallengeManager.generateAuthenticationChallenge + mockChallengeManager.storeAuthenticationChallenge ).toHaveBeenCalledTimes(1); }); @@ -486,7 +489,6 @@ describe('PasskeyService', () => { expect( webauthnAdapter.generateAuthenticationOptions ).toHaveBeenCalledWith(mockConfig, { - challenge: MOCK_CHALLENGE, allowCredentials: [], }); }); @@ -505,7 +507,6 @@ describe('PasskeyService', () => { expect( webauthnAdapter.generateAuthenticationOptions ).toHaveBeenCalledWith(mockConfig, { - challenge: MOCK_CHALLENGE, allowCredentials: [MOCK_CREDENTIAL_ID], }); }); @@ -518,7 +519,6 @@ describe('PasskeyService', () => { expect( webauthnAdapter.generateAuthenticationOptions ).toHaveBeenCalledWith(mockConfig, { - challenge: MOCK_CHALLENGE, allowCredentials: [], }); }); diff --git a/libs/accounts/passkey/src/lib/passkey.service.ts b/libs/accounts/passkey/src/lib/passkey.service.ts index b8cc90f2978..861bc82e55e 100644 --- a/libs/accounts/passkey/src/lib/passkey.service.ts +++ b/libs/accounts/passkey/src/lib/passkey.service.ts @@ -116,15 +116,16 @@ export class PasskeyService { await this.passkeyManager.checkPasskeyCount(uid); - const challenge = - await this.challengeManager.generateRegistrationChallenge(uidHex); - const options = await generateRegistrationOptions(this.config, { uid, email, - challenge, }); + await this.challengeManager.storeRegistrationChallenge( + options.challenge, + uidHex + ); + return options; } @@ -396,19 +397,18 @@ export class PasskeyService { async generateAuthenticationChallenge( uid?: Buffer ): Promise { - const challenge = - await this.challengeManager.generateAuthenticationChallenge(); - let allowCredentials: Buffer[] = []; if (uid) { const passkeys = await this.passkeyManager.listPasskeysForUser(uid); allowCredentials = passkeys.map((p) => p.credentialId); } - - return await generateAuthenticationOptions(this.config, { - challenge, + const options = await generateAuthenticationOptions(this.config, { allowCredentials, }); + + await this.challengeManager.storeAuthenticationChallenge(options.challenge); + + return options; } /** diff --git a/libs/accounts/passkey/src/lib/webauthn-adapter.spec.ts b/libs/accounts/passkey/src/lib/webauthn-adapter.spec.ts index d11444b434e..65bd377c5c3 100644 --- a/libs/accounts/passkey/src/lib/webauthn-adapter.spec.ts +++ b/libs/accounts/passkey/src/lib/webauthn-adapter.spec.ts @@ -123,7 +123,6 @@ describe('generateRegistrationOptions', () => { { uid, email: 'alice@example.com', - challenge: 'test-challenge', } ); @@ -133,7 +132,7 @@ describe('generateRegistrationOptions', () => { rpID: 'accounts.firefox.com', userName: 'alice@example.com', userID: uid, - challenge: 'test-challenge', + challenge: expect.stringMatching(/.{0,44}/), authenticatorSelection: expect.objectContaining({ userVerification: 'required', residentKey: 'required', @@ -150,7 +149,6 @@ describe('generateRegistrationOptions', () => { const result = await generateRegistrationOptions(mockConfig(), { uid: Buffer.alloc(16), email: 'user@example.com', - challenge: 'xyz', }); expect(result).toBe(fakeResult); @@ -247,7 +245,6 @@ describe('generateAuthenticationOptions', () => { await generateAuthenticationOptions( mockConfig({ userVerification: 'discouraged' }), { - challenge: 'random-challenge-abc', allowCredentials: [], } ); @@ -255,7 +252,7 @@ describe('generateAuthenticationOptions', () => { expect(libMocks.generateAuthenticationOptions).toHaveBeenCalledWith( expect.objectContaining({ rpID: 'accounts.firefox.com', - challenge: 'random-challenge-abc', + challenge: expect.stringMatching(/[A-Za-z-_0-9]{32}/), userVerification: 'discouraged', }) ); @@ -265,7 +262,6 @@ describe('generateAuthenticationOptions', () => { libMocks.generateAuthenticationOptions.mockResolvedValue({}); await generateAuthenticationOptions(mockConfig(), { - challenge: 'ch', allowCredentials: [], }); @@ -279,7 +275,6 @@ describe('generateAuthenticationOptions', () => { const credId = Buffer.from('helloworld'); await generateAuthenticationOptions(mockConfig(), { - challenge: 'ch', allowCredentials: [credId], }); @@ -297,7 +292,6 @@ describe('generateAuthenticationOptions', () => { libMocks.generateAuthenticationOptions.mockResolvedValue(fakeResult); const result = await generateAuthenticationOptions(mockConfig(), { - challenge: 'q1w2e3', allowCredentials: [], }); diff --git a/libs/accounts/passkey/src/lib/webauthn-adapter.ts b/libs/accounts/passkey/src/lib/webauthn-adapter.ts index ab60d0e4ee7..589c99022aa 100644 --- a/libs/accounts/passkey/src/lib/webauthn-adapter.ts +++ b/libs/accounts/passkey/src/lib/webauthn-adapter.ts @@ -18,14 +18,21 @@ import type { } from '@simplewebauthn/server'; import { PasskeyConfig } from './passkey.config'; +import { randomBytes } from 'node:crypto'; export interface RegistrationOptionsInput { /** User's uid as a Buffer, used as the WebAuthn userID. */ uid: Buffer; /** User's email address, used as the WebAuthn userName. */ email: string; - /** Challenge from ChallengeManager. */ - challenge: string; +} + +/** + * Creates a random 32 byte value that is base64url encoded. + * @returns + */ +export function generateRandomChallenge() { + return randomBytes(32).toString('base64url'); } /** @@ -38,6 +45,7 @@ export async function generateRegistrationOptions( config: PasskeyConfig, input: RegistrationOptionsInput ): Promise { + const challenge = generateRandomChallenge(); return libGenerateRegistrationOptions({ // rpName is deprecated field kept for backward compatibility; // spec recommends using rpId as a safe default. @@ -45,7 +53,7 @@ export async function generateRegistrationOptions( rpID: config.rpId, userName: input.email, userID: input.uid, - challenge: input.challenge, + challenge, authenticatorSelection: { residentKey: config.residentKey, userVerification: config.userVerification, @@ -122,8 +130,6 @@ export async function verifyRegistrationResponse( * `rpID` and `userVerification` are supplied by PasskeyConfig. */ export interface AuthenticationOptionsInput { - /** Challenge from ChallengeManager. */ - challenge: string; /** * Credential IDs to restrict authentication to. * Pass the user's stored credential IDs for known-user flows. @@ -142,10 +148,11 @@ export async function generateAuthenticationOptions( config: PasskeyConfig, input: AuthenticationOptionsInput ): Promise { + const challenge = generateRandomChallenge(); return libGenerateAuthenticationOptions({ rpID: config.rpId, userVerification: config.userVerification, - challenge: input.challenge, + challenge, allowCredentials: input.allowCredentials.length > 0 ? input.allowCredentials.map((id) => ({ id: id.toString('base64url') }))