Repository navigation
fix(kernel): rate-limit inbound address rotation and require step-up - #142
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![dos[bot]](https://avatars.githubusercontent.com/in/4711427?s=60&v=4){kind=small}
#142 makes kernel.inbound.rotate R3, so the rotation confirmation can answer STEP_UP_REQUIRED. It now shows the step-up prompt and, after a fresh code, repeats the rotation with the idempotency key it already keeps. Cancelling clears the refused request, so a later code rotates nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LrvuThKGaPX1uDwUiwmGQ
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>
1aaf25d to
440528d
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the inbound email address rotation command (kernel.inbound.rotate) from risk level R2 to R3, requiring a fresh second-factor authentication (MFA) within the last 5 minutes. It updates the UI, documentation, and tests to enforce and verify this new security requirement. The review feedback suggests externalizing the hardcoded Vietnamese string in the settings page to translation files for i18n compliance, and refactoring a test assertion to check the parsed JSON error code directly rather than using a fragile string containment check.
| Đị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> |
There was a problem hiding this comment.
The newly added paragraph contains a hardcoded Vietnamese string. To comply with the project's internationalization (i18n) standards (which use use-intl as outlined in the architecture plan), this user-facing text should be externalized to the translation files (e.g., messages/vi.json and messages/en.json) and retrieved using a translation hook.
| callerMfa = false; | ||
| const noMfa = await rotate(randomUUID()); | ||
| expect(noMfa.status).toBe(403); | ||
| expect(JSON.stringify(await noMfa.json())).toContain('STEP_UP_REQUIRED'); |
There was a problem hiding this comment.
Asserting the error response by stringifying the entire JSON and using toContain is loose and fragile. It is better to parse the JSON response and assert precisely on the error code property to ensure robust and reliable test coverage.
| expect(JSON.stringify(await noMfa.json())).toContain('STEP_UP_REQUIRED'); | |
| const res = await noMfa.json(); | |
| expect(res.error?.code).toBe('STEP_UP_REQUIRED'); |
#142 made kernel.inbound.rotate R3, so the rotation confirmation can answer STEP_UP_REQUIRED. It now shows the step-up prompt and, after a fresh code, repeats the rotation with the idempotency key it already keeps. Cancelling clears the refused request, so a later code rotates nothing. The e2e stack gives orgs an inbound address (INBOUND_DOMAIN in compose.e2e.yml; nothing delivers to it), and a new test rotates it without a second factor, meets the prompt, enables two-factor and gets a new address. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LrvuThKGaPX1uDwUiwmGQ
This branch was successfully deployed
1 participant
Stacked on #141: the base is #141's branch, so the diff shows only the step-up change. Merge #141 first; GitHub then retargets this PR to
main. It cannot go first:main's copy ofapps/kernel/test/invariant/inbound-rotation.db.test.tsrotates as anaal1owner and expects success. I checked locally that this code failsmain's copy (2 tests,STEP_UP_REQUIRED) and passes #141's copy.Decision: rotation becomes R3
Plan §4.7 defines R3 as "financial, legal, or irreversible". Rotation is irreversible:
#37 had chosen R2 ("external effect"), arguing that irreversibility only ruled out R1. Under §4.7's own definition, though, irreversible is R3, so this raises the risk level instead of adding a one-off step-up flag on an R2 command.
What R3 changes in practice:
aal2with a second factor in the last 5 minutes, otherwise it answers 403STEP_UP_REQUIRED. A hijackedaal1owner session can no longer cut off the org's inbound mail.aal,session_idandmfaAt.What changes
packages/kernel/src/commands.ts:risk: 'R3', with the reasoning in the comment. Plan §7.6 is updated.inbound-rotation.db.test.ts): a new test.aal1, with a stale MFA timestamp (10 minutes old), or withaal2but no timestamp is refused withSTEP_UP_REQUIRED.deniedwith its code.{ aal, sessionId, mfaAt }.inbound-rotation.db.test.ts(product flow): the HTTP caller has fresh MFA. Without it, the RPC procedure answers 403STEP_UP_REQUIRED, and the audit sequence now includes that refusal.apps/app/src/routes/settings.tsx): the confirmation tells the owner that MFA in the last 5 minutes is needed and that rotations per day are limited. ASTEP_UP_REQUIREDanswer shows as the error text, as for every R3 action onmaintoday. The idempotency key is kept until a rotation succeeds.mainhas no step-up challenge screen yet.StepUpPromptcomes with the SPA MFA flows in feat: SPA auth flows: PKCE callbacks, password recovery, TOTP MFA and step-up #101 (Auth flows in the SPA: password recovery, MFA enrollment and callbacks #32), which wire approvals and member changes into it. Rotation should be added to that list: I left a note on feat: SPA auth flows: PKCE callbacks, password recovery, TOTP MFA and step-up #101.AUTH_STEP_UP=required, an owner without a fresh second factor cannot rotate from the SPA. Compose defaults toofflocally.Tests
Local runs, on
mainat #137 (df5f249) plus #141, against PostgreSQL 17:pnpm -s lintandpnpm -s typecheckpass.pnpm exec vitest run: 404 passed, 9 skipped.pnpm exec vitest run --config vitest.invariant.config.ts: 197 passed.pnpm docs:checkpasses.Issue #113 status
Closes #113
🤖 Generated with Claude Code
📌 TL;DR
This PR elevates the inbound email address rotation command from Risk Level 2 to Risk Level 3, enforcing a mandatory Multi-Factor Authentication (MFA) step-up requirement for the last 5 minutes. It updates the frontend to communicate this new security requirement, adds comprehensive tests for the
STEP_UP_REQUIREDrejection flow, and documents the rationale for this irreversible action.🎯 Type of Change
🔍 Changes Walkthrough
apps/app/src/routes/settings.tsxSTEP_UP_REQUIREDerror handling and idempotency key retention logic.apps/kernel/test/inbound-rotation.db.test.tsaal2vsaal1). Added assertions to verify that rotation fails withSTEP_UP_REQUIREDwhen MFA is stale/missing, and that successful rotations audit the specific session and MFA timestamp.apps/kernel/test/invariant/inbound-rotation.db.test.tskernel.inbound.rotaterejects actors withaal1, stale MFA, or missing MFA timestamps. Confirms that rejected attempts are audited with theSTEP_UP_REQUIREDcode and that the token remains unchanged.docs/plan/crove-os-kernel.mdaal,session_id,mfaAt).packages/kernel/src/commands.tsrisklevel ofkernel.inbound.rotatefrom'R2'to'R3'. Updated comments to explain that R3 triggers the runtime's MFA step-up enforcement (P11).📊 Architectural Flow
sequenceDiagram participant User as Owner (SPA) participant API as HTTP API participant Runtime as Kernel Runtime participant DB as PostgreSQL User->>API: POST /rotate (idempotencyKey) API->>Runtime: execute('kernel.inbound.rotate', {actor, orgId}) Note over Runtime: Check Risk Level (R3) alt MFA not fresh (last 5 mins) Runtime-->>API: 403 STEP_UP_REQUIRED API-->>User: Error: MFA required else MFA is fresh Runtime->>DB: Check Rate Limits (Advisory Lock) alt Rate Limit Exceeded Runtime-->>API: 429 RATE_LIMITED else Within Limits Runtime->>DB: Retire old token, Issue new token Runtime->>DB: Insert Audit Row (outcome: ok, aal, sessionId) Runtime-->>API: 200 OK (newToken) API-->>User: Success (newAddress) end end