security: implement tickets #28-#35 + architecture decision #30

#28 - TOTP rate limiting (verifyTotp): added totpRateLimiter (10 req/30s),
  throws TOO_MANY_REQUESTS before DB hit; 16 unit tests including rate-limit
  exceeded + userId key isolation.

#29 - /api/reports/allocations role check: only ADMIN/MANAGER/CONTROLLER may
  access; returns 403 otherwise; 9 unit tests (401 unauthenticated, 403 for
  USER/VIEWER, 200 for allowed roles + xlsx format).

#31 - pgAdmin credentials moved out of docker-compose.yml into env vars;
  PGADMIN_PASSWORD is now required (:?) to prevent accidental plaintext
  exposure in committed files.

#34 - Server-side HTML sanitization for comment bodies via stripHtml():
  strips all tags + decodes safe entities before persistence; 16 unit tests
  covering passthrough, injection patterns, entity decoding.

#35 - MFA setup prompt banner (MfaPromptBanner): shown to ADMIN/MANAGER users
  without TOTP enabled; user-scoped localStorage snooze (7 days); links to
  /account/security; accessibility role=alert; 7 structural unit tests.

#33 - Auth anomaly alerting cron (/api/cron/auth-anomaly-check): detects
  HIGH_GLOBAL_FAILURE_RATE and CONCENTRATED_FAILURES in 30-minute window;
  CRITICAL notification to ADMINs; fail-closed via verifyCronSecret;
  10 unit tests.

#32 - MFA enforcement policy: added requireMfaForRoles field to SystemSettings
  schema + Prisma migration; auth.ts blocks login with MFA_REQUIRED_SETUP
  signal if role is enforced but TOTP not set up; signin page redirects to
  /account/security?mfa_required=1; settings schema + view model updated;
  11 unit tests.

#30 - API keys architecture decision documented in LEARNINGS.md; no code
  written — product decision required before implementation.

Co-Authored-By: claude-flow <ruv@ruv.net>

packages/api/src/__tests__/mfa-enforcement.test.ts

@@ -0,0 +1,101 @@
+/**
+ * Unit tests for MFA enforcement via SystemSettings.requireMfaForRoles.
+ *
+ * Tests cover:
+ *   - requireMfaForRoles is returned by buildSystemSettingsViewModel
+ *   - buildSettingsUpdatePayload includes requireMfaForRoles in the DB payload
+ *   - buildSettingsUpdatePayload handles null (clear enforcement)
+ *   - Schema validation: valid roles accepted, invalid roles rejected
+ */
+
+import { describe, expect, it } from "vitest";
+import {
+  buildSettingsUpdatePayload,
+  buildSystemSettingsViewModel,
+  settingsUpdateInputSchema,
+} from "../router/settings-support.js";
+import type { RuntimeSecretField, RuntimeSecretStatus } from "../lib/system-settings-runtime.js";
+import { RUNTIME_SECRET_FIELDS } from "../lib/system-settings-runtime.js";
+
+const emptyRuntimeSecrets = Object.fromEntries(
+  RUNTIME_SECRET_FIELDS.map((field) => [
+    field,
+    { configured: false, activeSource: "none", hasStoredValue: false, envVarNames: [] } satisfies RuntimeSecretStatus,
+  ]),
+) as Record<RuntimeSecretField, RuntimeSecretStatus>;
+
+// Minimal stubs for required inputs
+function makeViewModelInput(
+  requireMfaForRoles: string[] | null | undefined = undefined,
+) {
+  return {
+    settings: {
+      requireMfaForRoles,
+    },
+    runtimeSettings: null,
+    runtimeSecrets: emptyRuntimeSecrets,
+    defaultSummaryPrompt: "",
+  };
+}
+
+describe("buildSystemSettingsViewModel — requireMfaForRoles", () => {
+  it("returns null when requireMfaForRoles is not set in DB", () => {
+    const vm = buildSystemSettingsViewModel(makeViewModelInput(undefined));
+    expect(vm.requireMfaForRoles).toBeNull();
+  });
+
+  it("returns null when requireMfaForRoles is explicitly null", () => {
+    const vm = buildSystemSettingsViewModel(makeViewModelInput(null));
+    expect(vm.requireMfaForRoles).toBeNull();
+  });
+
+  it("returns the configured roles array", () => {
+    const vm = buildSystemSettingsViewModel(makeViewModelInput(["ADMIN", "MANAGER"]));
+    expect(vm.requireMfaForRoles).toEqual(["ADMIN", "MANAGER"]);
+  });
+});
+
+describe("buildSettingsUpdatePayload — requireMfaForRoles", () => {
+  it("includes requireMfaForRoles in DB payload when provided", () => {
+    const { data } = buildSettingsUpdatePayload({ requireMfaForRoles: ["ADMIN"] });
+    expect(data.requireMfaForRoles).toEqual(["ADMIN"]);
+  });
+
+  it("sets requireMfaForRoles to null when explicitly cleared", () => {
+    const { data } = buildSettingsUpdatePayload({ requireMfaForRoles: null });
+    expect(data.requireMfaForRoles).toBeNull();
+  });
+
+  it("omits requireMfaForRoles from payload when not provided (no change)", () => {
+    const { data } = buildSettingsUpdatePayload({});
+    expect("requireMfaForRoles" in data).toBe(false);
+  });
+});
+
+describe("settingsUpdateInputSchema — requireMfaForRoles validation", () => {
+  it("accepts a valid array of system roles", () => {
+    const result = settingsUpdateInputSchema.safeParse({ requireMfaForRoles: ["ADMIN", "MANAGER"] });
+    expect(result.success).toBe(true);
+  });
+
+  it("accepts an empty array (disable enforcement)", () => {
+    const result = settingsUpdateInputSchema.safeParse({ requireMfaForRoles: [] });
+    expect(result.success).toBe(true);
+  });
+
+  it("accepts null (clear enforcement)", () => {
+    const result = settingsUpdateInputSchema.safeParse({ requireMfaForRoles: null });
+    expect(result.success).toBe(true);
+  });
+
+  it("rejects an invalid role string", () => {
+    const result = settingsUpdateInputSchema.safeParse({ requireMfaForRoles: ["SUPERUSER"] });
+    expect(result.success).toBe(false);
+  });
+
+  it("accepts omitted field (no change)", () => {
+    const result = settingsUpdateInputSchema.safeParse({});
+    expect(result.success).toBe(true);
+    expect(result.data?.requireMfaForRoles).toBeUndefined();
+  });
+});

