feat(auth): two-factor authentication (TOTP + backup codes)

.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

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` |

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;

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;

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;

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;

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<string, unknown> = {}
+  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('<html></html>', { 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('<svg') && icon.includes('</svg>')
+}
+
+/** 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)
+  })
+})

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)
+    })
   })
 })