From 440528daad344300ca5b22ca4b5a77c9fa9ca9b1 Mon Sep 17 00:00:00 2001
From: JOY <5027251+JOY@users.noreply.github.com>
Date: Thu, 15 Oct 2026 19:19:23 +0700
Subject: [PATCH] fix(kernel): require MFA step-up for inbound address rotation

kernel.inbound.rotate becomes R3. Plan section 4.7 defines R3 as financial,
legal or irreversible, and rotation is irreversible: the old address is
tombstoned for good and the suppliers holding it are cut off. As R3 the
runtime requires aal2 with a second factor in the last 5 minutes (P11) and
the audit row records the assurance used, so a hijacked aal1 owner session
can no longer cut off an org's inbound mail. For agents R3 means review,
which R2 already required.

The invariant suite gains a step-up test (aal1, stale and missing MFA
timestamps refused with STEP_UP_REQUIRED and audited, fresh MFA rotates);
the settings page tells the owner that MFA is needed.

Part of #113.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 apps/app/src/routes/settings.tsx              |  6 ++-
 apps/kernel/test/inbound-rotation.db.test.ts  | 28 +++++++++---
 .../invariant/inbound-rotation.db.test.ts     | 44 +++++++++++++++++--
 docs/plan/crove-os-kernel.md                  |  2 +-
 packages/kernel/src/commands.ts               |  9 ++--
 5 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/apps/app/src/routes/settings.tsx b/apps/app/src/routes/settings.tsx
index 8bd3caa4..9cf24769 100644
--- a/apps/app/src/routes/settings.tsx
+++ b/apps/app/src/routes/settings.tsx
@@ -86,7 +86,10 @@ function MemberControls({ member }: { member: MemberRow }) {
   );
 }
 