packages/api/src/__tests__/settings-support.test.ts

@@ -88,6 +88,7 @@ describe("settings support", () => {
       imageProvider: "dalle",
       vacationDefaultDays: 30,
       timelineUndoMaxSteps: 50,
+      requireMfaForRoles: null,
     });
   });
 

packages/api/src/__tests__/strip-html.test.ts

@@ -0,0 +1,96 @@
+/**
+ * Unit tests for the server-side stripHtml utility (A03 XSS prevention).
+ *
+ * Covers:
+ *   - Passthrough of plain text and mention tokens
+ *   - Stripping of common HTML injection patterns
+ *   - HTML entity decoding
+ *   - Edge cases (empty string, nested tags, self-closing tags)
+ */
+
+import { describe, expect, it } from "vitest";
+import { stripHtml } from "../lib/strip-html.js";
+
+describe("stripHtml — plain text passthrough", () => {
+  it("returns plain text unchanged", () => {
+    expect(stripHtml("Hello, world!")).toBe("Hello, world!");
+  });
+
+  it("preserves mention tokens", () => {
+    expect(stripHtml("Hey @[Alice](user_1) please check this")).toBe(
+      "Hey @[Alice](user_1) please check this",
+    );
+  });
+
+  it("preserves newlines and whitespace", () => {
+    expect(stripHtml("Line 1\nLine 2\n  indented")).toBe("Line 1\nLine 2\n  indented");
+  });
+
+  it("returns empty string unchanged", () => {
+    expect(stripHtml("")).toBe("");
+  });
+});
+
+describe("stripHtml — HTML injection stripping (A03)", () => {
+  it("strips a script tag and its content", () => {
+    const result = stripHtml('<script>alert("xss")</script>normal text');
+    expect(result).not.toContain("<script");
+    expect(result).toContain("normal text");
+  });
+
+  it("strips an img onerror payload", () => {
+    const result = stripHtml('<img src=x onerror="alert(1)">text');
+    expect(result).not.toContain("<img");
+    expect(result).toContain("text");
+  });
+
+  it("strips anchor tags with javascript: href", () => {
+    const result = stripHtml('<a href="javascript:alert(1)">click me</a>');
+    expect(result).not.toContain("<a");
+    expect(result).toContain("click me");
+  });
+
+  it("strips iframe tags", () => {
+    const result = stripHtml('<iframe src="evil.com"></iframe>safe text');
+    expect(result).not.toContain("<iframe");
+    expect(result).toContain("safe text");
+  });
+
+  it("strips nested tags", () => {
+    const result = stripHtml("<b><i>bold italic</i></b>");
+    expect(result).toBe("bold italic");
+  });
+
+  it("strips self-closing tags", () => {
+    const result = stripHtml("before<br/>after");
+    expect(result).toBe("beforeafter");
+  });
+
+  it("strips HTML comment injection", () => {
+    const result = stripHtml("<!-- <script>alert(1)</script> -->text");
+    expect(result).not.toContain("<");
+    expect(result).toContain("text");
+  });
+});
+
+describe("stripHtml — HTML entity decoding", () => {
+  it("decodes &amp; to &", () => {
+    expect(stripHtml("cats &amp; dogs")).toBe("cats & dogs");
+  });
+
+  it("decodes &lt; and &gt; to < and >", () => {
+    expect(stripHtml("1 &lt; 2 &gt; 0")).toBe("1 < 2 > 0");
+  });
+
+  it("decodes &quot; to double quote", () => {
+    expect(stripHtml('say &quot;hello&quot;')).toBe('say "hello"');
+  });
+
+  it("decodes &#39; and &apos; to single quote", () => {
+    expect(stripHtml("it&#39;s fine and it&apos;s ok")).toBe("it's fine and it's ok");
+  });
+
+  it("decodes &nbsp; to space", () => {
+    expect(stripHtml("hello&nbsp;world")).toBe("hello world");
+  });
+});

