feat(auth): two-factor authentication (TOTP + backup codes) - #1059
Open
mmcintosh wants to merge 3 commits into
Open
feat(auth): two-factor authentication (TOTP + backup codes)#1059mmcintosh wants to merge 3 commits into
mmcintosh wants to merge 3 commits into
Conversation
Adds a two-factor-auth core plugin: TOTP enrolment with a scannable QR, single-use
backup codes, a per-account lockout, second-factor enforcement on the passwordless
sign-in paths, and an administrative reset for users who lose their authenticator.
Better Auth's twoFactor() is composed unconditionally, so enrolment state alone
decides whether a user is challenged. The plugin owns the enrolment surface
(/admin/two-factor); core owns the login challenge (/auth/two-factor) so that
turning plugins off stops new enrolments without locking out users who already
have a second factor.
What is enforced where:
- Challenge on password sign-in — Better Auth, via twoFactorRedirect.
- Challenge on magic-link / email-OTP — guardPasswordlessSecondFactor in the
/auth/* catch-all, since BA challenges only the password paths and would
otherwise hand an enrolled account a session with no code.
- Session upgrade after verification — POST /auth/two-factor/complete mints
the same JWT a password login mints, so a 2FA session is never weaker than
a password one. It derives the credential from the session's own user and
never from request input.
- Policy (issuer, lockout, backup-code count) is clamped on read, so no
stored value or failed load can disable the lockout or drop below 5 codes.
The QR is rendered server-side (qrcode-svg, already a core dep, Workers-safe)
because these pages have no client bundler and a CDN script on a page handling
TOTP secrets is not a trade worth making. Its size is COMPUTED from the symbol's
module count rather than fixed: scannability is CSS pixels per module, and the
issuer is operator-configurable to 64 chars and appears in the URI twice, so the
symbol ranges 49-69 modules. A fixed container delivered 4.14 px/module by
default and 3.06 with a long issuer — well-formed, and too small for a phone to
decode. It is now a constant 6.00 px/module at any issuer length, with the size
stamped on the <svg> rather than left to CSS.
Route input is pinned to ^otpauth://totp/ so the endpoint cannot render
arbitrary text as a QR from our origin — a phishing primitive, since a QR is
unreadable to the human deciding whether to trust it.
── Administrative reset (break-glass) ──
Without one, the complete list of ways back into an account with a lost
authenticator was an unused backup code or `wrangler d1 execute --remote`.
Everything an operator would reach for first is closed by design: password reset
deliberately mints no session, magic link and email OTP are refused for enrolled
users, and self-disable needs the session you cannot get. So a sole admin who
enrolled and lost phone plus codes locked the entire organisation out of the
portal until someone with Cloudflare credentials intervened.
POST /admin/two-factor-reset clears a user's second factor, and migration 0004
adds auth_user.two_factor_required so the reset can demand re-enrolment instead
of silently downgrading the account to password-only. The flag lives on auth_user
rather than auth_two_factor because the reset DELETEs that row — a flag stored
there would be destroyed by the action that sets it. required and enabled are
independent: required && !enrolled is what redirects to /admin/two-factor;
required && enrolled means enrolled and not permitted to turn it off.
Controls on the action, and why these:
- Admin role, matching how every other user-management route in admin-users.ts
is gated, so it is governed by role assignments operators already reason about.
- The target's email must be typed back. Not a second factor — it defends
against the realistic failure, which is resetting the wrong row from a list
of similar-looking ones. Password confirmation was considered and rejected:
Better Auth hashes with its own scrypt (salt:key, 161 chars) that
AuthManager.verifyPassword cannot read, and reimplementing it wrong would
break the break-glass itself.
- Every use writes a two_factor_reset security event naming both the actor and
the subject. Best-effort: an unavailable audit sink must not be the reason a
lockout recovery fails.
- A user under the requirement may not turn the factor back off. The redirect
middleware cannot cover that — POST /auth/two-factor/disable is not under
/admin/*, and /admin/two-factor (which hosts the disable form) has to stay
exempt or enrolment would be impossible — so a second guard sits in the
/auth/* catch-all beside guardPasswordlessSecondFactor, ahead of
auth.handler, and the page replaces the form with an explanation. Without it
a user told to enrol could enrol and immediately switch it off, leaving the
account password-only, BA no longer challenging, the passwordless paths
reopened and /api/* ungated, with no audit event to tell the admin.
Deliberately keyed to the REQUIREMENT and not to being enrolled, so ordinary
self-service 2FA management is untouched.
Mounted on its own prefix and NOT behind the plugin's deactivate→404 gate, for
the same reason the login challenge is not: deactivating the plugin stops nothing
about verification, so a recovery path that vanished with the surface would
disappear exactly when the lockout it fixes is still happening. The enforcement
middleware fails OPEN on any DB error and stands aside entirely when the plugin
is off, since /admin/two-factor 404s then and enforcing would loop the user to a
page that cannot exist.
The reset does not revoke the target's sessions — it is a recovery action, not a
containment one; deactivating the account is what containment is for.
Also fixes the plugin sidebar rendering icon NAMES as text ("lock-closed" beside
Two-Factor Auth, "book-open" for API Reference, "variable" for Global
Variables): plugin-menu projected `resolveIcon(icon) || icon`, leaking unmapped
names into markup the layout interpolates.
Verified in a browser: enrolment page and QR render, the displayed QR was proved
in-page to encode the same URI as the manual-entry secret, and the enrolment flow
was confirmed end-to-end with a physical phone scan.
Verified against a running wrangler dev server on real D1 and real Better Auth,
not only under the test harness: 0004 applied to an existing install without
locking out its enrolled user; the reset drove end-to-end (panel render, typed-
email refusal leaving the DB untouched, reset, target then signing in with
password alone, forced re-enrolment redirect for browsers and 403 for JSON, the
break-glass route correctly NOT exempt, audit event naming actor and subject);
the disable guard refused a mandated account while leaving ordinary self-service
disable working; and the 0004 self-heal restored the column after it was dropped
out from under a running app.
Known and deliberately out of scope: CSRF validation is inert across the whole
cookie-authenticated admin surface, because csrfProtection exempts requests with
no auth_token cookie and sign-in mints a Better Auth session cookie instead.
Pre-existing and repo-wide; the fix is the "OD2 Option B" csrf.ts port, which
changes global request handling and needs its own audit. Comments that claimed
CSRF was enforced on these routes have been corrected.
Tests: real Better Auth over real SQLite for the round trip and the lockout, and
41 new cases for the reset — SQL effects, the role and typed-email gates, the
enforcement middleware's exemptions, JSON branch and fail-open paths, the disable
guard, and the four states the enrolment page renders.
Notably the middleware must NOT exempt /admin/two-factor-reset, which a naive
prefix check would hand to the very users who owe an enrolment. E2E in
tests/e2e/101-two-factor-auth.spec.ts and 102-two-factor-admin-reset.spec.ts
(enrolling cases use throwaway accounts so they cannot take out the shared admin).
two-factor-lockout-engages.test.ts gets an explicit 30s timeout: it makes ~10
sequential BA round trips through real scrypt and landed at ~5.1s against the 5s
default, so any added concurrency turned it into a timeout that looked exactly
like a broken lockout.
npm test: 25 failed / 1886 passed — the same 25 pre-existing beta.25 failures as
on the parent commit, verified by diffing failing-test names against a clean
baseline run. Zero regressions.
…01/102→106/107 main has since taken 0003 (session_org) and 0004 (forms) for unrelated features — this branch's 0003_two_factor_lockout.sql/0004_two_factor_required.sql collided. Renumbered to 0006/0007, deliberately skipping 0005 (reserved for the FTS5 search PR, also in flight and further along in review). Synced both migrations/ copies, updated the self-heal comments and test-harness filename references in migrations.ts/d1-sqlite.ts/two-factor-adapter-create.test.ts, regenerated the bundle. E2E specs renumbered 101/102→106/107 to stay clear of both currently-open PRs (#1057 claims 104, #1058 claims 105) — real main's actual highest spec is 100, not 103 as an earlier broad search suggested.
The renumbering commit updated services/migrations.ts but missed 5 more files with the same stale references, found by review — cosmetic only, the actual filenames/logic were already correctly 0006/0007 everywhere.
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
1 participant
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.
Description
Adds a
two-factor-authcore plugin: TOTP enrolment with a server-rendered scannable QR code, single-use backup codes, per-account lockout after repeated failed verification attempts, second-factor enforcement layered onto the passwordless (magic-link/email-OTP) sign-in paths, session upgrade after verification, and an administrative reset flow for a user who loses both their authenticator and their backup codes — with forced re-enrolment afterward so "reset" doesn't silently become "permanently disabled."Changes
twoFactor()is composed unconditionally; enrolment state alone decides whether a user is challenged. The plugin owns the enrolment surface (/admin/two-factor); core owns the login challenge (/auth/two-factor), so disabling the plugin stops new enrolments without locking out users who already have a second factor.twoFactorRedirect; magic-link/email-OTP needed a separate guard (guardPasswordlessSecondFactorin the/auth/*catch-all) since BA only challenges the password path and would otherwise hand an enrolled account a session with no code; two more SonicJS-specific session-minting paths (OAuth account-linking, OTP login) get their ownhasVerifiedSecondFactorgates for the same reason.POST /auth/two-factor/complete) mints the same JWT a password login mints, derived only from the session's own user — never from request input — so a 2FA session is never weaker than a password one.qrcode-svg, already a core dependency, Workers-safe) — no client bundler on these pages, and a CDN script on a page handling TOTP secrets isn't a trade worth making. Module size is computed from the symbol's actual module count rather than fixed, so it stays scannable regardless of issuer-string length.POST /admin/two-factor-reset,adminrole + the target's email typed back) for the lockout scenario this feature would otherwise create with no recovery path. Addsauth_user.two_factor_required(migration0007) to force re-enrolment; the enforcement middleware sits outside the plugin's deactivate→404 gate and stands aside entirely when the plugin is off, so it can't strand anyone in a redirect loop to a 404 page.0006/0007(renumbered from the original0003/0004— those are now taken by unrelated features already onmain;0005deliberately skipped, reserved for another PR further along in review).Testing
Phone-tested against a real device weeks before this PR; re-verified end-to-end on a running
wrangler devwith real D1 + real Better Auth (not mocked) — full reset flow, forced re-enrolment for both browser and JSON callers, the disable guard both ways, all four enrolment-page states, and the self-heal restoring lockout columns after a liveDROP COLUMN.Unit Tests
tsc --noEmitclean.E2E Tests
106/107(renumbered to avoid colliding with two other currently-open PRs). Deliberately never enrols the shared admin test account —fullyParallel: trueCI would otherwise take every one of ~124 other specs'loginAsAdmininto a 2FA challenge; each enrolling test uses a throwaway account instead, andloginAsAdminitself now throws a loud, actionable error rather than a silent timeout if the shared admin is ever unexpectedly challenged.Screenshots/Videos
N/A — new admin/auth flow with no reference screenshots on hand; happy to add if useful for review.
Checklist