Skip to content

Commit 8f07d8e

Browse files
committed
fix(auth): make the second factor obey the operator's lockout policy (#3690)
`twoFactor()` was constructed with a schema and nothing else, so better-auth's built-in `accountLockout` defaults (on, 10 attempts, 15 minutes) governed two-factor verification regardless of what the admin configured. An operator who tightened the account lockout threshold to 3 got a password stage that locked at 3 and a second factor that still locked at 10 — the stricter door was the looser one, and nothing in the UI said so. Project `lockoutThreshold` / `lockoutDurationMinutes` onto better-auth's own `accountLockout` shape rather than adding a parallel `two_factor_lockout_*` pair: one policy, one mental model, and an upstream addition arrives as a new option instead of a conflict. The projection runs through `applyConfigPatch`, which resets the cached instance, so a settings change lands without a restart. Threshold `0` is deliberately not forwarded as `enabled: false`. It is the password stage's "off", and a deployment may leave that stage unlocked because rate limiting or an IdP covers it; the second factor is the last check before a session is issued, so it keeps better-auth's default instead of being switched off by a setting that never mentioned it. The threshold field also stops hiding behind `email_password_enabled` — two-factor verification exists in passwordless deployments, where it was previously unreachable. The dogfood test now runs under an explicit policy (7 attempts / 40 minutes), chosen to differ from every default, and derives its assertions from it — pinned to better-auth's defaults it could not tell "configured to 10" from "configuration ignored". Verified by removing the wiring: the lock never engages and both budget tests fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYLC8TfjzHGwatNxZKdX7H
1 parent 3177d51 commit 8f07d8e

10 files changed

Lines changed: 238 additions & 24 deletions

File tree

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
"@objectstack/service-settings": patch
4+
---
5+
6+
fix(auth): the second factor now obeys the operator's lockout policy instead of better-auth's defaults (#3690)
7+
8+
`auth-manager.ts` constructed `twoFactor()` with a schema and nothing else, so
9+
better-auth's built-in `accountLockout` defaults — on, 10 attempts, 15 minutes —
10+
governed two-factor verification no matter what the admin configured. An operator
11+
who tightened **Setup → Authentication → Account lockout threshold** to 3 got a
12+
password stage that locked at 3 and a second factor that still locked at 10: the
13+
stricter door was the looser one, with nothing in the UI saying so.
14+
15+
`lockout_threshold` / `lockout_duration_minutes` are now projected onto
16+
better-auth's own `accountLockout` shape (`enabled` / `maxFailedAttempts` /
17+
`durationSeconds`, minutes converted to seconds) rather than growing a parallel
18+
`two_factor_lockout_*` pair — one policy, one mental model, and a future upstream
19+
field arrives as a new option instead of a conflict. The projection goes through
20+
`applyConfigPatch`, which resets the cached better-auth instance, so a settings
21+
change takes effect without a restart.
22+
23+
Threshold `0` is deliberately **not** forwarded as `enabled: false`. It is the
24+
password stage's "off", and a deployment may leave that stage unlocked because
25+
rate limiting or an IdP covers it; the second factor is the last check before a
26+
session is issued, so it keeps better-auth's default rather than being switched
27+
off by a setting that never mentioned it.
28+
29+
The threshold field is also no longer hidden behind `email_password_enabled`
30+
two-factor verification exists in passwordless deployments, where the setting was
31+
previously unreachable.
32+
33+
Note the plugin caps attempts at 5 per challenge (`beginAttempt(5)`), which no
34+
option reaches; a threshold above 5 forces a fresh challenge rather than raising
35+
that cap.

content/docs/permissions/authentication.mdx

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -455,10 +455,22 @@ code that reads it). Changes take effect immediately; no restart is required.
455455

456456
| Setting | Effect |
457457
|---|---|
458-
| `lockout_threshold` | Lock an account after this many consecutive failed sign-ins. |
459-
| `lockout_duration_minutes` | How long a lockout lasts. Admins can clear it early with the **Unlock** action on the user record. |
458+
| `lockout_threshold` | Lock an account after this many consecutive failed sign-in attempts. Counts **both** stages: wrong passwords and wrong two-factor codes. |
459+
| `lockout_duration_minutes` | How long a lockout lasts, at either stage. Admins can clear a password-stage lockout early with the **Unlock** action on the user record. |
460460
| `rate_limit_max` / `rate_limit_window_seconds` | Per-IP request throttle on auth endpoints. |
461461