-// Owner only. One idempotency key per confirmation, so a retried request never rotates twice.
+// Owner only. One idempotency key per confirmation, so a retried request never rotates twice. R3 (#113): without a
+// second factor in the last 5 minutes the server answers STEP_UP_REQUIRED, shown below the buttons like any error;
+// the key is kept until a rotation succeeds, so a retry after a refusal can still rotate at most once. The fresh
+// MFA challenge screen (StepUpPrompt) lands with the SPA MFA flows (#32).
 function RotateInboundAddress() {
   const [confirming, setConfirming] = useState(false);
   const [idempotencyKey, setIdempotencyKey] = useState(() => crypto.randomUUID());
@@ -130,6 +133,7 @@ function RotateInboundAddress() {
         Địa chỉ hiện tại sẽ ngừng nhận thư ngay lập tức và không bao giờ được cấp lại. Thư gửi tới địa chỉ cũ sẽ bị bỏ qua mà không báo lỗi cho
         người gửi, vì vậy hãy gửi địa chỉ mới cho các nhà cung cấp.
       </p>
+      <p className="text-fg-3">Việc này cần xác thực hai lớp (MFA) trong 5 phút gần nhất. Mỗi tổ chức chỉ đổi được vài lần mỗi ngày.</p>
       <div className="flex gap-2">
         <Button variant="danger" disabled={rotate.isPending} onClick={() => rotate.mutate({ idempotencyKey })}>
           Xác nhận đổi địa chỉ
diff --git a/apps/kernel/test/inbound-rotation.db.test.ts b/apps/kernel/test/inbound-rotation.db.test.ts
index a61d0d1b..e248c3f9 100644
--- a/apps/kernel/test/inbound-rotation.db.test.ts
+++ b/apps/kernel/test/inbound-rotation.db.test.ts
@@ -163,8 +163,11 @@ suite('inbound address rotation on PostgreSQL', () => {
     await root.end();
   });
 
-  // The HTTP app as the API runs it; the RPC caller is whoever `caller` names.
+  // The HTTP app as the API runs it; the RPC caller is whoever `caller` names. Rotation is R3 (#113), so the caller
+  // has a fresh second factor unless `callerMfa` is false.
   let caller = '';
+  let callerMfa = true;
+  const SESSION = '5b0c1f2e-3d4a-4b5c-8d6e-7f8091a2b3c4';
   let app: ReturnType<typeof createHttpApp>;
   const httpApp = () =>
     (app ??= createHttpApp({
@@ -182,9 +185,9 @@ suite('inbound address rotation on PostgreSQL', () => {
         userId: caller,
         email: 'rot@example.com',
         name: null,
-        aal: 'aal1',
-        sessionId: randomUUID(),
-        mfaAt: null,
+        aal: callerMfa ? 'aal2' : 'aal1',
+        sessionId: SESSION,
+        mfaAt: callerMfa ? Math.floor(Date.now() / 1000) : null,
       }),
       pool,
       aiEnabled: false,
@@ -263,7 +266,7 @@ suite('inbound address rotation on PostgreSQL', () => {
       { token: oldToken, retired: true },
       { token: newToken, retired: false },
     ]);
-    expect(await rotateAudits(org.orgId)).toEqual([{ actor_id: users.owner, outcome: 'ok', detail: {} }]);
+    expect(await rotateAudits(org.orgId)).toMatchObject([{ actor_id: users.owner, outcome: 'ok', detail: { aal: 'aal2', sessionId: SESSION } }]);
     await expect(runtime.query('kernel.org.get', {}, { actor: asUser(users.owner), orgId: org.orgId })).resolves.toMatchObject({
       inboundAddress: `${newToken}@in.test`,
     });
@@ -282,6 +285,14 @@ suite('inbound address rotation on PostgreSQL', () => {
     caller = users.viewer;
     expect((await rotate(randomUUID())).status).toBe(403);
     expect(await tokenOf(org.orgId)).toBe(newToken);
+    // So is the owner without a fresh second factor (R3, #113): the SPA is told to run an MFA challenge.
+    caller = users.owner;
+    callerMfa = false;
+    const noMfa = await rotate(randomUUID());
+    expect(noMfa.status).toBe(403);
+    expect(JSON.stringify(await noMfa.json())).toContain('STEP_UP_REQUIRED');
+    callerMfa = true;
+    expect(await tokenOf(org.orgId)).toBe(newToken);
 
     // Mail to the new address and to the retired one: the API answers 202 to both (no oracle).
     expect((await deliver(oldToken, '7701')).status).toBe(202);
@@ -326,7 +337,12 @@ suite('inbound address rotation on PostgreSQL', () => {
     const stale = await rotate(key);
     expect(await stale.json()).toEqual({ json: latest });
     expect(await ledger(org.orgId)).toHaveLength(3);
-    expect((await rotateAudits(org.orgId)).map((a) => a.outcome)).toEqual(['ok', 'denied', 'ok']);
+    expect((await rotateAudits(org.orgId)).map((a) => [a.outcome, a.detail.code ?? null])).toEqual([
+      ['ok', null],
+      ['denied', 'FORBIDDEN'],
+      ['denied', 'STEP_UP_REQUIRED'],
+      ['ok', null],
+    ]);
   });
 
   it('drops mail queued for a token that is rotated away before the worker processes it', { timeout: 90_000 }, async () => {
diff --git a/apps/kernel/test/invariant/inbound-rotation.db.test.ts b/apps/kernel/test/invariant/inbound-rotation.db.test.ts
index ffec9a9a..98528b3a 100644
--- a/apps/kernel/test/invariant/inbound-rotation.db.test.ts
+++ b/apps/kernel/test/invariant/inbound-rotation.db.test.ts
@@ -1,6 +1,7 @@
-// Invariant suite (Decision 7): inbound address rotation (issue #37, plan §7.6). Only the owner rotates, only the
-// API login role may call the rotation definer, every token ever issued stays in the ledger, a retired token never
-// routes mail and is never issued again, even when the generator returns it. Keep this directory self-contained:
+// Invariant suite (Decision 7): inbound address rotation (issue #37, plan §7.6). Only the owner rotates, with a
+// fresh second factor (R3 since #113), only the API login role may call the rotation definer, every token ever
+// issued stays in the ledger, a retired token never routes mail and is never issued again, even when the generator
+// returns it. Keep this directory self-contained:
 // import package code under test, never helpers from outside test/invariant/. Set CROVE_TEST_ADMIN_URL to a
 // superuser URL; each run creates and drops its own database.
 import { randomUUID } from 'node:crypto';
@@ -246,6 +247,43 @@ suite('inbound address rotation invariants on PostgreSQL', () => {
     ).rejects.toThrow(/permission denied for function inbound_rotate/);
   });
 
+  it('needs a fresh second factor to rotate (R3, P11, #113): an aal1 or stale owner session is refused and audited', async () => {
+    const before = await tokenOf(org.orgId);
+    const ledgerBefore = await ledger(org.orgId);
+    const audits = (await rotateAudits(org.orgId)).length;
+    const stale = { ...asMfa(users.owner), mfaAt: Math.floor(Date.now() / 1000) - 600 };
+    const noTimestamp = { type: 'user' as const, userId: users.owner, aal: 'aal2' as const, sessionId: '0f6c3a52-8d3b-4c1e-9a77-5b2f0e9d1c00' };
+    for (const actor of [asUser(users.owner), stale, noTimestamp]) {
+      await expect(runtime.execute('kernel.inbound.rotate', {}, { actor, orgId: org.orgId, idempotencyKey: randomUUID() })).rejects.toMatchObject({
+        code: 'STEP_UP_REQUIRED',
+        status: 403,
+      });
+    }
+    // Nothing changed: same token, same ledger. Each refusal is audited with its code.
+    expect(await tokenOf(org.orgId)).toBe(before);
+    expect(await ledger(org.orgId)).toEqual(ledgerBefore);
+    const refused = (await rotateAudits(org.orgId)).slice(audits);
+    expect(refused.map((r) => [r.actor_id, r.outcome, r.detail.code])).toEqual([
+      [users.owner, 'denied', 'STEP_UP_REQUIRED'],
+      [users.owner, 'denied', 'STEP_UP_REQUIRED'],
+      [users.owner, 'denied', 'STEP_UP_REQUIRED'],
+    ]);
+
+    // With a second factor in the last 5 minutes the owner rotates, and the audit row records the assurance used.
+    const fresh = asMfa(users.owner);
+    const { inboundToken } = await runtime.execute<{ inboundToken: string }>('kernel.inbound.rotate', {}, {
+      actor: fresh,
+      orgId: org.orgId,
+      idempotencyKey: randomUUID(),
+    });
+    expect(await tokenOf(org.orgId)).toBe(inboundToken);
+    expect((await rotateAudits(org.orgId)).at(-1)).toEqual({
+      actor_id: users.owner,
+      outcome: 'ok',
+      detail: { aal: 'aal2', sessionId: fresh.sessionId, mfaAt: fresh.mfaAt },
+    });
+  });
+
   it('never issues a token that ever existed, even when the generator returns one (forced collision)', async () => {
     const retired = (await ledger(org.orgId)).find((r) => r.retired)?.token ?? '';
     const active = await tokenOf(otherOrg);
diff --git a/docs/plan/crove-os-kernel.md b/docs/plan/crove-os-kernel.md
index 5e4c6f0f..5a19d889 100644
--- a/docs/plan/crove-os-kernel.md
+++ b/docs/plan/crove-os-kernel.md
@@ -1215,7 +1215,7 @@ Superseded design (kept only so older references resolve): Crove-issued session
 
 - **Inbound email:**
   - Opaque, rotatable addresses stored in `inbound_address`. Old addresses are tombstoned and never reissued. No slug-based routing.
-  - Rotation is the command `kernel.inbound.rotate` (R2: suppliers hold the address, so the change has an effect outside Crove and cannot be undone), owner only, run through `execute()` with an audit row and an idempotency key. The definer `inbound_rotate()` retires the active token and draws a new one from `random_token()`; the UNIQUE constraint on `inbound_address.token` decides, and a collision with any token that ever existed draws again.
+  - Rotation is the command `kernel.inbound.rotate`, owner only, run through `execute()` with an audit row and an idempotency key. It is R3 since #113 (it was R2 in #37): the old address is tombstoned for good and the suppliers holding it are cut off, which is the "irreversible" case of §4.7, so a hijacked `aal1` owner session must not be able to do it. As R3 it needs a second factor in the last 5 minutes (P11; 403 `STEP_UP_REQUIRED` otherwise), and the audit row records `aal`, `session_id` and the MFA timestamp. For agents R3 means review, which R2 already required, so only users see a change. The definer `inbound_rotate()` retires the active token and draws a new one from `random_token()`; the UNIQUE constraint on `inbound_address.token` decides, and a collision with any token that ever existed draws again.
   - No grace period: from the moment of rotation, mail to the old token is dropped by the worker exactly like mail to a token that never existed. The API answers 202 to both, so a sender learns nothing about which tokens exist. The settings page (owner only) asks for confirmation and shows the new address.
   - Mail already queued for the old token when the rotation runs is dropped too: the worker resolves the org when it processes a message, not when the API accepts it. That is right for a leaked address, but a supplier's legitimate mail in that window is lost without a bounce, and today nothing tells the owner (only the worker logs the drop). Surfacing a count of such drops to the owner is a follow-up.
   - Rate limit (#113): every rotation leaves a permanent tombstone, so `inbound_rotate()` refuses a rotation when the org rotated 3 times in the last 24 hours or 50 times in total (`INBOUND_ROTATE_LIMIT_PER_DAY`, `INBOUND_ROTATE_LIMIT_TOTAL`, passed to the definer as transaction-local settings; a malformed value is refused): 429 `RATE_LIMITED` with `retryAfterSeconds` for the 24-hour rule, nothing written. The count runs under a per-org advisory lock with a 3 s lock timeout, so the limit holds across API instances, of concurrent rotations at the boundary exactly one wins, and a wait past the timeout is refused (429, rule `inbound_rotate_busy`). A replay of an idempotency key is answered before the definer and does not count.
diff --git a/packages/kernel/src/commands.ts b/packages/kernel/src/commands.ts
index 6406440b..83a6a68a 100644
--- a/packages/kernel/src/commands.ts
+++ b/packages/kernel/src/commands.ts
@@ -235,9 +235,10 @@ function assertRotateLimit(name: string, value: number): void {
 // Rotates the org's inbound email address (plan §7.6). The old token is tombstoned and never issued again; mail
 // to it is dropped by the worker like mail to an unknown token, including mail that was already queued for it
 // when the rotation ran (the worker resolves the org when it processes a message, not when the API accepts it), so
-// a supplier's mail in flight is lost without a bounce. R2: the address is shared with suppliers, so changing it
-// has an effect outside Crove (their mail to the old address stops arriving), and it cannot be undone, which rules
-// out R1. Owner only, enforced here and again in the definer. Over the limits the definer refuses with
+// a supplier's mail in flight is lost without a bounce. R3 (plan §4.7: irreversible), since #113: the old address
+// is tombstoned for good and the suppliers holding it are cut off, so a hijacked aal1 owner session must not be
+// able to do it. As R3 it needs a fresh second factor (P11, enforced by the runtime) and its audit row records the
+// assurance used. Owner only, enforced here and again in the definer. Over the limits the definer refuses with
 // INBOUND_ROTATE_RATE_LIMITED (429 RATE_LIMITED) before writing anything.
 export function inboundRotateCommand(limits: InboundRotateLimits = DEFAULT_INBOUND_ROTATE_LIMITS) {
   assertRotateLimit('perDay', limits.perDay);
@@ -245,7 +246,7 @@ export function inboundRotateCommand(limits: InboundRotateLimits = DEFAULT_INBOU
   return defineCommand({
     name: 'kernel.inbound.rotate',
     input: z.object({}),
-    risk: 'R2',
+    risk: 'R3',
     access: ['owner'],
     handler: async (ctx) => {
       if (ctx.actor.type !== 'user') throw new DomainError('FORBIDDEN', 403, 'Chỉ chủ sở hữu mới đổi được địa chỉ nhận hóa đơn');