fix(auth): make the second factor obey the operator's lockout policy (#3690) - #3706
Merged
Merged
Conversation
…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
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 16 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYLC8TfjzHGwatNxZKdX7H
os-zhuang
marked this pull request as ready for review
July 27, 2026 15:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3690.
问题
auth-manager.ts构造twoFactor()时只传了 schema:没传
accountLockout,所以 better-auth 的内置默认值(开启 / 10 次 / 15 分钟)原样生效,与管理员的配置无关。一个把 Setup → 认证 → 账户锁定阈值 收紧到 3 的管理员,得到的是:密码阶段 3 次锁定,第二因子仍然 10 次。更严的那道门反而更松,而且 UI 上没有任何提示。
方案:以 better-auth 的契约为准
复用现有的
lockout_threshold/lockout_duration_minutes,投影到 better-auth 自己的字段形状:maxFailedAttemptslockout_thresholddurationSecondslockout_duration_minutesenabled没有新增一对
two_factor_lockout_*设置。理由:一个策略、一个心智模型;而且贴着 upstream 的字段形状走,将来 better-auth 再加字段(比如按因子区分)是多一个 option,而不是和自造的并行策略打架。投影走
applyConfigPatch—— 它会清掉缓存的 better-auth 实例,下一个请求用新策略重建,所以改设置不需要重启。阈值 0 不映射成
enabled: false0是密码阶段的"关闭"。一个部署可能有意让密码阶段不锁(前面有限流、验证码或 IdP),但第二因子是签发会话前的最后一道校验 —— 不应该被一个从未提及它的设置顺手关掉。所以阈值为 0 / 未设置时不传accountLockout,better-auth 的默认值原样接管。两个阶段都仍然可显式配置,只有"未配置"这一档不同,而且是往锁的方向不同。顺带:阈值不再藏在
email_password_enabled后面两步验证在无密码部署里同样存在,原来的
visible条件会让那种部署完全够不到这个设置。lockout_duration_minutes保留lockout_threshold > 0的条件(没有阈值时时长确实无意义),只是不再依赖密码提供方。一个到不了的上限
插件在每个 challenge 内硬编码 5 次上限(
beginAttempt(5)),没有任何 option 能改。阈值设到 5 以上并不会抬高它 —— 只是攻击者每 5 次要重开一次登录流程,账户级预算才是跨 challenge 累加的那个。已在 manifest 帮助文本、文档和代码注释里写明。第二个提交:Unlock 也得覆盖第二因子
做完上面这一条之后发现的直接后果。
unlockUser只重置sys_user—— 两个阶段的计数器是独立的,所以锁在第二因子上的用户,管理员根本没有出口,只能等时长走完。这在第二因子需要 10 次失败时还能忍;现在阈值可配、3 是个合理选择,这就是管理员会经常撞上的锁。所以 Unlock Account 现在同时清两个阶段。
第二因子那部分是 best-effort 且排在主写之后:没有 2FA 注册记录、或者环境里根本没开过 2FA 的账户,仍然要正常解锁。
测试
单元(
auth-manager.test.ts,+6)unlockUser:两条注册记录都被清;查询失败时不拖垮解锁。端到端(
two-factor-lockout.dogfood.test.ts,4 → 5) —— 这里是关键修正。原来的断言硬编码了 better-auth 的默认值(10 次 / 15 分钟),所以它分辨不出"配置成 10"和"配置被忽略" —— 正是 #3690 从它眼皮底下溜过去的原因。现在整个文件跑在一个显式策略下(7 次 / 40 分钟),两个值都刻意区别于所有默认值,断言全部由它推导:
7高于每 challenge 的硬编码 5,所以耗尽预算仍然必须跨 challenge —— 被测列背后的账户级计数器还是被真正走到;7 ≠ 10,配置一旦失效就锁不上;40 ≠ 15,×60 一旦丢失,时长断言立刻落空。新增的第 5 个用例走完整条出口:重新锁上 → 调
/auth/admin/unlock-user→ 两个计数都归零 → 再登录成功。它在重新上锁之前取管理员会话 —— 锁上之后就再也拿不到了,而这恰恰是这条出口必须存在的原因。验证过它会失败:临时删掉
accountLockout那一行重建后跑,locked_until始终为 null,两个预算相关的测试如期失败;恢复后 5/5 通过。影响面
plugin-auth545/545,service-settings176/176,dogfood 该文件 5/5。content/docs/permissions/authentication.mdx与administrator-guide.mdx一并更新,说明两个阶段共用阈值、各自独立计数、0 的含义,以及 Unlock 覆盖两者。Refs #3647, #3667, ADR-0069 D2。