feat: add biometric auth for password-gated flows#2182
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughIntegrates biometric authentication across account flows by adding request types, backend biometric handlers and routing, a frontend biometric hook and messaging wrappers, biometric UI controls in PasswordInput, and wiring across create/import/derive/export/remove/manage-password. Also adjusts accountsForget biometric cleanup and updates AGENTS.md. ChangesBiometric Authentication Feature
Sequence DiagramsequenceDiagram
participant Screen as UI Screen
participant Hook as useBiometricAction
participant Msg as messaging.ts
participant Bg as Extension.handle
participant Store as EnrollmentStore
Screen->>Hook: runBiometricAction(action)
Hook->>Screen: isBiometricBusy=true
Hook->>Msg: action(auth from authenticateWithBiometric)
Msg->>Bg: pri(...biometric..., {credentialId, prfOutput, ...})
Bg->>Store: lookup enrollment / decrypt password
Bg-->>Msg: result (boolean|export json|false)
Msg-->>Screen: resolves action result
Hook->>Screen: isBiometricBusy=false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Review Summary by QodoAdd biometric authentication support for password-gated operations
WalkthroughsDescription• Add biometric authentication support for password-gated operations - New RequestBiometricAuthentication type for credential-based auth - Biometric variants for account export, batch export, and password change - Biometric variants for account creation and derivation operations • Implement useBiometricAction hook for unified biometric flow management • Enhance PasswordInput component with fingerprint icon and biometric verification UI • Extend messaging layer with biometric-enabled API functions • Update account removal and password management flows to support biometric auth Diagramflowchart LR
A["User Action<br/>Export/Create/Derive"] --> B["PasswordInput<br/>with Biometric"]
B --> C{Auth Method}
C -->|Password| D["Traditional<br/>Password Flow"]
C -->|Biometric| E["useBiometricAction<br/>Hook"]
E --> F["authenticateWithBiometric<br/>Get PRF Output"]
F --> G["RequestBiometricAuthentication<br/>credentialId + prfOutput"]
D --> H["Extension Handler<br/>Process Request"]
G --> H
H --> I["withBiometricPassword<br/>Decrypt & Execute"]
I --> J["Operation Complete<br/>Export/Create/Derive"]
File Changes1. packages/extension-base/src/background/handlers/Extension.ts
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/partials/RemoveAccount.tsx (1)
134-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't report removal success when
forgetAccountreturnsfalse.
forgetAccountreturns a boolean, but Lines 160-161 notify success and close the modal even when that call fails. In that case the account stays in place while the UI tells the user it was removed.Suggested fix
const success = await forgetAccount(_address); - if (success) { - if (accountsAssets?.balances?.[_address]) { - const latestStoredAssets = await getStorage(STORAGE_KEY.ASSETS, true).catch(() => undefined) as SavedAssets | undefined; - const baseAssets = latestStoredAssets?.balances ? latestStoredAssets : accountsAssets; - const updatedAssets: SavedAssets = { - ...baseAssets, - balances: { ...baseAssets.balances } - }; - - delete updatedAssets.balances[_address]; - setAccountsAssets(updatedAssets); - await setStorage(STORAGE_KEY.ASSETS, updatedAssets, true); - } - - await Promise.allSettled([ - cleanupNotificationAccount(_address), - cleanupAuthorizedAccount(_address) - ]); - - if (willProfileBeEmpty) { - setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.ALL).catch(console.error); - } - } - - notifier(true); - !isExtension && handleClose(); // in full-screen mode, close the modal on success + if (!success) { + throw new Error('Unable to remove account'); + } + + if (accountsAssets?.balances?.[_address]) { + const latestStoredAssets = await getStorage(STORAGE_KEY.ASSETS, true).catch(() => undefined) as SavedAssets | undefined; + const baseAssets = latestStoredAssets?.balances ? latestStoredAssets : accountsAssets; + const updatedAssets: SavedAssets = { + ...baseAssets, + balances: { ...baseAssets.balances } + }; + + delete updatedAssets.balances[_address]; + setAccountsAssets(updatedAssets); + await setStorage(STORAGE_KEY.ASSETS, updatedAssets, true); + } + + await Promise.allSettled([ + cleanupNotificationAccount(_address), + cleanupAuthorizedAccount(_address) + ]); + + if (willProfileBeEmpty) { + setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.ALL).catch(console.error); + } + + notifier(true); + !isExtension && handleClose(); // in full-screen mode, close the modal on success🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension-polkagate/src/partials/RemoveAccount.tsx` around lines 134 - 161, The code calls notifier(true) and handleClose() unconditionally even when forgetAccount(_address) returned false; move the success UI actions inside the success branch: after the existing if (success) { ... } block, call notifier(true) and !isExtension && handleClose() only when success is true (and optionally call notifier(false) on the failure path); update references to forgetAccount, notifier, handleClose, setAccountsAssets, setStorage, cleanupNotificationAccount and cleanupAuthorizedAccount so the UI success/close behavior is gated by the success boolean.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/extension-polkagate/src/components/PasswordInput.tsx`:
- Around line 175-186: The IconButton rendering the biometric action (controlled
by onBiometricClick) is not keyboard-accessible because it explicitly sets
tabIndex={-1}; remove that tabIndex (or set it to default) so the IconButton
(and its onClick handler handleBiometricClick / FingerScan icon) can receive
focus and be activated by keyboard when not disabled (biometricDisabled ||
isBiometricBusy), restoring keyboard access to the biometric path while
preserving existing disabled logic and aria-labels.
- Around line 88-96: The onChange handler in PasswordInput should preserve the
existing cleared semantics by calling onPassChange with an empty string (''),
not null, when the input is cleared—keep onPassChange typed as (pass: string) =>
void and ensure the onChange path (the function using setHasPasswordValue and
setShowPassword) passes '' for empty values so consumers that check falsy
behavior continue to work; also fix keyboard reachability for the biometric
IconButton in PasswordInput by removing tabIndex={-1} (or set it to 0) and
wiring a keyboard handler (e.g., onKeyDown/Enter/Space) that invokes the same
biometric action as the click handler so the IconButton is focusable and
operable via keyboard.
In
`@packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx`:
- Around line 55-67: Clear any previous error before starting the derivation,
and ensure the snackbar is opened for both success and failure paths: at start
of the handler call setError('') to avoid stale messages, then if derivation
succeeds call setError('') (or set a success message) and setShowSnackbar(true);
in the catch block setError(error instanceof Error ? error.message :
String(error)) and also setShowSnackbar(true) so failures are visible. Update
the flow around deriveAccount / deriveAccountWithBiometric (and
maybeChidAccount.suri) to guarantee error state is cleared before attempting and
that setShowSnackbar(true) is invoked on both success and failure.
In
`@packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx`:
- Around line 62-67: The effect that resets parent biometric state on parent
account change (the useEffect that calls setIsProperParentPassword,
setMaybeChidAccount, setParentBiometricAuth, setParentBiometricValidated) must
also clear the stored parent password to avoid validating with stale
credentials; add a call to clear the parent password state (e.g.,
setParentPassword(undefined) or equivalent setter for parentPassword) inside
that same effect so the old password is removed whenever newParentAddress
changes.
In `@packages/extension-polkagate/src/partials/RemoveAccount.tsx`:
- Around line 179-194: The onBiometricValidate callback should catch rejected
biometric promises: wrap the await runBiometricAction(...) call in a try/catch
inside onBiometricValidate (referencing onBiometricValidate and
runBiometricAction) so that any thrown error sets setPasswordError(true) and
returns, instead of letting the rejection bubble; on success continue to call
setBiometricValidated(true) and setPasswordError(false) as currently
implemented.
In
`@packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx`:
- Around line 95-100: The Create account primary button is only disabled based
on password/biometric state which allows a clickable no-op when name is empty;
update the DecisionButtons disabled prop to include name validation (e.g.
disabled={!name || (!password && !isBiometricValidated)}) so the button is
disabled until a non-empty name is provided and authentication is satisfied, and
keep onCreate/onConfirm behavior unchanged.
In
`@packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts`:
- Around line 149-150: The password input is not controlled when biometric mode
clears state via setPassword(undefined), so update the three callers
(SetNameAndPassword, ImportSeed, ImportRawSeed) to render PasswordInput with an
explicit controlled value prop (e.g. value={password ?? ''}) so the UI reflects
the hook state when onBiometricPassword calls setBiometricAuth and setPassword;
locate the useAccountImportOrCreate hook (where setBiometricAuth and setPassword
are called) and the PasswordInput usages in those three components and add
value={password ?? ''} to each PasswordInput.
In
`@packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx`:
- Around line 73-87: Do not set STORAGE_KEY.LAST_PASS_CHANGE until the biometric
password change actually succeeds: move the
setStorage(STORAGE_KEY.LAST_PASS_CHANGE, Date.now()) call to after
accountsChangePasswordAllWithBiometric(...) returns true. In the catch and
failure branches for the biometric flow (where
accountsChangePasswordAllWithBiometric is used), avoid the misleading "Current
password is wrong!" text—use a generic failure message (e.g., "Password change
failed" or "Biometric authentication failed") when calling setSnackbarText, and
keep setting setPasswordError(true), setShowSnackbar(true), and
setMissionSucceeded(false) on failure as before.
---
Outside diff comments:
In `@packages/extension-polkagate/src/partials/RemoveAccount.tsx`:
- Around line 134-161: The code calls notifier(true) and handleClose()
unconditionally even when forgetAccount(_address) returned false; move the
success UI actions inside the success branch: after the existing if (success) {
... } block, call notifier(true) and !isExtension && handleClose() only when
success is true (and optionally call notifier(false) on the failure path);
update references to forgetAccount, notifier, handleClose, setAccountsAssets,
setStorage, cleanupNotificationAccount and cleanupAuthorizedAccount so the UI
success/close behavior is gated by the success boolean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04172fcc-a0a6-46c2-924e-342600039299
📒 Files selected for processing (19)
packages/extension-base/src/background/handlers/Extension.tspackages/extension-base/src/background/types.tspackages/extension-base/src/utils/biometric.tspackages/extension-polkagate/src/components/MySnackbar.tsxpackages/extension-polkagate/src/components/PasswordInput.tsxpackages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsxpackages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsxpackages/extension-polkagate/src/fullscreen/home/DeriveAccount/index.tsxpackages/extension-polkagate/src/hooks/index.tspackages/extension-polkagate/src/hooks/useBiometricAction.tspackages/extension-polkagate/src/messaging.tspackages/extension-polkagate/src/partials/RemoveAccount.tsxpackages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsxpackages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsxpackages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsxpackages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.tspackages/extension-polkagate/src/popup/settings/accountSettings/Export.tsxpackages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsxpackages/extension-polkagate/src/util/biometric.ts
Contributor
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)
packages/extension-polkagate/src/components/MatchPasswordField.tsx (1)
32-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen minimum password length requirement.
The minimum password length of 1 character is insufficient for security. Industry best practices recommend at least 8-12 characters for password-based authentication. This is especially important in the context of this PR, which adds biometric authentication as an alternative—password-based flows should maintain robust security standards.
🔒 Suggested minimum length increase
-const MINIMUM_ALLOWED_PASSWORD_LENGTH = 1; +const MINIMUM_ALLOWED_PASSWORD_LENGTH = 8;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension-polkagate/src/components/MatchPasswordField.tsx` at line 32, The MINIMUM_ALLOWED_PASSWORD_LENGTH constant in MatchPasswordField.tsx is too low; change MINIMUM_ALLOWED_PASSWORD_LENGTH from 1 to a stronger value (e.g., 8 or 12) and update any related validation logic, UI messages, or tests that reference this constant (e.g., password length checks in MatchPasswordField component) so the validation and user feedback reflect the new minimum.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/extension-polkagate/src/components/MatchPasswordField.tsx`:
- Line 32: The MINIMUM_ALLOWED_PASSWORD_LENGTH constant in
MatchPasswordField.tsx is too low; change MINIMUM_ALLOWED_PASSWORD_LENGTH from 1
to a stronger value (e.g., 8 or 12) and update any related validation logic, UI
messages, or tests that reference this constant (e.g., password length checks in
MatchPasswordField component) so the validation and user feedback reflect the
new minimum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eec3d796-6bcb-449b-a230-245e1d4a6f21
📒 Files selected for processing (4)
packages/extension-polkagate/src/components/MatchPasswordField.tsxpackages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.tspackages/extension-polkagate/src/popup/settings/accountSettings/Export.tsxpackages/extension/public/locales/en/translation.json
✅ Files skipped from review due to trivial changes (1)
- packages/extension/public/locales/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
- packages/extension-polkagate/src/popup/settings/accountSettings/Export.tsx
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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 (2)
packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx (2)
62-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
pathErrorwhen the parent account changes.This effect swaps
suriPathto a fresh default for the new parent, but the old derivation-path error survives because the validator effect only sets errors. That leaves a stale error message visible on an otherwise valid default path.Suggested fix
useEffect(() => { setIsProperParentPassword(undefined); // reset password error on maybeChidAccount change setMaybeChidAccount(undefined); setParentBiometricAuth(undefined); setParentBiometricValidated(false); setParentPassword(undefined); + setPathError(''); }, [newParentAddress, setMaybeChidAccount, setParentBiometricValidated, setParentPassword]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx` around lines 62 - 68, The effect that runs on parent change (the useEffect that calls setIsProperParentPassword, setMaybeChidAccount, setParentBiometricAuth, setParentBiometricValidated, setParentPassword) must also clear any stale derivation-path error and reset the path state: call setPathError(undefined) and reset suriPath to the fresh default (via the existing setter for suriPath) inside that same effect so the old pathError doesn’t persist when a new parent address is selected; update the effect to include these two calls alongside the other setters.
90-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop the biometric session when validation is rejected.
If
validateDerivationPathWithBiometric(...)returnsfalse, this branch keepsparentBiometricAuthandisParentBiometricValidatedset. The UI still shows biometric auth as verified and the next click reuses the same rejected payload instead of forcing a fresh biometric prompt.Suggested fix
if (!_account) { + setParentBiometricAuth(undefined); + setParentBiometricValidated(false); setIsProperParentPassword(false); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx` around lines 90 - 97, When validateDerivationPathWithBiometric(parentAddress, suriPath, parentBiometricAuth) returns false, clear the stored biometric session and mark biometric as not validated so the UI forces a fresh prompt; specifically, in the branch inside the isParentBiometricValidated && parentBiometricAuth check where you call setIsProperParentPassword(false) and return, also call the state setters that clear parentBiometricAuth and set isParentBiometricValidated to false (e.g. setParentBiometricAuth(null/undefined) and setIsParentBiometricValidated(false)) so the rejected payload is dropped and the next action triggers a new biometric flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx`:
- Around line 62-68: The effect that runs on parent change (the useEffect that
calls setIsProperParentPassword, setMaybeChidAccount, setParentBiometricAuth,
setParentBiometricValidated, setParentPassword) must also clear any stale
derivation-path error and reset the path state: call setPathError(undefined) and
reset suriPath to the fresh default (via the existing setter for suriPath)
inside that same effect so the old pathError doesn’t persist when a new parent
address is selected; update the effect to include these two calls alongside the
other setters.
- Around line 90-97: When validateDerivationPathWithBiometric(parentAddress,
suriPath, parentBiometricAuth) returns false, clear the stored biometric session
and mark biometric as not validated so the UI forces a fresh prompt;
specifically, in the branch inside the isParentBiometricValidated &&
parentBiometricAuth check where you call setIsProperParentPassword(false) and
return, also call the state setters that clear parentBiometricAuth and set
isParentBiometricValidated to false (e.g. setParentBiometricAuth(null/undefined)
and setIsParentBiometricValidated(false)) so the rejected payload is dropped and
the next action triggers a new biometric flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83a5b77e-bc56-4082-9924-a06239c18a90
📒 Files selected for processing (10)
AGENTS.mdpackages/extension-base/src/background/handlers/Extension.tspackages/extension-polkagate/src/components/PasswordInput.tsxpackages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsxpackages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsxpackages/extension-polkagate/src/partials/RemoveAccount.tsxpackages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsxpackages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsxpackages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsxpackages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx
💤 Files with no reviewable changes (1)
- packages/extension-polkagate/src/components/PasswordInput.tsx
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx
- packages/extension-base/src/background/handlers/Extension.ts
- packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx
- packages/extension-polkagate/src/partials/RemoveAccount.tsx
- packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx
style: soften selected proxy chip in light mode
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.
closes #2164
Summary by CodeRabbit
New Features
UI/Style
Bug Fix
Documentation