Skip to content

keep-desktop: hardening & test-coverage backlog (27 items) #795

Description

@kwsantiago

Backlog of hardening / test-coverage findings in keep-desktop, verified still-open against current code by an automated triage pass. Each is an independent, pickup-able task — pick one and open a PR.

Findings

  • No tests for descriptor coordination UI/logic — No test coverage exists for descriptor coordination; grep for handle_descriptor_complete/handle_descriptor_contribution_needed/begin_descriptor_coordination in keep-desktop/src finds no test references and frost.rs has no cfg(test) module.
  • No descriptor string validation before storage — handle_descriptor_complete (keep-desktop/src/frost.rs:1457-1478) still builds WalletDescriptor from network-supplied external/internal_descriptor and calls store_wallet_descriptor (keep-core/src/lib.rs:997-1002) which persists without any descriptor-parse validation.
  • Auto-contribution leaks xpub without user approval — handle_descriptor_contribution_needed (keep-desktop/src/frost.rs:1202-1260) still auto-derives and contributes the xpub via contribute_descriptor with no user confirmation prompt; only network-validity and coordination-count checks gate it.
  • Connected status shown even when no peers found — The connect loop still breaks on the 10s timeout returning Ok(()) (frost.rs:472-486), and handle_connect_relay_result maps Ok(()) => ConnectionStatus::Connected (frost.rs:1508-1511), so Connected shows even with zero peers discovered.
  • block_in_place inside pre_sign risks deadlock — DesktopSigningHooks::pre_sign still uses tokio::task::block_in_place + Handle::current().block_on to await UI approval with a timeout (frost.rs:241-258); same deadlock-risk pattern, just relocated from app.rs to frost.rs.
  • active_share file persists on disk after vault lock — do_lock still only clears active_share_hex in memory (keep-desktop/src/app/helpers.rs:37) and never calls set_active_share_key(None), so the active_share file persists on disk after lock.
  • ExportData with Zeroizing bech32 travels through cloneable Message — message.rs:47-51 ExportData still #[derive(Clone)] holding Zeroizing bech32/frames, wrapped in cloneable Message::ExportGenerated(Result<ExportData,String>).
  • Import data field is not masked/secured — import.rs:407,414 masks the data field only when is_secret (mode==Nsec); the kshare1 FROST share data is still rendered with .secure(false) in plaintext.
  • Export bech32 string shown in plaintext on screen — export.rs:247-257 still renders the first 80 chars of the bech32 export as plaintext with no reveal toggle.
  • Passphrase strength meter uses naive scoring — export.rs:14-34 passphrase_strength still uses len+variety*5 heuristic; the cited 36-char 'aaaa...A1!' still scores as Strong.
  • CopyNpub does not clear clipboard on timer — app/shares.rs:555-557 handle_copy_npub only calls iced::clipboard::write(npub) with no start_clipboard_timer/clipboard_clear_at, unlike handle_copy_sensitive.
  • Zeroizing cloned through Message enum — message.rs:131 Message still #[derive(Clone)] and carries Zeroizing (e.g. RecoveryResult, VaultShareExported); Iced clones remain non-zeroized (inherent limitation, not fixed).
  • Unmapped error logging may expose file paths — util.rs:78-81 catch-all still logs tracing::warn!('Unmapped keep error: {e}'); StorageErr is unmapped and falls through, so its path-bearing message can still leak into logs.
  • CopyNpub leaks npub in debug logs — screen/shares.rs:19 (and nsec_keys.rs:49) derive Debug for a Message enum whose CopyNpub(String) variant prints the full npub, inconsistent with the *** redaction of copy ops.
  • Passphrase strength scorer gives misleading ratings — export.rs:14-34 scorer is still linear len+variety*5, not entropy-aware; a 15-char all-lowercase passphrase still scores 15 => Weak.
  • Desktop text_input on_input creates non-Zeroizing intermediate String — Pattern .on_input(|s| Message::PasswordChanged(Zeroizing::new(s))) still receives a plain String from Iced at keep-desktop/src/screen/unlock.rs:173; inherent framework limitation, unaddressed.
  • Bech32 truncated display shown in plaintext on export screen — First 80 chars of the bech32 export are still rendered as a plaintext text widget at keep-desktop/src/screen/export.rs:247-257.
  • Unmapped error variant logged with full error details — Catch-all still logs full error Display via tracing::warn!("Unmapped keep error: {e}") at keep-desktop/src/app/util.rs:79.
  • QR code data objects not zeroized when export screen is reset or navigated away — reset() clears qr_display to None without zeroizing the qr_code::Data contents at keep-desktop/src/screen/export.rs:142-146.
  • No minimum passphrase length enforced on import — Import still only checks self.passphrase.is_empty() with no minimum length at keep-desktop/src/screen/import.rs:314.
  • friendly_err leaks InvalidInput message content to UI — friendly_err still passes the inner message straight through: KeepError::InvalidInput(msg) => format!("Invalid input: {msg}") at keep-desktop/src/app/util.rs:67.
  • Toast banner pushes screen content down, no overlay — keep-desktop/src/app/mod.rs:729 still renders the toast as column![banner, screen], pushing screen content down rather than overlaying.
  • Tracing subscriber logs to stdout by default with no filtering — main.rs:26 still calls tracing_subscriber::fmt::init() with no explicit level/EnvFilter configuration, exactly the state flagged.
  • Export screen shows first 80 chars of bech32 share in cleartext — keep-desktop/src/screen/export.rs:247-257 still renders the first 80 chars of the bech32 share as an on-screen text widget.
  • ExportScreen retains encrypted share data after auto-lock — QrDisplay's qr_code::Data internals in keep-desktop/src/screen/export.rs:71-77 are still dropped without zeroization on auto-lock; no mitigation added.
  • Clipboard write creates non-zeroized copy of exported share — keep-desktop/src/app/shares.rs:600 still passes (*data).clone(), a non-zeroized plain String, into iced::clipboard::write.
  • take() pattern leaves Keep as None during async operations, allowing concurrent message dispatch to fail silently — The take()-then-put-back pattern still leaves Keep as None during the async window in with_keep_blocking (keep-desktop/src/app/util.rs:124-132), and no full UI lockout prevents a concurrent operation from hitting 'Keep not available'.

Grouped from individual tracked items; close each checkbox as its PR merges.

Metadata

Metadata

Assignees

No one assigned

    Labels

    desktopkeep-desktop GUI applicationp2PrioritysecuritySecurity-related issues

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions