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..3acefb52c --- /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 0006 (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 0006. */ + 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..e4d3c0ea3 --- /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/106-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..37b534edb --- /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/106-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/migrations-bundle.ts b/packages/core/src/db/migrations-bundle.ts index e9fa92d16..f1293b83c 100644 --- a/packages/core/src/db/migrations-bundle.ts +++ b/packages/core/src/db/migrations-bundle.ts @@ -1,7 +1,7 @@ /** * AUTO-GENERATED FILE - DO NOT EDIT * Generated by: scripts/generate-migrations.ts - * Generated at: 2026-08-10T20:47:30.547Z + * Generated at: 2026-08-18T03:32:49.645Z * * This file contains all migration SQL bundled for use in Cloudflare Workers * where filesystem access is not available at runtime. @@ -43,6 +43,20 @@ export const bundledMigrations: BundledMigration[] = [ filename: '0004_forms.sql', description: 'Migration 0004: Forms', sql: "-- Migration 0004: Forms and form_submissions tables\n-- Provides the forms plugin with its storage layer.\n\nCREATE TABLE IF NOT EXISTS forms (\n id TEXT PRIMARY KEY,\n name TEXT NOT NULL UNIQUE,\n display_name TEXT NOT NULL,\n description TEXT,\n category TEXT NOT NULL DEFAULT 'general',\n formio_schema TEXT NOT NULL,\n settings TEXT,\n is_active INTEGER NOT NULL DEFAULT 1,\n is_public INTEGER NOT NULL DEFAULT 1,\n managed INTEGER NOT NULL DEFAULT 0,\n icon TEXT,\n color TEXT,\n tags TEXT,\n submission_count INTEGER NOT NULL DEFAULT 0,\n view_count INTEGER NOT NULL DEFAULT 0,\n created_by TEXT REFERENCES auth_user(id),\n updated_by TEXT REFERENCES auth_user(id),\n created_at INTEGER NOT NULL,\n updated_at INTEGER NOT NULL\n);\n\nCREATE TABLE IF NOT EXISTS form_submissions (\n id TEXT PRIMARY KEY,\n form_id TEXT NOT NULL REFERENCES forms(id) ON DELETE CASCADE,\n submission_data TEXT NOT NULL,\n status TEXT NOT NULL DEFAULT 'pending',\n submission_number INTEGER,\n user_id TEXT REFERENCES auth_user(id),\n user_email TEXT,\n ip_address TEXT,\n user_agent TEXT,\n referrer TEXT,\n utm_source TEXT,\n utm_medium TEXT,\n utm_campaign TEXT,\n reviewed_by TEXT REFERENCES auth_user(id),\n reviewed_at INTEGER,\n review_notes TEXT,\n is_spam INTEGER NOT NULL DEFAULT 0,\n is_archived INTEGER NOT NULL DEFAULT 0,\n content_id TEXT,\n submitted_at INTEGER NOT NULL,\n updated_at INTEGER NOT NULL\n);\n\nCREATE INDEX IF NOT EXISTS idx_form_submissions_form_id ON form_submissions(form_id);\nCREATE INDEX IF NOT EXISTS idx_form_submissions_status ON form_submissions(status);\nCREATE INDEX IF NOT EXISTS idx_forms_category ON forms(category);\nCREATE INDEX IF NOT EXISTS idx_forms_is_active ON forms(is_active);\n" + }, + { + id: '0006', + name: 'Two Factor Lockout', + filename: '0006_two_factor_lockout.sql', + description: 'Migration 0006: Two Factor Lockout', + sql: "-- Migration 0006: Two-factor second-factor lockout columns\n--\n-- `auth_two_factor` already ships in 0001_core.sql, but only with the four columns Better\n-- Auth's twoFactor plugin needs to STORE an enrolment (secret / backup_codes / user_id /\n-- verified). It is missing the two columns the plugin writes on every VERIFY once\n-- `accountLockout` is enabled, so composing the plugin against the 0001 shape fails at the\n-- first `/two-factor/enable` (BA fills schema defaults on create, so the INSERT already\n-- names failed_verification_count).\n--\n-- Deliberately an ALTER in its own migration rather than an edit to 0001: D1 tracks applied\n-- migrations by FILENAME in `d1_migrations`, so an edit to 0001 would only reach greenfield\n-- installs and silently skip every DB that already ran it. As an ALTER, greenfield and\n-- already-migrated installs converge on the same shape.\n--\n-- There is also a runtime self-heal for these two columns in\n-- `MigrationService.ensureSchemaCompatibility()` (PRAGMA table_xinfo + ALTER, the same D45\n-- pattern used for the documents `q_*` columns). Belt and braces: a deployment that never\n-- ran this migration would otherwise 500 on enrolment instead of repairing itself.\n\n-- Consecutive failed second-factor verifications.\n--\n-- NOT NULL DEFAULT 0 is load-bearing, not cosmetic. BA compiles the lockout bump to\n-- `failed_verification_count = failed_verification_count + 1` (incrementOne), and\n-- `NULL + 1` is NULL, which verify-two-factor.mjs then reads back through `?? 0` as zero —\n-- forever. A nullable column here means the lockout silently never trips.\nALTER TABLE auth_two_factor ADD COLUMN failed_verification_count INTEGER NOT NULL DEFAULT 0;\n\n-- When the per-account second-factor lockout expires.\n--\n-- INTEGER (milliseconds), NOT the TEXT/ISO the sibling Infowall port uses. `lockedUntil` is\n-- a Better Auth `date` field, and its handling depends on the ADAPTER: the kysely adapter\n-- sets `supportsDates: false`, so BA stringifies to ISO before the write. SonicJS is on the\n-- **drizzle** adapter (better-auth-cloudflare → drizzleAdapter, provider 'sqlite'), which\n-- leaves `supportsDates` at its `true` default, so BA hands drizzle a real `Date` and the\n-- column mode does the conversion. Declared here to match\n-- `authTwoFactor.lockedUntil = integer('locked_until', { mode: 'timestamp_ms' })` in\n-- db/schema.ts — the same declaration auth_session.expires_at already uses.\nALTER TABLE auth_two_factor ADD COLUMN locked_until INTEGER;\n" + }, + { + id: '0007', + name: 'Two Factor Required', + filename: '0007_two_factor_required.sql', + description: 'Migration 0007: Two Factor Required', + sql: "-- Migration 0007: force re-enrolment after an administrative two-factor reset.\n--\n-- When an admin resets a locked-out user (lost phone AND lost backup codes — the only recovery\n-- path this feature otherwise has is direct database access), that user is left with NO second\n-- factor. Without a way to demand they set it up again, \"reset\" silently becomes \"permanently\n-- downgrade\", because nothing ever prompts them and the account quietly stays password-only.\n--\n-- `two_factor_required` is that demand. Set it and the user is redirected to /admin/two-factor and\n-- cannot use the rest of the admin portal until they enrol. It is INDEPENDENT of\n-- `two_factor_enabled`:\n--\n-- required=0, enabled=0 → optional, not enrolled (the default)\n-- required=0, enabled=1 → enrolled voluntarily\n-- required=1, enabled=0 → MUST enrol before using the portal ← what a reset leaves behind\n-- required=1, enabled=1 → enrolled, and may not turn it off\n--\n-- Kept on auth_user rather than auth_two_factor because it has to outlive the reset: the reset\n-- DELETEs the auth_two_factor row, so a flag stored there would be destroyed by the very action\n-- that needs to set it.\n--\n-- NOT NULL DEFAULT 0 so every existing row is \"not required\" — enabling this feature must never\n-- retroactively lock out an existing user.\nALTER TABLE auth_user ADD COLUMN two_factor_required INTEGER NOT NULL DEFAULT 0;\n" } ] diff --git a/packages/core/src/db/schema.ts b/packages/core/src/db/schema.ts index fb815316d..1dc7b1a92 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 0007). 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 0006: 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..f66dac309 --- /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 0006), 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 0007 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..5a2a440d3 --- /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 0006 and 0007 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..90eb3754d --- /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 0006 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..914f11074 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 0007) 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)}