From 8f07d8eb4af4b91c72345bc3503596e72982da94 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 15:13:42 +0000 Subject: [PATCH 1/2] fix(auth): make the second factor obey the operator's lockout policy (#3690) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01UYLC8TfjzHGwatNxZKdX7H --- .../two-factor-lockout-follows-settings.md | 35 +++++++++ content/docs/permissions/authentication.mdx | 16 ++++- .../plugin-auth/src/auth-manager.test.ts | 56 +++++++++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 43 +++++++++++ .../test/two-factor-lockout.dogfood.test.ts | 71 +++++++++++++++---- .../src/manifests/auth.manifest.test.ts | 18 +++++ .../src/manifests/auth.manifest.ts | 11 +-- .../src/translations/es-ES.ts | 4 +- .../src/translations/ja-JP.ts | 4 +- .../src/translations/zh-CN.ts | 4 +- 10 files changed, 238 insertions(+), 24 deletions(-) create mode 100644 .changeset/two-factor-lockout-follows-settings.md diff --git a/.changeset/two-factor-lockout-follows-settings.md b/.changeset/two-factor-lockout-follows-settings.md new file mode 100644 index 0000000000..cf1605d60d --- /dev/null +++ b/.changeset/two-factor-lockout-follows-settings.md @@ -0,0 +1,35 @@ +--- +"@objectstack/plugin-auth": patch +"@objectstack/service-settings": patch +--- + +fix(auth): the second factor now obeys the operator's lockout policy instead of better-auth's defaults (#3690) + +`auth-manager.ts` constructed `twoFactor()` with a schema and nothing else, so +better-auth's built-in `accountLockout` defaults — on, 10 attempts, 15 minutes — +governed two-factor verification no matter what the admin configured. An operator +who tightened **Setup → Authentication → 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, with nothing in the UI saying so. + +`lockout_threshold` / `lockout_duration_minutes` are now projected onto +better-auth's own `accountLockout` shape (`enabled` / `maxFailedAttempts` / +`durationSeconds`, minutes converted to seconds) rather than growing a parallel +`two_factor_lockout_*` pair — one policy, one mental model, and a future upstream +field arrives as a new option instead of a conflict. The projection goes through +`applyConfigPatch`, which resets the cached better-auth instance, so a settings +change takes effect 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 rather than being switched +off by a setting that never mentioned it. + +The threshold field is also no longer hidden behind `email_password_enabled` — +two-factor verification exists in passwordless deployments, where the setting was +previously unreachable. + +Note the plugin caps attempts at 5 per challenge (`beginAttempt(5)`), which no +option reaches; a threshold above 5 forces a fresh challenge rather than raising +that cap. diff --git a/content/docs/permissions/authentication.mdx b/content/docs/permissions/authentication.mdx index 5680f2319a..1f48b15517 100644 --- a/content/docs/permissions/authentication.mdx +++ b/content/docs/permissions/authentication.mdx @@ -455,10 +455,22 @@ code that reads it). Changes take effect immediately; no restart is required. | Setting | Effect | |---|---| -| `lockout_threshold` | Lock an account after this many consecutive failed sign-ins. | -| `lockout_duration_minutes` | How long a lockout lasts. Admins can clear it early with the **Unlock** action on the user record. | +| `lockout_threshold` | Lock an account after this many consecutive failed sign-in attempts. Counts **both** stages: wrong passwords and wrong two-factor codes. | +| `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. | | `rate_limit_max` / `rate_limit_window_seconds` | Per-IP request throttle on auth endpoints. | +The two stages keep **separate counters** — `sys_user.failed_login_count` for the +password check, `sys_two_factor.failed_verification_count` for the second factor — +so a locked password stage and a locked second factor are independent states. Each +counter resets on a success at its own stage. + +Setting `lockout_threshold` to `0` disables the **password-stage** lockout only. +Two-factor verification keeps a built-in limit of 10 attempts per 15 minutes, +because it is the last check before a session is issued. Two-factor verification +additionally caps attempts at 5 per challenge, which is not configurable: a +threshold above 5 simply forces the attacker to restart the sign-in flow more +often before the account-level lock engages. + ### Multi-factor (enforced MFA) | Setting | Effect | diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index ed298b8bc0..98744cb555 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -885,6 +885,62 @@ describe('AuthManager', () => { expect(tfPlugin._opts.schema.user.fields.twoFactorEnabled).toBe('two_factor_enabled'); }); + // #3690 — the operator's lockout policy has to reach the SECOND factor too. + // Before this, `twoFactor()` got only a schema, so the second factor sat on + // better-auth's built-in 10/900s no matter how the admin tuned sign-in: + // tightening the threshold to 3 left the last door before a session at 10. + describe('two-factor account lockout follows the operator settings (#3690)', () => { + const buildTwoFactor = async (extra: Record) => { + let capturedConfig: any; + (betterAuth as any).mockImplementation((config: any) => { + capturedConfig = config; + return { handler: vi.fn(), api: {} }; + }); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const manager = new AuthManager({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'http://localhost:3000', + plugins: { twoFactor: true }, + ...extra, + }); + await manager.getAuthInstance(); + warnSpy.mockRestore(); + return capturedConfig.plugins.find((p: any) => p.id === 'two-factor')._opts.accountLockout; + }; + + it('projects lockoutThreshold / lockoutDurationMinutes onto better-auth\'s contract', async () => { + // Minutes → seconds is the whole reason this needs a test: the two + // sides use different units for the same policy. + expect(await buildTwoFactor({ lockoutThreshold: 3, lockoutDurationMinutes: 30 })).toEqual({ + enabled: true, + maxFailedAttempts: 3, + durationSeconds: 1800, + }); + }); + + it('falls back to the password stage\'s 15-minute default when only a threshold is set', async () => { + expect(await buildTwoFactor({ lockoutThreshold: 5 })).toEqual({ + enabled: true, + maxFailedAttempts: 5, + durationSeconds: 900, + }); + }); + + // Threshold 0 is the password stage's "off". It is NOT forwarded as + // `enabled: false`: a deployment may leave the password stage unlocked + // because rate limiting or an IdP covers it, while the second factor — + // the last check before a session — keeps better-auth's default. Leaving + // the option unset is what hands that default back. + for (const [label, extra] of [ + ['threshold 0 (lockout explicitly off)', { lockoutThreshold: 0 }], + ['no lockout settings at all', {}], + ] as Array<[string, Record]>) { + it(`leaves better-auth's own default in place with ${label}`, async () => { + expect(await buildTwoFactor(extra)).toBeUndefined(); + }); + } + }); + it('should register magicLink plugin when enabled', async () => { let capturedConfig: any; (betterAuth as any).mockImplementation((config: any) => { diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index e86dcdc26f..3c065bcd05 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1855,6 +1855,9 @@ export class AuthManager { const { twoFactor } = await import('better-auth/plugins/two-factor'); plugins.push(twoFactor({ schema: buildTwoFactorPluginSchema(), + // ADR-0069 D2 (#3690) — the operator's lockout policy governs BOTH + // stages of sign-in, not just the password one. + accountLockout: this.resolveTwoFactorAccountLockout(), })); } @@ -3893,6 +3896,46 @@ export class AuthManager { } } + /** + * ADR-0069 D2 (#3690) — project the operator's lockout settings onto + * better-auth's own `twoFactor({ accountLockout })` contract, so the second + * factor obeys the same policy as the password stage above. + * + * Deliberately reuses `lockoutThreshold` / `lockoutDurationMinutes` rather + * than introducing a parallel `two_factor_lockout_*` pair: an admin who + * tightens sign-in to 3 attempts reasonably reads that as "the login flow is + * tightened", and before this the second factor silently stayed at + * better-auth's 10 — the stricter door was the looser one, with nothing in + * the UI saying so. Following better-auth's own field shape (rather than + * inventing ObjectStack settings on top of it) also means a future upstream + * addition here shows up as a new option, not a conflict. + * + * `undefined` when the threshold is 0/absent — better-auth's defaults (on, + * 10 attempts, 15 minutes) then stand. That is NOT mapped to + * `enabled: false`: `0` is the password stage's "off", and a deployment may + * leave THAT stage unlocked because other controls cover it (per-IP rate + * limiting, a captcha, an IdP in front). The second factor is the last door + * before a session is issued, so it is not turned off by a setting that + * never mentioned it. Both stages remain explicitly configurable; only the + * "unset" case differs, and it differs toward locking. + * + * Note the plugin ALSO caps attempts per challenge at a hardcoded 5 + * (`beginAttempt(5)` in its totp / backup-code verifiers), which no option + * reaches. A threshold above 5 therefore still forces a fresh challenge + * every 5 guesses; the account budget is what accumulates across them. + */ + private resolveTwoFactorAccountLockout(): + { enabled: boolean; maxFailedAttempts: number; durationSeconds: number } | undefined { + const threshold = Number(this.config.lockoutThreshold) || 0; + if (threshold <= 0) return undefined; + const minutes = Number(this.config.lockoutDurationMinutes) || 15; + return { + enabled: true, + maxFailedAttempts: threshold, + durationSeconds: minutes * 60, + }; + } + /** * ADR-0069 D7 — stamp `last_login_at` (+ `last_login_ip` when known) on a * successful sign-in. Best-effort and always fire-and-forget safe: a login 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 3d57ae86ba..36e25ff4d7 100644 --- a/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts +++ b/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts @@ -28,6 +28,19 @@ * would therefore pass while the lockout path stayed completely unexercised — * exactly the blind spot that let the missing columns through. Hence the * cookie plumbing below: it is what makes this test test anything. + * + * ## Why it runs under an explicit operator policy (#3690) + * + * The budget used to be better-auth's own default (10 failures / 15 minutes), + * asserted as a literal here. That made this file blind to #3690: the auth + * manager passed no `accountLockout` at all, so the second factor ignored the + * operator's `lockout_threshold` entirely — and a test pinned to the default + * value could not tell "configured to 10" from "configuration ignored". + * + * So the stack is patched to a policy that is DIFFERENT from every default + * below, and every assertion derives from it. If the wiring is ever dropped, + * the counts and the lock duration snap back to better-auth's defaults and + * these assertions fail — which is the point. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; @@ -38,6 +51,19 @@ import { bootStack, type VerifyStack } from '@objectstack/verify'; const SYS = { context: { isSystem: true } }; const ADMIN_PASSWORD = 'admin123'; +// [#3690] The operator policy this file runs under. Both values are chosen to +// differ from better-auth's defaults (10 attempts / 15 minutes) so an assertion +// cannot pass by accident if the settings stop reaching the plugin. +// +// 7 is also deliberately ABOVE the plugin's hardcoded per-challenge cap of 5 +// (`beginAttempt(5)`, not reachable by any option), so spending the budget +// still has to cross a challenge boundary — the account-level counter is the +// one backed by the columns under test. +const LOCKOUT_THRESHOLD = 7; +const LOCKOUT_DURATION_MINUTES = 40; +/** better-auth's per-two-factor-cookie cap. Hardcoded upstream, not configurable. */ +const MAX_PER_CHALLENGE = 5; + // ── RFC 6238 TOTP ────────────────────────────────────────────────────────── // Hand-rolled rather than imported: `@better-auth/utils/otp` is a transitive // 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', () => { stack = await bootStack(showcaseStack, {}); ql = await stack.kernel.getServiceAsync('objectql'); + // [#3690] Apply the operator policy the same way the settings service does + // — `applyConfigPatch` nulls the cached better-auth instance, so the next + // request rebuilds with the new `accountLockout`. Going through this seam + // (rather than constructing the manager by hand) is what proves a Setup-UI + // change actually reaches the second factor. + const auth = await stack.kernel.getServiceAsync('auth'); + auth.applyConfigPatch({ + lockoutThreshold: LOCKOUT_THRESHOLD, + lockoutDurationMinutes: LOCKOUT_DURATION_MINUTES, + }); + const token = await stack.signIn(); const me = await (await stack.apiAs(token, 'GET', '/auth/get-session')).json() as any; adminEmail = me?.user?.email; @@ -214,16 +251,18 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => { expect(after.lockedUntil ?? null).toBeNull(); }); - it('locks the account once the budget is spent, and refuses even a correct code', async () => { + it('locks the account once the operator-configured 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; + // (hardcoded), and MAX_FAILURES consecutive failures per ACCOUNT + // (`accountLockout.maxFailedAttempts`). Only the second one touches the + // columns under test, so reaching it takes a fresh challenge every + // MAX_PER_CHALLENGE tries. + // + // MAX_FAILURES is the operator's threshold, not better-auth's default — + // #3690. Were the config dropped, the account would still be unlocked at + // failure 7 and the assertion below would fail on a null `locked_until`. + const MAX_FAILURES = LOCKOUT_THRESHOLD; expect((await lockoutState()).count, 'precondition: a clean budget').toBe(0); @@ -246,7 +285,15 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => { 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 lasts the operator's duration, not better-auth's 15 minutes. + // `lockoutDurationMinutes` (minutes) is projected onto the plugin's + // `durationSeconds` — the units differ on the two sides, so this is where a + // ×60 that went missing would show up. The window is wide enough for the + // several seconds of guessing above, and far too narrow to admit 15. + const lockedForMs = new Date(String(locked.lockedUntil)).getTime() - Date.now(); + expect(lockedForMs).toBeGreaterThan((LOCKOUT_DURATION_MINUTES - 5) * 60_000); + expect(lockedForMs).toBeLessThanOrEqual(LOCKOUT_DURATION_MINUTES * 60_000); // The lock has to bite BEFORE the code is checked — otherwise it is not a // lockout, just a slower guess loop. @@ -262,9 +309,9 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => { 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. + // Expire the lock rather than waiting out `durationSeconds`. 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( diff --git a/packages/services/service-settings/src/manifests/auth.manifest.test.ts b/packages/services/service-settings/src/manifests/auth.manifest.test.ts index f1f2be9e55..b08e5bb603 100644 --- a/packages/services/service-settings/src/manifests/auth.manifest.test.ts +++ b/packages/services/service-settings/src/manifests/auth.manifest.test.ts @@ -70,6 +70,24 @@ describe('authSettingsManifest', () => { expect(byKey('mfa_grace_period_days').default).toBe(7); }); + // #3690 — the same pair now drives the second factor's lockout, not just the + // password stage's. Two-factor verification exists without email/password + // sign-in, so gating the threshold on `email_password_enabled` would leave a + // passwordless deployment unable to tune it at all. + it('keeps the lockout pair reachable in passwordless deployments (#3690)', () => { + const specs = authSettingsManifest.specifiers as any[]; + const byKey = (k: string) => specs.find((s) => s.key === k); + + expect(byKey('lockout_threshold').visible).toBeUndefined(); + // The duration is still meaningless with no threshold, so that condition + // stays — but it must no longer depend on the password provider. + expect(byKey('lockout_duration_minutes').visible).toBe('${data.lockout_threshold > 0}'); + + // The help text is the only place an admin learns the threshold covers + // both stages; the wiring is invisible otherwise. + expect(byKey('lockout_threshold').description).toMatch(/two-factor/i); + }); + it('exposes encrypted Google OAuth credential fields', () => { const keys = (authSettingsManifest.specifiers as any[]) .map((s) => s.key) diff --git a/packages/services/service-settings/src/manifests/auth.manifest.ts b/packages/services/service-settings/src/manifests/auth.manifest.ts index 0662d85f68..ccef8b9bb9 100644 --- a/packages/services/service-settings/src/manifests/auth.manifest.ts +++ b/packages/services/service-settings/src/manifests/auth.manifest.ts @@ -137,6 +137,10 @@ const manifest = { description: 'Brute-force protection: per-identity account lockout and per-IP rate limiting on auth endpoints.', }, + // [#3690] The threshold/duration pair governs BOTH sign-in stages — the + // password check and the second factor. Always visible: two-factor + // verification exists in passwordless deployments too, so gating these on + // `email_password_enabled` would leave the 2FA lockout untunable there. { type: 'number', key: 'lockout_threshold', @@ -146,8 +150,7 @@ const manifest = { min: 0, max: 20, description: - 'Lock an account after this many consecutive failed sign-ins. 0 disables lockout. While locked, sign-in is rejected even with the correct password.', - visible: "${data.email_password_enabled !== false}", + '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.', }, { type: 'number', @@ -157,8 +160,8 @@ const manifest = { default: 15, min: 1, max: 1440, - description: 'How long an account stays locked once the threshold is crossed.', - visible: "${data.email_password_enabled !== false && data.lockout_threshold > 0}", + description: 'How long an account stays locked once the threshold is crossed, at either sign-in stage.', + visible: '${data.lockout_threshold > 0}', }, { type: 'number', diff --git a/packages/services/service-settings/src/translations/es-ES.ts b/packages/services/service-settings/src/translations/es-ES.ts index 2868bf4b14..e6f9a6dd38 100644 --- a/packages/services/service-settings/src/translations/es-ES.ts +++ b/packages/services/service-settings/src/translations/es-ES.ts @@ -130,11 +130,11 @@ export const esES: TranslationData = { }, lockout_threshold: { label: 'Umbral de bloqueo de cuenta', - help: 'Bloquea una cuenta tras este número de inicios de sesión fallidos consecutivos. 0 desactiva el bloqueo. Mientras está bloqueada, el inicio de sesión se rechaza incluso con la contraseña correcta.', + help: 'Bloquea una cuenta tras este número de intentos de inicio de sesión fallidos consecutivos, tanto contraseñas incorrectas como códigos de doble factor incorrectos. Mientras está bloqueada, el inicio de sesión se rechaza incluso con las credenciales correctas. 0 desactiva el bloqueo en la etapa de contraseña; la verificación en dos pasos conserva entonces su límite integrado (10 intentos cada 15 minutos), por ser la última comprobación antes de emitir una sesión.', }, lockout_duration_minutes: { label: 'Duración del bloqueo (minutos)', - help: 'Cuánto permanece bloqueada una cuenta una vez superado el umbral.', + help: 'Cuánto permanece bloqueada una cuenta una vez superado el umbral, en cualquiera de las dos etapas de inicio de sesión.', }, rate_limit_max: { label: 'Límite de tasa de autenticación: máx. solicitudes', diff --git a/packages/services/service-settings/src/translations/ja-JP.ts b/packages/services/service-settings/src/translations/ja-JP.ts index 03b70b8b6a..d6b3adb5fe 100644 --- a/packages/services/service-settings/src/translations/ja-JP.ts +++ b/packages/services/service-settings/src/translations/ja-JP.ts @@ -130,11 +130,11 @@ export const jaJP: TranslationData = { }, lockout_threshold: { label: 'アカウントロックのしきい値', - help: '連続してこの回数サインインに失敗するとアカウントをロックします。0 でロック無効。ロック中は正しいパスワードでもサインインを拒否します。', + help: '連続してこの回数サインインに失敗するとアカウントをロックします(パスワードの誤りと二要素コードの誤りの両方を数えます)。ロック中は正しい資格情報でもサインインを拒否します。0 でパスワード段階のロックを無効化しますが、二要素認証はセッション発行前の最後の確認であるため、組み込みの上限(15 分間に 10 回)を保ちます。', }, lockout_duration_minutes: { label: 'ロック時間(分)', - help: 'しきい値を超えた後、アカウントがロックされ続ける時間。', + help: 'しきい値を超えた後、アカウントがロックされ続ける時間。どちらのサインイン段階でも同じです。', }, rate_limit_max: { label: '認証レート制限:最大リクエスト数', diff --git a/packages/services/service-settings/src/translations/zh-CN.ts b/packages/services/service-settings/src/translations/zh-CN.ts index b171ce7e6f..29da823633 100644 --- a/packages/services/service-settings/src/translations/zh-CN.ts +++ b/packages/services/service-settings/src/translations/zh-CN.ts @@ -292,11 +292,11 @@ export const zhCN: TranslationData = { }, lockout_threshold: { label: '账户锁定阈值', - help: '连续登录失败达到此次数后锁定账户。0 表示关闭锁定。锁定期间即使密码正确也会拒绝登录。', + help: '连续登录失败达到此次数后锁定账户 —— 密码错误和两步验证码错误都计入。锁定期间即使凭据正确也会拒绝登录。0 表示关闭密码阶段的锁定;此时两步验证仍保留其内置限制(15 分钟内 10 次),因为它是签发会话前的最后一道校验。', }, lockout_duration_minutes: { label: '锁定时长(分钟)', - help: '越过阈值后账户保持锁定的时长。', + help: '越过阈值后账户保持锁定的时长,两个登录阶段通用。', }, rate_limit_max: { label: '认证限流:最大请求数', From 28ed7cfff4ea84ac8bea8bb7519adfa790b1eddf Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 15:20:11 +0000 Subject: [PATCH 2/2] fix(auth): make Unlock Account release the second factor too (#3690) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unlockUser` reset only `sys_user.failed_login_count` / `locked_until`. Sign-in can lock at either stage and the two counters are independent, so a user locked at the second factor had no admin escape hatch at all — the only way out was waiting the duration out. That was survivable while the second-factor lock needed better-auth's 10 failures. Now that the threshold is operator-configurable and 3 is a reasonable choice, it is a lock admins will hit routinely, so the action has to cover it. The second-factor clear is best-effort and runs after the primary write: an account with no enrolment, or an environment where 2FA was never switched on, must still get the unlock the admin asked for. Covered by two unit tests (both enrolments cleared; a failing lookup does not sink the unlock) and an end-to-end one that re-locks the account, unlocks it through `/auth/admin/unlock-user`, and signs in again. The end-to-end test takes its admin session BEFORE re-locking — after the lock there is no way to obtain one, which is the whole reason the escape hatch has to exist. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UYLC8TfjzHGwatNxZKdX7H --- .../two-factor-lockout-follows-settings.md | 7 +++ .../docs/permissions/administrator-guide.mdx | 3 +- content/docs/permissions/authentication.mdx | 5 +- .../plugin-auth/src/auth-manager.test.ts | 40 ++++++++++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 27 +++++++++++ .../test/two-factor-lockout.dogfood.test.ts | 48 +++++++++++++++++++ 6 files changed, 127 insertions(+), 3 deletions(-) diff --git a/.changeset/two-factor-lockout-follows-settings.md b/.changeset/two-factor-lockout-follows-settings.md index cf1605d60d..7d5dad0a47 100644 --- a/.changeset/two-factor-lockout-follows-settings.md +++ b/.changeset/two-factor-lockout-follows-settings.md @@ -30,6 +30,13 @@ The threshold field is also no longer hidden behind `email_password_enabled` — two-factor verification exists in passwordless deployments, where the setting was previously unreachable. +The admin **Unlock Account** action now clears both stages. It only ever reset +`sys_user`, so a user locked at the second factor had no admin escape hatch and +had to wait the duration out — survivable while that lock needed 10 failures, +routine once an operator can set the threshold to 3. The second-factor clear is +best-effort and runs after the primary write, so an account with no enrolment +still unlocks normally. + Note the plugin caps attempts at 5 per challenge (`beginAttempt(5)`), which no option reaches; a threshold above 5 forces a fresh challenge rather than raising that cap. diff --git a/content/docs/permissions/administrator-guide.mdx b/content/docs/permissions/administrator-guide.mdx index 08bdf88d1f..7d38d5536f 100644 --- a/content/docs/permissions/administrator-guide.mdx +++ b/content/docs/permissions/administrator-guide.mdx @@ -83,7 +83,8 @@ only on reorgs. Why the tree matters: Day-to-day lifecycle actions live on each user's row menu and record header: **Ban / Unban** (blocks sign-in), **Unlock Account** (clears a brute-force -lockout early), **Set Password** (also mints one-time temporary passwords), +lockout early, at both the password and two-factor stages), **Set Password** +(also mints one-time temporary passwords), and **Impersonate User** (see [Step 4](#step-4--verify)). *(Rough edge today: users' own self-service password recovery depends on a configured email or SMS delivery service — wiring tracked in cloud#580. Until then, admin **Set diff --git a/content/docs/permissions/authentication.mdx b/content/docs/permissions/authentication.mdx index 1f48b15517..68a48ef78a 100644 --- a/content/docs/permissions/authentication.mdx +++ b/content/docs/permissions/authentication.mdx @@ -456,13 +456,14 @@ code that reads it). Changes take effect immediately; no restart is required. | Setting | Effect | |---|---| | `lockout_threshold` | Lock an account after this many consecutive failed sign-in attempts. Counts **both** stages: wrong passwords and wrong two-factor codes. | -| `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. | +| `lockout_duration_minutes` | How long a lockout lasts, at either stage. Admins can clear it early with the **Unlock** action on the user record, which releases both stages. | | `rate_limit_max` / `rate_limit_window_seconds` | Per-IP request throttle on auth endpoints. | The two stages keep **separate counters** — `sys_user.failed_login_count` for the password check, `sys_two_factor.failed_verification_count` for the second factor — so a locked password stage and a locked second factor are independent states. Each -counter resets on a success at its own stage. +counter resets on a success at its own stage, and the admin **Unlock Account** +action clears both at once. Setting `lockout_threshold` to `0` disables the **password-stage** lockout only. Two-factor verification keeps a built-in limit of 10 attempts per 15 minutes, diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index 98744cb555..441c81b68f 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -2472,6 +2472,46 @@ describe('AuthManager', () => { ); }); + // #3690 — sign-in can lock at either stage, and the counters are + // independent. Unlocking only `sys_user` left a 2FA-locked user with no + // admin escape hatch, which the now-configurable threshold makes routine. + it('unlockUser also clears the second factor\'s lock', async () => { + const engine = { + ...makeEngine({ id: 'u1' }), + find: vi.fn(async () => [{ id: 'tf1' }, { id: 'tf2' }]), + }; + const m = mgr(engine); + await expect(m.unlockUser('u1')).resolves.toBe(true); + expect(engine.find).toHaveBeenCalledWith( + 'sys_two_factor', + expect.objectContaining({ where: { user_id: 'u1' } }), + ); + for (const id of ['tf1', 'tf2']) { + expect(engine.update).toHaveBeenCalledWith( + 'sys_two_factor', + { id, failed_verification_count: 0, locked_until: null }, + expect.anything(), + ); + } + }); + + it('unlockUser still succeeds when the second-factor clear fails', async () => { + // No enrolment, a store that cannot serve the lookup, 2FA never switched + // on — none of that may turn the password-stage unlock the admin asked + // for into a failure. + const engine = { + ...makeEngine({ id: 'u1' }), + find: vi.fn(async () => { throw new Error('no such object: sys_two_factor'); }), + }; + const m = mgr(engine); + await expect(m.unlockUser('u1')).resolves.toBe(true); + expect(engine.update).toHaveBeenCalledWith( + 'sys_user', + { id: 'u1', failed_login_count: 0, locked_until: null }, + expect.anything(), + ); + }); + it('unlockUser returns false for an unknown user', async () => { const engine = makeEngine(null); const m = mgr(engine); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 3c065bcd05..b671056aa9 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -3961,6 +3961,14 @@ export class AuthManager { * ADR-0069 D2 — clear a user's lockout state (admin "Unlock" action). * Resets `failed_login_count` and `locked_until`. Returns false when no data * engine is wired or the user does not exist. + * + * [#3690] Clears BOTH stages. Sign-in can lock at the password check + * (`sys_user`) or at the second factor (`sys_two_factor`), and the two keep + * independent counters — so unlocking only the first left a 2FA-locked user + * with no admin escape hatch at all, waiting out the duration. That was + * survivable while the second factor sat on better-auth's 10-attempt default; + * with the threshold now operator-configurable (3 is a reasonable choice), + * it is a lock admins will hit routinely. */ public async unlockUser(userId: string): Promise { const engine = this.getDataEngine(); @@ -3975,6 +3983,25 @@ export class AuthManager { { id: userId, failed_login_count: 0, locked_until: null }, { context: SYSTEM_CTX } as any, ); + // Best-effort and deliberately after the primary write: a user with no + // enrolment (or an environment where 2FA was never switched on) must still + // get a successful unlock, and the password-stage clear is the part the + // admin asked for. + try { + const enrolments = await engine.find('sys_two_factor', { + where: { user_id: String(userId) }, fields: ['id'], context: SYSTEM_CTX, + } as any); + for (const row of (enrolments ?? []) as Array<{ id?: string }>) { + if (!row?.id) continue; + await engine.update( + 'sys_two_factor', + { id: row.id, failed_verification_count: 0, locked_until: null }, + { context: SYSTEM_CTX } as any, + ); + } + } catch { + // Never turn a successful password-stage unlock into a failure. + } return true; } 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 36e25ff4d7..9fef24b0ce 100644 --- a/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts +++ b/packages/qa/dogfood/test/two-factor-lockout.dogfood.test.ts @@ -116,6 +116,7 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => { let priorTwoFactor: string | undefined; let secret: Buffer; let adminEmail: string; + let adminUserId: string; beforeAll(async () => { // The two-factor plugin is opt-in (`twoFactor: … ?? false`), resolved once @@ -140,7 +141,9 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => { const token = await stack.signIn(); const me = await (await stack.apiAs(token, 'GET', '/auth/get-session')).json() as any; adminEmail = me?.user?.email; + adminUserId = String(me?.user?.id ?? ''); expect(adminEmail, 'could not resolve the seeded admin email').toBeTruthy(); + expect(adminUserId, 'could not resolve the seeded admin id').toBeTruthy(); // Enrol. `enable` returns the otpauth:// URI carrying the base32 secret; // the stored copy is symmetrically encrypted, so this is the only place @@ -334,4 +337,49 @@ describe('#3624 follow-up: better-auth 2FA lockout counts wrong codes', () => { 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); }); + + // [#3690] The admin escape hatch. `Unlock Account` used to clear only + // `sys_user`, so a user locked at the SECOND factor had no way out but to + // wait out the duration — tolerable while that lock needed 10 failures, much + // less so now that an operator can set the threshold to 3. + it('the admin unlock action releases a second-factor lock', async () => { + // A session that survives the lock. The one minted in beforeAll does not: + // completing enrolment rotates the session, so that bearer is stale by now. + // Take a fresh one through the full two-factor sign-in BEFORE re-locking — + // afterwards there is no way to obtain one, which is exactly why the admin + // escape hatch has to exist. + const bootstrap = await verifyWithCookie(await beginChallenge(), totp(secret)); + expect(bootstrap.status, `sign-in for the admin session: ${await bootstrap.clone().text()}`).toBe(200); + const adminSession = ((await bootstrap.json()) as { token?: string }).token; + expect(adminSession, 'two-factor sign-in returned no session token').toBeTruthy(); + + // Re-lock: the previous test cleared everything. + let failures = 0; + while (failures < LOCKOUT_THRESHOLD) { + const cookie = await beginChallenge(); + for (let i = 0; i < MAX_PER_CHALLENGE && failures < LOCKOUT_THRESHOLD; i++) { + await verifyWithCookie(cookie, '000000'); + failures++; + } + } + const locked = await lockoutState(); + expect(locked.lockedUntil ?? null, 'precondition: the account is locked again').not.toBeNull(); + expect(locked.count).toBe(LOCKOUT_THRESHOLD); + + // The lock gates VERIFICATION, not live sessions — so an already-signed-in + // admin can still act, which is what makes the escape hatch reachable. + const unlocked = await stack.apiAs(adminSession as string, 'POST', '/auth/admin/unlock-user', { + userId: adminUserId, + }); + expect(unlocked.status, `unlock-user: ${await unlocked.clone().text()}`).toBe(200); + + const cleared = await lockoutState(); + expect(cleared.lockedUntil ?? null, 'unlock must clear the second factor, not just the password stage').toBeNull(); + expect(cleared.count, 'unlock must reset the failure budget too — otherwise the next wrong code re-locks immediately').toBe(0); + + // And the user can actually sign in again, which is the point of unlocking. + const cookie = await beginChallenge(); + const ok = await verifyWithCookie(cookie, totp(secret)); + expect(ok.status, `still locked out after unlock: ${await ok.clone().text()}`).toBe(200); + }); });