Skip to content

Wallet password protection#764

Open
dkackman wants to merge 54 commits intoxch-dev:mainfrom
dkackman:password
Open

Wallet password protection#764
dkackman wants to merge 54 commits intoxch-dev:mainfrom
dkackman:password

Conversation

@dkackman
Copy link
Copy Markdown
Collaborator

fix #206

dkackman and others added 27 commits March 15, 2026 13:21
Design for issue xch-dev#206: opt-in per-operation password protection
for wallet secret access, transaction signing, and hardened key
generation. Includes biometric convenience layer design.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
10-task plan covering keychain core changes, API layer modifications,
backend endpoint threading, integration tests, and Tauri/frontend
skeleton. Includes bincode backward compatibility handling.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add password_protected bool field to the Secret variant of KeyData,
with LegacyKeyData enum and backward-compatible bincode deserialization
in from_bytes(). Update all construction sites to set the flag based on
whether a non-empty password was provided. Add is_password_protected()
accessor to Keychain.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add change_password() method to Keychain that decrypts with the old
password and re-encrypts with a new one, updating the password_protected
flag. Add KeyNotFound and NoSecretKey error variants. Include tests for
password changing, wrong password rejection, public key rejection,
flag-on-import behavior, serialization roundtrip, and legacy format
backward compatibility.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add password parameter to sign(), transact(), and transact_with() and
thread it from every endpoint request struct through to the keychain.
Add change_password endpoint and populate has_password on KeyInfo.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace two-step promptIfEnabled + requestPassword auth with unified
requestPassword returning string | null | undefined, where undefined
means cancelled.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bels

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Biometric authentication is a global setting (not per-wallet), so it
belongs in the Preferences section of GlobalSettings rather than the
per-wallet Security section.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dkackman dkackman marked this pull request as draft March 16, 2026 13:58
@dkackman
Copy link
Copy Markdown
Collaborator Author

Code review

Found 2 issues:

  1. Biometric unlock breaks after the first successful useskipKeychainRef.current.add(fingerprint) is called every time the OS keychain returns a stored password (as a "pending validation" marker), but is never cleared when the backend accepts that password. It is only cleared in handleSubmit — the manual-entry path. In practice: the first biometric-authenticated operation succeeds, the flag stays set, and every subsequent operation in the same session skips the keychain and falls back to the password dialog. Biometric effectively works only once per session.

const stored = await keychainGet(keychainKey(fingerprint));
if (stored !== null) {
// Mark as pending validation — if backend rejects, next call skips keychain
skipKeychainRef.current.add(fingerprint);
return stored;
}
// Fall through to password dialog if keychain retrieval fails

  1. Delete confirmation dialog stays open when the password prompt is cancelledsetIsDeleteOpen(false) is only called at the bottom of deleteSelf, after the backend call. The early return on password === undefined (user cancels the password dialog) exits before that line, leaving the delete dialog stuck open. The Details dialog handles this correctly by calling setIsDetailsOpen(false) before the early return.

const deleteSelf = async () => {
const password = await requestPassword(info.has_password);
if (password === undefined) return;
await commands
.deleteKey({ fingerprint: info.fingerprint })
.then(async () => {
await clearKeychainEntry(info.fingerprint);
setKeys(keys.filter((key) => key.fingerprint !== info.fingerprint));
})
.catch(addError);
setIsDeleteOpen(false);
};

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

1. Biometric unlock now works across multiple operations. The skip flag
   is cleared at the start of requestPassword when the previous keychain
   password was accepted (i.e., we're called again without a manual
   dialog entry in between).

2. Delete confirmation dialog now closes when the password prompt is
   cancelled, matching the Details dialog behavior.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The biometric toggle is a global setting (not per-wallet), so it
