feat(12.4): MFA + Cross-Device Approval

[FSM1]

Summary

• Backend bulletin board API for cross-device factor transfer (DeviceApproval entity, service, controller, 5 REST endpoints with 5-minute TTL auto-expiry)
• MFA hooks + Zustand store wrapping Core Kit MFA operations (enableMFA, factor management, key details) with REQUIRED_SHARE login flow branching and temporary auth
• MFA enrollment wizard (3-step: explain → show 24-word recovery phrase → confirm) with Settings Security tab, authorized device management, and recovery phrase regeneration
• Cross-device approval flow — new device waiting screen with countdown, ECIES ephemeral key exchange, approval modal on existing device, recovery phrase input fallback, post-login MFA enrollment prompt
• API client regeneration with type-safe generated functions replacing manual calls, plus permanent tssPubKey stability guard in enableMfa()

Requirements

Closes MFA-01, MFA-02, MFA-03, MFA-04

Verification

• 5/5 phase success criteria verified against codebase
• pnpm --filter api build + pnpm --filter web build both pass
• 467/467 API tests pass
• tssPubKey stability check guards keypair identity after MFA enrollment

Test plan

• Enable MFA from Settings > Security tab via enrollment wizard
• Verify 24-word recovery phrase is displayed correctly in 4-column grid
• Approve a new device from an existing authenticated device (two browser profiles)
• Recover access using recovery phrase on a new device
• Verify keypair (tssPubKey) remains identical after MFA enrollment
• Revoke a device factor from Settings > Security
• Regenerate recovery phrase from Settings > Security
• Verify non-MFA login flow is completely unaffected

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • MFA enrollment wizard (24‑word recovery phrase), recovery fallback, dismissible post‑login prompt, Security tab with device management and revoke
  • Cross‑device approval UI: request, waiting screen, poll status, approve/deny, cancel with 5‑minute auto‑expiry

• API

  • New device-approval REST endpoints and regenerated API client; frontend hooks and modal integrations for cross‑device flow

• Security

  • Login rate limiting and SIWE nonce consumption to mitigate replay

• Documentation

  • Roadmap, design, research, verification, and security review updated for Phase 12.4 (MFA + Cross‑Device)

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Implements Phase 12.4: MFA + Cross‑Device Approval. Adds a DeviceApproval bulletin‑board API, backend wiring and tests, frontend MFA state/hooks/UI (enrollment, recovery, approval), Core Kit login changes to support REQUIRED_SHARE, regenerated API client/models, styles, and extensive planning/design/verification docs.

Changes

