fix(api,web): MFA REQUIRED_SHARE auth flow + E2E test coverage#213
Merged
Conversation
Moved to done/, beginning implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0310757ed1e8
Three fixes for the MFA device approval flow:
1. LoginDto regex now accepts `pending-core-kit-*` placeholder publicKey
for REQUIRED_SHARE temp auth (previously rejected with 400).
2. useDeviceApproval generates uncompressed secp256k1 ephemeral public
key (65 bytes = 130 hex chars) instead of compressed (33 bytes).
3. Login.tsx redirect guards against isRequiredShare to prevent unmounting
DeviceWaitingScreen when temp access token sets isAuthenticated.
4. SettingsPage skips auth redirect during MFA enrollment to prevent
logout from token expiry during long Devnet enableMFA() calls.
5. useAuth removes navigate('/files') from login functions — Login.tsx
redirect effect handles navigation centrally.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 26ff72f8d979
Add E2E-targeting attributes to all MFA UI components: - MfaEnrollmentWizard: wizard steps, buttons, phrase grid, checkbox - DeviceWaitingScreen: root, countdown, recovery link, status - DeviceApprovalModal: modal, approve/deny buttons - RecoveryInput: root, textarea, submit/back buttons - RecoveryPhraseGrid: root container - SecurityTab: root, status badge, enable button Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e20d33bb4994
Serial test suite (TC-MFA-01 through TC-MFA-05) covering: - Wallet login and MFA enrollment with recovery phrase capture - MFA status reflected in Security tab - Device approval (approve) across two browser contexts - Recovery phrase restore on new device - Device approval (deny) with status verification Uses @johanneskares/wallet-mock for real Core Kit initialization on Sapphire Devnet. Tagged @mfa for optional CI filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 21cba03804d9
When MFA is enabled and a new device sends a placeholder publicKey
(pending-core-kit-{userId}), the auth service was only searching for
users whose publicKey matched the placeholder pattern. After the first
login, users have a real Core Kit publicKey, so the placeholder search
failed and created a NEW user — causing device approval requests to
go to the wrong userId.
Fix: when publicKey is a placeholder, also look up by authMethod
identifierHash (the hashed verifierId from the JWT). This finds the
existing user regardless of their current publicKey.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 6a373586dd93
|
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)
WalkthroughHandle placeholder publicKey ("pending-core-kit-{userId}") and REQUIRED_SHARE temp-auth flows: resolve/reuse placeholder identities, emit scoped temp access tokens (no refresh), enforce route-level scoped JWTs, add AllowScope/JwtAuthGuard, add Playwright E2E MFA tests/helpers, add MFA UI test hooks, and centralize pending-auth state and redirect gating. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as API Service
participant DB as Database
Client->>API: POST /login (publicKey: 04... or pending-core-kit-...)
alt publicKey is placeholder
API->>DB: Query user by publicKey = placeholder
DB-->>API: user?
alt user found
API->>API: validate JWT, extract payload.sub
API->>DB: Query user by identifierHash (payload.sub)
DB-->>API: user?
alt incoming publicKey is real
API->>DB: Update user's publicKey to real key
else
API->>API: keep placeholder (temp auth)
end
else
API->>DB: Lookup by identifierHash or create temp-auth user
end
else publicKey is real
API->>DB: Find or create user by real publicKey
end
API-->>Client: return auth response (tokens | scoped temp auth)
sequenceDiagram
actor SecondaryDevice
actor PrimaryDevice
participant API as API Service
participant DB as Database
SecondaryDevice->>API: Login with placeholder -> receive temp token + approval id
API->>DB: create device-approval record
DB-->>API: approval id
API-->>SecondaryDevice: temp auth response
SecondaryDevice->>SecondaryDevice: show DeviceWaitingScreen (polling)
PrimaryDevice->>API: Poll pending device-approvals
API-->>PrimaryDevice: list (includes approval id)
PrimaryDevice->>API: POST approve/deny approval id
API->>DB: update approval status
DB-->>API: updated status
API-->>SecondaryDevice: poll returns approved/denied
alt approved
SecondaryDevice->>API: complete auth (send real publicKey)
API->>DB: upgrade temp auth -> full session
SecondaryDevice-->>SecondaryDevice: navigate to /files
else denied
SecondaryDevice-->>SecondaryDevice: show denial state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Three new test cases for the placeholder publicKey resolution: - Placeholder login finding placeholder user does NOT overwrite publicKey - REQUIRED_SHARE temp auth finds existing user by identity (authMethod identifierHash) when user has real Core Kit publicKey - REQUIRED_SHARE creates new user when no existing identity found Raises auth.service.ts branch coverage from 82.35% to 85.29% (threshold: 84%). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d5c212a611af
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/e2e/utils/mfa-helpers.ts (1)
129-135: Prefer data-testid over CSS class for locator stability.Using
.recovery-phrase-wordCSS class is less stable than data-testid—styling refactors could silently break this helper. All other locators in this file use data-testid for consistency.♻️ Suggested change
- const wordSpans = grid.locator('.recovery-phrase-word'); + const wordSpans = grid.locator('[data-testid="recovery-phrase-word"]');This requires adding
data-testid="recovery-phrase-word"to the word spans inRecoveryPhraseGridcomponent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/mfa-helpers.ts` around lines 129 - 135, Replace the brittle CSS class selector in the helper with a stable data-testid: update the locator invocation (wordSpans = grid.locator(...)) to use '[data-testid="recovery-phrase-word"]' instead of '.recovery-phrase-word', and also add data-testid="recovery-phrase-word" to the RecoveryPhraseGrid component's word span elements so the test selector exists; keep the rest of the loop that reads textContent from wordSpans and pushes trimmed text unchanged.
🤖 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/api/src/auth/auth.service.ts`:
- Around line 73-123: Add unit tests for the new placeholder resolution branches
in the auth service: cover (1) placeholder→real key upgrade where
userRepository.findOne returns a placeholderUser (publicKey starts with
'pending-core-kit-') and loginDto.publicKey is a real key so
placeholderUser.publicKey is updated and saved via userRepository.save; (2)
REQUIRED_SHARE identity fallback hit where loginDto.publicKey starts with
'pending-core-kit-' and siweService.hashIdentifier(identifier) leads
authMethodRepository.findOne to return an authMethod with a user (assert
returned user used); (3) REQUIRED_SHARE miss where no authMethod is found
(assert behavior falls back to new-user path/no user); and (4) new-user fallback
when no verifierId/identifier matches. Use mocks for userRepository,
authMethodRepository, siweService.hashIdentifier, and assert the correct
repository calls (findOne/save) and logger messages for placeholderUser and
REQUIRED_SHARE branches.
- Around line 94-96: The query that finds placeholderUser uses
Like(`pending-core-kit-${verifierId}%`) which can match multiple publicKey
values sharing a prefix; update the userRepository.findOne call in
auth.service.ts to match the exact placeholder string instead of a pattern
(construct the exact publicKey value "pending-core-kit-${verifierId}" and use
that for equality in the where clause or the ORM's Equal comparator) so findOne
only resolves the intended placeholder user by exact publicKey match.
- Around line 90-121: The current REQUIRED_SHARE fallback builds identifier as
payload.email || payload.sub which causes provider-sub-based auth methods to be
missed when email is present; update the logic around identifier,
siweService.hashIdentifier, and the authMethodRepository.findOne call in the
login flow so you search for auth methods using both the email-derived hash and
the provider-sub-derived hash (when payload.sub exists) instead of only the
email hash: compute hashes for payload.email (normalized) and payload.sub
separately via siweService.hashIdentifier and query authMethodRepository.findOne
for either identifierHash (email) or subHash (provider sub), preferring a match
by provider-sub when present, then set user = authMethod.user if found.
In `@tests/e2e/utils/mfa-helpers.ts`:
- Around line 80-91: Replace the Promise.race pattern (the parallel
gridLocator.waitFor(...) and errorLocator.waitFor(...)) because the losing
waitFor can later reject; instead create a combined locator using Playwright's
locator.or() (e.g., combined = gridLocator.or(errorLocator)), call
combined.waitFor({ state: 'visible', timeout: enableTimeout }) to wait for
either to appear, then determine which one matched by checking
gridLocator.isVisible() (or errorLocator.isVisible()) and handle the error path
by reading errorLocator.textContent() and throwing as before; remove the two
separate waitFor calls and the Promise.race usage.
---
Nitpick comments:
In `@tests/e2e/utils/mfa-helpers.ts`:
- Around line 129-135: Replace the brittle CSS class selector in the helper with
a stable data-testid: update the locator invocation (wordSpans =
grid.locator(...)) to use '[data-testid="recovery-phrase-word"]' instead of
'.recovery-phrase-word', and also add data-testid="recovery-phrase-word" to the
RecoveryPhraseGrid component's word span elements so the test selector exists;
keep the rest of the loop that reads textContent from wordSpans and pushes
trimmed text unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.planning/STATE.md.planning/todos/done/2026-02-27-e2e-mfa-flows-wallet-login.mdapps/api/src/auth/auth.service.tsapps/api/src/auth/dto/login.dto.tsapps/web/src/api/models/loginDto.tsapps/web/src/components/mfa/DeviceApprovalModal.tsxapps/web/src/components/mfa/DeviceWaitingScreen.tsxapps/web/src/components/mfa/MfaEnrollmentWizard.tsxapps/web/src/components/mfa/RecoveryInput.tsxapps/web/src/components/mfa/RecoveryPhraseGrid.tsxapps/web/src/components/mfa/SecurityTab.tsxapps/web/src/hooks/useAuth.tsapps/web/src/hooks/useDeviceApproval.tsapps/web/src/routes/Login.tsxapps/web/src/routes/SettingsPage.tsxpackages/api-client/openapi.jsontests/e2e/tests/mfa-flows.spec.tstests/e2e/utils/mfa-helpers.tstests/e2e/utils/wallet-login-helpers.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/auth/auth.service.spec.ts (1)
580-583: Consider removing the unused third mock.The comment on line 583 indicates the userId fallback lookup is "not reached" in this test scenario. The third
.mockResolvedValueOnce(mockAuthMethod)appears unnecessary and could be removed to clarify the expected call sequence.♻️ Suggested simplification
authMethodRepository.findOne .mockResolvedValueOnce(mockAuthMethod) // identity lookup with relations - .mockResolvedValueOnce(mockAuthMethod) // userId + identifierHash lookup - .mockResolvedValueOnce(mockAuthMethod); // userId fallback (not reached) + .mockResolvedValueOnce(mockAuthMethod); // userId + identifierHash lookup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.service.spec.ts` around lines 580 - 583, The third .mockResolvedValueOnce(mockAuthMethod) on authMethodRepository.findOne is unused (comment says "not reached") and should be removed to simplify the test and clarify expected call sequence; update the test to only set mockResolvedValueOnce twice for authMethodRepository.findOne (for the identity lookup and the userId+identifierHash lookup) and remove the redundant mock that references mockAuthMethod.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/auth/auth.service.spec.ts`:
- Around line 580-583: The third .mockResolvedValueOnce(mockAuthMethod) on
authMethodRepository.findOne is unused (comment says "not reached") and should
be removed to simplify the test and clarify expected call sequence; update the
test to only set mockResolvedValueOnce twice for authMethodRepository.findOne
(for the identity lookup and the userId+identifierHash lookup) and remove the
redundant mock that references mockAuthMethod.
…SHARE Address CodeRabbit review feedback: - Replace Like wildcard with exact equality for placeholder publicKey matching - Replace identifier-hash lookup with direct userId lookup (payload.sub) for REQUIRED_SHARE temp auth — works for all auth types (email, Google, wallet) - Replace Promise.race with Playwright locator.or() in E2E helpers - Remove unused Like import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 5ce214f0d0b1
Use the staging signing key so Core Kit loginWithJWT validates against the matching JWKS endpoint during MFA E2E tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b770b3385c78
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/utils/mfa-helpers.ts (1)
122-139: Consider usingdata-testidfor recovery phrase word elements.Line 126 uses a class selector (
.recovery-phrase-word) while all other locators in this file consistently usedata-testidattributes. Class selectors can break if styling is refactored, whereasdata-testidis explicitly for testing and more resilient.♻️ Suggested refactor
- const wordSpans = grid.locator('.recovery-phrase-word'); + const wordSpans = grid.locator('[data-testid="recovery-phrase-word"]');This would require adding
data-testid="recovery-phrase-word"to each word span in theRecoveryPhraseGridcomponent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/mfa-helpers.ts` around lines 122 - 139, In extractRecoveryPhrase, replace the fragile class selector '.recovery-phrase-word' with a data-testid selector (e.g. '[data-testid="recovery-phrase-word"]') and update the RecoveryPhraseGrid component to add data-testid="recovery-phrase-word" to each word span; then use grid.locator('[data-testid="recovery-phrase-word"]') in the extractRecoveryPhrase function to collect and trim the 24 words as before.apps/api/src/auth/auth.service.ts (1)
86-87: Comment drift in REQUIRED_SHARE explanation.Line 86-87 still describes auth-method identifier-hash lookup, but the implemented branch uses direct
user.id = payload.sublookup. Please align the comment with actual behavior to avoid future regressions.Based on learnings, in this flow
payload.subfrom the CipherBox-issued JWT is the userId.Also applies to: 108-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.service.ts` around lines 86 - 87, Update the misleading comment around REQUIRED_SHARE to reflect that this branch looks up the user by the CipherBox JWT subject (payload.sub) which is the userId, not by an auth-method identifier hash; edit the comments near the REQUIRED_SHARE handling in auth.service.ts (and the similar comment at the later block around lines 108-110) so they reference payload.sub as the source of user.id and describe the direct user.id = payload.sub lookup performed by the code (refer to the function/method handling the CipherBox JWT user resolution in this file).
🤖 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/api/src/auth/auth.service.ts`:
- Around line 111-120: The REQUIRED_SHARE "temp auth" path mints full sessions
because when loginDto.publicKey.startsWith('pending-core-kit-') you fall through
to the normal token issuance (createTokens(user.id, user.publicKey)); change
token issuance for that branch so tokens are scoped/short‑lived: either call a
new createTempTokens (or pass an options object to createTokens) that embeds a
temp marker (e.g., authLevel: "temp" or scope: "temp") and uses a much shorter
refresh/access TTL, and then update JwtAuthGuard/token validation to reject
tokens without the expected authLevel/scope for protected routes (or add
route-level scope guards on device-approval endpoints). Locate the logic around
the REQUIRED_SHARE check (loginDto.publicKey.startsWith('pending-core-kit-')),
the createTokens call, and JwtAuthGuard to implement these changes.
---
Nitpick comments:
In `@apps/api/src/auth/auth.service.ts`:
- Around line 86-87: Update the misleading comment around REQUIRED_SHARE to
reflect that this branch looks up the user by the CipherBox JWT subject
(payload.sub) which is the userId, not by an auth-method identifier hash; edit
the comments near the REQUIRED_SHARE handling in auth.service.ts (and the
similar comment at the later block around lines 108-110) so they reference
payload.sub as the source of user.id and describe the direct user.id =
payload.sub lookup performed by the code (refer to the function/method handling
the CipherBox JWT user resolution in this file).
In `@tests/e2e/utils/mfa-helpers.ts`:
- Around line 122-139: In extractRecoveryPhrase, replace the fragile class
selector '.recovery-phrase-word' with a data-testid selector (e.g.
'[data-testid="recovery-phrase-word"]') and update the RecoveryPhraseGrid
component to add data-testid="recovery-phrase-word" to each word span; then use
grid.locator('[data-testid="recovery-phrase-word"]') in the
extractRecoveryPhrase function to collect and trim the 24 words as before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/e2e.ymlapps/api/src/auth/auth.service.spec.tsapps/api/src/auth/auth.service.tstests/e2e/utils/mfa-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/auth/auth.service.spec.ts
The secret is scoped to the 'staging' GitHub environment. Without declaring the environment, the E2E job gets an empty value and the API falls back to an ephemeral keypair — causing Core Kit JWT verification to fail with "crypto/rsa: verification error". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f8295f801413
The staging environment has protection rules requiring approval, which blocks the E2E job from starting. Moved the IDENTITY_JWT_PRIVATE_KEY to a repo-level secret instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 85529942788d
The mock wallet from @johanneskares/wallet-mock doesn't handle eth_chainId — unhandled methods fall through to the configured viem transport. Using http() (default mainnet RPC) caused eth_chainId to make a real HTTP request that hung in CI. Replace with a custom transport that returns mock data for all fallthrough RPC methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8e28ad0a0afc
After enableMfa() completes, the store is already set to isMfaEnabled=true. Calling checkMfaStatus() in handleEnrollmentComplete re-reads from Core Kit's getKeyDetails() which can return stale totalFactors on Sapphire Devnet before metadata propagation completes, overwriting the store back to false and showing [DISABLED]. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 3a2e842b2ada
The REQUIRED_SHARE login path was issuing full unrestricted session
tokens (15m access + 7d refresh) for temporary auth. Scoped tokens
now restrict access to device-approval endpoints only and skip
refresh token generation.
Changes:
- Add scope claim to JWT payload for scoped tokens
- TokenService.createTokens accepts scope and skipRefreshToken opts
- JwtAuthGuard enforces scope via @AllowScope decorator metadata
- Device-approval controller decorated with @AllowScope('device-approval')
- Auth controller skips refresh cookie for scoped tokens
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: f62a6e2f0cc7
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/api/src/auth/services/token.service.spec.ts (1)
117-133: Test could verify refresh token is still created when onlyscopeis provided.This test passes
{ scope: ['device-approval'] }withoutskipRefreshToken, which should still create a refresh token. Consider adding an assertion thatrefreshTokenRepo.savewas called:expect(jwtService.sign).toHaveBeenCalledWith( { sub: 'user-123', publicKey: 'public-key', scope: ['device-approval'] }, { expiresIn: '15m' } ); + expect(refreshTokenRepo.save).toHaveBeenCalled(); });This would make the test more complete by verifying the expected behavior difference between
scope-only vsscope + skipRefreshToken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/token.service.spec.ts` around lines 117 - 133, Update the test for createTokens to also assert a refresh token is created when only options.scope is provided: after calling service.createTokens('user-123', 'public-key', { scope: ['device-approval'] }), add an expectation that refreshTokenRepo.save was called (e.g., toHaveBeenCalled() or toHaveBeenCalledTimes(1)) to ensure refreshTokenRepo.save is invoked; keep the existing jwtService.sign assertion and do not set skipRefreshToken in this test so it verifies the branch that persists a refresh token.apps/api/src/auth/dto/login.dto.ts (1)
23-27: Consider tightening the placeholder suffix pattern.The current regex
pending-core-kit-.+accepts any non-empty suffix. If the placeholder format is alwayspending-core-kit-{UUID}, you could constrain it further to prevent malformed values from reaching downstream validation:-@Matches(/^(04[0-9a-fA-F]{128}|pending-core-kit-.+)$/, { +@Matches(/^(04[0-9a-fA-F]{128}|pending-core-kit-[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$/i, {That said, since the auth service validates
payload.subas a valid userId anyway, the current pattern is functional—this is just a defense-in-depth suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/dto/login.dto.ts` around lines 23 - 27, Tighten the Matches regex on the publicKey property in login.dto.ts: replace the permissive "pending-core-kit-.+" branch in the `@Matches`(...) decorator with a UUID-constraining pattern so placeholders must be "pending-core-kit-{UUID}" (use a standard UUID regex: 8-4-4-4-12 hex groups, case-insensitive). Update the decorator on the publicKey field (the `@Matches`(...) usage) to allow either the uncompressed secp256k1 key or "pending-core-kit-" followed by a valid UUID.apps/api/src/auth/services/token.service.ts (1)
37-42: Consider a typed interface for JWT payload.Using
Record<string, unknown>loses type safety. A typed interface would improve maintainability and catch payload issues at compile time.♻️ Optional: Define a typed payload interface
+interface AccessTokenPayload { + sub: string; + publicKey: string; + scope?: string[]; +} + async createTokens( userId: string, publicKey: string, options?: CreateTokenOptions ): Promise<TokenPair> { - const payload: Record<string, unknown> = { sub: userId, publicKey }; - if (options?.scope) { - payload.scope = options.scope; - } + const payload: AccessTokenPayload = { sub: userId, publicKey }; + if (options?.scope) { + payload.scope = options.scope; + } const accessToken = this.jwtService.sign(payload, { expiresIn: '15m' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/services/token.service.ts` around lines 37 - 42, Define a proper typed interface for the JWT payload (e.g. JwtPayload with sub: string, publicKey: string, and optional scope?: string | string[]) and replace the loose Record<string, unknown> usage in token.service.ts: use the JwtPayload type for the payload variable, ensure the optional scope is typed and assigned correctly, and update the call to this.jwtService.sign to use the JwtPayload type (or generics) so the payload structure is checked at compile time (refer to payload, userId, publicKey, options?.scope, accessToken, and this.jwtService.sign).
🤖 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/api/src/auth/guards/jwt-auth.guard.ts`:
- Around line 11-14: The current async canActivate in JwtAuthGuard awaits
super.canActivate(context) but doesn't handle the Observable<boolean>
possibility; change the canActivate signature to return Promise<boolean> |
boolean | Observable<boolean> and update the implementation to detect if
super.canActivate(context) returns an Observable (from super.canActivate),
convert it to a Promise using rxjs firstValueFrom, then await/return the
boolean; reference canActivate and super.canActivate(context) in JwtAuthGuard
when applying the change.
---
Nitpick comments:
In `@apps/api/src/auth/dto/login.dto.ts`:
- Around line 23-27: Tighten the Matches regex on the publicKey property in
login.dto.ts: replace the permissive "pending-core-kit-.+" branch in the
`@Matches`(...) decorator with a UUID-constraining pattern so placeholders must be
"pending-core-kit-{UUID}" (use a standard UUID regex: 8-4-4-4-12 hex groups,
case-insensitive). Update the decorator on the publicKey field (the
`@Matches`(...) usage) to allow either the uncompressed secp256k1 key or
"pending-core-kit-" followed by a valid UUID.
In `@apps/api/src/auth/services/token.service.spec.ts`:
- Around line 117-133: Update the test for createTokens to also assert a refresh
token is created when only options.scope is provided: after calling
service.createTokens('user-123', 'public-key', { scope: ['device-approval'] }),
add an expectation that refreshTokenRepo.save was called (e.g.,
toHaveBeenCalled() or toHaveBeenCalledTimes(1)) to ensure refreshTokenRepo.save
is invoked; keep the existing jwtService.sign assertion and do not set
skipRefreshToken in this test so it verifies the branch that persists a refresh
token.
In `@apps/api/src/auth/services/token.service.ts`:
- Around line 37-42: Define a proper typed interface for the JWT payload (e.g.
JwtPayload with sub: string, publicKey: string, and optional scope?: string |
string[]) and replace the loose Record<string, unknown> usage in
token.service.ts: use the JwtPayload type for the payload variable, ensure the
optional scope is typed and assigned correctly, and update the call to
this.jwtService.sign to use the JwtPayload type (or generics) so the payload
structure is checked at compile time (refer to payload, userId, publicKey,
options?.scope, accessToken, and this.jwtService.sign).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api/src/auth/auth.controller.tsapps/api/src/auth/auth.service.spec.tsapps/api/src/auth/auth.service.tsapps/api/src/auth/decorators/allow-scope.decorator.tsapps/api/src/auth/dto/login.dto.tsapps/api/src/auth/guards/jwt-auth.guard.tsapps/api/src/auth/services/token.service.spec.tsapps/api/src/auth/services/token.service.tsapps/api/src/auth/strategies/jwt.strategy.tsapps/api/src/common/types.tsapps/api/src/device-approval/device-approval.controller.tsapps/web/src/components/mfa/SecurityTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/mfa/SecurityTab.tsx
Move pending auth state (pendingCipherboxJwt, pendingAuthMethod) from useState in useAuth to Zustand store so it's shared across hook instances. Add generation counter to useDeviceApproval to prevent stale requestApproval completions from overwriting active ephemeral keys. Use stable refs for requestApproval/cancelRequest in DeviceWaitingScreen to prevent effect re-fires on upstream dependency cascades. Track handled request IDs to filter stale polling responses after approve/deny. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 298b5fb3187c
Add test for payload.scope attachment to fix branch coverage (75% → 83%) which was below the 80% threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: fbf6eb4f48fe
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/hooks/useDeviceApproval.ts`:
- Around line 282-300: cancelRequest currently clears local state but doesn't
invalidate in-flight requestApproval calls; update cancelRequest to advance the
generation token stored in requestGenRef (e.g., increment requestGenRef.current
or set a new sentinel) after capturing/clearing requestIdRef so any concurrent
requestApproval that checks a captured generation will detect the mismatch and
abort re-registering pollers/requests; ensure requestApproval uses the
generation token from requestGenRef to ignore stale completions. Reference
cancelRequest, requestGenRef, requestApproval, requestIdRef,
stopRequesterPolling, clearEphemeralKey, and setApprovalStatus when making this
change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api/src/auth/strategies/jwt.strategy.spec.tsapps/web/src/components/mfa/DeviceWaitingScreen.tsxapps/web/src/hooks/useAuth.tsapps/web/src/hooks/useDeviceApproval.tsapps/web/src/stores/auth.store.tstests/e2e/utils/mfa-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/utils/mfa-helpers.ts
…any from specs Increment requestGenRef in cancelRequest so stale requestApproval() completions detect the mismatch and abort (CodeRabbit review feedback). Replace all `as any` casts in shares spec files with proper types (RequestWithUser, User, ShareInvite) to resolve @typescript-eslint/ no-explicit-any lint warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 830be6c4b914
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/hooks/useDeviceApproval.ts (3)
403-413: Duplicated polling logic could be extracted.The poll function here mirrors lines 316-328. Consider extracting a shared
fetchFilteredPendinghelper to reduce duplication and ensure consistent handling.💡 Example extraction
const fetchFilteredPending = useCallback(async () => { const pending = await deviceApprovalApi.getPending(); const handled = handledRequestIdsRef.current; return handled.size > 0 ? pending.filter((r) => r.requestId && !handled.has(r.requestId)) : pending; }, []);Then use in both
pollPendingRequestsand the visibility effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useDeviceApproval.ts` around lines 403 - 413, Extract the duplicated polling/filtering logic into a shared helper (e.g., fetchFilteredPending) that calls deviceApprovalApi.getPending(), reads handledRequestIdsRef.current, and returns the filtered pending array; make it a stable function (useCallback) and replace the logic currently in poll (inside useDeviceApproval) and the similar logic in pollPendingRequests / the visibility effect to call fetchFilteredPending and setPendingRequests with its result.
319-324: Non-null assertion onr.requestIdmay mask type issues.If the
PendingApprovaltype allowsrequestIdto be optional/undefined, this assertion could cause unexpected behavior. Consider a safer check.💡 Suggested safer filter
- const filtered = - handled.size > 0 ? pending.filter((r) => !handled.has(r.requestId!)) : pending; + const filtered = + handled.size > 0 + ? pending.filter((r) => r.requestId && !handled.has(r.requestId)) + : pending;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useDeviceApproval.ts` around lines 319 - 324, The filter uses a non-null assertion on r.requestId which can hide missing IDs; update the filter in useDeviceApproval so it explicitly handles undefined requestId values instead of using r.requestId!. For example, change the predicate used with pending.filter to check if r.requestId is null/undefined and keep those items, otherwise check handledRequestIdsRef.current.has(r.requestId) (references: handledRequestIdsRef, pending, setPendingRequests, PendingApproval, requestId). Ensure the logic still only removes items whose defined requestId is present in handled.
71-74:handledRequestIdsRefis never cleared — potential memory accumulation.The Set accumulates request IDs indefinitely. While individual IDs are small, clearing this when polling stops would prevent unbounded growth during long sessions.
💡 Proposed fix to clear on polling stop
const stopApproverPolling = useCallback(() => { if (approverPollRef.current) { clearInterval(approverPollRef.current); approverPollRef.current = null; } isPollingPendingRef.current = false; + handledRequestIdsRef.current.clear(); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useDeviceApproval.ts` around lines 71 - 74, handledRequestIdsRef in useDeviceApproval is never cleared which can lead to unbounded Set growth; add logic to clear handledRequestIdsRef.current (call .clear()) when polling stops and on component unmount. Locate the polling effect or functions in useDeviceApproval (the effect that starts/stops polling or the startPolling/stopPolling handlers) and invoke handledRequestIdsRef.current.clear() in the stop path and in the effect cleanup so the Set is reset whenever polling is terminated or the hook unmounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/hooks/useDeviceApproval.ts`:
- Around line 403-413: Extract the duplicated polling/filtering logic into a
shared helper (e.g., fetchFilteredPending) that calls
deviceApprovalApi.getPending(), reads handledRequestIdsRef.current, and returns
the filtered pending array; make it a stable function (useCallback) and replace
the logic currently in poll (inside useDeviceApproval) and the similar logic in
pollPendingRequests / the visibility effect to call fetchFilteredPending and
setPendingRequests with its result.
- Around line 319-324: The filter uses a non-null assertion on r.requestId which
can hide missing IDs; update the filter in useDeviceApproval so it explicitly
handles undefined requestId values instead of using r.requestId!. For example,
change the predicate used with pending.filter to check if r.requestId is
null/undefined and keep those items, otherwise check
handledRequestIdsRef.current.has(r.requestId) (references: handledRequestIdsRef,
pending, setPendingRequests, PendingApproval, requestId). Ensure the logic still
only removes items whose defined requestId is present in handled.
- Around line 71-74: handledRequestIdsRef in useDeviceApproval is never cleared
which can lead to unbounded Set growth; add logic to clear
handledRequestIdsRef.current (call .clear()) when polling stops and on component
unmount. Locate the polling effect or functions in useDeviceApproval (the effect
that starts/stops polling or the startPolling/stopPolling handlers) and invoke
handledRequestIdsRef.current.clear() in the stop path and in the effect cleanup
so the Set is reset whenever polling is terminated or the hook unmounts.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/share-invites.controller.spec.tsapps/api/src/shares/shares.service.spec.tsapps/web/src/hooks/useDeviceApproval.ts
super.canActivate() can return boolean | Promise<boolean> | Observable<boolean> per the CanActivate interface contract. Use firstValueFrom from rxjs to convert Observable case before awaiting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b4b5b389586a
…ceApproval - Extract duplicated poll+filter logic into fetchFilteredPending callback used by both pollPendingRequests and the visibility effect - Replace non-null assertion (r.requestId!) with explicit truthy check - Clear handledRequestIdsRef when approver polling stops to prevent unbounded Set growth during long sessions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b111b0fa91ff
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/hooks/useDeviceApproval.ts (1)
245-256: Zero-fill superseded ephemeral secret before early return.When a
requestApproval()generation is stale, the function returns without explicitly wiping the localephemeral.secretKey. That key material remains in memory until GC.♻️ Suggested hardening
const requestApproval = useCallback(async () => { const gen = ++requestGenRef.current; + let localEphemeralSecret: Uint8Array | null = null; setApprovalStatus('requesting'); setApprovalError(null); try { // 1. Generate ephemeral secp256k1 keypair // keygen() returns compressed pubkey (33 bytes) by default, but the // backend DTO requires uncompressed (65 bytes = 130 hex chars). const ephemeral = secp256k1.keygen(); - ephemeralPrivKeyRef.current = ephemeral.secretKey; - const uncompressedPubKey = secp256k1.getPublicKey(ephemeral.secretKey, false); + localEphemeralSecret = ephemeral.secretKey; + ephemeralPrivKeyRef.current = localEphemeralSecret; + const uncompressedPubKey = secp256k1.getPublicKey(localEphemeralSecret, false); const ephemeralPubKeyHex = bytesToHex(uncompressedPubKey); @@ if (gen !== requestGenRef.current) { + localEphemeralSecret?.fill(0); // Stale: a newer requestApproval already started. Cancel this request // on the backend and bail — the newer call will set its own refs. try { await deviceApprovalApi.cancel(requestId); } catch { /* best-effort */ } return; } @@ } catch (err) { // Only handle errors for the active generation - if (gen !== requestGenRef.current) return; + if (gen !== requestGenRef.current) { + localEphemeralSecret?.fill(0); + return; + } clearEphemeralKey(); setApprovalError(err instanceof Error ? err.message : 'Failed to create approval request'); setApprovalStatus('error'); } }, [startRequesterPolling, clearEphemeralKey]);Also applies to: 264-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useDeviceApproval.ts` around lines 245 - 256, When detecting a stale generation in the requestApproval flow (the if-check comparing gen to requestGenRef.current), zero out the ephemeral secret before returning: set ephemeral.secretKey (and any other ephemeral secret fields on the ephemeral object) to a zeroed/empty value prior to calling deviceApprovalApi.cancel and the early return; do the same zero-fill in the other early-return path referenced in the comment (the latter ephemeral wipe around the requestId cancel branch). Locate symbols requestApproval, gen, requestGenRef, ephemeral.secretKey, deviceApprovalApi.cancel, and requestId to apply this change.
🤖 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/hooks/useDeviceApproval.ts`:
- Around line 105-106: The fetchFilteredPending() call can resolve after polling
has been stopped and then call setPendingRequests with stale data; add a
post-await guard that verifies polling is still active before updating state.
Introduce or reuse a ref flag (e.g., isPollingRef.current) that
stopApproverPolling() sets false and the poll starter sets true, and in the
places that call await fetchFilteredPending() (referenced by
fetchFilteredPending and the code paths that call setPendingRequests) check
isPollingRef.current (or that handledRequestIdsRef hasn’t been cleared)
immediately after the await and return early if polling stopped so
setPendingRequests is not invoked with stale results.
---
Nitpick comments:
In `@apps/web/src/hooks/useDeviceApproval.ts`:
- Around line 245-256: When detecting a stale generation in the requestApproval
flow (the if-check comparing gen to requestGenRef.current), zero out the
ephemeral secret before returning: set ephemeral.secretKey (and any other
ephemeral secret fields on the ephemeral object) to a zeroed/empty value prior
to calling deviceApprovalApi.cancel and the early return; do the same zero-fill
in the other early-return path referenced in the comment (the latter ephemeral
wipe around the requestId cancel branch). Locate symbols requestApproval, gen,
requestGenRef, ephemeral.secretKey, deviceApprovalApi.cancel, and requestId to
apply this change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/auth/guards/jwt-auth.guard.tsapps/web/src/hooks/useDeviceApproval.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/auth/guards/jwt-auth.guard.ts
Check isPollingPendingRef after await to prevent setPendingRequests from being called with stale data when the response resolves after polling has been stopped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ffe4c5f371e3
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>
5 tasks
4 tasks
Closed
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
pending-core-kit-*placeholder publicKey for REQUIRED_SHARE temp auth (was rejected with 400)useDeviceApprovalgenerates uncompressed secp256k1 ephemeral public key (130 hex chars) instead of compressed (66 hex chars)!isRequiredShare; SettingsPage skips auth redirect during MFA enrollmentnavigate('/files')from login functions — Login.tsx redirect effect handles navigation centrallyTest plan
--grep-invert @mfa)pnpm exec playwright test tests/mfa-flows.spec.tsagainst staging API🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores