feat(security): add Secret type with zeroize-on-drop and PasswordInput component#709
Merged
Merged
Conversation
…t component (#705, #707) Introduce `Secret` wrapper type that zeroes sensitive strings on drop with best-effort mlock via the `region` crate, and a reusable `PasswordInput` component with hold-to-reveal eye icon for masked text entry. - Secret type: zeroize-on-drop, mlock, redacted Debug, constant-time PartialEq, no Display/Deref (explicit expose_secret() access pattern) - PasswordInput: masked by default, eye icon with hold-to-reveal, builder API - Migrate all 12+ password/private-key input screens to use PasswordInput - Fix 5 screens that displayed passwords/keys in plaintext (no masking) - Wrap all WIF display values in Secret for zeroize-on-drop protection - Simplify ScreenWithWalletUnlock trait from 4 methods to 1 - Remove manual .zeroize() calls (Secret handles it automatically) Closes #705 Closes #707 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughIntroduces a memory-protected Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as PasswordInput (UI)
participant SecretState as Secret (memory)
participant Renderer as Renderer
participant Backend as Wallet/Caller
Note over User,Backend: Hold-to-reveal + unlock flow
User->>UI: PointerDown on eye icon
UI->>SecretState: set revealing = true
UI->>Renderer: request repaint (unmasked)
Renderer-->>UI: draw unmasked (expose_secret())
User->>UI: PointerUp / move-away
UI->>SecretState: set revealing = false
UI->>Renderer: request repaint (masked)
User->>UI: Enter or Click Unlock
UI->>Backend: provide password via PasswordInput.text() (exposed)
Backend-->>UI: unlock result (ok / error)
UI->>SecretState: clear() or take_secret() on success/failure
Note over SecretState: Drop/clear zeroizes underlying buffer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a standardized way to handle sensitive user inputs (passwords/private keys) across the UI by adding a Secret wrapper type (zeroize-on-drop + best-effort mlock) and a reusable PasswordInput component with hold-to-reveal masking, then migrating many screens to use them.
Changes:
- Added
model::secret::Secretandui::components::PasswordInput. - Migrated password/private key inputs across wallet and identity flows to be masked by default and zeroized on clear/drop.
- Updated backend identity input plumbing to carry secret key material as
Secretinstead ofString.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/wallets/wallets_screen/mod.rs | Replaces single-key wallet unlock password handling with PasswordInput. |
| src/ui/wallets/wallets_screen/dialogs.rs | Stores derived WIF private keys as Secret and exposes only when needed. |
| src/ui/wallets/single_key_send_screen.rs | Migrates wallet unlock password to PasswordInput. |
| src/ui/wallets/import_mnemonic_screen.rs | Uses PasswordInput for optional mnemonic password and private key input. |
| src/ui/wallets/create_asset_lock_screen.rs | Adapts ScreenWithWalletUnlock implementation to the new PasswordInput API. |
| src/ui/wallets/asset_lock_detail_screen.rs | Migrates unlock and WIF display storage to PasswordInput/Secret. |
| src/ui/wallets/add_new_wallet_screen.rs | Masks optional wallet password input via PasswordInput. |
| src/ui/network_chooser_screen.rs | Masks core RPC password input using PasswordInput. |
| src/ui/identities/keys/key_info_screen.rs | Masks private key input; wraps displayed key material in Secret. |
| src/ui/identities/keys/add_key_screen.rs | Masks private key input with PasswordInput (monospace). |
| src/ui/identities/add_new_identity_screen/mod.rs | Wraps displayed WIF keys in Secret for redacted debug/zeroize-on-drop. |
| src/ui/identities/add_existing_identity_screen.rs | Migrates multiple private-key inputs (including dynamic list) to PasswordInput and passes Secret to backend. |
| src/ui/components/wallet_unlock_popup.rs | Replaces checkbox-based show/hide with hold-to-reveal PasswordInput. |
| src/ui/components/wallet_unlock.rs | Simplifies ScreenWithWalletUnlock trait to provide a PasswordInput. |
| src/ui/components/password_input.rs | Adds hold-to-reveal masked input widget backed by Secret. |
| src/ui/components/mod.rs | Exports password_input module. |
| src/model/secret.rs | Adds Secret type (zeroize-on-drop + best-effort memory lock). |
| src/model/mod.rs | Exports secret module. |
| src/backend_task/identity/mod.rs | Migrates identity loading inputs to use Secret for private keys. |
| docs/ai-design/2026-03-09-password-input/wireframe.html | Adds interactive wireframe for the new input pattern. |
| docs/ai-design/2026-03-09-password-input/ux-spec.md | Adds UX specification for masking + hold-to-reveal behavior. |
| docs/ai-design/2026-03-09-password-input/manual-test-scenarios.md | Adds manual test plan for Secret + PasswordInput changes. |
| Cargo.toml | Adds region dependency for best-effort mlock. |
| Cargo.lock | Locks region and transitive dependencies. |
Comments suppressed due to low confidence (1)
src/backend_task/identity/mod.rs:360
verify_key_inputimmediately copies the secret into a newStringvia.trim().to_string(). That defeats the purpose of moving these inputs toSecret(the copied plaintextStringwon’t be zeroized/locked). Prefer operating on&str(expose_secret().trim()) directly for length checks and parsing, or wrap any required owned copy inSecret/Zeroizingso it is cleared on drop.
fn verify_key_input(
untrimmed_private_key: Secret,
type_key: &str,
) -> Result<Option<[u8; 32]>, String> {
let private_key = untrimmed_private_key.expose_secret().trim().to_string();
match private_key.len() {
64 => {
// hex
match hex::decode(private_key.as_str()) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/ui/wallets/wallets_screen/mod.rs (1)
1848-1924:⚠️ Potential issue | 🟠 MajorDon't refresh after canceling the SK unlock dialog.
close_dialogbecomestruefor both Cancel and successful unlock, but the post-dialog block treats both the same and fires the pending refresh wheneverpending_refresh_after_unlockis set. Canceling the prompt can therefore dispatchRefreshSingleKeyWalletInfoagainst a still-locked wallet.Possible fix
if self.show_sk_unlock_dialog { let mut close_dialog = false; + let mut unlocked = false; egui::Window::new("Unlock Wallet") .collapsible(false) .resizable(false) .show(ctx, |ui| { @@ let unlock_result = wallet.open(self.sk_password_input.text()); match unlock_result { Ok(_) => { + unlocked = true; close_dialog = true; } Err(_) => { MessageBanner::set_global(ui.ctx(), "Incorrect Password", MessageType::Error); } @@ if close_dialog { self.show_sk_unlock_dialog = false; self.sk_password_input.clear(); // Check if we were trying to refresh the SK wallet - if self.pending_refresh_after_unlock { + if unlocked && self.pending_refresh_after_unlock { self.pending_refresh_after_unlock = false; if let Some(wallet_arc) = &self.selected_single_key_wallet { self.refreshing = true; action |= AppAction::BackendTask(BackendTask::CoreTask( CoreTask::RefreshSingleKeyWalletInfo(wallet_arc.clone()), )); } + } else if !unlocked { + self.pending_refresh_after_unlock = false; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/wallets_screen/mod.rs` around lines 1848 - 1924, The post-dialog logic treats close_dialog true for both cancel and successful unlock, causing pending_refresh_after_unlock to trigger even when the user canceled; update the flow so only a successful unlock sets a flag that permits the refresh. Specifically, distinguish cancel vs successful unlock in the Unlock window handling around show_sk_unlock_dialog: set close_dialog = true on Cancel but do NOT clear or act on pending_refresh_after_unlock; on successful unlock (the branch inside attempt_unlock where wallet.open(...) returns Ok) set a new local flag (e.g., unlocked_successfully) or directly clear pending_refresh_after_unlock and perform the refresh dispatch (CoreTask::RefreshSingleKeyWalletInfo) there so the later post-dialog block only runs refresh when unlocked_successfully is true; ensure selected_single_key_wallet and pending_refresh_after_unlock are still referenced the same way.src/ui/identities/keys/key_info_screen.rs (2)
662-670:⚠️ Potential issue | 🟠 MajorUse a generic banner for local-save failures.
update_local_qualified_identity()is a storage/backend operation, but this branch formats the raw error straight into the user-facing banner. That leaks technical details into the UI; show a generic message and attachevia.with_details(e)instead.Possible fix
if let Err(e) = self .app_context .update_local_qualified_identity(&self.identity) { - MessageBanner::set_global( - self.app_context.egui_ctx(), - format!("Issue saving: {}", e), - MessageType::Error, - ); + MessageBanner::set_global( + self.app_context.egui_ctx(), + "Failed to save the private key locally.", + MessageType::Error, + ) + .with_details(e); }As per coding guidelines, "Never expose raw backend/database errors to users. Use a generic user-friendly message in the banner and attach technical details via
BannerHandle::with_details()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 662 - 670, The branch handling update_local_qualified_identity currently logs the raw error into MessageBanner::set_global; change it to show a generic, user-friendly error message via MessageBanner::set_global (e.g., "Failed to save identity") and attach the technical error by calling the banner handle's with_details(e) (use the app_context.egui_ctx() as before). Locate the code around update_local_qualified_identity and replace the format!("Issue saving: {}", e) usage with a generic string and call the banner's with_details(e) to include the raw error for debugging.
435-484:⚠️ Potential issue | 🔴 CriticalDon't reveal the wallet key from the "hidden" branch.
In the
else if self.wallet_openpath,view_private_key_even_if_encrypted_or_in_walletis stillfalse, but this block still derives the key, renders the WIF/hex rows, and cachesself.decrypted_private_key. The first frame after unlock can therefore expose the private key before the user clicks "View Private Key". Keep this branch non-revealing; if signing still needs the key, derive it on demand insign_message()or cache it without rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 435 - 484, The branch in the wallet_open path is deriving, rendering, and caching the private key even when view_private_key_even_if_encrypted_or_in_wallet is false; stop exposing the key by not deriving or rendering (and not setting self.decrypted_private_key) unless the explicit view flag is true. Change the block guarded by self.wallet_open so it only shows the "View Private Key" button and sets view_wallet_unlock/view_private_key_even_if_encrypted_or_in_wallet as before, but do not call selected_wallet.as_ref().unwrap().read().unwrap().private_key_at_derivation_path, do not construct the WIF/hex UI grid, and do not assign self.decrypted_private_key there; if signing requires the key derive it on demand inside sign_message() or derive-and-cache it into decrypted_private_key only in the code path where view_private_key_even_if_encrypted_or_in_wallet is true (or in a non-UI signing helper) so the UI never leaks the key on the first frame after unlock.src/ui/identities/add_existing_identity_screen.rs (1)
937-949:⚠️ Potential issue | 🟡 MinorClear the payout key when filling a regular masternode.
fill_random_masternode()updates voting/owner only. If the user previously filled an HPMN, the old payout key stays inpayout_address_private_key_inputand will be carried into the next load attempt.Suggested fix
self.voting_private_key_input .set_text(masternode.voter.private_key.clone()); self.owner_private_key_input .set_text(masternode.owner.private_key.clone()); + self.payout_address_private_key_input.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/add_existing_identity_screen.rs` around lines 937 - 949, When filling a random masternode in fill_random_masternode(), also clear the HPMN payout key field so an old payout key doesn't carry over; locate fill_random_masternode and add a step to reset payout_address_private_key_input (e.g., call its clear/set_text with empty string) right after setting identity_type = IdentityType::Masternode (or after setting owner/voter keys) so the UI state reflects that masternodes do not use the payout key.src/ui/components/wallet_unlock.rs (1)
69-90:⚠️ Potential issue | 🟠 MajorAvoid creating an unzeroized password copy.
The popup implementation passes
self.password_input.text()directly towallet_seed.open(). Theto_string()call here creates aStringallocation that is never zeroized, only cleared from the widget. Sincewallet_seed.open()accepts&str, pass the reference directly instead.Suggested fix
- let password_text = self.password_input().text().to_string(); - - let unlock_result = wallet.wallet_seed.open(&password_text); + let password_input = self.password_input(); + let unlock_result = wallet.wallet_seed.open(password_input.text()); @@ - self.password_input().clear(); + password_input.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/wallet_unlock.rs` around lines 69 - 90, The code creates an owned String via self.password_input().text().to_string(), which allocates a non-zeroized copy of the password; instead, use the &str returned by password_input().text() and pass that directly to wallet.wallet_seed.open() (i.e., remove the to_string()) so no owned String is allocated, keep calling self.password_input().clear() afterwards to remove the password from the widget; locate this change around the password handling in the unlock flow using password_input(), wallet_seed.open(), and self.password_input().clear().
🧹 Nitpick comments (6)
src/model/secret.rs (1)
57-60: Consider documenting the reallocation risk forexpose_secret_mut().When binding to egui's
TextEdit, if the user types beyond the pre-allocated capacity (128 bytes by default),Stringwill reallocate. This leaves old secret data in freed pages that were previously locked but are now un-zeroized. The lock guard also becomes stale (pointing to the old address).The current 128-byte pre-allocation mitigates this for typical passwords, but very long inputs or repeated push operations could trigger reallocation. Consider adding a doc comment warning about this limitation.
📝 Proposed documentation enhancement
/// Mutably borrow the backing `String` (needed for egui `TextEdit` binding). + /// + /// # Security Note + /// + /// If appending causes reallocation beyond the pre-locked capacity, old data + /// may remain in freed memory without zeroization. The default 128-byte + /// pre-allocation should suffice for typical passwords. pub fn expose_secret_mut(&mut self) -> &mut String { &mut self.inner }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/secret.rs` around lines 57 - 60, The expose_secret_mut(&mut self) -> &mut String method can trigger reallocation (and thus leave old secret bytes on freed pages and invalidate any lock guard) if the String grows beyond its reserved capacity (128 bytes); update the doc comment for expose_secret_mut (and mention self.inner / the 128-byte pre-allocation) to warn callers about this reallocation risk and that the lock becomes stale and old memory may not be zeroed, and suggest safe alternatives (e.g., pre-reserve sufficient capacity before binding, avoid pushing past capacity, or provide a separate locked byte-slice API) so callers using egui::TextEdit understand the limitation.src/backend_task/identity/mod.rs (1)
352-356: Minor: Temporary plaintext string created from Secret.The trimmed copy on line 356 creates a plain
Stringthat won't be zeroized. This is acceptable for short-lived backend processing, but worth noting that the protection is primarily for transport/storage rather than in-function processing.If you want to extend zeroization to this processing step in the future, consider wrapping the intermediate value:
let private_key = Zeroizing::new(untrimmed_private_key.expose_secret().trim().to_string());This would require importing
zeroize::Zeroizingdirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/mod.rs` around lines 352 - 356, The function verify_key_input creates a temporary plain String from Secret (untrimmed_private_key.expose_secret().trim().to_string()) which is not zeroized; to fix, wrap the intermediate trimmed string in zeroize::Zeroizing so the sensitive buffer is zeroed when dropped (change the local private_key to use Zeroizing::new(...trim().to_string()) and add use zeroize::Zeroizing at the top), or alternatively document and justify the deliberate tradeoff if you choose to leave it as-is; update references in verify_key_input accordingly.src/ui/identities/keys/key_info_screen.rs (1)
655-661: Clear the imported key fromprivate_key_inputonce it's stored.After
self.private_key_datais set, the input field is no longer shown, but it still retains a second plaintext copy until the screen is dropped. Clearing it here shortens the secret lifetime without changing the flow.Possible fix
} else if validation_result.unwrap() { // If valid, store the private key in the context and reset the input field self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None)); + self.private_key_input.clear(); self.identity.private_keys.insert_non_encrypted( (self.key.purpose().into(), self.key.id()), (self.key.clone().into(), private_key_bytes), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 655 - 661, After successfully validating and storing the imported private key in self.private_key_data and self.identity.private_keys (inside the branch where validation_result.unwrap() is true), clear the temporary plaintext held in the input widget by resetting/clearing self.private_key_input so the second plaintext copy is dropped; locate the logic around self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None)) and the subsequent self.identity.private_keys.insert_non_encrypted((self.key.purpose().into(), self.key.id()), (self.key.clone().into(), private_key_bytes)) and add a call to clear or set self.private_key_input to an empty string/None immediately after storing to shorten the secret lifetime.docs/ai-design/2026-03-09-password-input/wireframe.html (1)
684-744: Use one shared document-level release handler.Each eye-button bind adds its own
document.mouseuplistener, andaddKey()adds another one every time. After a few add/remove cycles the wireframe keeps accumulating global handlers for detached elements.Also applies to: 749-776, 788-832
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-03-09-password-input/wireframe.html` around lines 684 - 744, Multiple per-button document-level mouseup handlers are being registered (see the per-button document.addEventListener('mouseup', ...) and similar handlers added by addKey()), causing leaked global listeners; change to a single shared document-level release handler: remove the per-instance document.addEventListener('mouseup', ...) inside the loop and instead register one top-level mouseup listener that calls a shared mask-all function (or iterates visible/attached .password-input-wrapper elements) to call mask() behavior for all inputs, and ensure addKey() registers only the same shared handler (idempotently) or attaches/removes a single reference so multiple add/remove cycles do not accumulate handlers.src/ui/wallets/add_new_wallet_screen.rs (1)
152-170: Clear the password after a successful save.The migration masks the field, but
password_inputstill stays populated after wallet creation, so the success screen keeps the plaintext password alive in memory for the lifetime of this screen instance. Clearing it on the success path would tighten the security boundary without hurting retry UX on failure.Suggested fix
self.created_wallet_seed_hash = Some(new_wallet_seed_hash); + self.password_input.clear(); self.wallet_created = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/add_new_wallet_screen.rs` around lines 152 - 170, In save_wallet, after the successful wallet creation path (i.e., after encrypting/storing and right before returning Ok(AppAction...)), clear the in-memory password field to remove plaintext from this screen instance: call the appropriate method on self.password_input to zero/clear its contents (e.g., self.password_input.clear() or reset its text) only on the success path; ensure this happens after any use of self.password_input (e.g., after ClosedKeyItem::encrypt_seed, encrypt_message, and app_context.update_main_password) and does not run on error paths so retries still work.src/ui/wallets/asset_lock_detail_screen.rs (1)
362-366: Clipboard copy creates unprotected plaintext—acceptable tradeoff but worth documenting.The
to_string()call creates a regularStringthat won't be zeroized on drop. Additionally, the system clipboard will retain the WIF until overwritten. This is an inherent tradeoff of clipboard functionality when the user explicitly requests a copy.Consider adding a brief code comment noting this security tradeoff, so future maintainers understand the conscious decision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/asset_lock_detail_screen.rs` around lines 362 - 366, Add a brief inline comment at the wif_copy assignment (where wif.expose_secret().to_string() is used) documenting the security tradeoff: converting the secret to an unprotected String means it won't be zeroized on drop and placing it on the system clipboard will leave the WIF until overwritten by the OS/clipboard manager; mention this is an explicit user-driven decision to enable "Copy" and note potential alternatives (e.g., secure clipboard APIs or clearing clipboard) for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ai-design/2026-03-09-password-input/ux-spec.md`:
- Around line 183-279: The spec's API for PasswordInput is out of sync with the
shipped implementation: update this spec to match the actual symbols and types
in src/ui/components/password_input.rs by removing or fixing references to the
old String-backed API (fields like text, revealing), builder methods that don't
exist (with_label), trait types that aren't used (ComponentResponse,
InnerResponse), and response fields not returned by the real show()
implementation; instead, document the real constructor, actual public methods
(e.g., the real new()/builder names present in password_input.rs), the exact
show() signature and return type used in the file, how external error injection
is performed with the concrete method names (e.g., set_error if present or the
real setter), and the true text accessors (actual method names for
reading/clearing) so the spec matches the shipped code.
In `@src/ui/components/password_input.rs`:
- Around line 153-186: The repaint is only requested while self.revealing is
true, causing the TextEdit::password(!self.revealing) to sometimes render
plaintext for an extra frame after release; update the reveal handling in the
eye button code (where self.revealing is set from
eye_response.is_pointer_button_down_on() && eye_response.hovered()) to request a
UI repaint whenever the reveal state changes (on both transitions true→false and
false→true) instead of only while self.revealing is true—e.g., detect the
previous reveal value, compare to the new value, assign self.revealing, and call
ui.ctx().request_repaint() when they differ so the password field
(TextEdit::password(!self.revealing)) is re-rendered immediately.
In `@src/ui/components/wallet_unlock_popup.rs`:
- Around line 128-140: The current focus logic continuously calls
request_focus() while self.password_input.is_empty(), causing focus to snap
back; change this to request focus only once when the popup opens by adding a
boolean flag (e.g., needs_focus) on the popup struct that is set in open() and
cleared after the first focus request, then modify the pw_response block (around
password_input.show(ui) and pw_response.response.request_focus()) to check
needs_focus instead of is_empty() and clear needs_focus after calling
request_focus(); alternatively, detect widget creation via
pw_response.response.gained_focus_only() if available and only call
request_focus() in that single-frame case.
In `@src/ui/identities/add_existing_identity_screen.rs`:
- Around line 983-993: The code reinitializes self.keys_input to three
PasswordInput::new() entries after a successful load, which differs from the
constructor's empty manual-key state and can leave placeholder rows; update the
post-success reset to restore the constructor state by setting self.keys_input
to an empty vector (e.g., vec![] or Vec::new()) instead of recreating three
PasswordInput::new() entries so the form has no manual key inputs unless the
user explicitly adds them; locate the assignment to self.keys_input (triggered
by the "Load Another" flow) and replace the three-item vec with an empty one.
In `@src/ui/wallets/asset_lock_detail_screen.rs`:
- Around line 354-358: The code creates an unprotected temporary String via
wif.expose_secret().to_owned().as_str(), exposing the secret in memory; instead,
avoid allocating/copying by taking an immutable view into the secret and passing
a mutable reference to that &str to TextEdit::multiline (e.g., bind let mut
wif_view: &str = wif.expose_secret(); and pass &mut wif_view to
TextEdit::multiline), remove the to_owned()/String copy and don’t mutate the
secret, and mark the TextEdit as read-only (use
TextEdit::multiline(...).interactive(false) or the equivalent non-editable
setting) so the secret is displayed without creating an unzeroized plaintext
copy; update the usage around private_key_wif, TextEdit::multiline, and
expose_secret accordingly.
In `@src/ui/wallets/import_mnemonic_screen.rs`:
- Around line 103-112: The code currently clones the masked PasswordInput into
an owned String before parsing, defeating the Secret boundary and causing
allocations on each keystroke; instead borrow the trimmed &str from
self.private_key_input.text().trim() and pass that &str to
SingleKeyWallet::from_wif and SingleKeyWallet::from_hex (both accept &str), e.g.
bind let input = self.private_key_input.text().trim(); then call
SingleKeyWallet::from_wif(input, ...) .or_else(|_|
SingleKeyWallet::from_hex(input, self.app_context.network, ...)) so you avoid
heap allocation and keep the secret unowned.
---
Outside diff comments:
In `@src/ui/components/wallet_unlock.rs`:
- Around line 69-90: The code creates an owned String via
self.password_input().text().to_string(), which allocates a non-zeroized copy of
the password; instead, use the &str returned by password_input().text() and pass
that directly to wallet.wallet_seed.open() (i.e., remove the to_string()) so no
owned String is allocated, keep calling self.password_input().clear() afterwards
to remove the password from the widget; locate this change around the password
handling in the unlock flow using password_input(), wallet_seed.open(), and
self.password_input().clear().
In `@src/ui/identities/add_existing_identity_screen.rs`:
- Around line 937-949: When filling a random masternode in
fill_random_masternode(), also clear the HPMN payout key field so an old payout
key doesn't carry over; locate fill_random_masternode and add a step to reset
payout_address_private_key_input (e.g., call its clear/set_text with empty
string) right after setting identity_type = IdentityType::Masternode (or after
setting owner/voter keys) so the UI state reflects that masternodes do not use
the payout key.
In `@src/ui/identities/keys/key_info_screen.rs`:
- Around line 662-670: The branch handling update_local_qualified_identity
currently logs the raw error into MessageBanner::set_global; change it to show a
generic, user-friendly error message via MessageBanner::set_global (e.g.,
"Failed to save identity") and attach the technical error by calling the banner
handle's with_details(e) (use the app_context.egui_ctx() as before). Locate the
code around update_local_qualified_identity and replace the format!("Issue
saving: {}", e) usage with a generic string and call the banner's
with_details(e) to include the raw error for debugging.
- Around line 435-484: The branch in the wallet_open path is deriving,
rendering, and caching the private key even when
view_private_key_even_if_encrypted_or_in_wallet is false; stop exposing the key
by not deriving or rendering (and not setting self.decrypted_private_key) unless
the explicit view flag is true. Change the block guarded by self.wallet_open so
it only shows the "View Private Key" button and sets
view_wallet_unlock/view_private_key_even_if_encrypted_or_in_wallet as before,
but do not call
selected_wallet.as_ref().unwrap().read().unwrap().private_key_at_derivation_path,
do not construct the WIF/hex UI grid, and do not assign
self.decrypted_private_key there; if signing requires the key derive it on
demand inside sign_message() or derive-and-cache it into decrypted_private_key
only in the code path where view_private_key_even_if_encrypted_or_in_wallet is
true (or in a non-UI signing helper) so the UI never leaks the key on the first
frame after unlock.
In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 1848-1924: The post-dialog logic treats close_dialog true for both
cancel and successful unlock, causing pending_refresh_after_unlock to trigger
even when the user canceled; update the flow so only a successful unlock sets a
flag that permits the refresh. Specifically, distinguish cancel vs successful
unlock in the Unlock window handling around show_sk_unlock_dialog: set
close_dialog = true on Cancel but do NOT clear or act on
pending_refresh_after_unlock; on successful unlock (the branch inside
attempt_unlock where wallet.open(...) returns Ok) set a new local flag (e.g.,
unlocked_successfully) or directly clear pending_refresh_after_unlock and
perform the refresh dispatch (CoreTask::RefreshSingleKeyWalletInfo) there so the
later post-dialog block only runs refresh when unlocked_successfully is true;
ensure selected_single_key_wallet and pending_refresh_after_unlock are still
referenced the same way.
---
Nitpick comments:
In `@docs/ai-design/2026-03-09-password-input/wireframe.html`:
- Around line 684-744: Multiple per-button document-level mouseup handlers are
being registered (see the per-button document.addEventListener('mouseup', ...)
and similar handlers added by addKey()), causing leaked global listeners; change
to a single shared document-level release handler: remove the per-instance
document.addEventListener('mouseup', ...) inside the loop and instead register
one top-level mouseup listener that calls a shared mask-all function (or
iterates visible/attached .password-input-wrapper elements) to call mask()
behavior for all inputs, and ensure addKey() registers only the same shared
handler (idempotently) or attaches/removes a single reference so multiple
add/remove cycles do not accumulate handlers.
In `@src/backend_task/identity/mod.rs`:
- Around line 352-356: The function verify_key_input creates a temporary plain
String from Secret (untrimmed_private_key.expose_secret().trim().to_string())
which is not zeroized; to fix, wrap the intermediate trimmed string in
zeroize::Zeroizing so the sensitive buffer is zeroed when dropped (change the
local private_key to use Zeroizing::new(...trim().to_string()) and add use
zeroize::Zeroizing at the top), or alternatively document and justify the
deliberate tradeoff if you choose to leave it as-is; update references in
verify_key_input accordingly.
In `@src/model/secret.rs`:
- Around line 57-60: The expose_secret_mut(&mut self) -> &mut String method can
trigger reallocation (and thus leave old secret bytes on freed pages and
invalidate any lock guard) if the String grows beyond its reserved capacity (128
bytes); update the doc comment for expose_secret_mut (and mention self.inner /
the 128-byte pre-allocation) to warn callers about this reallocation risk and
that the lock becomes stale and old memory may not be zeroed, and suggest safe
alternatives (e.g., pre-reserve sufficient capacity before binding, avoid
pushing past capacity, or provide a separate locked byte-slice API) so callers
using egui::TextEdit understand the limitation.
In `@src/ui/identities/keys/key_info_screen.rs`:
- Around line 655-661: After successfully validating and storing the imported
private key in self.private_key_data and self.identity.private_keys (inside the
branch where validation_result.unwrap() is true), clear the temporary plaintext
held in the input widget by resetting/clearing self.private_key_input so the
second plaintext copy is dropped; locate the logic around self.private_key_data
= Some((PrivateKeyData::Clear(private_key_bytes), None)) and the subsequent
self.identity.private_keys.insert_non_encrypted((self.key.purpose().into(),
self.key.id()), (self.key.clone().into(), private_key_bytes)) and add a call to
clear or set self.private_key_input to an empty string/None immediately after
storing to shorten the secret lifetime.
In `@src/ui/wallets/add_new_wallet_screen.rs`:
- Around line 152-170: In save_wallet, after the successful wallet creation path
(i.e., after encrypting/storing and right before returning Ok(AppAction...)),
clear the in-memory password field to remove plaintext from this screen
instance: call the appropriate method on self.password_input to zero/clear its
contents (e.g., self.password_input.clear() or reset its text) only on the
success path; ensure this happens after any use of self.password_input (e.g.,
after ClosedKeyItem::encrypt_seed, encrypt_message, and
app_context.update_main_password) and does not run on error paths so retries
still work.
In `@src/ui/wallets/asset_lock_detail_screen.rs`:
- Around line 362-366: Add a brief inline comment at the wif_copy assignment
(where wif.expose_secret().to_string() is used) documenting the security
tradeoff: converting the secret to an unprotected String means it won't be
zeroized on drop and placing it on the system clipboard will leave the WIF until
overwritten by the OS/clipboard manager; mention this is an explicit user-driven
decision to enable "Copy" and note potential alternatives (e.g., secure
clipboard APIs or clearing clipboard) for future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27ca1c58-f999-41ad-9b79-410e39605c09
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomldocs/ai-design/2026-03-09-password-input/manual-test-scenarios.mddocs/ai-design/2026-03-09-password-input/ux-spec.mddocs/ai-design/2026-03-09-password-input/wireframe.htmlsrc/backend_task/identity/mod.rssrc/model/mod.rssrc/model/secret.rssrc/ui/components/mod.rssrc/ui/components/password_input.rssrc/ui/components/wallet_unlock.rssrc/ui/components/wallet_unlock_popup.rssrc/ui/identities/add_existing_identity_screen.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/keys/add_key_screen.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/network_chooser_screen.rssrc/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/asset_lock_detail_screen.rssrc/ui/wallets/create_asset_lock_screen.rssrc/ui/wallets/import_mnemonic_screen.rssrc/ui/wallets/single_key_send_screen.rssrc/ui/wallets/wallets_screen/dialogs.rssrc/ui/wallets/wallets_screen/mod.rs
- Implement egui::TextBuffer for Secret, eliminating expose_secret_mut() escape hatch — all text mutations now go through controlled methods that detect pointer changes and re-mlock after reallocation - Track heap pointer via locked_ptr field; relock_if_moved() re-locks the new buffer when String reallocates during insert_text/replace_with - Fix Clone and From<&str> to pre-allocate with correct capacity, avoiding intermediate unprotected String copies - Add tracing::debug! on mlock failure (was previously silent) - Update PartialEq doc to honestly describe length-leak limitation - Zeroize old content in TextBuffer::clear() and replace_with() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…and-password-input # Conflicts: # src/ui/components/wallet_unlock_popup.rs # src/ui/identities/keys/key_info_screen.rs # src/ui/wallets/asset_lock_detail_screen.rs # src/ui/wallets/import_mnemonic_screen.rs # src/ui/wallets/wallets_screen/dialogs.rs # src/ui/wallets/wallets_screen/mod.rs
Replace Zeroizing<String> with Secret for private key WIF values in address_table, dialogs, and mod.rs. Remove stale TODO about PasswordInput. Add missing dark_mode binding in SK unlock dialog. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- SEC-001: Disable egui Undoer (max_undos=0) in PasswordInput to prevent plaintext copies in undo history - SEC-003: Zeroize source string in new() before freeing old allocation - SEC-004: Pre-allocate full page (4096 bytes) to prevent reallocations - SEC-005: Zero trailing capacity after delete_char_range - SEC-006: Custom Drop zeroes full 0..capacity (not just 0..len) - SEC-007: Document .password(true) requirement in Secret doc comment - CODE-001: Enforce PAGE_SIZE minimum in with_capacity() - CODE-004: Deduplicate From<&str> and Clone to delegate to new() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ity, leak prevention) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/identities/keys/key_info_screen.rs (1)
444-500:⚠️ Potential issue | 🟠 MajorKeep the wallet-open branch from revealing the key by default.
Once
self.wallet_openbecomes true, this branch still fetches and renders the WIF/hex even whenview_private_key_even_if_encrypted_or_in_walletis false. Unlocking only to sign a message or inspect the screen will therefore expose the private key on-screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 444 - 500, The wallet-open branch currently fetches and renders the private key unconditionally when self.wallet_open is true; change it so the code only calls selected_wallet.read().unwrap().private_key_at_derivation_path(...) and renders the WIF/hex grid when either self.view_private_key_even_if_encrypted_or_in_wallet is true or self.decrypted_private_key.is_some(); otherwise do not attempt to fetch or display the key and keep self.decrypted_private_key as None. Constrain the UI button handling (view_private_key_even_if_encrypted_or_in_wallet / view_wallet_unlock) to trigger the fetch/display path, and ensure the match on private_key_at_derivation_path is only executed inside that guarded branch so the key is not exposed by default.
♻️ Duplicate comments (4)
src/ui/components/wallet_unlock_popup.rs (1)
128-133:⚠️ Potential issue | 🟡 MinorRequest focus only once per popup open.
As long as the field stays empty, this path calls
request_focus()every frame. That can steal focus back from the action buttons and make the dialog feel sticky.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/wallet_unlock_popup.rs` around lines 128 - 133, The password field is calling request_focus() every frame while empty which steals focus; modify the WalletUnlockPopup state (e.g., add a boolean like focus_requested) and only call self.password_input.request_focus() once per popup open: when showing the field in password_input.show(ui) check pw_response.response.gained_focus() || (self.password_input.is_empty() && !self.focus_requested), call request_focus() and then set self.focus_requested = true; ensure focus_requested is reset to false when the popup is opened/initialized so the behavior repeats correctly on subsequent opens.src/ui/components/password_input.rs (1)
167-200:⚠️ Potential issue | 🟠 MajorRequest repaint on both reveal-state transitions.
The field is rendered before
self.revealingis updated. When the pointer is released, the current frame can still paint plaintext, and the current code only forces another frame whileself.revealingstays true.Suggested fix
- // Update for NEXT frame. - self.revealing = eye_response.is_pointer_button_down_on() && eye_response.hovered(); + let next_revealing = + eye_response.is_pointer_button_down_on() && eye_response.hovered(); + let reveal_state_changed = next_revealing != self.revealing; + self.revealing = next_revealing; @@ - if self.revealing { + if self.revealing || reveal_state_changed { ui.ctx().request_repaint(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/password_input.rs` around lines 167 - 200, The code only requests a repaint while self.revealing is true, so release transitions can leave plaintext visible for the current frame; fix by detecting when the reveal state changes and requesting a repaint on both transitions: capture let old_revealing = self.revealing, update self.revealing = eye_response.is_pointer_button_down_on() && eye_response.hovered(), then if old_revealing != self.revealing call ui.ctx().request_repaint(); reference self.revealing, eye_response.is_pointer_button_down_on(), eye_response.hovered(), and ui.ctx().request_repaint() to locate where to add this change.src/ui/wallets/import_mnemonic_screen.rs (1)
102-112:⚠️ Potential issue | 🟠 MajorAvoid cloning the private key into ordinary
Strings here.Both the live-parse path and the save path allocate heap-owned copies of the masked secret. That punches through the
Secretboundary and adds unnecessary work on every keystroke.Suggested fix
- let input = self.private_key_input.text().trim().to_string(); + let input = self.private_key_input.text().trim(); @@ - let result = SingleKeyWallet::from_wif(&input, None, None) - .or_else(|_| SingleKeyWallet::from_hex(&input, self.app_context.network, None, None)); + let result = SingleKeyWallet::from_wif(input, None, None) + .or_else(|_| SingleKeyWallet::from_hex(input, self.app_context.network, None, None)); @@ - let input = self.private_key_input.text().trim().to_string(); + let input = self.private_key_input.text().trim(); @@ - let mut wallet = - SingleKeyWallet::from_wif(&input, password, alias.clone()).or_else(|_| { - SingleKeyWallet::from_hex(&input, self.app_context.network, password, alias) + let mut wallet = + SingleKeyWallet::from_wif(input, password, alias.clone()).or_else(|_| { + SingleKeyWallet::from_hex(input, self.app_context.network, password, alias) })?;Also applies to: 126-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/import_mnemonic_screen.rs` around lines 102 - 112, The code in try_parse_private_key is creating owned String copies of the masked private key each keystroke (private_key_input.text().trim().to_string()), then passing those owned Strings into SingleKeyWallet::from_wif/from_hex and later save paths (parsed_single_key_wallet handling), which breaks the Secret boundary and causes extra allocations; change to keep a borrowed &str (e.g., let input = self.private_key_input.text().trim();) and pass that &str to SingleKeyWallet::from_wif and ::from_hex so no temporary owned String is allocated, and ensure the save path (where parsed_single_key_wallet is stored) stores the Secret type or moves only the Secret container rather than cloning the underlying String; update call sites to accept &str if needed and avoid converting to owned String in try_parse_private_key and the save path.src/ui/wallets/asset_lock_detail_screen.rs (1)
354-358:⚠️ Potential issue | 🟠 MajorDon't create a throwaway
Stringjust to render the WIF.
to_owned()materializes an unprotected heap copy of the private key every frame the popup is visible. A borrowed&strview is enough for read-only display.Suggested fix
ui.label("Private Key (WIF):"); if let Some(ref wif) = self.private_key_wif { - ui.add(egui::TextEdit::multiline(&mut wif.expose_secret().to_owned().as_str()) + let mut wif_view = wif.expose_secret(); + ui.add(egui::TextEdit::multiline(&mut wif_view) .font(egui::FontId::monospace(12.0)) .desired_width(f32::INFINITY) .desired_rows(1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/asset_lock_detail_screen.rs` around lines 354 - 358, The code creates an owned String each frame via to_owned() just to render the WIF; instead, avoid heap-copying the secret and render a borrowed &str for read-only display. Replace the egui::TextEdit::multiline(...) call (which currently uses self.private_key_wif / wif.expose_secret().to_owned().as_str()) with a read-only widget such as egui::Label::new(wif.expose_secret().as_str()).monospace() (or Label with FontId::monospace) so you only borrow the secret string from self.private_key_wif and stop materializing an owned String every frame. Ensure you reference self.private_key_wif and wif.expose_secret() when making the change.
🧹 Nitpick comments (1)
src/ui/components/password_input.rs (1)
7-12: Implement the shared response trait onPasswordInputResponse.This is now a reusable component response type, but it doesn't implement
ComponentResponse, so it won't plug into the same generic component flow as the rest ofsrc/ui/components. As per coding guidelines,src/ui/components/**/*.rs: UI components should follow a lazy initialization pattern: domain data stored separately from UI components, builder methods for configuration, response struct withComponentResponsetrait, self-contained validation and error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/password_input.rs` around lines 7 - 12, Implement the ComponentResponse trait for PasswordInputResponse: add an impl ComponentResponse for PasswordInputResponse block that delegates required trait methods to the inner response field (Response) and exposes the changed flag appropriately; ensure methods like into_response()/as_response()/is_changed() (or the actual method names defined by ComponentResponse in your codebase) forward to self.response and return/consider self.changed, and include any component-specific validation/error handling within those trait methods so PasswordInputResponse plugs into the generic component flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/secret.rs`:
- Around line 99-110: The relock_if_moved logic only checks pointer changes and
misses in-place capacity growth; update relock_if_moved to compare both
self.inner.as_ptr() and self.inner.capacity() against tracked values (e.g.
locked_ptr and a new locked_capacity field) and when either differs, re-lock the
buffer with region::lock(self.inner.as_ptr(), self.inner.capacity()), store the
resulting guard into self._lock (overwriting/dropping the old guard) and update
both locked_ptr and locked_capacity to the current pointer and capacity so the
newly grown tail is properly mlocked.
- Around line 45-58: In Secret::new, the plaintext is copied into buf before
applying mlock (region::lock), exposing secret data in unprotected memory;
change the sequence to allocate buf with with_capacity, call region::lock on
buf.as_ptr()/buf.capacity() to mlock the destination buffer first, then
push_str(&source) to copy the secret into the now-locked buf, then zeroize
source and set locked_ptr accordingly (ensure error handling around region::lock
remains intact).
---
Outside diff comments:
In `@src/ui/identities/keys/key_info_screen.rs`:
- Around line 444-500: The wallet-open branch currently fetches and renders the
private key unconditionally when self.wallet_open is true; change it so the code
only calls selected_wallet.read().unwrap().private_key_at_derivation_path(...)
and renders the WIF/hex grid when either
self.view_private_key_even_if_encrypted_or_in_wallet is true or
self.decrypted_private_key.is_some(); otherwise do not attempt to fetch or
display the key and keep self.decrypted_private_key as None. Constrain the UI
button handling (view_private_key_even_if_encrypted_or_in_wallet /
view_wallet_unlock) to trigger the fetch/display path, and ensure the match on
private_key_at_derivation_path is only executed inside that guarded branch so
the key is not exposed by default.
---
Duplicate comments:
In `@src/ui/components/password_input.rs`:
- Around line 167-200: The code only requests a repaint while self.revealing is
true, so release transitions can leave plaintext visible for the current frame;
fix by detecting when the reveal state changes and requesting a repaint on both
transitions: capture let old_revealing = self.revealing, update self.revealing =
eye_response.is_pointer_button_down_on() && eye_response.hovered(), then if
old_revealing != self.revealing call ui.ctx().request_repaint(); reference
self.revealing, eye_response.is_pointer_button_down_on(),
eye_response.hovered(), and ui.ctx().request_repaint() to locate where to add
this change.
In `@src/ui/components/wallet_unlock_popup.rs`:
- Around line 128-133: The password field is calling request_focus() every frame
while empty which steals focus; modify the WalletUnlockPopup state (e.g., add a
boolean like focus_requested) and only call self.password_input.request_focus()
once per popup open: when showing the field in password_input.show(ui) check
pw_response.response.gained_focus() || (self.password_input.is_empty() &&
!self.focus_requested), call request_focus() and then set self.focus_requested =
true; ensure focus_requested is reset to false when the popup is
opened/initialized so the behavior repeats correctly on subsequent opens.
In `@src/ui/wallets/asset_lock_detail_screen.rs`:
- Around line 354-358: The code creates an owned String each frame via
to_owned() just to render the WIF; instead, avoid heap-copying the secret and
render a borrowed &str for read-only display. Replace the
egui::TextEdit::multiline(...) call (which currently uses self.private_key_wif /
wif.expose_secret().to_owned().as_str()) with a read-only widget such as
egui::Label::new(wif.expose_secret().as_str()).monospace() (or Label with
FontId::monospace) so you only borrow the secret string from
self.private_key_wif and stop materializing an owned String every frame. Ensure
you reference self.private_key_wif and wif.expose_secret() when making the
change.
In `@src/ui/wallets/import_mnemonic_screen.rs`:
- Around line 102-112: The code in try_parse_private_key is creating owned
String copies of the masked private key each keystroke
(private_key_input.text().trim().to_string()), then passing those owned Strings
into SingleKeyWallet::from_wif/from_hex and later save paths
(parsed_single_key_wallet handling), which breaks the Secret boundary and causes
extra allocations; change to keep a borrowed &str (e.g., let input =
self.private_key_input.text().trim();) and pass that &str to
SingleKeyWallet::from_wif and ::from_hex so no temporary owned String is
allocated, and ensure the save path (where parsed_single_key_wallet is stored)
stores the Secret type or moves only the Secret container rather than cloning
the underlying String; update call sites to accept &str if needed and avoid
converting to owned String in try_parse_private_key and the save path.
---
Nitpick comments:
In `@src/ui/components/password_input.rs`:
- Around line 7-12: Implement the ComponentResponse trait for
PasswordInputResponse: add an impl ComponentResponse for PasswordInputResponse
block that delegates required trait methods to the inner response field
(Response) and exposes the changed flag appropriately; ensure methods like
into_response()/as_response()/is_changed() (or the actual method names defined
by ComponentResponse in your codebase) forward to self.response and
return/consider self.changed, and include any component-specific
validation/error handling within those trait methods so PasswordInputResponse
plugs into the generic component flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a69252b-41cf-47a1-9430-1dd10d5951e5
📒 Files selected for processing (10)
src/model/secret.rssrc/ui/components/password_input.rssrc/ui/components/wallet_unlock_popup.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/asset_lock_detail_screen.rssrc/ui/wallets/import_mnemonic_screen.rssrc/ui/wallets/single_key_send_screen.rssrc/ui/wallets/wallets_screen/dialogs.rssrc/ui/wallets/wallets_screen/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/wallets/single_key_send_screen.rs
- src/ui/wallets/wallets_screen/mod.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Callers of Secret/PasswordInput were copying secret material into plain String via .to_string()/.trim().to_string(), creating unzeroized heap allocations that undermine the Secret type's guarantees. Refactored to pass &str references directly since downstream APIs (open, from_wif, from_hex, hex::decode) all accept &str. Added Secret::len() method. Addresses PR #709 review comments CMT-001 through CMT-007. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e plaintext leak Previously request_repaint() only fired while revealing was true, missing the true→false transition. Plaintext could remain visible for one extra frame after mouse release until an unrelated repaint. Addresses PR #709 review comment CMT-008. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…to DEFAULT_CAPACITY - Drop impl now uses slice.zeroize() instead of ptr::write_bytes to prevent the compiler from optimizing away the security-sensitive wipe. - Renamed PAGE_SIZE to DEFAULT_CAPACITY since 4096 is a buffer size choice, not necessarily the OS page size on all platforms. Addresses PR #709 review comments from Copilot. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/model/secret.rs (2)
53-66:⚠️ Potential issue | 🟠 MajorLock the destination buffer before copying plaintext.
The plaintext is copied into
bufat line 57 beforeregion::lock()runs at line 60. This exposes the secret in unprotected heap memory before mlock is applied. Reorder to lock the (empty but allocated) buffer first, then write the secret.Suggested fix
pub fn new(s: impl Into<String>) -> Self { let mut source: String = s.into(); let cap = source.len().max(DEFAULT_CAPACITY); let mut buf = String::with_capacity(cap); + let lock = region::lock(buf.as_ptr(), buf.capacity()) + .map_err(|e| { + tracing::debug!("mlock failed for Secret: {e}"); + e + }) + .ok(); + let locked_ptr = buf.as_ptr(); buf.push_str(&source); // Zeroize the original string before it's freed source.zeroize(); - let lock = region::lock(buf.as_ptr(), buf.capacity()) - .map_err(|e| { - tracing::debug!("mlock failed for Secret: {e}"); - e - }) - .ok(); - let locked_ptr = buf.as_ptr(); Self { inner: Zeroizing::new(buf), _lock: lock, locked_ptr, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/secret.rs` around lines 53 - 66, In Secret::new, the secret plaintext is currently written into buf via buf.push_str(&source) before calling region::lock, exposing it unprotected; change the order so you allocate buf with String::with_capacity(cap), call region::lock(buf.as_ptr(), buf.capacity()) to mlock the allocated buffer first (handling the Result the same way as the current map_err/ok), then copy the plaintext into buf using buf.push_str(&source), and only after the copy zeroize the original source (source.zeroize()); keep the existing locked_ptr assignment and error logging behavior around region::lock.
112-124:⚠️ Potential issue | 🟡 MinorTrack capacity to detect in-place growth.
A
Stringcan expand its capacity without moving the pointer (in-place reallocation). The current check only compares pointers, missing capacity growth that leaves the newly-added tail unlocked. Track both pointer and capacity.Suggested fix
pub struct Secret { inner: Zeroizing<String>, _lock: Option<region::LockGuard>, locked_ptr: *const u8, + locked_cap: usize, }Then update
new(),with_capacity(), andrelock_if_moved():fn relock_if_moved(&mut self) { - if self.inner.as_ptr() != self.locked_ptr { - self._lock = region::lock(self.inner.as_ptr(), self.inner.capacity()) + let ptr = self.inner.as_ptr(); + let cap = self.inner.capacity(); + if ptr != self.locked_ptr || cap != self.locked_cap { + self._lock = region::lock(ptr, cap) .map_err(|e| { tracing::debug!("mlock re-lock failed after reallocation: {e}"); e }) .ok(); - self.locked_ptr = self.inner.as_ptr(); + self.locked_ptr = ptr; + self.locked_cap = cap; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/secret.rs` around lines 112 - 124, relock_if_moved currently only compares self.inner.as_ptr() to self.locked_ptr so it misses in-place capacity growth; add tracking for capacity (e.g. a new field like locked_cap) and update constructors new() and with_capacity() to initialize locked_cap = self.inner.capacity(), then change relock_if_moved to re-lock when either the pointer changed or inner.capacity() > self.locked_cap, call region::lock(self.inner.as_ptr(), self.inner.capacity()) and on success update both self.locked_ptr and self.locked_cap (and preserve existing behavior of storing the guard in self._lock and handling errors).
🧹 Nitpick comments (1)
src/model/secret.rs (1)
187-191:take()returns an unprotectedString.The
TextBuffer::take()method returns a plainStringthat escapes theSecretboundary — the caller receives unprotected, non-zeroizing memory. This is required by theTextBuffertrait, but callers should be aware. Consider adding a doc comment warning about this behavior.Suggested documentation
fn take(&mut self) -> String { + // WARNING: The returned String is NOT protected by zeroize-on-drop. + // Callers should wrap it in Secret or zeroize manually if needed. let copy = self.inner.to_string(); Zeroize::zeroize(&mut *self.inner); copy }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/secret.rs` around lines 187 - 191, Add a doc comment to the Secret::take method (and mention TextBuffer::take behavior) warning that take() returns an unprotected plain String that escapes the Secret boundary: although the method zeroizes the internal buffer via Zeroize, the returned String is non-zeroizing and stays in memory until dropped, so callers must treat, securely handle, and explicitly zeroize or overwrite the returned value if needed; place this comment directly above the fn take(&mut self) -> String definition and reference TextBuffer::take in the comment so callers understand the contractual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/model/secret.rs`:
- Around line 53-66: In Secret::new, the secret plaintext is currently written
into buf via buf.push_str(&source) before calling region::lock, exposing it
unprotected; change the order so you allocate buf with
String::with_capacity(cap), call region::lock(buf.as_ptr(), buf.capacity()) to
mlock the allocated buffer first (handling the Result the same way as the
current map_err/ok), then copy the plaintext into buf using
buf.push_str(&source), and only after the copy zeroize the original source
(source.zeroize()); keep the existing locked_ptr assignment and error logging
behavior around region::lock.
- Around line 112-124: relock_if_moved currently only compares
self.inner.as_ptr() to self.locked_ptr so it misses in-place capacity growth;
add tracking for capacity (e.g. a new field like locked_cap) and update
constructors new() and with_capacity() to initialize locked_cap =
self.inner.capacity(), then change relock_if_moved to re-lock when either the
pointer changed or inner.capacity() > self.locked_cap, call
region::lock(self.inner.as_ptr(), self.inner.capacity()) and on success update
both self.locked_ptr and self.locked_cap (and preserve existing behavior of
storing the guard in self._lock and handling errors).
---
Nitpick comments:
In `@src/model/secret.rs`:
- Around line 187-191: Add a doc comment to the Secret::take method (and mention
TextBuffer::take behavior) warning that take() returns an unprotected plain
String that escapes the Secret boundary: although the method zeroizes the
internal buffer via Zeroize, the returned String is non-zeroizing and stays in
memory until dropped, so callers must treat, securely handle, and explicitly
zeroize or overwrite the returned value if needed; place this comment directly
above the fn take(&mut self) -> String definition and reference TextBuffer::take
in the comment so callers understand the contractual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f7d25a3-9c24-428c-a5ec-93c8e50bd840
📒 Files selected for processing (5)
src/backend_task/identity/mod.rssrc/model/secret.rssrc/ui/components/password_input.rssrc/ui/components/wallet_unlock.rssrc/ui/wallets/import_mnemonic_screen.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend_task/identity/mod.rs
Address 12 findings from grumpy-review triage: Security (secret.rs): - SEC-004: use volatile zeroize instead of ptr::write_bytes in delete_char_range - SEC-006: Clone via (*self.inner).clone() instead of .to_string() - SEC-007: CompareOnlyPtr newtype enforces Send/Sync invariant at type level - SEC-002: compile-time needs_drop assertion + ignored runtime Drop test - CODE-005: add Secret::trimmed() method Callsite fixes: - SEC-001: zeroize seed phrase Vec<String> on reset and Drop - CODE-001: use take_secret() instead of Secret::new(text().to_string()) - CODE-002: direct &str ref for TextEdit, comment on clipboard copy - CODE-006: derive Default instead of Secret::new(String::new()) - CODE-007: pass s directly to Secret::new() in set_text Convention fixes: - PROJ-002: make dashmate_password_input private - PROJ-005: request focus once on open, not every frame Add INTENTIONAL comments for 4 accepted risks (SEC-003, SEC-005, CODE-003, PROJ-001). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
src/ui/identities/keys/key_info_screen.rs (2)
632-691:⚠️ Potential issue | 🟠 MajorClear the submitted key once import succeeds.
After
private_key_datais populated,private_key_inputstill retains the original secret. If this screen later goes back to add mode, that old key will show up again in the input widget.🧹 Proposed fix
} else if validation_result.unwrap() { // If valid, store the private key in the context and reset the input field self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None)); self.identity.private_keys.insert_non_encrypted( (self.key.purpose().into(), self.key.id()), (self.key.clone().into(), private_key_bytes), ); + self.private_key_input.clear(); if let Err(e) = self .app_context .update_local_qualified_identity(&self.identity)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 632 - 691, In validate_and_store_private_key, after successfully storing the key into private_key_data and inserting into identity.private_keys and after app_context.update_local_qualified_identity succeeds, clear the UI input by resetting private_key_input (e.g. call the input widget's method to set an empty string or clear its text) so the secret is not retained in the field when the screen returns to add mode; ensure the clear happens only on full success (after update_local_qualified_identity returns Ok).
318-337:⚠️ Potential issue | 🟠 MajorAvoid rebuilding private-key display strings on every repaint.
These branches call
to_wif()/hex::encode()insideui()and then wrap the result in a freshSecret. The backing key material already lives in state, so every repaint just creates more plaintext copies than necessary. Cache the displaySecrets once when the key becomes viewable and clear them when the user hides or leaves the screen.Also applies to: 377-394, 415-435, 473-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/keys/key_info_screen.rs` around lines 318 - 337, The UI code in key_info_screen.rs is repeatedly calling private_key.to_wif() and hex::encode(clear) inside the rendering path, wrapping results in new Secret instances (e.g., variables like wif and private_key_hex) on every repaint; instead, compute and store these display Secrets once when the key becomes viewable (e.g., in the KeyInfoScreen state/struct) and clear/zeroize them when the user hides the key or exits the screen; update the ui rendering code to reference the cached Secret fields rather than calling to_wif()/hex::encode() inline, and apply the same change to the other display spots mentioned (the branches around the other ranges) so all displayed WIF/hex strings are created only on view transition and cleared on hide/leave.
♻️ Duplicate comments (3)
src/model/secret.rs (2)
68-81:⚠️ Potential issue | 🟠 MajorLock the destination buffer before the first copy.
buf.push_str(&source)runs beforeregion::lock(), so the constructor still places the secret in pageable heap memory before mlock is applied.🔒 Proposed fix
let mut source: String = s.into(); let cap = source.len().max(DEFAULT_CAPACITY); let mut buf = String::with_capacity(cap); - buf.push_str(&source); - // Zeroize the original string before it's freed - source.zeroize(); let lock = region::lock(buf.as_ptr(), buf.capacity()) .map_err(|e| { tracing::debug!("mlock failed for Secret: {e}"); e }) .ok(); + buf.push_str(&source); + // Zeroize the original string before it's freed + source.zeroize(); let locked_ptr = CompareOnlyPtr::new(buf.as_ptr());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/secret.rs` around lines 68 - 81, The constructor places the secret into pageable memory because buf.push_str(&source) runs before calling region::lock; fix by calling region::lock on the destination buffer before copying: after creating buf with String::with_capacity(cap) call region::lock(buf.as_ptr(), buf.capacity()) (or the equivalent lock logic used currently) and handle/map errors exactly as done now, then perform buf.push_str(&source), then zeroize source and proceed to create CompareOnlyPtr::new(buf.as_ptr()) and set the lock variable; ensure you reference and update the existing symbols pub fn new, buf, source, region::lock, buf.push_str, source.zeroize, and CompareOnlyPtr::new so the lock happens prior to any write into buf.
61-63:⚠️ Potential issue | 🟡 MinorTrack locked capacity, not just the pointer.
A
Stringcan grow in place without changingas_ptr(). With the current check, the newly-added tail can remain unlocked.🔁 Proposed fix
pub struct Secret { /// Dropped first -- zeroes the bytes. inner: Zeroizing<String>, /// Dropped second -- unlocks the page. _lock: Option<region::LockGuard>, /// Tracks the heap pointer so we can re-lock after reallocation. locked_ptr: CompareOnlyPtr, + /// Tracks locked capacity so in-place growth also triggers a re-lock. + locked_cap: usize, } @@ let lock = region::lock(buf.as_ptr(), buf.capacity()) .map_err(|e| { tracing::debug!("mlock failed for Secret: {e}"); e }) .ok(); let locked_ptr = CompareOnlyPtr::new(buf.as_ptr()); + let locked_cap = buf.capacity(); Self { inner: Zeroizing::new(buf), _lock: lock, locked_ptr, + locked_cap, } @@ let lock = region::lock(s.as_ptr(), s.capacity()) .map_err(|e| { tracing::debug!("mlock failed for Secret: {e}"); e }) .ok(); let locked_ptr = CompareOnlyPtr::new(s.as_ptr()); + let locked_cap = s.capacity(); Self { inner: Zeroizing::new(s), _lock: lock, locked_ptr, + locked_cap, } @@ fn relock_if_moved(&mut self) { - if !self.locked_ptr.addr_eq(self.inner.as_ptr()) { - self._lock = region::lock(self.inner.as_ptr(), self.inner.capacity()) + let ptr = self.inner.as_ptr(); + let cap = self.inner.capacity(); + if !self.locked_ptr.addr_eq(ptr) || self.locked_cap != cap { + self._lock = region::lock(ptr, cap) .map_err(|e| { tracing::debug!("mlock re-lock failed after reallocation: {e}"); e }) .ok(); - self.locked_ptr = CompareOnlyPtr::new(self.inner.as_ptr()); + self.locked_ptr = CompareOnlyPtr::new(ptr); + self.locked_cap = cap; } }Also applies to: 134-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/secret.rs` around lines 61 - 63, The code currently only tracks CompareOnlyPtr (locked_ptr) to detect reallocations, which misses in-place growth; add a locked_capacity: usize field alongside _lock and locked_ptr and update the re-locking/check logic (the code paths that inspect locked_ptr and re-acquire region::LockGuard) to compare both the current buffer pointer and the current String capacity to locked_ptr/locked_capacity so any in-place growth beyond the previously locked capacity will trigger extending/re-acquiring the lock; ensure all places that set _lock and locked_ptr also set locked_capacity to the current capacity and that unlocks/lock refreshes update locked_capacity accordingly.src/ui/identities/add_existing_identity_screen.rs (1)
980-990:⚠️ Potential issue | 🟡 MinorReset
keys_inputto empty vector to match constructor state.The constructor initializes
keys_inputas an empty vector (line 147), but the "Load Another" reset creates three emptyPasswordInputentries. This inconsistency changes form behavior after the first successful load, potentially leaving placeholder rows that users must manually remove.Suggested fix
- self.keys_input = vec![ - PasswordInput::new() - .with_hint_text("Private key (WIF or hex)") - .with_monospace(), - PasswordInput::new() - .with_hint_text("Private key (WIF or hex)") - .with_monospace(), - PasswordInput::new() - .with_hint_text("Private key (WIF or hex)") - .with_monospace(), - ]; + self.keys_input.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/add_existing_identity_screen.rs` around lines 980 - 990, The reset handler currently repopulates self.keys_input with three empty PasswordInput entries, but the constructor initializes keys_input as an empty Vec; update the reset logic to set self.keys_input = Vec::new() (or clear with self.keys_input.clear()) instead of creating three PasswordInput::new() instances so the form returns to the original empty state; locate the code that assigns to self.keys_input within the "Load Another" / reset branch and replace the vec![PasswordInput::new() ...] assignment with an empty vector assignment.
🧹 Nitpick comments (2)
src/ui/components/password_input.rs (1)
6-16: AlignPasswordInputResponsewith the shared component response trait.This type already only carries
Responseandchanged, so the current opt-out looks unnecessary and forcesPasswordInputto be handled differently from the rest ofsrc/ui/components.As per coding guidelines "UI components should follow a lazy initialization pattern: domain data stored separately from UI components, builder methods for configuration, response struct with
ComponentResponsetrait, self-contained validation and error handling."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/password_input.rs` around lines 6 - 16, PasswordInputResponse is currently opted out of the shared ComponentResponse trait; update it to implement ComponentResponse so PasswordInput can be handled like other components. Remove the INTENTIONAL opt-out comment and implement the ComponentResponse trait for PasswordInputResponse (map its fields appropriately: expose the underlying Response and the changed flag via the trait's required methods), and ensure PasswordInput::show / any callers still return PasswordInputResponse but now satisfy ComponentResponse so the component integrates with the common UI patterns.src/model/secret.rs (1)
405-416: This test never inspects the tail it claims to zero.
secret.inner.as_bytes()only exposes0..len, so this still passes if[len..capacity)retains deleted data. Either rename the test or inspect the tail in a controlled ignored test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/secret.rs` around lines 405 - 416, The test test_delete_char_range_zeroes_trailing claims to verify trailing capacity is zeroed but only reads secret.inner.as_bytes() (0..len), so update the test to actually inspect the tail or change its intent: either (A) rename the test to remove the "zeroes_trailing" claim, or (B) implement a controlled inspection of the tail after calling Secret::delete_char_range by temporarily accessing the raw buffer between inner.len() and inner.capacity() (e.g., using an unsafe block to read the bytes via secret.inner.as_mut_ptr()/std::slice::from_raw_parts and assert they are zero) and mark the test #[ignore] or #[cfg(...)] if you want to avoid UB in normal test runs; reference test_delete_char_range_zeroes_trailing, Secret::delete_char_range, secret.inner, as_bytes(), inner.len(), and inner.capacity() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ui/identities/keys/key_info_screen.rs`:
- Around line 632-691: In validate_and_store_private_key, after successfully
storing the key into private_key_data and inserting into identity.private_keys
and after app_context.update_local_qualified_identity succeeds, clear the UI
input by resetting private_key_input (e.g. call the input widget's method to set
an empty string or clear its text) so the secret is not retained in the field
when the screen returns to add mode; ensure the clear happens only on full
success (after update_local_qualified_identity returns Ok).
- Around line 318-337: The UI code in key_info_screen.rs is repeatedly calling
private_key.to_wif() and hex::encode(clear) inside the rendering path, wrapping
results in new Secret instances (e.g., variables like wif and private_key_hex)
on every repaint; instead, compute and store these display Secrets once when the
key becomes viewable (e.g., in the KeyInfoScreen state/struct) and clear/zeroize
them when the user hides the key or exits the screen; update the ui rendering
code to reference the cached Secret fields rather than calling
to_wif()/hex::encode() inline, and apply the same change to the other display
spots mentioned (the branches around the other ranges) so all displayed WIF/hex
strings are created only on view transition and cleared on hide/leave.
---
Duplicate comments:
In `@src/model/secret.rs`:
- Around line 68-81: The constructor places the secret into pageable memory
because buf.push_str(&source) runs before calling region::lock; fix by calling
region::lock on the destination buffer before copying: after creating buf with
String::with_capacity(cap) call region::lock(buf.as_ptr(), buf.capacity()) (or
the equivalent lock logic used currently) and handle/map errors exactly as done
now, then perform buf.push_str(&source), then zeroize source and proceed to
create CompareOnlyPtr::new(buf.as_ptr()) and set the lock variable; ensure you
reference and update the existing symbols pub fn new, buf, source, region::lock,
buf.push_str, source.zeroize, and CompareOnlyPtr::new so the lock happens prior
to any write into buf.
- Around line 61-63: The code currently only tracks CompareOnlyPtr (locked_ptr)
to detect reallocations, which misses in-place growth; add a locked_capacity:
usize field alongside _lock and locked_ptr and update the re-locking/check logic
(the code paths that inspect locked_ptr and re-acquire region::LockGuard) to
compare both the current buffer pointer and the current String capacity to
locked_ptr/locked_capacity so any in-place growth beyond the previously locked
capacity will trigger extending/re-acquiring the lock; ensure all places that
set _lock and locked_ptr also set locked_capacity to the current capacity and
that unlocks/lock refreshes update locked_capacity accordingly.
In `@src/ui/identities/add_existing_identity_screen.rs`:
- Around line 980-990: The reset handler currently repopulates self.keys_input
with three empty PasswordInput entries, but the constructor initializes
keys_input as an empty Vec; update the reset logic to set self.keys_input =
Vec::new() (or clear with self.keys_input.clear()) instead of creating three
PasswordInput::new() instances so the form returns to the original empty state;
locate the code that assigns to self.keys_input within the "Load Another" /
reset branch and replace the vec![PasswordInput::new() ...] assignment with an
empty vector assignment.
---
Nitpick comments:
In `@src/model/secret.rs`:
- Around line 405-416: The test test_delete_char_range_zeroes_trailing claims to
verify trailing capacity is zeroed but only reads secret.inner.as_bytes()
(0..len), so update the test to actually inspect the tail or change its intent:
either (A) rename the test to remove the "zeroes_trailing" claim, or (B)
implement a controlled inspection of the tail after calling
Secret::delete_char_range by temporarily accessing the raw buffer between
inner.len() and inner.capacity() (e.g., using an unsafe block to read the bytes
via secret.inner.as_mut_ptr()/std::slice::from_raw_parts and assert they are
zero) and mark the test #[ignore] or #[cfg(...)] if you want to avoid UB in
normal test runs; reference test_delete_char_range_zeroes_trailing,
Secret::delete_char_range, secret.inner, as_bytes(), inner.len(), and
inner.capacity() when making the change.
In `@src/ui/components/password_input.rs`:
- Around line 6-16: PasswordInputResponse is currently opted out of the shared
ComponentResponse trait; update it to implement ComponentResponse so
PasswordInput can be handled like other components. Remove the INTENTIONAL
opt-out comment and implement the ComponentResponse trait for
PasswordInputResponse (map its fields appropriately: expose the underlying
Response and the changed flag via the trait's required methods), and ensure
PasswordInput::show / any callers still return PasswordInputResponse but now
satisfy ComponentResponse so the component integrates with the common UI
patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d3a0e65-80de-4923-854d-1085e4214566
📒 Files selected for processing (10)
src/model/secret.rssrc/ui/components/password_input.rssrc/ui/components/wallet_unlock_popup.rssrc/ui/identities/add_existing_identity_screen.rssrc/ui/identities/add_new_identity_screen/mod.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/network_chooser_screen.rssrc/ui/wallets/asset_lock_detail_screen.rssrc/ui/wallets/import_mnemonic_screen.rssrc/ui/wallets/wallets_screen/dialogs.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/identities/add_new_identity_screen/mod.rs
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
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.
Issue being fixed or feature implemented
Closes #705 — Sensitive strings (passwords, WIF private keys) stored as plain
Stringwithout memory zeroingCloses #707 — Reusable password input component with hold-to-reveal
User Story
Imagine you are managing your Dash wallets and identities. Previously, when you typed a password or private key, it was displayed in plaintext on screen — anyone watching could read it. And even after you navigated away, the sensitive data lingered in memory, potentially accessible to malware or memory dumps.
Now, all password and private key fields are masked by default with a small eye icon — hold the eye to peek at what you typed, release to re-mask. Behind the scenes, all sensitive strings are automatically zeroed when no longer needed, and their memory pages are locked to prevent swapping to disk.
What was done?
New types and components
Secrettype (src/model/secret.rs) — Zeroize-on-drop wrapper with best-effortmlockviaregioncrate. RedactedDebug, constant-timePartialEq, noDisplay/Deref(explicitexpose_secret()access)PasswordInputcomponent (src/ui/components/password_input.rs) — Masked text input with hold-to-reveal eye icon, builder API, error displaySecurity fixes
Secretfor zeroize-on-dropScreenWithWalletUnlocktrait simplified from 4 methods to 1.zeroize()calls removed (Secret handles it automatically)Migration scope
PasswordInputand/orSecretIdentityInputToLoadbackend struct fields migrated toSecretHow has this been tested?
cargo build --all-features— cleancargo clippy --all-features --all-targets -- -D warnings— zero warningscargo +nightly fmt --all— cleancargo test --all-features --workspace— 42 passed, 0 faileddocs/ai-design/2026-03-09-password-input/manual-test-scenarios.mdBreaking Changes
None — all changes are internal. No public API changes.
Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores