diff --git a/.changeset/lockout-spares-passkeys.md b/.changeset/lockout-spares-passkeys.md new file mode 100644 index 0000000..931831c --- /dev/null +++ b/.changeset/lockout-spares-passkeys.md @@ -0,0 +1,12 @@ +--- +'seamless-auth-api': minor +--- + +Account lockout no longer takes away a user's passkeys, and `lockoutSeconds` is enforced (GHSA-66gq-4pwg-33jq). + +- Passkey sign-in is not subject to lockout, and failed passkey assertions no longer count towards it. An assertion cannot be guessed, and because `/login` issues an ephemeral token from an email address alone, anyone could previously submit wrong OTP codes until an account locked and so shut its owner out of every method, passkeys included. +- `POST /login` for a locked account now continues with `loginMethods: ["passkey"]` when the account has a passkey, and answers `423 account_locked` only when it has none. +- The lock starts at the failure that brings `maxFailures` inside one `windowSeconds` window and lasts `lockoutSeconds` from there. Previously the lock lasted only as long as the failures stayed inside the trailing window, whatever `lockoutSeconds` said, and `retryAfterSeconds` reported `lockoutSeconds` rather than the time actually left. It is now the time left. +- The lockout counter only counts the failure types that lock, so passkey failures recorded before this release stop counting. + +An account with no passkey can still be locked by repeated wrong codes, which is what lockout is for. Enrolling a passkey keeps a user able to sign in through a lock someone else triggered. diff --git a/src/controllers/authentication.ts b/src/controllers/authentication.ts index 21a3ef4..327b8d7 100644 --- a/src/controllers/authentication.ts +++ b/src/controllers/authentication.ts @@ -23,7 +23,7 @@ import { decoyPrincipalForSubject, decoySubjectFor, } from '../services/decoyPrincipal.js'; -import { rejectIfUserLocked } from '../services/lockoutPolicyService.js'; +import { getUserLockoutStatus } from '../services/lockoutPolicyService.js'; import { getLoginPolicy, LoginMethod, @@ -306,9 +306,7 @@ export const login = async (req: Request, res: Response) => { }); } - if (await rejectIfUserLocked({ userId: user.id, req, res })) { - return; - } + const lockout = await getUserLockoutStatus(user.id); if (!user.verified) { logger.warn('Login attempt for unverified account'); @@ -328,13 +326,39 @@ export const login = async (req: Request, res: Response) => { Credential.findOne({ where: { userId: user.id } }), getLoginPolicy(), ]); - const loginMethods = resolveAvailableLoginMethods({ + const availableMethods = resolveAvailableLoginMethods({ policy: loginPolicy, user, hasPasskeyCredential: Boolean(credential), passkeyAvailable, }); + // A lock stops the code factors, which can be guessed. A passkey cannot, so a locked + // account can still continue with one, which keeps a lock someone else triggered + // from shutting the owner out. + const loginMethods = lockout.locked + ? availableMethods.filter((method) => method === 'passkey') + : availableMethods; + + if (lockout.locked && loginMethods.length === 0) { + await AuthEventService.log({ + userId: user.id, + type: 'login_suspicious', + req, + metadata: { + reason: 'Account lockout policy active', + failureCount: lockout.failureCount, + retryAfterSeconds: lockout.retryAfterSeconds, + }, + }); + + return res.status(423).json({ + error: 'account_locked', + message: 'Too many failed authentication attempts. Try again later.', + retryAfterSeconds: lockout.retryAfterSeconds, + }); + } + if (loginMethods.length === 0) { logger.error('Login attempt had no allowed continuation methods'); return await respondWithDecoy({ diff --git a/src/controllers/webauthn.ts b/src/controllers/webauthn.ts index 8f2015a..cb809e4 100644 --- a/src/controllers/webauthn.ts +++ b/src/controllers/webauthn.ts @@ -29,7 +29,6 @@ import type { WebAuthnAuthenticatorAttachment } from '../schemas/webauthn.reques import { serializeCredential } from '../services/apiResponseSerializers.js'; import { evaluateAuthenticatorPolicy } from '../services/authenticatorPolicyService.js'; import { AuthEventService } from '../services/authEventService.js'; -import { rejectIfUserLocked } from '../services/lockoutPolicyService.js'; import { ensureMetadataServiceReady, hasMetadataStatement, @@ -500,10 +499,9 @@ const verifyWebAuthn = async (req: Request, res: Response) => { const phone = verifiedUser.phone; let user = verifiedUser; - if (await rejectIfUserLocked({ userId: user.id, req, res })) { - return; - } - + // No lockout check. Lockout exists to stop guessing, and an assertion cannot be + // guessed. Applying it here let failures of a code factor, which anyone holding an + // ephemeral token can produce, take a user's passkeys away too. if (!phone && !email) { logger.error('No pre authenticated Identifier found'); await AuthEventService.log({ diff --git a/src/services/authFailureCounter.ts b/src/services/authFailureCounter.ts index a148ce3..8149572 100644 --- a/src/services/authFailureCounter.ts +++ b/src/services/authFailureCounter.ts @@ -20,7 +20,9 @@ const logger = getLogger('authFailureCounter'); */ export const LOCKOUT_FAILURE_TYPES = [ 'login_failed', - 'webauthn_login_failed', + // Not 'webauthn_login_failed'. An assertion cannot be guessed, so counting failed + // ones protects nothing, and it let anyone holding an ephemeral token lock a user + // out of passkeys by posting garbage. 'verify_otp_failed', 'totp_failed', // A failed second factor is a failed authentication attempt like any other. Left @@ -73,20 +75,26 @@ export async function recordAuthFailure(params: { } /** - * How many failures a user has accumulated inside the window. + * When a user's most recent lockout failures happened, newest first, at most `limit`. * - * Throws rather than returning a number it does not have. The caller decides - * what an unknown count means, and for lockout that has to be "assume locked": - * the previous implementation coalesced a failed query to zero, which read as - * not locked and removed the control exactly when the database was unhealthy. + * Filtered by type as well as user, so rows recorded under a type that no longer + * counts (passkey failures, before they were dropped) do not keep an account locked. + * + * Throws rather than returning a list it does not have. The caller decides what an + * unknown history means, and for lockout that has to be "assume locked": the previous + * implementation coalesced a failed query to zero, which read as not locked and + * removed the control exactly when the database was unhealthy. */ -export async function countRecentFailures(userId: string, since: Date): Promise { - const count = await AuthFailure.count({ +export async function recentFailureTimes(userId: string, limit: number): Promise { + const rows = await AuthFailure.findAll({ + attributes: ['occurredAt'], where: { userId, - occurredAt: { [Op.gte]: since }, + type: { [Op.in]: LOCKOUT_FAILURE_TYPES }, }, + order: [['occurredAt', 'DESC']], + limit, }); - return Number(count); + return rows.map((row) => new Date(row.occurredAt)); } diff --git a/src/services/lockoutPolicyService.ts b/src/services/lockoutPolicyService.ts index b9114ff..b85f4c6 100644 --- a/src/services/lockoutPolicyService.ts +++ b/src/services/lockoutPolicyService.ts @@ -10,7 +10,7 @@ import { getSystemConfig } from '../config/getSystemConfig.js'; import type { LockoutPolicy } from '../schemas/systemConfig.schema.js'; import getLogger from '../utils/logger.js'; import { AuthEventService } from './authEventService.js'; -import { countRecentFailures } from './authFailureCounter.js'; +import { recentFailureTimes } from './authFailureCounter.js'; const logger = getLogger('lockoutPolicy'); @@ -37,6 +37,14 @@ async function getLockoutPolicy(): Promise { }; } +/** + * Whether a user is locked out, and for how long. + * + * The lock starts at the failure that brought `maxFailures` inside one `windowSeconds` + * window and lasts `lockoutSeconds` from there. It used to be "at least `maxFailures` + * inside the trailing window", which ignored `lockoutSeconds` entirely and reported it + * as the retry time whatever was actually left. + */ export async function getUserLockoutStatus(userId: string, now = new Date()) { const policy = await getLockoutPolicy(); @@ -50,12 +58,10 @@ export async function getUserLockoutStatus(userId: string, now = new Date()) { }; } - const windowStart = new Date(now.getTime() - policy.windowSeconds * 1000); - - let failureCount: number; + let failures: Date[]; try { - failureCount = await countRecentFailures(userId, windowStart); + failures = await recentFailureTimes(userId, policy.maxFailures); } catch (error) { // Fail closed. This used to coalesce a failed query to zero, which read as // not locked, so the brute force protection came off exactly when the @@ -72,10 +78,23 @@ export async function getUserLockoutStatus(userId: string, now = new Date()) { }; } + const windowStart = now.getTime() - policy.windowSeconds * 1000; + const failureCount = failures.filter((at) => at.getTime() >= windowStart).length; + + if (failures.length < policy.maxFailures) { + return { locked: false, failureCount, retryAfterSeconds: 0, policy, countUnavailable: false }; + } + + const newest = failures[0].getTime(); + const oldest = failures[failures.length - 1].getTime(); + const crossedThreshold = newest - oldest <= policy.windowSeconds * 1000; + const lockedUntil = newest + policy.lockoutSeconds * 1000; + const locked = crossedThreshold && now.getTime() < lockedUntil; + return { - locked: failureCount >= policy.maxFailures, + locked, failureCount, - retryAfterSeconds: policy.lockoutSeconds, + retryAfterSeconds: locked ? Math.ceil((lockedUntil - now.getTime()) / 1000) : 0, policy, countUnavailable: false, }; diff --git a/tests/integration/authentication/authentication.spec.ts b/tests/integration/authentication/authentication.spec.ts index fe5f5f6..e4b4c20 100644 --- a/tests/integration/authentication/authentication.spec.ts +++ b/tests/integration/authentication/authentication.spec.ts @@ -362,9 +362,12 @@ describe('POST /login', () => { ); }); - it('rejects login for a locked account', async () => { + it('rejects login for a locked account with no passkey', async () => { (User.findOne as any).mockResolvedValue(buildUser({ verified: true })); - (AuthFailure.count as any).mockResolvedValue(10); + (Credential.findOne as any).mockResolvedValue(null); + (AuthFailure.findAll as any).mockResolvedValue( + Array.from({ length: 10 }, () => ({ occurredAt: new Date() })), + ); (getSystemConfig as any).mockResolvedValue({ lockout_policy: { enabled: true, maxFailures: 10, windowSeconds: 900, lockoutSeconds: 900 }, }); @@ -373,6 +376,27 @@ describe('POST /login', () => { expect(res.status).toBe(423); expect(res.body.error).toBe('account_locked'); + expect(res.body.retryAfterSeconds).toBeGreaterThan(0); + expect(res.body.retryAfterSeconds).toBeLessThanOrEqual(900); + }); + + it('lets a locked account continue with its passkey only', async () => { + (User.findOne as any).mockResolvedValue(buildUser({ verified: true })); + (Credential.findOne as any).mockResolvedValue({ id: 'cred-1' }); + (signEphemeralToken as any).mockResolvedValue('token'); + (AuthFailure.findAll as any).mockResolvedValue( + Array.from({ length: 10 }, () => ({ occurredAt: new Date() })), + ); + (getSystemConfig as any).mockResolvedValue({ + login_methods: ['passkey', 'email_otp', 'magic_link'], + passkey_login_fallback_enabled: true, + lockout_policy: { enabled: true, maxFailures: 10, windowSeconds: 900, lockoutSeconds: 900 }, + }); + + const res = await request(app).post('/login').send({ identifier: 'test@example.com' }); + + expect(res.status).toBe(200); + expect(res.body.loginMethods).toEqual(['passkey']); }); it('logs in with default TTL when no access token TTL is configured', async () => { diff --git a/tests/integration/totp/totp.spec.ts b/tests/integration/totp/totp.spec.ts index b0731c0..982ee10 100644 --- a/tests/integration/totp/totp.spec.ts +++ b/tests/integration/totp/totp.spec.ts @@ -91,7 +91,9 @@ describe('TOTP routes', () => { // Step-up elevates a session for operations such as device replacement recovery, so // an account the lockout policy has stopped must not be able to elevate through it. it('refuses TOTP step-up while the account is locked', async () => { - (AuthFailure.count as any).mockResolvedValue(10); + (AuthFailure.findAll as any).mockResolvedValue( + Array.from({ length: 10 }, () => ({ occurredAt: new Date() })), + ); (getSystemConfig as any).mockResolvedValue({ lockout_policy: { enabled: true, diff --git a/tests/integration/webauthn/webauthn.spec.ts b/tests/integration/webauthn/webauthn.spec.ts index 8a02c8a..db7065e 100644 --- a/tests/integration/webauthn/webauthn.spec.ts +++ b/tests/integration/webauthn/webauthn.spec.ts @@ -1228,8 +1228,12 @@ describe('POST /webauthn/login/finish', () => { expect(res.body).toEqual({ error: 'prf_output_not_allowed' }); }); - it('returns 423 when the account is locked', async () => { - (AuthFailure.count as any).mockResolvedValue(10); + it('does not apply lockout to passkey sign-in', async () => { + // An assertion cannot be guessed, and failures of a code factor, which anyone with + // an ephemeral token can produce, must not take the owner's passkeys away. + (AuthFailure.findAll as any).mockResolvedValue( + Array.from({ length: 10 }, () => ({ occurredAt: new Date() })), + ); (getSystemConfig as any).mockResolvedValue({ lockout_policy: { enabled: true, @@ -1243,8 +1247,8 @@ describe('POST /webauthn/login/finish', () => { .post('/webauthn/login/finish') .send({ assertionResponse: { id: 'cred-1' } }); - expect(res.status).toBe(423); - expect(res.body.error).toBe('account_locked'); + expect(res.status).not.toBe(423); + expect(AuthFailure.findAll).not.toHaveBeenCalled(); }); it('returns 500 when the assertion verification throws', async () => { diff --git a/tests/setup/mocks.ts b/tests/setup/mocks.ts index b3e023f..0fed258 100644 --- a/tests/setup/mocks.ts +++ b/tests/setup/mocks.ts @@ -41,10 +41,11 @@ vi.mock('../../src/models/systemConfig.js', () => ({ vi.mock('../../src/models/authFailures.js', () => ({ AuthFailure: { create: vi.fn(), - // Defaults to zero so the shared mock user is not locked out. A spec that + // Defaults to no failures so the shared mock user is not locked out. A spec that // exercises lockout sets its own value, and a spec that makes this reject is // exercising the fail-closed path deliberately. count: vi.fn(() => Promise.resolve(0)), + findAll: vi.fn(() => Promise.resolve([])), destroy: vi.fn(), }, })); diff --git a/tests/unit/services/authFailureCounter.spec.ts b/tests/unit/services/authFailureCounter.spec.ts index bb5e0b1..85915c3 100644 --- a/tests/unit/services/authFailureCounter.spec.ts +++ b/tests/unit/services/authFailureCounter.spec.ts @@ -3,7 +3,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { AuthFailure } from '../../../src/models/authFailures.js'; import { getAuditHealth, resetAuditHealthForTests } from '../../../src/services/auditHealth.js'; import { - countRecentFailures, + recentFailureTimes, isLockoutFailureType, recordAuthFailure, } from '../../../src/services/authFailureCounter.js'; @@ -58,25 +58,27 @@ describe('recordAuthFailure', () => { }); }); -describe('countRecentFailures', () => { - it('counts a user inside the window', async () => { - (AuthFailure.count as any).mockResolvedValue(4); - const since = new Date('2026-08-31T00:00:00.000Z'); +describe('recentFailureTimes', () => { + it('returns the newest lockout failures for a user, filtered by type', async () => { + const at = new Date('2026-10-16T12:00:00.000Z'); + (AuthFailure.findAll as any).mockResolvedValue([{ occurredAt: at }]); - await expect(countRecentFailures('user-1', since)).resolves.toBe(4); - expect(AuthFailure.count).toHaveBeenCalledWith( - expect.objectContaining({ where: expect.objectContaining({ userId: 'user-1' }) }), + await expect(recentFailureTimes('user-1', 10)).resolves.toEqual([at]); + expect(AuthFailure.findAll).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ userId: 'user-1', type: expect.anything() }), + order: [['occurredAt', 'DESC']], + limit: 10, + }), ); }); - // The caller decides what an unknown count means. Returning a number here would + // The caller decides what an unknown history means. Returning a list here would // be inventing one, which is exactly how the old `|| 0` removed the control. - it('propagates a failed query instead of coalescing it to zero', async () => { - (AuthFailure.count as any).mockRejectedValue(new Error('connection pool exhausted')); + it('propagates a failed query instead of inventing an empty history', async () => { + (AuthFailure.findAll as any).mockRejectedValue(new Error('connection pool exhausted')); - await expect(countRecentFailures('user-1', new Date())).rejects.toThrow( - 'connection pool exhausted', - ); + await expect(recentFailureTimes('user-1', 10)).rejects.toThrow('connection pool exhausted'); }); }); @@ -88,5 +90,7 @@ describe('isLockoutFailureType', () => { // step-up was the one code-checking endpoint whose guesses reached no counter. expect(isLockoutFailureType('mfa_otp_failed')).toBe(true); expect(isLockoutFailureType('login_success')).toBe(false); + // An assertion cannot be guessed, so a failed one does not count towards a lock. + expect(isLockoutFailureType('webauthn_login_failed')).toBe(false); }); }); diff --git a/tests/unit/services/lockoutPolicyService.spec.ts b/tests/unit/services/lockoutPolicyService.spec.ts index a5c20fd..6485e5a 100644 --- a/tests/unit/services/lockoutPolicyService.spec.ts +++ b/tests/unit/services/lockoutPolicyService.spec.ts @@ -5,7 +5,7 @@ vi.mock('../../../src/config/getSystemConfig.js', () => ({ })); vi.mock('../../../src/services/authFailureCounter.js', () => ({ - countRecentFailures: vi.fn(), + recentFailureTimes: vi.fn(), })); vi.mock('../../../src/services/authEventService.js', () => ({ @@ -16,12 +16,21 @@ vi.mock('../../../src/services/authEventService.js', () => ({ import { getSystemConfig } from '../../../src/config/getSystemConfig.js'; import { AuthEventService } from '../../../src/services/authEventService.js'; -import { countRecentFailures } from '../../../src/services/authFailureCounter.js'; +import { recentFailureTimes } from '../../../src/services/authFailureCounter.js'; import { getUserLockoutStatus, rejectIfUserLocked, } from '../../../src/services/lockoutPolicyService.js'; +const NOW = new Date('2026-10-16T12:00:00.000Z'); + +/** Failures `secondsAgo` before NOW, newest first, the order the counter returns. */ +function failuresAt(...secondsAgo: number[]) { + return secondsAgo + .sort((a, b) => a - b) + .map((seconds) => new Date(NOW.getTime() - seconds * 1000)); +} + describe('lockoutPolicyService', () => { beforeEach(() => { vi.clearAllMocks(); @@ -47,14 +56,14 @@ describe('lockoutPolicyService', () => { countUnavailable: false, policy: expect.objectContaining({ enabled: false }), }); - expect(countRecentFailures).not.toHaveBeenCalled(); + expect(recentFailureTimes).not.toHaveBeenCalled(); }); it('falls back to the default policy when config lookup fails', async () => { (getSystemConfig as any).mockRejectedValue(new Error('config unavailable')); - (countRecentFailures as any).mockResolvedValue(10); + (recentFailureTimes as any).mockResolvedValue(failuresAt(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)); - await expect(getUserLockoutStatus('user-1')).resolves.toEqual( + await expect(getUserLockoutStatus('user-1', NOW)).resolves.toEqual( expect.objectContaining({ locked: true, failureCount: 10, @@ -64,7 +73,7 @@ describe('lockoutPolicyService', () => { }); it('does nothing when the account is not locked', async () => { - (countRecentFailures as any).mockResolvedValue(0); + (recentFailureTimes as any).mockResolvedValue([]); const req = { ip: '127.0.0.1', headers: {} } as any; const res = { status: vi.fn().mockReturnThis(), json: vi.fn().mockReturnThis() } as any; @@ -75,9 +84,9 @@ describe('lockoutPolicyService', () => { }); it('reports unlocked when failures are below the configured threshold', async () => { - (countRecentFailures as any).mockResolvedValue(2); + (recentFailureTimes as any).mockResolvedValue(failuresAt(10, 20)); - await expect(getUserLockoutStatus('user-1')).resolves.toEqual( + await expect(getUserLockoutStatus('user-1', NOW)).resolves.toEqual( expect.objectContaining({ locked: false, failureCount: 2, @@ -86,9 +95,9 @@ describe('lockoutPolicyService', () => { }); it('reports locked when failures meet the configured threshold', async () => { - (countRecentFailures as any).mockResolvedValue(3); + (recentFailureTimes as any).mockResolvedValue(failuresAt(0, 30, 60)); - await expect(getUserLockoutStatus('user-1')).resolves.toEqual( + await expect(getUserLockoutStatus('user-1', NOW)).resolves.toEqual( expect.objectContaining({ locked: true, failureCount: 3, @@ -97,8 +106,56 @@ describe('lockoutPolicyService', () => { ); }); + it('counts the lock from the failure that crossed the threshold', async () => { + (recentFailureTimes as any).mockResolvedValue(failuresAt(200, 230, 260)); + + await expect(getUserLockoutStatus('user-1', NOW)).resolves.toEqual( + expect.objectContaining({ locked: true, retryAfterSeconds: 400 }), + ); + }); + + // lockoutSeconds used to be ignored: the account unlocked as soon as the failures + // aged out of the window, whatever lockoutSeconds said. + it('keeps the account locked for lockoutSeconds even after the window has passed', async () => { + (getSystemConfig as any).mockResolvedValue({ + lockout_policy: { enabled: true, maxFailures: 3, windowSeconds: 60, lockoutSeconds: 3600 }, + }); + (recentFailureTimes as any).mockResolvedValue(failuresAt(1200, 1210, 1220)); + + await expect(getUserLockoutStatus('user-1', NOW)).resolves.toEqual( + expect.objectContaining({ locked: true, failureCount: 0, retryAfterSeconds: 2400 }), + ); + }); + + it('unlocks once lockoutSeconds has passed, even if the window is longer', async () => { + (getSystemConfig as any).mockResolvedValue({ + lockout_policy: { enabled: true, maxFailures: 3, windowSeconds: 3600, lockoutSeconds: 60 }, + }); + (recentFailureTimes as any).mockResolvedValue(failuresAt(120, 130, 140)); + + await expect(getUserLockoutStatus('user-1', NOW)).resolves.toEqual( + expect.objectContaining({ locked: false, failureCount: 3, retryAfterSeconds: 0 }), + ); + }); + + it('does not lock when the failures are spread wider than one window', async () => { + (recentFailureTimes as any).mockResolvedValue(failuresAt(0, 500, 1000)); + + await expect(getUserLockoutStatus('user-1', NOW)).resolves.toEqual( + expect.objectContaining({ locked: false }), + ); + }); + + it('asks the counter for no more failures than the threshold', async () => { + (recentFailureTimes as any).mockResolvedValue([]); + + await getUserLockoutStatus('user-1', NOW); + + expect(recentFailureTimes).toHaveBeenCalledWith('user-1', 3); + }); + it('returns a lockout response and audit event when active', async () => { - (countRecentFailures as any).mockResolvedValue(3); + (recentFailureTimes as any).mockResolvedValue([new Date(), new Date(), new Date()]); const req = { ip: '127.0.0.1', @@ -130,7 +187,7 @@ describe('lockoutPolicyService', () => { // as not locked, so brute force protection came off exactly when the database // was unhealthy. An unknown count now means locked. it('treats the account as locked when the counter cannot be read', async () => { - (countRecentFailures as any).mockRejectedValue(new Error('connection pool exhausted')); + (recentFailureTimes as any).mockRejectedValue(new Error('connection pool exhausted')); const status = await getUserLockoutStatus('user-1'); @@ -139,7 +196,7 @@ describe('lockoutPolicyService', () => { }); it('refuses the request when the counter cannot be read', async () => { - (countRecentFailures as any).mockRejectedValue(new Error('table is locked')); + (recentFailureTimes as any).mockRejectedValue(new Error('table is locked')); const res: any = { status: vi.fn().mockReturnThis(), json: vi.fn().mockReturnThis() }; await expect(rejectIfUserLocked({ userId: 'user-1', req: {} as never, res })).resolves.toBe(