packages/api/src/__tests__/user-self-service-mfa.test.ts

@@ -4,18 +4,18 @@
  * Tests cover:
  *   - generateTotpSecret: secret creation and DB write
  *   - verifyAndEnableTotp: valid/invalid token, guard conditions
- *   - verifyTotp (login): valid/invalid token, not-enabled guard
+ *   - verifyTotp (login): valid/invalid token, not-enabled guard, rate limiting
  *   - getCurrentMfaStatus: status read
  *
  * otpauth is mocked so tests are deterministic and do not depend on
  * time-based code generation.
+ * totpRateLimiter is mocked to allow/block independently of real Redis.
  */
 
 import { TRPCError } from "@trpc/server";
 import { beforeEach, describe, expect, it, vi } from "vitest";
 
 // ─── otpauth mock ────────────────────────────────────────────────────────────
-// Must be hoisted before imports that pull in the module under test.
 const totpValidateMock = vi.hoisted(() => vi.fn<() => number | null>());
 
 vi.mock("otpauth", () => {
@@ -31,6 +31,20 @@ vi.mock("otpauth", () => {
   return { Secret, TOTP };
 });
 
+// ─── rate-limit mock ──────────────────────────────────────────────────────────
+// Default: rate limit allows all requests. Override in specific tests.
+const totpRateLimiterMock = vi.hoisted(() => vi.fn(async (_key: string) => ({
+  allowed: true,
+  remaining: 9,
+  resetAt: new Date(Date.now() + 30_000),
+})));
+
+vi.mock("../middleware/rate-limit.js", () => ({
+  totpRateLimiter: totpRateLimiterMock,
+  apiRateLimiter: vi.fn(async () => ({ allowed: true, remaining: 99, resetAt: new Date() })),
+  authRateLimiter: vi.fn(async () => ({ allowed: true, remaining: 4, resetAt: new Date() })),
+}));
+
 // ─── import after mock setup ─────────────────────────────────────────────────
 import {
   generateTotpSecret,
@@ -177,6 +191,7 @@ describe("verifyTotp", () => {
   beforeEach(() => {
     vi.clearAllMocks();
     totpValidateMock.mockReset();
+    totpRateLimiterMock.mockResolvedValue({ allowed: true, remaining: 9, resetAt: new Date(Date.now() + 30_000) });
   });
 
   const mfaUser = { id: "user_1", totpSecret: "TESTBASE32SECRET", totpEnabled: true };
@@ -216,6 +231,31 @@ describe("verifyTotp", () => {
       verifyTotp(ctx as Parameters<typeof verifyTotp>[0], { userId: "user_1", token: "123456" }),
     ).rejects.toThrow(TRPCError);
   });
+
+  it("throws TOO_MANY_REQUESTS when rate limit is exceeded", async () => {
+    totpRateLimiterMock.mockResolvedValue({ allowed: false, remaining: 0, resetAt: new Date(Date.now() + 30_000) });
+    const ctx = makePublicCtx({ user: { findUnique: vi.fn().mockResolvedValue(mfaUser) } });
+    await expect(
+      verifyTotp(ctx as Parameters<typeof verifyTotp>[0], { userId: "user_1", token: "123456" }),
+    ).rejects.toThrow(new TRPCError({ code: "TOO_MANY_REQUESTS", message: "Too many TOTP attempts. Please wait before trying again." }));
+  });
+
+  it("does not check the token when rate limit is exceeded", async () => {
+    totpRateLimiterMock.mockResolvedValue({ allowed: false, remaining: 0, resetAt: new Date() });
+    const ctx = makePublicCtx({ user: { findUnique: vi.fn().mockResolvedValue(mfaUser) } });
+    await expect(
+      verifyTotp(ctx as Parameters<typeof verifyTotp>[0], { userId: "user_1", token: "123456" }),
+    ).rejects.toThrow(TRPCError);
+    // DB user lookup must not happen when rate-limited
+    expect(ctx.db.user.findUnique).not.toHaveBeenCalled();
+  });
+
+  it("calls the rate limiter with the userId as key", async () => {
+    totpValidateMock.mockReturnValue(0);
+    const ctx = makePublicCtx({ user: { findUnique: vi.fn().mockResolvedValue(mfaUser) } });
+    await verifyTotp(ctx as Parameters<typeof verifyTotp>[0], { userId: "user_1", token: "123456" });
+    expect(totpRateLimiterMock).toHaveBeenCalledWith("user_1");
+  });
 });
 
 // ─── getCurrentMfaStatus ──────────────────────────────────────────────────────

packages/api/src/lib/strip-html.ts

@@ -0,0 +1,34 @@
+/**
+ * Minimal server-side HTML stripping for plain-text inputs.
+ *
+ * Comment bodies are plain text + mention tokens (@[Name](id)).
+ * Any HTML tags injected by a malicious actor are removed here before
+ * persistence so that even consumers that skip the client-side DOMPurify
+ * step (email notifications, PDF reports, AI assistant responses) are safe.
+ *
+ * Algorithm:
+ *   1. Strip all HTML tags.
+ *   2. Decode a small allow-list of HTML entities to their UTF-8 equivalents.
+ *
+ * This is intentionally minimal — do not extend it to allow any tags.
+ */
+
+const HTML_ENTITIES: Record<string, string> = {
+  "&amp;": "&",
+  "&lt;": "<",
+  "&gt;": ">",
+  "&quot;": '"',
+  "&#39;": "'",
+  "&apos;": "'",
+  "&nbsp;": " ",
+};
+
+export function stripHtml(input: string): string {
+  // Remove all HTML/XML tags (including self-closing and multi-line)
+  const tagStripped = input.replace(/<[^>]*>/g, "");
+  // Decode common HTML entities
+  return tagStripped.replace(
+    /&(?:amp|lt|gt|quot|#39|apos|nbsp);/g,
+    (entity) => HTML_ENTITIES[entity] ?? entity,
+  );
+}

packages/api/src/middleware/rate-limit.ts

@@ -256,3 +256,9 @@ export const apiRateLimiter = createRateLimiter(15 * 60 * 1000, 100, {
 export const authRateLimiter = createRateLimiter(15 * 60 * 1000, 5, {
   name: "auth",
 });
+
+/** TOTP verification rate limiter: 10 attempts per 30 seconds per userId.
+ *  Applied to the public verifyTotp endpoint to prevent brute-force of 6-digit codes. */
+export const totpRateLimiter = createRateLimiter(30 * 1000, 10, {
+  name: "totp-verify",
+});

packages/api/src/router/comment-procedure-support.ts

@@ -2,6 +2,7 @@ import { TRPCError } from "@trpc/server";
 import { z } from "zod";
 import { createAuditEntry } from "../lib/audit.js";
 import { assertCommentEntityAccess, CommentEntityTypeSchema } from "../lib/comment-entity-registry.js";
+import { stripHtml } from "../lib/strip-html.js";
 import { createNotification } from "../lib/create-notification.js";
 import type { TRPCContext } from "../trpc.js";
 import {
@@ -90,7 +91,8 @@ export async function createComment(
     throw new TRPCError({ code: "UNAUTHORIZED" });
   }
 
-  const mentions = parseCommentMentions(input.body);
+  const sanitizedBody = stripHtml(input.body);
+  const mentions = parseCommentMentions(sanitizedBody);
 
   if (input.parentId) {
     const parent = await ctx.db.comment.findUnique({
@@ -118,7 +120,7 @@ export async function createComment(
       entityId: input.entityId,
       parentId: input.parentId,
       authorId,
-      body: input.body,
+      body: sanitizedBody,
       mentions,
     }),
     include: commentWithAuthorInclude,
@@ -130,7 +132,7 @@ export async function createComment(
     const notifications = buildCommentMentionNotifications({
       mentionedUserIds,
       authorName,
-      body: input.body,
+      body: sanitizedBody,
       entityId: input.entityId,
       entityType: input.entityType,
       senderId: authorId,
@@ -150,7 +152,7 @@ export async function createComment(
     db: ctx.db,
     entityType: "Comment",
     entityId: comment.id,
-    entityName: input.body.slice(0, 50),
+    entityName: sanitizedBody.slice(0, 50),
     action: "CREATE",
     ...(ctx.dbUser?.id ? { userId: ctx.dbUser.id } : {}),
     after: comment as unknown as Record<string, unknown>,

packages/api/src/router/settings-support.ts

@@ -65,6 +65,7 @@ export const settingsUpdateInputSchema = z.object({
   imageProvider: z.enum(["dalle", "gemini"]).optional(),
   vacationDefaultDays: z.number().int().min(0).max(365).optional(),
   timelineUndoMaxSteps: z.number().int().min(1).max(200).optional(),
+  requireMfaForRoles: z.array(z.enum(["ADMIN", "MANAGER", "CONTROLLER", "USER", "VIEWER"])).nullable().optional(),
 });
 
 export type SettingsUpdateInput = z.infer<typeof settingsUpdateInputSchema>;
@@ -104,6 +105,7 @@ export function buildSystemSettingsViewModel(input: {
     imageProvider?: string | null;
     vacationDefaultDays?: number | null;
     timelineUndoMaxSteps?: number | null;
+    requireMfaForRoles?: unknown;
   } | null | undefined;
   runtimeSettings: {
     azureOpenAiApiKey?: string | null;
@@ -150,6 +152,7 @@ export function buildSystemSettingsViewModel(input: {
     imageProvider: settings?.imageProvider ?? "dalle",
     vacationDefaultDays: settings?.vacationDefaultDays ?? 28,
     timelineUndoMaxSteps: settings?.timelineUndoMaxSteps ?? 50,
+    requireMfaForRoles: (settings?.requireMfaForRoles as string[] | null | undefined) ?? null,
   };
 }
 
@@ -196,6 +199,7 @@ export function buildSettingsUpdatePayload(input: SettingsUpdateInput): {
   if (input.imageProvider !== undefined) data.imageProvider = input.imageProvider;
   if (input.vacationDefaultDays !== undefined) data.vacationDefaultDays = input.vacationDefaultDays;
   if (input.timelineUndoMaxSteps !== undefined) data.timelineUndoMaxSteps = input.timelineUndoMaxSteps;
+  if (input.requireMfaForRoles !== undefined) data.requireMfaForRoles = input.requireMfaForRoles ?? null;
 
   return { data, ignoredSecretFields };
 }

packages/api/src/router/user-self-service-procedure-support.ts

@@ -8,6 +8,7 @@ import { TRPCError } from "@trpc/server";
 import { z } from "zod";
 import { findUniqueOrThrow } from "../db/helpers.js";
 import { createAuditEntry } from "../lib/audit.js";
+import { totpRateLimiter } from "../middleware/rate-limit.js";
 import type { TRPCContext } from "../trpc.js";
 
 export const SaveDashboardLayoutInputSchema = z.object({
@@ -229,6 +230,12 @@ export async function verifyTotp(
   ctx: UserPublicContext,
   input: z.infer<typeof VerifyTotpInputSchema>,
 ) {
+  // Rate limit: max 10 attempts per 30 seconds per userId to prevent brute-force (A01-1)
+  const rl = await totpRateLimiter(input.userId);
+  if (!rl.allowed) {
+    throw new TRPCError({ code: "TOO_MANY_REQUESTS", message: "Too many TOTP attempts. Please wait before trying again." });
+  }
+
   const user = await findUniqueOrThrow(
     ctx.db.user.findUnique({
       where: { id: input.userId },

packages/db/prisma/schema.prisma

@@ -1572,6 +1572,9 @@ model SystemSettings {
   sessionIdleTimeout      Int?     @default(1800)     // Idle timeout in seconds (30min)
   // Concurrent session limit (kick-oldest strategy)
   maxConcurrentSessions   Int?     @default(3)
+  // MFA enforcement: roles that are required to have TOTP enabled (JSON array of SystemRole strings)
+  // e.g. ["ADMIN"] or ["ADMIN","MANAGER"]. Null = no enforcement.
+  requireMfaForRoles      Json?    @db.JsonB
   updatedAt               DateTime @updatedAt
 
   @@map("system_settings")