fix(auth): google OAuth brave fallback, wallet SIWE, sync & UX fixes#137
Conversation
… migration - Google OAuth: fall back to native Google Sign In button when One Tap is blocked (Brave, strict privacy browsers) - Identity endpoints: add intent=link parameter that verifies credential ownership without creating orphan users, fixing cross-account collision when linking the same email used for Google login - Add missing device_approvals table migration for production/staging Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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. WalkthroughAdds an optional "intent" ('login' | 'link') to Google, Email OTP, and Wallet identity flows; when intent='link' the backend issues a link-verification JWT and skips user lookup/creation. Also adds email normalization, a device_approvals migration, Google native button fallback, frontend model/api updates, and small SIWE/wagmi/file-browser adjustments. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Web Client
participant API as Identity API
participant JWT as JWT Issuer
User->>Frontend: Initiate "Link account" (Google / Email / Wallet)
Frontend->>API: identityGoogle/identityEmailVerify/identityWallet(creds, intent='link')
API->>API: Normalize email (if present) and detect intent
alt intent === 'link'
API->>JWT: Issue link-verification JWT (identifier='link-verification', email=normalized)
JWT-->>API: token
API-->>Frontend: { email, userId: null, isNewUser: null, token }
else intent !== 'link'
API->>API: Proceed with normal lookup/create and issue regular JWT
API-->>Frontend: { email, userId, isNewUser, token }
end
Frontend-->>User: Show result / next steps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/api/src/migrations/1740000000000-AddDeviceApprovals.ts (1)
15-16: Consider adding a foreign key constraint onuser_idreferencing theuserstable.Without a FK constraint, orphaned rows can accumulate if users are deleted. This is a minor concern since device approvals are short-lived (5min expiry), but a FK with
ON DELETE CASCADEwould maintain referential integrity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/migrations/1740000000000-AddDeviceApprovals.ts` around lines 15 - 16, Add a foreign key on the device_approvals table for the "user_id" column referencing the users table to prevent orphaned rows; update the AddDeviceApprovals migration to declare "user_id" as REFERENCES users(id) (or add ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE) so deletions of users cascade to device_approvals and maintain referential integrity.apps/api/src/auth/controllers/identity.controller.ts (1)
267-272: Wallet link endpoint returns unusedidToken— inconsistent with the linking flow.The
POST /auth/linkendpoint for wallet linking does not use the returnedidToken(seeLinkMethodDto:idTokenis not required for wallet). Wallet linking instead verifies the wallet address via SIWE signature verification inlinkWalletMethod, not by extracting a claim from the JWT. While this is the correct pattern for Web3 auth (SIWE-based verification rather than JWT claims), the endpoint returns anidTokenfor wallet that the client cannot use, which is confusing API design. Google and email paths both return the JWT in the response, but wallets do not benefit from the JWT claim approach since verification happens via a different mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/controllers/identity.controller.ts` around lines 267 - 272, The link flow is returning an unused idToken (created by jwtIssuerService.signIdentityJwt) for dto.intent === 'link', which confuses clients because wallet linking uses SIWE in linkWalletMethod rather than a JWT claim; remove the creation/return of idToken in that branch and return only the fields the client expects (e.g., { userId: '', isNewUser: false }) or set idToken to undefined, and update LinkMethodDto/response typing and any callers to reflect that wallet link responses do not include an idToken; ensure references to jwtIssuerService.signIdentityJwt and linkWalletMethod are adjusted accordingly.apps/web/src/components/auth/GoogleLoginButton.tsx (2)
116-137: Redundantgoogle.accounts.id.initializecall — andhandleCredentialResponsealready wrapssetLoading(false), so the extrasetLoading(true)wrapper may leave loading stuck if the user dismisses the native popup.The native button useEffect re-initializes GIS (lines 121-128) even though the prior
handleClickalready initialized it moments before the fallback triggered. More importantly, the callback wrapshandleCredentialResponsewith asetLoading(true)call (line 124), but there is no timeout or dismiss handler for the native OAuth popup — unlike the custom button path which has a 60-second safety timeout. If a user opens the native popup and closes it without selecting an account,loadingwill remaintrueindefinitely.Since the custom button is hidden at that point, the stuck
loadingflag is mostly invisible, but it does feed intoisDisabled(line 174), which could matter ifshowNativeButtonis ever toggled back or if future code reads this state.Suggested simplification: skip the re-initialize and drop the loading wrapper
The initialization from
handleClick(line 158) already set the callback. You only needrenderButton:useEffect(() => { if (!showNativeButton || !ready || !nativeButtonRef.current) return; - // Re-initialize to ensure the callback is current - google.accounts.id.initialize({ - client_id: clientId!, - callback: (response: { credential: string }) => { - setLoading(true); - handleCredentialResponse(response); - }, - auto_select: false, - }); - google.accounts.id.renderButton(nativeButtonRef.current, { theme: 'filled_black', size: 'large', width: 300, text: 'continue_with', shape: 'rectangular', }); - }, [showNativeButton, ready, clientId, handleCredentialResponse]); + }, [showNativeButton, ready]);If you do keep the re-initialize (e.g., for the case where the component re-renders and the callback reference changes), at minimum drop the
setLoading(true)wrapper since there's no corresponding reset on popup dismiss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/auth/GoogleLoginButton.tsx` around lines 116 - 137, The useEffect that renders the native Google button is unnecessarily re-calling google.accounts.id.initialize and is wrapping handleCredentialResponse with setLoading(true), which can leave loading stuck if the user dismisses the native popup; update the effect (the one referencing nativeButtonRef, showNativeButton, ready) to skip the initialize call and only call google.accounts.id.renderButton when ready (relying on the initialize done in handleClick), and remove the setLoading(true) wrapper from the callback so the effect calls only renderButton (or if you must re-initialize to refresh the callback, re-initialize without calling setLoading and let handleCredentialResponse manage loading state).
182-209: Native button container renders correctly, but consider resettingshowNativeButtonwhen the component is reused.If
GoogleLoginButtonis rendered in both the login page and theLinkedMethodssettings panel (where the parent mounts/unmounts it), the state resets naturally on remount. However, if the same instance is kept mounted while toggling visibility,showNativeButtonwill remaintruefrom a previous interaction. This is likely fine for the current usage but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/auth/GoogleLoginButton.tsx` around lines 182 - 209, The local state showNativeButton can persist across reuses when the component stays mounted; update GoogleLoginButton to reset showNativeButton to false when the parent toggles visibility or when the component is unmounted/reused by adding an effect that listens for a visibility prop change (or runs a cleanup on unmount) and sets showNativeButton to false, referencing showNativeButton and nativeButtonRef in that effect so the native container is re-initialized correctly when reused.
🤖 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/migrations/1740000000000-AddDeviceApprovals.ts`:
- Line 22: Change the migration column name from "createdAt" to "created_at" in
the AddDeviceApprovals migration and update the DeviceApprovals entity to map
the timestamp to snake_case by using `@CreateDateColumn`({ name: 'created_at' })
(keep the entity property name if desired, but ensure the DB column name is
'created_at'); also update any references that assume the DB column name if
present. This keeps the schema consistent with other columns like user_id,
device_id, expires_at, etc.
---
Nitpick comments:
In `@apps/api/src/auth/controllers/identity.controller.ts`:
- Around line 267-272: The link flow is returning an unused idToken (created by
jwtIssuerService.signIdentityJwt) for dto.intent === 'link', which confuses
clients because wallet linking uses SIWE in linkWalletMethod rather than a JWT
claim; remove the creation/return of idToken in that branch and return only the
fields the client expects (e.g., { userId: '', isNewUser: false }) or set
idToken to undefined, and update LinkMethodDto/response typing and any callers
to reflect that wallet link responses do not include an idToken; ensure
references to jwtIssuerService.signIdentityJwt and linkWalletMethod are adjusted
accordingly.
In `@apps/api/src/migrations/1740000000000-AddDeviceApprovals.ts`:
- Around line 15-16: Add a foreign key on the device_approvals table for the
"user_id" column referencing the users table to prevent orphaned rows; update
the AddDeviceApprovals migration to declare "user_id" as REFERENCES users(id)
(or add ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY (user_id) REFERENCES
users(id) ON DELETE CASCADE) so deletions of users cascade to device_approvals
and maintain referential integrity.
In `@apps/web/src/components/auth/GoogleLoginButton.tsx`:
- Around line 116-137: The useEffect that renders the native Google button is
unnecessarily re-calling google.accounts.id.initialize and is wrapping
handleCredentialResponse with setLoading(true), which can leave loading stuck if
the user dismisses the native popup; update the effect (the one referencing
nativeButtonRef, showNativeButton, ready) to skip the initialize call and only
call google.accounts.id.renderButton when ready (relying on the initialize done
in handleClick), and remove the setLoading(true) wrapper from the callback so
the effect calls only renderButton (or if you must re-initialize to refresh the
callback, re-initialize without calling setLoading and let
handleCredentialResponse manage loading state).
- Around line 182-209: The local state showNativeButton can persist across
reuses when the component stays mounted; update GoogleLoginButton to reset
showNativeButton to false when the parent toggles visibility or when the
component is unmounted/reused by adding an effect that listens for a visibility
prop change (or runs a cleanup on unmount) and sets showNativeButton to false,
referencing showNativeButton and nativeButtonRef in that effect so the native
container is re-initialized correctly when reused.
Remove duplicate handleSiweFlow call from handleConnectorClick — the useEffect already triggers it when wagmi state updates, causing a race where two nonces are fetched and the user signs twice. Also disable wagmi reconnectOnMount since CipherBox disconnects after each SIWE flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/components/auth/WalletLoginButton.tsx (1)
41-46:handleSiweFlowis missing from the effect's dependency array — now more consequential.With the inline call removed from
handleConnectorClick, this effect is the sole SIWE entry point.handleSiweFlowcloses oversignMessageAsync,disconnect, andonLogin, none of which appear in the dep array. If any of those references change between renders (e.g.,onLoginprop identity changes), the effect would invoke a stale closure.The
siweInProgressguard prevents double-fires but doesn't address staleness. Consider stabilising via a ref:♻️ Suggested approach
+ const handleSiweFlowRef = useRef(handleSiweFlow); + useEffect(() => { handleSiweFlowRef.current = handleSiweFlow; }); + useEffect(() => { if (isConnected && address && phase === 'connecting' && !siweInProgress.current) { siweInProgress.current = true; - handleSiweFlow(address); + handleSiweFlowRef.current(address); } }, [isConnected, address, phase]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/auth/WalletLoginButton.tsx` around lines 41 - 46, The effect is calling handleSiweFlow but the function closes over mutable values (signMessageAsync, disconnect, onLogin) that aren't in the dependency array, risking stale closures; fix by stabilising those references or handleSiweFlow itself: either memoize handleSiweFlow with useCallback including signMessageAsync, disconnect and onLogin in its dependency list and then add that callback to the effect deps, or keep a mutable ref (e.g., latestOnLoginRef) for changing props like onLogin and have handleSiweFlow read from those refs so handleSiweFlow can be safely referenced in the useEffect dependency array (or made stable). Ensure siweInProgress remains as the guard and update the useEffect dependencies to include the stable handleSiweFlow (or the memoized dependencies) so the effect never invokes a stale closure.
🤖 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/lib/wagmi/config.ts`:
- Around line 21-24: The createConfig() call currently includes reconnectOnMount
which is not a valid option there; remove the reconnectOnMount property from the
wagmiConfig created by createConfig() and instead pass reconnectOnMount={false}
as a prop on the WagmiProvider component so the provider controls reconnect
behavior; update the wagmiConfig (remove reconnectOnMount) and add
reconnectOnMount={false} to the WagmiProvider usage (reference symbols:
createConfig, wagmiConfig, reconnectOnMount, WagmiProvider).
---
Nitpick comments:
In `@apps/web/src/components/auth/WalletLoginButton.tsx`:
- Around line 41-46: The effect is calling handleSiweFlow but the function
closes over mutable values (signMessageAsync, disconnect, onLogin) that aren't
in the dependency array, risking stale closures; fix by stabilising those
references or handleSiweFlow itself: either memoize handleSiweFlow with
useCallback including signMessageAsync, disconnect and onLogin in its dependency
list and then add that callback to the effect deps, or keep a mutable ref (e.g.,
latestOnLoginRef) for changing props like onLogin and have handleSiweFlow read
from those refs so handleSiweFlow can be safely referenced in the useEffect
dependency array (or made stable). Ensure siweInProgress remains as the guard
and update the useEffect dependencies to include the stable handleSiweFlow (or
the memoized dependencies) so the effect never invokes a stale closure.
When a vault has never published (no uploads yet), resolveIpnsRecord returns null. Previously this threw and retried endlessly, showing "SYNC FAILED" on re-login to an empty vault. Now returns normally so useSyncPolling completes initial sync and shows the empty file browser. Transient failures (network errors, 5xx) still throw and retry as before. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/file-browser/FileBrowser.tsx (1)
255-265: LGTM — comments accurately reflect the contract.The comment on lines 255–257 matches
resolveIpnsRecord's implementation exactly: 404 /!response.success→ null, network errors / 5xx → throw. The comment on lines 260–263 correctly explains the consequence:handleSyncreturning normally (not throwing) letsuseSyncPollingadvanceinitialSyncComplete, surfacing theEmptyStateinstead of spinning forever. One small addition worth considering: the comment currently frames the null path around the initial-sync scenario, but thisreturnfires unconditionally — it also applies during steady-state polling when a null unexpectedly comes back. Adding a one-liner noting the post-initial-sync case ("no record to sync against, skip silently") would make the guard's full intent self-evident.📝 Suggested comment expansion
if (!resolved) { - // No IPNS record found. This is expected for vaults that have - // never published (empty vault, no uploads yet). Return normally - // so useSyncPolling marks initial sync complete and shows the - // empty file browser instead of retrying forever. + // No IPNS record found — two cases: + // 1. Pre-initial-sync: vault has never published (empty vault, no uploads yet). + // Returning normally lets useSyncPolling mark initialSyncComplete and show + // the empty file browser instead of retrying forever. + // 2. Post-initial-sync: record transiently absent; skip silently — next poll + // will retry. (Transient failures throw and are caught by useSyncPolling.) return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/FileBrowser.tsx` around lines 255 - 265, Update the explanatory comment around the resolved-null guard in handleSync: clarify that resolveIpnsRecord returning null is valid both during initial sync (allowing useSyncPolling to set initialSyncComplete and show EmptyState) and during steady-state polling (meaning "no record to sync against, skip silently" — do nothing). Reference resolveIpnsRecord, handleSync, useSyncPolling and initialSyncComplete in the one-line addition so readers understand the guard's intent across both initial and subsequent polls.
🤖 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/components/file-browser/FileBrowser.tsx`:
- Around line 255-265: Update the explanatory comment around the resolved-null
guard in handleSync: clarify that resolveIpnsRecord returning null is valid both
during initial sync (allowing useSyncPolling to set initialSyncComplete and show
EmptyState) and during steady-state polling (meaning "no record to sync against,
skip silently" — do nothing). Reference resolveIpnsRecord, handleSync,
useSyncPolling and initialSyncComplete in the one-line addition so readers
understand the guard's intent across both initial and subsequent polls.
The 4px margin-top gap between trigger and dropdown caused mouseLeave to fire on .user-menu when crossing the gap. Replace with calc() offset and an invisible ::before pseudo-element bridge. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/styles/layout.css (1)
278-280: Add:focus-visiblecounterparts to the:hoverrules in this section.Both
.user-menu-triggerand.user-menu-itemare interactive elements with:hoverstyles but no:focus-visiblestyles. Since this PR is already touching the User Menu block, this is a good opportunity to close the keyboard-navigation gap.♿ Proposed additions for keyboard accessibility
.user-menu-trigger:hover { color: var(--color-text-primary); } + +.user-menu-trigger:focus-visible { + color: var(--color-text-primary); + outline: 1px solid var(--color-border-dim); + outline-offset: 2px; +}.user-menu-item:hover { color: var(--color-text-primary); background-color: var(--color-nav-active); text-decoration: none; } + +.user-menu-item:focus-visible { + color: var(--color-text-primary); + background-color: var(--color-nav-active); + text-decoration: none; + outline: 1px solid var(--color-border-dim); + outline-offset: -1px; +}As per coding guidelines: "Every custom interactive element with
:hoverstyles must also have:focus-visiblestyles for keyboard accessibility."Also applies to: 324-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/styles/layout.css` around lines 278 - 280, Add :focus-visible counterparts for the interactive selectors that currently only have :hover rules: update .user-menu-trigger and .user-menu-item to include matching :focus-visible styles (mirroring the :hover color/visual treatment) so keyboard users receive the same visual feedback; ensure you apply clear focus indicators (outline or ring) consistent with the hover state, keep selector specificity identical (e.g., .user-menu-trigger:focus-visible alongside .user-menu-trigger:hover), and add the same for the .user-menu-item block referenced around the other hover rules.
🤖 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/styles/layout.css`:
- Around line 278-280: Add :focus-visible counterparts for the interactive
selectors that currently only have :hover rules: update .user-menu-trigger and
.user-menu-item to include matching :focus-visible styles (mirroring the :hover
color/visual treatment) so keyboard users receive the same visual feedback;
ensure you apply clear focus indicators (outline or ring) consistent with the
hover state, keep selector specificity identical (e.g.,
.user-menu-trigger:focus-visible alongside .user-menu-trigger:hover), and add
the same for the .user-menu-item block referenced around the other hover rules.
FSM1
changed the title
Move reconnectOnMount from createConfig() (not a valid option in wagmi v3) to WagmiProvider prop. Rename createdAt column to created_at in device_approvals migration and entity to match snake_case convention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
/auth/identity/google,/auth/identity/email/verify-otp,/auth/identity/wallet) acceptintent=linkparameter that verifies credential ownership without creating a new user — fixes cross-account collision when linking the same email used for initial Google logindevice_approvalstable migration for production/staging (synchronize:trueonly created it in dev/test)handleConnectorClickand theuseEffectboth calledhandleSiweFlow, racing with different nonces — now only the effect triggers SIWE. Also disables wagmireconnectOnMountto prevent stale connector errors in Brave.::beforehover bridgeTest plan
device_approvalstable is created by migration🤖 Generated with Claude Code