462+
The two stages keep **separate counters**`sys_user.failed_login_count` for the
463+
password check, `sys_two_factor.failed_verification_count` for the second factor —
464+
so a locked password stage and a locked second factor are independent states. Each
465+
counter resets on a success at its own stage.
466+
467+
Setting `lockout_threshold` to `0` disables the **password-stage** lockout only.
468+
Two-factor verification keeps a built-in limit of 10 attempts per 15 minutes,
469+
because it is the last check before a session is issued. Two-factor verification
470+
additionally caps attempts at 5 per challenge, which is not configurable: a
471+
threshold above 5 simply forces the attacker to restart the sign-in flow more
472+
often before the account-level lock engages.
473+
462474
### Multi-factor (enforced MFA)
463475

464476
| Setting | Effect |

packages/plugins/plugin-auth/src/auth-manager.test.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -885,6 +885,62 @@ describe('AuthManager', () => {
885885
expect(tfPlugin._opts.schema.user.fields.twoFactorEnabled).toBe('two_factor_enabled');
886886
});
887887

888+
// #3690 — the operator's lockout policy has to reach the SECOND factor too.
889+
// Before this, `twoFactor()` got only a schema, so the second factor sat on
890+
// better-auth's built-in 10/900s no matter how the admin tuned sign-in:
891+
// tightening the threshold to 3 left the last door before a session at 10.
892+
describe('two-factor account lockout follows the operator settings (#3690)', () => {
893+
const buildTwoFactor = async (extra: Record<string, unknown>) => {
894+
let capturedConfig: any;
895+
(betterAuth as any).mockImplementation((config: any) => {
896+
capturedConfig = config;
897+
return { handler: vi.fn(), api: {} };
898+
});
899+
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
900+
const manager = new AuthManager({
901+
secret: 'test-secret-at-least-32-chars-long',
902+
baseUrl: 'http://localhost:3000',
903+
plugins: { twoFactor: true },
904+
...extra,
905+
});
906+
await manager.getAuthInstance();
907+
warnSpy.mockRestore();
908+
return capturedConfig.plugins.find((p: any) => p.id === 'two-factor')._opts.accountLockout;
909+
};
910+
911+
it('projects lockoutThreshold / lockoutDurationMinutes onto better-auth\'s contract', async () => {
912+
// Minutes → seconds is the whole reason this needs a test: the two
913+
// sides use different units for the same policy.
914+
expect(await buildTwoFactor({ lockoutThreshold: 3, lockoutDurationMinutes: 30 })).toEqual({
915+
enabled: true,
916+
maxFailedAttempts: 3,
917+
durationSeconds: 1800,
918+
});
919+
});
920+
921+
it('falls back to the password stage\'s 15-minute default when only a threshold is set', async () => {
922+
expect(await buildTwoFactor({ lockoutThreshold: 5 })).toEqual({
923+
enabled: true,
924+
maxFailedAttempts: 5,
925+
durationSeconds: 900,
926+
});
927+
});
928+
929+
// Threshold 0 is the password stage's "off". It is NOT forwarded as
930+
// `enabled: false`: a deployment may leave the password stage unlocked
931+
// because rate limiting or an IdP covers it, while the second factor —
932+
// the last check before a session — keeps better-auth's default. Leaving
933+
// the option unset is what hands that default back.
934+
for (const [label, extra] of [
935+
['threshold 0 (lockout explicitly off)', { lockoutThreshold: 0 }],
936+
['no lockout settings at all', {}],
937+
] as Array<[string, Record<string, unknown>]>) {
938+
it(`leaves better-auth's own default in place with ${label}`, async () => {
939+
expect(await buildTwoFactor(extra)).toBeUndefined();
940+
});
941+
}
942+
});
943+
888944
it('should register magicLink plugin when enabled', async () => {
889945
let capturedConfig: any;
890946
(betterAuth as any).mockImplementation((config: any) => {

packages/plugins/plugin-auth/src/auth-manager.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1855,6 +1855,9 @@ export class AuthManager {
18551855
const { twoFactor } = await import('better-auth/plugins/two-factor');
18561856
plugins.push(twoFactor({
18571857
schema: buildTwoFactorPluginSchema(),
1858+
// ADR-0069 D2 (#3690) — the operator's lockout policy governs BOTH
1859+
// stages of sign-in, not just the password one.
1860+
accountLockout: this.resolveTwoFactorAccountLockout(),
18581861
}));
18591862
}
18601863

@@ -3893,6 +3896,46 @@ export class AuthManager {
38933896
}
38943897
}
38953898

3899+
/**
3900+
* ADR-0069 D2 (#3690) — project the operator's lockout settings onto
3901+
* better-auth's own `twoFactor({ accountLockout })` contract, so the second
3902+
* factor obeys the same policy as the password stage above.
3903+
*
3904+
* Deliberately reuses `lockoutThreshold` / `lockoutDurationMinutes` rather
3905+
* than introducing a parallel `two_factor_lockout_*` pair: an admin who
3906+
* tightens sign-in to 3 attempts reasonably reads that as "the login flow is
3907+
* tightened", and before this the second factor silently stayed at
3908+
* better-auth's 10 — the stricter door was the looser one, with nothing in
3909+
* the UI saying so. Following better-auth's own field shape (rather than
3910+
* inventing ObjectStack settings on top of it) also means a future upstream
3911+
* addition here shows up as a new option, not a conflict.
3912+
*
3913+
* `undefined` when the threshold is 0/absent — better-auth's defaults (on,
3914+
* 10 attempts, 15 minutes) then stand. That is NOT mapped to
3915+
* `enabled: false`: `0` is the password stage's "off", and a deployment may
3916+
* leave THAT stage unlocked because other controls cover it (per-IP rate
3917+
* limiting, a captcha, an IdP in front). The second factor is the last door
3918+
* before a session is issued, so it is not turned off by a setting that
3919+
* never mentioned it. Both stages remain explicitly configurable; only the
3920+
* "unset" case differs, and it differs toward locking.
3921+
*
3922+
* Note the plugin ALSO caps attempts per challenge at a hardcoded 5
3923+
* (`beginAttempt(5)` in its totp / backup-code verifiers), which no option
3924+
* reaches. A threshold above 5 therefore still forces a fresh challenge
3925+
* every 5 guesses; the account budget is what accumulates across them.
3926+
*/
3927+
private resolveTwoFactorAccountLockout():
3928+
{ enabled: boolean; maxFailedAttempts: number; durationSeconds: number } | undefined {
3929+
const threshold = Number(this.config.lockoutThreshold) || 0;
3930+
if (threshold <= 0) return undefined;
3931+
const minutes = Number(this.config.lockoutDurationMinutes) || 15;
3932+
return {
3933+
enabled: true,
3934+
maxFailedAttempts: threshold,
3935+
durationSeconds: minutes * 60,
3936+
};
3937+
}
3938+
38963939
/**
38973940
* ADR-0069 D7 — stamp `last_login_at` (+ `last_login_ip` when known) on a
38983941
* successful sign-in. Best-effort and always fire-and-forget safe: a login

packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts

Lines changed: 59 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,19 @@
2828
* would therefore pass while the lockout path stayed completely unexercised —
2929
* exactly the blind spot that let the missing columns through. Hence the
3030
* cookie plumbing below: it is what makes this test test anything.
31+
*
32+
* ## Why it runs under an explicit operator policy (#3690)
33+
*
34+
* The budget used to be better-auth's own default (10 failures / 15 minutes),
35+
* asserted as a literal here. That made this file blind to #3690: the auth
36+
* manager passed no `accountLockout` at all, so the second factor ignored the
37+
* operator's `lockout_threshold` entirely — and a test pinned to the default
38+
* value could not tell "configured to 10" from "configuration ignored".
39+
*
40+
* So the stack is patched to a policy that is DIFFERENT from every default
41+
* below, and every assertion derives from it. If the wiring is ever dropped,
42+
* the counts and the lock duration snap back to better-auth's defaults and
43+
* these assertions fail — which is the point.
3144
*/
3245

3346
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
@@ -38,6 +51,19 @@ import { bootStack, type VerifyStack } from '@objectstack/verify';
3851
const SYS = { context: { isSystem: true } };
3952
const ADMIN_PASSWORD = 'admin123';
4053

54+
// [#3690] The operator policy this file runs under. Both values are chosen to
55+
// differ from better-auth's defaults (10 attempts / 15 minutes) so an assertion
56+
// cannot pass by accident if the settings stop reaching the plugin.
57+
//
58+
// 7 is also deliberately ABOVE the plugin's hardcoded per-challenge cap of 5
59+
// (`beginAttempt(5)`, not reachable by any option), so spending the budget
60+
// still has to cross a challenge boundary — the account-level counter is the
61+
// one backed by the columns under test.
62+
const LOCKOUT_THRESHOLD = 7;
63+
const LOCKOUT_DURATION_MINUTES = 40;
64+
/** better-auth's per-two-factor-cookie cap. Hardcoded upstream, not configurable. */
65+
const MAX_PER_CHALLENGE = 5;
66+
4167
// ── RFC 6238 TOTP ──────────────────────────────────────────────────────────
4268
// Hand-rolled rather than imported: `@better-auth/utils/otp` is a transitive
4369
// dependency, and adding it as a direct one to generate six digits would tie
@@ -100,6 +126,17 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => {
100126
stack = await bootStack(showcaseStack, {});
101127
ql = await stack.kernel.getServiceAsync<any>('objectql');
102128

129+
// [#3690] Apply the operator policy the same way the settings service does
130+
// — `applyConfigPatch` nulls the cached better-auth instance, so the next
131+
// request rebuilds with the new `accountLockout`. Going through this seam
132+
// (rather than constructing the manager by hand) is what proves a Setup-UI
133+
// change actually reaches the second factor.
134+
const auth = await stack.kernel.getServiceAsync<any>('auth');
135+
auth.applyConfigPatch({
136+
lockoutThreshold: LOCKOUT_THRESHOLD,
137+
lockoutDurationMinutes: LOCKOUT_DURATION_MINUTES,
138+
});
139+
103140
const token = await stack.signIn();
104141
const me = await (await stack.apiAs(token, 'GET', '/auth/get-session')).json() as any;
105142
adminEmail = me?.user?.email;
@@ -214,16 +251,18 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => {
214251
expect(after.lockedUntil ?? null).toBeNull();
215252
});
216253

217-
it('locks the account once the budget is spent, and refuses even a correct code', async () => {
254+
it('locks the account once the operator-configured budget is spent, and refuses even a correct code', async () => {
218255
// Two budgets stack here, and the test has to respect both. better-auth
219256
// allows MAX_PER_CHALLENGE verifications per two-factor cookie
220-
// (`beginAttempt(5)`) and MAX_FAILURES consecutive failures per ACCOUNT
221-
// (`accountLockout.maxFailedAttempts`, default 10). Only the second one
222-
// touches the columns under test, so reaching it takes a fresh challenge
223-
// every MAX_PER_CHALLENGE tries. Both are better-auth's defaults — the
224-
// auth manager passes no `accountLockout` config.
225-
const MAX_PER_CHALLENGE = 5;
226-
const MAX_FAILURES = 10;
257+
// (hardcoded), and MAX_FAILURES consecutive failures per ACCOUNT
258+
// (`accountLockout.maxFailedAttempts`). Only the second one touches the
259+
// columns under test, so reaching it takes a fresh challenge every
260+
// MAX_PER_CHALLENGE tries.
261+
//
262+
// MAX_FAILURES is the operator's threshold, not better-auth's default —
263+
// #3690. Were the config dropped, the account would still be unlocked at
264+
// failure 7 and the assertion below would fail on a null `locked_until`.
265+
const MAX_FAILURES = LOCKOUT_THRESHOLD;
227266

228267
expect((await lockoutState()).count, 'precondition: a clean budget').toBe(0);
229268

@@ -246,7 +285,15 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => {
246285
locked.lockedUntil ?? null,
247286
`budget spent (${MAX_FAILURES} failures) but locked_until was never stamped`,
248287
).not.toBeNull();
249-
expect(new Date(String(locked.lockedUntil)).getTime()).toBeGreaterThan(Date.now());
288+
289+
// The lock lasts the operator's duration, not better-auth's 15 minutes.
290+
// `lockoutDurationMinutes` (minutes) is projected onto the plugin's
291+
// `durationSeconds` — the units differ on the two sides, so this is where a
292+
// ×60 that went missing would show up. The window is wide enough for the
293+
// several seconds of guessing above, and far too narrow to admit 15.
294+
const lockedForMs = new Date(String(locked.lockedUntil)).getTime() - Date.now();
295+
expect(lockedForMs).toBeGreaterThan((LOCKOUT_DURATION_MINUTES - 5) * 60_000);
296+
expect(lockedForMs).toBeLessThanOrEqual(LOCKOUT_DURATION_MINUTES * 60_000);
250297

251298
// The lock has to bite BEFORE the code is checked — otherwise it is not a
252299
// lockout, just a slower guess loop.
@@ -262,9 +309,9 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => {
262309
const before = await lockoutState();
263310
expect(before.lockedUntil ?? null, 'precondition: carries the lock from the previous test').not.toBeNull();
264311

265-
// Expire the lock rather than waiting out `durationSeconds` (900 by
266-
// default). This is the one place the test reaches past the API: there is
267-
// no endpoint that ages a lock.
312+
// Expire the lock rather than waiting out `durationSeconds`. This is the
313+
// one place the test reaches past the API: there is no endpoint that ages
314+
// a lock.
268315
const users = await ql.find('sys_user', { where: { email: adminEmail }, limit: 1 }, SYS);
269316
const rows = await ql.find('sys_two_factor', { where: { user_id: String(users[0]?.id) }, limit: 1 }, SYS);
270317
await ql.update(

packages/services/service-settings/src/manifests/auth.manifest.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,24 @@ describe('authSettingsManifest', () => {
7070
expect(byKey('mfa_grace_period_days').default).toBe(7);
7171
});
7272

73+
// #3690 — the same pair now drives the second factor's lockout, not just the
74+
// password stage's. Two-factor verification exists without email/password
75+
// sign-in, so gating the threshold on `email_password_enabled` would leave a
76+
// passwordless deployment unable to tune it at all.
77+
it('keeps the lockout pair reachable in passwordless deployments (#3690)', () => {
78+
const specs = authSettingsManifest.specifiers as any[];
79+
const byKey = (k: string) => specs.find((s) => s.key === k);
80+
81+
expect(byKey('lockout_threshold').visible).toBeUndefined();
82+
// The duration is still meaningless with no threshold, so that condition
83+
// stays — but it must no longer depend on the password provider.
84+
expect(byKey('lockout_duration_minutes').visible).toBe('${data.lockout_threshold > 0}');
85+
86+
// The help text is the only place an admin learns the threshold covers
87+
// both stages; the wiring is invisible otherwise.
88+
expect(byKey('lockout_threshold').description).toMatch(/two-factor/i);
89+
});
90+
7391
it('exposes encrypted Google OAuth credential fields', () => {
7492
const keys = (authSettingsManifest.specifiers as any[])
7593
.map((s) => s.key)

packages/services/service-settings/src/manifests/auth.manifest.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,10 @@ const manifest = {
137137
description:
138138
'Brute-force protection: per-identity account lockout and per-IP rate limiting on auth endpoints.',
139139
},
140+
// [#3690] The threshold/duration pair governs BOTH sign-in stages — the
141+
// password check and the second factor. Always visible: two-factor
142+
// verification exists in passwordless deployments too, so gating these on
143+
// `email_password_enabled` would leave the 2FA lockout untunable there.
140144
{
141145
type: 'number',
142146
key: 'lockout_threshold',
@@ -146,8 +150,7 @@ const manifest = {
146150
min: 0,
147151
max: 20,
148152
description:
149-
'Lock an account after this many consecutive failed sign-ins. 0 disables lockout. While locked, sign-in is rejected even with the correct password.',
150-
visible: "${data.email_password_enabled !== false}",
153+
'Lock an account after this many consecutive failed sign-in attempts — wrong passwords and wrong two-factor codes alike. While locked, sign-in is rejected even with the correct credentials. 0 disables the password-stage lockout; two-factor verification then keeps its built-in limit of 10 attempts per 15 minutes, since it is the last check before a session is issued.',
151154
},
152155
{
153156
type: 'number',
@@ -157,8 +160,8 @@ const manifest = {
157160
default: 15,
158161
min: 1,
159162
max: 1440,
160-
description: 'How long an account stays locked once the threshold is crossed.',
161-
visible: "${data.email_password_enabled !== false && data.lockout_threshold > 0}",
163+
description: 'How long an account stays locked once the threshold is crossed, at either sign-in stage.',
164+
visible: '${data.lockout_threshold > 0}',
162165
},
163166
{
164167
type: 'number',

0 commit comments

Comments
 (0)