belongs in GlobalSettings Preferences, not the per-wallet Security
section. Updated spec to match implementation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements wallet password protection (issue #206) across the backend and frontend, including password propagation through signing/secret-access endpoints and a new UI/auth flow that optionally bridges biometrics to OS keychain storage on mobile.

Changes:

  • Add password-aware key management to the Rust keychain and expose has_password + change_password through the API/bindings.
  • Introduce a unified frontend auth gate (PasswordProvider / usePassword) and thread password through sensitive commands (UI + WalletConnect).
  • Add mobile keychain plugin support (Rust + JS) and update settings UI for password/biometric behavior.

Reviewed changes

Copilot reviewed 46 out of 50 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/walletconnect/handler.ts Extends WalletConnect handler context to use password-based auth.
src/walletconnect/commands/offers.ts Prompts for password and forwards it to offer endpoints.
src/walletconnect/commands/high-level.ts Prompts for password and forwards it to signing/tx endpoints.
src/walletconnect/commands/chip0002.ts Prompts for password and forwards it to CHIP-0002 signing endpoints.
src/pages/Settings.tsx Adds password management UI and keychain cleanup when disabling biometrics.
src/pages/Offers.tsx Adds password prompt for cancel-all offers flow.
src/pages/Offer.tsx Adds password prompt for taking an offer.
src/hooks/usePassword.ts Adds hook wrapper for PasswordContext.
src/hooks/useOfferProcessor.ts Adds password prompt and forwards password in offer creation flow.
src/contexts/WalletConnectContext.tsx Uses PasswordContext for WC request auth; passes hasPassword.
src/contexts/PasswordContext.tsx New unified auth gate (password dialog + biometric/keychain bridge).
src/contexts/ErrorContext.tsx Adds toast handling for wrong-password and certain unauthorized cases.
src/components/dialogs/PasswordDialog.tsx New reusable password entry dialog used by PasswordProvider.
src/components/WalletCard.tsx Adds password prompt for delete and secret-key display; clears keychain on delete.
src/components/OfferRowCard.tsx Adds password prompt for cancel-offer action.
src/components/NftCard.tsx Adjusts ARIA attributes on NFT card container.
src/components/ConfirmationDialog.tsx Adds password prompt and forwards password when signing coin spends.
src/bindings.ts Extends request types with optional password; adds changePassword + has_password.
src/App.tsx Adds PasswordProvider to the provider tree.
src-tauri/src/lib.rs Registers change_password command and initializes keychain plugin on mobile.
src-tauri/gen/apple/sage-tauri_iOS/sage-tauri_iOS.entitlements Formatting change in generated iOS entitlements.
src-tauri/gen/apple/sage-tauri_iOS/Info.plist Formatting change in generated iOS Info.plist.
src-tauri/gen/android/app/src/main/AndroidManifest.xml Adds tools namespace/replace entry for Android manifest.
src-tauri/capabilities/mobile.json Enables keychain capability for mobile builds.
src-tauri/Cargo.toml Adds Rust dependency on tauri-plugin-keychain.
pnpm-lock.yaml Adds JS dependency lock entries for tauri-plugin-keychain and related transitive deps.
package.json Adds JS dependency tauri-plugin-keychain.
docs/superpowers/specs/2026-03-15-password-protection-design.md Adds design spec describing password + biometric/keychain bridge.
crates/sage/src/utils/spends.rs Threads password into secret extraction for signing.
crates/sage/src/error.rs Maps new keychain errors to Unauthorized.
crates/sage/src/endpoints/wallet_connect.rs Threads password into WalletConnect signing endpoints.
crates/sage/src/endpoints/transactions.rs Threads password through transact/signing path across tx endpoints.
crates/sage/src/endpoints/offers.rs Threads password through offer signing/cancel flows.
crates/sage/src/endpoints/keys.rs Adds password support for import/secret retrieval; exposes has_password; adds change_password endpoint.
crates/sage/src/endpoints/actions.rs Threads password into hardened derivation flow.
crates/sage/src/endpoints/action_system.rs Threads password into action-system transaction creation flow.
crates/sage-wallet/src/wallet/offer.rs Formatting changes in offer tests (no functional change).
crates/sage-rpc/src/tests.rs Adds RPC-level tests for password protection and change_password.
crates/sage-keychain/src/keychain.rs Adds password_protected flag, legacy deserialization fallback, and change_password implementation + tests.
crates/sage-keychain/src/key_data.rs Adds password_protected to stored secret key data.
crates/sage-keychain/src/error.rs Adds KeyNotFound/NoSecretKey errors.
crates/sage-api/src/types/key_info.rs Adds has_password to KeyInfo type.
crates/sage-api/src/requests/wallet_connect.rs Adds optional password fields to WC signing requests.
crates/sage-api/src/requests/transactions.rs Adds optional password fields to transaction/signing requests.
crates/sage-api/src/requests/offers.rs Adds optional password fields to offer requests.
crates/sage-api/src/requests/keys.rs Adds optional password fields to key import/secret retrieval; adds ChangePassword request/response.
crates/sage-api/src/requests/actions.rs Adds optional password field to IncreaseDerivationIndex.
crates/sage-api/src/requests/action_system.rs Adds optional password field to CreateTransaction.
crates/sage-api/endpoints.json Registers change_password endpoint.
Cargo.lock Adds Rust lock entries for tauri-plugin-keychain.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dkackman and others added 5 commits March 16, 2026 12:39
Wraps the async enable/disable/keychain cleanup in try/catch so
failures don't leave the UI in an inconsistent state.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
requestPassword now takes an optional fingerprint parameter for
operations on non-active wallets (e.g. WalletCard view details /
delete). This ensures keychain lookup and storage use the correct
wallet's fingerprint instead of the currently active one.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The skip/lastKeychainFingerprint mechanism had a bug where stale
keychain passwords would loop indefinitely (skip cleared on every
call, so stale entry retried each time).

Removed both refs entirely. The recovery path for stale keychain
entries is now: cancel the OS biometric prompt → keychain retrieval
fails → password dialog appears → type correct password → handleSubmit
updates keychain. Simpler and correct.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the backend rejects a keychain-retrieved password (decrypt error),
ErrorContext calls notifyDecryptError() which marks the fingerprint as
stale in PasswordContext. The next requestPassword call skips keychain
and shows the dialog directly. When the user types the correct password
via the dialog, handleSubmit clears the stale flag and updates keychain.

This gives the user a recoverable path without needing to know they
should cancel the biometric prompt.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dkackman
Copy link
Copy Markdown
Collaborator Author

Code review

Found 1 issue:

  1. Concurrent requestPassword calls corrupt pendingRef — first caller's promise hangs indefinitelypendingRef is a single useRef<PasswordRequest | null>. If two operations simultaneously reach the password dialog path (Case 4), the second write to pendingRef.current silently overwrites the first. handleSubmit only resolves the second caller. The first caller's Promise is permanently unresolved — that operation hangs with no error, no timeout, and no way to recover. A realistic trigger: a WalletConnect request arrives while the user is also initiating a UI signing operation.

// Case 4: Has password → show dialog (fallback or no biometric)
if (hasPassword) {
return new Promise<string | null | undefined>((resolve) => {
pendingRef.current = { resolve, fingerprint };
setOpen(true);
});
}

The fix is to either (a) queue pending requests instead of replacing them, or (b) reject the in-flight request when a new one arrives (so at least the caller fails fast rather than hanging).

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

dkackman and others added 17 commits March 16, 2026 16:24
Remove all keychain password storage — password wallets always use the
dialog, biometric is a standalone gate for no-password wallets only.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dkackman dkackman marked this pull request as ready for review March 20, 2026 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Password protection

2 participants