From 75abe011fbec3878c29f682d104ed8ada17e3018 Mon Sep 17 00:00:00 2001 From: Mark McIntosh Date: Thu, 30 Jul 2026 11:11:54 -0400 Subject: [PATCH 1/3] feat(auth): two-factor authentication (TOTP + backup codes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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. --- .github/workflows/pr-tests.yml | 2 +- CLAUDE.md | 2 +- .../migrations/0006_two_factor_lockout.sql | 38 ++ .../migrations/0007_two_factor_required.sql | 23 + .../migrations/0006_two_factor_lockout.sql | 38 ++ .../migrations/0007_two_factor_required.sql | 23 + .../middleware/plugin-menu-icons.test.ts | 111 +++++ .../plugins/mount-integration.test.ts | 12 + .../routes/auth-two-factor-redirect.test.ts | 180 ++++++++ .../two-factor-complete.integration.test.ts | 158 +++++++ .../passwordless-second-factor-guard.test.ts | 310 +++++++++++++ .../services/second-factor-guard.test.ts | 134 ++++++ .../two-factor-adapter-create.test.ts | 98 ++++ .../services/two-factor-composition.test.ts | 196 ++++++++ .../two-factor-lockout-columns.test.ts | 124 +++++ .../two-factor-lockout-engages.test.ts | 122 +++++ .../services/two-factor-roundtrip.test.ts | 266 +++++++++++ .../services/two-factor-settings.test.ts | 196 ++++++++ .../core/src/__tests__/utils/d1-sqlite.ts | 32 +- packages/core/src/__tests__/utils/totp.ts | 88 ++++ packages/core/src/app.ts | 60 ++- packages/core/src/auth/config.ts | 72 ++- .../auth/passwordless-second-factor-guard.ts | 222 +++++++++ packages/core/src/auth/second-factor-guard.ts | 110 +++++ packages/core/src/auth/two-factor-settings.ts | 152 ++++++ packages/core/src/db/schema.ts | 58 ++- packages/core/src/middleware/plugin-menu.ts | 25 +- .../core/src/plugins/core-plugins/index.ts | 2 + .../core-plugins/oauth-providers/index.ts | 18 + .../core-plugins/otp-login-plugin/index.ts | 15 + .../security-audit-plugin/types.ts | 5 + .../__tests__/admin-gate.test.ts | 254 ++++++++++ .../__tests__/enrolment-page-required.test.ts | 61 +++ .../two-factor-auth/__tests__/qr.test.ts | 183 ++++++++ .../__tests__/recovery.sqlite.test.ts | 435 ++++++++++++++++++ .../__tests__/registration.test.ts | 74 +++ .../components/challenge-page.ts | 136 ++++++ .../components/enrolment-page.ts | 312 +++++++++++++ .../core-plugins/two-factor-auth/index.ts | 216 +++++++++ .../two-factor-auth/manifest.json | 34 ++ .../core-plugins/two-factor-auth/recovery.ts | 433 +++++++++++++++++ .../core-plugins/two-factor-auth/routes.ts | 255 ++++++++++ .../core/src/plugins/manifest-registry.ts | 31 +- packages/core/src/routes/admin-users.ts | 13 +- packages/core/src/routes/auth.ts | 49 ++ packages/core/src/services/migrations.test.ts | 12 +- packages/core/src/services/migrations.ts | 76 +++ packages/core/src/services/plugin-service.ts | 10 + .../templates/pages/admin-profile.template.ts | 17 +- .../pages/admin-user-edit.template.ts | 145 ++++++ tests/e2e/101-two-factor-auth.spec.ts | 318 +++++++++++++ tests/e2e/102-two-factor-admin-reset.spec.ts | 259 +++++++++++ tests/e2e/utils/test-helpers.ts | 20 +- 53 files changed, 6205 insertions(+), 30 deletions(-) create mode 100644 my-sonicjs-app/migrations/0006_two_factor_lockout.sql create mode 100644 my-sonicjs-app/migrations/0007_two_factor_required.sql create mode 100644 packages/core/migrations/0006_two_factor_lockout.sql create mode 100644 packages/core/migrations/0007_two_factor_required.sql create mode 100644 packages/core/src/__tests__/middleware/plugin-menu-icons.test.ts create mode 100644 packages/core/src/__tests__/routes/auth-two-factor-redirect.test.ts create mode 100644 packages/core/src/__tests__/routes/two-factor-complete.integration.test.ts create mode 100644 packages/core/src/__tests__/services/passwordless-second-factor-guard.test.ts create mode 100644 packages/core/src/__tests__/services/second-factor-guard.test.ts create mode 100644 packages/core/src/__tests__/services/two-factor-adapter-create.test.ts create mode 100644 packages/core/src/__tests__/services/two-factor-composition.test.ts create mode 100644 packages/core/src/__tests__/services/two-factor-lockout-columns.test.ts create mode 100644 packages/core/src/__tests__/services/two-factor-lockout-engages.test.ts create mode 100644 packages/core/src/__tests__/services/two-factor-roundtrip.test.ts create mode 100644 packages/core/src/__tests__/services/two-factor-settings.test.ts create mode 100644 packages/core/src/__tests__/utils/totp.ts create mode 100644 packages/core/src/auth/passwordless-second-factor-guard.ts create mode 100644 packages/core/src/auth/second-factor-guard.ts create mode 100644 packages/core/src/auth/two-factor-settings.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/admin-gate.test.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/enrolment-page-required.test.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/qr.test.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/recovery.sqlite.test.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/registration.test.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/components/challenge-page.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/components/enrolment-page.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/index.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/manifest.json create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/recovery.ts create mode 100644 packages/core/src/plugins/core-plugins/two-factor-auth/routes.ts create mode 100644 tests/e2e/101-two-factor-auth.spec.ts create mode 100644 tests/e2e/102-two-factor-admin-reset.spec.ts diff --git a/.github/workflows/pr-tests.yml b/.github/workflows/pr-tests.yml index d0cd42967..2f13c5c1c 100644 --- a/.github/workflows/pr-tests.yml +++ b/.github/workflows/pr-tests.yml @@ -221,7 +221,7 @@ jobs: if echo "$CHANGED_SRC" | grep -qE 'routes/api|api-content|api-documents|api-media'; then TAGS="$TAGS|@api" fi - if echo "$CHANGED_SRC" | grep -qE 'middleware/auth|admin-settings|api-keys|services/api-key'; then + if echo "$CHANGED_SRC" | grep -qE 'middleware/auth|admin-settings|api-keys|services/api-key|src/auth/|routes/auth|two-factor'; then TAGS="$TAGS|@auth|@api-keys" fi if echo "$CHANGED_SRC" | grep -qE 'admin-database|database-tools'; then diff --git a/CLAUDE.md b/CLAUDE.md index fb1e6847e..c3a149e77 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -189,7 +189,7 @@ test.describe('Media Upload @media', () => { … }) | `src/routes/admin-content*` or `src/services/documents*` | `@smoke @content` | | `src/routes/admin-media*` or `src/services/media*` | `@smoke @media` | | `src/routes/api*` | `@smoke @api` | -| `src/middleware/auth*` or `src/routes/admin-settings*` | `@smoke @auth` | +| `src/middleware/auth*`, `src/auth/*`, `src/routes/auth*`, `src/routes/admin-settings*`, anything `two-factor` | `@smoke @auth @api-keys` | | `src/services/api-keys*` or related | `@smoke @api-keys` | | `src/routes/admin-database*` | `@smoke @database` | | `packages/core/migrations/*` | `@smoke @content @media @api` | diff --git a/my-sonicjs-app/migrations/0006_two_factor_lockout.sql b/my-sonicjs-app/migrations/0006_two_factor_lockout.sql new file mode 100644 index 000000000..0e03d15db --- /dev/null +++ b/my-sonicjs-app/migrations/0006_two_factor_lockout.sql @@ -0,0 +1,38 @@ +-- Migration 0006: Two-factor second-factor lockout columns +-- +-- `auth_two_factor` already ships in 0001_core.sql, but only with the four columns Better +-- Auth's twoFactor plugin needs to STORE an enrolment (secret / backup_codes / user_id / +-- verified). It is missing the two columns the plugin writes on every VERIFY once +-- `accountLockout` is enabled, so composing the plugin against the 0001 shape fails at the +-- first `/two-factor/enable` (BA fills schema defaults on create, so the INSERT already +-- names failed_verification_count). +-- +-- Deliberately an ALTER in its own migration rather than an edit to 0001: D1 tracks applied +-- migrations by FILENAME in `d1_migrations`, so an edit to 0001 would only reach greenfield +-- installs and silently skip every DB that already ran it. As an ALTER, greenfield and +-- already-migrated installs converge on the same shape. +-- +-- There is also a runtime self-heal for these two columns in +-- `MigrationService.ensureSchemaCompatibility()` (PRAGMA table_xinfo + ALTER, the same D45 +-- pattern used for the documents `q_*` columns). Belt and braces: a deployment that never +-- ran this migration would otherwise 500 on enrolment instead of repairing itself. + +-- Consecutive failed second-factor verifications. +-- +-- NOT NULL DEFAULT 0 is load-bearing, not cosmetic. BA compiles the lockout bump to +-- `failed_verification_count = failed_verification_count + 1` (incrementOne), and +-- `NULL + 1` is NULL, which verify-two-factor.mjs then reads back through `?? 0` as zero — +-- forever. A nullable column here means the lockout silently never trips. +ALTER TABLE auth_two_factor ADD COLUMN failed_verification_count INTEGER NOT NULL DEFAULT 0; + +-- When the per-account second-factor lockout expires. +-- +-- INTEGER (milliseconds), NOT the TEXT/ISO the sibling Infowall port uses. `lockedUntil` is +-- a Better Auth `date` field, and its handling depends on the ADAPTER: the kysely adapter +-- sets `supportsDates: false`, so BA stringifies to ISO before the write. SonicJS is on the +-- **drizzle** adapter (better-auth-cloudflare → drizzleAdapter, provider 'sqlite'), which +-- leaves `supportsDates` at its `true` default, so BA hands drizzle a real `Date` and the +-- column mode does the conversion. Declared here to match +-- `authTwoFactor.lockedUntil = integer('locked_until', { mode: 'timestamp_ms' })` in +-- db/schema.ts — the same declaration auth_session.expires_at already uses. +ALTER TABLE auth_two_factor ADD COLUMN locked_until INTEGER; diff --git a/my-sonicjs-app/migrations/0007_two_factor_required.sql b/my-sonicjs-app/migrations/0007_two_factor_required.sql new file mode 100644 index 000000000..5e1a06489 --- /dev/null +++ b/my-sonicjs-app/migrations/0007_two_factor_required.sql @@ -0,0 +1,23 @@ +-- Migration 0007: force re-enrolment after an administrative two-factor reset. +-- +-- When an admin resets a locked-out user (lost phone AND lost backup codes — the only recovery +-- path this feature otherwise has is direct database access), that user is left with NO second +-- factor. Without a way to demand they set it up again, "reset" silently becomes "permanently +-- downgrade", because nothing ever prompts them and the account quietly stays password-only. +-- +-- `two_factor_required` is that demand. Set it and the user is redirected to /admin/two-factor and +-- cannot use the rest of the admin portal until they enrol. It is INDEPENDENT of +-- `two_factor_enabled`: +-- +-- required=0, enabled=0 → optional, not enrolled (the default) +-- required=0, enabled=1 → enrolled voluntarily +-- required=1, enabled=0 → MUST enrol before using the portal ← what a reset leaves behind +-- required=1, enabled=1 → enrolled, and may not turn it off +-- +-- Kept on auth_user rather than auth_two_factor because it has to outlive the reset: the reset +-- DELETEs the auth_two_factor row, so a flag stored there would be destroyed by the very action +-- that needs to set it. +-- +-- NOT NULL DEFAULT 0 so every existing row is "not required" — enabling this feature must never +-- retroactively lock out an existing user. +ALTER TABLE auth_user ADD COLUMN two_factor_required INTEGER NOT NULL DEFAULT 0; diff --git a/packages/core/migrations/0006_two_factor_lockout.sql b/packages/core/migrations/0006_two_factor_lockout.sql new file mode 100644 index 000000000..0e03d15db --- /dev/null +++ b/packages/core/migrations/0006_two_factor_lockout.sql @@ -0,0 +1,38 @@ +-- Migration 0006: Two-factor second-factor lockout columns +-- +-- `auth_two_factor` already ships in 0001_core.sql, but only with the four columns Better +-- Auth's twoFactor plugin needs to STORE an enrolment (secret / backup_codes / user_id / +-- verified). It is missing the two columns the plugin writes on every VERIFY once +-- `accountLockout` is enabled, so composing the plugin against the 0001 shape fails at the +-- first `/two-factor/enable` (BA fills schema defaults on create, so the INSERT already +-- names failed_verification_count). +-- +-- Deliberately an ALTER in its own migration rather than an edit to 0001: D1 tracks applied +-- migrations by FILENAME in `d1_migrations`, so an edit to 0001 would only reach greenfield +-- installs and silently skip every DB that already ran it. As an ALTER, greenfield and +-- already-migrated installs converge on the same shape. +-- +-- There is also a runtime self-heal for these two columns in +-- `MigrationService.ensureSchemaCompatibility()` (PRAGMA table_xinfo + ALTER, the same D45 +-- pattern used for the documents `q_*` columns). Belt and braces: a deployment that never +-- ran this migration would otherwise 500 on enrolment instead of repairing itself. + +-- Consecutive failed second-factor verifications. +-- +-- NOT NULL DEFAULT 0 is load-bearing, not cosmetic. BA compiles the lockout bump to +-- `failed_verification_count = failed_verification_count + 1` (incrementOne), and +-- `NULL + 1` is NULL, which verify-two-factor.mjs then reads back through `?? 0` as zero — +-- forever. A nullable column here means the lockout silently never trips. +ALTER TABLE auth_two_factor ADD COLUMN failed_verification_count INTEGER NOT NULL DEFAULT 0; + +-- When the per-account second-factor lockout expires. +-- +-- INTEGER (milliseconds), NOT the TEXT/ISO the sibling Infowall port uses. `lockedUntil` is +-- a Better Auth `date` field, and its handling depends on the ADAPTER: the kysely adapter +-- sets `supportsDates: false`, so BA stringifies to ISO before the write. SonicJS is on the +-- **drizzle** adapter (better-auth-cloudflare → drizzleAdapter, provider 'sqlite'), which +-- leaves `supportsDates` at its `true` default, so BA hands drizzle a real `Date` and the +-- column mode does the conversion. Declared here to match +-- `authTwoFactor.lockedUntil = integer('locked_until', { mode: 'timestamp_ms' })` in +-- db/schema.ts — the same declaration auth_session.expires_at already uses. +ALTER TABLE auth_two_factor ADD COLUMN locked_until INTEGER; diff --git a/packages/core/migrations/0007_two_factor_required.sql b/packages/core/migrations/0007_two_factor_required.sql new file mode 100644 index 000000000..5e1a06489 --- /dev/null +++ b/packages/core/migrations/0007_two_factor_required.sql @@ -0,0 +1,23 @@ +-- Migration 0007: force re-enrolment after an administrative two-factor reset. +-- +-- When an admin resets a locked-out user (lost phone AND lost backup codes — the only recovery +-- path this feature otherwise has is direct database access), that user is left with NO second +-- factor. Without a way to demand they set it up again, "reset" silently becomes "permanently +-- downgrade", because nothing ever prompts them and the account quietly stays password-only. +-- +-- `two_factor_required` is that demand. Set it and the user is redirected to /admin/two-factor and +-- cannot use the rest of the admin portal until they enrol. It is INDEPENDENT of +-- `two_factor_enabled`: +-- +-- required=0, enabled=0 → optional, not enrolled (the default) +-- required=0, enabled=1 → enrolled voluntarily +-- required=1, enabled=0 → MUST enrol before using the portal ← what a reset leaves behind +-- required=1, enabled=1 → enrolled, and may not turn it off +-- +-- Kept on auth_user rather than auth_two_factor because it has to outlive the reset: the reset +-- DELETEs the auth_two_factor row, so a flag stored there would be destroyed by the very action +-- that needs to set it. +-- +-- NOT NULL DEFAULT 0 so every existing row is "not required" — enabling this feature must never +-- retroactively lock out an existing user. +ALTER TABLE auth_user ADD COLUMN two_factor_required INTEGER NOT NULL DEFAULT 0; diff --git a/packages/core/src/__tests__/middleware/plugin-menu-icons.test.ts b/packages/core/src/__tests__/middleware/plugin-menu-icons.test.ts new file mode 100644 index 000000000..a2bafd816 --- /dev/null +++ b/packages/core/src/__tests__/middleware/plugin-menu-icons.test.ts @@ -0,0 +1,111 @@ +/** + * Sidebar icon resolution for plugin menu entries. + * + * The bug this pins was visible on every page of the admin panel: the sidebar showed the literal + * text `lock-closed` where the Two-Factor Auth icon belongs, and `book-open` for API Reference. + * Both plugins declare an icon NAME in their manifest, `middleware/plugin-menu.ts` could not + * resolve either name, and the fallback was `resolveIcon(m.icon) || m.icon` — so the unresolved + * name was handed to the catalyst layout, which interpolates it as markup. + * + * Nothing caught it because both the resolved SVG and the raw name are `string`: the types agree, + * the page renders, and only a human looking at the sidebar can see the difference. So these + * assert the one property that distinguishes them — the value must be SVG markup, never a name. + */ +import { describe, it, expect } from 'vitest' +import { PLUGIN_REGISTRY } from '../../plugins/manifest-registry' + +// The module keeps ICON_SVG/resolveIcon private, so drive them the way the app does: through the +// exported middleware, with a context stub that captures what it sets on `pluginMenuItems`. +const { pluginMenuMiddleware } = await import('../../middleware/plugin-menu') + +type MenuItem = { label: string; path: string; icon: string } + +/** + * Run the middleware and return what it set for rendering. + * + * `activeSlugs` drives the MANIFEST path (plugins listed in PLUGIN_REGISTRY whose document row is + * active). That is the path that carried the bug: those entries reach the final projection with + * their raw manifest icon NAME. The singleton path is deliberately not used here — its entries are + * pre-resolved by `resolvePluginMenuItems`, so a test driving it passes either way. An earlier + * version of this file made exactly that mistake and stayed green against the broken code. + */ +async function renderMenu(activeSlugs: string[]) { + const captured: Record = {} + const c = { + env: { + DB: { + prepare: () => ({ + bind: () => ({ all: async () => ({ results: activeSlugs.map((slug) => ({ slug })) }) }), + all: async () => ({ results: [] }), + first: async () => null, + }), + }, + }, + req: { path: '/admin', url: 'http://localhost/admin' }, + res: new Response('', { headers: { 'content-type': 'text/html' } }), + get: (k: string) => captured[k], + set: (k: string, v: unknown) => { + captured[k] = v + }, + } + + await pluginMenuMiddleware()(c as never, async () => {}) + return (captured['pluginMenuItems'] as MenuItem[]) ?? [] +} + +/** SVG markup, as opposed to a bare icon name that would render as text. */ +function isSvgMarkup(icon: string) { + return icon.trim().startsWith('') +} + +/** Every plugin whose manifest puts an entry in the sidebar. */ +const MENU_PLUGINS = Object.entries(PLUGIN_REGISTRY) + .map(([, p]) => p as { id: string; adminMenu?: { icon?: string; label?: string } | null }) + .filter((p) => !!p.adminMenu) + +describe('plugin sidebar icons', () => { + it('renders SVG for every icon the shipped manifests declare', async () => { + // Driven off the registry rather than a hardcoded list, so adding a plugin whose icon name has + // no mapping fails here instead of showing the name as text in the sidebar. + expect(MENU_PLUGINS.length, 'no plugin manifests declare an adminMenu').toBeGreaterThan(0) + + const items = await renderMenu(MENU_PLUGINS.map((p) => p.id)) + expect(items.length).toBe(MENU_PLUGINS.length) + + for (const item of items) { + expect(isSvgMarkup(item.icon), `"${item.label}" rendered a non-SVG icon: ${item.icon}`).toBe(true) + } + }) + + it.each(['two-factor-auth', 'api-docs-plugin'])( + 'renders a real icon for %s — both showed their name as text', + async (slug) => { + // Only assert on plugins that are actually in the registry, so this does not become a + // tripwire for unrelated plugin removals. + if (!MENU_PLUGINS.some((p) => p.id === slug)) return + const items = await renderMenu([slug]) + expect(items).toHaveLength(1) + expect(isSvgMarkup(items[0]!.icon)).toBe(true) + }, + ) + + it('never passes an unresolved icon name through as markup', async () => { + // The regression itself. A plugin id that is in the registry but whose icon name is unknown + // must render the fallback, never the raw string. + const target = MENU_PLUGINS[0]! + const original = target.adminMenu!.icon + target.adminMenu!.icon = 'no-such-icon-name' + try { + const items = await renderMenu([target.id]) + expect(items).toHaveLength(1) + expect(items[0]!.icon).not.toContain('no-such-icon-name') + expect(isSvgMarkup(items[0]!.icon)).toBe(true) + } finally { + target.adminMenu!.icon = original + } + }) + + it('renders nothing extra when no plugin is active', async () => { + expect(await renderMenu([])).toHaveLength(0) + }) +}) diff --git a/packages/core/src/__tests__/plugins/mount-integration.test.ts b/packages/core/src/__tests__/plugins/mount-integration.test.ts index d055ff78a..aaaecc31f 100644 --- a/packages/core/src/__tests__/plugins/mount-integration.test.ts +++ b/packages/core/src/__tests__/plugins/mount-integration.test.ts @@ -89,5 +89,17 @@ describe('plugin mounting via createSonicJSApp', () => { expect(hasPathPrefix(paths, '/admin/content')).toBe(true) expect(hasPathPrefix(paths, '/api')).toBe(true) }) + + it('still serves the 2FA login challenge when disableAll is true', () => { + // Better Auth composes `twoFactor()` unconditionally (auth/config.ts), so turning plugins + // off does NOT stop enrolled users being challenged at sign-in. While the challenge page was + // mounted by the plugin, those users were redirected to `/auth/two-factor` and got a 404 — + // locked out of an app that still demanded their second factor. Core mounts it now. + const paths = routePaths(createSonicJSApp({ plugins: { disableAll: true } })) + expect(hasPathPrefix(paths, '/auth/two-factor')).toBe(true) + // The ENROLMENT surface is plugin-owned and must still be gone: disabling plugins should + // stop new enrolments without stranding existing ones. + expect(hasPathPrefix(paths, '/admin/two-factor')).toBe(false) + }) }) }) diff --git a/packages/core/src/__tests__/routes/auth-two-factor-redirect.test.ts b/packages/core/src/__tests__/routes/auth-two-factor-redirect.test.ts new file mode 100644 index 000000000..42634d0b9 --- /dev/null +++ b/packages/core/src/__tests__/routes/auth-two-factor-redirect.test.ts @@ -0,0 +1,180 @@ +/** + * The `twoFactorRedirect` branch in both login handlers. + * + * Better Auth answers a pending second factor with HTTP **200** and `{twoFactorRedirect:true}` — + * no `user`, no `token`, and it deletes the session it had just created. Both SonicJS login + * handlers read BA's outcome through `if (!baRes.ok)`, so before this branch existed a CORRECT + * password fell straight through into the success path: + * + * POST /auth/login → minted a JWT from `baBody.user ?? {}`, i.e. + * generateToken(undefined, undefined, 'viewer') — a signed token for + * a principal that does not exist, set as `auth_token` AND returned + * as `token`. app.ts's Bearer-JWT fallback then populates + * c.get('user') = {userId: undefined}, which requireAuth() accepts. + * POST /auth/login/form → reported "Login successful! Redirecting…" and sent the browser to + * /admin/content with no session, bouncing back to login. + * + * These tests drive the real handlers with `createAuth` replaced by a stub that reproduces BA's + * three outcomes. + */ +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { Hono } from 'hono' + +/** What the stubbed BA handler should answer on /auth/sign-in/email. */ +let baOutcome: 'twoFactorRedirect' | 'success' | 'failure' = 'twoFactorRedirect' + +const generateToken = vi.fn(async () => 'signed.jwt.token') + +vi.mock('../../auth/config', () => ({ + createAuth: () => ({ + handler: async () => { + if (baOutcome === 'failure') { + return new Response(JSON.stringify({ message: 'Invalid email or password' }), { + status: 401, + headers: { 'Content-Type': 'application/json' }, + }) + } + if (baOutcome === 'twoFactorRedirect') { + // The real shape: 200, a challenge cookie, and no user/token. + return new Response(JSON.stringify({ twoFactorRedirect: true, twoFactorMethods: ['totp'] }), { + status: 200, + headers: { + 'Content-Type': 'application/json', + 'Set-Cookie': 'better-auth.two_factor=chal; Path=/; HttpOnly', + }, + }) + } + return new Response( + JSON.stringify({ user: { id: 'u1', email: 'a@test.local', role: 'admin', name: 'A B' }, token: 'ba-token' }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ) + }, + }), +})) + +vi.mock('../../middleware', async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + requireAuth: () => async (_c: unknown, next: () => Promise) => next(), + AuthManager: { ...(actual.AuthManager as object), generateToken }, + } +}) + +// The security-audit / logging / lockout helpers all reach for D1; the stub DB below answers +// enough for the handler to run, and none of it is what these tests assert. +function stubDb() { + const stmt = { + bind: () => stmt, + first: async () => null, + all: async () => ({ results: [] }), + run: async () => ({ success: true }), + } + return { prepare: () => stmt, batch: async () => [] } +} + +async function post(path: string, body: Record, htmx = false) { + const { default: authRoutes } = await import('../../routes/auth') + const app = new Hono() + app.route('/auth', authRoutes as never) + + const isForm = path.endsWith('/form') + const headers: Record = isForm + ? {} + : { 'Content-Type': 'application/json' } + if (htmx) headers['HX-Request'] = 'true' + + let payload: BodyInit + if (isForm) { + const fd = new FormData() + for (const [k, v] of Object.entries(body)) fd.append(k, v) + payload = fd + } else { + payload = JSON.stringify(body) + } + + return app.request( + path, + { method: 'POST', headers, body: payload }, + { DB: stubDb(), JWT_SECRET: 'test-secret-value-32-chars-long!!', CACHE_KV: undefined }, + ) +} + +const CREDS = { email: 'a@test.local', password: 'correct-horse-battery' } + +beforeEach(() => { + vi.clearAllMocks() + baOutcome = 'twoFactorRedirect' +}) + +describe('POST /auth/login — second factor pending', () => { + it('answers 200 with twoFactorRequired, not 401', async () => { + const res = await post('/auth/login', CREDS) + expect(res.status).toBe(200) + expect(await res.json()).toMatchObject({ + twoFactorRequired: true, + twoFactorMethods: ['totp'], + redirectTo: '/auth/two-factor', + }) + }) + + it('mints NO token — the whole point of the branch', async () => { + const res = await post('/auth/login', CREDS) + const body = (await res.json()) as Record + expect(body.token).toBeUndefined() + expect(body.user).toBeUndefined() + expect(generateToken).not.toHaveBeenCalled() + }) + + it('sets no auth_token cookie', async () => { + const res = await post('/auth/login', CREDS) + const cookies = (res.headers as unknown as { getSetCookie?: () => string[] }).getSetCookie?.() ?? [] + expect(cookies.some((c) => c.startsWith('auth_token='))).toBe(false) + }) + + it('forwards BA\'s challenge cookie, which verify-totp needs', async () => { + const res = await post('/auth/login', CREDS) + const cookies = (res.headers as unknown as { getSetCookie?: () => string[] }).getSetCookie?.() ?? [] + expect(cookies.some((c) => c.includes('better-auth.two_factor='))).toBe(true) + }) + + it('still mints a token on a plain successful sign-in', async () => { + baOutcome = 'success' + const res = await post('/auth/login', CREDS) + expect(res.status).toBe(200) + expect(await res.json()).toMatchObject({ token: 'signed.jwt.token' }) + expect(generateToken).toHaveBeenCalledWith('u1', 'a@test.local', 'admin', expect.anything(), expect.anything()) + }) + + it('still 401s a genuinely bad password', async () => { + baOutcome = 'failure' + const res = await post('/auth/login', CREDS) + expect(res.status).toBe(401) + expect(generateToken).not.toHaveBeenCalled() + }) +}) + +describe('POST /auth/login/form — second factor pending', () => { + it('sends the browser to the challenge page, not to /admin/content', async () => { + const html = await (await post('/auth/login/form', CREDS)).text() + expect(html).toContain("window.location.href = '/auth/two-factor'") + expect(html).not.toContain('/admin/content') + }) + + it('does not claim the login succeeded', async () => { + const html = await (await post('/auth/login/form', CREDS)).text() + expect(html).not.toMatch(/Login successful/i) + expect(html).toMatch(/two-step verification/i) + }) + + it('uses HX-Redirect for an HTMX submit', async () => { + const res = await post('/auth/login/form', CREDS, true) + expect(res.headers.get('HX-Redirect')).toBe('/auth/two-factor') + }) + + it('still redirects to /admin/content on a plain successful sign-in', async () => { + baOutcome = 'success' + const res = await post('/auth/login/form', CREDS, true) + expect(res.headers.get('HX-Redirect')).toBe('/admin/content') + }) +}) diff --git a/packages/core/src/__tests__/routes/two-factor-complete.integration.test.ts b/packages/core/src/__tests__/routes/two-factor-complete.integration.test.ts new file mode 100644 index 000000000..9c64f50e6 --- /dev/null +++ b/packages/core/src/__tests__/routes/two-factor-complete.integration.test.ts @@ -0,0 +1,158 @@ +/** + * `POST /auth/two-factor/complete` — the session upgrade that mints the same `auth_token` JWT a + * password login mints, so a post-challenge session is not weaker than a password one. + * + * Driven against a REAL better-sqlite3 D1 with the REAL `hasVerifiedSecondFactor`, + * `AuthManager.generateToken` and `getJwtExpirySecondsFromDb`. + * + * Why this file exists: this route is the newest code in the two-factor change and it MINTS A + * CREDENTIAL, but every other test in the feature stops at Better Auth's own endpoints. The + * claims worth pinning down are that it refuses an anonymous caller, refuses a caller with no + * verified second factor, and derives the token from the SESSION rather than from request input. + * + * ── What this file does NOT prove ── + * The harness supplies `c.get('user')` directly, and `requireAuth()` only checks that key for + * presence — so `c.get('user')` IS half the security decision, and the half that lives in + * app.ts's Better Auth session middleware is replaced here. The property that a caller holding + * only the `better-auth.two_factor` CHALLENGE cookie (password proven, code not yet entered) + * cannot reach this route is therefore asserted nowhere in the suite: it rests on that middleware + * resolving no user from a pending challenge. If anything ever teaches it to read the challenge + * cookie — say, to render the user's email on the challenge page — this route would hand out a + * full JWT for a factor that was never verified, and every test here would stay green. The E2E + * spec's `expectSessionUpgraded` is the only place the composed path is exercised. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { Hono } from 'hono' +import { twoFactorChallengeRoutes } from '../../plugins/core-plugins/two-factor-auth/routes' +import { AuthManager } from '../../middleware/auth' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' + +const JWT_SECRET = 'test-jwt-secret-value-32-chars-long!!' +const USER_ID = 'user-enrolled-1' +const EMAIL = 'enrolled@test.local' + +let db: TestD1 + +/** Build an app that mounts the challenge routes with an optional signed-in principal. */ +function makeApp(user?: { userId: string; email: string; role?: string }) { + const app = new Hono() + app.use('*', async (c, next) => { + c.env = { DB: db, JWT_SECRET, ENVIRONMENT: 'production' } as never + if (user) c.set('user' as never, user as never) + await next() + }) + app.route('/auth/two-factor', twoFactorChallengeRoutes) + return app +} + +function post(app: Hono, body: unknown = {}) { + return app.request('/auth/two-factor/complete', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }) +} + +/** Insert an `auth_two_factor` row directly — BA owns the real writes. */ +function seedEnrolment(userId: string, verified: 0 | 1) { + db.raw + .prepare( + `INSERT INTO auth_two_factor (id, secret, backup_codes, user_id, verified, created_at, updated_at) + VALUES (?, 'enc-secret', '[]', ?, ?, ?, ?)`, + ) + .run(`tf-${userId}-${verified}`, userId, verified, Date.now(), Date.now()) +} + +beforeEach(() => { + db = createTestD1() + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(console, 'warn').mockImplementation(() => {}) +}) + +afterEach(() => { + db.close() + vi.restoreAllMocks() +}) + +describe('POST /auth/two-factor/complete', () => { + it('refuses an anonymous caller with 401 and sets no cookie', async () => { + const res = await post(makeApp()) + expect(res.status).toBe(401) + expect(res.headers.get('set-cookie')).toBeNull() + }) + + it('refuses an authenticated caller who holds no second factor, and mints nothing', async () => { + // A password-only session must not be able to convert itself through this route: it is a + // session upgrade for the population that just passed a challenge, not a token vending machine. + const res = await post(makeApp({ userId: 'no-2fa', email: 'plain@test.local', role: 'admin' })) + expect(res.status).toBe(400) + expect(res.headers.get('set-cookie')).toBeNull() + }) + + it('refuses a STARTED-but-unconfirmed enrolment (verified = 0)', async () => { + // The window between /two-factor/enable and the first successful verify-totp. No challenge + // has been passed, so there is nothing to upgrade. + seedEnrolment(USER_ID, 0) + const res = await post(makeApp({ userId: USER_ID, email: EMAIL, role: 'admin' })) + expect(res.status).toBe(400) + expect(res.headers.get('set-cookie')).toBeNull() + }) + + it('mints auth_token for a verified enrolment, with the same cookie attributes as a password login', async () => { + seedEnrolment(USER_ID, 1) + const res = await post(makeApp({ userId: USER_ID, email: EMAIL, role: 'admin' })) + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ ok: true }) + + const cookie = res.headers.get('set-cookie') ?? '' + expect(cookie).toContain('auth_token=') + expect(cookie).toContain('HttpOnly') + expect(cookie).toContain('SameSite=Strict') + expect(cookie).toContain('Path=/') + // ENVIRONMENT is 'production' in this harness, so the cookie must be Secure. + expect(cookie).toContain('Secure') + + // Max-Age must be the resolved JWT TTL. A 0 or NaN here would sail past every other + // assertion in this file while making the cookie a session cookie (or dropping it outright). + const maxAge = Number(/Max-Age=(\d+)/.exec(cookie)?.[1]) + expect(Number.isFinite(maxAge)).toBe(true) + expect(maxAge).toBeGreaterThan(0) + + const token = /auth_token=([^;]+)/.exec(cookie)?.[1] ?? '' + const payload = await AuthManager.verifyToken(decodeURIComponent(token), JWT_SECRET) + expect(payload).toBeTruthy() + expect(payload!.userId).toBe(USER_ID) + expect(payload!.email).toBe(EMAIL) + expect(payload!.role).toBe('admin') + // The JWT's own expiry must agree with the cookie's, or the cookie outlives the credential. + expect(payload!.exp - payload!.iat).toBe(maxAge) + }) + + it('derives the token from the SESSION, never from request input', async () => { + // The route's core safety claim. If it ever read identity off the body, this is the request + // that would turn a viewer's post-challenge session into an admin credential. + seedEnrolment(USER_ID, 1) + const app = makeApp({ userId: USER_ID, email: EMAIL, role: 'viewer' }) + const res = await post(app, { + userId: 'attacker', + email: 'attacker@evil.test', + role: 'admin', + isSuperAdmin: true, + }) + expect(res.status).toBe(200) + + const token = /auth_token=([^;]+)/.exec(res.headers.get('set-cookie') ?? '')?.[1] ?? '' + const payload = await AuthManager.verifyToken(decodeURIComponent(token), JWT_SECRET) + expect(payload!.userId).toBe(USER_ID) + expect(payload!.email).toBe(EMAIL) + expect(payload!.role).toBe('viewer') + }) + + it('does not let one user upgrade on another user\'s enrolment', async () => { + // hasVerifiedSecondFactor is keyed by user_id; a row for someone else must not satisfy it. + seedEnrolment('somebody-else', 1) + const res = await post(makeApp({ userId: USER_ID, email: EMAIL, role: 'admin' })) + expect(res.status).toBe(400) + expect(res.headers.get('set-cookie')).toBeNull() + }) +}) diff --git a/packages/core/src/__tests__/services/passwordless-second-factor-guard.test.ts b/packages/core/src/__tests__/services/passwordless-second-factor-guard.test.ts new file mode 100644 index 000000000..d2832db8c --- /dev/null +++ b/packages/core/src/__tests__/services/passwordless-second-factor-guard.test.ts @@ -0,0 +1,310 @@ +/** + * `guardPasswordlessSecondFactor` — the block that stops a magic link or an emailed code from + * standing in for password + TOTP. + * + * These tests exist mostly to pin the properties that are easy to lose: + * - INITIATE endpoints must answer BA's own success shape (no 2FA-enrolment oracle) and must + * answer the RIGHT one per endpoint + * - the COMPLETE endpoint must answer 403 with a message + * - the request body must survive the guard, because BA parses it afterwards + * - unrelated /auth paths must pass straight through + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { Hono } from 'hono' +import type { Context } from 'hono' +import type { D1Database } from '@cloudflare/workers-types' +import { + guardPasswordlessSecondFactor, + GUARDED_PASSWORDLESS_PATHS, +} from '../../auth/passwordless-second-factor-guard' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' + +let db: TestD1 + +function seedUser(id: string, email: string) { + db.raw + .prepare( + `INSERT INTO auth_user (id, email, first_name, last_name, created_at, updated_at) + VALUES (?, ?, 'A', 'B', 0, 0)`, + ) + .run(id, email) +} + +function seedEnrolment(userId: string, verified: 0 | 1) { + db.raw + .prepare( + `INSERT INTO auth_two_factor (id, secret, backup_codes, user_id, verified, created_at, updated_at) + VALUES (?, 'enc', 'enc', ?, ?, 0, 0)`, + ) + .run(`tf-${userId}`, userId, verified) +} + +/** + * Drive the guard through a real Hono app so the body-clone behaviour is exercised for real + * rather than mocked. `downstream` records what Better Auth would have seen. + */ +async function run( + path: string, + body: unknown, + method = 'POST', +): Promise<{ status: number; json: unknown; reachedBetterAuth: boolean; downstreamBody: unknown }> { + let reachedBetterAuth = false + let downstreamBody: unknown = undefined + + const app = new Hono<{ Bindings: { DB: unknown } }>() + app.on(['GET', 'POST'], '/auth/*', async (c) => { + const refused = await guardPasswordlessSecondFactor( + c as unknown as Context<{ Bindings: { DB: D1Database } }>, + ) + if (refused) return refused + reachedBetterAuth = true + // Stand-in for auth.handler(c.req.raw): proves the stream the guard cloned is still intact. + downstreamBody = await c.req.raw.json().catch(() => 'UNPARSEABLE') + return c.json({ downstream: true }) + }) + + const res = await app.request( + path, + { + method, + headers: { 'Content-Type': 'application/json' }, + body: body === undefined ? undefined : JSON.stringify(body), + }, + { DB: db }, + ) + return { + status: res.status, + json: await res.json().catch(() => null), + reachedBetterAuth, + downstreamBody, + } +} + +beforeEach(() => { + db = createTestD1() + vi.spyOn(console, 'warn').mockImplementation(() => {}) +}) + +afterEach(() => { + db.close() + vi.restoreAllMocks() +}) + +describe('guardPasswordlessSecondFactor — enrolled accounts', () => { + beforeEach(() => { + seedUser('u1', 'enrolled@test.local') + seedEnrolment('u1', 1) + }) + + it('refuses a magic-link request with BA\'s own {status:true}', async () => { + const r = await run('/auth/sign-in/magic-link', { email: 'enrolled@test.local' }) + expect(r.status).toBe(200) + expect(r.json).toEqual({ status: true }) + expect(r.reachedBetterAuth).toBe(false) + }) + + it('refuses a sign-in OTP request with BA\'s own {success:true}', async () => { + // Deliberately a DIFFERENT shape from magic-link: matching each endpoint's own success body + // is what makes the block indistinguishable from a real send. + const r = await run('/auth/email-otp/send-verification-otp', { + email: 'enrolled@test.local', + type: 'sign-in', + }) + expect(r.status).toBe(200) + expect(r.json).toEqual({ success: true }) + expect(r.reachedBetterAuth).toBe(false) + }) + + it('refuses OTP sign-in completion with Better Auth\'s own invalid-code error', async () => { + // Deliberately indistinguishable from a wrong code. A distinctive response here would be a + // free oracle: this guard runs BEFORE BA validates the OTP, so an unauthenticated caller + // could post any 6 digits and learn from the reply whether the address has an account with + // a second factor. Mirrors APIError.from('BAD_REQUEST', EMAIL_OTP_ERROR_CODES.INVALID_OTP). + const r = await run('/auth/sign-in/email-otp', { email: 'enrolled@test.local', otp: '000000' }) + expect(r.status).toBe(400) + expect(r.json).toEqual({ message: 'Invalid OTP', code: 'INVALID_OTP' }) + expect(r.reachedBetterAuth).toBe(false) + }) + + it('refuses a form-encoded initiation too, so the block does not rest on BA\'s media-type default', async () => { + // BA currently pins these endpoints to application/json, so a form body 415s before the + // handler. If that ever widens, the guard must still apply rather than silently opening. + const app = new Hono<{ Bindings: { DB: unknown } }>() + let reached = false + app.post('/auth/*', async (c) => { + const refused = await guardPasswordlessSecondFactor( + c as unknown as Context<{ Bindings: { DB: D1Database } }>, + ) + if (refused) return refused + reached = true + return c.json({ downstream: true }) + }) + const res = await app.request( + '/auth/sign-in/magic-link', + { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: 'email=enrolled%40test.local', + }, + { DB: db }, + ) + expect(reached).toBe(false) + expect(await res.json()).toEqual({ status: true }) + }) + + it('fails CLOSED when the user lookup throws', async () => { + // BA does its own lookup afterwards and can succeed where ours failed, so passing through on + // error would mail a link to an enrolled account. + vi.spyOn(console, 'error').mockImplementation(() => {}) + const brokenDb = { + prepare: () => ({ bind: () => ({ all: async () => { throw new Error('D1_ERROR: offline') } }) }), + } + const app = new Hono<{ Bindings: { DB: unknown } }>() + let reached = false + app.post('/auth/*', async (c) => { + const refused = await guardPasswordlessSecondFactor( + c as unknown as Context<{ Bindings: { DB: D1Database } }>, + ) + if (refused) return refused + reached = true + return c.json({ downstream: true }) + }) + const res = await app.request( + '/auth/sign-in/magic-link', + { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ email: 'enrolled@test.local' }), + }, + { DB: brokenDb }, + ) + expect(reached).toBe(false) + expect(await res.json()).toEqual({ status: true }) + }) + + it('checks every case-insensitive match, not just the first row', async () => { + // auth_user.email is UNIQUE under BINARY collation, so a capitalised twin can coexist. An + // unordered LIMIT 1 could return the non-enrolled twin and let the enrolled account through. + // + // SEED ORDER IS THE WHOLE TEST. `email = ? COLLATE NOCASE` cannot use the BINARY index, so + // SQLite full-scans in rowid order and `LIMIT 1` yields whichever row was inserted first. + // The earlier version of this test seeded the non-enrolled twin SECOND, behind the enrolled + // `u1` from the enclosing beforeEach — so reverting resolveUsersByEmail to `LIMIT 1` still + // returned the enrolled row and the test stayed green while the bypass was wide open. + // Re-seed from scratch with the NON-enrolled twin first so the assertion can actually fail. + db.raw.prepare(`DELETE FROM auth_two_factor`).run() + db.raw.prepare(`DELETE FROM auth_user`).run() + seedUser('u-twin-plain', 'ENROLLED@test.local') // first row scanned; NOT enrolled + seedUser('u-twin-enrolled', 'enrolled@test.local') + seedEnrolment('u-twin-enrolled', 1) + + const r = await run('/auth/sign-in/magic-link', { email: 'enrolled@test.local' }) + expect(r.reachedBetterAuth).toBe(false) + }) + + it('matches the address case-insensitively — a miss here would be a bypass', async () => { + // BA's magic-link endpoint passes ctx.body.email through verbatim, so an address that + // arrives capitalised must still resolve to the enrolled account. + const r = await run('/auth/sign-in/magic-link', { email: 'Enrolled@Test.Local' }) + expect(r.reachedBetterAuth).toBe(false) + expect(r.json).toEqual({ status: true }) + }) + + it('lets password reset through — it mints no session', async () => { + const r = await run('/auth/email-otp/send-verification-otp', { + email: 'enrolled@test.local', + type: 'forget-password', + }) + expect(r.reachedBetterAuth).toBe(true) + }) + + it('lets email verification through — it mints no session in this configuration', async () => { + const r = await run('/auth/email-otp/send-verification-otp', { + email: 'enrolled@test.local', + type: 'email-verification', + }) + expect(r.reachedBetterAuth).toBe(true) + }) +}) + +describe('guardPasswordlessSecondFactor — pass-through cases', () => { + it('lets a user with no enrolment through', async () => { + seedUser('u1', 'plain@test.local') + const r = await run('/auth/sign-in/magic-link', { email: 'plain@test.local' }) + expect(r.reachedBetterAuth).toBe(true) + }) + + it('lets a user whose enrolment is unconfirmed through', async () => { + seedUser('u1', 'pending@test.local') + seedEnrolment('u1', 0) + const r = await run('/auth/sign-in/magic-link', { email: 'pending@test.local' }) + expect(r.reachedBetterAuth).toBe(true) + }) + + it('lets an unknown address through, so the guard adds no enumeration signal of its own', async () => { + const r = await run('/auth/sign-in/magic-link', { email: 'nobody@test.local' }) + expect(r.reachedBetterAuth).toBe(true) + }) + + it('does not touch password sign-in — BA challenges there itself', async () => { + seedUser('u1', 'enrolled@test.local') + seedEnrolment('u1', 1) + const r = await run('/auth/sign-in/email', { email: 'enrolled@test.local', password: 'x' }) + expect(r.reachedBetterAuth).toBe(true) + }) + + it('does not touch OAuth callbacks — MFA there is the provider\'s contract', async () => { + seedUser('u1', 'enrolled@test.local') + seedEnrolment('u1', 1) + const r = await run('/auth/callback/github', undefined, 'GET') + expect(r.reachedBetterAuth).toBe(true) + }) + + it('passes an unparseable body through for BA to reject', async () => { + const app = new Hono<{ Bindings: { DB: unknown } }>() + let reached = false + app.post('/auth/*', async (c) => { + const refused = await guardPasswordlessSecondFactor( + c as unknown as Context<{ Bindings: { DB: D1Database } }>, + ) + if (refused) return refused + reached = true + return c.json({ ok: true }) + }) + const res = await app.request( + '/auth/sign-in/magic-link', + { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: 'not-json' }, + { DB: db }, + ) + expect(res.status).toBe(200) + expect(reached).toBe(true) + }) + + it('ignores a GET to a guarded path', async () => { + seedUser('u1', 'enrolled@test.local') + seedEnrolment('u1', 1) + const r = await run('/auth/sign-in/magic-link', undefined, 'GET') + expect(r.reachedBetterAuth).toBe(true) + }) +}) + +describe('guardPasswordlessSecondFactor — body preservation', () => { + it('leaves the request stream intact for Better Auth', async () => { + // The guard reads c.req.raw.clone(). Using c.req.json() would consume the stream and BA's + // own parse would then fail on every passwordless request — a total outage, not a subtle bug. + seedUser('u1', 'plain@test.local') + const r = await run('/auth/sign-in/magic-link', { email: 'plain@test.local', extra: 42 }) + expect(r.reachedBetterAuth).toBe(true) + expect(r.downstreamBody).toEqual({ email: 'plain@test.local', extra: 42 }) + }) +}) + +describe('guarded path inventory', () => { + it('is exactly the set of BA endpoints that mint a session without a 2FA challenge', async () => { + expect(GUARDED_PASSWORDLESS_PATHS.initiate).toEqual([ + '/auth/sign-in/magic-link', + '/auth/email-otp/send-verification-otp', + ]) + expect(GUARDED_PASSWORDLESS_PATHS.complete).toEqual(['/auth/sign-in/email-otp']) + }) +}) diff --git a/packages/core/src/__tests__/services/second-factor-guard.test.ts b/packages/core/src/__tests__/services/second-factor-guard.test.ts new file mode 100644 index 000000000..740bb6de1 --- /dev/null +++ b/packages/core/src/__tests__/services/second-factor-guard.test.ts @@ -0,0 +1,134 @@ +/** + * `hasVerifiedSecondFactor` / `getEnrolmentState` against a real (SQLite) D1. + * + * The three properties worth pinning are the ones a refactor would silently invert: + * 1. it keys off `auth_two_factor.verified`, NOT `auth_user.two_factor_enabled` + * 2. a query ERROR blocks (fail closed) — but a MISSING TABLE does not + * 3. `getEnrolmentState` fails the other way (open), because it feeds a page, not a gate + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import type { D1Database } from '@cloudflare/workers-types' +import { hasVerifiedSecondFactor, getEnrolmentState } from '../../auth/second-factor-guard' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' + +let db: TestD1 + +function seedUser(id: string, email: string, twoFactorEnabled: 0 | 1 = 0) { + db.raw + .prepare( + `INSERT INTO auth_user (id, email, first_name, last_name, created_at, updated_at, two_factor_enabled) + VALUES (?, ?, 'A', 'B', 0, 0, ?)`, + ) + .run(id, email, twoFactorEnabled) +} + +function seedEnrolment(userId: string, verified: 0 | 1) { + db.raw + .prepare( + `INSERT INTO auth_two_factor (id, secret, backup_codes, user_id, verified, created_at, updated_at) + VALUES (?, 'enc', 'enc', ?, ?, 0, 0)`, + ) + .run(`tf-${userId}`, userId, verified) +} + +/** A DB whose every query rejects — stands in for a D1 outage, not a schema problem. */ +function brokenDb(message: string): D1Database { + return { + prepare: () => ({ + bind: () => ({ + first: async () => { + throw new Error(message) + }, + }), + }), + } as unknown as D1Database +} + +beforeEach(() => { + db = createTestD1() +}) + +afterEach(() => { + db.close() + vi.restoreAllMocks() +}) + +describe('hasVerifiedSecondFactor', () => { + it('is false for a user with no enrolment row', async () => { + seedUser('u1', 'a@test.local') + expect(await hasVerifiedSecondFactor(db as unknown as D1Database, 'u1')).toBe(false) + }) + + it('is false while an enrolment is started but unconfirmed (verified = 0)', async () => { + // Blocking here would lock the user out MID-ENROLMENT: they would hold a secret they had + // not yet proven, and no way back in. + seedUser('u1', 'a@test.local') + seedEnrolment('u1', 0) + expect(await hasVerifiedSecondFactor(db as unknown as D1Database, 'u1')).toBe(false) + }) + + it('is true once the enrolment is confirmed (verified = 1)', async () => { + seedUser('u1', 'a@test.local') + seedEnrolment('u1', 1) + expect(await hasVerifiedSecondFactor(db as unknown as D1Database, 'u1')).toBe(true) + }) + + it('ignores auth_user.two_factor_enabled — the flag BA sets before proof', async () => { + seedUser('u1', 'a@test.local', 1) + expect(await hasVerifiedSecondFactor(db as unknown as D1Database, 'u1')).toBe(false) + }) + + it('does not match another user\'s enrolment', async () => { + seedUser('u1', 'a@test.local') + seedUser('u2', 'b@test.local') + seedEnrolment('u2', 1) + expect(await hasVerifiedSecondFactor(db as unknown as D1Database, 'u1')).toBe(false) + }) + + it('is false for an empty user id (nothing to protect)', async () => { + expect(await hasVerifiedSecondFactor(db as unknown as D1Database, '')).toBe(false) + }) + + it('BLOCKS on a query error — fail closed', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + expect(await hasVerifiedSecondFactor(brokenDb('D1_ERROR: network'), 'u1')).toBe(true) + }) + + it('does NOT block when the table is absent — nobody can be enrolled', async () => { + // A deployment whose schema predates auth_two_factor has no enrolments to protect; + // blocking would lock every user out of every passwordless path at once. + expect(await hasVerifiedSecondFactor(brokenDb('no such table: auth_two_factor'), 'u1')).toBe(false) + }) +}) + +describe('getEnrolmentState', () => { + it('reports not-enrolled with no row', async () => { + expect(await getEnrolmentState(db as unknown as D1Database, 'u1')).toEqual({ + enrolled: false, + verified: false, + }) + }) + + it('distinguishes started-but-unconfirmed from confirmed', async () => { + seedUser('u1', 'a@test.local') + seedEnrolment('u1', 0) + expect(await getEnrolmentState(db as unknown as D1Database, 'u1')).toEqual({ + enrolled: true, + verified: false, + }) + + db.raw.prepare(`UPDATE auth_two_factor SET verified = 1 WHERE user_id = 'u1'`).run() + expect(await getEnrolmentState(db as unknown as D1Database, 'u1')).toEqual({ + enrolled: true, + verified: true, + }) + }) + + it('fails OPEN on error — it feeds a page, not a gate', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + expect(await getEnrolmentState(brokenDb('D1_ERROR: network'), 'u1')).toEqual({ + enrolled: false, + verified: false, + }) + }) +}) diff --git a/packages/core/src/__tests__/services/two-factor-adapter-create.test.ts b/packages/core/src/__tests__/services/two-factor-adapter-create.test.ts new file mode 100644 index 000000000..57d630c37 --- /dev/null +++ b/packages/core/src/__tests__/services/two-factor-adapter-create.test.ts @@ -0,0 +1,98 @@ +/** + * Enrolment must survive Better Auth's OWN insert. + * + * This test exists because the rest of the 2FA suite seeded `auth_two_factor` by hand — and + * hand-written SQL supplies `created_at`/`updated_at`, which is exactly what BA does NOT. + * `getAuthTables()` adds those two fields to the four CORE models only and spreads plugin + * tables verbatim (@better-auth/core/dist/db/get-tables.mjs), and BA's twoFactor schema + * declares just secret/backupCodes/userId/verified/failedVerificationCount/lockedUntil. So + * `POST /two-factor/enable` inserts a row with no timestamps, drizzle emits explicit NULLs for + * the absent NOT NULL columns, and SQLite rejects it — enrolment 500s for every user, and the + * page reports it as a wrong password. + * + * Anything that drives this through real drizzle against the real migrations would have caught + * it. PRAGMA column checks did not. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import Database from 'better-sqlite3' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { dirname, join } from 'node:path' +import { drizzle } from 'drizzle-orm/better-sqlite3' +import { authTwoFactor } from '../../db/schema' + +const MIGRATIONS_DIR = join(dirname(fileURLToPath(import.meta.url)), '../../../migrations') + +/** + * The exact object Better Auth's adapter factory hands the drizzle adapter for + * `create({ model: 'twoFactor' })` — every declared field, and nothing else. + * Mirrors better-auth/dist/plugins/two-factor/index.mjs `enableTwoFactor`. + */ +const BA_CREATE_PAYLOAD = { + id: 'tf-1', + secret: 'encrypted-secret', + backupCodes: 'encrypted-backup-codes', + userId: 'user-1', + verified: false, + failedVerificationCount: 0, + lockedUntil: null, +} as const + +let sqlite: Database.Database +let db: ReturnType + +beforeEach(() => { + sqlite = new Database(':memory:') + sqlite.pragma('foreign_keys = OFF') + for (const m of ['0001_core.sql', '0002_documents.sql', '0006_two_factor_lockout.sql']) { + sqlite.exec(readFileSync(join(MIGRATIONS_DIR, m), 'utf8')) + } + db = drizzle(sqlite) +}) + +afterEach(() => sqlite.close()) + +describe('auth_two_factor accepts Better Auth\'s own insert', () => { + it('inserts with only the fields BA declares — no timestamps supplied', () => { + expect(() => db.insert(authTwoFactor).values(BA_CREATE_PAYLOAD).run()).not.toThrow() + }) + + it('fills created_at / updated_at itself, since BA never sends them', () => { + const before = Date.now() + db.insert(authTwoFactor).values(BA_CREATE_PAYLOAD).run() + const row = sqlite + .prepare(`SELECT created_at AS c, updated_at AS u FROM auth_two_factor WHERE id = 'tf-1'`) + .get() as { c: number; u: number } + expect(row.c).toBeGreaterThanOrEqual(before) + expect(row.u).toBeGreaterThanOrEqual(before) + }) + + it('binds booleans through the column mode rather than handing SQLite a raw boolean', () => { + // The drizzle adapter leaves BA's supportsBooleans at its `true` default, so BA passes a JS + // boolean straight through. Without mode:'boolean' better-sqlite3 throws + // "can only bind numbers, strings, bigints, buffers, and null". + db.insert(authTwoFactor).values({ ...BA_CREATE_PAYLOAD, verified: true }).run() + expect( + (sqlite.prepare(`SELECT verified AS v FROM auth_two_factor WHERE id = 'tf-1'`).get() as { v: number }).v, + ).toBe(1) + }) + + it('round-trips lockedUntil as a Date, which is what BA writes for a `date` field', () => { + const when = new Date(Date.now() + 900_000) + db.insert(authTwoFactor).values({ ...BA_CREATE_PAYLOAD, lockedUntil: when }).run() + const stored = ( + sqlite.prepare(`SELECT locked_until AS l FROM auth_two_factor WHERE id = 'tf-1'`).get() as { l: number } + ).l + expect(stored).toBe(when.getTime()) + const [read] = db.select().from(authTwoFactor).all() + expect(read!.lockedUntil).toBeInstanceOf(Date) + expect(read!.lockedUntil!.getTime()).toBe(when.getTime()) + }) + + it('names every column in the generated INSERT, so no NOT NULL column is left to a NULL bind', () => { + const { sql } = db.insert(authTwoFactor).values(BA_CREATE_PAYLOAD).toSQL() + for (const col of ['created_at', 'updated_at', 'failed_verification_count']) { + expect(sql).toContain(`"${col}"`) + } + }) +}) diff --git a/packages/core/src/__tests__/services/two-factor-composition.test.ts b/packages/core/src/__tests__/services/two-factor-composition.test.ts new file mode 100644 index 000000000..2b4363f48 --- /dev/null +++ b/packages/core/src/__tests__/services/two-factor-composition.test.ts @@ -0,0 +1,196 @@ +/** + * How `twoFactor()` is composed into the Better Auth options. + * + * Every assertion here corresponds to a way this can silently do nothing: + * + * - not registering `auth_two_factor` in the drizzle schema map → BA cannot resolve the model + * - declaring `schema.twoFactor.fields` → double-maps a property key onto a column name + * - gating composition on plugin status → deactivation silently downgrades enrolled accounts + * - policy read lazily → BA snapshots options at construction, so the values never arrive + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { getDefaultAuthOptions } from '../../auth/config' +import { authTwoFactor, authUser } from '../../db/schema' +import { + invalidateTwoFactorPolicy, + loadTwoFactorPolicy, + TWO_FACTOR_POLICY_DEFAULTS, + TWO_FACTOR_PLUGIN_ID, +} from '../../auth/two-factor-settings' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' +import type { Bindings } from '../../app' + +let db: TestD1 + +function optionsFor() { + return getDefaultAuthOptions( + { DB: db, BETTER_AUTH_SECRET: 'x'.repeat(32) } as unknown as Bindings, + 'https://example.test', + ) +} + +/** Pull the twoFactor plugin entry out of the composed options. */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- BA plugin objects are loosely typed +function twoFactorPlugin(): any { + const opts = optionsFor() as unknown as { plugins: Array<{ id?: string }> } + return opts.plugins.find((p) => p.id === 'two-factor') +} + +beforeEach(() => { + db = createTestD1() + invalidateTwoFactorPolicy() +}) + +afterEach(() => { + db.close() + invalidateTwoFactorPolicy() +}) + +describe('twoFactor composition', () => { + it('is always composed — no plugin-status gate', () => { + // Gating on `active` would let a deactivation stop challenging every enrolled account while + // the profile page still read "Enabled". Plugin status gates the SURFACE only. + expect(twoFactorPlugin()).toBeDefined() + }) + + it('stays composed when the plugin row says INACTIVE — the ADR the whole design rests on', async () => { + // The test above only proves composition when no status document exists at all, which is the + // default state, so it would stay green under a gate that treats "unknown" as active. Seed a + // genuinely DEACTIVATED plugin row and load the policy from it (the one place plugin state + // reaches this module), then assert the second factor is still composed. + // + // If this ever fails, deactivating the plugin has silently downgraded every enrolled account + // to password-only while `/admin/profile` still reads "Enabled" — the exact failure the + // unconditional composition exists to prevent. + const now = Math.floor(Date.now() / 1000) + db.raw + .prepare( + `INSERT INTO documents (id, root_id, type_id, tenant_id, slug, locale, data, + version_number, is_current_draft, is_published, created_at, updated_at) + VALUES (?, ?, 'plugin', 'default', ?, 'en', ?, 1, 1, 0, ?, ?)`, + ) + .run( + 'doc-2fa-inactive', + 'doc-2fa-inactive', + TWO_FACTOR_PLUGIN_ID, + JSON.stringify({ status: 'inactive', is_active: false, settings: { maxFailedAttempts: 3 } }), + now, + now, + ) + await loadTwoFactorPolicy(db) + + const plugin = twoFactorPlugin() + expect(plugin, 'twoFactor() vanished for a deactivated plugin row').toBeDefined() + // …and the deactivated row's SETTINGS are still honoured, so the surface being off does not + // quietly revert the policy either. + expect(plugin.options.accountLockout.maxFailedAttempts).toBe(3) + }) + + it('points at the auth_two_factor table', () => { + expect(twoFactorPlugin().options.twoFactorTable).toBe('auth_two_factor') + }) + + it('resolves the twoFactor model through the real schema map and writes a real row', async () => { + // The strong version of "is auth_two_factor registered?". BA resolves models by modelName + // against the drizzle schema map, so this drives the COMPOSED adapter — the same call + // `POST /two-factor/enable` makes. + // + // Remove the `auth_two_factor: authTwoFactor` entry from auth/config.ts and this fails with + // `Model "twoFactor" not found in schema`. Rename either side and it fails too. + // + // This used to assert on the *error message* of a query that could not execute, because the + // test D1 shim lacked `Statement.raw()` (which drizzle's D1 driver needs for RETURNING). With + // that added, the write actually lands and we can assert on storage instead — strictly better, + // and the reason the round-trip suite exists at all. + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- BA adapter factory is untyped + const options = optionsFor() as any + const adapter = options.database(options) + + db.raw + .prepare( + `INSERT INTO auth_user (id, email, first_name, last_name, created_at, updated_at) + VALUES ('u1', 'a@test.local', 'A', 'B', 0, 0)`, + ) + .run() + + const created = await adapter.create({ + model: 'twoFactor', + data: { + secret: 'enc-secret', + backupCodes: 'enc-codes', + userId: 'u1', + verified: true, + failedVerificationCount: 0, + lockedUntil: null, + }, + }) + + // BA generated the id and got its own field names back. + expect(created).toMatchObject({ userId: 'u1', secret: 'enc-secret', verified: true }) + expect(typeof created.id).toBe('string') + + // And the row is in the table BA was told to use, with the two columns BA never sends + // populated by the drizzle defaults (see two-factor-adapter-create.test.ts). + const row = db.raw + .prepare( + `SELECT user_id, secret, backup_codes, verified, failed_verification_count, + locked_until, created_at, updated_at + FROM auth_two_factor WHERE id = ?`, + ) + .get(created.id) as Record + expect(row).toMatchObject({ + user_id: 'u1', + secret: 'enc-secret', + backup_codes: 'enc-codes', + verified: 1, + failed_verification_count: 0, + locked_until: null, + }) + expect(row.created_at).toBeGreaterThan(0) + expect(row.updated_at).toBeGreaterThan(0) + }) + + it('enables per-account lockout from the policy defaults', () => { + const lockout = twoFactorPlugin().options.accountLockout + expect(lockout).toEqual({ + enabled: true, + maxFailedAttempts: TWO_FACTOR_POLICY_DEFAULTS.maxFailedAttempts, + durationSeconds: TWO_FACTOR_POLICY_DEFAULTS.lockoutDurationSeconds, + }) + }) + + it('reads the loaded policy, not just the defaults', async () => { + db.raw + .prepare( + `INSERT INTO documents (id, root_id, type_id, slug, tenant_id, is_current_draft, data) + VALUES ('d','r','plugin', ?, 'default', 1, ?)`, + ) + .run( + TWO_FACTOR_PLUGIN_ID, + JSON.stringify({ + status: 'active', + settings: { issuer: 'Acme', maxFailedAttempts: 3, lockoutDurationSeconds: 600, backupCodeCount: 7 }, + }), + ) + await loadTwoFactorPolicy(db as unknown as never) + + const plugin = twoFactorPlugin() + expect(plugin.options.issuer).toBe('Acme') + expect(plugin.options.accountLockout).toMatchObject({ maxFailedAttempts: 3, durationSeconds: 600 }) + expect(plugin.options.backupCodeOptions).toEqual({ amount: 7 }) + }) + + it('sets trustDeviceMaxAge to 0 — leaving it UNSET would enable a 30-day bypass', () => { + // BA reads `options?.trustDeviceMaxAge ?? 2592e3`, so omitting the key opts IN to a 30-day + // trusted-device cookie that any client can request with `{trustDevice:true}` on verify. + // `0` is not nullish, so it wins the `??` and every trust record expires immediately. + expect(twoFactorPlugin().options.trustDeviceMaxAge).toBe(0) + }) + + it('does not enable skipVerificationOnEnable — an unproven secret must not count', () => { + // With it on, /two-factor/enable would flip two_factor_enabled before the user has proven a + // live code, and a mistyped secret would lock them out of their own account. + const options = twoFactorPlugin().options + expect(Object.prototype.hasOwnProperty.call(options, 'skipVerificationOnEnable')).toBe(false) + }) +}) diff --git a/packages/core/src/__tests__/services/two-factor-lockout-columns.test.ts b/packages/core/src/__tests__/services/two-factor-lockout-columns.test.ts new file mode 100644 index 000000000..bbe0cd26d --- /dev/null +++ b/packages/core/src/__tests__/services/two-factor-lockout-columns.test.ts @@ -0,0 +1,124 @@ +/** + * Migration 0003 + the runtime self-heal for the `auth_two_factor` lockout columns. + * + * Without both columns, Better Auth's `/two-factor/enable` INSERT fails (BA fills schema + * defaults on create, so the statement already names `failed_verification_count`) and enrolment + * 500s. The self-heal exists for a DB that has 0001 but never got 0003. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import Database from 'better-sqlite3' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { dirname, join } from 'node:path' +import type { D1Database } from '@cloudflare/workers-types' +import { MigrationService } from '../../services/migrations' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' + +const MIGRATIONS_DIR = join(dirname(fileURLToPath(import.meta.url)), '../../../migrations') + +function columnsOf(sqlite: Database.Database, table: string): string[] { + return (sqlite.prepare(`PRAGMA table_xinfo('${table}')`).all() as Array<{ name: string }>).map( + (r) => r.name, + ) +} + +describe('migration 0003 (greenfield)', () => { + let db: TestD1 + + beforeEach(() => { + db = createTestD1() + }) + afterEach(() => db.close()) + + it('gives auth_two_factor both lockout columns', () => { + const cols = columnsOf(db.raw, 'auth_two_factor') + expect(cols).toContain('failed_verification_count') + expect(cols).toContain('locked_until') + }) + + it('defaults failed_verification_count to 0 and NOT NULL, so BA\'s +1 bump cannot go NULL', () => { + db.raw + .prepare( + `INSERT INTO auth_two_factor (id, secret, backup_codes, user_id, created_at, updated_at) + VALUES ('t','s','b','u', 0, 0)`, + ) + .run() + const row = db.raw + .prepare(`SELECT failed_verification_count AS n, locked_until AS l FROM auth_two_factor`) + .get() as { n: number; l: number | null } + expect(row.n).toBe(0) + expect(row.l).toBeNull() + + db.raw.prepare(`UPDATE auth_two_factor SET failed_verification_count = failed_verification_count + 1`).run() + expect( + (db.raw.prepare(`SELECT failed_verification_count AS n FROM auth_two_factor`).get() as { n: number }).n, + ).toBe(1) + }) + + it('declares locked_until as INTEGER — drizzle timestamp_ms, not the kysely ISO string', () => { + // The drizzle adapter leaves BA's supportsDates at true, so BA hands over a Date and the + // drizzle column mode converts it. A TEXT column here would sort wrong on every comparison. + const info = db.raw.prepare(`PRAGMA table_xinfo('auth_two_factor')`).all() as Array<{ + name: string + type: string + }> + expect(info.find((c) => c.name === 'locked_until')?.type).toBe('INTEGER') + }) +}) + +describe('ensureSchemaCompatibility self-heal', () => { + let sqlite: Database.Database + + /** A DB that ran 0001 + 0002 only — i.e. never got migration 0003. */ + function legacyDb(): D1Database { + sqlite = new Database(':memory:') + sqlite.pragma('foreign_keys = OFF') + for (const m of ['0001_core.sql', '0002_documents.sql']) { + sqlite.exec(readFileSync(join(MIGRATIONS_DIR, m), 'utf8')) + } + const stmt = (sql: string) => ({ + bind: (...binds: unknown[]) => ({ + first: async () => sqlite.prepare(sql).get(...(binds as never[])) ?? null, + all: async () => ({ results: sqlite.prepare(sql).all(...(binds as never[])) }), + run: async () => sqlite.prepare(sql).run(...(binds as never[])), + }), + first: async () => sqlite.prepare(sql).get() ?? null, + all: async () => ({ results: sqlite.prepare(sql).all() }), + run: async () => sqlite.prepare(sql).run(), + }) + return { prepare: stmt } as unknown as D1Database + } + + afterEach(() => { + sqlite?.close() + vi.restoreAllMocks() + }) + + it('adds both missing columns', async () => { + vi.spyOn(console, 'log').mockImplementation(() => {}) + const db = legacyDb() + expect(columnsOf(sqlite, 'auth_two_factor')).not.toContain('failed_verification_count') + + await new MigrationService(db).ensureSchemaCompatibility() + + const cols = columnsOf(sqlite, 'auth_two_factor') + expect(cols).toContain('failed_verification_count') + expect(cols).toContain('locked_until') + }) + + it('is idempotent — a second pass adds nothing and throws nothing', async () => { + vi.spyOn(console, 'log').mockImplementation(() => {}) + const db = legacyDb() + const service = new MigrationService(db) + await service.ensureSchemaCompatibility() + const after = columnsOf(sqlite, 'auth_two_factor') + await expect(service.ensureSchemaCompatibility()).resolves.toBeUndefined() + expect(columnsOf(sqlite, 'auth_two_factor')).toEqual(after) + }) + + it('does nothing when the table is absent, rather than failing bootstrap', async () => { + const db = legacyDb() + sqlite.exec('DROP TABLE auth_two_factor') + await expect(new MigrationService(db).ensureSchemaCompatibility()).resolves.toBeUndefined() + }) +}) diff --git a/packages/core/src/__tests__/services/two-factor-lockout-engages.test.ts b/packages/core/src/__tests__/services/two-factor-lockout-engages.test.ts new file mode 100644 index 000000000..6897659bf --- /dev/null +++ b/packages/core/src/__tests__/services/two-factor-lockout-engages.test.ts @@ -0,0 +1,122 @@ +/** + * Does the per-account second-factor lockout actually LOCK? + * + * `two-factor-roundtrip.test.ts` proves the failure COUNTER reaches the column, and calls that + * counter "the control that actually bounds guessing". Those are different claims, and only the + * second one matters: BA writes `lockedUntil` only when the value RETURNED by its increment-update + * is >= maxFailedAttempts — + * + * ((await ctx.context.adapter.update({ ..., increment: { failedVerificationCount: 1 } })) + * ?.failedVerificationCount ?? 0) >= maxFailedAttempts + * + * — so an adapter whose update returns null, or a row without that field, leaves the `?? 0` in + * charge and the account never locks. With BA rate limiting off repo-wide, that lockout is the + * only thing bounding TOTP guessing: BA's own per-challenge `beginAttempt(5)` is keyed to the + * challenge cookie, so an attacker just signs in again for a fresh one. + * + * It also pins the OPTION NAMES. `verify-two-factor.mjs` reads `accountLockout.maxFailedAttempts` + * and `accountLockout.durationSeconds` off the composed plugin options and falls back to + * `?? 10` / `?? 900` per key. A rename on either side (ours or a BA upgrade) silently reverts to + * BA's defaults while every composition test — which asserts our own object back at us — stays + * green. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { createAuth } from '../../auth/config' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' +import { totpFromOtpauthUri } from '../utils/totp' +import type { Bindings } from '../../app' + +const EMAIL = 'lockout@roundtrip.test' +const PASSWORD = 'correct-horse-battery-staple' +const ORIGIN = 'https://sonic.test' + +let db: TestD1 +let kv: Map + +function makeAuth() { + const env = { + DB: db, + CACHE_KV: { + get: async (k: string) => kv.get(k) ?? null, + put: async (k: string, v: string) => void kv.set(k, v), + delete: async (k: string) => void kv.delete(k), + }, + BETTER_AUTH_SECRET: 'test-better-auth-secret-at-least-32-chars', + BETTER_AUTH_URL: ORIGIN, + JWT_SECRET: 'test-jwt-secret-value-32-chars-long!!', + } as unknown as Bindings + return createAuth(env, undefined, ORIGIN) +} + +async function call(path: string, body: unknown, cookies: string[] = []) { + const auth = makeAuth() + const headers: Record = { 'Content-Type': 'application/json', Origin: ORIGIN } + if (cookies.length) headers.Cookie = cookies.map((c) => c.split(';')[0]).join('; ') + const res = await auth.handler( + new Request(`${ORIGIN}/auth${path}`, { method: 'POST', headers, body: JSON.stringify(body) }), + ) + const setCookie = + (res.headers as unknown as { getSetCookie?: () => string[] }).getSetCookie?.() ?? [] + return { status: res.status, body: await res.json().catch(() => null), cookies: setCookie } +} + +function lockRow() { + return db.raw + .prepare(`SELECT failed_verification_count AS n, locked_until AS until FROM auth_two_factor`) + .get() as { n: number; until: number | null } +} + +beforeEach(() => { + db = createTestD1() + kv = new Map() + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.spyOn(console, 'log').mockImplementation(() => {}) +}) + +afterEach(() => { + db.close() + vi.restoreAllMocks() +}) + +describe('per-account second-factor lockout', () => { + it('locks the account after maxFailedAttempts and refuses even a CORRECT code', async () => { + await call('/sign-up/email', { email: EMAIL, password: PASSWORD, name: 'Lockout User' }) + const first = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + const enable = await call('/two-factor/enable', { password: PASSWORD }, first.cookies) + const totpUri: string = enable.body.totpURI + await call('/two-factor/verify-totp', { code: await totpFromOtpauthUri(totpUri) }, first.cookies) + + // Default policy = 5 consecutive failures. Take a FRESH challenge each time so BA's + // per-challenge beginAttempt(5) limiter is never the thing that stops us — the claim under + // test is the per-ACCOUNT lockout, which is what bounds an attacker who simply re-signs-in. + for (let i = 0; i < 5; i++) { + const challenged = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + expect(challenged.body).toMatchObject({ twoFactorRedirect: true }) + const bad = await call('/two-factor/verify-totp', { code: '000000' }, challenged.cookies) + expect(bad.status).not.toBe(200) + console.info(`attempt ${i + 1}:`, JSON.stringify(lockRow())) + } + + const after = lockRow() + expect(after.n, 'failure counter did not reach the threshold').toBeGreaterThanOrEqual(5) + expect(after.until, 'lockedUntil was never written — the lockout does not engage').not.toBeNull() + expect(after.until!).toBeGreaterThan(Date.now()) + + // The real proof: a VALID code must now be refused. + const challenged = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + const good = await call( + '/two-factor/verify-totp', + { code: await totpFromOtpauthUri(totpUri) }, + challenged.cookies, + ) + expect(good.status, `a valid code still worked while locked: ${JSON.stringify(good.body)}`).toBe( + 429, + ) + // Explicit timeout, not the 5s default. This test makes ~10 sequential Better Auth round + // trips, several of which run BA's real scrypt password hash, and lands around 5s on an idle + // machine — close enough to the default that adding any concurrent test file to the run tips + // it into a timeout that looks exactly like a broken lockout. The work is genuinely slow, so + // the limit is what should move. + }, 30_000) +}) diff --git a/packages/core/src/__tests__/services/two-factor-roundtrip.test.ts b/packages/core/src/__tests__/services/two-factor-roundtrip.test.ts new file mode 100644 index 000000000..e714e3cf1 --- /dev/null +++ b/packages/core/src/__tests__/services/two-factor-roundtrip.test.ts @@ -0,0 +1,266 @@ +/** + * The full second-factor round trip, driven through a REAL Better Auth instance over REAL + * (better-sqlite3) D1: sign-up → sign-in → enable → verify-totp → sign-in → challenge → verify. + * + * ── Why this file exists ── + * The donor commit this feature was ported from concluded that "better-auth cannot be driven below + * E2E" — no `BETTER_AUTH_SECRET` in the harness, no KV, and `withCloudflare` needing a Workers `cf` + * context. That conclusion was inherited into this port and it is **false**. All three are trivially + * satisfiable, as the Infowall agent demonstrated independently: + * + * - `BETTER_AUTH_SECRET` — just put it in the env object. + * - KV — `createKVStorage` uses only get/put/delete, so a Map is a complete substitute. + * - `cf` — `withCloudflare` only checks truthiness; `getDefaultAuthOptions` already passes `{}`. + * + * The one real blocker in THIS repo was the test D1 shim missing `Statement.raw()`, which drizzle's + * D1 driver needs for the RETURNING clause on every adapter write. Added in `utils/d1-sqlite.ts`. + * + * It matters because the critical defect in this port — `auth_two_factor.created_at/updated_at` + * declared NOT NULL with no default, so BA's own INSERT died and the UI reported it as a wrong + * password — was invisible to every test that seeded rows by hand, and would have failed HERE on + * the first assertion. Tests that assert on generated SQL and composed options prove the wiring; + * only running the thing proves the feature. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { createAuth } from '../../auth/config' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' +import { totpFromOtpauthUri } from '../utils/totp' +import type { Bindings } from '../../app' + +const EMAIL = 'admin@roundtrip.test' +const PASSWORD = 'correct-horse-battery-staple' +const ORIGIN = 'https://sonic.test' + +let db: TestD1 +/** Map-backed KV. `createKVStorage` only ever calls get/put/delete. */ +let kv: Map + +function makeAuth() { + const env = { + DB: db, + CACHE_KV: { + get: async (k: string) => kv.get(k) ?? null, + put: async (k: string, v: string) => void kv.set(k, v), + delete: async (k: string) => void kv.delete(k), + }, + BETTER_AUTH_SECRET: 'test-better-auth-secret-at-least-32-chars', + BETTER_AUTH_URL: ORIGIN, + JWT_SECRET: 'test-jwt-secret-value-32-chars-long!!', + } as unknown as Bindings + return createAuth(env, undefined, ORIGIN) +} + +/** POST a JSON body to a BA endpoint, forwarding cookies in and collecting them out. */ +async function call( + path: string, + body: unknown, + cookies: string[] = [], +): Promise<{ status: number; body: any; cookies: string[] }> { + const auth = makeAuth() + const headers: Record = { + 'Content-Type': 'application/json', + Origin: ORIGIN, + } + if (cookies.length) headers.Cookie = cookies.map((c) => c.split(';')[0]).join('; ') + const res = await auth.handler( + new Request(`${ORIGIN}/auth${path}`, { method: 'POST', headers, body: JSON.stringify(body) }), + ) + const setCookie = + (res.headers as unknown as { getSetCookie?: () => string[] }).getSetCookie?.() ?? [] + return { + status: res.status, + body: await res.json().catch(() => null), + cookies: setCookie, + } +} + +/** True when a Set-Cookie list contains a live (non-cleared) BA session cookie. */ +function hasSession(cookies: string[]): boolean { + return cookies.some( + (c) => c.includes('better-auth.session_token=') && !/session_token=;/.test(c) && !/Max-Age=0/.test(c), + ) +} + +/** Merge two cookie jars, later values winning. */ +function mergeCookies(a: string[], b: string[]): string[] { + const jar = new Map() + for (const c of [...a, ...b]) jar.set(c.split('=')[0]!, c) + return [...jar.values()] +} + +/** Sign up (first user is always allowed) and return the resulting cookie jar. */ +async function signUp() { + const res = await call('/sign-up/email', { email: EMAIL, password: PASSWORD, name: 'Admin User' }) + expect(res.status, `sign-up failed: ${JSON.stringify(res.body)}`).toBe(200) + return res.cookies +} + +beforeEach(() => { + db = createTestD1() + kv = new Map() + // The user-create `after` hook seeds RBAC documents; noisy but non-fatal on this harness. + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.spyOn(console, 'log').mockImplementation(() => {}) +}) + +afterEach(() => { + db.close() + vi.restoreAllMocks() +}) + +describe('two-factor round trip against real Better Auth', () => { + it('enrols, then challenges the next password sign-in, then verifies with a live code', async () => { + // ── 1. Account exists and password sign-in works with no second factor ────────────── + await signUp() + const first = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + expect(first.status).toBe(200) + expect(first.body?.twoFactorRedirect).toBeUndefined() + expect(hasSession(first.cookies)).toBe(true) + + // ── 2. Enable — this is the call that the NOT NULL defect made impossible ─────────── + const enable = await call('/two-factor/enable', { password: PASSWORD }, first.cookies) + expect(enable.status, `enable failed: ${JSON.stringify(enable.body)}`).toBe(200) + const totpUri: string = enable.body.totpURI + expect(totpUri).toMatch(/^otpauth:\/\/totp\//) + expect(Array.isArray(enable.body.backupCodes)).toBe(true) + expect(enable.body.backupCodes.length).toBe(10) + + // The row BA wrote — assert on storage, not on the mapping we asked it to resolve. + const row = db.raw + .prepare(`SELECT user_id, verified, failed_verification_count, locked_until, created_at FROM auth_two_factor`) + .get() as { user_id: string; verified: number; failed_verification_count: number; locked_until: number | null; created_at: number } + expect(row).toBeTruthy() + expect(row.verified).toBe(0) // not proven yet + expect(row.failed_verification_count).toBe(0) + expect(row.locked_until).toBeNull() + expect(row.created_at).toBeGreaterThan(0) // the defect: this used to be an unsatisfiable NULL + + // ── 3. Confirm with a live code ──────────────────────────────────────────────────── + const confirm = await call( + '/two-factor/verify-totp', + { code: await totpFromOtpauthUri(totpUri) }, + first.cookies, + ) + expect(confirm.status, `verify-totp failed: ${JSON.stringify(confirm.body)}`).toBe(200) + expect( + (db.raw.prepare(`SELECT verified AS v FROM auth_two_factor`).get() as { v: number }).v, + ).toBe(1) + expect( + (db.raw.prepare(`SELECT two_factor_enabled AS e FROM auth_user`).get() as { e: number }).e, + ).toBe(1) + + // ── 4. The password alone is no longer enough ─────────────────────────────────────── + const second = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + expect(second.status).toBe(200) // 200, not 401 — the password was correct + expect(second.body).toMatchObject({ twoFactorRedirect: true }) + expect(second.body.user).toBeUndefined() + expect(second.body.token).toBeUndefined() + expect(hasSession(second.cookies)).toBe(false) + + // ── 5. The challenge resolves into a session ──────────────────────────────────────── + const challenge = await call( + '/two-factor/verify-totp', + { code: await totpFromOtpauthUri(totpUri) }, + second.cookies, + ) + expect(challenge.status, `challenge failed: ${JSON.stringify(challenge.body)}`).toBe(200) + expect(hasSession(challenge.cookies)).toBe(true) + }) + + it('accepts a backup code at the challenge, and spends it', async () => { + const session = await signUp() + const enable = await call('/two-factor/enable', { password: PASSWORD }, session) + const totpUri: string = enable.body.totpURI + const backupCodes: string[] = enable.body.backupCodes + await call('/two-factor/verify-totp', { code: await totpFromOtpauthUri(totpUri) }, session) + + const challenged = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + expect(challenged.body).toMatchObject({ twoFactorRedirect: true }) + + const used = await call('/two-factor/verify-backup-code', { code: backupCodes[0]! }, challenged.cookies) + expect(used.status, `backup code rejected: ${JSON.stringify(used.body)}`).toBe(200) + expect(hasSession(used.cookies)).toBe(true) + + // Single-use: the same code must not resolve a second challenge. + const again = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + const reuse = await call('/two-factor/verify-backup-code', { code: backupCodes[0]! }, again.cookies) + expect(reuse.status).not.toBe(200) + expect(hasSession(reuse.cookies)).toBe(false) + }) + + it('rejects a wrong code and counts it against the per-account lockout', async () => { + const session = await signUp() + const enable = await call('/two-factor/enable', { password: PASSWORD }, session) + await call('/two-factor/verify-totp', { code: await totpFromOtpauthUri(enable.body.totpURI) }, session) + + const challenged = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + const bad = await call('/two-factor/verify-totp', { code: '000000' }, challenged.cookies) + expect(bad.status).not.toBe(200) + expect(hasSession(bad.cookies)).toBe(false) + + // The lockout counter is the control that actually bounds guessing (BA rate limiting is off — + // see the note in auth/config.ts), so prove the increment reaches the column. + const count = ( + db.raw.prepare(`SELECT failed_verification_count AS n FROM auth_two_factor`).get() as { n: number } + ).n + expect(count).toBeGreaterThanOrEqual(1) + }) + + it('does NOT honour trustDevice — the 30-day bypass stays closed', async () => { + // Break-it proof for `trustDeviceMaxAge: 0` in auth/config.ts. Remove that line and this test + // goes red: the second sign-in returns a session instead of a challenge, which is precisely the + // bypass. Asserting `options.trustDeviceMaxAge === undefined` — the intuitive thing to write — + // would have LOCKED IN the bug, because BA reads `?? 2592e3`. + const session = await signUp() + const enable = await call('/two-factor/enable', { password: PASSWORD }, session) + const totpUri: string = enable.body.totpURI + await call('/two-factor/verify-totp', { code: await totpFromOtpauthUri(totpUri) }, session) + + const challenged = await call('/sign-in/email', { email: EMAIL, password: PASSWORD }) + expect(challenged.body).toMatchObject({ twoFactorRedirect: true }) + + // Ask to be trusted, exactly as a hand-rolled client could. + const trusted = await call( + '/two-factor/verify-totp', + { code: await totpFromOtpauthUri(totpUri), trustDevice: true }, + challenged.cookies, + ) + expect(trusted.status).toBe(200) + expect(hasSession(trusted.cookies)).toBe(true) + + // Sign in again carrying every cookie BA just set, including any trust cookie. + const after = await call( + '/sign-in/email', + { email: EMAIL, password: PASSWORD }, + mergeCookies(challenged.cookies, trusted.cookies), + ) + expect(after.body, 'trustDevice bought a password-only sign-in').toMatchObject({ + twoFactorRedirect: true, + }) + expect(hasSession(after.cookies)).toBe(false) + }) +}) + +describe('the shared TOTP helper produces codes Better Auth accepts', () => { + it('verifies against BA, which is what de-risks the E2E spec', async () => { + // tests/e2e/101-two-factor-auth.spec.ts imports this same function. If it drifted — most + // likely by handing BA the base32 URI value instead of the decoded secret — the browser round + // trip would fail in CI with a 401 indistinguishable from a real product bug. + const session = await signUp() + const enable = await call('/two-factor/enable', { password: PASSWORD }, session) + const code = await totpFromOtpauthUri(enable.body.totpURI) + expect(code).toMatch(/^\d{6}$/) + const res = await call('/two-factor/verify-totp', { code }, session) + expect(res.status, `BA rejected our computed code (${code})`).toBe(200) + }) + + it('a code from the previous step no longer verifies, proving it is time-based', async () => { + const session = await signUp() + const enable = await call('/two-factor/enable', { password: PASSWORD }, session) + // Two full periods back is outside BA's tolerance window. + const stale = await totpFromOtpauthUri(enable.body.totpURI, Date.now() - 90_000) + const res = await call('/two-factor/verify-totp', { code: stale }, session) + expect(res.status).not.toBe(200) + }) +}) diff --git a/packages/core/src/__tests__/services/two-factor-settings.test.ts b/packages/core/src/__tests__/services/two-factor-settings.test.ts new file mode 100644 index 000000000..9a311c1d0 --- /dev/null +++ b/packages/core/src/__tests__/services/two-factor-settings.test.ts @@ -0,0 +1,196 @@ +/** + * The two-factor policy: normalization/clamping, the document-backed load, and the + * sync-getter contract `auth/config.ts` depends on. + * + * The clamp is the security-relevant half. The admin form declares min/max, but a form is a + * hint — `parseFormDataToSettings` will happily persist whatever a hand-rolled POST sends, and + * `maxFailedAttempts: 10000` would disable the lockout that is the only thing bounding a + * distributed TOTP guesser. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import type { D1Database } from '@cloudflare/workers-types' +import { + normalizeTwoFactorPolicy, + loadTwoFactorPolicy, + getTwoFactorPolicy, + invalidateTwoFactorPolicy, + refreshTwoFactorPolicy, + TWO_FACTOR_POLICY_DEFAULTS, + TWO_FACTOR_PLUGIN_ID, +} from '../../auth/two-factor-settings' +import { createTestD1, type TestD1 } from '../utils/d1-sqlite' + +let db: TestD1 + +function seedSettings(settings: unknown) { + db.raw + .prepare( + `INSERT INTO documents (id, root_id, type_id, slug, tenant_id, is_current_draft, data) + VALUES (?, ?, 'plugin', ?, 'default', 1, ?)`, + ) + .run('doc-2fa', 'root-2fa', TWO_FACTOR_PLUGIN_ID, JSON.stringify({ status: 'active', settings })) +} + +beforeEach(() => { + db = createTestD1() + invalidateTwoFactorPolicy() +}) + +afterEach(() => { + db.close() + invalidateTwoFactorPolicy() + vi.restoreAllMocks() +}) + +describe('normalizeTwoFactorPolicy', () => { + it('returns the defaults for empty / junk input', () => { + expect(normalizeTwoFactorPolicy(undefined)).toEqual(TWO_FACTOR_POLICY_DEFAULTS) + expect(normalizeTwoFactorPolicy(null)).toEqual(TWO_FACTOR_POLICY_DEFAULTS) + expect(normalizeTwoFactorPolicy('nonsense')).toEqual(TWO_FACTOR_POLICY_DEFAULTS) + expect(normalizeTwoFactorPolicy({})).toEqual(TWO_FACTOR_POLICY_DEFAULTS) + }) + + it('accepts values inside the declared bounds', () => { + expect( + normalizeTwoFactorPolicy({ + issuer: 'Acme CMS', + maxFailedAttempts: 3, + lockoutDurationSeconds: 1800, + backupCodeCount: 20, + }), + ).toEqual({ + issuer: 'Acme CMS', + maxFailedAttempts: 3, + lockoutDurationSeconds: 1800, + backupCodeCount: 20, + }) + }) + + it('clamps a lockout threshold that would disable the lockout', () => { + expect(normalizeTwoFactorPolicy({ maxFailedAttempts: 10_000 }).maxFailedAttempts).toBe(10) + expect(normalizeTwoFactorPolicy({ maxFailedAttempts: 0 }).maxFailedAttempts).toBe(3) + expect(normalizeTwoFactorPolicy({ maxFailedAttempts: -5 }).maxFailedAttempts).toBe(3) + }) + + it('clamps lockout duration and backup-code count', () => { + expect(normalizeTwoFactorPolicy({ lockoutDurationSeconds: 1 }).lockoutDurationSeconds).toBe(300) + expect(normalizeTwoFactorPolicy({ lockoutDurationSeconds: 99_999 }).lockoutDurationSeconds).toBe(3600) + expect(normalizeTwoFactorPolicy({ backupCodeCount: 1 }).backupCodeCount).toBe(5) + expect(normalizeTwoFactorPolicy({ backupCodeCount: 500 }).backupCodeCount).toBe(20) + }) + + it('coerces numeric strings, because FormData values arrive as strings', () => { + const p = normalizeTwoFactorPolicy({ + maxFailedAttempts: '7', + lockoutDurationSeconds: '600', + backupCodeCount: '8', + }) + expect(p).toMatchObject({ maxFailedAttempts: 7, lockoutDurationSeconds: 600, backupCodeCount: 8 }) + }) + + it('falls back rather than letting NaN reach BA\'s arithmetic', () => { + // `NULL + 1` and `NaN >= max` both read as "never lock out". + expect(normalizeTwoFactorPolicy({ maxFailedAttempts: 'five' }).maxFailedAttempts).toBe( + TWO_FACTOR_POLICY_DEFAULTS.maxFailedAttempts, + ) + expect(normalizeTwoFactorPolicy({ maxFailedAttempts: NaN }).maxFailedAttempts).toBe( + TWO_FACTOR_POLICY_DEFAULTS.maxFailedAttempts, + ) + }) + + it('rounds fractional values to integers', () => { + expect(normalizeTwoFactorPolicy({ maxFailedAttempts: 4.6 }).maxFailedAttempts).toBe(5) + }) + + it('ignores a blank issuer', () => { + expect(normalizeTwoFactorPolicy({ issuer: ' ' }).issuer).toBe(TWO_FACTOR_POLICY_DEFAULTS.issuer) + }) + + it('strips otpauth:// delimiters out of the issuer', () => { + // The issuer is interpolated into an otpauth:// label. ':' would split the label into a + // different issuer/account pair in the user's authenticator. + expect(normalizeTwoFactorPolicy({ issuer: 'Evil:Corp?x#y' }).issuer).toBe('Evil Corp x y') + }) + + it('bounds issuer length', () => { + expect(normalizeTwoFactorPolicy({ issuer: 'z'.repeat(500) }).issuer).toHaveLength(64) + }) +}) + +describe('policy load / cache', () => { + it('returns defaults before anything has been loaded', () => { + expect(getTwoFactorPolicy()).toEqual(TWO_FACTOR_POLICY_DEFAULTS) + }) + + it('reads the plugin document settings and exposes them via the sync getter', async () => { + seedSettings({ issuer: 'Acme', maxFailedAttempts: 4 }) + await loadTwoFactorPolicy(db as unknown as D1Database) + expect(getTwoFactorPolicy()).toMatchObject({ issuer: 'Acme', maxFailedAttempts: 4 }) + }) + + it('clamps stored out-of-range values on read, not just in the form', async () => { + seedSettings({ maxFailedAttempts: 9999 }) + await loadTwoFactorPolicy(db as unknown as D1Database) + expect(getTwoFactorPolicy().maxFailedAttempts).toBe(10) + }) + + it('uses defaults when the plugin document has no settings key', async () => { + db.raw + .prepare( + `INSERT INTO documents (id, root_id, type_id, slug, tenant_id, is_current_draft, data) + VALUES ('d','r','plugin',?, 'default', 1, ?)`, + ) + .run(TWO_FACTOR_PLUGIN_ID, JSON.stringify({ status: 'active' })) + await loadTwoFactorPolicy(db as unknown as D1Database) + expect(getTwoFactorPolicy()).toEqual(TWO_FACTOR_POLICY_DEFAULTS) + }) + + it('caches — a second load does not re-query', async () => { + seedSettings({ issuer: 'First' }) + await loadTwoFactorPolicy(db as unknown as D1Database) + db.raw + .prepare(`UPDATE documents SET data = ? WHERE slug = ?`) + .run(JSON.stringify({ status: 'active', settings: { issuer: 'Second' } }), TWO_FACTOR_PLUGIN_ID) + await loadTwoFactorPolicy(db as unknown as D1Database) + expect(getTwoFactorPolicy().issuer).toBe('First') + }) + + it('refresh re-reads, so a settings write lands in the writing isolate', async () => { + seedSettings({ issuer: 'First' }) + await loadTwoFactorPolicy(db as unknown as D1Database) + db.raw + .prepare(`UPDATE documents SET data = ? WHERE slug = ?`) + .run(JSON.stringify({ status: 'active', settings: { issuer: 'Second' } }), TWO_FACTOR_PLUGIN_ID) + await refreshTwoFactorPolicy(db as unknown as D1Database) + expect(getTwoFactorPolicy().issuer).toBe('Second') + }) + + it('degrades to defaults — never throws — when the read fails', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const broken = { + prepare: () => ({ + bind: () => ({ + first: async () => { + throw new Error('D1_ERROR: offline') + }, + }), + }), + } as unknown as D1Database + await expect(loadTwoFactorPolicy(broken)).resolves.toEqual(TWO_FACTOR_POLICY_DEFAULTS) + }) + + it('defaults sit inside the clamp bounds, so a failed load still yields a usable policy', () => { + // This used to be titled "the strict end of every knob, so a failed load cannot weaken + // policy" and asserted `<= 10` / `>= 300` / `>= 5` — which normalizeTwoFactorPolicy already + // guarantees for ANY input, so it was close to a tautology, and the claim was false besides: + // the defaults are mid-range (5 / 900 / 10), so a failed load hands back 5 attempts to an + // operator who had tightened it to 3. + // + // The property that IS real is containment: whatever happens, the lockout cannot be disabled + // and backup codes cannot drop below 5. Assert that against the bounds themselves, so + // widening BOUNDS without revisiting the defaults trips here. + // Clamping the defaults must be a NO-OP. If someone lowers backupCodeCount to 3 or raises + // maxFailedAttempts to 50, the clamp rewrites it and this equality breaks. + expect(normalizeTwoFactorPolicy(TWO_FACTOR_POLICY_DEFAULTS)).toEqual(TWO_FACTOR_POLICY_DEFAULTS) + }) +}) diff --git a/packages/core/src/__tests__/utils/d1-sqlite.ts b/packages/core/src/__tests__/utils/d1-sqlite.ts index 16161c145..2c31a3d9a 100644 --- a/packages/core/src/__tests__/utils/d1-sqlite.ts +++ b/packages/core/src/__tests__/utils/d1-sqlite.ts @@ -15,7 +15,12 @@ import { ensureScalarSchema, resetScalarSchemaCache } from '../../services/docum // services delete derived rows explicitly rather than relying on cascade. const MIGRATIONS_DIR = join(dirname(fileURLToPath(import.meta.url)), '../../../migrations') -const DOC_MIGRATIONS = ['0001_core.sql', '0002_documents.sql'] +const DOC_MIGRATIONS = [ + '0001_core.sql', + '0002_documents.sql', + '0006_two_factor_lockout.sql', + '0007_two_factor_required.sql', +] // better-sqlite3 only accepts numbers/strings/bigints/buffers/null. Coerce the values the // services bind (undefined, booleans) the same way D1's binder tolerates them. @@ -57,6 +62,31 @@ class TestStatement { return (colName ? (row[colName] as T) : (row as unknown as T)) } + /** + * D1's `Statement.raw()` — rows as positional arrays rather than objects. + * + * Required to drive Better Auth in these tests, not a completeness nicety. `getDefaultAuthOptions` + * builds `drizzle(env.DB)` from `drizzle-orm/d1`, and that driver reaches for `.raw()` on any + * statement with a RETURNING clause (`d1/session.cjs` → `this.stmt.bind(...).raw()`), which is + * every write BA performs through the adapter. Without it, driving BA against this harness fails + * with `this.stmt.bind(...).raw is not a function` — the reason an earlier version of this suite + * could only assert on GENERATED SQL and never on what BA actually wrote. + */ + async raw(options?: { columnNames?: boolean }): Promise { + // better-sqlite3's .raw() flips the statement into array mode and returns the statement. + const stmt = this.sqlite.prepare(this.sql).raw() + const rows = stmt.all(...(this.binds as never[])) as unknown[][] + if (options?.columnNames) { + // `.columns()` throws on statements that return no data; D1 would give [] there too. + try { + return [stmt.columns().map((c) => c.name), ...rows] as T[] + } catch { + return rows as T[] + } + } + return rows as T[] + } + // Used by batch() to execute a write statement inside the shared transaction. execInBatch(): void { this.sqlite.prepare(this.sql).run(...(this.binds as never[])) diff --git a/packages/core/src/__tests__/utils/totp.ts b/packages/core/src/__tests__/utils/totp.ts new file mode 100644 index 000000000..0a81fdf55 --- /dev/null +++ b/packages/core/src/__tests__/utils/totp.ts @@ -0,0 +1,88 @@ +/** + * RFC 6238 TOTP over WebCrypto — test-only. + * + * Exists so tests can compute a code Better Auth will actually accept, which is the difference + * between asserting that wrong inputs are rejected and asserting that the real enrolment flow + * works. There is no OTP dependency in this repo and this is ~40 lines, so it stays inline. + * + * Deliberately shared between two very different tiers: + * - `__tests__/services/two-factor-roundtrip.test.ts` drives real BA over real SQLite and proves + * the codes this produces are accepted by `createOTP(...).verify()`. + * - `tests/e2e/101-two-factor-auth.spec.ts` imports the SAME function for the browser round trip. + * + * One implementation, and the cheap tier verifies it. Two copies would let the E2E copy drift and + * fail in CI for a reason that looks like a product bug. + * + * ── The trap this encodes ── + * `secret` in an `otpauth://` URI is **base32 of the raw secret**, while BA HMACs the raw secret + * bytes. So the URI value MUST be base32-decoded before use. Passing it through verbatim yields a + * perfectly well-formed six-digit code that is simply wrong, and BA rejects it with the same 401 a + * genuine mismatch produces — which reads as "TOTP is broken" rather than "the test is wrong". + * {@link totpFromOtpauthUri} exists so no caller has to remember this. + */ + +/** Default TOTP parameters — match Better Auth's twoFactor defaults (SHA-1, 6 digits, 30s). */ +const DIGITS = 6 +const PERIOD_SECONDS = 30 + +/** + * Compute the current TOTP code for a **base32-encoded** secret, as it appears in an `otpauth://` + * URI's `secret` parameter. + * + * @param base32Secret the `secret` query parameter, base32, padding optional + * @param atMs evaluate at this instant instead of now (for step-boundary tests) + */ +export async function totp(base32Secret: string, atMs: number = Date.now()): Promise { + return hotp(base32Decode(base32Secret), Math.floor(atMs / 1000 / PERIOD_SECONDS)) +} + +/** Pull `secret` out of an `otpauth://` URI and compute its current code. */ +export async function totpFromOtpauthUri(uri: string, atMs: number = Date.now()): Promise { + const secret = new URL(uri).searchParams.get('secret') + if (!secret) throw new Error(`otpauth URI has no secret parameter: ${uri}`) + return totp(secret, atMs) +} + +/** HMAC-SHA1 based one-time password (RFC 4226) over an 8-byte big-endian counter. */ +async function hotp(key: Uint8Array, counter: number): Promise { + const msg = new Uint8Array(8) + for (let i = 7, c = counter; i >= 0; i--, c = Math.floor(c / 256)) msg[i] = c % 256 + const cryptoKey = await crypto.subtle.importKey( + 'raw', + key as unknown as ArrayBuffer, + { name: 'HMAC', hash: 'SHA-1' }, + false, + ['sign'], + ) + const mac = new Uint8Array( + await crypto.subtle.sign('HMAC', cryptoKey, msg as unknown as ArrayBuffer), + ) + // Dynamic truncation: low nibble of the last byte selects the 4-byte window. + const offset = mac[mac.length - 1]! & 0x0f + const bin = + ((mac[offset]! & 0x7f) << 24) | + (mac[offset + 1]! << 16) | + (mac[offset + 2]! << 8) | + mac[offset + 3]! + return String(bin % 10 ** DIGITS).padStart(DIGITS, '0') +} + +/** RFC 4648 base32 decode, padding optional, non-alphabet characters ignored. */ +export function base32Decode(input: string): Uint8Array { + const alphabet = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ234567' + const clean = input.replace(/=+$/, '').toUpperCase() + let bits = 0 + let value = 0 + const out: number[] = [] + for (const ch of clean) { + const idx = alphabet.indexOf(ch) + if (idx === -1) continue + value = (value << 5) | idx + bits += 5 + if (bits >= 8) { + out.push((value >>> (bits - 8)) & 0xff) + bits -= 8 + } + } + return new Uint8Array(out) +} diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index ff5209a43..2dd757008 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -41,11 +41,19 @@ import { userProfilesPlugin } from './plugins/core-plugins/user-profiles' import { aiSearchPlugin } from './plugins/core-plugins/ai-search-plugin' import { securityAuditPlugin } from './plugins/core-plugins/security-audit-plugin' import { securityAuditMiddleware, securityAuditApiRoutes, securityAuditAdminRoutes } from './plugins/core-plugins/security-audit-plugin' +import { + twoFactorAuthPlugin, + twoFactorChallengeRoutes, + twoFactorRecoveryRoutes, + enforceTwoFactorEnrolment, + guardRequiredSecondFactorDisable, +} from './plugins/core-plugins/two-factor-auth' import { apiKeysPlugin, apiKeyAuthMiddleware } from './plugins/core-plugins/api-keys-plugin' import { stripePlugin } from './plugins/core-plugins/stripe-plugin' import { formsPlugin } from './plugins/core-plugins/forms-plugin' import { requireAuth, requireRole, requireRbac, AuthManager } from './middleware/auth' import { createAuth } from './auth/config' +import { guardPasswordlessSecondFactor } from './auth/passwordless-second-factor-guard' import { adminRbacRoutes } from './routes/admin-rbac' import { pluginMenuMiddleware } from './middleware/plugin-menu' import { menuMiddleware } from './middleware/menu' @@ -283,6 +291,10 @@ export function createSonicJSApp(config: SonicJSConfig = {}): SonicJSApp { const magicLinkPlugin = createMagicLinkAuthPlugin() const corePluginsBeforeCatchAll = [ securityAuditPlugin, + // Mounts /admin/two-factor (enrolment) and /auth/two-factor (login challenge). Must be in + // the BEFORE-catch-all list: the challenge page has to win the route match against the + // `/auth/*` Better Auth catch-all registered further down. + twoFactorAuthPlugin, apiKeysPlugin, aiSearchPlugin, oauthProvidersPlugin, @@ -613,6 +625,21 @@ export function createSonicJSApp(config: SonicJSConfig = {}): SonicJSApp { } }) + // Forced two-factor re-enrolment. Runs last of the /admin/* middleware — after requireAuth + // (it needs c.get('user')) and after the RBAC shell (a user who owes an enrolment should still + // land on a page they have permission for once they finish). + // + // Wired here rather than in the plugin's register(): Hono composes matched handlers in + // registration order, and plugin registration is interleaved with the app.route('/admin/...') + // calls below, so middleware added from a plugin would silently not run for any admin route + // mounted before it. See recovery.ts. + app.use('/admin/*', enforceTwoFactorEnrolment()) + + // The break-glass reset. Mounted unconditionally and on its own prefix — deactivating the + // two-factor plugin does not stop Better Auth from challenging enrolled users, so the recovery + // path must outlive the plugin's own surface. See recovery.ts. + app.route('/admin/two-factor-reset', twoFactorRecoveryRoutes) + // Plugin-specific API routes that would otherwise be shadowed by the generic // /api/:collection/:id catch-all must be mounted BEFORE app.route('/api', apiRoutes). app.route('/api/security-audit', securityAuditApiRoutes as any) @@ -675,13 +702,44 @@ export function createSonicJSApp(config: SonicJSConfig = {}): SonicJSApp { app.route('/admin/logs', adminLogsRoutes) app.route('/admin/rbac', adminRbacRoutes) app.route('/admin', adminUsersRoutes) + + // The 2FA login challenge, mounted from CORE and deliberately OUTSIDE the `disableAll` guard. + // + // `twoFactor()` is composed into Better Auth unconditionally (auth/config.ts), so an enrolled + // user is challenged on sign-in whether or not plugins are enabled. When the challenge PAGE was + // mounted by the plugin, `config.plugins.disableAll` short-circuited `boot()` before + // `wirePlugins()` and the redirect to `/auth/two-factor` landed on a 404 — locking every + // enrolled user out of an app that still demanded their second factor. + // + // Only the ENROLMENT surface (`/admin/two-factor`) belongs to the plugin, because turning the + // plugin off should stop new enrolments without stranding existing ones. These routes carry no + // plugin-active gate for the same reason. + app.route('/auth/two-factor', twoFactorChallengeRoutes) + app.route('/auth', authRoutes) // Better Auth handler — serves /auth/sign-in/*, /auth/sign-up/*, /auth/sign-out, // /auth/get-session, /auth/callback/* etc. Registered AFTER authRoutes so the // page-render routes (GET /auth/login, /auth/register) take precedence; only // Better Auth's own API paths fall through to this catch-all. - app.on(['GET', 'POST'], '/auth/*', (c) => { + app.on(['GET', 'POST'], '/auth/*', async (c) => { + // Second-factor enforcement for the passwordless flows. BA's twoFactor plugin challenges + // only on /sign-in/email|username|phone-number, so magicLink and emailOTP would otherwise + // hand an enrolled account a session with no code. Runs BEFORE auth.handler so nothing is + // mailed; reads a body clone so the stream handed to BA is untouched. + const refused = await guardPasswordlessSecondFactor(c as unknown as Context<{ Bindings: { DB: D1Database } }>) + if (refused) return refused + + // A user whose admin MANDATED a second factor may not turn it off. Also runs before + // auth.handler, because BA's /two-factor/disable deletes the enrolment row in its handler — + // an after-hook would fire once the account was already unprotected. The /admin/* enforcement + // middleware cannot cover this: wrong path, and the page hosting the disable form has to stay + // exempt so enrolment is possible at all. + const disableRefused = await guardRequiredSecondFactorDisable( + c as unknown as Context<{ Bindings: { DB: D1Database }; Variables: { user?: { userId?: string } } }>, + ) + if (disableRefused) return disableRefused + const reqUrl = new URL(c.req.url) const requestBaseURL = `${reqUrl.protocol}//${reqUrl.host}` const auth = createAuth(c.env, config.auth?.extendBetterAuth, requestBaseURL) diff --git a/packages/core/src/auth/config.ts b/packages/core/src/auth/config.ts index f4033cd2a..d7c343bc6 100644 --- a/packages/core/src/auth/config.ts +++ b/packages/core/src/auth/config.ts @@ -52,9 +52,11 @@ import { APIError } from 'better-auth/api' import { magicLink } from 'better-auth/plugins/magic-link' import { emailOTP } from 'better-auth/plugins/email-otp' import { organization } from 'better-auth/plugins/organization' +import { twoFactor } from 'better-auth/plugins/two-factor' import { drizzle } from 'drizzle-orm/d1' -import { authUser, authSession, authAccount, authVerification, authTenant, authTenantMember, authTenantInvitation, authTenantTeam } from '../db/schema' +import { authUser, authSession, authAccount, authVerification, authTwoFactor, authTenant, authTenantMember, authTenantInvitation, authTenantTeam } from '../db/schema' import { isRegistrationEnabled, isFirstUserRegistration } from '../services/auth-validation' +import { getTwoFactorPolicy } from './two-factor-settings' import type { Bindings } from '../app' /** @@ -99,7 +101,7 @@ export function getDefaultAuthOptions(env: Bindings, requestBaseURL?: string) { db, options: { // Keys MUST match modelName values — BA resolves by modelName, not by JS variable name. - schema: { auth_user: authUser, auth_session: authSession, auth_account: authAccount, auth_verification: authVerification, auth_tenant: authTenant, auth_tenant_member: authTenantMember, auth_tenant_invitation: authTenantInvitation, auth_tenant_team: authTenantTeam }, + schema: { auth_user: authUser, auth_session: authSession, auth_account: authAccount, auth_verification: authVerification, auth_two_factor: authTwoFactor, auth_tenant: authTenant, auth_tenant_member: authTenantMember, auth_tenant_invitation: authTenantInvitation, auth_tenant_team: authTenantTeam }, }, }, kv: env.CACHE_KV, // session secondary storage → getSession skips D1 @@ -240,6 +242,72 @@ export function getDefaultAuthOptions(env: Bindings, requestBaseURL?: string) { expiresIn: 10 * 60, }), + // Second factor — TOTP + single-use backup codes + per-ACCOUNT lockout. + // + // Composed UNCONDITIONALLY, and that is the security decision, not laziness. If this + // were gated on the plugin row being active, deactivating the plugin would silently + // downgrade every enrolled account to password-only: sign-in would stop asking for a + // code, nothing would error, and /admin/profile would still read "Enabled". Plugin + // status therefore gates the enrolment SURFACE only (routes 404, sidebar hides) — never + // verification. A gate that needs an `EXISTS` probe would also put a D1 round-trip on + // `createAuth`, which is synchronous and runs on every authenticated request. + // + // NOTE no `schema.*.fields` maps. The drizzle adapter resolves a BA field to a drizzle + // PROPERTY KEY, and `authTwoFactor`/`authUser` already declare `userId`, `backupCodes`, + // `failedVerificationCount`, `lockedUntil`, `twoFactorEnabled` — the snake_case column + // names live on the drizzle columns. Adding maps here would double-map and break both + // reads and writes. (The sibling Infowall port needs them because it is on the kysely + // adapter, which takes raw column names.) + twoFactor({ + // Read synchronously — BA snapshots these onto its plugin options at construction + // time and `verify-two-factor.mjs` pulls the lockout config back off that object, so + // a lazily-resolved value would never be seen. Loaded once per isolate from the + // plugin's onBoot; defaults (the strict end of every knob) apply until then. + ...(() => { + const policy = getTwoFactorPolicy() + return { + issuer: policy.issuer, + // Per-ACCOUNT, so rotating IPs does not help. This is the control that matters + // once an attacker already holds a valid password: a TOTP code is only a million + // possibilities, and per-IP rate limiting does not bound a distributed guesser. + accountLockout: { + enabled: true, + maxFailedAttempts: policy.maxFailedAttempts, + durationSeconds: policy.lockoutDurationSeconds, + }, + backupCodeOptions: { amount: policy.backupCodeCount }, + } + })(), + twoFactorTable: 'auth_two_factor', + // Trusted-device remember-me is switched OFF, and it takes an explicit `0` to do it. + // + // Leaving the option unset does NOT disable the feature: BA reads + // `options?.trustDeviceMaxAge ?? 2592e3` (two-factor/index.mjs, and again in + // verify-two-factor.mjs), i.e. it defaults to THIRTY DAYS, and `verifyTOTPBodySchema` + // accepts `trustDevice` from the request body. So any client — not just the page shipped + // here, which never sends it — could post + // `{code:'', trustDevice:true}` once and then sign in with the password + // alone for a month, refreshed on every sign-in. One phished code or one stolen backup + // code would convert into a month-long password-only bypass, invisible in normal use. + // + // `0` is not nullish, so it wins the `??`: the cookie is written with `Max-Age=0` and the + // trust record's `expiresAt` is `Date.now() + 0`, which can never satisfy the sign-in + // hook's `expiresAt > new Date()` check. A trusted-device cookie is a second bypass + // surface to reason about and this release does not take it on. + // + // Break-it proof: __tests__/services/two-factor-roundtrip.test.ts goes red without this + // line — the second password sign-in returns a session instead of a challenge. + trustDeviceMaxAge: 0, + // BA's own /two-factor/* rate limit (3 per 10s) is left alone: a `rateLimit.customRules` + // entry REPLACES a plugin's rule rather than stacking with it. Note that rule is NOT + // currently active — `rateLimit.enabled` defaults to BA's `isProduction`, which reads + // `process.env.NODE_ENV`, and nothing in this repo sets it (wrangler.toml sets only + // ENVIRONMENT). What actually bounds TOTP guessing today is the per-account lockout + // configured above (D1-backed, atomic via `incrementOne`) plus BA's in-code + // `beginAttempt(5)` per challenge. Turning BA rate limiting on globally is a separate, + // repo-wide decision — it would apply to every /auth/* endpoint. + }), + organization({ schema: { organization: { diff --git a/packages/core/src/auth/passwordless-second-factor-guard.ts b/packages/core/src/auth/passwordless-second-factor-guard.ts new file mode 100644 index 000000000..ace15cf7b --- /dev/null +++ b/packages/core/src/auth/passwordless-second-factor-guard.ts @@ -0,0 +1,222 @@ +/** + * Refuse passwordless (email-possession) sign-in for accounts that hold a verified second + * factor. + * + * ── The hole this closes ── + * Better Auth's `twoFactor` plugin challenges on `/sign-in/email|username|phone-number` and + * nothing else. `auth/config.ts` also composes `magicLink` and `emailOTP`, both of which mint a + * full session on their own endpoints. So without this guard, a user who enrolled in TOTP could + * sign in with a link or code mailed to their inbox and never see a second factor — with + * `/admin/profile` still reading "Enabled". A control that reports success and does nothing is + * worse than no control. + * + * It is also incoherent on its own terms: email possession is exactly what the design refuses to + * accept AS a second factor, so it cannot be allowed to stand in for the first one plus the + * second. + * + * ── Why here and not in a BA hook ── + * `magic-link/verify` finishes with `throw ctx.redirect(callbackURL)`, so a BA `after` hook is + * not a dependable chokepoint. The `/auth/*` catch-all in `app.ts` is our own code, runs ahead + * of `auth.handler()`, and can read the request body — so the block lands on the request that + * would START the flow, before any token or code is mailed. + * + * ── Enumeration ── + * Every refusal is byte-identical to what Better Auth itself would answer, so an unauthenticated + * caller learns nothing about who has a second factor: + * - the INITIATE endpoints return BA's own success shape and send nothing. BA already answers + * `{success:true}` for an address with no account at all, so this adds no new signal. + * - the COMPLETE endpoint returns BA's own `INVALID_OTP` error. An earlier revision returned a + * distinctive `403 TWO_FACTOR_REQUIRED` here, justified by "the caller already produced a + * code from the inbox" — which was FALSE: this guard runs before BA validates the code, so + * `{"email":"…","otp":"000000"}` with no inbox access was a free oracle for both + * account-existence and 2FA-enrolment status. Do not reintroduce a distinguishable response. + * + * ── Deliberately NOT guarded ── + * - `/auth/callback/*` (social/OAuth). BA does not challenge there either. Delegating MFA to + * the identity provider is the conventional contract, and blocking it would lock out an + * account whose only credential is the provider. + * - `GET /auth/magic-link/verify`. **A residual window remains here, and it is not closed.** + * Blocking the send means an enrolled user never receives a NEW link, but a link mailed in + * the 15 minutes BEFORE they enrolled still resolves to a session with no second factor. It + * is not guarded because the token cannot be resolved to a user without consuming it: BA + * stores verification values in KV (this app supplies `secondaryStorage`, and + * `verification.storeInDatabase` is unset, so `auth_verification` is not used), keyed by + * BA's internal format, and `storeToken` is not exported. Closing it properly means moving + * verification values into D1 — a repo-wide change affecting magic-link, email-OTP and email + * verification, out of scope here. The exposure requires an attacker who owns the inbox AND a + * link requested in the same 15-minute window in which the victim enrolled. + * - `/auth/email-otp/verify-email`. It mints a session only under + * `emailVerification.autoSignInAfterVerification`, which SonicJS does not set; guarding it + * unconditionally would 403 legitimate email verification for enrolled users. **If that + * option is ever enabled, add the path to {@link COMPLETE_PATHS}.** + * - `/auth/email-otp/request-password-reset`, `/forget-password/email-otp`, + * `/email-otp/reset-password`. Password reset does not mint a session in this codebase — it + * redirects to the login page, where the second factor is enforced. + * + * Bespoke SonicJS session-minting endpoints under `/auth/` (the `otp-login` and + * `oauth-providers` plugins) are mounted BEFORE this catch-all and so never reach it; they call + * {@link hasVerifiedSecondFactor} directly in their own handlers. + */ +import type { Context } from 'hono' +import type { D1Database } from '@cloudflare/workers-types' +import { hasVerifiedSecondFactor } from './second-factor-guard' + +/** + * Endpoints that START a passwordless flow. Blocked by answering BA's success shape without + * forwarding, so no link/code is ever mailed and nothing is leaked. + * + * Keyed by the full request path (BA's `basePath` is `/auth` — see `getDefaultAuthOptions`). + * `sign-in/magic-link` answers `{status:true}`; `email-otp/send-verification-otp` answers + * `{success:true}` — mirrored exactly so a client cannot distinguish a blocked call. + */ +const INITIATE_PATHS: Record> = { + '/auth/sign-in/magic-link': { status: true }, + '/auth/email-otp/send-verification-otp': { success: true }, +} + +/** Endpoints that COMPLETE a passwordless sign-in. Blocked with BA's own invalid-code error. */ +const COMPLETE_PATHS = new Set(['/auth/sign-in/email-otp']) + +/** + * Better Auth's own answer to a bad OTP — `email-otp/routes.mjs` throws + * `APIError.from('BAD_REQUEST', EMAIL_OTP_ERROR_CODES.INVALID_OTP)`. Mirrored verbatim so a + * refusal is indistinguishable from a wrong code. + */ +const BA_INVALID_OTP = { message: 'Invalid OTP', code: 'INVALID_OTP' } + +/** + * `send-verification-otp` is multi-purpose. Only the sign-in variant mints a session; the other + * two are email verification and password reset, and blocking those would break unrelated flows + * for an enrolled user. + */ +const OTP_SIGN_IN_TYPE = 'sign-in' + +/** Outcome of resolving an email to a user id. */ +type UserLookup = + | { kind: 'found'; userIds: string[] } + | { kind: 'absent' } + /** The query threw. Callers must fail CLOSED — see resolveUsersByEmail. */ + | { kind: 'error' } + +/** + * Inspect a request bound for the Better Auth handler. Returns a `Response` when the request + * must be refused, or `null` to let it through. + * + * Body handling: reads a **clone**, never `c.req.json()`. Hono caches a parsed body on the + * request, but the underlying `c.req.raw` stream would be consumed and `auth.handler(c.req.raw)` + * would then fail to parse it. + */ +export async function guardPasswordlessSecondFactor( + c: Context<{ Bindings: { DB: D1Database } }> +): Promise { + const path = new URL(c.req.url).pathname + const initiateResponse = INITIATE_PATHS[path] + const isComplete = COMPLETE_PATHS.has(path) + if (!initiateResponse && !isComplete) return null + if (c.req.method !== 'POST') return null + + const body = await readBody(c) + if (!body) { + // Nothing parseable, so there is no email to check and BA will reject the request itself + // (its router pins these endpoints to application/json). Passing through cannot bypass + // anything: a request BA refuses to parse never reaches a session mint. + return null + } + + const email = typeof body.email === 'string' ? body.email.trim() : '' + if (!email) return null + + // Only the sign-in variant of send-verification-otp leads to a session. + if (path === '/auth/email-otp/send-verification-otp' && body.type !== OTP_SIGN_IN_TYPE) { + return null + } + + const lookup = await resolveUsersByEmail(c.env.DB, email) + if (lookup.kind === 'absent') return null + + // Fail CLOSED on a lookup error. The earlier revision passed these through, reasoning that a + // user we cannot resolve cannot be shown to be enrolled — but BA performs its OWN lookup + // afterwards and can succeed where ours failed, mailing a link to an enrolled account. That + // made a transient D1 error a bypass, one line away from the deliberate fail-closed in + // hasVerifiedSecondFactor. + const enrolled = + lookup.kind === 'error' || + (await anyEnrolled(c.env.DB, lookup.userIds)) + if (!enrolled) return null + + if (initiateResponse) { + console.warn('[two-factor] passwordless initiation refused for an enrolled account:', path) + return c.json(initiateResponse) + } + console.warn('[two-factor] passwordless completion refused for an enrolled account:', path) + return c.json(BA_INVALID_OTP, 400) +} + +/** + * Parse the request body as JSON, falling back to form encodings. + * + * Today only JSON can reach these endpoints — BA's router pins them to + * `allowedMediaTypes: ["application/json"]`, so a form-encoded POST 415s before the handler. The + * fallback is here so this guard does not depend on that default: if BA ever accepts form bodies + * on `/sign-in/magic-link` or `/sign-in/email-otp`, or an app widens `allowedMediaTypes` through + * `extendBetterAuth`, the block still applies instead of silently opening. + */ +async function readBody( + c: Context<{ Bindings: { DB: D1Database } }> +): Promise | null> { + const contentType = c.req.header('Content-Type') ?? '' + if (contentType.includes('application/x-www-form-urlencoded') || contentType.includes('multipart/form-data')) { + try { + const form = await c.req.raw.clone().formData() + return Object.fromEntries(form.entries()) + } catch { + return null + } + } + try { + const parsed = (await c.req.raw.clone().json()) as unknown + return parsed && typeof parsed === 'object' ? (parsed as Record) : null + } catch { + return null + } +} + +/** + * Resolve every user id whose email matches, case-insensitively. + * + * `COLLATE NOCASE` deliberately, even though it forgoes `idx_auth_user_email`: SonicJS's own + * login routes lowercase before storing, but BA's magic-link endpoint passes `ctx.body.email` + * through verbatim, so a case-sensitive compare could MISS an enrolled account — and a miss here + * is a bypass, not a slow query. These endpoints are low-traffic by nature. + * + * Returns ALL matches rather than `LIMIT 1`. `auth_user.email` is UNIQUE under BINARY collation, + * so `alice@x.com` and `ALICE@x.com` can both exist; an unordered `LIMIT 1` could return the + * non-enrolled twin and let the enrolled account through. + */ +async function resolveUsersByEmail(db: D1Database, email: string): Promise { + try { + const rows = await db + .prepare(`SELECT id FROM auth_user WHERE email = ? COLLATE NOCASE`) + .bind(email) + .all<{ id: string }>() + const userIds = (rows.results ?? []).map((r) => r.id) + return userIds.length === 0 ? { kind: 'absent' } : { kind: 'found', userIds } + } catch (e) { + console.error('[two-factor] user lookup failed in passwordless guard; failing closed', e) + return { kind: 'error' } + } +} + +/** True if ANY of the matched accounts holds a verified second factor. */ +async function anyEnrolled(db: D1Database, userIds: string[]): Promise { + for (const id of userIds) { + if (await hasVerifiedSecondFactor(db, id)) return true + } + return false +} + +/** Exported for tests — the exact set of paths this guard is responsible for. */ +export const GUARDED_PASSWORDLESS_PATHS = Object.freeze({ + initiate: Object.freeze(Object.keys(INITIATE_PATHS)), + complete: Object.freeze([...COMPLETE_PATHS]), +}) diff --git a/packages/core/src/auth/second-factor-guard.ts b/packages/core/src/auth/second-factor-guard.ts new file mode 100644 index 000000000..9440bb8c9 --- /dev/null +++ b/packages/core/src/auth/second-factor-guard.ts @@ -0,0 +1,110 @@ +/** + * Second-factor enforcement for sign-in paths Better Auth's `twoFactor` plugin does not cover. + * + * ── Why this file exists ── + * BA's twoFactor plugin installs exactly one after-hook, and its matcher is: + * + * context.path === '/sign-in/email' || '/sign-in/username' || '/sign-in/phone-number' + * + * (better-auth/dist/plugins/two-factor/index.mjs). Every OTHER path that mints a session + * therefore issues one with no second factor. In SonicJS that means the two BA plugins + * composed alongside it in `auth/config.ts`: + * + * - `magicLink` → POST /auth/sign-in/magic-link, GET /auth/magic-link/verify + * - `emailOTP` → POST /auth/sign-in/email-otp, POST /auth/email-otp/verify-email + * + * A user who enrols in TOTP could request a magic link and sign in with no second factor at + * all — while `/admin/profile` reads "Enabled". That is strictly worse than having no second + * factor, because it is a control that reports success and does nothing. It is also the same + * channel the design refuses to accept AS a second factor (whoever owns the inbox owns the + * code), so leaving it as a complete FIRST-factor bypass would be incoherent. + * + * Social/OAuth sign-in (`/auth/callback/*`) is deliberately NOT covered — see + * `passwordless-second-factor-guard.ts`. + * + * ── Fail closed, deliberately ── + * Most SonicJS DB helpers degrade open on a D1 error, which is right in front of a CMS read. + * This sits in front of a session mint, and the failure mode of guessing "no second factor" + * is handing an unchallenged session to precisely the account that asked for the extra + * factor. So a query error here BLOCKS. + */ +import type { D1Database } from '@cloudflare/workers-types' + +/** + * True when `userId` has a COMPLETED second-factor enrolment, and must therefore not be + * handed a session by any path that has not verified one. + * + * Keys off `auth_two_factor.verified = 1`, NOT `auth_user.two_factor_enabled`. In BA 1.6.22 the + * two flip together — `verifyTOTP` sets both at the first successful verification, and + * `/two-factor/enable` sets neither unless `skipVerificationOnEnable` is on (which this app does + * not enable). `verified` is still the right column: it is the one BA's own sign-in gate consults + * (`if (isSignIn && twoFactor.verified === false) throw TOTP_NOT_ENABLED`), it stays correct if + * `skipVerificationOnEnable` is ever turned on (where the user flag WOULD lead enrolment), and it + * is scoped to the enrolment row rather than to a user column other features may come to write. + * + * Returns `true` (blocking) if the query throws — see the fail-closed note above. A missing + * `auth_two_factor` table is the one exception: it means nobody can possibly be enrolled, so + * it resolves `false` rather than locking everyone out of a deployment whose schema predates + * the table. + */ +export async function hasVerifiedSecondFactor(db: D1Database, userId: string): Promise { + if (!userId) { + // No principal to check. Callers resolve the id from a request body, so an empty value + // means "user not found" — nothing to protect, and returning true would break sign-up. + return false + } + try { + const row = await db + .prepare(`SELECT 1 AS present FROM auth_two_factor WHERE user_id = ? AND verified = 1 LIMIT 1`) + .bind(userId) + .first<{ present: number }>() + return row?.present === 1 + } catch (e) { + const message = e instanceof Error ? e.message : String(e) + // "no such table" is structural, not a failure: the table does not exist in this + // deployment, so there are no enrolments to protect. Anything else is a real error and + // must block rather than silently wave a session through. + if (/no such table/i.test(message)) return false + console.error('[second-factor-guard] enrolment lookup failed; blocking session mint', e) + return true + } +} + +/** What the enrolment UI needs in order to pick which of its three states to render. */ +export interface TwoFactorEnrolmentState { + /** A row exists — enrolment has at least been started. */ + enrolled: boolean + /** + * The enrolment was proven against a live code. Only a verified enrolment actually + * challenges at sign-in, and only a verified enrolment blocks the passwordless paths. + */ + verified: boolean +} + +/** + * Read one user's enrolment state. + * + * Read-only over BA's own table: this codebase never writes `auth_two_factor`. BA owns the + * secret, the backup codes and the lockout counters, and the secret is encrypted with BA's + * key — writing them from here would mean two authorities over one table. + * + * Fails OPEN to `{enrolled:false, verified:false}`, unlike {@link hasVerifiedSecondFactor}: + * this feeds a UI, not a security decision. A DB hiccup should render "not enrolled", not a + * 500 on the account page. + */ +export async function getEnrolmentState( + db: D1Database, + userId: string +): Promise { + try { + const row = await db + .prepare(`SELECT verified FROM auth_two_factor WHERE user_id = ? LIMIT 1`) + .bind(userId) + .first<{ verified: number }>() + if (!row) return { enrolled: false, verified: false } + return { enrolled: true, verified: row.verified === 1 } + } catch (e) { + console.error('[second-factor-guard] enrolment state read failed; rendering not-enrolled', e) + return { enrolled: false, verified: false } + } +} diff --git a/packages/core/src/auth/two-factor-settings.ts b/packages/core/src/auth/two-factor-settings.ts new file mode 100644 index 000000000..735a88758 --- /dev/null +++ b/packages/core/src/auth/two-factor-settings.ts @@ -0,0 +1,152 @@ +/** + * Two-factor policy: the knobs `twoFactor()` is composed from in `auth/config.ts`. + * + * ── Why the getter is synchronous ── + * `createAuth()` is synchronous and runs on EVERY request (app.ts session middleware), and + * BA reads `issuer` / `accountLockout` / `backupCodeOptions` off the static options object it + * was constructed with — `verify-two-factor.mjs` pulls the lockout config via + * `ctx.context.getPlugin('two-factor')?.options`, not through a resolver. So the values have + * to be available *at construction time*, and construction cannot await a D1 read without + * putting a round-trip on the authenticated hot path. + * + * The split is therefore: {@link loadTwoFactorPolicy} (async, one D1 read) is called once per + * isolate from the plugin's `onBoot`, and {@link getTwoFactorPolicy} (sync) is what + * `getDefaultAuthOptions` reads. Ordering is guaranteed by app.ts: the `boot()` middleware + * that runs plugin `onBoot` is registered ahead of the session middleware, and it is awaited. + * + * ── Staleness contract ── + * One load per isolate, no TTL — the same contract `isPluginActive()` already ships with. + * A settings change takes effect on new isolates immediately and on a warm one when it + * recycles, or right away in the isolate that performed the write if it calls + * {@link invalidateTwoFactorPolicy}. Before the first load, and on any read failure, the + * DEFAULTS below apply. + * + * Be precise about what that buys, because an earlier version of this comment overstated it: + * the defaults are the SHIPPED defaults, not the extreme end of each range (5 attempts and 900s + * sit mid-way through BOUNDS' 3–10 and 300–3600). So a failed load reverts an operator's + * override in EITHER direction — someone who tightened `maxFailedAttempts` to 3 gets 5 back. + * What is actually guaranteed is the BOUND, not the direction: every value the policy can ever + * take, loaded or defaulted, is inside BOUNDS, so no read failure and no stored value can + * disable the lockout or reduce backup codes below 5. + * + * Note what is NOT here: nothing gates whether `twoFactor()` is composed at all. That is + * deliberate — see the ADR in project-plan.md. Plugin status gates the enrolment surface; + * it must never gate verification, or deactivating the plugin would silently downgrade every + * enrolled account to password-only while the UI still read "Enabled". + */ +import type { D1Database } from '@cloudflare/workers-types' + +export interface TwoFactorPolicy { + /** Label shown beside the account in the user's authenticator app. */ + issuer: string + /** Consecutive failed second-factor verifications before the account locks. */ + maxFailedAttempts: number + /** How long that lock lasts, in seconds. */ + lockoutDurationSeconds: number + /** How many single-use backup codes are minted at enrolment. */ + backupCodeCount: number +} + +/** Plugin id — must match `manifest.json` `id` and the plugin document's slug. */ +export const TWO_FACTOR_PLUGIN_ID = 'two-factor-auth' + +export const TWO_FACTOR_POLICY_DEFAULTS: Readonly = Object.freeze({ + issuer: 'SonicJS', + maxFailedAttempts: 5, + lockoutDurationSeconds: 900, + backupCodeCount: 10, +}) + +/** + * Bounds every knob. An operator-supplied value outside these is clamped rather than + * rejected: a settings form should not be able to disable the lockout (maxFailedAttempts: + * 10_000) or mint a single backup code, and a value that arrives as a string from FormData + * must not silently become NaN inside BA's arithmetic. + */ +const BOUNDS = { + maxFailedAttempts: { min: 3, max: 10 }, + lockoutDurationSeconds: { min: 300, max: 3600 }, + backupCodeCount: { min: 5, max: 20 }, +} as const + +let cached: TwoFactorPolicy | null = null + +function clampInt(value: unknown, fallback: number, bounds: { min: number; max: number }): number { + const n = typeof value === 'number' ? value : Number(value) + if (!Number.isFinite(n)) return fallback + return Math.min(bounds.max, Math.max(bounds.min, Math.round(n))) +} + +/** Normalize whatever is stored in the plugin document into a usable, bounded policy. */ +export function normalizeTwoFactorPolicy(raw: unknown): TwoFactorPolicy { + const s = (raw && typeof raw === 'object' ? raw : {}) as Record + const issuer = typeof s.issuer === 'string' && s.issuer.trim() !== '' ? s.issuer.trim() : TWO_FACTOR_POLICY_DEFAULTS.issuer + return { + // The issuer lands in an `otpauth://` URI. Strip the two characters that would let a + // stored value break out of the label segment and forge a different account entry. + issuer: issuer.replace(/[:?#]/g, ' ').slice(0, 64), + maxFailedAttempts: clampInt(s.maxFailedAttempts, TWO_FACTOR_POLICY_DEFAULTS.maxFailedAttempts, BOUNDS.maxFailedAttempts), + lockoutDurationSeconds: clampInt(s.lockoutDurationSeconds, TWO_FACTOR_POLICY_DEFAULTS.lockoutDurationSeconds, BOUNDS.lockoutDurationSeconds), + backupCodeCount: clampInt(s.backupCodeCount, TWO_FACTOR_POLICY_DEFAULTS.backupCodeCount, BOUNDS.backupCodeCount), + } +} + +/** + * Read the policy from the plugin's settings document, once per isolate. + * + * Plugin settings are document-backed (`type_id = 'plugin'`, `slug = `) — the + * legacy `plugins` table does not exist on greenfield SonicJS v3. This reads the same place + * `PluginService.updatePluginSettings()` writes, so the admin form is not save-nowhere chrome. + */ +export async function loadTwoFactorPolicy(db: D1Database): Promise { + if (cached) return cached + let stored: unknown = {} + try { + const row = (await db + .prepare( + `SELECT data FROM documents + WHERE slug = ? AND type_id = 'plugin' AND tenant_id = 'default' + AND is_current_draft = 1 AND deleted_at IS NULL` + ) + .bind(TWO_FACTOR_PLUGIN_ID) + .first()) as { data: string | Record } | null + if (row?.data) { + const data = typeof row.data === 'string' ? JSON.parse(row.data) : row.data + stored = (data as { settings?: unknown })?.settings ?? {} + } + } catch (e) { + // Degrades to the shipped defaults — still inside BOUNDS, so the lockout stays enabled and + // backup codes stay >= 5. It does NOT preserve an operator's tighter override; see the + // staleness note in the module header. + console.warn('[two-factor] settings read failed; using policy defaults', e) + } + cached = normalizeTwoFactorPolicy(stored) + return cached +} + +/** + * The policy `getDefaultAuthOptions` composes from. Returns the defaults until + * {@link loadTwoFactorPolicy} has run in this isolate. + */ +export function getTwoFactorPolicy(): TwoFactorPolicy { + return cached ?? { ...TWO_FACTOR_POLICY_DEFAULTS } +} + +/** + * Drop the isolate cache. Prefer {@link refreshTwoFactorPolicy} on a settings write: the + * loader is once-guarded, so invalidating alone would leave this isolate on the DEFAULTS for + * the rest of its life rather than on the operator's saved values. + */ +export function invalidateTwoFactorPolicy(): void { + cached = null +} + +/** + * Re-read the policy after a settings write, so the change lands in the writing isolate + * immediately instead of waiting for it to recycle. Other warm isolates still pick it up on + * recycle — the same staleness contract as `isPluginActive`. + */ +export async function refreshTwoFactorPolicy(db: D1Database): Promise { + cached = null + return loadTwoFactorPolicy(db) +} diff --git a/packages/core/src/db/schema.ts b/packages/core/src/db/schema.ts index fb815316d..da11525ab 100644 --- a/packages/core/src/db/schema.ts +++ b/packages/core/src/db/schema.ts @@ -26,8 +26,16 @@ export const authUser = sqliteTable('auth_user', { // Account lockout: reset on success; set on threshold failures failedLoginCount: integer('failed_login_count').notNull().default(0), lockedUntil: integer('locked_until'), - // 2FA enrollment flag (twoFactor BA plugin) - twoFactorEnabled: integer('two_factor_enabled').notNull().default(0), + // 2FA enrollment flag, contributed to the USER model by BA's twoFactor plugin. + // `mode: 'boolean'` is required, not stylistic: the drizzle adapter leaves BA's + // `supportsBooleans` at its `true` default, so BA writes a JS `true`/`false` here and + // drizzle must be the thing that converts it to 1/0. Matches emailVerified/isSuperAdmin. + twoFactorEnabled: integer('two_factor_enabled', { mode: 'boolean' }).notNull().default(false), + // Set by an admin reset (migration 0004). Independent of twoFactorEnabled: `required && !enabled` + // is the state a reset leaves behind, and it forces the user to /admin/two-factor until they + // enrol again. Plain integer rather than `mode: 'boolean'` — Better Auth never writes this + // column, so nothing needs the boolean round-trip and the SQL reads/writes 0/1 directly. + twoFactorRequired: integer('two_factor_required').notNull().default(0), // timestamp_ms so Better Auth's Date values round-trip; matches SonicJS's // existing Date.now() (ms) convention for these columns. createdAt: integer('created_at', { mode: 'timestamp_ms' }).notNull(), @@ -81,6 +89,52 @@ export const authVerification = sqliteTable('auth_verification', { updatedAt: integer('updated_at', { mode: 'timestamp_ms' }).notNull(), }); +/** + * Better Auth `twoFactor` plugin storage — TOTP secret, single-use backup codes, and the + * per-account second-factor lockout counters. + * + * Property keys are deliberately the BA field names (`userId`, `backupCodes`, + * `failedVerificationCount`, `lockedUntil`). The drizzle adapter maps a BA field to a + * drizzle **property key**, not to a column name, so matching them here means the plugin + * needs no `schema.twoFactor.fields` map at all — and adding one would double-map. + * + * Registered in the BA d1 schema map under the key `auth_two_factor`, which must equal the + * `twoFactorTable` modelName (see auth/config.ts). + */ +export const authTwoFactor = sqliteTable('auth_two_factor', { + id: text('id').primaryKey(), + // Encrypted with the Better Auth secret; BA never returns it from an endpoint. + secret: text('secret').notNull(), + backupCodes: text('backup_codes').notNull(), + userId: text('user_id') + .notNull() + .references(() => authUser.id, { onDelete: 'cascade' }), + // BA defaults this true. It is false only between POST /two-factor/enable and the first + // successful /two-factor/verify-totp — the window where enrolment has started but has not + // been proven against a live code. + verified: integer('verified', { mode: 'boolean' }).notNull().default(true), + // See migration 0003: NOT NULL DEFAULT 0 keeps BA's `count = count + 1` bump from + // evaluating to NULL, which would make the lockout silently unreachable. + failedVerificationCount: integer('failed_verification_count').notNull().default(0), + // timestamp_ms, matching auth_session.expires_at. BA hands the drizzle adapter a real + // `Date` (supportsDates defaults to true), so the mode is what round-trips it. + lockedUntil: integer('locked_until', { mode: 'timestamp_ms' }), + // `$defaultFn` is load-bearing, not tidiness. Better Auth adds createdAt/updatedAt to its four + // CORE models only — `getAuthTables()` spreads PLUGIN tables verbatim + // (@better-auth/core/dist/db/get-tables.mjs) — and the twoFactor schema declares neither. So + // `POST /two-factor/enable` creates a row with no timestamps, drizzle emits explicit NULLs for + // absent notNull columns, and the INSERT dies on `NOT NULL constraint failed: + // auth_two_factor.created_at`. Enrolment would 500 for every user, and the page would report + // it as a wrong password. Defaulting here (rather than in SQL) also repairs already-migrated + // databases, because an ALTER cannot retro-add a default. + // Covered by __tests__/services/two-factor-adapter-create.test.ts. + createdAt: integer('created_at', { mode: 'timestamp_ms' }).notNull().$defaultFn(() => new Date()), + updatedAt: integer('updated_at', { mode: 'timestamp_ms' }) + .notNull() + .$defaultFn(() => new Date()) + .$onUpdateFn(() => new Date()), +}); + // BA internal resolution aliases export const session = authSession; export const account = authAccount; diff --git a/packages/core/src/middleware/plugin-menu.ts b/packages/core/src/middleware/plugin-menu.ts index 4b2e6d103..5c8445d06 100644 --- a/packages/core/src/middleware/plugin-menu.ts +++ b/packages/core/src/middleware/plugin-menu.ts @@ -33,8 +33,24 @@ const ICON_SVG: Record = { 'pencil-square': '', 'server': '', 'building-office': '', + 'lock-closed': '', + 'book-open': '', + 'variable': '', + 'bolt': '', } +/** Rendered when a menu entry supplies no icon, or a name this map does not know. */ +const FALLBACK_ICON = `` + +/** + * Resolve a menu entry's `icon` to SVG markup. + * + * Returns `''` for a name this map does not know — callers MUST substitute {@link FALLBACK_ICON} + * rather than falling back to the raw input. Passing the name through renders it as literal text + * in the sidebar ("lock-closed" appearing where the icon belongs), because the catalyst layout + * interpolates this value as markup. That is a rendering bug the type system cannot catch: both + * the resolved SVG and the unresolved name are `string`. + */ function resolveIcon(iconName?: string): string { if (!iconName) return '' // If it's already SVG markup, return as-is @@ -47,8 +63,7 @@ const MARKER = '' function renderMenuItem(item: { label: string; path: string; icon?: string }, currentPath: string): string { const isActive = currentPath === item.path || currentPath.startsWith(item.path) - const fallbackIcon = `` - const resolvedIcon = resolveIcon(item.icon) || fallbackIcon + const resolvedIcon = resolveIcon(item.icon) || FALLBACK_ICON return ` ${isActive ? '' : ''} @@ -140,7 +155,11 @@ export function pluginMenuMiddleware() { } activeMenuItems.sort((a, b) => a.order - b.order) - c.set('pluginMenuItems', activeMenuItems.map(m => ({ label: m.label, path: m.path, icon: resolveIcon(m.icon) || m.icon || '' }))) + // NOT `|| m.icon` — that passed an UNRESOLVED icon name through as if it were markup, and the + // catalyst layout interpolates this straight into the sidebar, so every plugin whose manifest + // named an icon absent from ICON_SVG rendered the literal string ("lock-closed", "book-open") + // where its icon should be. + c.set('pluginMenuItems', activeMenuItems.map(m => ({ label: m.label, path: m.path, icon: resolveIcon(m.icon) || FALLBACK_ICON }))) await next() diff --git a/packages/core/src/plugins/core-plugins/index.ts b/packages/core/src/plugins/core-plugins/index.ts index 14c566258..b8f318222 100644 --- a/packages/core/src/plugins/core-plugins/index.ts +++ b/packages/core/src/plugins/core-plugins/index.ts @@ -46,6 +46,7 @@ export { versioningPlugin, createVersioningPlugin } from './versioning-plugin' export { mcpPlugin, createMcpPlugin } from './mcp-plugin' export type { McpConfigInput, McpConfig } from './mcp-plugin' export { menuPlugin, createMenuPlugin } from './menu-plugin' +export { twoFactorAuthPlugin, createTwoFactorAuthPlugin } from './two-factor-auth' // Core plugins list - now imported from auto-generated registry export const CORE_PLUGIN_IDS = [ @@ -70,6 +71,7 @@ export const CORE_PLUGIN_IDS = [ 'multi-tenant', 'versioning', 'menu', + 'two-factor-auth', ] as const export type CorePluginNames = (typeof CORE_PLUGIN_IDS)[number] diff --git a/packages/core/src/plugins/core-plugins/oauth-providers/index.ts b/packages/core/src/plugins/core-plugins/oauth-providers/index.ts index 24c9f605f..77942e66c 100644 --- a/packages/core/src/plugins/core-plugins/oauth-providers/index.ts +++ b/packages/core/src/plugins/core-plugins/oauth-providers/index.ts @@ -23,6 +23,7 @@ import { } from './oauth-service' import { AuthManager } from '../../../middleware' import { getJwtExpirySecondsFromDb } from '../../../middleware/auth' +import { hasVerifiedSecondFactor } from '../../../auth/second-factor-guard' const STATE_COOKIE_NAME = 'oauth_state' const STATE_COOKIE_MAX_AGE = 600 // 10 minutes @@ -220,6 +221,23 @@ function buildOauthApi(): Hono { return c.redirect('/auth/login?error=Account is deactivated') } + // Second-factor gate. This branch auto-links a provider identity to a pre-existing LOCAL + // account matched only by email address, then mints a session without Better Auth — so + // BA's second-factor challenge never runs, and an attacker who controls any provider + // account bearing the victim's email address would bypass a second factor the victim + // deliberately enrolled in. Unlike a provider the user linked themselves (handled above, + // where the provider is trusted to have done its own MFA), nothing here was ever + // confirmed by the account owner. + // + // This route is mounted ahead of the /auth/* catch-all in app.ts, so + // guardPasswordlessSecondFactor never sees it; the check has to be here. + if (await hasVerifiedSecondFactor((c.env as any).DB, existingUser.id)) { + return c.redirect( + '/auth/login?error=' + + encodeURIComponent('This account uses two-factor authentication. Sign in with your password, then enter your authenticator code.') + ) + } + // Link OAuth to existing account await oauthService.createOAuthAccount({ userId: existingUser.id, diff --git a/packages/core/src/plugins/core-plugins/otp-login-plugin/index.ts b/packages/core/src/plugins/core-plugins/otp-login-plugin/index.ts index 30845c9b5..dd3452b2d 100644 --- a/packages/core/src/plugins/core-plugins/otp-login-plugin/index.ts +++ b/packages/core/src/plugins/core-plugins/otp-login-plugin/index.ts @@ -15,6 +15,7 @@ import { renderOTPEmail } from './email-templates' import { AuthManager } from '../../../middleware' import { getEmailService, hasEmailService } from '../../../services/email/email-service-singleton' import { getJwtExpirySecondsFromDb } from '../../../middleware/auth' +import { hasVerifiedSecondFactor } from '../../../auth/second-factor-guard' import { SettingsService } from '../../../services/settings' import { getCustomData } from '../user-profiles' import { dispatchHookEvent } from '../../hooks/dispatch-event' @@ -285,6 +286,20 @@ function buildOtpApi(): Hono { }, 403) } + // Second-factor gate. This route mints a session WITHOUT Better Auth, so BA's second-factor + // challenge never runs here — which would leave an emailed code as a complete bypass of a + // second factor the user deliberately enrolled in, while /admin/profile still read + // "Enabled". It also sits ahead of the /auth/* catch-all in app.ts, so + // guardPasswordlessSecondFactor never sees it; the check has to be here. + // + // Deliberately placed AFTER the code has been consumed, so a refused attempt also spends + // the code — otherwise this becomes an oracle for probing which accounts have 2FA. + if (await hasVerifiedSecondFactor(db, user.id)) { + return c.json({ + error: 'This account uses two-factor authentication. Sign in with your password, then enter your authenticator code.' + }, 403) + } + // Generate JWT token const tokenTtl = await getJwtExpirySecondsFromDb(db, c.env as any) const token = await AuthManager.generateToken(user.id, user.email, user.role, (c.env as any).JWT_SECRET, tokenTtl) diff --git a/packages/core/src/plugins/core-plugins/security-audit-plugin/types.ts b/packages/core/src/plugins/core-plugins/security-audit-plugin/types.ts index 6fd7797a0..e3cdc182a 100644 --- a/packages/core/src/plugins/core-plugins/security-audit-plugin/types.ts +++ b/packages/core/src/plugins/core-plugins/security-audit-plugin/types.ts @@ -8,6 +8,11 @@ export type SecurityEventType = | 'suspicious_activity' | 'logout' | 'permission_denied' + // An admin cleared another account's second factor (two-factor-auth plugin's break-glass path). + // Its own type rather than 'suspicious_activity' because it is a legitimate, expected action + // that nonetheless removes a security control from an account other than the actor's own — + // exactly the shape that needs to be findable after the fact. + | 'two_factor_reset' export type SecuritySeverity = 'info' | 'warning' | 'critical' diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/admin-gate.test.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/admin-gate.test.ts new file mode 100644 index 000000000..626f73d72 --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/admin-gate.test.ts @@ -0,0 +1,254 @@ +/** + * Gate tests for the plugin's two mounts, against a real (SQLite) D1 and live requests through + * the actual sub-apps. + * + * The two mounts are deliberately asymmetric, and that asymmetry is the thing most likely to be + * "tidied up" by a later reader — so both directions are pinned here: + * + * /admin/two-factor → requireAuth + deactivate→404 + * /auth/two-factor → NO auth, NO plugin gate (a gated challenge page would strand every + * enrolled user mid-login, because BA has already deleted the session + * cookie by the time the challenge is issued) + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { Hono } from 'hono' +import { twoFactorAdminRoutes, twoFactorChallengeRoutes } from '../routes' +import { TWO_FACTOR_PLUGIN_ID } from '../../../../auth/two-factor-settings' +import { invalidatePluginStatusCache } from '../../../../middleware/plugin-middleware' +import { createTestD1, type TestD1 } from '../../../../__tests__/utils/d1-sqlite' + +let db: TestD1 + +const ADMIN = { userId: 'u1', email: 'admin@test.local', role: 'admin' } +const VIEWER = { userId: 'u2', email: 'viewer@test.local', role: 'viewer' } + +function seedStatus(status: 'active' | 'inactive') { + db.raw + .prepare( + `INSERT INTO documents (id, root_id, type_id, slug, tenant_id, is_current_draft, data) + VALUES (?, ?, 'plugin', ?, 'default', 1, ?)`, + ) + .run('doc-2fa', 'root-2fa', TWO_FACTOR_PLUGIN_ID, JSON.stringify({ status })) +} + +function seedEnrolment(userId: string, verified: 0 | 1) { + db.raw + .prepare( + `INSERT INTO auth_two_factor (id, secret, backup_codes, user_id, verified, created_at, updated_at) + VALUES (?, 'enc', 'enc', ?, ?, 0, 0)`, + ) + .run(`tf-${userId}`, userId, verified) +} + +function request( + mount: 'admin' | 'challenge', + path: string, + opts: { user?: typeof ADMIN; accept?: string } = {}, +) { + const app = new Hono<{ Bindings: { DB: unknown }; Variables: { user?: typeof ADMIN } }>() + app.use('*', async (c, next) => { + if (opts.user) c.set('user', opts.user) + await next() + }) + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- test env binding + const routes = (mount === 'admin' ? twoFactorAdminRoutes : twoFactorChallengeRoutes) as any + app.route(mount === 'admin' ? '/admin/two-factor' : '/auth/two-factor', routes) + return app.request(path, { headers: opts.accept ? { Accept: opts.accept } : {} }, { DB: db }) +} + +/** POST through the challenge mount, returning the response plus its Set-Cookie list. */ +async function requestPost( + path: string, + opts: { user?: typeof ADMIN; accept?: string } = {}, + body: Record = {}, +) { + const app = new Hono<{ Bindings: { DB: unknown }; Variables: { user?: typeof ADMIN } }>() + app.use('*', async (c, next) => { + if (opts.user) c.set('user', opts.user) + await next() + }) + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- test env binding + app.route('/auth/two-factor', twoFactorChallengeRoutes as any) + const res = await app.request( + path, + { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...(opts.accept ? { Accept: opts.accept } : {}) }, + body: JSON.stringify(body), + }, + { DB: db, JWT_SECRET: 'test-secret-value-32-chars-long!!', ENVIRONMENT: 'development' }, + ) + const cookies = (res.headers as unknown as { getSetCookie?: () => string[] }).getSetCookie?.() ?? [] + return { status: res.status, cookies } +} + +beforeEach(() => { + db = createTestD1() + invalidatePluginStatusCache(TWO_FACTOR_PLUGIN_ID) +}) + +afterEach(() => { + db.close() + invalidatePluginStatusCache(TWO_FACTOR_PLUGIN_ID) +}) + +describe('two-factor enrolment page gate (/admin/two-factor)', () => { + it('404s when the plugin has no row at all', async () => { + const res = await request('admin', '/admin/two-factor', { user: ADMIN }) + expect(res.status).toBe(404) + }) + + it('404s when the plugin row is explicitly inactive', async () => { + seedStatus('inactive') + const res = await request('admin', '/admin/two-factor', { user: ADMIN }) + expect(res.status).toBe(404) + }) + + it('401s an unauthenticated request — the auth gate runs before the plugin gate', async () => { + seedStatus('active') + const res = await request('admin', '/admin/two-factor', { accept: 'application/json' }) + expect(res.status).toBe(401) + expect(await res.json()).toMatchObject({ error: 'Authentication required' }) + }) + + it('serves the page to any authenticated role — enrolment is self-service, not admin-only', async () => { + seedStatus('active') + const res = await request('admin', '/admin/two-factor', { user: VIEWER }) + expect(res.status).toBe(200) + expect(await res.text()).toContain('Two-Factor Authentication') + }) + + it('renders the "off" state with the setup form when there is no enrolment', async () => { + seedStatus('active') + const html = await (await request('admin', '/admin/two-factor', { user: ADMIN })).text() + expect(html).toContain('id="enrolForm"') + expect(html).toContain('>Disabled<') + // The "on" panel exists in the DOM but must start hidden. + expect(html).toMatch(/id="state-on" class="hidden/) + }) + + it('renders the "started, not confirmed" state for verified = 0, and says what to do', async () => { + // The status label alone would leave a user staring at a "Begin setup" button with no + // indication that pressing it discards the half-finished enrolment (BA's /enable deletes and + // recreates the row), or that 2FA is not actually protecting them yet. + seedStatus('active') + seedEnrolment(ADMIN.userId, 0) + const html = await (await request('admin', '/admin/two-factor', { user: ADMIN })).text() + expect(html).toContain('Started, not confirmed') + expect(html).toMatch(/never confirmed with a live code/) + expect(html).toMatch(/discards the unconfirmed one/) + }) + + it('renders the "on" state for verified = 1, with the setup form hidden', async () => { + seedStatus('active') + seedEnrolment(ADMIN.userId, 1) + const html = await (await request('admin', '/admin/two-factor', { user: ADMIN })).text() + expect(html).toContain('>Enabled<') + expect(html).toMatch(/id="state-off" class="hidden/) + expect(html).toContain('id="disableForm"') + }) + + it('does not leak another user\'s enrolment into this user\'s page', async () => { + seedStatus('active') + seedEnrolment(VIEWER.userId, 1) + const html = await (await request('admin', '/admin/two-factor', { user: ADMIN })).text() + expect(html).toContain('>Disabled<') + }) + + it('tells the user that enrolling disables the emailed sign-in paths', async () => { + // Not decoration: passwordless-second-factor-guard.ts really does refuse magic links for + // enrolled accounts, and a user who found that out by having links stop arriving would + // reasonably file it as a bug. + seedStatus('active') + const html = await (await request('admin', '/admin/two-factor', { user: ADMIN })).text() + expect(html).toMatch(/magic links and emailed sign-in codes are\s+disabled/) + }) +}) + +describe('two-factor challenge page (/auth/two-factor)', () => { + it('serves unauthenticated — the caller has no session by construction', async () => { + const res = await request('challenge', '/auth/two-factor') + expect(res.status).toBe(200) + expect(await res.text()).toContain('Two-step verification') + }) + + it('serves even when the plugin is deactivated, so enrolled users are never stranded', async () => { + seedStatus('inactive') + const res = await request('challenge', '/auth/two-factor') + expect(res.status).toBe(200) + }) + + it('offers backup-code entry unconditionally', async () => { + // BA's twoFactorMethods only ever contains 'totp'/'otp' — never 'backup_code'. Keying the + // UI off that list would hide this form exactly when the device is lost. + const html = await (await request('challenge', '/auth/two-factor')).text() + expect(html).toContain('id="backupForm"') + expect(html).toContain('verify-backup-code') + }) + + it('posts to the Better Auth mount, not to /api/auth (the sibling port\'s basePath)', async () => { + const html = await (await request('challenge', '/auth/two-factor')).text() + expect(html).toContain("'/auth/two-factor/' + path") + expect(html).not.toContain('/api/auth/') + }) + + it('sends the CSRF double-submit header on every verify POST', async () => { + const html = await (await request('challenge', '/auth/two-factor')).text() + expect(html).toContain("'X-CSRF-Token': csrf()") + }) + + it('upgrades the session via /complete before navigating', async () => { + // Without this call the post-2FA browser holds only better-auth.session_token, and + // csrfProtection exempts any request with no auth_token cookie — so CSRF validation would be + // silently off for the whole session of every 2FA user. + const html = await (await request('challenge', '/auth/two-factor')).text() + expect(html).toContain("post('complete', {})") + expect(html.indexOf("post('complete', {})")).toBeLessThan(html.indexOf("window.location.href = '/admin/content'")) + }) +}) + +describe('POST /auth/two-factor/complete', () => { + it('mints a Strict auth_token for a user who holds a verified second factor', async () => { + seedEnrolment(ADMIN.userId, 1) + const res = await requestPost('/auth/two-factor/complete', { user: ADMIN }) + expect(res.status).toBe(200) + const setCookie = res.cookies.find((c) => c.startsWith('auth_token=')) + expect(setCookie).toBeTruthy() + expect(setCookie).toContain('HttpOnly') + expect(setCookie).toContain('SameSite=Strict') + }) + + it('401s an unauthenticated caller', async () => { + const res = await requestPost('/auth/two-factor/complete', { accept: 'application/json' }) + expect(res.status).toBe(401) + expect(res.cookies.some((c) => c.startsWith('auth_token='))).toBe(false) + }) + + it('refuses a user with no verified second factor — nothing was challenged', async () => { + const res = await requestPost('/auth/two-factor/complete', { user: ADMIN }) + expect(res.status).toBe(400) + expect(res.cookies.some((c) => c.startsWith('auth_token='))).toBe(false) + }) + + it('refuses a user whose enrolment is unconfirmed', async () => { + seedEnrolment(ADMIN.userId, 0) + const res = await requestPost('/auth/two-factor/complete', { user: ADMIN }) + expect(res.status).toBe(400) + }) + + it('derives the JWT subject from the session, never from the request body', async () => { + seedEnrolment(ADMIN.userId, 1) + const res = await requestPost( + '/auth/two-factor/complete', + { user: ADMIN }, + { userId: 'someone-else', role: 'admin' }, + ) + expect(res.status).toBe(200) + const token = res.cookies.find((c) => c.startsWith('auth_token='))!.split('=')[1]!.split(';')[0]! + const payload = JSON.parse( + Buffer.from(token.split('.')[1]!.replace(/-/g, '+').replace(/_/g, '/'), 'base64').toString(), + ) as { userId: string; email: string } + expect(payload.userId).toBe(ADMIN.userId) + expect(payload.email).toBe(ADMIN.email) + }) +}) diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/enrolment-page-required.test.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/enrolment-page-required.test.ts new file mode 100644 index 000000000..5e77be304 --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/enrolment-page-required.test.ts @@ -0,0 +1,61 @@ +/** + * How the enrolment page renders under `auth_user.two_factor_required`. + * + * The flag drives TWO different conditions and conflating them is the easy mistake: + * - `required && !verified` → the amber "set up to continue" banner + * - `required` → the disable form is replaced by an explanation + * + * A user who has satisfied the requirement is `required && verified`: no banner (they owe + * nothing), but still no disable form (they may not turn it off). + */ +import { describe, it, expect } from 'vitest' +import { renderTwoFactorEnrolmentPage } from '../components/enrolment-page' + +const BANNER = 'Set up two-factor to continue' +const LOCKED = 'Required by an administrator' +const DISABLE_FORM = 'id="disableForm"' + +function render(over: Partial[0]> = {}) { + return renderTwoFactorEnrolmentPage({ verified: false, pending: false, ...over }) +} + +describe('enrolment page under a two-factor requirement', () => { + it('offers the disable form on an ordinary enrolled account', () => { + const html = render({ verified: true }) + expect(html).toContain(DISABLE_FORM) + expect(html).not.toContain(LOCKED) + expect(html).not.toContain(BANNER) + }) + + it('replaces the disable form with an explanation once 2FA is required', () => { + const html = render({ verified: true, required: true }) + expect(html).not.toContain(DISABLE_FORM) + expect(html).toContain(LOCKED) + }) + + it('shows no banner to a required user who HAS enrolled — they owe nothing', () => { + // Getting this wrong would tell someone with a working second factor that they need to set + // one up, on every page load, forever. + expect(render({ verified: true, required: true })).not.toContain(BANNER) + }) + + it('shows the banner to a required user who has NOT enrolled', () => { + const html = render({ verified: false, required: true }) + expect(html).toContain(BANNER) + expect(html).toContain('An administrator reset the two-factor authentication on your account') + }) + + it('never renders the banner without the flag, whatever the enrolment state', () => { + expect(render({ verified: false })).not.toContain(BANNER) + expect(render({ verified: false, pending: true })).not.toContain(BANNER) + expect(render({ verified: true })).not.toContain(BANNER) + }) + + it('guards the disable binding so the script survives the form being absent', () => { + // One IIFE holds the enrol, verify and disable handlers. An unguarded + // `$('disableForm').addEventListener` throws on null when the form is replaced, taking + // enrolment down with it — for exactly the users who are being forced to enrol. + const html = render({ verified: true, required: true }) + expect(html).toContain("if ($('disableForm'))") + }) +}) diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/qr.test.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/qr.test.ts new file mode 100644 index 000000000..ba4e6fc04 --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/qr.test.ts @@ -0,0 +1,183 @@ +/** + * `POST /admin/two-factor/qr` — the QR renderer the enrolment page calls after Better Auth + * returns an `otpauth://` URI. + * + * This exists because the page originally shipped no QR at all, which made desktop enrolment + * effectively impossible: the admin panel is on a laptop, the authenticator is on a phone, and an + * `otpauth://` link has nothing to open. The tests below pin the two things that matter — that it + * really produces a scannable SVG encoding the URI, and that it cannot be used to render + * arbitrary content as a QR from this origin. + */ +import { describe, it, expect, vi } from 'vitest' +import { Hono } from 'hono' + +// The route module pulls requireAuth and the plugin-active gate from the middleware barrel; both +// are exercised in admin-gate.test.ts. Here they are pass-throughs so the QR logic is isolated. +vi.mock('../../../../middleware', async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + requireAuth: () => async (_c: unknown, next: () => Promise) => next(), + } +}) +vi.mock('../../../../middleware/plugin-middleware', () => ({ + isPluginActive: async () => true, + invalidatePluginStatusCache: () => {}, +})) + +const { twoFactorAdminRoutes } = await import('../routes') + +const VALID_URI = + 'otpauth://totp/SonicJS:admin@sonicjs.com?secret=JBSWY3DPEHPK3PXP&issuer=SonicJS&algorithm=SHA1&digits=6&period=30' + +function makeApp() { + const app = new Hono() + app.use('*', async (c, next) => { + c.env = { DB: {} } as never + c.set('user' as never, { userId: 'u1', email: 'a@test.local', role: 'admin' } as never) + await next() + }) + app.route('/admin/two-factor', twoFactorAdminRoutes) + return app +} + +function postQr(uri: unknown) { + return makeApp().request('/admin/two-factor/qr', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ uri }), + }) +} + +describe('POST /admin/two-factor/qr', () => { + it('returns an inline SVG for a TOTP enrolment URI', async () => { + const res = await postQr(VALID_URI) + expect(res.status).toBe(200) + const { svg } = (await res.json()) as { svg: string } + + expect(svg.startsWith(' { + // Two different secrets must not produce the same image; that is the cheapest available + // proof that the content actually reaches the encoder rather than a constant being returned. + const a = (await (await postQr(VALID_URI)).json()) as { svg: string } + const b = (await ( + await postQr(VALID_URI.replace('JBSWY3DPEHPK3PXP', 'MFRGGZDFMZTWQ2LK')) + ).json()) as { svg: string } + expect(a.svg).not.toEqual(b.svg) + }) + + it('refuses anything that is not an otpauth://totp/ URI', async () => { + // Otherwise this is a "render any text as a QR, from your origin" service — a phishing + // primitive, because a QR is unreadable to the human deciding whether to trust it. + for (const bad of [ + 'https://evil.test/steal', + 'otpauth://hotp/SonicJS:a@b.c?secret=X', + 'javascript:alert(1)', + '', + 42, + null, + { uri: VALID_URI }, + ]) { + const res = await postQr(bad) + expect(res.status, `accepted ${JSON.stringify(bad)}`).toBe(400) + } + }) + + it('refuses an over-long URI rather than encoding an unbounded payload', async () => { + const res = await postQr(`${VALID_URI}&pad=${'x'.repeat(600)}`) + expect(res.status).toBe(400) + }) + + it('refuses a request with no JSON body at all', async () => { + const res = await makeApp().request('/admin/two-factor/qr', { method: 'POST' }) + expect(res.status).toBe(400) + }) +}) + +/** + * ── Scannability ── + * + * Everything above passes whether or not the QR can actually be read, which is how a version that + * rendered at 3.1 CSS px/module shipped green: the SVG is well-formed, encodes the URI, and refuses + * bad input either way. Scannability is a property of CSS pixels PER MODULE, and the module count + * grows with the URI — `issuer` is operator-configurable to 64 chars and Better Auth writes it into + * the URI twice, so a long issuer plus a long email is a 69-module symbol where the shipped default + * is 49. + * + * These tests pin the density directly, so a fixed size can never be reintroduced without failing. + */ +describe('POST /admin/two-factor/qr — scannability', () => { + /** The floor below which phone cameras stop reliably decoding a screen. */ + const MIN_CSS_PX_PER_MODULE = 4 + + function uriFor(issuer: string, email: string) { + return ( + `otpauth://totp/${encodeURIComponent(`${issuer}:${email}`)}` + + `?secret=JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP&issuer=${encodeURIComponent(issuer)}` + + `&algorithm=SHA1&digits=6&period=30` + ) + } + + const CASES = [ + { name: 'shipped default issuer', uri: uriFor('SonicJS', 'admin@sonicjs.com') }, + { name: 'max-length issuer', uri: uriFor('A'.repeat(64), 'admin@sonicjs.com') }, + { + name: 'max-length issuer + long email', + uri: uriFor('A'.repeat(64), 'marketing.operations@some-long-company-name.example.com'), + }, + ] + + async function render(uri: string) { + const res = await postQr(uri) + expect(res.status, `rejected a legitimate URI of ${uri.length} chars`).toBe(200) + return (await res.json()) as { svg: string; renderPx: number; modulesAcross: number } + } + + it.each(CASES)('stays above the scannable floor — $name', async ({ uri }) => { + const { renderPx, modulesAcross } = await render(uri) + expect(renderPx / modulesAcross).toBeGreaterThanOrEqual(MIN_CSS_PX_PER_MODULE) + }) + + it('sizes the element itself, not just the container', async () => { + // A viewBox-only is sized entirely by its parent, which is what made the density an + // invisible CSS detail. The painted size has to be on the element. + const { svg, renderPx } = await render(CASES[0]!.uri) + const openTag = svg.match(/]*>/)![0] + expect(openTag).toContain(`width="${renderPx}"`) + expect(openTag).toContain(`height="${renderPx}"`) + // The viewBox must survive alongside them, or the scale is no longer uniform. + expect(openTag).toMatch(/viewBox="0 0 \d+ \d+"/) + }) + + it('grows the rendered size as the symbol grows', async () => { + // The invariant a hardcoded width violates: a denser symbol must come back physically larger. + const [small, medium, large] = await Promise.all(CASES.map((c) => render(c.uri))) + expect(medium!.modulesAcross).toBeGreaterThan(small!.modulesAcross) + expect(large!.modulesAcross).toBeGreaterThan(medium!.modulesAcross) + expect(medium!.renderPx).toBeGreaterThan(small!.renderPx) + expect(large!.renderPx).toBeGreaterThan(medium!.renderPx) + }) + + it('keeps a 4-module quiet zone at every symbol size', async () => { + // Measured in viewBox units off the module path, not assumed from the option we passed: + // `padding` is in modules, so the border scales with the symbol and a regression here is + // exactly the "won't scan on some phones" class of bug. + for (const { uri, name } of CASES) { + const { svg, modulesAcross } = await render(uri) + const viewBox = Number(svg.match(/viewBox="0 0 (\d+)/)![1]) + const unitsPerModule = viewBox / modulesAcross + const path = svg.match(/]*d="([^"]+)"/)![1]! + const xs = [...path.matchAll(/M(-?[\d.]+)/g)].map((m) => Number(m[1])) + expect(Math.min(...xs) / unitsPerModule, `quiet zone wrong for ${name}`).toBeCloseTo(4, 1) + } + }) +}) diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/recovery.sqlite.test.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/recovery.sqlite.test.ts new file mode 100644 index 000000000..d81dfc200 --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/recovery.sqlite.test.ts @@ -0,0 +1,435 @@ +/** + * Administrative two-factor reset, against a real (SQLite) D1 and live requests through the + * actual sub-app. + * + * This is the break-glass path — the only thing standing between a lost phone and a Cloudflare + * credentials incident — so the tests here are deliberately about SQL effects and gate behavior + * rather than shapes. A mock DB would pass every one of them while the UPDATE silently touched + * no rows (R10). + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { Hono } from 'hono' +import { + twoFactorRecoveryRoutes, + resetUserTwoFactor, + owesTwoFactorEnrolment, + enforceTwoFactorEnrolment, + guardRequiredSecondFactorDisable, + isTwoFactorRequired, + ENROLMENT_PATH, +} from '../recovery' +import { TWO_FACTOR_PLUGIN_ID } from '../../../../auth/two-factor-settings' +import { invalidatePluginStatusCache } from '../../../../middleware/plugin-middleware' +import { ensureTwoFactorRequiredColumn } from '../../../../services/migrations' +import { createTestD1, type TestD1 } from '../../../../__tests__/utils/d1-sqlite' + +let db: TestD1 + +const ADMIN = { userId: 'admin-1', email: 'admin@test.local', role: 'admin' } +const EDITOR = { userId: 'editor-1', email: 'editor@test.local', role: 'editor' } +const TARGET = { userId: 'user-1', email: 'locked.out@test.local', role: 'editor' } + +function seedUser(u: { userId: string; email: string; role: string }, twoFactorEnabled = 0) { + db.raw + .prepare( + `INSERT INTO auth_user (id, email, email_verified, created_at, updated_at, + first_name, last_name, role, two_factor_enabled) + VALUES (?, ?, 1, 0, 0, 'Test', 'User', ?, ?)`, + ) + .run(u.userId, u.email, u.role, twoFactorEnabled) +} + +/** A completed enrolment, plus a live lockout — the state a locked-out user is actually in. */ +function seedEnrolment(userId: string, verified: 0 | 1 = 1, lockedUntil: number | null = null) { + db.raw + .prepare( + `INSERT INTO auth_two_factor (id, secret, backup_codes, user_id, verified, created_at, + updated_at, failed_verification_count, locked_until) + VALUES (?, 'enc', 'enc', ?, ?, 0, 0, 5, ?)`, + ) + .run(`tf-${userId}`, userId, verified, lockedUntil) +} + +function seedPluginStatus(status: 'active' | 'inactive') { + db.raw + .prepare( + `INSERT INTO documents (id, root_id, type_id, slug, tenant_id, is_current_draft, data) + VALUES (?, ?, 'plugin', ?, 'default', 1, ?)`, + ) + .run('doc-2fa', 'root-2fa', TWO_FACTOR_PLUGIN_ID, JSON.stringify({ status })) +} + +function readUser(userId: string) { + return db.raw + .prepare(`SELECT two_factor_enabled AS enabled, two_factor_required AS required + FROM auth_user WHERE id = ?`) + .get(userId) as { enabled: number; required: number } | undefined +} + +/** POST through the real sub-app with `user` pre-set, the way requireAuth would leave it. */ +function postReset(body: unknown, user?: typeof ADMIN) { + const app = new Hono<{ Bindings: { DB: unknown }; Variables: { user?: typeof ADMIN } }>() + app.use('*', async (c, next) => { + if (user) c.set('user', user) + await next() + }) + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- test env binding + app.route('/admin/two-factor-reset', twoFactorRecoveryRoutes as any) + return app.request( + '/admin/two-factor-reset', + { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }, + { DB: db }, + ) +} + +/** Drive the enforcement middleware in front of a trivial handler. */ +function requestGuarded(path: string, user?: typeof ADMIN, accept = 'text/html') { + const app = new Hono<{ Bindings: { DB: unknown }; Variables: { user?: typeof ADMIN } }>() + app.use('*', async (c, next) => { + if (user) c.set('user', user) + await next() + }) + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- test env binding + app.use('/admin/*', enforceTwoFactorEnrolment() as any) + app.get('/admin/*', (c) => c.text('reached the handler')) + return app.request(path, { headers: { Accept: accept } }, { DB: db }) +} + +beforeEach(() => { + db = createTestD1() + invalidatePluginStatusCache(TWO_FACTOR_PLUGIN_ID) + seedUser(ADMIN) + seedUser(EDITOR) +}) + +afterEach(() => { + db.close() + invalidatePluginStatusCache(TWO_FACTOR_PLUGIN_ID) +}) + +describe('resetUserTwoFactor — the SQL effect', () => { + beforeEach(() => { + seedUser(TARGET, 1) + seedEnrolment(TARGET.userId, 1, Date.now() + 900_000) + }) + + it('destroys the enrolment and clears the user flag together', async () => { + await resetUserTwoFactor(db as never, TARGET.userId, true) + + const enrolment = db.raw + .prepare(`SELECT 1 FROM auth_two_factor WHERE user_id = ?`) + .get(TARGET.userId) + expect(enrolment).toBeUndefined() + // Both halves, because either one alone is a broken state: an orphaned enabled=1 makes the + // UI claim a protection that no longer exists. + expect(readUser(TARGET.userId)).toEqual({ enabled: 0, required: 1 }) + }) + + it('clears an active lockout as a side effect of deleting the row', async () => { + // failed_verification_count and locked_until live on auth_two_factor (migration 0003), so + // the DELETE is also the lockout fix. If those columns ever move to auth_user, this test is + // what should fail. + await resetUserTwoFactor(db as never, TARGET.userId, true) + const locked = db.raw + .prepare(`SELECT locked_until FROM auth_two_factor WHERE user_id = ?`) + .get(TARGET.userId) + expect(locked).toBeUndefined() + }) + + it('leaves the account password-only when re-enrolment is not demanded', async () => { + await resetUserTwoFactor(db as never, TARGET.userId, false) + expect(readUser(TARGET.userId)).toEqual({ enabled: 0, required: 0 }) + }) + + it('touches nobody else', async () => { + seedUser({ userId: 'bystander', email: 'bystander@test.local', role: 'editor' }, 1) + seedEnrolment('bystander', 1) + + await resetUserTwoFactor(db as never, TARGET.userId, true) + + expect(readUser('bystander')).toEqual({ enabled: 1, required: 0 }) + expect( + db.raw.prepare(`SELECT 1 FROM auth_two_factor WHERE user_id = 'bystander'`).get(), + ).toBeDefined() + }) +}) + +describe('POST /admin/two-factor-reset', () => { + beforeEach(() => { + seedUser(TARGET, 1) + seedEnrolment(TARGET.userId, 1) + }) + + it('resets when an admin confirms the target email', async () => { + const res = await postReset( + { userId: TARGET.userId, confirmEmail: TARGET.email }, + ADMIN, + ) + expect(res.status).toBe(200) + await expect(res.json()).resolves.toMatchObject({ + ok: true, + email: TARGET.email, + wasEnrolled: true, + // Defaults on, because a reset with no follow-up silently downgrades the account. + requireReenrolment: true, + }) + expect(readUser(TARGET.userId)).toEqual({ enabled: 0, required: 1 }) + }) + + it('accepts the confirmation regardless of case or surrounding space', async () => { + const res = await postReset( + { userId: TARGET.userId, confirmEmail: ` ${TARGET.email.toUpperCase()} ` }, + ADMIN, + ) + expect(res.status).toBe(200) + }) + + it('changes nothing when the typed email does not match', async () => { + const res = await postReset( + { userId: TARGET.userId, confirmEmail: 'someone.else@test.local' }, + ADMIN, + ) + expect(res.status).toBe(400) + // The whole point of the confirmation is the mis-click, so assert the DB is untouched + // rather than just the status code. + expect(readUser(TARGET.userId)).toEqual({ enabled: 1, required: 0 }) + expect( + db.raw.prepare(`SELECT 1 FROM auth_two_factor WHERE user_id = ?`).get(TARGET.userId), + ).toBeDefined() + }) + + it('refuses a non-admin', async () => { + const res = await postReset({ userId: TARGET.userId, confirmEmail: TARGET.email }, EDITOR) + expect(res.status).toBe(403) + expect(readUser(TARGET.userId)).toEqual({ enabled: 1, required: 0 }) + }) + + it('refuses an anonymous caller', async () => { + const res = await postReset({ userId: TARGET.userId, confirmEmail: TARGET.email }) + expect(res.status).toBe(401) + expect(readUser(TARGET.userId)).toEqual({ enabled: 1, required: 0 }) + }) + + it('404s an unknown user without leaking whether the email was right', async () => { + const res = await postReset({ userId: 'nope', confirmEmail: TARGET.email }, ADMIN) + expect(res.status).toBe(404) + }) + + it('rejects a malformed body rather than guessing', async () => { + const res = await postReset({ confirmEmail: TARGET.email }, ADMIN) + expect(res.status).toBe(400) + }) + + it('permits self-reset — reaching here already required passing the factor', async () => { + db.raw.prepare(`UPDATE auth_user SET two_factor_enabled = 1 WHERE id = ?`).run(ADMIN.userId) + seedEnrolment(ADMIN.userId, 1) + + const res = await postReset({ userId: ADMIN.userId, confirmEmail: ADMIN.email }, ADMIN) + expect(res.status).toBe(200) + expect(readUser(ADMIN.userId)).toEqual({ enabled: 0, required: 1 }) + }) + + it('still works when the plugin is deactivated', async () => { + // Deactivating the plugin does not stop Better Auth challenging enrolled users, so recovery + // must outlive the surface. This is the assertion that stops someone "tidying" the + // deactivate→404 gate onto this mount. + seedPluginStatus('inactive') + const res = await postReset({ userId: TARGET.userId, confirmEmail: TARGET.email }, ADMIN) + expect(res.status).toBe(200) + }) +}) + +describe('owesTwoFactorEnrolment', () => { + beforeEach(() => seedUser(TARGET)) + + it('is true only for required-and-not-enrolled', async () => { + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + await expect(owesTwoFactorEnrolment(db as never, TARGET.userId)).resolves.toBe(true) + }) + + it('is false once they enrol, even though the flag stays set', async () => { + // required && verified means "enrolled, and may not turn it off" — the flag is not cleared, + // so reading it alone would trap the user on the enrolment page forever. + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + seedEnrolment(TARGET.userId, 1) + await expect(owesTwoFactorEnrolment(db as never, TARGET.userId)).resolves.toBe(false) + }) + + it('is false for an unconfirmed enrolment, so a half-finished setup still enforces', async () => { + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + seedEnrolment(TARGET.userId, 0) + await expect(owesTwoFactorEnrolment(db as never, TARGET.userId)).resolves.toBe(true) + }) + + it('is false when nothing was demanded', async () => { + await expect(owesTwoFactorEnrolment(db as never, TARGET.userId)).resolves.toBe(false) + }) + + it('is false for an unknown or empty user id', async () => { + await expect(owesTwoFactorEnrolment(db as never, 'ghost')).resolves.toBe(false) + await expect(owesTwoFactorEnrolment(db as never, '')).resolves.toBe(false) + }) +}) + +describe('enforceTwoFactorEnrolment', () => { + beforeEach(() => { + seedUser(TARGET) + seedPluginStatus('active') + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + }) + + it('redirects a user who owes an enrolment', async () => { + const res = await requestGuarded('/admin/content', TARGET) + expect(res.status).toBe(302) + // Bare path, no explanatory query string — the enrolment page reads the flag itself, so + // nobody can hand a colleague a link that fakes "your admin reset your 2FA". + expect(res.headers.get('location')).toBe(ENROLMENT_PATH) + }) + + it('lets them reach the enrolment page and its QR endpoint', async () => { + await expect((await requestGuarded(ENROLMENT_PATH, TARGET)).text()).resolves.toContain( + 'reached the handler', + ) + await expect((await requestGuarded(`${ENROLMENT_PATH}/qr`, TARGET)).text()).resolves.toContain( + 'reached the handler', + ) + }) + + it('does NOT exempt the reset route, which shares the enrolment prefix as a string', async () => { + // `/admin/two-factor-reset`.startsWith('/admin/two-factor') is true — a naive prefix check + // would hand a user who owes an enrolment the break-glass endpoint. + const res = await requestGuarded('/admin/two-factor-reset', TARGET) + expect(res.status).toBe(302) + }) + + it('answers a JSON caller with 403 rather than a redirect', async () => { + const res = await requestGuarded('/admin/api/whatever', TARGET, 'application/json') + expect(res.status).toBe(403) + await expect(res.json()).resolves.toMatchObject({ enrolmentPath: ENROLMENT_PATH }) + }) + + it('stands aside for a user who owes nothing', async () => { + await expect((await requestGuarded('/admin/content', ADMIN)).text()).resolves.toContain( + 'reached the handler', + ) + }) + + it('stands aside for an anonymous request', async () => { + await expect((await requestGuarded('/admin/content')).text()).resolves.toContain( + 'reached the handler', + ) + }) + + it('stands aside when the plugin is deactivated, instead of looping to a 404', async () => { + db.raw.prepare(`UPDATE documents SET data = ? WHERE id = 'doc-2fa'`).run( + JSON.stringify({ status: 'inactive' }), + ) + invalidatePluginStatusCache(TWO_FACTOR_PLUGIN_ID) + // /admin/two-factor 404s while the plugin is off, so enforcing would redirect the user in a + // loop to a page that cannot exist. Deactivating is the operator's escape hatch. + await expect((await requestGuarded('/admin/content', TARGET)).text()).resolves.toContain( + 'reached the handler', + ) + }) + + it('fails OPEN when the column is missing, rather than locking out the portal', async () => { + db.raw.exec(`ALTER TABLE auth_user DROP COLUMN two_factor_required`) + await expect((await requestGuarded('/admin/content', TARGET)).text()).resolves.toContain( + 'reached the handler', + ) + }) +}) + +describe('guardRequiredSecondFactorDisable', () => { + /** Drive the guard the way app.ts does: in front of a stub standing in for auth.handler. */ + function post(path: string, user?: typeof ADMIN, method = 'POST') { + const app = new Hono<{ Bindings: { DB: unknown }; Variables: { user?: typeof ADMIN } }>() + app.use('*', async (c, next) => { + if (user) c.set('user', user) + await next() + }) + app.on(['GET', 'POST'], '/auth/*', async (c) => { + const refused = await guardRequiredSecondFactorDisable(c as never) + if (refused) return refused + return c.json({ status: true, reachedBetterAuth: true }) + }) + return app.request(path, { method }, { DB: db }) + } + + beforeEach(() => { + seedUser(TARGET, 1) + seedEnrolment(TARGET.userId, 1) + }) + + it('refuses the disable when an admin requires 2FA on the account', async () => { + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + + const res = await post('/auth/two-factor/disable', TARGET) + expect(res.status).toBe(403) + await expect(res.json()).resolves.toMatchObject({ code: 'TWO_FACTOR_REQUIRED' }) + }) + + it('refuses even though the user owes NO enrolment — the two conditions differ', async () => { + // required && verified: `owesTwoFactorEnrolment` is false here, so a guard written against + // that helper would wave this straight through. This is the whole reason isTwoFactorRequired + // exists as a separate read. + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + await expect(owesTwoFactorEnrolment(db as never, TARGET.userId)).resolves.toBe(false) + + expect((await post('/auth/two-factor/disable', TARGET)).status).toBe(403) + }) + + it('lets the disable through when nothing was mandated', async () => { + const res = await post('/auth/two-factor/disable', TARGET) + expect(res.status).toBe(200) + await expect(res.json()).resolves.toMatchObject({ reachedBetterAuth: true }) + }) + + it('stands aside for an anonymous caller — Better Auth refuses it anyway', async () => { + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + const res = await post('/auth/two-factor/disable') + expect(res.status).toBe(200) + }) + + it('guards only the disable path, not the rest of the two-factor surface', async () => { + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + // Enrolling and verifying must stay reachable — they are how the requirement gets satisfied. + expect((await post('/auth/two-factor/enable', TARGET)).status).toBe(200) + expect((await post('/auth/two-factor/verify-totp', TARGET)).status).toBe(200) + expect((await post('/auth/sign-in/email', TARGET)).status).toBe(200) + }) + + it('ignores non-POST requests to the disable path', async () => { + db.raw.prepare(`UPDATE auth_user SET two_factor_required = 1 WHERE id = ?`).run(TARGET.userId) + expect((await post('/auth/two-factor/disable', TARGET, 'GET')).status).toBe(200) + }) + + it('fails OPEN when the column is missing, rather than freezing self-management', async () => { + db.raw.exec(`ALTER TABLE auth_user DROP COLUMN two_factor_required`) + expect((await post('/auth/two-factor/disable', TARGET)).status).toBe(200) + }) +}) + +describe('ensureTwoFactorRequiredColumn — the 0004 self-heal', () => { + it('restores the column on a database that never got the migration', async () => { + seedUser(TARGET, 1) + db.raw.exec(`ALTER TABLE auth_user DROP COLUMN two_factor_required`) + + await ensureTwoFactorRequiredColumn(db as never) + + // DEFAULT 0 is the load-bearing part: an existing user must not become retroactively locked + // out of the portal by the column appearing. + expect(readUser(TARGET.userId)).toEqual({ enabled: 1, required: 0 }) + }) + + it('is idempotent and silent when the column is already there', async () => { + seedUser(TARGET) + await ensureTwoFactorRequiredColumn(db as never) + await ensureTwoFactorRequiredColumn(db as never) + expect(readUser(TARGET.userId)).toEqual({ enabled: 0, required: 0 }) + }) +}) diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/registration.test.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/registration.test.ts new file mode 100644 index 000000000..19dea6d02 --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/__tests__/registration.test.ts @@ -0,0 +1,74 @@ +/** + * Plugin registration shape, and the two id/path facts that are silent when wrong. + */ +import { describe, it, expect } from 'vitest' +import { twoFactorAuthPlugin, createTwoFactorAuthPlugin } from '../index' +import manifest from '../manifest.json' +import { TWO_FACTOR_PLUGIN_ID } from '../../../../auth/two-factor-settings' +import { CORE_PLUGIN_IDS } from '../../index' +import { PLUGIN_REGISTRY } from '../../../manifest-registry' + +describe('two-factor-auth plugin registration', () => { + it('is a v3 definePlugin', () => { + expect(twoFactorAuthPlugin.__sonicV3).toBe(true) + expect(createTwoFactorAuthPlugin()).toBe(twoFactorAuthPlugin) + }) + + it('uses one id everywhere — code, manifest, registry, core list', () => { + // `isPluginActive()` and the plugin-settings document both key off this exact string, so a + // divergence leaves the surface permanently 404 with nothing in the logs. + expect(twoFactorAuthPlugin.id).toBe('two-factor-auth') + expect(TWO_FACTOR_PLUGIN_ID).toBe('two-factor-auth') + expect(manifest.id).toBe('two-factor-auth') + expect(PLUGIN_REGISTRY['two-factor-auth']?.id).toBe('two-factor-auth') + expect(CORE_PLUGIN_IDS).toContain('two-factor-auth') + }) + + it('ships active on a fresh install', () => { + expect(PLUGIN_REGISTRY['two-factor-auth']?.is_core).toBe(true) + expect(PLUGIN_REGISTRY['two-factor-auth']?.defaultActive).toBe(true) + }) + + it('links its sidebar entry at /admin/two-factor, not under /admin/plugins', () => { + // /admin/plugins/two-factor would be shadowed by adminPluginRoutes, taking out both the + // plugin-detail page and this plugin's own /configure settings form. + expect(twoFactorAuthPlugin.menu?.[0]?.path).toBe('/admin/two-factor') + expect(manifest.adminMenu.path).toBe('/admin/two-factor') + }) + + it('declares the policy knobs as a configSchema, so the admin form is real', () => { + const schema = twoFactorAuthPlugin.configSchema + expect(Object.keys(schema ?? {})).toEqual([ + 'issuer', + 'maxFailedAttempts', + 'lockoutDurationSeconds', + 'backupCodeCount', + ]) + }) + + it('keeps configSchema bounds in step with the clamp in normalizeTwoFactorPolicy', () => { + const schema = twoFactorAuthPlugin.configSchema as Record + expect(schema.maxFailedAttempts).toMatchObject({ min: 3, max: 10 }) + expect(schema.lockoutDurationSeconds).toMatchObject({ min: 300, max: 3600 }) + expect(schema.backupCodeCount).toMatchObject({ min: 5, max: 20 }) + }) + + it('declares only the administrative permission — self-enrolment is not gated', () => { + expect(Object.keys(manifest.permissions)).toEqual(['two-factor:manage']) + }) + + it('registers ONLY the enrolment surface — the challenge belongs to core', () => { + // The plugin used to mount `/auth/two-factor` as well, which put the login challenge behind + // `config.plugins.disableAll`. Better Auth composes `twoFactor()` unconditionally, so with + // plugins off an enrolled user was still challenged and then redirected to a 404 — locked out + // of an app that was still demanding their second factor. Core now mounts the challenge + // (app.ts), and this asserts the plugin does not take it back. + const mounted: string[] = [] + twoFactorAuthPlugin.register?.({ + route: (path: string) => { + mounted.push(path) + }, + } as never) + expect(mounted).toEqual(['/admin/two-factor']) + }) +}) diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/components/challenge-page.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/components/challenge-page.ts new file mode 100644 index 000000000..9e0c0c5dd --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/components/challenge-page.ts @@ -0,0 +1,136 @@ +/** + * Login challenge page — `/auth/two-factor`. + * + * Reached after a password has been accepted but before a session exists. Renders standalone + * (mirroring the login page rather than the admin layout) for the same reason it is mounted + * outside `/admin/*`: there is no session yet, so there is no admin chrome, no user menu, and no + * permissions to render. + * + * ── Backup codes are always offered ── + * NOT keyed off Better Auth's `twoFactorMethods`, which only ever contains `'totp'` and `'otp'` + * — never `'backup_code'` (see the after-hook in better-auth/plugins/two-factor). Keying the UI + * off that list would hide backup-code entry exactly when it matters most: the user has lost the + * device that generates the codes the list is telling them to use. + * + * ── CSRF ── + * These POSTs normally carry no `auth_token` cookie (BA deleted the session cookie when it issued + * the challenge), and `csrfProtection` exempts cookie-less requests. But a browser that still + * holds a stale `auth_token` from a previous session WOULD be validated, so the header is sent + * unconditionally — the GET of this page sets `csrf_token`, so it is always available. Sending it + * when it is not required is inert; omitting it when it is required is a 403 nobody can debug. + */ + +const INPUT_CLASS = + 'w-full rounded-lg bg-white dark:bg-zinc-800 px-3 py-2 text-sm text-zinc-950 dark:text-white ring-1 ring-inset ring-zinc-950/10 dark:ring-white/10' + +export function renderTwoFactorChallengePage(): string { + return ` + + + + + Two-Factor Verification - SonicJS AI + + + + + + +
+
+

Two-step verification

+

+ Enter the 6-digit code from your authenticator app to finish signing in. +

+ +
+
+ + + +
+ + + + + + +
+ +

Back to sign in

+
+
+ + + +` +} diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/components/enrolment-page.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/components/enrolment-page.ts new file mode 100644 index 000000000..35c821500 --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/components/enrolment-page.ts @@ -0,0 +1,312 @@ +/** + * Enrolment page — `/admin/two-factor`. + * + * ── QR code ── + * Authenticator apps take an `otpauth://` URI. This page renders it three ways, in descending + * order of how people actually enrol: a scannable QR, the `otpauth://` link (which opens the app + * directly when the admin panel IS on the phone), and the raw secret for manual entry. + * + * An earlier version shipped only the link and the secret, arguing that every authenticator + * supports manual entry so a QR dependency "would buy one paste". That was wrong about the + * primary flow: the admin panel is used on a desktop and the authenticator lives on a phone, so + * there is no handler for the link and no shared clipboard — the user is left hand-typing a + * 32-character base32 secret across devices, or unable to enrol at all. The QR is rendered + * server-side by `POST /admin/two-factor/qr` (see routes.ts) because these pages have no client + * bundler and a CDN script tag on the page that handles TOTP secrets is not a trade worth making. + * + * ── Backup codes ── + * Shown exactly once, at enrolment, and the page says so plainly rather than burying it. With + * no external identity provider they are the ONLY route back in if the device is lost. + * + * ── Passwordless notice ── + * The page states that enrolling disables magic-link / email-code sign-in for the account. + * That is a real consequence of `auth/passwordless-second-factor-guard.ts`, and a user who + * discovers it by having a link silently stop arriving would reasonably call it a bug. + * + * ── CSRF ── + * Every mutation is a cookie-authenticated POST to `/auth/two-factor/*`, which is NOT on + * `csrfProtection`'s exempt list, so each one carries `X-CSRF-Token` read from the `csrf_token` + * cookie the GET of this page just set. Without the header they all 403. + */ +import { renderAdminLayoutCatalyst } from '../../../../templates/layouts/admin-layout-catalyst.template' +import { escapeHtml } from '../../../../utils/sanitize' + +export interface EnrolmentPageData { + /** A verified enrolment exists — render the "on" state. */ + verified: boolean + /** A row exists but was never confirmed against a live code. */ + pending: boolean + /** + * The RAW `auth_user.two_factor_required` flag — an admin mandates a second factor here. + * + * Two different things are derived from it, and they are not the same condition: + * - `required && !verified` → the amber "set up to continue" banner. The portal is closed. + * - `required` alone → the disable form is replaced by an explanation. A user who has + * satisfied the requirement still may not turn it back off. + * + * Read from the database, never from the request: this tells the user their administrator acted + * on their account, and a query parameter would let anyone put those words on their screen. + */ + required?: boolean + user?: { name: string; email: string; role: string } + version?: string + dynamicMenuItems?: Array<{ label: string; path: string; icon: string }> +} + +const INPUT_CLASS = + 'w-full rounded-lg bg-white dark:bg-zinc-800 px-3 py-2 text-sm text-zinc-950 dark:text-white ring-1 ring-inset ring-zinc-950/10 dark:ring-white/10' +const PRIMARY_BUTTON_CLASS = + 'rounded-lg bg-zinc-900 dark:bg-white px-3 py-2 text-sm font-medium text-white dark:text-zinc-900 hover:bg-zinc-700 dark:hover:bg-zinc-200' + +export function renderTwoFactorEnrolmentPage(data: EnrolmentPageData): string { + const { verified, pending } = data + + const statusLabel = verified ? 'Enabled' : pending ? 'Started, not confirmed' : 'Disabled' + const statusClass = verified + ? 'bg-green-50 dark:bg-green-500/10 text-green-700 dark:text-green-400 ring-green-600/20 dark:ring-green-500/20' + : 'bg-zinc-50 dark:bg-zinc-800 text-zinc-600 dark:text-zinc-400 ring-zinc-500/10 dark:ring-zinc-400/20' + + const content = ` +
+

Two-Factor Authentication

+${data.required && !verified ? ` +
+

Set up two-factor to continue

+

+ An administrator reset the two-factor authentication on your account. The rest of the + admin portal stays closed until you finish setting it up again. +

+
+` : ''} +
+

+ Add a time-based one-time password (TOTP) from an authenticator app as a second factor + on your account. You will be asked for a code after your password on every sign-in. +

+ +

+ Status + ${escapeHtml(statusLabel)} +

+ +
+

+ While two-factor authentication is on, magic links and emailed sign-in codes are + disabled for this account — anyone who reads your inbox would otherwise be able + to skip the second factor entirely. Sign in with your password and your authenticator. +

+
+ + ${pending ? ` +
+

+ A previous setup was started but never confirmed with a live code, so two-factor is + not active. Start again below — that issues a fresh secret and a + fresh set of backup codes, and discards the unconfirmed one. +

+
` : ''} + +
+
+ + + +
+
+ + + +
+

+ Two-factor authentication is active on this account. You will be asked for a code after + your password on every sign-in. +

+ ${data.required ? ` +
+

Required by an administrator

+

+ Two-factor authentication is mandatory on this account and cannot be turned off here. + Ask an administrator if this needs to change. +

+
+ ` : ` +
+ + + +
+ `} +
+ + +
+
+ + ` + + return renderAdminLayoutCatalyst({ + title: 'Two-Factor Authentication', + pageTitle: 'Two-Factor Authentication', + currentPath: '/admin/two-factor', + user: data.user, + version: data.version, + dynamicMenuItems: data.dynamicMenuItems, + content, + }) +} diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/index.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/index.ts new file mode 100644 index 000000000..447965e9e --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/index.ts @@ -0,0 +1,216 @@ +/** + * two-factor-auth — TOTP second factor with single-use backup codes. + * + * ── What lives where ── + * This plugin owns the SURFACE and the POLICY: the enrolment page, the login challenge page, the + * `/complete` session upgrade, and the settings that feed the policy knobs. It does NOT own the Better Auth plugin + * instance — `auth/config.ts` composes `twoFactor()`, because BA is constructed per request + * by a synchronous `createAuth()` that a plugin's `register(app)` cannot reach into. + * + * The policy is read once per isolate here in `onBoot` and handed to `createAuth` through the + * module-level cache in `auth/two-factor-settings.ts` — see that file for why the getter has + * to be synchronous. + * + * ── Two mounts, deliberately asymmetric ── + * - `/admin/two-factor` — enrolment. Behind the global `/admin/*` requireAuth + + * requireRbac('portal','access') gate, plus a local requireAuth and a deactivate→404 gate. + * - `/auth/two-factor` — the login challenge. Mounted outside `/admin/*` and NOT gated, + * because it must be reachable WITHOUT a session: at challenge time the caller has proven + * a password and nothing else, and Better Auth has already deleted the session cookie it + * briefly created. That is a decision, not an oversight — a challenge page that 404s when + * the plugin is deactivated would strand every enrolled user mid-login, which is exactly + * the silent-downgrade failure the unconditional composition in `auth/config.ts` exists to + * prevent. + * + * ── Prefixes ── + * Enrolment is `/admin/two-factor`, not under `/admin/plugins/`. Plugin routes are registered + * ahead of `adminPluginRoutes` and Hono's first match wins, so mounting at + * `/admin/plugins/two-factor-auth` would make THIS sub-app shadow the generic plugin-detail page + * for this plugin (`/admin/plugins/:id`). Keeping enrolment on its own prefix leaves both the + * detail page and the schema-driven settings form at + * `/admin/plugins/two-factor-auth/configure` untouched. (`api-docs` does mount under + * `/admin/plugins/api-docs` and accepts that trade; a security control gets its own prefix.) + * + * `/auth/two-factor` does not collide with Better Auth: every BA two-factor endpoint is + * `POST /auth/two-factor/` (enable, disable, verify-totp, verify-backup-code, + * get-totp-uri, generate-backup-codes), and there is no bare `/two-factor` route. Hono + * flattens `app.route()` sub-apps into the parent router, so the POSTs fall through to the + * `/auth/*` catch-all that serves BA. + */ + +import { definePlugin } from '../../sdk/define-plugin' +import { twoFactorAdminRoutes } from './routes' +import { loadTwoFactorPolicy, TWO_FACTOR_PLUGIN_ID } from '../../../auth/two-factor-settings' +import { + ensureTwoFactorLockoutColumns, + ensureTwoFactorRequiredColumn, +} from '../../../services/migrations' +import type { D1Database } from '@cloudflare/workers-types' +import manifest from './manifest.json' + +// Heroicons "lock-closed" (matches the manifest adminMenu icon). +const TWO_FACTOR_ICON = `` + +export const twoFactorAuthPlugin = definePlugin({ + id: TWO_FACTOR_PLUGIN_ID, + version: manifest.version, + name: manifest.name, + description: manifest.description, + sonicjsVersionRange: '^3.0.0', + author: { name: manifest.author }, + + /** + * Only the ENROLMENT surface. The login challenge (`/auth/two-factor`) is mounted by core in + * app.ts, unconditionally — see the comment there. Mounting it here too would put it behind + * `config.plugins.disableAll`, which is what previously 404'd enrolled users out of an app whose + * Better Auth still demanded their second factor. + */ + register(app) { + app.route('/admin/two-factor', twoFactorAdminRoutes) + }, + + /** + * Three jobs, all idempotent. + * + * 0. Repair the columns migrations 0003 and 0004 add — `auth_two_factor`'s lockout pair and + * `auth_user.two_factor_required` — if those migrations never ran. Both also live in + * `MigrationService.ensureSchemaCompatibility()`, but the bootstrap middleware skips that + * entirely once the `_sonicjs_bootstrap_` KV marker is set (24h TTL) — so on most + * cold isolates it would not run at all. `onBoot` runs on every isolate, which is what makes + * the repair the belt-and-braces the migration comments claim it is. The 0004 repair matters + * most: the enrolment-enforcement middleware reads that column on every admin request, so a + * database missing it would 500 the whole portal rather than degrade. + * + * 1. Make sure the plugin's own document row exists. `PluginBootstrapService` only *checks* + * plugins whose name starts with `core-` when deciding whether a bootstrap is needed, so + * on a database that was already bootstrapped before this plugin existed, nothing would + * ever install it — and `isPluginActive()` would then 404 the enrolment page forever. + * Create-if-missing only: an existing row is returned untouched, so an admin's + * deactivation is never silently undone by the next cold isolate. + * + * 2. Load the policy into the isolate cache that `createAuth` reads synchronously. `boot()` + * is registered ahead of the session middleware in app.ts and awaited, so by the first + * `createAuth()` of the isolate the real settings are in place; until then the strict + * defaults apply. + */ + async onBoot(ctx) { + const db = (ctx.env as { DB?: D1Database } | undefined)?.DB + if (!db) return + + await ensureTwoFactorLockoutColumns(db) + await ensureTwoFactorRequiredColumn(db) + + try { + const { PluginService } = await import('../../../services/plugin-service') + const service = new PluginService(db) + if (!(await service.getPlugin(TWO_FACTOR_PLUGIN_ID))) { + await service.installPlugin({ + id: TWO_FACTOR_PLUGIN_ID, + name: TWO_FACTOR_PLUGIN_ID, + display_name: manifest.name, + description: manifest.description, + version: manifest.version, + author: manifest.author, + category: manifest.category, + icon: manifest.iconEmoji, + // `false`, matching what PluginBootstrapService writes on the greenfield path + // (`is_core: plugin.name.startsWith("core-")` — this id does not). Passing + // `manifest.is_core` here would give the SAME plugin a different `isCore` depending on + // which path installed it, which `uninstallPlugin`'s core guard reads. + // `manifest.is_core: true` still does its real job — it is what puts this plugin in + // BOOTSTRAP_PLUGIN_IDS on a fresh install. + is_core: false, + settings: manifest.defaultSettings, + permissions: Object.keys(manifest.permissions), + }) + // installPlugin wrote a row this isolate may already have cached as inactive. + const { invalidatePluginStatusCache } = await import('../../../middleware/plugin-middleware') + invalidatePluginStatusCache(TWO_FACTOR_PLUGIN_ID) + console.log('[two-factor] registered plugin row (was missing)') + } + } catch (e) { + // Non-fatal: the surface 404s until the row exists, but verification (auth/config.ts) is + // never gated on plugin status, so an enrolled user can still sign in. + console.error('[two-factor] plugin row registration failed', e) + } + + try { + await loadTwoFactorPolicy(db) + } catch (e) { + // Never fail boot for a settings read — defaults are the safe end of every knob. + console.error('[two-factor] policy load failed during onBoot; using defaults', e) + } + }, + + menu: [ + { label: 'Two-Factor Auth', path: '/admin/two-factor', icon: TWO_FACTOR_ICON, order: 86 }, + ], + + /** + * Policy settings, rendered automatically at `/admin/plugins/two-factor-auth/configure` (the + * generic `/:id/configure` form, so the path carries the full plugin id) and persisted by + * `PluginService.updatePluginSettings` into the plugin document — the same place + * `loadTwoFactorPolicy` reads, so this is not save-nowhere chrome. + * + * Bounds are duplicated in `normalizeTwoFactorPolicy`, which clamps on read. The form is a + * hint; the clamp is the guarantee. + */ + configSchema: { + issuer: { + type: 'string', + label: 'Issuer name', + default: 'SonicJS', + description: "Label shown beside the account in the user's authenticator app.", + maxLength: 64, + }, + maxFailedAttempts: { + type: 'number', + label: 'Failed attempts before lockout', + default: 5, + min: 3, + max: 10, + description: + 'Consecutive failed second-factor verifications before the account is locked. Per-account, so rotating IPs does not help an attacker.', + }, + lockoutDurationSeconds: { + type: 'number', + label: 'Lockout duration (seconds)', + default: 900, + min: 300, + max: 3600, + description: 'How long the second-factor lockout lasts.', + }, + backupCodeCount: { + type: 'number', + label: 'Backup code count', + default: 10, + min: 5, + max: 20, + description: + 'Single-use codes minted at enrolment. With no external identity provider these are the only route back in if the authenticator device is lost.', + }, + }, + + activate: async () => console.log('[TwoFactorAuth] Plugin activated'), + deactivate: async () => console.log('[TwoFactorAuth] Plugin deactivated'), +}) + +export function createTwoFactorAuthPlugin() { + return twoFactorAuthPlugin +} + +export { twoFactorAdminRoutes, twoFactorChallengeRoutes } from './routes' +// Mounted by core in app.ts, NOT by `register()` above — see recovery.ts for why both the reset +// routes and the enforcement middleware have to sit outside this plugin's deactivate→404 gate +// and outside plugin registration order. +export { + twoFactorRecoveryRoutes, + enforceTwoFactorEnrolment, + guardRequiredSecondFactorDisable, + resetUserTwoFactor, + isTwoFactorRequired, + RECOVERY_PREFIX, + ENROLMENT_PATH, + BA_DISABLE_PATH, +} from './recovery' +export default twoFactorAuthPlugin diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/manifest.json b/packages/core/src/plugins/core-plugins/two-factor-auth/manifest.json new file mode 100644 index 000000000..62c2b96cd --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/manifest.json @@ -0,0 +1,34 @@ +{ + "id": "two-factor-auth", + "name": "Two-Factor Authentication", + "version": "1.0.0", + "description": "Time-based one-time passwords (TOTP) with single-use backup codes and per-account second-factor lockout.", + "author": "SonicJS Team", + "category": "security", + "tags": [ + "2fa", + "totp", + "mfa", + "authentication", + "security" + ], + "dependencies": [], + "permissions": { + "two-factor:manage": "Change two-factor policy settings" + }, + "adminMenu": { + "label": "Two-Factor Auth", + "icon": "lock-closed", + "path": "/admin/two-factor", + "order": 86 + }, + "iconEmoji": "🔐", + "is_core": true, + "defaultActive": true, + "defaultSettings": { + "issuer": "SonicJS", + "maxFailedAttempts": 5, + "lockoutDurationSeconds": 900, + "backupCodeCount": 10 + } +} diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/recovery.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/recovery.ts new file mode 100644 index 000000000..5f73f22a6 --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/recovery.ts @@ -0,0 +1,433 @@ +/** + * Administrative recovery for two-factor auth — the break-glass path. + * + * ── Why this exists ── + * Without it, the complete list of ways back into an account with a lost authenticator is: + * an unused backup code, or `wrangler d1 execute --remote`. Everything an operator would + * reasonably reach for is closed by design: + * + * - Password reset does not mint a session (`passwordless-second-factor-guard.ts`), so it + * hands the user a new password and the same unpassable challenge. + * - Magic link and email OTP are refused outright for enrolled users — whoever owns the + * inbox would otherwise own the account, which is the thing the second factor is for. + * - Self-service disable requires a session, which requires the second factor. + * + * So a sole admin who enrols and then loses phone and codes locks the entire organisation out + * of the admin portal, recoverable only by someone holding Cloudflare credentials. This turns + * that incident into another admin clicking a button. + * + * ── The cost, stated plainly ── + * This makes any admin account a path around any user's second factor. It does NOT hand over + * the account — the password is still required, and this never touches it — but it does remove + * a control from someone else's account. Hence: admin role required, the target's email must be + * typed back, and every use writes a `two_factor_reset` security event. + * + * ── Not behind the deactivate→404 gate ── + * `twoFactorAdminRoutes` 404s when the plugin is deactivated. These routes deliberately do not, + * for the same reason the login challenge does not: `auth/config.ts` composes Better Auth's + * `twoFactor` plugin unconditionally, so deactivating this plugin stops nothing about + * verification — enrolled users are still challenged. A recovery path that disappeared exactly + * when the surface was turned off would be worse than none, because the lockout it is meant to + * fix would still be happening. + * + * Mounted on its own prefix (`/admin/two-factor-reset`) rather than under `/admin/two-factor`: + * Hono flattens `app.route()` sub-apps into the parent, so anything mounted at the enrolment + * prefix would inherit that sub-app's `use('*')` deactivate gate — the very thing this must not + * have. + */ +import { Hono } from 'hono' +import type { Context, MiddlewareHandler } from 'hono' +import { z } from 'zod' +import { requireAuth, requireRole } from '../../../middleware' +import { isPluginActive } from '../../../middleware/plugin-middleware' +import { TWO_FACTOR_PLUGIN_ID } from '../../../auth/two-factor-settings' +import type { D1Database } from '@cloudflare/workers-types' +import type { Bindings, Variables } from '../../../app' + +/** Where a user who owes an enrolment is sent, and the one place they may go until they finish. */ +export const ENROLMENT_PATH = '/admin/two-factor' + +/** Public prefix for the reset endpoint. Mounted in app.ts — see the module docblock. */ +export const RECOVERY_PREFIX = '/admin/two-factor-reset' + +const resetRequestSchema = z.object({ + /** Target user. Never the acting admin implicitly — it must always be named. */ + userId: z.string().min(1, 'userId is required'), + /** + * The target's email, typed by the operator. Compared case-insensitively against the record. + * + * This is not theatre and it is not a second authentication factor: it defends against the + * realistic failure, which is resetting the wrong row. The action is invoked from a list of + * similar-looking rows, it is irreversible for the user (their enrolment is destroyed), and + * the ids are opaque. A password confirmation would be the stronger control, but Better Auth + * hashes with its own scrypt (`salt:key`) that `AuthManager.verifyPassword` cannot read, and + * reimplementing it wrong would break the break-glass itself. + */ + confirmEmail: z.string().min(1, 'confirmEmail is required'), + /** + * Leave `two_factor_required = 1` so the user must enrol again before using the portal. + * + * Defaults to true because the alternative is worse than it looks: a reset with no follow-up + * silently converts "protected by 2FA" into "password only" and nothing ever prompts the user + * about it. Set false deliberately — e.g. offboarding, or a user who should not have been + * enrolled at all. + */ + requireReenrolment: z.boolean().optional().default(true), +}) + +export type TwoFactorResetRequest = z.infer + +export interface TwoFactorResetOutcome { + /** The row actually cleared, for the audit record and the confirmation message. */ + email: string + /** Whether the user had a completed enrolment before this ran (false = reset was a no-op repair). */ + wasEnrolled: boolean + requireReenrolment: boolean +} + +/** + * Clear one user's second factor. + * + * Both statements go in a single `batch` so the pair cannot half-apply. A DELETE that landed + * without the UPDATE would leave `two_factor_enabled = 1` with no enrolment row behind it — + * a user who reads "Enabled" everywhere and is challenged by nothing. + * + * Deleting the `auth_two_factor` row also clears `failed_verification_count` and `locked_until` + * (migration 0003 put both there), so this fixes a lockout as well as a lost device. + * + * Exported separately from the route so the DB effect is testable without a Hono context. + */ +export async function resetUserTwoFactor( + db: D1Database, + userId: string, + requireReenrolment: boolean, +): Promise { + await db.batch([ + db.prepare(`DELETE FROM auth_two_factor WHERE user_id = ?`).bind(userId), + db + .prepare(`UPDATE auth_user SET two_factor_enabled = 0, two_factor_required = ? WHERE id = ?`) + .bind(requireReenrolment ? 1 : 0, userId), + ]) +} + +/** Both halves of a user's second-factor policy state, in one round trip. */ +interface TwoFactorPolicyState { + /** `auth_user.two_factor_required` — an admin demanded a second factor on this account. */ + required: boolean + /** A COMPLETED enrolment exists (`auth_two_factor.verified = 1`). */ + verified: boolean +} + +/** + * Read the policy state. + * + * One query rather than two, because the enforcement middleware runs on every admin request and a + * second sequential D1 read there would be felt. + * + * Resolves to "no policy, not enrolled" on any error — see the fail-open note on + * {@link enforceTwoFactorEnrolment}. Every caller here shares it, so the banner the enrolment page + * shows, the redirect that sent the user there, and the disable guard can never disagree. + */ +async function readPolicyState(db: D1Database, userId: string): Promise { + if (!userId) return { required: false, verified: false } + try { + const row = await db + .prepare( + `SELECT u.two_factor_required AS required, + (SELECT tf.verified FROM auth_two_factor tf WHERE tf.user_id = u.id LIMIT 1) AS verified + FROM auth_user u + WHERE u.id = ?`, + ) + .bind(userId) + .first<{ required: number; verified: number | null }>() + return { required: row?.required === 1, verified: row?.verified === 1 } + } catch (e) { + console.error('[two-factor] policy state lookup failed; treating as unset', e) + return { required: false, verified: false } + } +} + +/** + * Does this user owe an enrolment right now? + * + * `required && verified` is a user who has enrolled and may not turn it back off — the flag stays + * set, and this must not act on it. Only `required && !verified` owes anything. + */ +export async function owesTwoFactorEnrolment(db: D1Database, userId: string): Promise { + const { required, verified } = await readPolicyState(db, userId) + return required && !verified +} + +/** + * Is a second factor MANDATED on this account, regardless of whether one currently exists? + * + * Distinct from {@link owesTwoFactorEnrolment} on purpose, and the difference is the whole point + * of the disable guard: a user who has satisfied the requirement is `required && verified`, so + * "owes an enrolment" is false for them while "may not turn it off" is still true. + */ +export async function isTwoFactorRequired(db: D1Database, userId: string): Promise { + return (await readPolicyState(db, userId)).required +} + +/** + * Write the audit record. + * + * Best-effort, and that is a deliberate ranking: this feature exists so that nobody is ever + * locked out of their own admin panel, so an unavailable audit sink (the security-audit plugin + * is separately installable, and its document type may not be registered) must not be the reason + * a break-glass fails. A failure here is loud in the logs rather than silent. + */ +async function auditReset( + db: D1Database, + fields: { + actorId: string + actorEmail: string + targetId: string + targetEmail: string + wasEnrolled: boolean + requireReenrolment: boolean + ipAddress?: string + userAgent?: string + }, +): Promise { + try { + const { SecurityAuditService } = await import( + '../security-audit-plugin/services/security-audit-service' + ) + await new SecurityAuditService(db).logEvent({ + eventType: 'two_factor_reset', + // Not 'info': someone's second factor was removed by someone else. This is the kind of + // event that should stand out when an account is later found to be compromised. + severity: 'warning', + // The SUBJECT of the event, so it surfaces when filtering by the affected account. The + // actor is in details — losing that would make the record useless for accountability. + userId: fields.targetId, + email: fields.targetEmail, + ipAddress: fields.ipAddress, + userAgent: fields.userAgent, + requestPath: RECOVERY_PREFIX, + requestMethod: 'POST', + details: { + actorId: fields.actorId, + actorEmail: fields.actorEmail, + wasEnrolled: fields.wasEnrolled, + requireReenrolment: fields.requireReenrolment, + selfService: fields.actorId === fields.targetId, + }, + blocked: false, + }) + } catch (e) { + console.error('[two-factor] reset succeeded but the audit event could not be written', e) + } +} + +const twoFactorRecoveryRoutes = new Hono<{ Bindings: Bindings; Variables: Variables }>() + +// `/admin/*` is already gated globally by requireAuth + requireRbac('portal','access'). Both are +// asserted again locally: this is the one route in the plugin that acts on somebody ELSE's +// account, so its gate should be readable at the route rather than inferred from where it is +// mounted. `requireRole(['admin'])` matches how routes/admin-users.ts gates every other +// user-management action — a reset is user management, and reusing that gate means it is +// governed by the same role assignment operators already reason about. +twoFactorRecoveryRoutes.use('*', requireAuth()) +twoFactorRecoveryRoutes.use('*', requireRole(['admin'])) + +/** + * POST /admin/two-factor-reset — clear a user's second factor. + * + * Self-reset is permitted. Reaching this route already required passing the second factor being + * reset (or never having had one), so it grants nothing new, and it is the natural "I replaced my + * phone" flow. It is recorded with `selfService: true` so it is still distinguishable in the log. + * + * Deliberately does NOT revoke the target's existing sessions: a reset is a recovery action, not + * a containment action, and signing a colleague out mid-work to fix their lost phone would be a + * surprise. Containment is what deactivating the account is for. + */ +twoFactorRecoveryRoutes.post('/', async (c) => { + const actor = c.get('user')! + + const parsed = resetRequestSchema.safeParse(await c.req.json().catch(() => null)) + if (!parsed.success) { + return c.json({ error: parsed.error.issues[0]?.message ?? 'Invalid request' }, 400) + } + const { userId, confirmEmail, requireReenrolment } = parsed.data + + const target = await c.env.DB.prepare( + `SELECT u.id, u.email, u.two_factor_enabled AS enabled, + (SELECT tf.verified FROM auth_two_factor tf WHERE tf.user_id = u.id LIMIT 1) AS verified + FROM auth_user u + WHERE u.id = ?`, + ) + .bind(userId) + .first<{ id: string; email: string; enabled: number; verified: number | null }>() + + if (!target) { + return c.json({ error: 'User not found' }, 404) + } + + // Case- and whitespace-insensitive: the operator is retyping what the page showed them, and + // failing on a trailing space would only teach them to paste it, which defeats the check. + if (confirmEmail.trim().toLowerCase() !== target.email.trim().toLowerCase()) { + return c.json( + { error: 'The email you typed does not match this user. Nothing was changed.' }, + 400, + ) + } + + const wasEnrolled = target.verified === 1 || target.enabled === 1 + + try { + await resetUserTwoFactor(c.env.DB, target.id, requireReenrolment) + } catch (e) { + console.error('[two-factor] reset failed', e) + return c.json({ error: 'Failed to reset two-factor authentication' }, 500) + } + + await auditReset(c.env.DB, { + actorId: actor.userId, + actorEmail: actor.email, + targetId: target.id, + targetEmail: target.email, + wasEnrolled, + requireReenrolment, + ipAddress: c.req.header('cf-connecting-ip') ?? c.req.header('x-forwarded-for'), + userAgent: c.req.header('user-agent'), + }) + + const outcome: TwoFactorResetOutcome = { + email: target.email, + wasEnrolled, + requireReenrolment, + } + return c.json({ ok: true, ...outcome }) +}) + +/** + * Force a user who owes an enrolment back to the enrolment page. + * + * Without this, "reset" quietly means "downgrade to password-only": the user's enrolment is gone, + * nothing ever prompts them, and the account stays unprotected indefinitely while the admin who + * pressed the button believes they restored access to a 2FA-protected account. + * + * `two_factor_required` lives on `auth_user`, not `auth_two_factor`, precisely because the reset + * DELETEs that row — a flag stored there would be destroyed by the action that needs to set it. + * + * ── Deliberately fails OPEN ── + * Every failure mode here resolves to "let the request through": + * + * - a DB error → `next()`. This gate enforces a policy; it does not authenticate anybody. The + * session was already validated upstream by requireAuth. Failing closed would convert a + * transient D1 error into every admin being locked out of the portal, which is the exact + * class of outage this whole feature exists to prevent. + * - the plugin being deactivated → `next()`, checked FIRST. `/admin/two-factor` 404s when the + * plugin is off, so enforcing then would redirect the user in a loop to a page that cannot + * exist. Deactivating the plugin is the operator's own escape hatch from a bad required-flag. + * + * Mounted from app.ts alongside the other `/admin/*` middleware rather than from the plugin's + * `register()`, because Hono composes matched handlers in registration order: plugin registration + * runs interleaved with `app.route('/admin/...')` calls, so middleware added there would silently + * not run for the admin routes mounted before it. + */ +export function enforceTwoFactorEnrolment(): MiddlewareHandler<{ + Bindings: Bindings + Variables: Variables +}> { + return async (c, next) => { + const user = c.get('user') as { userId?: string } | undefined + if (!user?.userId) return next() + + const path = new URL(c.req.url).pathname + // The enrolment surface itself, or the user could never satisfy the requirement. Covers + // `/admin/two-factor` and `/admin/two-factor/qr`; the trailing-boundary check keeps it from + // also exempting `/admin/two-factor-reset`, which a user who owes an enrolment has no + // business reaching. + if (path === ENROLMENT_PATH || path.startsWith(`${ENROLMENT_PATH}/`)) return next() + + try { + if (!(await isPluginActive(c.env.DB, TWO_FACTOR_PLUGIN_ID))) return next() + if (!(await owesTwoFactorEnrolment(c.env.DB, user.userId))) return next() + } catch (e) { + console.error('[two-factor] enrolment enforcement check failed; allowing request', e) + return next() + } + + const accept = c.req.header('Accept') || '' + if (accept.includes('text/html')) { + // No explanatory query parameter: the enrolment page reads the same flag and renders the + // banner itself. A `?message=` would be attacker-controlled text on a security page — + // anyone could hand a colleague a link that claims their 2FA was reset. + return c.redirect(ENROLMENT_PATH) + } + return c.json( + { + error: 'Two-factor enrolment required', + message: 'Your administrator reset your two-factor authentication. Enrol again to continue.', + enrolmentPath: ENROLMENT_PATH, + }, + 403, + ) + } +} + +/** Better Auth's disable endpoint, as mounted under this app's `/auth` basePath. */ +export const BA_DISABLE_PATH = '/auth/two-factor/disable' + +/** + * Refuse `POST /auth/two-factor/disable` for an account where an admin has MANDATED a second + * factor. + * + * ── Why the redirect middleware is not enough ── + * `enforceTwoFactorEnrolment` is an access gate, not a write-block: it makes the portal + * unreachable while a demanded factor is missing, but it never sees this request. Two independent + * reasons — the path is `/auth/*`, not `/admin/*`, and `/admin/two-factor` (which hosts the + * disable form) has to stay exempt or nobody could ever complete an enrolment. + * + * So without this, a user told to enrol could enrol, walk back to the same page, and switch it + * off. They would be bounced to the enrolment page on their next admin request, so it is a loop + * rather than an escape — but in the interval the account is genuinely password-only: BA stops + * challenging (its after-hook reads `user.twoFactorEnabled`), the passwordless sign-in paths + * re-open (`hasVerifiedSecondFactor` keys off the row this deletes), and nothing gates `/api/*`. + * Meanwhile the admin who set the requirement has no signal, because a BA disable writes none of + * our audit events. + * + * ── Why here ── + * Same chokepoint and same reasoning as {@link guardPasswordlessSecondFactor}: the `/auth/*` + * catch-all in `app.ts` is our own code and runs ahead of `auth.handler()`, so the refusal lands + * before BA deletes anything. A BA `after` hook would fire after the write. + * + * ── Not an enumeration risk ── + * Unlike the passwordless guard, this answers a caller already authenticated AS the account in + * question, so a plain-language reason leaks nothing about anyone else — and the user needs to + * know why the button did not work. + * + * Fails OPEN, matching {@link enforceTwoFactorEnrolment}: `readPolicyState` resolves to "not + * required" on a DB error. A transient D1 fault should not permanently freeze a user's ability to + * manage their own second factor, and the admin reset remains available either way. + */ +export async function guardRequiredSecondFactorDisable( + c: Context<{ Bindings: { DB: D1Database }; Variables: { user?: { userId?: string } } }>, +): Promise { + if (c.req.method !== 'POST') return null + if (new URL(c.req.url).pathname !== BA_DISABLE_PATH) return null + + // Resolved by the session middleware in app.ts, which runs on `*` ahead of this catch-all. No + // session means BA will refuse the call itself (`sensitiveSessionMiddleware`), so there is + // nothing to protect and nothing to leak. + const user = c.get('user') + if (!user?.userId) return null + + if (!(await isTwoFactorRequired(c.env.DB, user.userId))) return null + + console.warn('[two-factor] disable refused — an administrator requires 2FA on this account') + return c.json( + { + error: 'Two-factor authentication is required on this account', + message: + 'An administrator requires two-factor authentication on your account, so it cannot be turned off. Contact an administrator if this needs to change.', + code: 'TWO_FACTOR_REQUIRED', + }, + 403, + ) +} + +export { twoFactorRecoveryRoutes } diff --git a/packages/core/src/plugins/core-plugins/two-factor-auth/routes.ts b/packages/core/src/plugins/core-plugins/two-factor-auth/routes.ts new file mode 100644 index 000000000..dfbe1c74b --- /dev/null +++ b/packages/core/src/plugins/core-plugins/two-factor-auth/routes.ts @@ -0,0 +1,255 @@ +/** + * two-factor-auth route handlers. + * + * Two mounts with deliberately different gating — see the plugin entrypoint for why: + * + * - `twoFactorAdminRoutes` → /admin/two-factor (session required, deactivate→404) + * - `twoFactorChallengeRoutes` → /auth/two-factor (NO session — that is the point) + * + * Neither mount proxies Better Auth. Enrolment and verification are browser-direct calls to + * `/auth/two-factor/*`, which the `/auth/*` catch-all in app.ts already serves. Wrapping them + * here would add a second place that has to know BA's request/response shapes, and BA sets + * signed cookies that a server-side hop would have to forward by hand. + */ +import { Hono } from 'hono' +import { setCookie } from 'hono/cookie' +import { requireAuth, AuthManager } from '../../../middleware' +import { getJwtExpirySecondsFromDb } from '../../../middleware/auth' +// Sourced from its defining module rather than the middleware barrel, matching api-docs: +// the barrel does not re-export invalidatePluginStatusCache, and keeping the status cache and +// its invalidator resolved through one module keeps them coherent under the test runner. +import { isPluginActive } from '../../../middleware/plugin-middleware' +import { getEnrolmentState, hasVerifiedSecondFactor } from '../../../auth/second-factor-guard' +import { isTwoFactorRequired } from './recovery' +import { TWO_FACTOR_PLUGIN_ID } from '../../../auth/two-factor-settings' +import { renderTwoFactorEnrolmentPage } from './components/enrolment-page' +import { renderTwoFactorChallengePage } from './components/challenge-page' +import type { Bindings, Variables } from '../../../app' + +const twoFactorAdminRoutes = new Hono<{ Bindings: Bindings; Variables: Variables }>() +const twoFactorChallengeRoutes = new Hono<{ Bindings: Bindings; Variables: Variables }>() + +/** Quiet zone the QR spec requires, in modules, on all four sides. */ +const QUIET_ZONE_MODULES = 4 + +/** + * CSS pixels per QR module — the number that decides whether a phone can actually read the code + * off a desktop screen. + * + * Below roughly 4 px/module, decoding a screen becomes unreliable on ordinary phone cameras at a + * natural distance. 6 leaves headroom for the things we cannot measure from here: a + * standard-density monitor, browser zoom under 100%, and a camera held at arm's length. + * + * Raise this before touching `ecl` or the quiet zone if a scan ever fails — it is the variable + * with the most direct effect and it costs only layout width. + */ +const CSS_PX_PER_MODULE = 6 + +// Defense-in-depth: `/admin/*` is already globally gated by requireAuth + +// requireRbac('portal','access'), but assert auth locally so the sub-app is safe if remounted +// and the intent is explicit at the plugin boundary. +twoFactorAdminRoutes.use('*', requireAuth()) + +// Deactivate→404 gate. SonicJS mounts plugin routes unconditionally and only hides the sidebar +// entry when a plugin is deactivated, so routes stay reachable by direct URL. Uses the BOOLEAN +// isPluginActive (not requireActivePlugin, which throws and becomes a 500 via onError). +// +// Best-effort across warm isolates: isPluginActive caches per isolate with no TTL and +// invalidation only clears the isolate that toggled. Acceptable here — this gate covers the +// enrolment SURFACE only. Verification is never gated on plugin status (auth/config.ts). +twoFactorAdminRoutes.use('*', async (c, next) => { + if (!(await isPluginActive(c.env.DB, TWO_FACTOR_PLUGIN_ID))) { + return c.notFound() + } + return next() +}) + +/** + * Enrolment page. + * + * NOT permission-gated beyond authentication: every authenticated user manages their OWN second + * factor, and the BA endpoints this page calls resolve the user from the session — there is no + * arbitrary user id to pass. + * + * The policy SETTINGS surface is separate and administrative: it lives on the generic + * schema-driven form at `/admin/plugins/two-factor-auth/configure`, which gates on + * `user?.role !== 'admin'` (routes/admin-plugins.ts). The `two-factor:manage` permission the + * manifest declares is registered for the plugins UI and is NOT currently enforced by any + * handler — the role check is what actually gates it. + */ +twoFactorAdminRoutes.get('/', async (c) => { + // requireAuth() above guarantees this. + const user = c.get('user')! + const [state, required] = await Promise.all([ + getEnrolmentState(c.env.DB, user.userId), + // The RAW policy flag, not "owes an enrolment". The page needs both meanings and derives them + // from this plus `verified`: the amber banner is `required && !verified`, while hiding the + // disable form is `required` alone — a user who has satisfied the requirement still may not + // turn it back off. + isTwoFactorRequired(c.env.DB, user.userId), + ]) + + return c.html( + renderTwoFactorEnrolmentPage({ + verified: state.verified, + pending: state.enrolled && !state.verified, + required, + user: { name: user.email, email: user.email, role: user.role }, + version: c.get('appVersion'), + dynamicMenuItems: c.get('pluginMenuItems'), + }), + ) +}) + +/** + * POST /admin/two-factor/qr — render an `otpauth://` URI as a scannable QR code. + * + * The enrolment page originally shipped the URI as a link plus the secret as selectable text and + * no QR, on the reasoning that every authenticator supports manual entry. That reasoning does not + * survive contact with the actual flow: the admin panel is used on a DESKTOP and the authenticator + * lives on a PHONE, so an `otpauth://` link has no handler to open and the only path left is + * hand-typing a 32-character base32 secret across devices. A QR is the only practical way to get + * the account into the app. + * + * Server-side because these pages have no client bundler — the alternative was a CDN script tag on + * the page that handles TOTP secrets. `qrcode-svg` is already a core dependency, pure JS, and safe + * on Workers (no canvas, no Node built-ins). + * + * The URI is posted back rather than derived here because only the browser holds it: Better Auth + * returns it once, from `/auth/two-factor/enable`, and never stores it in plaintext. + * + * Behind the same `requireAuth()` + deactivate→404 gates as the page (the `use('*')` pair above). + * The caller sends `X-CSRF-Token` like every other admin POST, but note that CSRF validation is + * currently inert app-wide — csrfProtection exempts requests without an `auth_token` cookie and + * sign-in mints a Better Auth session cookie instead. `requireAuth()` is what actually protects + * this route today. Rendering a QR is a read, so the exposure is bounded; the fix belongs in + * csrf.ts, not here. + */ +twoFactorAdminRoutes.post('/qr', async (c) => { + const body = await c.req.json().catch(() => null) + const uri = (body as { uri?: unknown } | null)?.uri + + // Pin the input to a TOTP enrolment URI. Without this the endpoint is a generic + // "render any text I give you as a QR code, from your origin" service — which is a phishing + // primitive, since a QR is unreadable to the human deciding whether to trust it. + if (typeof uri !== 'string' || uri.length > 512 || !/^otpauth:\/\/totp\//.test(uri)) { + return c.json({ error: 'Expected an otpauth://totp/ URI' }, 400) + } + + const { default: QRCode } = await import('qrcode-svg') + const qr = new QRCode({ + content: uri, + // 4 modules is the quiet zone the QR spec requires. The surrounding element's white padding + // is NOT a substitute — a scanner sees the screen, not our box model, and anything less than + // 4 is where "the code just won't scan on some phones" comes from. + padding: QUIET_ZONE_MODULES, + // Coordinate space only. `container: 'svg-viewbox'` emits a viewBox and NO width/height on the + // , so this number sets the units the path is drawn in and has no effect on the size the + // browser paints — that comes from the width/height attributes added below. + width: 256, + height: 256, + // Medium recovery: enough redundancy for a phone camera on a screen, without inflating + // the module count for a URI this long. + ecl: 'M', + // `join` merges the per-module rects into one path — ~190KB down to ~56KB on a long URI. + join: true, + container: 'svg-viewbox', + }) + + // ── Why the rendered size is computed, not a constant ── + // Scannability is governed by CSS pixels PER MODULE, not by the overall size, and the module + // count grows with the URI. `issuer` is operator-configurable up to 64 chars + // (auth/two-factor-settings.ts) and Better Auth puts it in the URI TWICE — once in the label, + // once as a parameter — so a long issuer plus a long email reaches ~285 chars and a 69-module + // symbol, versus 49 modules for the shipped 'SonicJS' default. + // + // A fixed size therefore silently degrades: at the 236 CSS px this shipped with, a default + // install got 4.1 px/module and a 64-char issuer got 3.1 — under the ~4 px/module floor where + // phone cameras stop decoding screens reliably. The failure is invisible to every assertion in + // qr.test.ts, because the SVG is perfectly well-formed either way. + // + // Deriving the size from the symbol keeps the density fixed instead, so no issuer or email + // length can push it under the floor. + const modulesAcross = qr.qrcode.moduleCount + QUIET_ZONE_MODULES * 2 + const renderPx = modulesAcross * CSS_PX_PER_MODULE + + const svg = qr + .svg() + // Strip the XML prolog: valid in a standalone .svg file, meaningless inline in HTML. + .replace(/^<\?xml[^>]*\?>\s*/, '') + // Pin the painted size on the element itself. Without width/height a viewBox-only is + // sized entirely by its container, which is what made the density a CSS detail that no test + // could see. The viewBox is retained, so this is a uniform scale. + .replace(' { + return c.html(renderTwoFactorChallengePage()) +}) + +/** + * POST /auth/two-factor/complete — bring a just-challenged session up to the full SonicJS shape. + * + * Better Auth's `verify-totp` / `verify-backup-code` set only `better-auth.session_token`. That is + * enough for `requireAuth()` (app.ts resolves `c.get('user')` from the BA session), but it leaves + * a 2FA user's session strictly WEAKER than a password-login session in two ways: + * + * 1. `csrfProtection` treats a request with no `auth_token` cookie as token-authenticated and + * skips validation entirely (middleware/csrf.ts). So every admin POST for the rest of that + * session would go unvalidated — the users who opted into the strongest authentication would + * get the weakest CSRF posture. BA's session cookie is `SameSite=Lax`, which stops a pure + * cross-site POST but not a same-site attacker, which is the case CSRF tokens exist for. + * 2. No JWT, so API/Bearer callers behave differently after a 2FA login than after a password + * login. + * + * Rather than loosening the global CSRF rule (that would start requiring tokens from every + * BA-session-only client, including the E2E helper), this mints exactly what `POST /auth/login` + * mints, so the two sign-in paths converge. + * + * Restricted to callers who actually hold a verified second factor — the population this exists + * for. It grants nothing a password login would not: the JWT is derived from the session's own + * user, never from request input. + */ +twoFactorChallengeRoutes.post('/complete', requireAuth(), async (c) => { + const user = c.get('user') + if (!user) return c.json({ error: 'Authentication required' }, 401) + + if (!(await hasVerifiedSecondFactor(c.env.DB, user.userId))) { + // No enrolment => nothing was challenged => nothing to upgrade. + return c.json({ error: 'No second factor on this account' }, 400) + } + + const tokenTtl = await getJwtExpirySecondsFromDb(c.env.DB, c.env) + const token = await AuthManager.generateToken( + user.userId, + user.email, + user.role ?? 'viewer', + c.env.JWT_SECRET, + tokenTtl, + ) + const isDev = c.env.ENVIRONMENT === 'development' || !c.env.ENVIRONMENT + // Same attributes as POST /auth/login's auth_token. + setCookie(c, 'auth_token', token, { + httpOnly: true, + secure: !isDev, + sameSite: 'Strict', + path: '/', + maxAge: tokenTtl, + }) + return c.json({ ok: true }) +}) + +export { twoFactorAdminRoutes, twoFactorChallengeRoutes } diff --git a/packages/core/src/plugins/manifest-registry.ts b/packages/core/src/plugins/manifest-registry.ts index ac3afbcfc..3e88efed0 100644 --- a/packages/core/src/plugins/manifest-registry.ts +++ b/packages/core/src/plugins/manifest-registry.ts @@ -2,7 +2,7 @@ * Plugin Registry - AUTO-GENERATED * * Generated by: packages/scripts/generate-plugin-registry.mjs - * Generated at: 2026-07-02T23:43:29.620Z + * Generated at: 2026-07-29T22:30:20.557Z * Source: All manifest.json files in src/plugins/ * * DO NOT EDIT MANUALLY - run the generator script instead. @@ -793,6 +793,35 @@ export const PLUGIN_REGISTRY: Record = { } }, + 'two-factor-auth': { + "id": "two-factor-auth", + "codeName": "two-factor-auth", + "displayName": "Two-Factor Authentication", + "description": "Time-based one-time passwords (TOTP) with single-use backup codes and per-account second-factor lockout.", + "version": "1.0.0", + "author": "SonicJS Team", + "category": "security", + "iconEmoji": "🔐", + "is_core": true, + "defaultActive": true, + "permissions": [ + "two-factor:manage" + ], + "dependencies": [], + "defaultSettings": { + "issuer": "SonicJS", + "maxFailedAttempts": 5, + "lockoutDurationSeconds": 900, + "backupCodeCount": 10 + }, + "adminMenu": { + "label": "Two-Factor Auth", + "icon": "lock-closed", + "path": "/admin/two-factor", + "order": 86 + } + }, + 'user-profiles': { "id": "user-profiles", "codeName": "user-profiles", diff --git a/packages/core/src/routes/admin-users.ts b/packages/core/src/routes/admin-users.ts index c6d2406f7..cc03ebbe7 100644 --- a/packages/core/src/routes/admin-users.ts +++ b/packages/core/src/routes/admin-users.ts @@ -75,7 +75,7 @@ userRoutes.get('/profile', async (c) => { // Get user profile data const userStmt = db.prepare(` SELECT id, email, first_name, last_name, phone, bio, avatar, - timezone, language, theme, email_notifications, 0 as two_factor_enabled, + timezone, language, theme, email_notifications, two_factor_enabled, role, created_at, last_login_at FROM auth_user WHERE id = ? AND is_active = 1 @@ -500,7 +500,7 @@ userRoutes.get('/users', async (c) => { const usersStmt = db.prepare(` SELECT u.id, u.email, u.first_name, u.last_name, u.role, u.avatar, u.created_at, u.last_login_at, u.updated_at, - u.email_verified, 0 as two_factor_enabled, u.is_active + u.email_verified, u.two_factor_enabled, u.is_active FROM auth_user u ${whereClause} ORDER BY u.created_at DESC @@ -808,7 +808,7 @@ userRoutes.get('/users/:id', async (c) => { // Get user data (including inactive users for admin access) const userStmt = db.prepare(` SELECT id, email, first_name, last_name, phone, bio, avatar, - role, is_active, email_verified, 0 as two_factor_enabled, created_at, last_login_at + role, is_active, email_verified, two_factor_enabled, created_at, last_login_at FROM auth_user WHERE id = ? `) @@ -861,9 +861,13 @@ userRoutes.get('/users/:id/edit', async (c) => { try { // Get user data (removed bio - now in profile) + // two_factor_required (migration 0004) drives the Two-Factor Recovery panel. Safe to name + // unconditionally: ensureTwoFactorRequiredColumn() adds it from both the bootstrap path and + // the two-factor plugin's onBoot, which runs on every isolate. const userStmt = db.prepare(` SELECT id, email, first_name, last_name, phone, avatar, - role, is_active, email_verified, 0 as two_factor_enabled, created_at, last_login_at + role, is_active, email_verified, two_factor_enabled, two_factor_required, + created_at, last_login_at FROM auth_user WHERE id = ? `) @@ -905,6 +909,7 @@ userRoutes.get('/users/:id/edit', async (c) => { isActive: Boolean(userToEdit.is_active), emailVerified: Boolean(userToEdit.email_verified), twoFactorEnabled: Boolean(userToEdit.two_factor_enabled), + twoFactorRequired: Boolean(userToEdit.two_factor_required), createdAt: userToEdit.created_at, lastLoginAt: userToEdit.last_login_at, profile diff --git a/packages/core/src/routes/auth.ts b/packages/core/src/routes/auth.ts index d47105743..b62fb499c 100644 --- a/packages/core/src/routes/auth.ts +++ b/packages/core/src/routes/auth.ts @@ -314,6 +314,31 @@ authRoutes.post('/login', await setCsrfCookie(c) const baBody = await baRes.json() as any + + // Second factor pending. Better Auth answers a 2FA challenge with HTTP **200** and a body + // of `{twoFactorRedirect:true}` — no `user`, no `token`, and it deletes the session it had + // just created. So `baRes.ok` above is TRUE and this is NOT a credential failure. + // + // This must be handled before the JWT mint below. `baBody.user` is absent here, so + // `generateToken(undefined, undefined, 'viewer', …)` would sign a token for a + // non-existent principal, set it as `auth_token`, and hand it back as `token` — and + // app.ts's Bearer-JWT fallback would then populate `c.get('user')` with + // `{userId: undefined}`, which `requireAuth()` accepts because the object is truthy. + // + // 200, not 401: the credentials were correct, and a 401 would teach API clients to + // re-prompt for the password. BA's signed challenge cookie has already been forwarded onto + // this response by the Set-Cookie loop above, which is what /auth/two-factor/verify-totp + // needs in order to resolve the challenge. + if (baBody?.twoFactorRedirect === true) { + return c.json({ + twoFactorRequired: true, + // BA only ever reports 'totp'/'otp' here — never 'backup_code'. A client must offer + // backup-code entry unconditionally rather than keying off this list. + twoFactorMethods: baBody.twoFactorMethods ?? [], + redirectTo: '/auth/two-factor', + }) + } + const user = baBody.user ?? {} // Mint a JWT so API callers can use Bearer token auth (same as /register) @@ -699,6 +724,30 @@ authRoutes.post('/login/form', await setCsrfCookie(c) + // Second factor pending — see the sibling branch in POST /auth/login for the full note. + // The password was CORRECT and `baRes.ok` is true, so without this branch the handler + // reports "Login successful! Redirecting…" and sends the browser to /admin/content with no + // session, which bounces straight back to the login page. BA's signed challenge cookie is + // already on this response; send the browser to the challenge page instead. + const baChallengeBody = (await baRes + .clone() + .json() + .catch(() => null)) as { twoFactorRedirect?: boolean } | null + if (baChallengeBody?.twoFactorRedirect === true) { + const isHtmxChallenge = c.req.header('HX-Request') === 'true' + if (isHtmxChallenge) { + c.header('HX-Redirect', '/auth/two-factor') + } + return c.html(html` +
+

Password accepted. Redirecting for two-step verification…

+ +
+ `) + } + if (email === DEMO_EMAIL) { c.executionCtx?.waitUntil(trackDemoLogin(c.env.CACHE_KV)) } diff --git a/packages/core/src/services/migrations.test.ts b/packages/core/src/services/migrations.test.ts index b03db45ee..f53e932cb 100644 --- a/packages/core/src/services/migrations.test.ts +++ b/packages/core/src/services/migrations.test.ts @@ -102,7 +102,11 @@ describe('MigrationService', () => { const service = new MigrationService(db as any) const migrations = await service.getAvailableMigrations() - expect(migrations.map(m => m.id)).toEqual(['0001', '0002', '0003', '0004']) + // The greenfield inventory. 0006 (auth_two_factor lockout columns) and 0007 + // (auth_user.two_factor_required) are ALTERs rather than edits to 0001, because D1 tracks + // applied migrations by filename — an edit to 0001 would reach greenfield installs only. + // 0005 is deliberately skipped here — reserved for the FTS5 search PR, still in flight. + expect(migrations.map(m => m.id)).toEqual(['0001', '0002', '0003', '0004', '0006', '0007']) expect(migrations.find(m => m.id === '029')).toBeUndefined() expect(db._mocks.prepare).not.toHaveBeenCalledWith(expect.stringContaining('CREATE TABLE IF NOT EXISTS migrations')) }) @@ -133,6 +137,8 @@ describe('MigrationService', () => { { name: '0002_documents.sql', applied_at: '2026-01-01T00:00:01.000Z' }, { name: '0003_session_org.sql', applied_at: '2026-01-01T00:00:02.000Z' }, { name: '0004_forms.sql', applied_at: '2026-01-01T00:00:03.000Z' }, + { name: '0006_two_factor_lockout.sql', applied_at: '2026-01-01T00:00:04.000Z' }, + { name: '0007_two_factor_required.sql', applied_at: '2026-01-01T00:00:05.000Z' }, ], existingTables: ['users', 'documents', 'document_types'], existingColumns: [] @@ -141,9 +147,9 @@ describe('MigrationService', () => { const service = new MigrationService(db as any) const status = await service.getMigrationStatus() - expect(status.appliedMigrations).toBe(4) + expect(status.appliedMigrations).toBe(6) expect(status.pendingMigrations).toBe(0) - expect(status.lastApplied).toBe('2026-01-01T00:00:03.000Z') + expect(status.lastApplied).toBe('2026-01-01T00:00:05.000Z') }) }) }) diff --git a/packages/core/src/services/migrations.ts b/packages/core/src/services/migrations.ts index 2aedf8870..bc5371cad 100644 --- a/packages/core/src/services/migrations.ts +++ b/packages/core/src/services/migrations.ts @@ -96,6 +96,8 @@ export class MigrationService { if (await this.checkTablesExist(['documents'])) { await this.ensureDocumentGeneratedColumns() } + await ensureTwoFactorLockoutColumns(this.db) + await ensureTwoFactorRequiredColumn(this.db) } /** @@ -246,3 +248,77 @@ export class MigrationService { } } } + +/** + * Ensure `auth_two_factor` carries the two second-factor lockout columns. + * + * Migration 0006 adds them; this is the runtime safety net for a DB that has 0001 (which creates + * the table without them) but never got 0006 — a partially-migrated preview, or a deploy where + * `wrangler d1 migrations apply` was skipped. Without the columns, Better Auth's + * `/two-factor/enable` INSERT names `failed_verification_count` (drizzle emits every declared + * column) and hard-fails, so an operator would see enrolment 500 rather than a schema error they + * can act on. + * + * Module-level and exported, NOT a private method, because it needs two callers: + * - `MigrationService.ensureSchemaCompatibility()` — the bootstrap path, which the bootstrap + * middleware SKIPS entirely once the `_sonicjs_bootstrap_` KV marker is set (24h + * TTL). On its own, that would leave the repair unrun for up to a day on most cold isolates. + * - the two-factor plugin's `onBoot`, which runs on EVERY isolate via app.ts's `boot()`. + * + * Same shape as `ensureDocumentGeneratedColumns`: `table_xinfo` probe + ALTER, fully idempotent, + * silent when there is nothing to do, and non-fatal on error (bootstrap must not fail on a repair). + */ +export async function ensureTwoFactorLockoutColumns(db: D1Database): Promise { + try { + const exists = await db + .prepare(`SELECT name FROM sqlite_master WHERE type='table' AND name='auth_two_factor'`) + .first() + if (!exists) return + const info = await db.prepare(`PRAGMA table_xinfo('auth_two_factor')`).all<{ name: string }>() + const present = new Set((info.results ?? []).map((r) => r.name)) + // Column types mirror migration 0006 exactly — see that file for why + // failed_verification_count is NOT NULL DEFAULT 0 and locked_until is INTEGER (ms). + const wanted: Array<[string, string]> = [ + ['failed_verification_count', 'INTEGER NOT NULL DEFAULT 0'], + ['locked_until', 'INTEGER'], + ] + for (const [column, definition] of wanted) { + if (present.has(column)) continue + await db.prepare(`ALTER TABLE auth_two_factor ADD COLUMN ${column} ${definition}`).run() + console.log(`[MigrationService] Self-healed auth_two_factor.${column}`) + } + } catch (error) { + console.error('[MigrationService] auth_two_factor lockout column repair failed:', error) + } +} + +/** + * Runtime safety net for `auth_user.two_factor_required` (migration 0007). + * + * Same rationale and same callers as {@link ensureTwoFactorLockoutColumns}: a DB that has 0001 but + * never got 0007 would otherwise fail every admin-portal request, because the enrolment-enforcement + * middleware SELECTs this column on each one. A missing column there is a 500 on every page rather + * than a schema error anyone can act on. + * + * Separate from the lockout repair because the two touch different tables and either can be missing + * independently. Idempotent, silent when there is nothing to do, non-fatal on error. + */ +export async function ensureTwoFactorRequiredColumn(db: D1Database): Promise { + try { + const exists = await db + .prepare(`SELECT name FROM sqlite_master WHERE type='table' AND name='auth_user'`) + .first() + if (!exists) return + const info = await db.prepare(`PRAGMA table_xinfo('auth_user')`).all<{ name: string }>() + const present = new Set((info.results ?? []).map((r) => r.name)) + if (present.has('two_factor_required')) return + // Mirrors migration 0007 exactly. DEFAULT 0 matters: existing users must not become + // retroactively locked out of the portal by the column appearing. + await db + .prepare(`ALTER TABLE auth_user ADD COLUMN two_factor_required INTEGER NOT NULL DEFAULT 0`) + .run() + console.log('[MigrationService] Self-healed auth_user.two_factor_required') + } catch (error) { + console.error('[MigrationService] auth_user.two_factor_required repair failed:', error) + } +} diff --git a/packages/core/src/services/plugin-service.ts b/packages/core/src/services/plugin-service.ts index e8e5cb26e..e9719b286 100644 --- a/packages/core/src/services/plugin-service.ts +++ b/packages/core/src/services/plugin-service.ts @@ -1,6 +1,7 @@ import type { D1Database } from '@cloudflare/workers-types' import { invalidateTenantCache } from '../middleware/tenant' import { invalidatePluginStatusCache } from '../middleware/plugin-middleware' +import { TWO_FACTOR_PLUGIN_ID, refreshTwoFactorPolicy } from '../auth/two-factor-settings' export interface PluginData { id: string @@ -212,6 +213,15 @@ export class PluginService { `).bind(JSON.stringify(settings), now, pluginId, TYPE_ID, TENANT).run() // Multi-tenant resolver settings (header name, subdomain config) live in plugin settings. invalidateTenantCache() + // The two-factor policy (issuer, lockout thresholds, backup-code count) is snapshotted + // onto the Better Auth plugin options at construction time, so createAuth reads it from a + // module-level cache synchronously. Re-read it here — a plain invalidation would leave this + // isolate on the defaults, which is worse than stale. + if (pluginId === TWO_FACTOR_PLUGIN_ID) { + await refreshTwoFactorPolicy(this.db).catch((e) => + console.error('[plugin-service] two-factor policy refresh failed', e) + ) + } await this.logActivity(pluginId, 'settings_updated', null) } diff --git a/packages/core/src/templates/pages/admin-profile.template.ts b/packages/core/src/templates/pages/admin-profile.template.ts index 6d2c7ac63..dc94c0280 100644 --- a/packages/core/src/templates/pages/admin-profile.template.ts +++ b/packages/core/src/templates/pages/admin-profile.template.ts @@ -309,16 +309,18 @@ export function renderProfilePage(data: ProfilePageData): string { Change Password - + ${data.profile.two_factor_enabled ? 'Manage' : 'Enable'} 2FA + @@ -410,11 +412,6 @@ export function renderProfilePage(data: ProfilePageData): string { document.getElementById('password-form').reset(); } - function toggle2FA() { - // TODO: Implement 2FA toggle - alert('Two-factor authentication setup coming soon!'); - } - // Close modal on escape key document.addEventListener('keydown', function(e) { if (e.key === 'Escape' && !document.getElementById('password-modal').classList.contains('hidden')) { diff --git a/packages/core/src/templates/pages/admin-user-edit.template.ts b/packages/core/src/templates/pages/admin-user-edit.template.ts index f552ec918..a31b3ea3f 100644 --- a/packages/core/src/templates/pages/admin-user-edit.template.ts +++ b/packages/core/src/templates/pages/admin-user-edit.template.ts @@ -18,6 +18,12 @@ export interface UserEditData { isActive: boolean emailVerified: boolean twoFactorEnabled: boolean + /** + * The user owes an enrolment — set by an admin reset. Independent of `twoFactorEnabled`: + * `required && !enabled` is what forces the redirect to /admin/two-factor, and + * `required && enabled` means enrolled and not allowed to turn it off. + */ + twoFactorRequired: boolean createdAt: number lastLoginAt?: number profile?: UserProfileData @@ -44,6 +50,91 @@ export interface UserEditPageData { } } +/** + * Break-glass panel for a user whose authenticator is gone. + * + * The action is only offered when there is something to clear (`enabled`) or something to + * release (`required`). Rendering a destructive control on every user row would be noise, and + * on an unenrolled account it does nothing worth a button. + * + * Presented as a distinct amber panel rather than inside the Danger Zone: this is a recovery + * action taken to GIVE someone access, and burying it beside "Delete User" is how an admin + * ends up not finding it at 2am — which is the entire scenario it exists for. + */ +function renderTwoFactorRecoverySection(u: UserEditData): string { + const uid = escapeHtml(u.id) + const email = escapeHtml(u.email) + + const state = u.twoFactorEnabled + ? u.twoFactorRequired + ? { label: 'Enrolled — required', tone: 'blue', detail: 'This user has a second factor and may not turn it off.' } + : { label: 'Enrolled', tone: 'blue', detail: 'This user has a working second factor.' } + : u.twoFactorRequired + ? { label: 'Re-enrolment pending', tone: 'amber', detail: 'This user must set up a second factor before they can use the admin portal.' } + : { label: 'Not enrolled', tone: 'zinc', detail: 'This user signs in with a password only.' } + + const badgeTone: Record = { + blue: 'bg-blue-50 dark:bg-blue-500/10 text-blue-700 dark:text-blue-400 ring-blue-700/10 dark:ring-blue-500/20', + amber: 'bg-amber-50 dark:bg-amber-500/10 text-amber-700 dark:text-amber-400 ring-amber-700/10 dark:ring-amber-500/20', + zinc: 'bg-zinc-50 dark:bg-zinc-500/10 text-zinc-600 dark:text-zinc-400 ring-zinc-700/10 dark:ring-zinc-500/20', + } + + const action = u.twoFactorEnabled || u.twoFactorRequired ? ` +
+

+ Resetting removes this user's second factor and any active lockout, letting them sign in + with their password alone. It does not change their password. Every reset is recorded in + the security audit log. +

+ + + + +
+ + +
+ + + + +
` : '' + + return ` +
+

Two-Factor Recovery

+
+ ${state.label} +
+

${state.detail}

+ ${action} +
+` +} + function renderTenantMembershipsSection( userId: string, tm?: UserEditPageData['tenantMemberships'], @@ -391,6 +482,8 @@ export function renderUserEditPage(data: UserEditPageData): string { + ${renderTwoFactorRecoverySection(data.userToEdit)} +

Danger Zone

@@ -433,6 +526,58 @@ export function renderUserEditPage(data: UserEditPageData): string { ${renderTenantMembershipsSection(data.userToEdit.id, data.tenantMemberships)}