Cohort / File(s)	Summary
Planning & Docs ./learnings/2026-02-15-phase-12.4-mfa-cross-device.md, .planning/REQUIREMENTS.md, .planning/ROADMAP.md, .planning/STATE.md, .planning/phases/12.4-mfa-cross-device/*, .planning/todos/pending/*, designs/*	New Phase 12.4 research, plans, verification, security review, todos, design tokens and mockups documenting MFA + cross‑device approval decisions and verification artifacts.
API: DeviceApproval Module apps/api/src/device-approval/device-approval.entity.ts, apps/api/src/device-approval/device-approval.service.ts, apps/api/src/device-approval/device-approval.controller.ts, apps/api/src/device-approval/device-approval.module.ts, apps/api/src/device-approval/dto/*, apps/api/src/device-approval/*.spec.ts	New TypeORM entity, DTOs, service, controller, module and unit tests implementing 5 JWT‑protected endpoints (request, status, pending, respond, cancel) with TTL (5 min), read-time auto‑expiry, and explicit state transitions and validations.
API Wiring & OpenAPI apps/api/src/app.module.ts, apps/api/scripts/generate-openapi.ts, packages/api-client/openapi.json	Registers DeviceApprovalModule and entity; updates OpenAPI generator and regenerates spec to include device‑approval paths, models, and client schemas.
Web: MFA Core & State apps/web/src/stores/mfa.store.ts, apps/web/src/hooks/useMfa.ts	Adds Zustand MFA store and useMfa hook exposing MFA operations (check, enable, inputFactor, recover, list/delete, regenerate) with FactorInfo typing and BN/hex handling for Core Kit integration.
Web: Device Approval Flow & API client apps/web/src/hooks/useDeviceApproval.ts, apps/web/src/api/device-approval/device-approval.ts, apps/web/src/services/device-approval.service.ts, apps/web/src/api/models/*	Adds useDeviceApproval hook (requester + approver lifecycles), generated Orval React‑Query client and models, and a backward‑compatible wrapper service used by UI.
Web: Auth / CoreKit Integration apps/web/src/lib/web3auth/hooks.ts, apps/web/src/hooks/useAuth.ts	loginWithCoreKit now returns CoreKitLoginResult ('logged_in'
Web: UI Components & Routes apps/web/src/components/mfa/*, apps/web/src/routes/Login.tsx, apps/web/src/routes/Settings.tsx, apps/web/src/components/layout/AppShell.tsx	Adds MFA UI suite (MfaEnrollmentWizard, RecoveryPhraseGrid/Input, SecurityTab, AuthorizedDevices, DeviceWaitingScreen, DeviceApprovalModal, MfaEnrollmentPrompt); integrates REQUIRED_SHARE flows into Login and mounts modal/prompt in AppShell; Settings becomes tabbed.
Web: Styling & Dependencies apps/web/src/App.css, apps/web/package.json, apps/web/src/lib/device/info.ts, apps/web/src/stores/device-registry.store.ts	Large CSS additions for MFA components; adds runtime deps bn.js and @tkey/common-types (+ @types/bn.js dev); minor doc/comment updates.
Web: API client regen & models apps/web/src/api/models/*, apps/web/src/api/device-approval/*, apps/web/src/services/device-approval.service.ts	Generated API models/types and React‑Query client (Orval) added; wrapper preserves backward compatibility and normalizes responses (e.g., createRequest returning requestId).
Security & Auth hardening apps/api/src/auth/*	Adds login throttling, SIWE nonce consumption with Redis, stricter login DTO validation for publicKey format, PII redaction in auth responses, and Redis lifecycle handling in AuthService.

Sequence Diagram(s)

sequenceDiagram
    participant NewDevice as New Device
    participant Backend as Backend API
    participant ExistingDevice as Existing Device

    NewDevice->>Backend: POST /device-approval/request (ephemeralPubKey, deviceId, deviceName)
    Backend-->>NewDevice: { requestId } (TTL=5m)
    NewDevice->>Backend: Poll GET /device-approval/:id/status
    Backend-->>NewDevice: pending

    ExistingDevice->>Backend: GET /device-approval/pending
    Backend-->>ExistingDevice: pending list (includes ephemeralPubKey)
    ExistingDevice->>ExistingDevice: ECIES encrypt factorKey with ephemeralPubKey
    ExistingDevice->>Backend: POST /device-approval/:id/respond (approve + encryptedFactorKey)
    Backend->>Backend: set status='approved'

    NewDevice->>Backend: Poll GET /device-approval/:id/status
    Backend-->>NewDevice: approved + encryptedFactorKey
    NewDevice->>NewDevice: ECIES decrypt, input factor, commit to Core Kit
    NewDevice->>Backend: completeRequiredShare (real publicKey) -> full auth

Loading

sequenceDiagram
    participant User as Approver
    participant Modal as DeviceApprovalModal
    participant Service as useDeviceApproval
    participant API as Backend API
    participant CoreKit as Core Kit

    User->>Modal: modal mounts (app visible)
    Modal->>Service: pollPendingRequests()
    Service->>API: GET /device-approval/pending
    API-->>Service: pending items
    Service-->>Modal: display first request

    User->>Modal: Click "Approve"
    Modal->>Service: approveRequest(requestId)
    Service->>CoreKit: getCurrentFactorKey()
    Service->>Service: ECIES encrypt factorKey with ephemeralPubKey
    Service->>API: POST /device-approval/:id/respond (approve + encryptedFactorKey)
    API-->>Service: success
    Service->>Modal: remove request from UI

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• feat(12): Core Kit Identity Provider Foundation #123: Overlaps Core Kit auth/login surfaces — related to REQUIRED_SHARE handling and web3auth/login hook changes.
• feat: SIWE wallet login + unified identity (Phase 12.3) #126: Touches web3auth and useAuth flows; related to the new CoreKitLoginResult and pending REQUIRED_SHARE state handling.
• feat(12.2): Encrypted Device Registry #125: Related changes to device-registry and auth flow that interact with device-approval polling and approval behavior.

Suggested labels

render-preview

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 76.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(12.4): MFA + Cross-Device Approval' is a concise, clear summary of the main change in the PR. It accurately captures the primary feature being introduced (MFA with cross-device approval functionality for Phase 12.4) and follows conventional commit conventions.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/phase-12.4-mfa-cross-device

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 12

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

.planning/REQUIREMENTS.md (1)

414-414: ⚠️ Potential issue | 🟡 Minor

Stale "Last updated" date.

The footer still reads 2026-02-11 but this PR updates MFA requirement statuses as of 2026-02-15.

🤖 Fix all issues with AI agents

In `@apps/api/src/device-approval/device-approval.entity.ts`:
- Around line 3-33: Add database indexes to DeviceApproval to support the common
query patterns: create a composite index on (userId, status) — optionally
include expiresAt if you want the DB to optimize TTL-style queries — and ensure
expiresAt is indexed for efficient scans of expired rows; update the Entity
declaration (DeviceApproval) to include these indexes (using `@Index` decorators
or the Entity's indices option) referencing the userId, status, and expiresAt
fields so getPending queries do not cause full table scans.

In `@apps/api/src/device-approval/dto/create-approval.dto.ts`:
- Around line 4-28: CreateApprovalDto currently lacks strict length/format
checks: add `@MaxLength`(255) to deviceName; add `@Length`(64,64) and
`@IsHexadecimal`() to deviceId to enforce SHA-256 hex format; and add
`@Length`(130,130) to ephemeralPublicKey (in addition to `@IsHexadecimal`()) to
enforce the uncompressed secp256k1 130-char hex key; update imports for any
added class-validator decorators.

In `@apps/api/src/device-approval/dto/respond-approval.dto.ts`:
- Around line 24-30: The respondedByDeviceId field in RespondApprovalDto lacks
format validation; add the same SHA-256 constraints used for
CreateApprovalDto.deviceId by annotating respondedByDeviceId with
`@IsHexadecimal`() and `@Length`(64, 64) (and ensure these validators are imported
from class-validator) so the DTO enforces a 64-char hex SHA-256 device ID.

In `@apps/web/src/components/mfa/DeviceApprovalModal.tsx`:
- Around line 63-87: The UI shows stale errors because respondError isn't
cleared when currentRequest changes; add a useEffect in the DeviceApprovalModal
component that watches currentRequest and calls setRespondError(null) (and
optionally setIsResponding(false)) whenever currentRequest changes so each new
request starts with a clean error state; reference the existing state setter
setRespondError and the currentRequest variable to implement this reset.

In `@apps/web/src/components/mfa/MfaEnrollmentPrompt.tsx`:
- Around line 8-17: getDismissalKey currently embeds the raw user email into the
localStorage key (STORAGE_KEY_PREFIX + email) which persists PII; change it to
use a non-PII identifier by replacing the email with an existing
userId/identifier_hash from useAuthStore.getState() (preferable) or compute the
SHA-256 hash of the email following the project's identifier hashing pattern;
update getDismissalKey to pull useAuthStore.getState().userId or
useAuthStore.getState().identifier_hash if available, otherwise compute
SHA-256(email) and return `${STORAGE_KEY_PREFIX}${identifier}` so the key no
longer contains plaintext PII.

In `@apps/web/src/components/mfa/MfaEnrollmentWizard.tsx`:
- Around line 41-44: handleGoToStep2 currently always calls handleEnableMfa
which can re-enable MFA and regenerate the recovery phrase on repeated
navigation; change handleGoToStep2 (and the analogous call around lines 50-52)
to first check whether MFA/enrollment data or the generated
mnemonic/recoveryPhrase already exists (e.g., from component state or a prop
like mnemonic/recoveryPhrase or isMfaEnabled) and only call handleEnableMfa if
that data is missing/false, otherwise just setStep(2); this ensures enableMfa()
is skipped on return visits and prevents duplicate generation or re-enablement.

In `@apps/web/src/hooks/useDeviceApproval.ts`:
- Around line 277-284: The approveRequest callback calls
coreKit.getCurrentFactorKey() without guarding for null; add a null check after
calling coreKit.getCurrentFactorKey() in the approveRequest function (the
useCallback named approveRequest) and handle the null case (throw a clear error
or return early) before accessing factorKeyResult.factorKey, so factorKeyHex and
factorKeyBytes are only computed when factorKeyResult is non-null.
- Around line 196-200: In approveRequest, guard against
coreKit.getCurrentFactorKey() returning null by checking the result and throwing
a clear error if factorKey is missing (e.g., if (!factorKeyResult?.factorKey)
throw new Error('Factor key unavailable')) before accessing .factorKey;
additionally, after any call to unwrapKey that returns factorKeyBytes (used in
approveRequest and handleApprovalSuccess), explicitly zero-fill the byte array
(factorKeyBytes.fill(0)) immediately after you convert/derive the hex/string
form and before leaving the function, and ensure any other sensitive buffers are
similarly cleared (the existing clearEphemeralKey call can remain for the
ephemeral secretKey).

In `@apps/web/src/hooks/useMfa.ts`:
- Around line 88-101: The TSS public key mismatch check in useMfa (variables
preMfaTssPub and postMfaTssPub after calling enableMFA) only logs the critical
condition but lets execution continue; change this to surface the failure to the
caller by throwing an Error (or rejecting the promise) when preX !== postX ||
preY !== postY so the enrollment flow can abort or show a user-facing warning;
ensure the thrown error includes the pre/post key details and use the same check
location in useMfa around coreKit.getKeyDetails().tssPubKey so callers of
enableMFA or the hook can catch and handle the failure.

In `@apps/web/src/routes/Login.tsx`:
- Around line 109-112: The polling loop currently swallows errors from
completeRequiredShare() causing backend/auth failures to never surface; update
handleApprovalSuccess to explicitly await completeRequiredShare() inside its
try/catch and on any error set the component state approvalError (or call the
existing error handler) and stop further polling, and implement
handleApprovalComplete to receive and handle/report errors (e.g., set
approvalError or trigger a UI failure path) so that failures from
completeRequiredShare(), device factor creation, or vault loading are propagated
to the UI instead of being ignored; ensure the polling loop’s generic catch no
longer silently continues when the failure originated from
completeRequiredShare() by rethrowing or delegating to the approvalError setter
so errors are visible.
- Around line 103-107: The handleRecoveryComplete callback can let rejections
from completeRequiredShare propagate uncaught; wrap the await
completeRequiredShare() call in a try/catch inside handleRecoveryComplete
(function handleRecoveryComplete) and handle errors by calling the existing UI
error reporting path (e.g., set an error state, call the toast/notify helper
used elsewhere, or invoke a provided onError handler) and optionally log
details; ensure the catch prevents the unhandled rejection and surfaces a
user-friendly message on the MFA screen while preserving any original logging of
the thrown error.

In `@apps/web/src/services/device-approval.service.ts`:
- Around line 37-40: The createRequest implementation silently defaults
result.requestId to '' which masks missing IDs and causes downstream polling
with an empty id; update the createRequest function (which calls
deviceApprovalControllerCreateRequest(dto) and accepts CreateApprovalDto) to
validate that result.requestId is present and non-empty and throw a clear Error
(including context like "device approval create returned empty requestId") when
it's missing instead of returning an empty string, then return the validated
requestId.

🟡 Minor comments (13)

designs/DESIGN.md-230-230 (1)

230-230: ⚠️ Potential issue | 🟡 Minor

Design decision log references "12-word phrase" but the system uses 24 words.

Every other reference in this PR (RecoveryPhraseGrid, research doc, plan docs, learnings) consistently describes a 24-word BIP39 mnemonic from keyToMnemonic(). This design decision entry says "4x3 word grid with numbered cells" / "12-word phrase," which contradicts the actual implementation (4×6 = 24 words, or 6×4).

-| 2026-02-15 | 12.4  | Recovery phrase shown as 4x3 word grid with numbered cells         | Clear, scannable layout for 12-word phrase                  |
+| 2026-02-15 | 12.4  | Recovery phrase shown as 4x6 word grid with numbered cells         | Clear, scannable layout for 24-word phrase                  |

apps/web/src/api/models/deviceApprovalControllerGetPending200Item.ts-9-16 (1)

9-16: ⚠️ Potential issue | 🟡 Minor

Add required array to the OpenAPI schema in the getPending endpoint to mark all fields as required.

The generated frontend type has all fields as optional because the @ApiResponse schema on the getPending endpoint (lines 100-115 in the controller) is missing a required array. OpenAPI spec treats properties as optional by default when required is not specified.

Update the schema to include required: ['requestId', 'deviceId', 'deviceName', 'ephemeralPublicKey', 'createdAt', 'expiresAt'] in the items object. After fixing the schema, run pnpm api:generate to regenerate the frontend client types with required fields.

apps/web/src/routes/Login.tsx-192-223 (1)

192-223: ⚠️ Potential issue | 🟡 Minor

Placeholder href="#" links in footer.

The help, privacy, and terms links use href="#" which scrolls to the top of the page. Consider using href pointing to actual pages, or using <button> elements if these are not yet implemented, to avoid unexpected scroll behavior.

apps/web/src/components/mfa/DeviceApprovalModal.tsx-96-101 (1)

96-101: ⚠️ Potential issue | 🟡 Minor

aria-modal="true" without focus trap breaks screen reader expectations.

The dialog backdrop declares aria-modal="true", which tells assistive technology that content behind the modal is inert. However, there's no focus management — no focus trap on mount, no return-focus on dismiss. Keyboard users can tab out of the modal to background elements.

apps/web/src/components/mfa/MfaEnrollmentPrompt.tsx-42-60 (1)

42-60: ⚠️ Potential issue | 🟡 Minor

'default' fallback key risks cross-user prompt suppression on shared browsers.

When userEmail is null (e.g., wallet-only auth, or email not yet loaded), the key falls back to cipherbox_mfa_prompt_dismissed_default. If one user dismisses the prompt before their email is populated, a different user on the same browser will never see the prompt because they'll read the same default key.

Consider deferring the check until the email/userId is available, or not persisting to localStorage when the user identity is unknown.

apps/web/src/components/mfa/DeviceApprovalModal.tsx-68-68 (1)

68-68: ⚠️ Potential issue | 🟡 Minor

Add validation for required API response fields before use.

All fields accessed with non-null assertions (requestId, ephemeralPublicKey, expiresAt, createdAt) are marked as optional in the generated type DeviceApprovalControllerGetPending200Item. The guard on line 64 only checks if currentRequest exists; it doesn't validate these individual fields. If the API response omits any of them, undefined will be silently passed to approveRequest(), denyRequest(), or to the Date constructor, causing runtime errors or invalid behavior.

Add an early validation guard that ensures all required fields are present before proceeding:

if (!currentRequest?.requestId || !currentRequest?.ephemeralPublicKey || !currentRequest?.expiresAt || !currentRequest?.createdAt) {
  return; // or throw
}

Also applies to: lines 81, 92–93

apps/api/scripts/generate-openapi.ts-156-156 (1)

156-156: ⚠️ Potential issue | 🟡 Minor

Update OpenAPI tag casing to match other tags: both controller and config need changes.

The controller's @ApiTags('device-approval') and the .addTag('device-approval', ...) in generate-openapi.ts are currently consistent with each other, but both deviate from the PascalCase pattern used for all other tags ('Health', 'Root', 'Auth', 'Identity', 'Vault', 'Files', 'IPFS', 'IPNS').

To align with the established convention, both the controller and this script must be updated to use 'DeviceApproval':

Changes required

apps/api/src/device-approval/device-approval.controller.ts (line 24):

-@ApiTags('device-approval')
+@ApiTags('DeviceApproval')

apps/api/scripts/generate-openapi.ts (line 156):

-    .addTag('device-approval', 'Cross-device approval endpoints')
+    .addTag('DeviceApproval', 'Cross-device approval endpoints')

apps/web/src/components/mfa/DeviceWaitingScreen.tsx-31-49 (1)

31-49: ⚠️ Potential issue | 🟡 Minor

Add eslint-disable comment to document intentional mount-only effect.

The requestApproval and cancelRequest functions are wrapped in useCallback in the useDeviceApproval hook with stable dependencies, so they maintain consistent identity across renders. However, ESLint's exhaustive-deps rule still flags missing dependencies. Add an explicit eslint-disable comment to document this is intentional:

Proposed fix

  useEffect(() => {
    startTimeRef.current = Date.now();
    requestApproval();

    // Countdown timer
    countdownRef.current = setInterval(() => {
      const elapsed = Date.now() - startTimeRef.current;
      const remaining = Math.max(0, APPROVAL_TTL_MS - elapsed);
      setCountdown(remaining);
    }, 1000);

    return () => {
      if (countdownRef.current) {
        clearInterval(countdownRef.current);
      }
      // Cancel request on unmount
      cancelRequest();
    };
-  }, []);
+  // eslint-disable-next-line react-hooks/exhaustive-deps -- mount-only: requestApproval and cancelRequest have stable identity
+  }, []);

apps/web/src/components/mfa/RecoveryInput.tsx-55-63 (1)

55-63: ⚠️ Potential issue | 🟡 Minor

Keyboard handler bypasses the isRecovering guard.

The button disables itself when isRecovering is true, but handleKeyDown calls handleRecover() unconditionally. Pressing Enter during an in-flight recovery can trigger a duplicate concurrent call.

Proposed fix

  const handleKeyDown = useCallback(
    (e: React.KeyboardEvent) => {
-     if (e.key === 'Enter' && !e.shiftKey) {
+     if (e.key === 'Enter' && !e.shiftKey && !isRecovering) {
        e.preventDefault();
        handleRecover();
      }
    },
-   [handleRecover]
+   [handleRecover, isRecovering]
  );

apps/web/src/components/mfa/AuthorizedDevices.tsx-29-29 (1)

29-29: ⚠️ Potential issue | 🟡 Minor

Factor list and count do not refresh after device revocation.

After deleteFactor completes, the Core Kit state changes internally, but factors is derived via useMemo(() => getFactors(), [getFactors]) where getFactors has dependency [coreKit]. Since coreKit is a stable singleton from context, the getFactors reference remains stable, and the useMemo does not re-execute. The revoked device will remain visible, and factorCount will not update (since setFactorDetails is only called in checkMfaStatus, never after deleteFactor).

Add a useEffect in AuthorizedDevices to call checkMfaStatus() after successful deletion, or restructure to read factors from a reactive Zustand store selector instead of a synchronous getter.

apps/web/src/hooks/useAuth.ts-243-268 (1)

243-268: ⚠️ Potential issue | 🟡 Minor

Pending state set before temp-login can fail — error leaves stale MFA state.

setPendingCipherboxJwt / setPendingAuthMethod are written (lines 246-247) before the authApi.login call (line 253). If the temp login throws, the catch block re-throws without clearing pending state. The component then sees isRequiredShare === true (from Core Kit) with pending state populated but no valid access token, which means cross-device approval API calls will fail with 401s.

Move the state writes after the temp login succeeds, or clear them in the catch block:

Proposed fix — set pending state after temp login succeeds

         if (coreKitStatus === 'required_share') {
-          // MFA enabled but device factor missing.
-          // Store pending auth info for later completion after factor input.
-          setPendingCipherboxJwt(cipherboxJwt);
-          setPendingAuthMethod('google');
-
           // Obtain temporary backend access token so the new device can
           // call bulletin board API endpoints (device-approval/*).
-          // Uses placeholder publicKey since Core Kit is in REQUIRED_SHARE
-          // state and we can't export the TSS key yet.
           const tempLoginResponse = await authApi.login({
             idToken: cipherboxJwt,
             publicKey: `pending-core-kit-${userId}`,
             loginType: 'corekit',
           });
           setAccessToken(tempLoginResponse.accessToken);
 
+          // Store pending auth info only after temp token is secured
+          setPendingCipherboxJwt(cipherboxJwt);
+          setPendingAuthMethod('google');
+
           if (email) {
             setUserEmail(email);
           }

Apply the same reordering in loginWithEmail (lines 305-306 → after line 313) and loginWithWallet (lines 346-347 → after line 354).

packages/api-client/openapi.json-810-852 (1)

810-852: ⚠️ Potential issue | 🟡 Minor

Add required arrays to inline response schemas in device-approval endpoints.

The response schemas for POST /device-approval/request, GET /device-approval/{requestId}/status, and GET /device-approval/pending define properties without a required array. This causes OpenAPI code generators to type all response fields as optional (e.g., requestId?: string), even though the backend always returns them as non-optional.

Fix by adding required arrays to the @ApiResponse decorator schemas in the controller:

`@ApiResponse`({
  status: 201,
  description: 'Approval request created',
  schema: {
    properties: {
      requestId: { type: 'string', format: 'uuid' },
    },
    required: ['requestId'],  // Add this
  },
})

Apply similarly to the status (required: ['status']) and pending (items.required: ['requestId', 'deviceId', 'deviceName', 'ephemeralPublicKey', 'createdAt', 'expiresAt']) endpoints.

Also: The device-approval tag uses kebab-case while other tags use PascalCase (Auth, Identity, IPFS, Vault, Root). Consider renaming to DeviceApproval for consistency.

apps/web/src/hooks/useMfa.ts-159-192 (1)

159-192: ⚠️ Potential issue | 🟡 Minor

getFactors may produce duplicate entries when a factor has multiple descriptions, causing React key warnings in AuthorizedDevices.

shareDescriptions is Record<string, string[]> — each factor pub hex maps to an array of description strings. The inner loop (line 166) iterates all descriptions per factor and pushes a separate FactorInfo for each. If a factor has multiple descriptions, the same factorPubHex appears multiple times in the returned array.

In AuthorizedDevices (line 114), factors are mapped directly with key={device.factorPubHex}. Duplicate factorPubHex values would create duplicate React keys and cause rendering issues.

However, current usage shows createFactor is only called once per factor with a single shareDescription. If Core Kit's design reserves the array structure for future multi-description support and never actually creates multiple descriptions per factor in practice, the inner loop effectively iterates once and duplicates don't manifest. Worth clarifying with the team whether this is intentional defensive code or should be deduplicated for safety.

🧹 Nitpick comments (23)

apps/web/src/components/mfa/RecoveryPhraseGrid.tsx (1)

9-11: Consider a stricter type or runtime guard for the words prop.

The prop accepts string[] of any length, but the component is documented and styled for exactly 24 words. A mismatched array would silently render a broken grid. A lightweight guard or tuple type could catch misuse early.

💡 Optional: add a dev-time assertion

 type RecoveryPhraseGridProps = {
   words: string[];
 };
 
 export function RecoveryPhraseGrid({ words }: RecoveryPhraseGridProps) {
+  if (import.meta.env.DEV && words.length !== 24) {
+    console.warn(`RecoveryPhraseGrid: expected 24 words, got ${words.length}`);
+  }
   return (

apps/api/src/device-approval/dto/respond-approval.dto.ts (1)

14-22: Consider adding @MaxLength to encryptedFactorKey.

ECIES ciphertext for a 32-byte factor key should be a bounded size. Without a cap, an attacker with a valid JWT could store arbitrarily large hex strings in the encrypted_factor_key text column. A generous limit (e.g., 1024) would prevent abuse while accommodating the ECIES overhead.

apps/api/src/device-approval/device-approval.entity.ts (2)

26-27: Consider naming the createdAt column explicitly for consistency.

All other temporal/nullable columns use explicit name mappings (e.g., expires_at, responded_by), but createdAt relies on TypeORM's default snake_case naming strategy. This is fine if you have a global naming strategy configured, but inconsistent with the explicit mappings used elsewhere in this entity.

Proposed fix

-  `@CreateDateColumn`()
+  `@CreateDateColumn`({ name: 'created_at' })
   createdAt!: Date;

---

20-21: Add DB-level enum constraint to status column for data integrity.

The column defaults to a plain varchar and could accept invalid values via raw SQL or migration errors. TypeORM 0.3.28 supports the enum type for PostgreSQL. Consider enforcing valid values at the database level:

Proposed fix

-  `@Column`({ default: 'pending' })
+  `@Column`({ type: 'enum', enum: ['pending', 'approved', 'denied', 'expired'], default: 'pending' })
   status!: 'pending' | 'approved' | 'denied' | 'expired';

apps/api/src/device-approval/device-approval.service.ts (1)

144-146: NotFoundException is semantically misleading for a non-pending request that does exist.

The request was found (line 136–138 handles not-found); it's just not in pending status. A BadRequestException with the same message would be more accurate.

     if (approval.status !== 'pending') {
-      throw new NotFoundException('Only pending requests can be cancelled');
+      throw new BadRequestException('Only pending requests can be cancelled');
     }

apps/web/src/api/models/deviceApprovalControllerCreateRequest201.ts (1)

9-11: Add required: ['requestId'] to the @ApiResponse schema in the backend controller.

The DeviceApprovalController.createRequest endpoint returns { requestId: string } (guaranteed non-null), but the @ApiResponse schema at lines 42–46 of device-approval.controller.ts omits the required array. This causes orval to generate an optional field (requestId?: string), forcing consumers like DeviceApprovalModal.tsx to use non-null assertions (requestId!).

Fix: Add required: ['requestId'] to the schema object in the @ApiResponse decorator so the generated type reflects the actual backend contract.

apps/web/src/routes/Settings.tsx (2)

59-82: Add keyboard navigation for the ARIA tabs pattern.

The role="tab" elements lack roving tabIndex and arrow-key handling. Per WAI-ARIA Tabs pattern, the active tab should have tabIndex={0}, inactive tabs tabIndex={-1}, and Left/Right arrow keys should move focus between tabs. Currently all tabs are individually focusable via Tab key, which works but isn't the recommended interaction model.

♿ Proposed keyboard navigation additions

             <button
               type="button"
               role="tab"
               id="tab-linked-methods"
               aria-selected={activeTab === 'linked-methods'}
               aria-controls="panel-linked-methods"
+              tabIndex={activeTab === 'linked-methods' ? 0 : -1}
               className={`settings-tab ${activeTab === 'linked-methods' ? 'active' : ''}`}
               onClick={() => setActiveTab('linked-methods')}
+              onKeyDown={(e) => {
+                if (e.key === 'ArrowRight') {
+                  setActiveTab('security');
+                  document.getElementById('tab-security')?.focus();
+                }
+              }}
             >
               LINKED METHODS
             </button>
             <button
               type="button"
               role="tab"
               id="tab-security"
               aria-selected={activeTab === 'security'}
               aria-controls="panel-security"
+              tabIndex={activeTab === 'security' ? 0 : -1}
               className={`settings-tab ${activeTab === 'security' ? 'active' : ''}`}
               onClick={() => setActiveTab('security')}
+              onKeyDown={(e) => {
+                if (e.key === 'ArrowLeft') {
+                  setActiveTab('linked-methods');
+                  document.getElementById('tab-linked-methods')?.focus();
+                }
+              }}
             >
               SECURITY
             </button>

As per coding guidelines: "When adding role and tabIndex={0} to an element, must also add keyboard interaction contract for that role."

---

85-101: Redundant hidden + conditional rendering — minor nit.

Using both hidden={activeTab !== '...'} and {activeTab === '...' && <Component />} is doubly-guarding. The conditional rendering already prevents the component from mounting, making hidden unnecessary. This is harmless but slightly noisy. Pick one approach — conditional rendering alone is sufficient and avoids rendering DOM nodes for inactive panels.

apps/web/src/components/mfa/DeviceWaitingScreen.tsx (1)

87-182: renderContent as a standalone function works but could be a component for cleaner React rendering.

This is purely a rendering helper with no hooks, so it works as a plain function. The approach is fine. One minor observation: the denied, expired, and error cases share identical action-button markup (retry + recovery fallback). If this grows, consider extracting those shared actions.

apps/web/src/components/mfa/RecoveryInput.tsx (1)

74-86: Textarea lacks an accessible label.

Screen readers won't associate the description paragraph with the textarea. Consider adding an aria-label or linking it via aria-labelledby / aria-describedby to the description.

Proposed fix

        <textarea
          className="recovery-input-textarea"
          value={phrase}
          onChange={(e) => setPhrase(e.target.value)}
          onKeyDown={handleKeyDown}
          placeholder="Enter your 24-word recovery phrase..."
          rows={4}
          disabled={isRecovering}
          autoFocus
          spellCheck={false}
          autoComplete="off"
          autoCapitalize="off"
+         aria-label="24-word recovery phrase"
        />

apps/web/src/components/mfa/AuthorizedDevices.tsx (2)

134-138: Prefer a CSS class over inline style={{ color: '#EF4444' }}.

The rest of the component consistently uses class-based styling. The authorized-devices-revoke-btn class could be extended with a .danger variant, similar to the recovery-section-confirm-btn.danger pattern already in App.css.

---

183-197: formatRelativeTime handles negative diff unexpectedly.

If timestampMs is in the future (e.g., due to clock skew between devices), diff will be negative, and the function falls through all guards to toLocaleDateString — which would show a future date. A simple guard at the top would be more robust.

Proposed fix

 function formatRelativeTime(timestampMs: number): string {
   const now = Date.now();
   const diff = now - timestampMs;
 
+  if (diff < 0) return 'just now';
   if (diff < 60_000) return 'just now';

apps/web/src/App.css (2)

1005-1012: Hardcoded #EF4444 used instead of var(--color-error).

Multiple selectors (.mfa-wizard-danger-box, .device-waiting-countdown.warning, and throughout the Device Approval Modal section) use the literal #EF4444 while the rest of the file consistently uses var(--color-error) for the same color. This makes theme changes brittle.

Example fix for .mfa-wizard-danger-box

 .mfa-wizard-danger-box {
   padding: var(--spacing-sm);
-  border: 1px solid `#EF4444`;
+  border: 1px solid var(--color-error);
   background-color: rgb(239 68 68 / 8%);
   font-size: var(--font-size-xs);
-  color: `#EF4444`;
+  color: var(--color-error);
   line-height: 1.6;
 }

Apply the same pattern to .device-waiting-countdown.warning, .device-approval-warning, .device-approval-btn-deny, and .device-approval-btn-deny:focus-visible.

Also applies to: 1571-1573

---

1788-1797: Hardcoded #00D084 used instead of var(--color-green-primary).

.device-approval-modal border and .device-approval-btn-approve use the literal #00D084. The rest of the file references var(--color-green-primary) for this color.

Proposed fix

 .device-approval-modal {
   ...
-  border: 1px solid `#00D084`;
+  border: 1px solid var(--color-green-primary);
   ...
 }

 .device-approval-btn-approve {
-  background-color: `#00D084`;
-  border: 1px solid `#00D084`;
+  background-color: var(--color-green-primary);
+  border: 1px solid var(--color-green-primary);
   ...
 }

Also applies to: 1889-1891

apps/api/src/device-approval/device-approval.controller.ts (1)

18-22: Extract RequestWithUser to a shared types file.

This interface is duplicated across 6 files (device-approval, vault, ipfs, ipns, auth controllers, and ipfs spec). Create apps/api/src/common/types.ts (or apps/api/src/auth/types.ts) and consolidate the definitions. Note that definitions vary slightly: most extend Request with { user: { id: string } }, but ipfs extends ExpressRequest and auth uses { user: User } — align these before consolidation.

apps/web/src/hooks/useAuth.ts (3)

304-316: Extract duplicated REQUIRED_SHARE handling into a shared helper.

The same pattern (set pending state → temp login → set token → early return) is copy-pasted across loginWithGoogle, loginWithEmail, and loginWithWallet. A single helper would eliminate the duplication and ensure the three paths stay in sync as the flow evolves.

Sketch: extracted helper

+  const handleRequiredShare = useCallback(
+    async (cipherboxJwt: string, userId: string, method: string, email?: string) => {
+      const tempLoginResponse = await authApi.login({
+        idToken: cipherboxJwt,
+        publicKey: `pending-core-kit-${userId}`,
+        loginType: 'corekit',
+      });
+      setAccessToken(tempLoginResponse.accessToken);
+      setPendingCipherboxJwt(cipherboxJwt);
+      setPendingAuthMethod(method);
+      if (email) setUserEmail(email);
+    },
+    [setAccessToken, setUserEmail]
+  );

Then each login method's required_share branch becomes a single call:

await handleRequiredShare(cipherboxJwt, userId, 'google', email);
return;

Also applies to: 345-356

---

440-483: Session restoration should skip when isRequiredShare is true.

If Core Kit restores from localStorage in a state where both coreKitLoggedIn and isRequiredShare are true (edge case during partial MFA), the effect would call initializeOrLoadVault(), which will fail because Core Kit can't export the TSS key yet. Adding an early-out guard is cheap insurance:

Proposed guard

-      if (coreKitLoggedIn && !isAuthenticated && !isLoggingIn && !restoringRef.current) {
+      if (coreKitLoggedIn && !isAuthenticated && !isLoggingIn && !isRequiredShare && !restoringRef.current) {

And add isRequiredShare to the dependency array at line 475.

---

253-258: The placeholder publicKey does not grant unauthorized access; refactor the magic prefix to a constant instead.

The temp login with pending-core-kit-${userId} does create a session with a placeholder publicKey, but the JWT token doesn't use publicKey for endpoint authorization—only the sub (userId) claim is validated by JwtStrategy. The temp token correctly grants access to authenticated vault operations, and there's no separate "scope" mechanism needed.

However, the pending-core-kit- prefix appears in 6 locations (3 in useAuth.ts, 3 in backend auth logic) and should be extracted to a shared constant for maintainability and grep-ability.

apps/web/src/hooks/useDeviceApproval.ts (4)

104-150: Decrypted factor key bytes (factorKeyBytes) not zeroed after use.

The ephemeral private key is properly zero-filled (line 136), but the decrypted factor key bytes at line 119 and the hex string at line 120 remain in memory. The factor key is sensitive key material (it grants access to the user's MFA shares). While JS doesn't guarantee memory clearing, zeroing the Uint8Array reduces the window for memory-scanning attacks, consistent with the ephemeral key handling already in place.

Proposed fix — zero-fill factorKeyBytes after inputFactorKey

       // 1. ECIES-decrypt the factor key
       const encrypted = hexToBytes(encryptedFactorKeyHex);
       const factorKeyBytes = await unwrapKey(encrypted, ephPrivKey);
       const factorKeyHex = bytesToHex(factorKeyBytes);

       // 2. Input the factor key to complete Core Kit login
       await inputFactorKey(factorKeyHex);
+
+      // Zero-fill decrypted factor key material
+      factorKeyBytes.fill(0);

---

155-186: Requester polling continues indefinitely on persistent server errors.

The catch block at line 176 silently swallows all errors, including 401 (expired temp token) or 403. For a 5-minute TTL window this is low-risk, but consider tracking consecutive failures and stopping polling or surfacing an error after N failures (e.g., 5) to avoid a bad UX where the spinner runs for 5 minutes with a dead backend connection.

---

254-271: isPollingPending guard may be stale due to React state batching.

pollPendingRequests captures isPollingPending via the useCallback dependency array. If called twice in rapid succession (e.g., component re-renders), both calls may read false before React batches the setIsPollingPending(true) update, creating two concurrent intervals. The approverPollRef.current check in the visibility effect provides a partial secondary guard, but pollPendingRequests itself doesn't check the ref.

Consider checking approverPollRef.current as the primary guard:

Proposed fix

  const pollPendingRequests = useCallback(() => {
-   if (isPollingPending) return;
+   if (approverPollRef.current) return;  // interval already running
    setIsPollingPending(true);

---

326-351: Approver poll function is duplicated between pollPendingRequests and the visibility effect.

The async poll closure at lines 258-265 and 333-340 are identical. Consider extracting a shared helper to reduce duplication.

packages/api-client/openapi.json (1)

1122-1125: Tag naming inconsistency: "device-approval" vs PascalCase convention.

All other tags use PascalCase naming ("Auth", "Vault", "IPFS", "Health", etc.), but the new tag is lowercase kebab-case "device-approval". This doesn't affect functionality but creates an inconsistency in the generated client's tag grouping and documentation.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @.planning/todos/pending/2026-02-15-security-review-medium-term-fixes.md:
- Line 15: Update the H-08 entry to remove the misplaced concern about the
Ed25519 algorithm and instead describe the plaintext storage vulnerability and
remediation: state that the device Ed25519 private key (H-08) is stored as
plaintext in IndexedDB and can be exfiltrated via XSS, and instruct implementers
to stop persisting raw key material by either (a) wrapping/encrypting the
private key with a session-derived symmetric key before writing to IndexedDB, or
(b) using non-extractable WebCrypto CryptoKey objects (makePrivate
non-extractable) and storing only handles/encoded references; reference H-08 and
any code paths that write/read the "device" key to/from IndexedDB so maintainers
can replace plaintext persistence with one of these secure approaches.

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/web/src/routes/Login.tsx (1)

133-155: ⚠️ Potential issue | 🟠 Major

loginError is not rendered in the isRequiredShare view — recovery failures are invisible.

When handleRecoveryComplete catches an error (Line 109), it sets loginError. But the error <div> at Lines 185-189 is only rendered in the normal login form branch, which is gated behind !isRequiredShare. When isRequiredShare is true (Lines 134-155), there is no loginError display, so the user sees no feedback after a recovery failure.

Proposed fix — render error in the MFA challenge view

  if (isRequiredShare) {
    return (
      <>
        <StagingBanner variant="login" />
        <div className="login-container">
          <MatrixBackground opacity={0.3} frameInterval={50} />
+         {loginError && (
+           <div className="login-error" role="alert" aria-live="polite">
+             {loginError}
+           </div>
+         )}
          {mfaView === 'waiting' ? (

🤖 Fix all issues with AI agents

In `@apps/api/src/auth/auth.service.ts`:
- Around line 384-389: Wrap the Redis deletion call that consumes the SIWE nonce
in a try/catch and surface a clear error: around the nonceKey /
this.redis.del(nonceKey) in auth.service.ts (where nonceKey =
`siwe:nonce:${parsed.nonce}`) catch connection or ioredis errors and rethrow a
controlled HTTP error (e.g., BadRequestException for missing/expired nonce, and
a more specific 503 or BadRequest with a message like "Redis unavailable while
consuming nonce" for Redis connection failures) so the call site doesn't bubble
raw ioredis exceptions; apply the exact same pattern to the walletLogin call in
identity.controller.ts that uses this.redis.del at the corresponding location.

In `@apps/api/src/device-approval/device-approval.service.ts`:
- Around line 103-138: The respond method currently reads and updates a
DeviceApproval row without any locking, allowing a TOCTOU race where concurrent
calls can both pass the pending check and overwrite each other; update respond
to perform the read-and-write inside a DB transaction and acquire a row-level
lock (or use optimistic locking via a `@VersionColumn` on the DeviceApproval
entity) so only one concurrent responder can succeed: open a transaction via
this.repo.manager.transaction, re-fetch the approval using the transaction
manager with a pessimistic write lock (or rely on manager.save with version
checks), then run the existing status/expiry/self-approval/encryptedFactorKey
logic and save via the transaction manager to ensure atomicity and prevent
concurrent approve/deny races.

In `@apps/api/src/device-approval/dto/respond-approval.dto.ts`:
- Around line 14-22: The encryptedFactorKey field (encryptedFactorKey) lacks an
upper size bound allowing arbitrarily large hex payloads; add a `@MaxLength`(...)
validator (e.g. 512) to the property and import it from class-validator, and
also update the ApiPropertyOptional metadata to include maxLength so OpenAPI
reflects the limit; modify the RespondApprovalDto property decorators to include
`@MaxLength`(512) and ApiPropertyOptional({ ..., maxLength: 512 }) to enforce and
document the expected ECIES ciphertext size.

In `@apps/web/src/components/mfa/MfaEnrollmentWizard.tsx`:
- Around line 139-149: The retry path can show "MFA already enabled" because
enableMfa() throws when store.isMfaEnabled is true even if the client-side
recovery phrase was never captured; update the logic so the retry button/handler
distinguishes a partial-enablement state and recovers the mnemonic instead of
re-calling enableMfa blindly: modify enableMfa() (and/or handleEnableMfa) to,
when store.isMfaEnabled is true, attempt to fetch or regenerate the recovery
phrase / TSS pub-key verification (or surface a specific "partial enablement —
retrieve recovery phrase" error) and pipe that flow back to the UI, referencing
enableMfa, handleEnableMfa, and store.isMfaEnabled to locate the change.

In `@apps/web/src/hooks/useDeviceApproval.ts`:
- Around line 143-148: The polling loop keeps running when handleApprovalSuccess
throws, so modify the poll error path to stop further polling: when an exception
is caught inside handleApprovalSuccess (the try/catch around setApprovalError /
setApprovalStatus), call the same stopPolling/clearInterval logic used to cancel
the requester (the function referenced as poll/stopPolling in this hook) and
ensure you do not re-throw without stopping the interval; also ensure
clearEphemeralKey() and setApprovalStatus('error') remain executed.
Additionally, update the dependency array of handleApprovalSuccess (the
useCallback that builds the success handler) to include any state or functions
it uses so the callback is stable and polling uses the latest version. This
change should also be applied to the other poll block in the 156-187 region.

🧹 Nitpick comments (15)

packages/api-client/openapi.json (2)

830-838: Inline response schemas are missing type: "object".

The inline schemas for the 201 response of POST /device-approval/request (Line 831), the 200 response of GET …/status (Line 874), and the array items of GET /pending (Line 918) all declare properties without setting "type": "object". Some code generators (openapi-generator, orval, etc.) may emit unknown or untyped output for these.

Example fix for the /request 201 response (apply the same pattern to the other two)

               "schema": {
+                "type": "object",
                 "properties": {
                   "requestId": {
                     "type": "string",
                     "format": "uuid"
                   }
-                }
+                },
+                "required": ["requestId"]
               }

Also consider extracting these into named $ref schemas under components/schemas (consistent with the rest of the spec) to improve reuse and generated-client ergonomics.

Also applies to: 873-886, 917-941

---

1122-1124: Tag naming is inconsistent with existing conventions.

All existing tags use PascalCase ("Auth", "Health", "Vault", "IPFS"), but the new tag is "device-approval" (kebab-case). This inconsistency propagates into generated client method groupings.

Consider renaming to "DeviceApproval" and updating the tags arrays in the five new endpoints accordingly.

apps/api/src/auth/auth.service.ts (1)

30-30: Redis client created outside NestJS DI — consider injecting a shared instance.

Instantiating Redis directly in the constructor bypasses NestJS's dependency injection. If other services (e.g., DeviceApprovalService, IdentityController) also need Redis, each will create its own connection. A shared RedisModule (or a custom provider) would centralize configuration, simplify testing (easy to mock), and let NestJS manage the connection lifecycle.

Not blocking since lazyConnect + onModuleDestroy works, but worth consolidating.

Also applies to: 43-50

apps/api/src/device-approval/dto/create-approval.dto.ts (1)

28-34: Optional: validate the 04 uncompressed-key prefix.

@Length(130, 130) + @IsHexadecimal() prevents most junk, but an uncompressed secp256k1 key must start with 04. A @Matches(/^04/) would reject compressed keys or random 130-char hex strings earlier, before they hit the ECIES layer.

Proposed addition

+import { Matches } from 'class-validator';
 ...
  `@IsHexadecimal`()
  `@Length`(130, 130, {
    message: 'ephemeralPublicKey must be 130 hex chars (uncompressed secp256k1)',
  })
+ `@Matches`(/^04/, { message: 'ephemeralPublicKey must start with 04 (uncompressed)' })
  ephemeralPublicKey!: string;

apps/api/src/device-approval/device-approval.entity.ts (1)

21-22: Optional: consider a DB-level enum for status.

The TypeScript union type provides compile-time safety, but the DB column has no constraint preventing invalid values from raw SQL, migrations, or other writers. A DB enum would add a defense-in-depth layer.

Example

- `@Column`({ default: 'pending' })
- status!: 'pending' | 'approved' | 'denied' | 'expired';
+ `@Column`({ type: 'enum', enum: ['pending', 'approved', 'denied', 'expired'], default: 'pending' })
+ status!: 'pending' | 'approved' | 'denied' | 'expired';

apps/web/src/hooks/useDeviceApproval.ts (1)

332-356: Duplicated poll function between pollPendingRequests and visibility effect.

The inline poll async function at Lines 338-344 duplicates the one in pollPendingRequests (Lines 259-265). Consider extracting a shared fetchPending helper to avoid drift.

Example extraction

+ const fetchPending = useCallback(async () => {
+   try {
+     const pending = await deviceApprovalApi.getPending();
+     setPendingRequests(pending);
+   } catch {
+     // Network error -- continue polling
+   }
+ }, []);

  const pollPendingRequests = useCallback(() => {
    if (isPollingPending) return;
    setIsPollingPending(true);
-   const poll = async () => { ... };
-   approverPollRef.current = setInterval(poll, 5000);
-   void poll();
+   approverPollRef.current = setInterval(fetchPending, 5000);
+   void fetchPending();
  }, [isPollingPending, fetchPending]);

And similarly use fetchPending in the visibility effect.

apps/web/src/components/mfa/MfaEnrollmentPrompt.tsx (1)

48-67: Minor: setVisible(true) could fire after unmount if the async hash completes late.

The getDismissalKeyAsync().then(...) at Line 59 runs asynchronously. If MfaEnrollmentPrompt unmounts before the promise resolves, setVisible(true) fires on an unmounted component. React 18 no longer warns about this, but it's a wasted state update. Since this component is mounted in AppShell (persistent), it's unlikely in practice.

If you want to be defensive, an abort flag or ref guard in the cleanup would prevent the stale update. As per coding guidelines, inside async functions that reference React refs, always re-check for null after each await.

Example guard

  useEffect(() => {
    if (!isAuthenticated || checkedRef.current) return;
    checkedRef.current = true;
+   let cancelled = false;

    const { isMfaEnabled: enabled } = checkMfaStatus();
    if (enabled) return;

    if (hasSeenPrompt) return;

    void getDismissalKeyAsync().then((key) => {
+     if (cancelled) return;
      try {
        if (localStorage.getItem(key) === 'true') return;
      } catch { }
      setVisible(true);
    });
+
+   return () => { cancelled = true; };
  }, [isAuthenticated, checkMfaStatus, hasSeenPrompt]);

apps/web/src/components/mfa/DeviceApprovalModal.tsx (4)

63-66: Good: stale error state is now cleared on request change.

The useEffect resetting respondError when currentRequest?.requestId changes addresses the prior feedback. However, isResponding is not reset here — if an in-flight approve/deny completes after the request changes (e.g., removed by polling), the next request could briefly inherit a stale isResponding = true state, leaving buttons disabled.

Proposed fix

  useEffect(() => {
    setRespondError(null);
+   setIsResponding(false);
  }, [currentRequest?.requestId]);

Based on learnings: "When a dialog/modal resets state on close, ensure cleanup of all loading/error states, resolved data, timers (clearTimeout, clearInterval), and animation frames (cancelAnimationFrame)"

---

68-92: Non-null assertions on requestId and ephemeralPublicKey may mask undefined values.

Lines 73 and 86 use currentRequest.requestId! and currentRequest.ephemeralPublicKey!. If the API or generated types make these fields optional and they happen to be undefined, the ! operator silently passes undefined to approveRequest/denyRequest, which could cause a silent 404 or malformed request rather than an early, debuggable error.

Consider adding a guard or using a type that guarantees these fields are present for pending requests.

---

171-195: CountdownValue interval keeps ticking after reaching zero.

The setInterval continues running every second even after remaining hits 0. For a modal that may stay visible (e.g., user doesn't dismiss), this is a minor resource waste.

Proposed fix — clear interval on expiry

  useEffect(() => {
    intervalRef.current = setInterval(() => {
-     setRemaining(Math.max(0, expiresAt - Date.now()));
+     const r = Math.max(0, expiresAt - Date.now());
+     setRemaining(r);
+     if (r <= 0 && intervalRef.current) {
+       clearInterval(intervalRef.current);
+       intervalRef.current = null;
+     }
    }, 1000);
    return () => {
      if (intervalRef.current) clearInterval(intervalRef.current);
    };
  }, [expiresAt]);

---

100-165: Modal backdrop has no Escape-key dismiss handler.

The backdrop has role="dialog" and aria-modal="true", but there's no keyboard handler for Escape to deny/dismiss. This is an accessibility gap for keyboard-only users — the only way to interact is via the buttons.

apps/api/src/device-approval/device-approval.service.spec.ts (1)

126-129: Double invocation pattern in error assertions is wasteful.

Calling the service method twice — once for exception type and once for message — executes the full code path redundantly. This pattern is repeated in multiple tests (lines 126–129, 288–293, 306–312, 345–351, 363–368, 390–393, 401–404).

A single assertion can check both:

-await expect(service.getStatus(testRequestId, testUserId)).rejects.toThrow(NotFoundException);
-await expect(service.getStatus(testRequestId, testUserId)).rejects.toThrow(
-  'Approval request not found'
-);
+await expect(service.getStatus(testRequestId, testUserId)).rejects.toThrow(
+  new NotFoundException('Approval request not found')
+);

apps/api/src/device-approval/device-approval.controller.ts (1)

19-23: RequestWithUser interface is duplicated across controllers.

This local interface likely exists in other controllers (e.g., auth). Consider extracting it to a shared types file.

#!/bin/bash
# Check if RequestWithUser is defined in multiple places
rg -n 'interface RequestWithUser' --type=ts

apps/api/src/device-approval/device-approval.service.ts (2)

152-155: NotFoundException is semantically misleading for a non-pending cancel.

The entity exists but isn't in the cancellable state. A BadRequestException (or 409 Conflict) would be more accurate and help callers distinguish "not found" from "wrong state".

Proposed fix

    if (approval.status !== 'pending') {
-     throw new NotFoundException('Only pending requests can be cancelled');
+     throw new BadRequestException('Only pending requests can be cancelled');
    }

---

22-34: No limit on concurrent pending requests per user (M-04).

createRequest doesn't check how many pending requests the user already has. An authenticated attacker could flood the table with pending requests (up to the throttle limit of 3/min, accumulating over time). Consider capping active pending requests per user (e.g., max 5).

Proposed approach

  async createRequest(userId: string, dto: CreateApprovalDto): Promise<{ requestId: string }> {
+   const pendingCount = await this.repo.count({
+     where: { userId, status: 'pending' as const, expiresAt: MoreThan(new Date()) },
+   });
+   if (pendingCount >= 5) {
+     throw new BadRequestException('Too many pending approval requests');
+   }
+
    const approval = this.repo.create({

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@apps/web/src/hooks/useDeviceApproval.ts`:
- Around line 104-150: The guard throws at the top of handleApprovalSuccess
(checking ephemeralPrivKey via ephemeralPrivKeyRef.current and coreKit) occur
before the try/catch and can become unhandled rejections when invoked from
poll(); move those checks into the start of the existing try block so any thrown
errors are captured by the catch, keeping the rest of the logic (unwrapKey,
inputFactorKey, coreKit.createFactor, coreKit.setDeviceFactor,
coreKit.commitChanges, clearEphemeralKey, completeRequiredShare) unchanged and
ensuring clearEphemeralKey() and setApprovalError(...) still run in the catch.

🧹 Nitpick comments (2)

apps/web/src/hooks/useDeviceApproval.ts (2)

335-359: Duplicated poll closure between pollPendingRequests and the visibility effect.

The anonymous poll function at lines 341–348 is identical to lines 262–268. Extract a stable callback (e.g., fetchPending) and reuse it in both places to avoid divergence during future edits.

Sketch

+ const fetchPending = useCallback(async () => {
+   try {
+     const pending = await deviceApprovalApi.getPending();
+     setPendingRequests(pending);
+   } catch {
+     // Network error -- continue polling
+   }
+ }, []);
+
  const pollPendingRequests = useCallback(() => {
    if (isPollingPending) return;
    setIsPollingPending(true);
-   const poll = async () => { ... };
-   approverPollRef.current = setInterval(poll, 5000);
-   void poll();
+   approverPollRef.current = setInterval(fetchPending, 5000);
+   void fetchPending();
- }, [isPollingPending]);
+ }, [isPollingPending, fetchPending]);

And similarly replace the inline poll in the visibility useEffect.

---

258-275: Same setInterval + async overlap applies to approver polling.

Less impactful here since setPendingRequests is idempotent with fresh server data, but for consistency with the requester fix, consider using setTimeout chaining here too.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@apps/api/src/auth/auth.service.ts`:
- Around line 52-54: The onModuleDestroy method should defensively handle
failures from this.redis.quit(): wrap the await this.redis.quit() call in a
try/catch inside AuthService::onModuleDestroy, and in the catch either check and
ignore expected errors (e.g., when the client was never connected or already
closed) or log a debug/warn message via the service logger instead of letting
the exception bubble; ensure the code still awaits successful quit when possible
but does not throw if quit() fails.

In `@apps/web/src/routes/Login.tsx`:
- Around line 210-218: The three anchor tags in Login.tsx that render the footer
links (the <a href="#" className="footer-link"> elements with text "[help]",
"[privacy]", "[terms]") use href="#" which causes a scroll-to-top and is bad for
accessibility; replace these placeholders with either semantic <button> elements
styled with className="footer-link" or keep anchors but stop the navigation by
using href="#!" or handling onClick to call event.preventDefault(), and when
real URLs are ready swap them in so the links point to the actual pages.

🧹 Nitpick comments (5)

apps/api/src/device-approval/device-approval.service.ts (3)

124-127: Self-response check blocks deny with a misleading error message.

The guard at Line 125 fires for both approve and deny actions, but the message says "A device cannot approve its own request." If self-denial should also be blocked (reasonable), consider updating the message. If only self-approval should be blocked, move this check inside the dto.action === 'approve' branch.

Suggested message fix (if self-deny should also be blocked)

-        throw new BadRequestException('A device cannot approve its own request');
+        throw new BadRequestException('A device cannot respond to its own request');

---

156-158: NotFoundException is semantically incorrect for a state precondition failure.

The request exists but isn't in pending state. A BadRequestException (or ConflictException) would be more accurate — NotFoundException implies the resource doesn't exist, which could confuse API consumers debugging cancellation failures.

Suggested fix

     if (approval.status !== 'pending') {
-      throw new NotFoundException('Only pending requests can be cancelled');
+      throw new BadRequestException('Only pending requests can be cancelled');
     }

---

22-34: No duplicate-request guard — a device can create unbounded pending requests.

createRequest doesn't check for an existing pending request from the same deviceId/userId. A misbehaving client could flood the table with pending rows (each living for 5 minutes). Consider either upsert semantics or a uniqueness check to limit one active request per device.

apps/web/src/routes/Login.tsx (1)

95-101: Clear loginError when switching between MFA views.

If handleRecoveryComplete fails and sets loginError, then the user switches back to the waiting screen via handleBackToWaiting, the stale error message persists below the DeviceWaitingScreen. Similarly, switching to recovery keeps any prior error visible.

Proposed fix

  const handleRecoveryFallback = useCallback(() => {
+   setLoginError(null);
    setMfaView('recovery');
  }, []);

  const handleBackToWaiting = useCallback(() => {
+   setLoginError(null);
    setMfaView('waiting');
  }, []);

apps/api/src/auth/auth.service.ts (1)

30-50: Consider injecting Redis via NestJS DI instead of creating instances directly.

The codebase creates independent Redis instances in at least three places (AuthService, IdentityController, EmailOtpService), each duplicating the same configuration logic. A shared NestJS provider (similar to the existing BullModule pattern in app.module.ts) would centralize configuration, eliminate duplication, and simplify testing by allowing module-level mocking instead of relying on jest.mock.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

onModuleDestroy should handle a failed quit() gracefully.

If Redis never connected (due to lazyConnect: true) or the connection is already broken, this.redis.quit() can throw. Wrap it defensively.

Proposed fix

   async onModuleDestroy(): Promise<void> {
-    await this.redis.quit();
+    try {
+      await this.redis.quit();
+    } catch {
+      // Redis may never have connected (lazyConnect) or already be disconnected
+    }
   }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	async onModuleDestroy(): Promise<void> {
	await this.redis.quit();
	}
	async onModuleDestroy(): Promise<void> {
	try {
	await this.redis.quit();
	} catch {
	// Redis may never have connected (lazyConnect) or already be disconnected
	}
	}

🤖 Prompt for AI Agents

In `@apps/api/src/auth/auth.service.ts` around lines 52 - 54, The onModuleDestroy
method should defensively handle failures from this.redis.quit(): wrap the await
this.redis.quit() call in a try/catch inside AuthService::onModuleDestroy, and
in the catch either check and ignore expected errors (e.g., when the client was
never connected or already closed) or log a debug/warn message via the service
logger instead of letting the exception bubble; ensure the code still awaits
successful quit when possible but does not throw if quit() fails.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Placeholder href="#" links cause scroll-to-top and are inaccessible.

The help, privacy, and terms links use href="#" which scrolls the page to the top on click and doesn't convey intent to assistive technology. If these are placeholders, consider using <button> elements styled as links, or at minimum href="#!" / event.preventDefault() to avoid the scroll jump. When real URLs are available, swap them in.

🤖 Prompt for AI Agents

In `@apps/web/src/routes/Login.tsx` around lines 210 - 218, The three anchor tags
in Login.tsx that render the footer links (the <a href="#"
className="footer-link"> elements with text "[help]", "[privacy]", "[terms]")
use href="#" which causes a scroll-to-top and is bad for accessibility; replace
these placeholders with either semantic <button> elements styled with
className="footer-link" or keep anchors but stop the navigation by using
href="#!" or handling onClick to call event.preventDefault(), and when real URLs
are ready swap them in so the links point to the actual pages.