From cf1c04dc01424cba3e581a4a856a81bfb5476c01 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 13:30:03 +0000 Subject: [PATCH] test(auth): cover the lock itself and its expiry, not just the counter (#3624 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3659 pinned the two ends of the 2FA failure budget — a wrong code is counted, a correct one resets it. It stopped short of the transitions that make it a LOCKOUT rather than a counter, which is the part that actually protects the account. Three more assertions: 1. **The lock engages.** Spending the budget stamps `locked_until` in the future. Reaching it means respecting two stacked budgets: better-auth allows 5 verifications per two-factor cookie (`beginAttempt(5)`) and 10 consecutive failures per ACCOUNT (`accountLockout.maxFailedAttempts`). Only the second touches the columns under test, so the loop takes a fresh challenge every 5 tries. The counter is asserted after every single failure, not just at the end. 2. **The lock bites before the code is checked.** A locked account refuses even a VALID TOTP with 429. Without this the "lockout" would just be a slower guess loop. 3. **An expired lock clears itself.** better-auth clears it through a GUARDED `incrementOne` — `where locked_until <= now`, `set { failedVerificationCount: 0, lockedUntil: null }`. That is the only place the ObjectQL adapter has to handle `lte` against a datetime, so this covers the operator as much as the columns: a driver mishandling it would leave the user locked out forever with no error anywhere. The test ages the lock through the data engine because no endpoint can — the default duration is 900s. Test-only, and `@objectstack/dogfood` is private, hence the empty changeset. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UYLC8TfjzHGwatNxZKdX7H --- .changeset/two-factor-lockout-extension.md | 4 + .../test/two-factor-lockout.dogfood.test.ts | 74 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 .changeset/two-factor-lockout-extension.md diff --git a/.changeset/two-factor-lockout-extension.md b/.changeset/two-factor-lockout-extension.md new file mode 100644 index 0000000000..e1730a8ae1 --- /dev/null +++ b/.changeset/two-factor-lockout-extension.md @@ -0,0 +1,4 @@ +--- +--- + +test-only: extends the 2FA lockout dogfood cover (#3624 follow-up). No package changes, nothing to release. diff --git a/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts b/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts index 2312174539..3d57ae86ba 100644 --- a/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts +++ b/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts @@ -213,4 +213,78 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => { ).toBe(0); expect(after.lockedUntil ?? null).toBeNull(); }); + + it('locks the account once the budget is spent, and refuses even a correct code', async () => { + // Two budgets stack here, and the test has to respect both. better-auth + // allows MAX_PER_CHALLENGE verifications per two-factor cookie + // (`beginAttempt(5)`) and MAX_FAILURES consecutive failures per ACCOUNT + // (`accountLockout.maxFailedAttempts`, default 10). Only the second one + // touches the columns under test, so reaching it takes a fresh challenge + // every MAX_PER_CHALLENGE tries. Both are better-auth's defaults — the + // auth manager passes no `accountLockout` config. + const MAX_PER_CHALLENGE = 5; + const MAX_FAILURES = 10; + + expect((await lockoutState()).count, 'precondition: a clean budget').toBe(0); + + let failures = 0; + while (failures < MAX_FAILURES) { + const cookie = await beginChallenge(); + for (let i = 0; i < MAX_PER_CHALLENGE && failures < MAX_FAILURES; i++) { + const res = await verifyWithCookie(cookie, '000000'); + expect(res.status, `wrong code #${failures + 1} produced a server error`).toBeLessThan(500); + failures++; + expect( + (await lockoutState()).count, + `the counter must advance on every failure (after #${failures})`, + ).toBe(failures); + } + } + + const locked = await lockoutState(); + expect( + locked.lockedUntil ?? null, + `budget spent (${MAX_FAILURES} failures) but locked_until was never stamped`, + ).not.toBeNull(); + expect(new Date(String(locked.lockedUntil)).getTime()).toBeGreaterThan(Date.now()); + + // The lock has to bite BEFORE the code is checked — otherwise it is not a + // lockout, just a slower guess loop. + const cookie = await beginChallenge(); + const rejected = await verifyWithCookie(cookie, totp(secret)); + expect( + rejected.status, + `a locked account must refuse even a valid code: ${await rejected.clone().text()}`, + ).toBe(429); + }); + + it('lazily clears an expired lock on the next attempt', async () => { + const before = await lockoutState(); + expect(before.lockedUntil ?? null, 'precondition: carries the lock from the previous test').not.toBeNull(); + + // Expire the lock rather than waiting out `durationSeconds` (900 by + // default). This is the one place the test reaches past the API: there is + // no endpoint that ages a lock. + const users = await ql.find('sys_user', { where: { email: adminEmail }, limit: 1 }, SYS); + const rows = await ql.find('sys_two_factor', { where: { user_id: String(users[0]?.id) }, limit: 1 }, SYS); + await ql.update( + 'sys_two_factor', + { id: rows[0].id, locked_until: new Date(Date.now() - 60_000).toISOString() }, + SYS, + ); + + // better-auth clears an expired lock through a GUARDED incrementOne — + // `where locked_until <= now`, `set { failedVerificationCount: 0, + // lockedUntil: null }`. That is the only place the adapter has to handle + // `lte` against a datetime, so this covers the operator as much as the + // columns: a driver that mishandled it would leave the user locked out + // forever with no error anywhere. + const cookie = await beginChallenge(); + const ok = await verifyWithCookie(cookie, totp(secret)); + expect(ok.status, `expired lock was not cleared: ${await ok.clone().text()}`).toBe(200); + + const after = await lockoutState(); + expect(after.lockedUntil ?? null, 'the expired lock must be cleared, not merely ignored').toBeNull(); + expect(after.count, 'clearing an expired lock must reset the failure budget too').toBe(0); + }); });