feat(12): Core Kit Identity Provider Foundation#123
Conversation
Insert a decimal phase for urgent work discovered mid-milest Entire-Checkpoint: c961fe9a7d37
Phase 12: Multi-Factor Authentication - Implementation decisions documented - Phase boundary established - Critical research flags: key identity after MFA, test automation path - Reference: ChainSafe Files SDK integration pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 12: Multi-Factor Authentication - Confirmed key identity preserved after MFA (Shamir SSS splits, not replaces) - Mapped SDK hooks: useEnableMFA, useManageMFA, useWeb3AuthUser.isMFAEnabled - Documented all 6 MFA factor types and configuration patterns - E2E testing strategy: environment-aware mfaLevel to protect test accounts - Identified SCALE plan requirement for production mfaSettings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 12: Multi-Factor Authentication - 2 plan(s) in 2 wave(s) - 1 parallel (wave 1), 1 sequential (wave 2 checkpoint) - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…012z1v) Entire-Checkpoint: 5f0af6f2363f
Architectural pivot from PnP Modal SDK to MPC Core Kit based on discussion revealing insufficient control over MFA UX, cross-device approval, and wallet login unification. Phase 12 split into dependency chain: - 12: Core Kit + CipherBox identity provider (this phase) - 12.2: Encrypted device registry on IPFS - 12.3: SIWE + unified identity (wallet unification) - 12.4: MFA enrollment + cross-device approval Key decisions: CipherBox as identity provider (sub=userId), identity trilemma resolved (wallet-only + unified, SPOF accepted with mitigations), no mandatory email for wallet users. Deleted outdated PnP-based plans (12-01, 12-02). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 12: Core Kit Identity Provider Foundation - 5 plans in 4 waves - Wave 1: Backend identity provider + Core Kit SDK install (parallel) - Wave 2: Frontend auth flow rewrite (loginWithJWT, key export) - Wave 3: Custom CipherBox-branded login UI (Google + email) - Wave 4: PnP migration (importTssKey) + cleanup + E2E verify - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address 3 blockers and 2 warnings from plan verification: - BLOCKER 1: Commit to GIS google.accounts.id approach, remove alternatives - BLOCKER 2: Add backend task to resolve placeholder publicKey users - BLOCKER 3: Add Web3Auth dashboard verifier config checkpoint - WARNING 2: Add E2E test auth helper update task to Plan 05 - WARNING 3: Clarify ioredis (not BullMQ) for Redis OTP storage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add @web3auth/mpc-core-kit for programmatic factor management - Add @toruslabs/tss-dkls-lib for secp256k1 TSS signing (WASM) - Add @web3auth/ethereum-mpc-provider for EIP-1193 compatibility - Existing @web3auth/modal preserved (removed in Plan 05) - Vite build succeeds with existing Buffer/process polyfills Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 495845202f26
- Fix jose v6 type compatibility (KeyLike -> CryptoKey | KeyObject) - Strip RSA private fields from JWK without unused-var lint violations - Register IdentityController and JwtIssuerService in auth module - JWKS endpoint at GET /auth/.well-known/jwks.json returns RS256 public key - JwtIssuerService signs identity JWTs with iss=cipherbox, aud=web3auth, sub=userId Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Core Kit singleton with environment-aware network selection (DEVNET/MAINNET) - getCoreKit() creates single Web3AuthMPCCoreKit instance with DKLS TSS lib - initCoreKit() initializes SDK and returns COREKIT_STATUS - CoreKitProvider wraps singleton in React context with status tracking - useCoreKit() hook exposes coreKit instance, status, error, and reinitialize - Handles COREKIT_STATUS state machine: NOT_INITIALIZED, INITIALIZED, LOGGED_IN, REQUIRED_SHARE - manualSync: true for explicit commitChanges() control - NOT mounted in main.tsx yet -- Plan 03 wires the auth flow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 - Task 1: Install Core Kit SDK + Polyfills (already committed) - Task 2: Core Kit Singleton + React Context Provider SUMMARY: .planning/phases/12-multi-factor-authentication/12-02-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- GoogleOAuthService: verify Google idTokens via Google JWKS - EmailOtpService: OTP generation, argon2 hashing, Redis storage with TTL - IdentityController: POST /auth/identity/google, send-otp, verify-otp - Find-or-create user by email with cross-auth-method linking - Rate limiting on OTP send (5 req/15min per IP via @Throttle) - OpenAPI spec updated with Identity tag and all new endpoints - Web API client regenerated with identity endpoint types - All 391 existing tests pass, backward compatible Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 - Task 1: JWT Issuer Service + JWKS Endpoint - Task 2: Google OAuth + Email OTP + Identity Endpoints SUMMARY: .planning/phases/12-multi-factor-authentication/12-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace PnP useAuthFlow with useCoreKitAuth hook - Add loginWithGoogle, loginWithEmailOtp, getVaultKeypair, getPublicKeyHex, logout to Core Kit hooks - Add identity provider endpoints to authApi - Update LinkedMethods for Core Kit compatibility - Add deprecated useAuthFlow shim for build compat Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rewrite useAuth hook: loginWithGoogle, loginWithEmail
entry points using Core Kit loginWithJWT flow
- Mount CoreKitProvider in main.tsx, remove PnP providers
- Add placeholder publicKey resolution in backend login()
(pending-core-kit-{userId} -> real Core Kit publicKey)
- Extract Web3Auth session JWT from coreKit.signatures
(authenticateUser() not available in Core Kit SDK)
- Session restoration via Core Kit built-in sessions
- Remove deprecated useAuthFlow shim from hooks.ts
- Update UserMenu and remove PnP userInfo dependency
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 3/3 - Configure Web3Auth custom verifier (manual) - Core Kit auth hooks + API client extensions - Backend placeholder resolution + useAuth rewrite + CoreKitProvider SUMMARY: .planning/phases/12-multi-factor-authentication/12-03-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- GoogleLoginButton: GIS library dynamic load, popup flow, disabled state - EmailLoginForm: two-step email/OTP with resend cooldown, back navigation - Terminal aesthetic CSS: green-on-black, monospace, glow effects - Accessible: ARIA labels, focus-visible, live regions for errors - Exported from components/auth/index.ts barrel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace AuthButton (Web3Auth modal) with GoogleLoginButton + EmailLoginForm - Login page shows Google OAuth button, "// or" divider, email OTP form - handleGoogleLogin/handleEmailLogin wired to useAuth hooks - AuthButton.tsx marked @deprecated (kept for Plan 05 cleanup) - Error states displayed via login-error role="alert" - Terminal aesthetic preserved: matrix background, green-on-black Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Core Kit SDK v3.5.0 doesn't have authenticateUser() — session tokens in coreKit.signatures are NOT verifiable JWTs. Instead, pass the CipherBox-issued JWT (used for loginWithJWT) to the backend with loginType 'corekit'. Backend verifies it against its own JWKS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add getMigrationKey() helper that reads PnP migration key from localStorage - Wire optional migrationKey parameter into loginWithGoogle and loginWithEmailOtp - Auto-detect migration key from localStorage if not explicitly passed - Add console logging for Core Kit publicKey after login (migration debugging) - Add new-user vault re-initialization warning for devnet migration period Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove @web3auth/modal from web app dependencies - Delete apps/web/src/lib/web3auth/config.ts (PnP config, replaced by core-kit.ts) - Delete apps/web/src/lib/web3auth/provider.tsx (PnP provider, replaced by core-kit-provider.tsx) - Update lockfile to reflect removed package - Verify no remaining PnP imports in codebase - Core Kit is now the sole Web3Auth integration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rewrite web3auth-helpers for CipherBox login UI - Remove Web3Auth modal popup/iframe automation - Add data-testid attrs to EmailLoginForm and GoogleLoginButton for E2E targeting - Update LoginPage page object with email/OTP methods - Add SKIP_CORE_KIT direct API auth fallback - Update logout assertion for new login page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 3/3 (Task 4 checkpoint pending) - PnP migration support via importTssKey - @web3auth/modal removed, dead code deleted - E2E test auth helpers updated for Core Kit SUMMARY: .planning/phases/12-multi-factor-authentication/12-05-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All 5 plans executed and verified end-to-end: - Plan 01: Backend identity provider (JWKS, Google OAuth, email OTP) - Plan 02: Core Kit SDK + React context provider - Plan 03: Frontend auth flow rewrite - Plan 04: Custom login UI + corekit login type fix - Plan 05: PnP migration, SDK removal, E2E updates Verified: email login, session persistence, logout, no PnP remnants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements Phase 12: Core Kit Identity Provider Foundation — replaces Web3Auth PnP modal with an MPC Core Kit and CipherBox IDP. Adds backend identity endpoints (Google OAuth, Email OTP, JWKS), frontend Core Kit singleton/provider and hooks, new login UI (Google + Email OTP), test-login for E2E, extensive planning/docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend<br/>(React)
participant CoreKit as Core Kit<br/>(MPC)
participant API as Backend<br/>Identity API
participant JWKS as JWKS<br/>Endpoint
User->>Frontend: Click Google Login
Frontend->>Frontend: Load GIS & Request idToken
User->>Frontend: Complete Google OAuth
Frontend->>API: POST /auth/identity/google (idToken)
API->>API: Verify Google idToken
API->>API: Find or create user, issue CipherBox JWT
API-->>Frontend: Return {idToken,userId,isNewUser}
Frontend->>CoreKit: loginWithJWT(cipherboxJwt, importTssKey?)
CoreKit->>JWKS: Fetch JWKS for verification
CoreKit-->>Frontend: Return COREKIT_STATUS
Frontend->>API: POST /auth/login (loginType: corekit, publicKey)
API->>API: Verify CipherBox JWT, resolve placeholder key
API-->>Frontend: Return {accessToken, refreshToken}
Frontend->>Frontend: Store tokens and load vault
Frontend-->>User: Redirect to /files
sequenceDiagram
actor User
participant Frontend as Frontend<br/>(React)
participant API as Backend<br/>Identity API
participant Redis as Redis<br/>(OTP Store)
participant Email as Email<br/>Service
User->>Frontend: Enter email
Frontend->>API: POST /auth/identity/email/send-otp
API->>Redis: Check rate limit
alt Within limit
API->>API: Generate OTP and hash
API->>Redis: Store hashed OTP (TTL)
API->>Email: Send OTP (or log in dev)
API-->>Frontend: Success
else Rate limit exceeded
API-->>Frontend: 429
end
User->>Frontend: Enter OTP
Frontend->>API: POST /auth/identity/email/verify-otp
API->>Redis: Retrieve hashed OTP
alt Valid OTP
API->>API: Find/create user and issue CipherBox JWT
API->>Redis: Delete OTP (single-use)
API-->>Frontend: Return {idToken, userId, isNewUser}
else Invalid/expired
API-->>Frontend: 401
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Entire-Checkpoint: 6249230f2186
Entire-Checkpoint: 294ec7fb98f3
Entire-Checkpoint: 343a47c5f8d0
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/auth/dto/login.dto.ts (1)
7-13:⚠️ Potential issue | 🟡 MinorAPI description is misleading for the
corekitlogin type.The
idTokendescription says "JWT ID token from Web3Auth," but forcorekitlogins, this is a CipherBox-issued RS256 JWT (from/auth/identity/*endpoints), not a Web3Auth token. Consider updating the description to reflect both cases.Proposed fix
`@ApiProperty`({ - description: 'JWT ID token from Web3Auth', + description: 'JWT ID token (Web3Auth-issued for social/external_wallet, or CipherBox-issued for corekit)', example: 'eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9...', })
🤖 Fix all issues with AI agents
In @.planning/STATE.md:
- Around line 23-32: Update the arithmetic in .planning/STATE.md: correct "Total
plans completed" from 78 to 77, correct "Total execution time" from 6.3 hours to
6.35 hours, and correct "Average duration" from 4.6 min to 4.95 min (computed as
381 total minutes ÷ 77 plans); ensure the three header/table entries named
"Total plans completed", "Total execution time", and "Average duration" are
replaced with these corrected values.
In `@apps/api/src/auth/auth.service.ts`:
- Around line 246-256: In refreshByToken, the lookup currently restricts to
type: 'email_passwordless' so Google-only users get email undefined; change the
authMethodRepository.findOne call (the emailMethod lookup) to remove the type
filter and instead query for the given userId with identifier IS NOT NULL (or
equivalent) ordered by lastUsedAt DESC so you return the most recently used auth
method's identifier (email) for any provider; update the variable emailMethod
and returned email to use that identifier.
In `@apps/web/src/hooks/useAuth.ts`:
- Around line 91-125: The catch inside initializeOrLoadVault currently logs
non-404 errors and lets execution continue, causing callers like
completeBackendAuth to proceed without vault keys; replace the
console.error('[useAuth] Failed to load vault:', error) with re-throwing the
original error (e.g., throw error) so vaultApi.getVault() failures (500/network)
propagate to the login flow and can be surfaced to the user; keep the 404 branch
behavior (initialize vault) unchanged and ensure any surrounding async callers
handle the propagated error.
🧹 Nitpick comments (9)
.github/workflows/e2e.yml (1)
106-106: Consider using a GitHub Actions secret instead of a hardcoded test secret.The
TEST_LOGIN_SECRETis hardcoded in the workflow YAML (e2e-test-secret-ci-only). While the test-login endpoint has defense-in-depth guards (rejects production, requiresNODE_ENV !== 'production'), storing it as a GitHub secret (e.g.,${{ secrets.TEST_LOGIN_SECRET }}) would follow the principle of least exposure and avoid committing secrets to version control, even test-only ones.Also applies to: 136-138
apps/web/src/lib/web3auth/hooks.ts (1)
183-195:hexToBytesandbytesToHexare duplicated fromsignatureKeyDerivation.ts.
bytesToHex(and likelyhexToBytes) already exists inapps/web/src/lib/crypto/signatureKeyDerivation.ts(lines 313-317). Consider importing from there or extracting both into a shared utility module to avoid drift.apps/api/src/auth/services/email-otp.service.ts (2)
47-55: Rate-limit check has a TOCTOU race condition.The
GET+ compare +INCRsequence (lines 52-55 vs. 72) is not atomic. Two concurrentsendOtpcalls for the same email could both readattempts = 4, both pass the check, and both increment — allowing one extra OTP beyondMAX_SEND_ATTEMPTS. For a rate limiter this is low-severity, but it can be tightened by usingINCRfirst, then checking the result:Proposed fix
- // Check rate limit - const rateLimitKey = `otp-attempts:${normalizedEmail}`; - const attempts = await this.redis.get(rateLimitKey); - if (attempts && parseInt(attempts, 10) >= MAX_SEND_ATTEMPTS) { - throw new BadRequestException('Too many OTP requests. Please try again later.'); - } + // Check rate limit (atomic: INCR first, then check) + const rateLimitKey = `otp-attempts:${normalizedEmail}`; + const currentAttempts = await this.redis.incr(rateLimitKey); + if (currentAttempts === 1) { + await this.redis.expire(rateLimitKey, RATE_LIMIT_TTL); + } + if (currentAttempts > MAX_SEND_ATTEMPTS) { + throw new BadRequestException('Too many OTP requests. Please try again later.'); + }Then remove the duplicate INCR block at lines 72-75.
27-34: Service creates its own Redis connection instead of reusing a shared one.
EmailOtpServiceinstantiates a newRedisclient directly. If BullMQ or other services in the app already use Redis, consider injecting a shared Redis provider to avoid connection proliferation and simplify configuration management.apps/web/src/lib/api/auth.ts (1)
23-29: LocalIdentityTokenResponsetype duplicates the generatedIdentityTokenResponseDto.A generated type exists at
apps/web/src/api/models/identityTokenResponseDto.tswith the same shape. Consider importing it to avoid manual sync when the backend DTO changes.apps/api/src/auth/dto/identity.dto.ts (1)
33-39: Consider adding a numeric pattern constraint on the OTP field.
@Length(6, 6)allows any 6-character string (e.g.,"abcdef"), which would pass DTO validation but always fail OTP verification. Adding@Matches(/^\d{6}$/)would return a clearer 400 error instead of a 401 from the OTP service.Proposed fix
+import { IsString, IsNotEmpty, IsEmail, Length, Matches } from 'class-validator'; ... `@IsString`() `@Length`(6, 6) + `@Matches`(/^\d{6}$/, { message: 'OTP must be a 6-digit number' }) otp!: string;packages/api-client/openapi.json (2)
997-1022:TestLoginResponseDtoexposes a private key field in the public OpenAPI contract.While the endpoint itself is guarded by
TEST_LOGIN_SECRETand blocked in production, theprivateKeyHexfield is visible in the OpenAPI spec and will appear in all generated API clients. Consider whether this test-only DTO should be excluded from the public spec (e.g., via a separate spec or@ApiExcludeEndpoint()) to avoid accidentally suggesting private key transport is part of the standard API surface.If this is intentional for E2E tooling that consumes the generated client, this is acceptable—just flagging the optics.
263-274: JWKS endpoint has no response schema — generated clients will lack type info.The
200response only has adescriptionstring. Adding an inline schema (or aJwksResponseDtoreference) would give generated API clients a typed response.Suggested schema
"responses": { "200": { - "description": "JWKS containing RS256 public key for JWT verification" + "description": "JWKS containing RS256 public key for JWT verification", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "keys": { + "type": "array", + "items": { + "type": "object" + } + } + }, + "required": ["keys"] + } + } + } }apps/api/src/auth/auth.service.ts (1)
386-389: Minor: step numbering is1, 2, 2, 3, 4— second// 2.should be// 3.Line 388 duplicates step number 2.
Fix
- // 2. Generate deterministic secp256k1 keypair from email + // 3. Generate deterministic secp256k1 keypair from emailAnd renumber subsequent steps accordingly (3→4, 4→5).
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/api/src/auth/auth.service.ts (1)
116-152:⚠️ Potential issue | 🔴 Critical
authMethodis possiblynull— causes pipeline failure and potential runtime NPE.In the
corekitbranch (lines 118-128), bothfindOnecalls can returnnull(new user with no auth methods yet, or identity controller didn't create one). Control then falls through to line 151 whereauthMethod.lastUsedAtis accessed unconditionally, causing TS18047 and a runtime crash.The non-corekit branch is safe because lines 138-143 always create a new auth method when
findOnereturns null.Proposed fix — add a null guard for the corekit path
// Fallback: find any auth method for this user (identity controller should have created one) authMethod = await this.authMethodRepository.findOne({ where: { userId: user.id }, }); + if (!authMethod) { + // Identity controller should have created an auth method; create one as a safety net + authMethod = await this.authMethodRepository.save({ + userId: user.id, + type: (payload.email ? 'email_passwordless' : 'google') as AuthMethod['type'], + identifier: payload.email || payload.sub || 'unknown', + }); + } } else {
🧹 Nitpick comments (2)
apps/api/src/auth/auth.service.ts (1)
381-398: Defense-in-depth and timing-safe comparison look solid; minor duplicate step numbering.The production guard (line 382-385), missing-secret guard (line 389-391), and
timingSafeEqual(line 394) are all appropriate. However, there are two comments labeled "step 2" (lines 387 and 398).Fix step numbering
- // 2. Validate TEST_LOGIN_SECRET with timing-safe comparison + // 2. Validate TEST_LOGIN_SECRET with timing-safe comparison ... - // 2. Generate deterministic secp256k1 keypair from email + // 3. Generate deterministic secp256k1 keypair from email const { publicKeyHex, privateKeyHex } = this.generateDeterministicKeypair(email); - // 3. Find or create user by email + // 4. Find or create user by email const normalizedEmail = email.toLowerCase().trim(); ... - // 4. Issue tokens + // 5. Issue tokensapps/api/src/auth/auth.service.spec.ts (1)
758-773:testLogintests don't cover theNODE_ENV === 'production'guard and use an imprecise mock.
- There's no test asserting that
testLoginthrowsForbiddenException('Test login is not available in production')whenNODE_ENVis'production'.configService.get.mockReturnValue(...)returns the same value for every key, soNODE_ENVinadvertently gets the same value asTEST_LOGIN_SECRET. UsemockImplementationto return key-specific values for more robust tests.Suggested additions
+ it('should throw ForbiddenException in production', async () => { + configService.get.mockImplementation((key: string) => { + if (key === 'NODE_ENV') return 'production'; + if (key === 'TEST_LOGIN_SECRET') return 'test-secret'; + return undefined; + }); + + await expect(service.testLogin('test@example.com', 'test-secret')).rejects.toThrow( + ForbiddenException + ); + }); + it('should throw ForbiddenException if TEST_LOGIN_SECRET not set', async () => { - configService.get.mockReturnValue(undefined); + configService.get.mockImplementation((key: string) => { + if (key === 'NODE_ENV') return 'test'; + return undefined; + }); await expect(service.testLogin('test@example.com', 'any-secret')).rejects.toThrow( ForbiddenException ); });
…erage - Add safety net: create email_passwordless auth method if identity controller didn't create one (shouldn't happen in practice) - Fixes TypeScript error: 'authMethod' is possibly null - Add tests for corekit fallback branches (identifier miss, full miss) - Branch coverage now meets 84% threshold for auth.service.ts - Regenerate OpenAPI spec Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…andling - Broaden refreshByToken email lookup to include both email_passwordless and google auth methods so Google-only users get their email in the refresh response - Re-throw non-404 vault errors in initializeOrLoadVault instead of silently swallowing them, preventing broken file browser state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…essons Captures key gotchas from 5-plan phase across 29 CodeRabbit review threads: - Auth method deduplication bug pattern (two paths creating same entity) - TypeScript strictness mismatch between ts-jest and ts-node/nest build - Core Kit SDK v3.5.0 missing authenticateUser() workaround - Placeholder publicKey resolution pattern - CodeRabbit iterative review workflow (plan for 2-3 rounds) - E2E test decoupling from Web3Auth via test-login endpoint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/hooks/useAuth.ts`:
- Around line 288-326: The session restore path in restoreSession (inside
useEffect) performs async work (authApi.refresh and initializeOrLoadVault)
without toggling the login/loading flag, so isLoading remains false while vault
keys load; modify restoreSession to set the existing isLoggingIn state (or
introduce an isRestoringSession flag) to true at the start of the try block and
ensure it is set back to false in a finally block after
setAccessToken/setUserEmail/initializeOrLoadVault (and after attempted
coreKitLogout in the catch) so consuming components see a loading state during
refresh; update any setters or dispatchers used to toggle
isLoggingIn/isRestoringSession accordingly where restoreSession is defined.
🧹 Nitpick comments (3)
apps/web/src/hooks/useAuth.ts (1)
170-227:isLoggingIninuseCallbackdeps causes unnecessary callback identity churn.Both
loginWithGoogleandloginWithEmailincludeisLoggingIn(auseStatevalue) in their dependency arrays to make the guard check work. This means every login attempt recreates both callbacks twice (once whenisLoggingInflips totrue, again when it flips tofalse), which can cascade re-renders to any consuming component.A
useReffor the guard would keep callback references stable:Suggested approach
- const [isLoggingIn, setIsLoggingIn] = useState(false); + const [isLoggingIn, setIsLoggingIn] = useState(false); + const isLoggingInRef = useRef(false);Then in the login callbacks:
const loginWithGoogle = useCallback( async (googleIdToken: string): Promise<void> => { - if (isLoggingIn) return; + if (isLoggingInRef.current) return; + isLoggingInRef.current = true; setIsLoggingIn(true); try { // ... } finally { + isLoggingInRef.current = false; setIsLoggingIn(false); } }, - [isLoggingIn, coreKitLoginGoogle, completeBackendAuth, setUserEmail, navigate] + [coreKitLoginGoogle, completeBackendAuth, setUserEmail, navigate] );Same for
loginWithEmail.apps/api/src/auth/auth.service.ts (1)
393-408: Step numbering typo: two steps labeled "2."Lines 399 and 410 are both labeled
// 2.— the second should be// 3.(and subsequent step renumbered to// 4.).Proposed fix
- // 2. Validate TEST_LOGIN_SECRET with timing-safe comparison + // 2. Validate TEST_LOGIN_SECRET with timing-safe comparison const expectedSecret = this.configService.get<string>('TEST_LOGIN_SECRET'); ... - // 2. Generate deterministic secp256k1 keypair from email + // 3. Generate deterministic secp256k1 keypair from email const { publicKeyHex, privateKeyHex } = this.generateDeterministicKeypair(email); - // 3. Find or create user by email + // 4. Find or create user by email const normalizedEmail = email.toLowerCase().trim(); ... - // 4. Issue tokens + // 5. Issue tokens const tokens = await this.tokenService.createTokens(user.id, user.publicKey);apps/api/src/auth/auth.service.spec.ts (1)
836-843: Missing test for the production environment guard.The
testLoginmethod has a defense-in-depth check that throwsForbiddenExceptionwhenNODE_ENV === 'production'(auth.service.ts line 395-397), but no test covers this branch. Consider adding:it('should throw ForbiddenException in production environment', async () => { configService.get.mockImplementation((key: string) => { if (key === 'NODE_ENV') return 'production'; if (key === 'TEST_LOGIN_SECRET') return 'test-secret'; return undefined; }); await expect(service.testLogin('test@example.com', 'test-secret')).rejects.toThrow( ForbiddenException ); });Also, the existing tests use
configService.get.mockReturnValue(...)which returns the same value for bothNODE_ENVandTEST_LOGIN_SECRETcalls. Consider usingmockImplementationto differentiate keys for more precise testing.
- Stop leaking internal error details to clients in Google OAuth and CipherBox JWT verification (log details server-side instead) - Add @matches(/^\d{6}$/) validator on OTP DTO for digits-only enforcement - Add JWKS endpoint response schema to OpenAPI spec - Remove duplicate hexToBytes/bytesToHex utils from hooks.ts (import from @cipherbox/crypto instead), fixes deprecated substr usage - Guard GoogleLoginButton GIS script load against unmounted state updates - Add ref guard to prevent concurrent session restore in useAuth - Fix stale "Web3Auth account linking" copy in LinkedMethods - Fix stale "Do NOT mount in main.tsx" comment in CoreKitProvider - Throw explicit error in getCoreKit() instead of non-null assertion - Regenerate API client with updated schemas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Session restore runs authApi.refresh() + initializeOrLoadVault() async, but isLoading was false during that window. Components like /files could render before vault keys were available. Set isLoggingIn=true at the start of restoreSession so isLoading reflects the in-progress state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two tasks: integrate @sendgrid/mail into EmailOtpService for real email delivery, and wire SENDGRID_API_KEY, SENDGRID_FROM_EMAIL, GOOGLE_CLIENT_ID, and VITE_GOOGLE_CLIENT_ID through the staging deploy pipeline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add @sendgrid/mail as runtime dependency - Send OTP email via SendGrid when SENDGRID_API_KEY is configured - Graceful degradation: SendGrid failure does not break OTP flow - Keep dev-mode console logging for local development - Add 3 unit tests for SendGrid send/skip/failure paths - Document SENDGRID_API_KEY and SENDGRID_FROM_EMAIL in .env.example Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add VITE_GOOGLE_CLIENT_ID to web build env - Add SENDGRID_API_KEY, SENDGRID_FROM_EMAIL, GOOGLE_CLIENT_ID to .env.staging generation - Document VITE_GOOGLE_CLIENT_ID in apps/web/.env.example Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 - Integrate SendGrid into EmailOtpService - Wire SendGrid and Google OAuth env vars through staging pipeline SUMMARY: .planning/quick/015-sendgrid-email-otp-and-google-oauth-staging/015-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: d7389a3e9644
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
importTssKeyfor existing usersWhat Changed
/identity/google,/identity/send-otp,/identity/verify-otp,/identity/tokenendpoints, JWT issuer service (RS256), JWKS endpointuseAuthhooks, customGoogleLoginButtonandEmailLoginFormcomponentsgetMigrationKey()helper reads PnP key from localStorage once, passes toimportTssKey, then deletes@web3auth/modaldependency, deleted PnPconfig.tsandprovider.tsxdata-testidattributesKey Decisions
sub=userId) — enables multi-auth linking, less data to Web3AuthimportTssKeyvia localStorage one-time read-and-delete pattern for PnP migrationjoselibrary for identity JWTs (separate RS256 signing keys from internal auth)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests