diff --git a/.changeset/brave-hoops-turn.md b/.changeset/brave-hoops-turn.md new file mode 100644 index 0000000..dd9a17b --- /dev/null +++ b/.changeset/brave-hoops-turn.md @@ -0,0 +1,28 @@ +--- +'seamless-auth-api': minor +--- + +Give WebAuthn challenges their own store, with an expiry and one-time use. + +Challenges lived in a single `users.challenge` column shared by registration, +login and step-up. Three consequences, all fixed here: + +- **Flows clobbered each other.** Starting a login invalidated a registration + already in flight for the same user, and a second tab invalidated the first. + Challenges are now keyed by user and flow, so registration, login and step-up + can be outstanding at once. +- **Nothing expired.** The column had no lifetime, so a challenge stayed valid + until some later flow happened to overwrite it. The `timeout` in the credential + options is only a hint to the browser and was never enforced. Challenges now + expire server side after five minutes, comfortably longer than that hint so no + legitimate ceremony is cut short. +- **A challenge could outlive its ceremony.** It is now spent when verification + reads it, before anything else can fail, so an attempt that fails leaves + nothing redeemable behind. + +A magic link completing also spends any half-finished WebAuthn ceremony for that +user, preserving what the old defensive clear did. + +`users.challenge` and `users.challengeContext` are no longer read or written. +They are left in place so this release can be rolled back, and should be dropped +in a follow-up once it has run in production. diff --git a/src/controllers/magicLinks.ts b/src/controllers/magicLinks.ts index d14d5f4..3be7e7f 100644 --- a/src/controllers/magicLinks.ts +++ b/src/controllers/magicLinks.ts @@ -16,6 +16,7 @@ import { AuthEventService } from '../services/authEventService.js'; import { getLoginPolicy, isLoginMethodEnabled } from '../services/loginPolicyService.js'; import { sendMagicLinkEmail } from '../services/messagingService.js'; import { issueSessionAndRespond } from '../services/sessionIssuance.js'; +import { invalidateChallengesForUser } from '../services/webauthnChallengeService.js'; import { AuthenticatedRequest } from '../types/types.js'; import getLogger from '../utils/logger.js'; import { hashDeviceFingerprint, hashSha256 } from '../utils/utils.js'; @@ -253,11 +254,14 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { req, }); - user.challenge = ''; user.verified = true; await user.save(); + // A different route completed the sign-in, so any half-finished WebAuthn + // ceremony for this user should not still be redeemable. + await invalidateChallengesForUser(user.id); + await AuthEventService.log({ userId: user.id, type: 'registration_success', @@ -278,7 +282,6 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { await user.update({ lastLogin: new Date(), - challengeContext: null, }); return; diff --git a/src/controllers/stepUp.ts b/src/controllers/stepUp.ts index 7b3a5ba..10ce6d1 100644 --- a/src/controllers/stepUp.ts +++ b/src/controllers/stepUp.ts @@ -22,6 +22,7 @@ import { recordStepUpVerification, serializeStepUpStatus, } from '../services/stepUpService.js'; +import { consumeChallenge, issueChallenge } from '../services/webauthnChallengeService.js'; import { AuthenticatedRequest } from '../types/types.js'; import getLogger from '../utils/logger.js'; @@ -111,7 +112,9 @@ export const startWebAuthnStepUp = async (req: Request, res: Response) => { extensions: buildPrfAuthenticationExtensions(prf), }); - await user.update({ + await issueChallenge({ + userId: user.id, + purpose: 'step_up', challenge: options.challenge, }); @@ -162,7 +165,11 @@ export const finishWebAuthnStepUp = async (req: Request, res: Response) => { return res.status(400).json({ error: 'prf_output_not_allowed' }); } - if (!user.challenge || typeof assertionId !== 'string') { + // Consumed before the credential lookup, so every exit below leaves the + // challenge spent rather than live. + const issued = await consumeChallenge({ userId: user.id, purpose: 'step_up' }); + + if (!issued || typeof assertionId !== 'string') { await AuthEventService.log({ userId: user.id, type: 'step_up_failed', @@ -186,8 +193,7 @@ export const finishWebAuthnStepUp = async (req: Request, res: Response) => { return res.status(401).json({ error: 'step_up_failed' }); } - const expectedChallenge = user.challenge; - await user.update({ challenge: null }); + const expectedChallenge = issued.challenge; try { const { origins, rpid } = await getSystemConfig(); diff --git a/src/controllers/webauthn.ts b/src/controllers/webauthn.ts index 99a3d5b..0a4fda4 100644 --- a/src/controllers/webauthn.ts +++ b/src/controllers/webauthn.ts @@ -28,19 +28,16 @@ import type { WebAuthnAuthenticatorAttachment } from '../schemas/webauthn.reques import { AuthEventService } from '../services/authEventService.js'; import { rejectIfUserLocked } from '../services/lockoutPolicyService.js'; import { issueSessionAndRespond } from '../services/sessionIssuance.js'; +import { consumeChallenge, issueChallenge } from '../services/webauthnChallengeService.js'; import { AuthenticatedRequest } from '../types/types.js'; import getLogger from '../utils/logger.js'; const logger = getLogger('webauthn'); -function getRegistrationChallengeContext(user: User) { - const webauthnRegistration = user.challengeContext?.webauthnRegistration; - - if (typeof webauthnRegistration !== 'object' || webauthnRegistration === null) { +function getRegistrationChallengeContext(context: Record | null | undefined) { + if (!context) { return { prfRequested: false, requirePrf: false }; } - const context = webauthnRegistration as Record; - return { prfRequested: context.prfRequested === true, requirePrf: context.requirePrf === true, @@ -152,15 +149,11 @@ const registerWebAuthn = async (req: Request, res: Response) => { extensions: buildPrfRegistrationExtensions(prfRequested), }); - await verifiedUser.update({ + await issueChallenge({ + userId: verifiedUser.id, + purpose: 'registration', challenge: options.challenge, - challengeContext: { - ...(verifiedUser.challengeContext ?? {}), - webauthnRegistration: { - prfRequested, - requirePrf, - }, - }, + context: { prfRequested, requirePrf }, }); logger.info('Generated registration options for user'); @@ -231,7 +224,11 @@ const verifyWebAuthnRegistration = async (req: Request, res: Response) => { return res.status(403).json({ error: 'Not allowed' }); } - const expectedChallenge = user.challenge; + // Consumed before verification, so the challenge is spent however this + // attempt turns out and a failure cannot leave one live to replay against. + const issued = await consumeChallenge({ userId: user.id, purpose: 'registration' }); + const expectedChallenge = issued?.challenge; + if (!expectedChallenge) { logger.error('Unexpected user challegnge supplied.'); await AuthEventService.log({ @@ -278,7 +275,7 @@ const verifyWebAuthnRegistration = async (req: Request, res: Response) => { } const { credential, credentialBackedUp, credentialDeviceType } = registrationInfo; - const challengeContext = getRegistrationChallengeContext(user); + const challengeContext = getRegistrationChallengeContext(issued?.context); const prfCapable = getRegistrationPrfCapable(attestationResponse) || metadata.prfCapable === true; @@ -312,8 +309,6 @@ const verifyWebAuthnRegistration = async (req: Request, res: Response) => { }); await user.update({ - challenge: null, - challengeContext: null, lastLogin: new Date(), verified: true, }); @@ -403,7 +398,9 @@ const generateWebAuthn = async (req: Request, res: Response) => { extensions: buildPrfAuthenticationExtensions(prf), }); - await user.update({ + await issueChallenge({ + userId: user.id, + purpose: 'authentication', challenge: options.challenge, }); @@ -466,7 +463,13 @@ const verifyWebAuthn = async (req: Request, res: Response) => { return res.status(403).json({ error: 'Not allowed' }); } - if (!user || !user.challenge) { + // Consumed before anything else can fail, so this path cannot leave a live + // challenge behind for an assertion to be replayed against. + const issued = user + ? await consumeChallenge({ userId: user.id, purpose: 'authentication' }) + : null; + + if (!user || !issued) { logger.error('User or user challenge missing'); await AuthEventService.log({ userId: user.id, @@ -495,7 +498,7 @@ const verifyWebAuthn = async (req: Request, res: Response) => { return res.status(401).json({ error: 'Authentication failed.' }); } - const expectedChallenge = user.challenge; + const expectedChallenge = issued.challenge; let verification; try { diff --git a/src/migrations/20260829040000-create-webauthn-challenges.cjs b/src/migrations/20260829040000-create-webauthn-challenges.cjs new file mode 100644 index 0000000..710fe1c --- /dev/null +++ b/src/migrations/20260829040000-create-webauthn-challenges.cjs @@ -0,0 +1,74 @@ +'use strict'; + +/** + * Moves WebAuthn challenges off the single `users.challenge` column. + * + * That column was shared by registration, login and step-up, so two flows for + * one user clobbered each other, and it had no expiry: a challenge stayed valid + * until some later flow happened to overwrite it. + * + * @type {import('sequelize-cli').Migration} + */ +module.exports = { + async up(queryInterface, Sequelize) { + await queryInterface.createTable('webauthn_challenges', { + id: { + type: Sequelize.UUID, + defaultValue: Sequelize.literal('gen_random_uuid()'), + primaryKey: true, + }, + user_id: { + type: Sequelize.UUID, + allowNull: false, + references: { model: 'users', key: 'id' }, + onDelete: 'CASCADE', + }, + purpose: { + type: Sequelize.STRING, + allowNull: false, + }, + challenge: { + type: Sequelize.STRING, + allowNull: false, + }, + // Flow state that used to live in users.challengeContext, for example + // whether the registration asked for PRF. + context: { + type: Sequelize.JSONB, + allowNull: true, + }, + expires_at: { + type: Sequelize.DATE, + allowNull: false, + }, + consumed_at: { + type: Sequelize.DATE, + allowNull: true, + }, + created_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn('NOW'), + }, + updated_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn('NOW'), + }, + }); + + // Every lookup is "the live challenge for this user and this flow". + await queryInterface.addIndex('webauthn_challenges', ['user_id', 'purpose'], { + name: 'webauthn_challenges_user_purpose_idx', + }); + + // Supports reaping expired rows without scanning the table. + await queryInterface.addIndex('webauthn_challenges', ['expires_at'], { + name: 'webauthn_challenges_expires_at_idx', + }); + }, + + async down(queryInterface) { + await queryInterface.dropTable('webauthn_challenges'); + }, +}; diff --git a/src/models/webauthnChallenges.ts b/src/models/webauthnChallenges.ts new file mode 100644 index 0000000..75ef997 --- /dev/null +++ b/src/models/webauthnChallenges.ts @@ -0,0 +1,97 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the GNU Affero General Public License v3.0 + * See LICENSE file in the project root for full license information + */ + +import { DataTypes, Model, Optional, Sequelize } from 'sequelize'; + +export type WebAuthnChallengePurpose = 'registration' | 'authentication' | 'step_up'; + +export interface WebAuthnChallengeAttributes { + id: string; + userId: string; + purpose: WebAuthnChallengePurpose; + challenge: string; + context?: Record | null; + expiresAt: Date; + consumedAt?: Date | null; + createdAt?: Date; + updatedAt?: Date; +} + +type WebAuthnChallengeCreationAttributes = Optional< + WebAuthnChallengeAttributes, + 'id' | 'context' | 'consumedAt' | 'createdAt' | 'updatedAt' +>; + +export class WebAuthnChallenge + extends Model + implements WebAuthnChallengeAttributes +{ + declare id: string; + declare userId: string; + declare purpose: WebAuthnChallengePurpose; + declare challenge: string; + declare context: Record | null; + declare expiresAt: Date; + declare consumedAt: Date | null; + declare readonly createdAt: Date; + declare readonly updatedAt: Date; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + static associate(models: any) { + WebAuthnChallenge.belongsTo(models.User, { + foreignKey: 'userId', + onDelete: 'CASCADE', + as: 'user', + }); + } +} + +const initializeWebAuthnChallengeModel = (sequelize: Sequelize) => { + WebAuthnChallenge.init( + { + id: { + type: DataTypes.UUID, + defaultValue: DataTypes.UUIDV4, + primaryKey: true, + }, + userId: { + type: DataTypes.UUID, + allowNull: false, + }, + purpose: { + type: DataTypes.STRING, + allowNull: false, + }, + challenge: { + type: DataTypes.STRING, + allowNull: false, + }, + context: { + type: DataTypes.JSONB, + allowNull: true, + }, + expiresAt: { + type: DataTypes.DATE, + allowNull: false, + }, + consumedAt: { + type: DataTypes.DATE, + allowNull: true, + }, + }, + { + sequelize, + modelName: 'WebAuthnChallenge', + tableName: 'webauthn_challenges', + underscored: true, + timestamps: true, + }, + ); + + return WebAuthnChallenge; +}; + +export default initializeWebAuthnChallengeModel; diff --git a/src/services/webauthnChallengeService.ts b/src/services/webauthnChallengeService.ts new file mode 100644 index 0000000..9c206ac --- /dev/null +++ b/src/services/webauthnChallengeService.ts @@ -0,0 +1,117 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the GNU Affero General Public License v3.0 + * See LICENSE file in the project root for full license information + */ + +import { Op } from 'sequelize'; + +import { WebAuthnChallenge, type WebAuthnChallengePurpose } from '../models/webauthnChallenges.js'; + +/** + * How long a challenge stays usable. + * + * Comfortably longer than the 60 second `timeout` in the credential options, + * which is only a hint to the browser, so a user who takes a while to find a + * security key or answer a biometric prompt is not cut off. Short enough that a + * captured challenge is worth minutes rather than however long it took some + * later flow to overwrite it. + */ +export const CHALLENGE_TTL_SECONDS = 300; + +export interface IssuedChallenge { + challenge: string; + context: Record | null; +} + +/** + * Records a challenge for one user and one flow. + * + * Any challenge still outstanding for the same user and flow is consumed first. + * Only the newest matters, and leaving the older ones live would widen the + * window a caller can replay against. + */ +export async function issueChallenge(params: { + userId: string; + purpose: WebAuthnChallengePurpose; + challenge: string; + context?: Record | null; + now?: Date; +}): Promise { + const now = params.now ?? new Date(); + + await consumeOutstanding(params.userId, params.purpose, now); + + return WebAuthnChallenge.create({ + userId: params.userId, + purpose: params.purpose, + challenge: params.challenge, + context: params.context ?? null, + expiresAt: new Date(now.getTime() + CHALLENGE_TTL_SECONDS * 1000), + }); +} + +/** + * Takes the live challenge for one user and one flow, and spends it. + * + * Consuming on read rather than on success is deliberate: call this once at the + * start of verification and the challenge is spent however the rest of the + * attempt turns out, so a failed attempt cannot leave a live challenge behind + * for someone to replay an assertion against. + */ +export async function consumeChallenge(params: { + userId: string; + purpose: WebAuthnChallengePurpose; + now?: Date; +}): Promise { + const now = params.now ?? new Date(); + + const record = await WebAuthnChallenge.findOne({ + where: { + userId: params.userId, + purpose: params.purpose, + consumedAt: null, + expiresAt: { [Op.gt]: now }, + }, + order: [['createdAt', 'DESC']], + }); + + if (!record) { + return null; + } + + await record.update({ consumedAt: now }); + + return { challenge: record.challenge, context: record.context ?? null }; +} + +/** + * Spends everything outstanding for a user, across every flow. + * + * Used when a different authentication route completes and any half-finished + * WebAuthn ceremony should not still be redeemable. + */ +export async function invalidateChallengesForUser(userId: string, now = new Date()) { + await WebAuthnChallenge.update( + { consumedAt: now }, + { + where: { + userId, + consumedAt: null, + }, + }, + ); +} + +async function consumeOutstanding(userId: string, purpose: WebAuthnChallengePurpose, now: Date) { + await WebAuthnChallenge.update( + { consumedAt: now }, + { + where: { + userId, + purpose, + consumedAt: null, + }, + }, + ); +} diff --git a/tests/factories/webauthnChallengeFactory.ts b/tests/factories/webauthnChallengeFactory.ts new file mode 100644 index 0000000..8494dc1 --- /dev/null +++ b/tests/factories/webauthnChallengeFactory.ts @@ -0,0 +1,21 @@ +import { vi } from 'vitest'; + +/** + * A live challenge row, as `consumeChallenge` would find it. + * + * `update` is a spy so a test can assert the challenge was actually spent, + * which is the property the store exists to guarantee. + */ +export function buildWebAuthnChallenge(overrides: Record = {}) { + return { + id: 'challenge-1', + userId: 'user-1', + purpose: 'authentication', + challenge: 'challenge', + context: null, + expiresAt: new Date(Date.now() + 300_000), + consumedAt: null, + update: vi.fn(), + ...overrides, + } as any; +} diff --git a/tests/integration/stepUp/stepUp.spec.ts b/tests/integration/stepUp/stepUp.spec.ts index 4938af6..e1dcd52 100644 --- a/tests/integration/stepUp/stepUp.spec.ts +++ b/tests/integration/stepUp/stepUp.spec.ts @@ -1,3 +1,5 @@ +import { WebAuthnChallenge } from '../../../src/models/webauthnChallenges'; +import { buildWebAuthnChallenge } from '../../factories/webauthnChallengeFactory'; import { Application } from 'express'; import request from 'supertest'; import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -14,6 +16,7 @@ beforeAll(async () => { beforeEach(() => { vi.clearAllMocks(); + (WebAuthnChallenge.findOne as any).mockResolvedValue(buildWebAuthnChallenge()); }); describe('GET /step-up/status', () => { diff --git a/tests/integration/webauthn/webauthn.spec.ts b/tests/integration/webauthn/webauthn.spec.ts index 648d0ee..d2794d4 100644 --- a/tests/integration/webauthn/webauthn.spec.ts +++ b/tests/integration/webauthn/webauthn.spec.ts @@ -9,6 +9,8 @@ import { buildSystemConfig } from '../../factories/systemConfigFactory'; import { Session } from '../../../src/models/sessions'; import { User } from '../../../src/models/users'; import { buildUser } from '../../factories/userFactory'; +import { WebAuthnChallenge } from '../../../src/models/webauthnChallenges'; +import { buildWebAuthnChallenge } from '../../factories/webauthnChallengeFactory'; import { buildCredential } from '../../factories/credentialFactory'; import { generateRefreshToken, hashRefreshToken, signAccessToken } from '../../../src/lib/token'; import { AuthEvent } from '../../../src/models/authEvents'; @@ -65,6 +67,7 @@ beforeAll(async () => { beforeEach(() => { vi.clearAllMocks(); + (WebAuthnChallenge.findOne as any).mockResolvedValue(buildWebAuthnChallenge()); (AuthEvent.count as any).mockResolvedValue(0); (getSystemConfig as any).mockResolvedValue({ app_name: 'SeamlessAuth', @@ -294,6 +297,60 @@ describe('GET /webauthn/register/start', () => { }); }); +describe('challenge is spent on every login outcome', () => { + // The login path previously never cleared the challenge, so a captured + // assertion stayed replayable until some later flow happened to overwrite it. + it('spends the challenge even when verification fails', async () => { + const record = buildWebAuthnChallenge({ purpose: 'authentication' }); + (WebAuthnChallenge.findOne as any).mockResolvedValue(record); + (Credential.findOne as any).mockResolvedValue(buildCredential({ id: 'cred-1' })); + + const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); + (verifyAuthenticationResponse as any).mockRejectedValue(new Error('bad assertion')); + + await request(app) + .post('/webauthn/login/finish') + .send({ assertionResponse: { id: 'cred-1' } }); + + expect(record.update).toHaveBeenCalledWith( + expect.objectContaining({ consumedAt: expect.any(Date) }), + ); + }); + + it('spends the challenge on a successful login', async () => { + const record = buildWebAuthnChallenge({ purpose: 'authentication' }); + (WebAuthnChallenge.findOne as any).mockResolvedValue(record); + (Credential.findOne as any).mockResolvedValue( + buildCredential({ id: 'cred-1', update: vi.fn() }), + ); + (Session.create as any).mockResolvedValue({ id: 'session-1' }); + + const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); + (verifyAuthenticationResponse as any).mockResolvedValue({ + verified: true, + authenticationInfo: { newCounter: 1 }, + }); + + await request(app) + .post('/webauthn/login/finish') + .send({ assertionResponse: { id: 'cred-1' } }); + + expect(record.update).toHaveBeenCalledWith( + expect.objectContaining({ consumedAt: expect.any(Date) }), + ); + }); + + it('refuses a second attempt once the challenge is spent', async () => { + (WebAuthnChallenge.findOne as any).mockResolvedValue(null); + + const res = await request(app) + .post('/webauthn/login/finish') + .send({ assertionResponse: { id: 'cred-1' } }); + + expect(res.status).toBe(401); + }); +}); + describe('POST /webauthn/register/finish', () => { it('creates credential and session', async () => { const user = buildUser(); @@ -377,16 +434,16 @@ describe('POST /webauthn/register/finish', () => { }); it('rejects PRF-required registration when credential is not PRF-capable', async () => { - const user = buildUser({ - challengeContext: { - webauthnRegistration: { - prfRequested: true, - requirePrf: true, - }, - }, - }); + const user = buildUser(); (User.findOne as any).mockResolvedValue(user); + // The flow's PRF requirement travels with the challenge it was issued for. + (WebAuthnChallenge.findOne as any).mockResolvedValue( + buildWebAuthnChallenge({ + purpose: 'registration', + context: { prfRequested: true, requirePrf: true }, + }), + ); const { verifyRegistrationResponse } = await import('@simplewebauthn/server'); (verifyRegistrationResponse as any).mockResolvedValue({ @@ -429,8 +486,9 @@ describe('POST /webauthn/register/finish', () => { expect(res.status).toBe(403); }); - it('rejects verification when the stored challenge is missing', async () => { - (User.findOne as any).mockResolvedValue(buildUser({ challenge: null })); + it('rejects verification when no live challenge exists', async () => { + (User.findOne as any).mockResolvedValue(buildUser()); + (WebAuthnChallenge.findOne as any).mockResolvedValue(null); const res = await request(app) .post('/webauthn/register/finish') diff --git a/tests/setup/mocks.ts b/tests/setup/mocks.ts index 3a85d81..64d59b2 100644 --- a/tests/setup/mocks.ts +++ b/tests/setup/mocks.ts @@ -37,6 +37,15 @@ vi.mock('../../src/models/credentials.js', () => ({ }, })); +vi.mock('../../src/models/webauthnChallenges.js', () => ({ + WebAuthnChallenge: { + create: vi.fn(), + findOne: vi.fn(), + update: vi.fn(), + destroy: vi.fn(), + }, +})); + vi.mock('../../src/models/totpCredentials.js', () => ({ TotpCredential: { create: vi.fn(), diff --git a/tests/unit/controllers/stepUp.spec.ts b/tests/unit/controllers/stepUp.spec.ts index 6caf346..54e8751 100644 --- a/tests/unit/controllers/stepUp.spec.ts +++ b/tests/unit/controllers/stepUp.spec.ts @@ -11,6 +11,8 @@ import { Session } from '../../../src/models/sessions.js'; import { buildCredential } from '../../factories/credentialFactory.js'; import { buildSession } from '../../factories/sessionFactory.js'; import { buildUser } from '../../factories/userFactory.js'; +import { WebAuthnChallenge } from '../../../src/models/webauthnChallenges'; +import { buildWebAuthnChallenge } from '../../factories/webauthnChallengeFactory'; function prfSalt(byte = 1) { return Buffer.alloc(32, byte).toString('base64url'); @@ -36,8 +38,12 @@ function buildRes() { beforeEach(() => { vi.clearAllMocks(); + challengeRecord.update = vi.fn(); + (WebAuthnChallenge.findOne as any).mockResolvedValue(challengeRecord); }); +const challengeRecord = buildWebAuthnChallenge({ purpose: 'step_up' }); + describe('step-up controller', () => { it('starts a WebAuthn step-up challenge for authenticated users with credentials', async () => { const user = buildUser(); @@ -60,7 +66,9 @@ describe('step-up controller', () => { rpID: 'localhost', }), ); - expect(user.update).toHaveBeenCalledWith({ challenge: 'challenge' }); + expect(WebAuthnChallenge.create).toHaveBeenCalledWith( + expect.objectContaining({ purpose: 'step_up', challenge: 'challenge' }), + ); expect(res.json).toHaveBeenCalledWith({ challenge: 'challenge' }); }); @@ -99,7 +107,7 @@ describe('step-up controller', () => { }); it('finishes WebAuthn step-up and records freshness on the current session', async () => { - const user = buildUser({ challenge: 'challenge' }); + const user = buildUser(); const credential = buildCredential({ id: 'cred-1', userId: user.id }); const session = buildSession({ stepUpVerifiedAt: null, stepUpMethod: null }); @@ -124,7 +132,9 @@ describe('step-up controller', () => { await finishWebAuthnStepUp(req, res); - expect(user.update).toHaveBeenCalledWith({ challenge: null }); + expect(challengeRecord.update).toHaveBeenCalledWith( + expect.objectContaining({ consumedAt: expect.any(Date) }), + ); expect(credential.update).toHaveBeenCalledWith( expect.objectContaining({ counter: 2, @@ -143,7 +153,7 @@ describe('step-up controller', () => { }); it('does not update session freshness when verification fails', async () => { - const user = buildUser({ challenge: 'challenge' }); + const user = buildUser(); const credential = buildCredential({ id: 'cred-1', userId: user.id }); (Credential.findOne as any).mockResolvedValue(credential); @@ -173,7 +183,7 @@ describe('step-up controller', () => { }); it('rejects WebAuthn step-up responses that include PRF output', async () => { - const user = buildUser({ challenge: 'challenge' }); + const user = buildUser(); const req = buildReq({ user, body: { @@ -322,7 +332,8 @@ describe('step-up controller', () => { }); it('rejects finishing step-up when the challenge or assertion id is missing', async () => { - const user = buildUser({ challenge: null }); + const user = buildUser(); + (WebAuthnChallenge.findOne as any).mockResolvedValue(null); const req = buildReq({ user, body: { assertionResponse: { id: 'cred-1' } } }); const res = buildRes(); @@ -334,7 +345,7 @@ describe('step-up controller', () => { }); it('rejects finishing step-up when the credential is not found', async () => { - const user = buildUser({ challenge: 'challenge' }); + const user = buildUser(); (Credential.findOne as any).mockResolvedValue(null); const req = buildReq({ user, body: { assertionResponse: { id: 'cred-1' } } }); @@ -347,7 +358,7 @@ describe('step-up controller', () => { }); it('rejects finishing step-up when the session cannot be recorded', async () => { - const user = buildUser({ challenge: 'challenge' }); + const user = buildUser(); const credential = buildCredential({ id: 'cred-1', userId: user.id }); (Credential.findOne as any).mockResolvedValue(credential); @@ -374,7 +385,7 @@ describe('step-up controller', () => { }); it('returns 401 when verification throws during finish', async () => { - const user = buildUser({ challenge: 'challenge' }); + const user = buildUser(); const credential = buildCredential({ id: 'cred-1', userId: user.id }); (Credential.findOne as any).mockResolvedValue(credential); @@ -391,7 +402,9 @@ describe('step-up controller', () => { await finishWebAuthnStepUp(req, res); - expect(user.update).toHaveBeenCalledWith({ challenge: null }); + expect(challengeRecord.update).toHaveBeenCalledWith( + expect.objectContaining({ consumedAt: expect.any(Date) }), + ); expect(res.status).toHaveBeenCalledWith(401); expect(res.json).toHaveBeenCalledWith({ error: 'step_up_failed' }); }); diff --git a/tests/unit/models/models.spec.ts b/tests/unit/models/models.spec.ts index f7ed32d..9f332e2 100644 --- a/tests/unit/models/models.spec.ts +++ b/tests/unit/models/models.spec.ts @@ -10,6 +10,7 @@ vi.unmock('../../../src/models/magicLinks.js'); vi.unmock('../../../src/models/oauthIdentities.js'); vi.unmock('../../../src/models/organizations.js'); vi.unmock('../../../src/models/organizationMemberships.js'); +vi.unmock('../../../src/models/webauthnChallenges.js'); describe('models initialization', () => { beforeEach(() => { @@ -28,6 +29,7 @@ describe('models initialization', () => { expect(models.User).toBeDefined(); expect(models.Session).toBeDefined(); expect(models.AuthEvent).toBeDefined(); + expect(models.WebAuthnChallenge).toBeDefined(); }); it('models expose attributes', async () => { diff --git a/tests/unit/services/webauthnChallengeService.spec.ts b/tests/unit/services/webauthnChallengeService.spec.ts new file mode 100644 index 0000000..1f200f2 --- /dev/null +++ b/tests/unit/services/webauthnChallengeService.spec.ts @@ -0,0 +1,124 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { WebAuthnChallenge } from '../../../src/models/webauthnChallenges'; +import { + CHALLENGE_TTL_SECONDS, + consumeChallenge, + invalidateChallengesForUser, + issueChallenge, +} from '../../../src/services/webauthnChallengeService'; +import { buildWebAuthnChallenge } from '../../factories/webauthnChallengeFactory'; + +const NOW = new Date('2026-01-01T00:00:00.000Z'); + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe('issueChallenge', () => { + it('stores the challenge against one user and one flow, with a bounded life', async () => { + (WebAuthnChallenge.create as any).mockResolvedValue({}); + + await issueChallenge({ + userId: 'user-1', + purpose: 'registration', + challenge: 'abc', + now: NOW, + }); + + expect(WebAuthnChallenge.create).toHaveBeenCalledWith({ + userId: 'user-1', + purpose: 'registration', + challenge: 'abc', + context: null, + expiresAt: new Date(NOW.getTime() + CHALLENGE_TTL_SECONDS * 1000), + }); + }); + + it('spends anything still outstanding for the same flow', async () => { + (WebAuthnChallenge.create as any).mockResolvedValue({}); + + await issueChallenge({ userId: 'user-1', purpose: 'step_up', challenge: 'abc', now: NOW }); + + expect(WebAuthnChallenge.update).toHaveBeenCalledWith( + { consumedAt: NOW }, + { where: { userId: 'user-1', purpose: 'step_up', consumedAt: null } }, + ); + }); + + it('leaves a different flow alone, so registration and login can overlap', async () => { + (WebAuthnChallenge.create as any).mockResolvedValue({}); + + await issueChallenge({ + userId: 'user-1', + purpose: 'registration', + challenge: 'abc', + now: NOW, + }); + + const [, options] = (WebAuthnChallenge.update as any).mock.calls[0]; + + expect(options.where.purpose).toBe('registration'); + }); +}); + +describe('consumeChallenge', () => { + it('only looks at unspent challenges that have not expired', async () => { + (WebAuthnChallenge.findOne as any).mockResolvedValue(buildWebAuthnChallenge()); + + await consumeChallenge({ userId: 'user-1', purpose: 'authentication', now: NOW }); + + const [options] = (WebAuthnChallenge.findOne as any).mock.calls[0]; + + expect(options.where.consumedAt).toBeNull(); + expect(options.where.expiresAt).toBeDefined(); + expect(options.order).toEqual([['createdAt', 'DESC']]); + }); + + it('spends the challenge as it hands it back', async () => { + const record = buildWebAuthnChallenge({ challenge: 'abc' }); + (WebAuthnChallenge.findOne as any).mockResolvedValue(record); + + const result = await consumeChallenge({ + userId: 'user-1', + purpose: 'authentication', + now: NOW, + }); + + expect(result).toEqual({ challenge: 'abc', context: null }); + expect(record.update).toHaveBeenCalledWith({ consumedAt: NOW }); + }); + + it('returns nothing when no live challenge exists', async () => { + (WebAuthnChallenge.findOne as any).mockResolvedValue(null); + + expect( + await consumeChallenge({ userId: 'user-1', purpose: 'authentication', now: NOW }), + ).toBeNull(); + }); + + it('carries the flow context back with the challenge', async () => { + (WebAuthnChallenge.findOne as any).mockResolvedValue( + buildWebAuthnChallenge({ context: { requirePrf: true } }), + ); + + const result = await consumeChallenge({ + userId: 'user-1', + purpose: 'registration', + now: NOW, + }); + + expect(result?.context).toEqual({ requirePrf: true }); + }); +}); + +describe('invalidateChallengesForUser', () => { + it('spends every outstanding challenge across all flows', async () => { + await invalidateChallengesForUser('user-1', NOW); + + expect(WebAuthnChallenge.update).toHaveBeenCalledWith( + { consumedAt: NOW }, + { where: { userId: 'user-1', consumedAt: null } }, + ); + }); +});