fix(lockout): keep passkey sign-in out of lockout and enforce lockoutSeconds

.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.

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({

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({

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<number> {
-  const count = await AuthFailure.count({
+export async function recentFailureTimes(userId: string, limit: number): Promise<Date[]> {
+  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));
 }

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<LockoutPolicy> {
   };
 }
 
+/**
+ * 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,
   };

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 () => {

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,

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 () => {

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(),
   },
 }));

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);
   });
 });