From be8081c3b408307f0ae60a6cc7bdf6f57e7ef0bd Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 11:27:02 +0200 Subject: [PATCH] Enforce the MFA policy as a guard, and stop 403ing public routes The last live non-shell middleware verified the JWT and read the users row a second time on every /api request, then threw the loaded user away - two jwt.verify calls and two SELECTs per authenticated request, because it ran before Nest and could not see what the auth guard had already resolved. The guard reads req.user. It also corrects an asymmetry that had grown quietly. The middleware decided on two literal path lists, and every public /api endpoint added after those lists were written - /api/config, /api/help/*, the public journey and share routes, /api/health/features - answered a logged-in user without MFA with a 403 while answering the same request from a stranger perfectly well. Nothing intended that; the lists simply stopped being maintained. Keying on @Public() means the exemption cannot drift from the route again. This is a deliberate behaviour change and the only one here. @MfaExempt carries the reason for the other kind of exemption: the routes that let somebody complete setup. Without them the policy locks out every new account, which is why the middleware had its second list. One ordering trap on the way: global guards run before route guards, so the auth guard standing down on a controller with its own @UseGuards chain left req.user unset when the MFA guard ran behind it, and it waved everything through. AUTH-020 caught it. It resolves the user now without refusing - the declared chain still owns the 401. --- server/src/middleware/globalMiddleware.ts | 2 - server/src/middleware/mfaPolicy.ts | 112 ------------- server/src/nest/app.module.ts | 5 + .../src/nest/auth/auth-public.controller.ts | 2 + server/src/nest/auth/auth.controller.ts | 6 + server/src/nest/auth/global-auth.guard.ts | 14 +- server/src/nest/auth/mfa-policy.guard.ts | 93 +++++++++++ server/src/nest/auth/passkey.controller.ts | 6 + .../tests/unit/middleware/mfaPolicy.test.ts | 100 ----------- .../tests/unit/nest/mfa-policy.guard.test.ts | 157 ++++++++++++++++++ 10 files changed, 281 insertions(+), 216 deletions(-) delete mode 100644 server/src/middleware/mfaPolicy.ts create mode 100644 server/src/nest/auth/mfa-policy.guard.ts delete mode 100644 server/tests/unit/middleware/mfaPolicy.test.ts create mode 100644 server/tests/unit/nest/mfa-policy.guard.test.ts diff --git a/server/src/middleware/globalMiddleware.ts b/server/src/middleware/globalMiddleware.ts index 5c3264c9e..0555dec3e 100644 --- a/server/src/middleware/globalMiddleware.ts +++ b/server/src/middleware/globalMiddleware.ts @@ -5,7 +5,6 @@ import helmet from 'helmet'; import cookieParser from 'cookie-parser'; import { readEnv, type AppEnv } from '../app-config'; import { logDebug, logWarn, logError } from '../nest/audit/audit-log.logger'; -import { enforceGlobalMfaPolicy } from './mfaPolicy'; /** * The global request pipeline shared by the legacy Express app and the NestJS @@ -175,7 +174,6 @@ export function applyGlobalMiddleware( app.use(express.urlencoded({ extended: true })); } app.use(cookieParser()); - app.use(enforceGlobalMfaPolicy); // Request logging with sensitive field redaction const SENSITIVE_KEYS = new Set(['password', 'new_password', 'current_password', 'token', 'jwt', 'authorization', 'cookie', 'client_secret', 'mfa_token', 'code', 'smtp_pass']); diff --git a/server/src/middleware/mfaPolicy.ts b/server/src/middleware/mfaPolicy.ts deleted file mode 100644 index e0fc0483d..000000000 --- a/server/src/middleware/mfaPolicy.ts +++ /dev/null @@ -1,112 +0,0 @@ -import { Request, Response, NextFunction } from 'express'; -import { readEnv } from '../app-config'; -import { db } from '../db/database'; -import { extractToken, verifyJwtAndLoadUser } from '../nest/auth/jwt-verify'; -import { DEMO_EMAILS } from '../nest/common/demo'; - -/** Paths that never require MFA (public or pre-auth). */ -export function isPublicApiPath(method: string, pathNoQuery: string): boolean { - if (method === 'GET' && pathNoQuery === '/api/health') return true; - if (method === 'GET' && pathNoQuery === '/api/auth/app-config') return true; - if (method === 'POST' && pathNoQuery === '/api/auth/login') return true; - if (method === 'POST' && pathNoQuery === '/api/auth/register') return true; - if (method === 'POST' && pathNoQuery === '/api/auth/demo-login') return true; - if (method === 'GET' && pathNoQuery.startsWith('/api/auth/invite/')) return true; - if (method === 'POST' && pathNoQuery === '/api/auth/mfa/verify-login') return true; - // Unauthenticated passkey (primary) login ceremony. - if (method === 'POST' && pathNoQuery === '/api/auth/passkey/login/options') return true; - if (method === 'POST' && pathNoQuery === '/api/auth/passkey/login/verify') return true; - if (pathNoQuery.startsWith('/api/auth/oidc/')) return true; - return false; -} - -/** Authenticated paths allowed while MFA is not yet enabled (setup + lockout recovery). */ -export function isMfaSetupExemptPath(method: string, pathNoQuery: string): boolean { - if (method === 'GET' && pathNoQuery === '/api/auth/me') return true; - if (method === 'POST' && pathNoQuery === '/api/auth/mfa/setup') return true; - if (method === 'POST' && pathNoQuery === '/api/auth/mfa/enable') return true; - // Allow enrolling a passkey as the second factor (a user-verified passkey - // satisfies require_mfa), so a fresh user under the policy isn't stuck. - if (method === 'POST' && pathNoQuery === '/api/auth/passkey/register/options') return true; - if (method === 'POST' && pathNoQuery === '/api/auth/passkey/register/verify') return true; - if (method === 'GET' && pathNoQuery === '/api/auth/passkey/credentials') return true; - if ((method === 'GET' || method === 'PUT') && pathNoQuery === '/api/auth/app-settings') return true; - return false; -} - -/** - * When app_settings.require_mfa is true, block API access for users without MFA enabled, - * except for public routes and MFA setup endpoints. - */ -export function enforceGlobalMfaPolicy(req: Request, res: Response, next: NextFunction): void { - const pathNoQuery = (req.originalUrl || req.url || '').split('?')[0]; - - if (!pathNoQuery.startsWith('/api')) { - next(); - return; - } - - if (isPublicApiPath(req.method, pathNoQuery)) { - next(); - return; - } - - // Accept both the httpOnly session cookie (regular SPA users) and the - // Authorization header (MCP / API clients). Previously this only looked - // at the header so every normal cookie-authenticated session sailed - // past `require_mfa` unchecked. - const token = extractToken(req); - if (!token) { - next(); - return; - } - - // Use the shared verify helper so the `password_version` gate applies - // here too — a JWT stolen before a password reset would otherwise - // continue to satisfy this middleware until its natural 24h expiry. - const verified = verifyJwtAndLoadUser(token); - if (!verified) { - next(); - return; - } - const userId = verified.id; - - const requireRow = db.prepare("SELECT value FROM app_settings WHERE key = 'require_mfa'").get() as { value: string } | undefined; - if (requireRow?.value !== 'true') { - next(); - return; - } - - if (readEnv().demo.enabled && verified.email && DEMO_EMAILS.has(verified.email)) { - next(); - return; - } - - const row = db.prepare('SELECT mfa_enabled FROM users WHERE id = ?').get(userId) as - | { mfa_enabled: number | boolean } - | undefined; - if (!row) { - next(); - return; - } - - // A user-verified passkey is phishing-resistant and inherently two-factor, so - // owning at least one satisfies the require_mfa policy exactly like TOTP does. - // (All stored passkeys were registered with userVerification required.) - const mfaOk = row.mfa_enabled === 1 || row.mfa_enabled === true; - const passkeyOk = !!db.prepare('SELECT 1 FROM webauthn_credentials WHERE user_id = ? LIMIT 1').get(userId); - if (mfaOk || passkeyOk) { - next(); - return; - } - - if (isMfaSetupExemptPath(req.method, pathNoQuery)) { - next(); - return; - } - - res.status(403).json({ - error: 'Two-factor authentication is required. Complete setup in Settings.', - code: 'MFA_REQUIRED', - }); -} diff --git a/server/src/nest/app.module.ts b/server/src/nest/app.module.ts index 776834846..5e1c8ff06 100644 --- a/server/src/nest/app.module.ts +++ b/server/src/nest/app.module.ts @@ -6,6 +6,7 @@ import { DatabaseModule } from './database/database.module'; import { RealtimeModule } from './realtime/realtime.module'; import { HealthModule } from './health/health.module'; import { GlobalAuthGuard } from './auth/global-auth.guard'; +import { MfaPolicyGuard } from './auth/mfa-policy.guard'; import { WeatherModule } from './weather/weather.module'; import { HelpModule } from './help/help.module'; import { AirportsModule } from './airports/airports.module'; @@ -69,6 +70,10 @@ import { IdempotencyInterceptor } from './common/idempotency.interceptor'; // @OptionalAuth(), or declares its own @UseGuards chain. Protection used to // be opt-in, which made a forgotten guard a silent bypass instead of an error. { provide: APP_GUARD, useClass: GlobalAuthGuard }, + // Second, and only second: it reads the user the guard above resolved + // instead of verifying the token a second time, which is what the Express + // middleware it replaces did on every /api request. + { provide: APP_GUARD, useClass: MfaPolicyGuard }, // Global error-envelope normaliser (DI-registered so it also catches // framework-level exceptions like the not-found handler). { provide: APP_FILTER, useClass: TrekExceptionFilter }, diff --git a/server/src/nest/auth/auth-public.controller.ts b/server/src/nest/auth/auth-public.controller.ts index 09c98d1b0..a107711c4 100644 --- a/server/src/nest/auth/auth-public.controller.ts +++ b/server/src/nest/auth/auth-public.controller.ts @@ -9,6 +9,7 @@ import { AuditService } from '../audit/audit.service'; import { willDropSecureCookie } from '../common/cookie'; import type { User } from '../../types'; import { Public } from './public.decorator'; +import { MfaExempt } from './mfa-policy.guard'; const WINDOW = 15 * 60 * 1000; const LOGIN_MIN_LATENCY_MS = 350; @@ -35,6 +36,7 @@ export class AuthPublicController { } @Get('app-config') + @MfaExempt('bootstrap config, read before the client knows whether it must set up MFA') @UseGuards(OptionalJwtGuard) appConfig(@Req() req: Request) { return this.auth.getAppConfig((req.user as User | undefined) ?? undefined); diff --git a/server/src/nest/auth/auth.controller.ts b/server/src/nest/auth/auth.controller.ts index 2ab443b23..96a183611 100644 --- a/server/src/nest/auth/auth.controller.ts +++ b/server/src/nest/auth/auth.controller.ts @@ -43,6 +43,7 @@ import { CurrentUser } from './current-user.decorator'; import { getClientIp } from '../audit/client-ip'; import { AuditService } from '../audit/audit.service'; import type { User } from '../../types'; +import { MfaExempt } from './mfa-policy.guard'; const WINDOW = 15 * 60 * 1000; const ALLOWED_AVATAR_EXTS = ['.jpg', '.jpeg', '.png', '.gif', '.webp']; @@ -83,6 +84,7 @@ export class AuthController { } @Get('me') + @MfaExempt('the client needs to know who it is to render the setup screen') me(@CurrentUser() user: User) { const loaded = this.auth.getCurrentUser(user.id); if (!loaded) { @@ -176,6 +178,7 @@ export class AuthController { } @Get('app-settings') + @MfaExempt('the setup screen reads the policy it is asking the user to satisfy') getAppSettings(@CurrentUser() user: User) { const result = this.auth.getAppSettings(user.id); if (result.error) { @@ -185,6 +188,7 @@ export class AuthController { } @Put('app-settings') + @MfaExempt('an admin locked out by their own policy must still be able to lift it') updateAppSettings(@CurrentUser() user: User, @Body() body: AppSettingsUpdateDto, @Req() req: Request) { const result = this.auth.updateAppSettings(user.id, body); if (result.error) { @@ -199,6 +203,7 @@ export class AuthController { // AtlasModule import. @Post('mfa/setup') + @MfaExempt('completing setup is the way out of the policy') @HttpCode(200) async mfaSetup(@CurrentUser() user: User) { const result = this.auth.setupMfa(user.id, user.email); @@ -215,6 +220,7 @@ export class AuthController { } @Post('mfa/enable') + @MfaExempt('completing setup is the way out of the policy') @HttpCode(200) mfaEnable(@CurrentUser() user: User, @Body() body: MfaEnableDto, @Req() req: Request) { this.limit('mfa', req, 5); diff --git a/server/src/nest/auth/global-auth.guard.ts b/server/src/nest/auth/global-auth.guard.ts index 39b6d149b..eea4d1dd5 100644 --- a/server/src/nest/auth/global-auth.guard.ts +++ b/server/src/nest/auth/global-auth.guard.ts @@ -44,12 +44,22 @@ export class GlobalAuthGuard implements CanActivate { return true; } - // A declared chain decides for itself, in the order it declared. + // A declared chain decides for itself, in the order it declared — but the + // user still gets resolved here, without refusing. Global guards run before + // route guards, so standing down silently would leave req.user unset for the + // MFA guard behind us, and it would wave every request through. Resolving + // without throwing keeps the declared chain in charge of the 401 while the + // rest of the chain can still see who is calling. Absent or invalid token + // leaves req.user null, which is what it was before. const declared = [ ...(this.reflector.get(GUARDS_METADATA, handler) ?? []), ...(this.reflector.get(GUARDS_METADATA, controller) ?? []), ]; - if (declared.length > 0) return true; + if (declared.length > 0) { + const declaredToken = extractToken(req); + (req as { user: unknown }).user = (declaredToken ? verifyJwtAndLoadUser(declaredToken) : null) || null; + return true; + } const token = extractToken(req); if (!token) { diff --git a/server/src/nest/auth/mfa-policy.guard.ts b/server/src/nest/auth/mfa-policy.guard.ts new file mode 100644 index 000000000..cf66f9b16 --- /dev/null +++ b/server/src/nest/auth/mfa-policy.guard.ts @@ -0,0 +1,93 @@ +import { CanActivate, ExecutionContext, HttpException, Injectable, SetMetadata } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import type { Request } from 'express'; +import { DatabaseService } from '../database/database.service'; +import { RuntimeEnvService } from '../app-config/runtime-env.service'; +import { DEMO_EMAILS } from '../common/demo'; +import { IS_PUBLIC } from './public.decorator'; +import type { User } from '../../types'; + +/** Metadata key `@MfaExempt()` writes. */ +export const MFA_EXEMPT = 'trek:mfa-exempt'; + +/** + * Reachable by an authenticated user who has not set up MFA yet, while the + * require_mfa policy is on. + * + * Two kinds qualify and both need the reason spelled out: the routes that let + * somebody *complete* setup (otherwise the policy locks out every new account), + * and the pre-auth ceremonies that carry a session but are not yet a session the + * policy should judge. + */ +export const MfaExempt = (reason: string) => SetMetadata(MFA_EXEMPT, { reason }); + +type MfaRequest = Request & { user?: User }; + +/** + * Enforces app_settings.require_mfa, as a guard rather than an Express layer. + * + * The middleware it replaces re-verified the JWT and re-read the users row on + * every single /api request, then threw the loaded user away — two jwt.verify + * calls and two SELECTs per authenticated request, because it ran before Nest + * and could not see what the auth guard had already resolved. This one reads + * `req.user`, which the global auth guard put there. + * + * It also corrects an asymmetry that had grown quietly. The middleware decided + * on two literal path lists, and every public /api endpoint added after those + * lists were written — /api/config, /api/help/*, the public journey and share + * routes, /api/health/features — answered a logged-in user without MFA with a + * 403 while answering the same request from a stranger perfectly well. Nothing + * intended that; the lists simply stopped being maintained. Keying on @Public() + * instead of a path list means the exemption cannot drift from the route again. + * That is a deliberate behaviour change, and the only one here. + */ +@Injectable() +export class MfaPolicyGuard implements CanActivate { + constructor( + private readonly db: DatabaseService, + private readonly env: RuntimeEnvService, + private readonly reflector: Reflector, + ) {} + + canActivate(context: ExecutionContext): boolean { + const handler = context.getHandler(); + const controller = context.getClass(); + + // A route a stranger may call is a route the policy has no business judging. + if (this.reflector.getAllAndOverride(IS_PUBLIC, [handler, controller])) return true; + + const user = context.switchToHttp().getRequest().user; + // No session: whoever answers next decides, exactly as before. + if (!user) return true; + + const requireRow = this.db.get<{ value: string }>( + "SELECT value FROM app_settings WHERE key = 'require_mfa'", + ); + if (requireRow?.value !== 'true') return true; + + if (this.env.isDemoMode() && user.email && DEMO_EMAILS.has(user.email)) return true; + + const row = this.db.get<{ mfa_enabled: number | boolean }>( + 'SELECT mfa_enabled FROM users WHERE id = ?', + user.id, + ); + if (!row) return true; + + // A user-verified passkey is phishing-resistant and inherently two-factor, so + // owning at least one satisfies require_mfa exactly like TOTP does. + // (All stored passkeys were registered with userVerification required.) + const mfaOk = row.mfa_enabled === 1 || row.mfa_enabled === true; + const passkeyOk = !!this.db.get('SELECT 1 FROM webauthn_credentials WHERE user_id = ? LIMIT 1', user.id); + if (mfaOk || passkeyOk) return true; + + if (this.reflector.getAllAndOverride(MFA_EXEMPT, [handler, controller])) return true; + + throw new HttpException( + { + error: 'Two-factor authentication is required. Complete setup in Settings.', + code: 'MFA_REQUIRED', + }, + 403, + ); + } +} diff --git a/server/src/nest/auth/passkey.controller.ts b/server/src/nest/auth/passkey.controller.ts index f2b1982dd..1b2a18b52 100644 --- a/server/src/nest/auth/passkey.controller.ts +++ b/server/src/nest/auth/passkey.controller.ts @@ -10,6 +10,7 @@ import { AuditService } from '../audit/audit.service'; import { PasskeyService } from './passkey.service'; import { PasskeyRegisterOptionsDto, PasskeyRegisterVerifyDto, PasskeyLoginVerifyDto, PasskeyRenameDto, PasskeyDeleteDto } from './auth.dto'; import type { User } from '../../types'; +import { MfaExempt } from './mfa-policy.guard'; const WINDOW = 15 * 60 * 1000; const LOGIN_MIN_LATENCY_MS = 350; @@ -43,6 +44,7 @@ export class PasskeyController { // ── Registration (authenticated) ── @Post('register/options') + @MfaExempt('a user-verified passkey satisfies require_mfa, so enrolling one is a way out') @HttpCode(200) @UseGuards(PasskeyEnabledGuard, JwtAuthGuard) async registerOptions(@CurrentUser() user: User, @Body() body: PasskeyRegisterOptionsDto, @Req() req: Request) { @@ -53,6 +55,7 @@ export class PasskeyController { } @Post('register/verify') + @MfaExempt('a user-verified passkey satisfies require_mfa, so enrolling one is a way out') @HttpCode(200) @UseGuards(PasskeyEnabledGuard, JwtAuthGuard) async registerVerify(@CurrentUser() user: User, @Body() body: PasskeyRegisterVerifyDto, @Req() req: Request) { @@ -64,6 +67,7 @@ export class PasskeyController { // ── Authentication (public — primary login) ── @Post('login/options') + @MfaExempt('unauthenticated passkey login ceremony') @HttpCode(200) @UseGuards(PasskeyEnabledGuard) async loginOptions(@Req() req: Request) { @@ -74,6 +78,7 @@ export class PasskeyController { } @Post('login/verify') + @MfaExempt('unauthenticated passkey login ceremony') @HttpCode(200) @UseGuards(PasskeyEnabledGuard) async loginVerify(@Body() body: PasskeyLoginVerifyDto, @Req() req: Request, @Res({ passthrough: true }) res: Response) { @@ -95,6 +100,7 @@ export class PasskeyController { // ── Management (authenticated, owner-scoped — NOT toggle-gated) ── @Get('credentials') + @MfaExempt('the setup screen lists what the user already has') @UseGuards(JwtAuthGuard) list(@CurrentUser() user: User) { return { credentials: this.passkeys.listPasskeys(user.id) }; diff --git a/server/tests/unit/middleware/mfaPolicy.test.ts b/server/tests/unit/middleware/mfaPolicy.test.ts deleted file mode 100644 index cc3d8a7c1..000000000 --- a/server/tests/unit/middleware/mfaPolicy.test.ts +++ /dev/null @@ -1,100 +0,0 @@ -import { describe, it, expect, vi } from 'vitest'; - -vi.mock('../../../src/db/database', () => ({ - db: { prepare: () => ({ get: vi.fn(), all: vi.fn() }) }, -})); -vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret' })); - -import { isPublicApiPath, isMfaSetupExemptPath } from '../../../src/middleware/mfaPolicy'; - -// ── isPublicApiPath ────────────────────────────────────────────────────────── - -describe('isPublicApiPath', () => { - // AUTH-001 — Public paths must bypass MFA - it('AUTH-001: GET /api/health is public', () => { - expect(isPublicApiPath('GET', '/api/health')).toBe(true); - }); - - it('GET /api/auth/app-config is public', () => { - expect(isPublicApiPath('GET', '/api/auth/app-config')).toBe(true); - }); - - it('POST /api/auth/login is public', () => { - expect(isPublicApiPath('POST', '/api/auth/login')).toBe(true); - }); - - it('POST /api/auth/register is public', () => { - expect(isPublicApiPath('POST', '/api/auth/register')).toBe(true); - }); - - it('POST /api/auth/demo-login is public', () => { - expect(isPublicApiPath('POST', '/api/auth/demo-login')).toBe(true); - }); - - it('GET /api/auth/invite/ is public', () => { - expect(isPublicApiPath('GET', '/api/auth/invite/abc123')).toBe(true); - expect(isPublicApiPath('GET', '/api/auth/invite/xyz-789')).toBe(true); - }); - - it('POST /api/auth/mfa/verify-login is public', () => { - expect(isPublicApiPath('POST', '/api/auth/mfa/verify-login')).toBe(true); - }); - - it('OIDC paths are public (any method)', () => { - expect(isPublicApiPath('GET', '/api/auth/oidc/callback')).toBe(true); - expect(isPublicApiPath('POST', '/api/auth/oidc/login')).toBe(true); - expect(isPublicApiPath('GET', '/api/auth/oidc/discovery')).toBe(true); - }); - - it('GET /api/trips is not public', () => { - expect(isPublicApiPath('GET', '/api/trips')).toBe(false); - }); - - it('POST /api/auth/login with wrong method (GET) is not public', () => { - expect(isPublicApiPath('GET', '/api/auth/login')).toBe(false); - }); - - it('GET /api/auth/me is not public', () => { - expect(isPublicApiPath('GET', '/api/auth/me')).toBe(false); - }); - - it('DELETE /api/auth/logout is not public', () => { - expect(isPublicApiPath('DELETE', '/api/auth/logout')).toBe(false); - }); -}); - -// ── isMfaSetupExemptPath ───────────────────────────────────────────────────── - -describe('isMfaSetupExemptPath', () => { - it('GET /api/auth/me is MFA-setup exempt', () => { - expect(isMfaSetupExemptPath('GET', '/api/auth/me')).toBe(true); - }); - - it('POST /api/auth/mfa/setup is MFA-setup exempt', () => { - expect(isMfaSetupExemptPath('POST', '/api/auth/mfa/setup')).toBe(true); - }); - - it('POST /api/auth/mfa/enable is MFA-setup exempt', () => { - expect(isMfaSetupExemptPath('POST', '/api/auth/mfa/enable')).toBe(true); - }); - - it('GET /api/auth/app-settings is MFA-setup exempt', () => { - expect(isMfaSetupExemptPath('GET', '/api/auth/app-settings')).toBe(true); - }); - - it('PUT /api/auth/app-settings is MFA-setup exempt', () => { - expect(isMfaSetupExemptPath('PUT', '/api/auth/app-settings')).toBe(true); - }); - - it('POST /api/auth/app-settings is NOT exempt (wrong method)', () => { - expect(isMfaSetupExemptPath('POST', '/api/auth/app-settings')).toBe(false); - }); - - it('GET /api/trips is NOT exempt', () => { - expect(isMfaSetupExemptPath('GET', '/api/trips')).toBe(false); - }); - - it('GET /api/auth/logout is NOT exempt', () => { - expect(isMfaSetupExemptPath('GET', '/api/auth/logout')).toBe(false); - }); -}); diff --git a/server/tests/unit/nest/mfa-policy.guard.test.ts b/server/tests/unit/nest/mfa-policy.guard.test.ts new file mode 100644 index 000000000..d555ace87 --- /dev/null +++ b/server/tests/unit/nest/mfa-policy.guard.test.ts @@ -0,0 +1,157 @@ +/** + * MfaPolicyGuard — the require_mfa policy, moved off the Express layer. + * + * The cases that matter here are the exemptions, because that is where the + * middleware had drifted: it decided on two literal path lists, and every public + * /api endpoint added after those lists were written answered a logged-in user + * without MFA with a 403 while answering a stranger fine. MFA-011 pins the + * correction. + */ +import { describe, it, expect, vi } from 'vitest'; +import { HttpException } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { MfaExempt, MFA_EXEMPT, MfaPolicyGuard } from '../../../src/nest/auth/mfa-policy.guard'; +import { Public, IS_PUBLIC } from '../../../src/nest/auth/public.decorator'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; +import { DEMO_EMAIL_PRIMARY } from '../../../src/nest/common/demo'; +import type { User } from '../../../src/types'; + +const user = { id: 7, email: 'u@example.test', role: 'user' } as User; + +interface Rows { + requireMfa?: string; + mfaEnabled?: number | boolean | undefined; + hasPasskey?: boolean; + userMissing?: boolean; +} + +function makeGuard(rows: Rows = {}, meta: { public?: boolean; exempt?: boolean } = {}, demo = false) { + const get = vi.fn((sql: string) => { + if (sql.includes('app_settings')) return rows.requireMfa === undefined ? undefined : { value: rows.requireMfa }; + if (sql.includes('FROM users')) return rows.userMissing ? undefined : { mfa_enabled: rows.mfaEnabled ?? 0 }; + if (sql.includes('webauthn_credentials')) return rows.hasPasskey ? { 1: 1 } : undefined; + return undefined; + }); + const reflector = { + getAllAndOverride: vi.fn((key: string) => + (key === IS_PUBLIC && meta.public) || (key === MFA_EXEMPT && meta.exempt) ? { reason: 'x' } : undefined, + ), + } as unknown as Reflector; + const guard = new MfaPolicyGuard( + { get } as unknown as DatabaseService, + { isDemoMode: () => demo } as unknown as RuntimeEnvService, + reflector, + ); + return { guard, get }; +} + +function ctx(request: Record) { + return { + switchToHttp: () => ({ getRequest: () => request }), + getHandler: () => function handler() {}, + getClass: () => class Controller {}, + } as never; +} + +const thrown = (run: () => unknown) => { + try { + run(); + } catch (err) { + expect(err).toBeInstanceOf(HttpException); + const e = err as HttpException; + return { status: e.getStatus(), body: e.getResponse() }; + } + throw new Error('expected throw'); +}; + +const ENFORCED: Rows = { requireMfa: 'true', mfaEnabled: 0 }; + +describe('MfaPolicyGuard', () => { + it('MFA-001: refuses a user without MFA while the policy is on, with the legacy body', () => { + const { guard } = makeGuard(ENFORCED); + expect(thrown(() => guard.canActivate(ctx({ user })))).toEqual({ + status: 403, + body: { + error: 'Two-factor authentication is required. Complete setup in Settings.', + code: 'MFA_REQUIRED', + }, + }); + }); + + it('MFA-002: lets a user with TOTP through', () => { + const { guard } = makeGuard({ requireMfa: 'true', mfaEnabled: 1 }); + expect(guard.canActivate(ctx({ user }))).toBe(true); + }); + + it('MFA-003: a user-verified passkey satisfies the policy like TOTP does', () => { + const { guard } = makeGuard({ requireMfa: 'true', mfaEnabled: 0, hasPasskey: true }); + expect(guard.canActivate(ctx({ user }))).toBe(true); + }); + + it('MFA-004: does nothing when the policy is off, and never reads the users row', () => { + const { guard, get } = makeGuard({ requireMfa: 'false', mfaEnabled: 0 }); + expect(guard.canActivate(ctx({ user }))).toBe(true); + expect(get.mock.calls.some((c) => String(c[0]).includes('FROM users'))).toBe(false); + }); + + it('MFA-005: does nothing when the setting row is absent at all', () => { + const { guard } = makeGuard({ mfaEnabled: 0 }); + expect(guard.canActivate(ctx({ user }))).toBe(true); + }); + + it('MFA-006: an anonymous request is not the policy business', () => { + const { guard, get } = makeGuard(ENFORCED); + expect(guard.canActivate(ctx({}))).toBe(true); + expect(get).not.toHaveBeenCalled(); + }); + + it('MFA-007: the demo account is exempt while demo mode is on', () => { + const demoUser = { id: 1, email: DEMO_EMAIL_PRIMARY, role: 'user' } as User; + expect(makeGuard(ENFORCED, {}, true).guard.canActivate(ctx({ user: demoUser }))).toBe(true); + // …and not otherwise. + expect(thrown(() => makeGuard(ENFORCED, {}, false).guard.canActivate(ctx({ user: demoUser })))).toEqual({ + status: 403, + body: expect.objectContaining({ code: 'MFA_REQUIRED' }), + }); + }); + + it('MFA-008: a user row that vanished mid-request is let through, not 403d', () => { + const { guard } = makeGuard({ requireMfa: 'true', userMissing: true }); + expect(guard.canActivate(ctx({ user }))).toBe(true); + }); + + it('MFA-009: @MfaExempt is the way out — setup would be unreachable otherwise', () => { + const { guard } = makeGuard(ENFORCED, { exempt: true }); + expect(guard.canActivate(ctx({ user }))).toBe(true); + }); + + it('MFA-010: it reads req.user rather than verifying the token again', () => { + // The middleware this replaces called jwt.verify and SELECTed the users row a + // second time on every /api request, then discarded the result. + const { guard, get } = makeGuard({ requireMfa: 'true', mfaEnabled: 1 }); + guard.canActivate(ctx({ user, headers: { authorization: 'Bearer nonsense' } })); + expect(get.mock.calls.map((c) => c[0]).every((sql) => !String(sql).includes('password_version'))).toBe(true); + }); + + it('MFA-011: a @Public route no longer 403s a logged-in user without MFA', () => { + // The correction. /api/config, /api/help/*, the public journey and share + // routes and /api/health/features all answered a stranger fine and answered + // this user with a 403, because they were added after the middleware path + // lists were written and nobody noticed. + const { guard, get } = makeGuard(ENFORCED, { public: true }); + expect(guard.canActivate(ctx({ user }))).toBe(true); + expect(get).not.toHaveBeenCalled(); + }); + + it('MFA-012: the decorators write what the guard reads', () => { + class Probe { + @MfaExempt('setup would be unreachable') + exempt() {} + @Public('anonymous') + open() {} + } + expect(Reflect.getMetadata(MFA_EXEMPT, Probe.prototype.exempt)).toEqual({ reason: 'setup would be unreachable' }); + expect(Reflect.getMetadata(IS_PUBLIC, Probe.prototype.open)).toEqual({ reason: 'anonymous' }); + }); +});