fix(web): MFA auth flow + Security tab display bugs#210
Merged
Conversation
…pproval 401 Three interrelated MFA bugs fixed: 1. Device factor not auto-detected on re-login: Web3Auth SDK's handleExistingUser() does not check localStorage for device factor after hashedShare deletion (post-MFA). Added explicit getDeviceFactor() + inputFactorKey() in doLoginWithCoreKit REQUIRED_SHARE branch. 2. Recovery redirects to login: inputFactorKey() called syncStatus() prematurely, transitioning React context to LOGGED_IN before backend auth completed. Session restoration effect fired, refresh failed, coreKitLogout() undid recovery. Fix: defer syncStatus() to completeRequiredShare() after backend auth succeeds. 3. Device approval 401: DeviceWaitingScreen fired requestApproval() on mount before temp access token was stored (race between React flush and async continuation). Fix: wait for accessToken in auth store before firing request. Debug session: .planning/debug/resolved/mfa-auth-flow-broken.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0d55c2e26968
…lastActive - Fix getFactors() to normalize factor types via tssShareIndex when module is "Other" (enableMFA default): index 2→DeviceShare, 3→SeedPhrase - Fix getFactors() to extract additionalMetadata from flat JSON structure (Web3Auth spreads it at top level, not nested) - Add deviceId/browserName metadata to recoverWithMnemonic() createFactor - Pass shareDescription to enableMFA() for correct recovery factor tagging - Broaden AuthorizedDevices registry map to include pending devices (not just authorized), so recovery-created devices show lastSeenAt - Fall back to "just now" for current device when registry hasn't loaded Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4ffcfbbacd4a
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughSeven interrelated MFA bugs were diagnosed and fixed: device-factor auto-detection during REQUIRED_SHARE login, Core Kit ↔ React state sync, token-gated approval request initiation, device registry inclusion of non-revoked devices and lastSeen rendering, factor description/type normalization and metadata extraction, device registry handling (authorized vs pending), and related TypeScript/ESLint cleanups. Changes
Sequence DiagramssequenceDiagram
participant User
participant LoginFlow as Web3Auth Login
participant LocalStorage as Local Storage
participant CoreKit as Core Kit SDK
participant Backend as Backend Auth
participant ReactState as React State
User->>LoginFlow: Initiate login
LoginFlow->>CoreKit: Check login status
CoreKit-->>LoginFlow: REQUIRED_SHARE
LoginFlow->>LocalStorage: getDeviceFactor()
alt device factor exists
LocalStorage-->>LoginFlow: deviceFactorHex
LoginFlow->>CoreKit: inputFactorKey(deviceFactor)
CoreKit->>Backend: Validate factor
Backend-->>CoreKit: Validation result
alt valid
CoreKit-->>LoginFlow: LOGGED_IN
LoginFlow->>CoreKit: commit()
CoreKit-->>LoginFlow: committed
LoginFlow->>ReactState: syncStatus()
ReactState-->>LoginFlow: updated
LoginFlow-->>User: success
else invalid
CoreKit-->>LoginFlow: error
LoginFlow-->>LoginFlow: remain REQUIRED_SHARE
end
else no device factor
LocalStorage-->>LoginFlow: null
LoginFlow-->>LoginFlow: true REQUIRED_SHARE
end
sequenceDiagram
participant Component as Device Waiting Screen
participant Token as Auth Store (accessToken)
participant Effect as Effect Hook
participant ApprovalAPI as Approval API
participant UI as Countdown Timer UI
Component->>Effect: mount
Effect->>Token: read accessToken
alt token not available
Token-->>Effect: null
Effect->>Effect: wait for token
else token available
Token-->>Effect: accessToken
Effect->>Effect: check requestFiredRef
alt not fired
Effect->>Effect: set requestFiredRef = true
Effect->>ApprovalAPI: fire approval request
ApprovalAPI-->>Effect: request started
Effect->>UI: start countdown
UI-->>Component: render timer
else already fired
Effect->>Effect: skip
end
end
Component->>Component: unmount
Component->>UI: clear countdown interval
Component->>ApprovalAPI: cancelRequest()
ApprovalAPI-->>Component: cancelled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/components/mfa/DeviceWaitingScreen.tsx (1)
69-73:⚠️ Potential issue | 🟡 MinorReset
requestFiredRefinhandleRetryto allow auto-request re-triggering on retryWhen the user clicks retry,
handleRetry()should resetrequestFiredRef.current = falsealong with the other state. Currently, if therequestApprovalfunction reference changes after a retry (e.g., due to parent re-renders), the first useEffect won't re-fire the auto-request becauserequestFiredRef.currentis stilltrue. This violates the expected behavior of the retry action: allowing a fresh approval flow.const handleRetry = useCallback(() => { requestFiredRef.current = false; startTimeRef.current = Date.now(); setCountdown(APPROVAL_TTL_MS); requestApproval(); }, [requestApproval]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/mfa/DeviceWaitingScreen.tsx` around lines 69 - 73, handleRetry currently resets startTimeRef and countdown and calls requestApproval but does not reset requestFiredRef, so the auto-request useEffect may not re-trigger after a retry; update handleRetry to set requestFiredRef.current = false before resetting startTimeRef.current and calling requestApproval (ensure you reference handleRetry, requestFiredRef, startTimeRef, setCountdown, requestApproval, and APPROVAL_TTL_MS) so a retry allows a fresh approval flow to be auto-fired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/components/mfa/DeviceWaitingScreen.tsx`:
- Around line 69-73: handleRetry currently resets startTimeRef and countdown and
calls requestApproval but does not reset requestFiredRef, so the auto-request
useEffect may not re-trigger after a retry; update handleRetry to set
requestFiredRef.current = false before resetting startTimeRef.current and
calling requestApproval (ensure you reference handleRetry, requestFiredRef,
startTimeRef, setCountdown, requestApproval, and APPROVAL_TTL_MS) so a retry
allows a fresh approval flow to be auto-fired.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.planning/debug/resolved/mfa-auth-flow-broken.mdapps/web/src/components/mfa/AuthorizedDevices.tsxapps/web/src/components/mfa/DeviceWaitingScreen.tsxapps/web/src/hooks/useAuth.tsapps/web/src/hooks/useMfa.tsapps/web/src/lib/web3auth/hooks.ts
Without this reset, if requestApproval's reference changes after retry (e.g. parent re-render), the auto-fire useEffect won't re-trigger because requestFiredRef.current stays true from the initial request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 442c2624f0ae
dorny/paths-filter@v3 was leaking the src filter's ** glob pattern into the desktop filter evaluation, causing desktop builds (Windows + macOS cargo check + Tauri build) to run on every PR regardless of whether desktop files changed. Replace ** with explicit directory patterns (apps/, packages/, tee-worker/, tests/, .github/) to isolate the filters correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8b7f503d9e83
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/mfa/DeviceWaitingScreen.tsx`:
- Around line 26-31: The effect that creates the polling interval can leak if it
sets a new interval while an old one is still active; before creating a new
interval in DeviceWaitingScreen (the logic that uses countdownRef,
requestFiredRef and requestApproval), defensively clear any existing interval by
checking countdownRef.current and calling clearInterval on it, then assign the
new interval id to countdownRef.current; also ensure you clear
countdownRef.current in the effect cleanup and when handleRetry resets
requestFiredRef so you don't leave a stale interval running.
Defensive fix: if handleRetry resets requestFiredRef and the effect re-fires (e.g. requestApproval reference changes), a new setInterval was created without clearing the previous one. Clear any existing interval at the start of the effect to prevent leaks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 428a2491751c
FSM1
added a commit
that referenced
this pull request
Feb 27, 2026
Verify and close two active debug sessions after confirming all issues are fixed on main with commit/PR cross-references. corekit-auth-uat: 4 issues resolved (ISSUE-001 tab crash, ISSUE-003 JWKS persistence, ISSUE-004 SecurityTab wiring, ISSUE-002 documented). Added post-session follow-up fixes (#205, #210, #213) and E2E coverage mapping for 19 previously-skipped UAT test cases. mfa-banner-footer-overlap: grid-area fix (#143) and z-index follow-up verified on main with file/line references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b1ae6e4f2098
2 tasks
FSM1
added a commit
that referenced
this pull request
Feb 27, 2026
Verify and close two active debug sessions after confirming all issues are fixed on main with commit/PR cross-references. corekit-auth-uat: 4 issues resolved (ISSUE-001 tab crash, ISSUE-003 JWKS persistence, ISSUE-004 SecurityTab wiring, ISSUE-002 documented). Added post-session follow-up fixes (#205, #210, #213) and E2E coverage mapping for 19 previously-skipped UAT test cases. mfa-banner-footer-overlap: grid-area fix (#143) and z-index follow-up verified on main with file/line references. Entire-Checkpoint: b1ae6e4f2098 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
6 tasks
5 tasks
Closed
This was referenced Mar 24, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tssShareIndexnormalization, flat metadata extraction from Web3Auth JSON, device metadata in recovery flow,lastActiveregistry map broadened to include pending devicesenableMFAdefaults,addFactorDescriptionspread behavior,handleExistingUsernot checking localStorage)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation