From 18430c06224c672c1922a0d329bad569788c876a Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Fri, 28 Aug 2026 23:50:30 -0400 Subject: [PATCH] fix(webauthn): give challenges their own store, with expiry and one-time use Challenges lived in a single users.challenge column shared by registration, login and step-up, so the three flows clobbered each other and a second tab invalidated the first. The column also had no lifetime: the timeout in the credential options is only a hint to the browser, and nothing server side ever enforced it, so a challenge stayed valid until some later flow happened to overwrite it. Challenges now live in webauthn_challenges, keyed by user and flow, with a server enforced five minute life. That is comfortably longer than the sixty second client hint, so a user hunting for a security key is not cut off. They are also spent when verification reads them, before anything else can fail, so no outcome leaves a redeemable challenge behind. That closes the replay finding tracked privately alongside this issue, whose fix belonged in the same change. The per-flow context that used to sit in users.challengeContext travels with the challenge it was issued for. 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 once it has run in production. Migration verified up and down against Postgres 17. Closes #164 --- .changeset/brave-hoops-turn.md | 28 ++++ src/controllers/magicLinks.ts | 7 +- src/controllers/stepUp.ts | 14 +- src/controllers/webauthn.ts | 45 ++++--- ...60829040000-create-webauthn-challenges.cjs | 74 +++++++++++ src/models/webauthnChallenges.ts | 97 ++++++++++++++ src/services/webauthnChallengeService.ts | 117 +++++++++++++++++ tests/factories/webauthnChallengeFactory.ts | 21 +++ tests/integration/stepUp/stepUp.spec.ts | 3 + tests/integration/webauthn/webauthn.spec.ts | 78 +++++++++-- tests/setup/mocks.ts | 9 ++ tests/unit/controllers/stepUp.spec.ts | 33 +++-- tests/unit/models/models.spec.ts | 2 + .../services/webauthnChallengeService.spec.ts | 124 ++++++++++++++++++ 14 files changed, 605 insertions(+), 47 deletions(-) create mode 100644 .changeset/brave-hoops-turn.md create mode 100644 src/migrations/20260829040000-create-webauthn-challenges.cjs create mode 100644 src/models/webauthnChallenges.ts create mode 100644 src/services/webauthnChallengeService.ts create mode 100644 tests/factories/webauthnChallengeFactory.ts create mode 100644 tests/unit/services/webauthnChallengeService.spec.ts 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 } }, + ); + }); +});