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

[kwsantiago]

Hardening and test-coverage findings in keep-desktop.

Read this before picking anything up. The original list held twenty-seven entries and far fewer distinct defects. A manual pass on 2026-07-30 read the code for each: five clusters covering twelve entries were the same defect filed repeatedly, four entries were wrong about the code, and one would have caused harm if implemented as written. Deduplicated and corrected below.

Entries here name where the code looks wrong. They are not instructions. Read the code before acting on one, and if it does not say what the entry says, correct the entry.

Open

• Coordination handlers untested — Descriptor validation itself has four tests covering a valid pair and malformed external, internal and network inputs. The two coordination handlers, the one that contributes an extended public key and the one that completes and stores, have none. (Narrowed from an entry claiming descriptor coordination was untested outright.)
• Auto-contribution sends an xpub without asking — The contribution handler derives and contributes the account xpub on request with no user approval step.
• Connected status shown when no peers were found — The connect loop breaks on its timeout returning success, and the result handler maps that to connected.
• block_in_place inside pre_sign risks deadlock — Awaits UI approval from inside the node's async handler.
• Secrets travel through a cloneable message enum — Two entries, one for the export payload and one for the enum generally. Zeroizing values are carried through a Clone message type, so a clone outlives the zeroizing wrapper's guarantee.
• Passphrase scoring does not detect repetition — Reworded rather than closed. The linear length-plus-variety formula both original entries described is gone; scoring now estimates the alphabet the passphrase draws from. That still does not deliver what the entries wanted, because alphabet-based entropy cannot see repetition, so a long repeated string still rates well. The defect stands; anyone verifying against the formula in the old text would have wrongly closed it.
• Import share data is rendered unmasked — The import field masks only when the mode is a private key; pasted share export data renders in the clear.
• Text input creates a non-zeroizing intermediate — The input callback receives a plain String before wrapping it.
• Clipboard write copies the export into a non-zeroized string — The share copy path clones the value into a plain String for the clipboard.
• Toast pushes content instead of overlaying — Rendered as a column above the screen.
• take() leaves the vault handle empty during async work — Concurrent dispatch during that window fails silently.
• npub appears in debug logs — A message enum deriving Debug prints the full npub, inconsistent with the hashing applied elsewhere.
• QR payload is not zeroized, and cannot be from here — Real and not locally fixable. The widget library stores its module matrix in a private field with no accessor and no zeroizing wrapper, so dropping it releases the buffer uncleared. Closing this needs upstream support, or not retaining the type and rebuilding it every frame, which trades one persistent buffer for a stream of transient ones. Bounded: the buffer holds the passphrase-encrypted export, not key material, and the source string beside it is already wiped on reset.

Removed as already fixed

Locking now removes the on-disk active-share pointer before dropping the master key, with the ordering documented at the call site because clearing it requires an unlocked vault.

Removed as wrong

Export display shows the share in plaintext (three entries). The value is the passphrase-encrypted export, so what is on screen is ciphertext, not key material. More decisively, the same screen renders a QR code of the whole value directly above that text by design, because scanning it is how the share reaches the phone. Hiding eighty characters of a kilobyte-long ciphertext while a scannable encoding of all of it sits above protects nothing. One narrow note worth keeping: the truncation only applies above eighty characters, so a shorter value would render in full. That cannot happen for a share export, but it matters if that screen is reused for a shorter secret.

Unmapped error logging may expose file paths (two entries). The storage error type carries a path on several variants and deliberately leaves it out of its display formatting; the file-not-found, already-exists and permission-denied variants render as a code plus a generic phrase. Every variant was checked. The catch-all logs a code and a category, never a location.

friendly_err leaks invalid-input content to the interface. This is the one case where passing the detail through is correct. The catch-all returns a generic string and logs the detail, which is the right split. Invalid-input messages are validation text such as an unrecognised bech32 string; suppressing them leaves the user with a refusal and no way to tell what they typed wrong. Checked for any carrying a path or secret; there are none.

Copying an npub does not clear the clipboard. An npub is a public identifier, published on relays and handed to every app the user connects. Clearing it after a timeout breaks the ordinary reason for copying it and protects nothing. Our own timer sets a flag named for holding a secret and takes its duration from a secret-clearing setting, so routing a public value through it would misreport what the clipboard holds. Another established signer implementation draws the same line, using its sensitive-clipboard helper for the private key and seed words only, with public keys on the plain path.

Tracing subscriber logs without filtering. The environment-filter feature is enabled for the workspace, so the default initialiser reads the standard log-level variable and defaults to info. What actually differs from the command-line binary is that it writes to standard output rather than standard error and sets no explicit fallback, which is a smaller and different point.

Removed as harmful

No minimum passphrase length enforced on import. Do not implement this. Import does not choose a passphrase; it supplies one that must match what an export already used. A length rule there is not a policy, it is a filter on values the user may legitimately have to enter, and anyone holding a share exported under a shorter passphrase would find it unimportable, with the failure appearing as a validation refusal rather than anything explaining their data is fine and the rule is new. Locking a user out of their own share is worse than the weak passphrase it would discourage. The rule belongs where the passphrase is chosen and is already there: the export screen enforces a minimum before proceeding and requires the confirmation field to match.

[kwsantiago]

Triage pass (verified against current code). Dispositions:

Being fixed (PR #853):

• active_share file persists on disk after vault lock — do_lock now calls set_active_share_key(None) before lock() to remove the on-disk pointer.

Already defended (verified):

• no descriptor-string validation before storage — keep-frost-net independently reconstructs the descriptor via the real builder and NACKs on mismatch (node/descriptor.rs reconstruct_descriptor + policy-hash check + "Descriptor mismatch" rejection) before completion fires, so desktop stores an already-validated string.

Not a defect (verified):

• block_in_place deadlock in pre_sign — standard sync-to-async bridge; block_in_place frees the worker thread, no deadlock on the multithreaded runtime.
• CopyNpub no clipboard-clear timer / npub in debug logs — npub and group pubkeys are public identifiers, not secrets; auto-clear and redaction are for secret copies only.
• no minimum passphrase length on import — import must accept whatever passphrase encrypted the export; decryption failure is the real gate.
• friendly_err leaks InvalidInput to UI — these are validation descriptions (e.g. "must be 64 hex"), not secrets.
• take() leaves Keep None during async ops — deliberate: avoids holding the mutex across the await and returns an explicit error, not a silent failure.

Intentional / inherent framework limitation:

• Zeroizing / QR Data cloned or dropped un-zeroized through the Iced Message enum (7/12/16/19/25/26) — Iced requires Message: Clone and hands owned Strings from text_input; the carried payloads are passphrase-encrypted ciphertext, and the Zeroizing wrappers are wiped on drop.
• bech32 first-80-char preview shown in plaintext (9/17/24) — deliberate partial preview of an encrypted export blob; full value is in the QR + Copy button behind a warning banner.
• auto-xpub contribution without a prompt — intended WDC co-signer flow; the xpub is only meaningful to peers already sharing group_pubkey and the node verifies the peer share index.
• passphrase strength meter heuristic (10/15) — advisory only; the load-bearing control is the MIN_EXPORT_PASSPHRASE_LEN gate.
• unmapped-error catch-all logs full Display (13/18) — path-bearing variants are all mapped to generic messages above; the catch-all only logs, the UI gets a generic string.
• tracing fmt::init honors RUST_LOG and secret Debug impls are redacted; toast layout is cosmetic.

Remaining (keeps this open): "Connected" status shown on connect-timeout with zero peers (borderline: the node genuinely is relay-connected), and descriptor-coordination test coverage for the pure helpers.

[kwsantiago]

Re-verified against current main. The headline is duplication rather than staleness: this list has twenty-seven entries but far fewer distinct defects.

Duplicate clusters

• The bech32 export display appears three times: as the plaintext-on-screen item, the truncated-display item, and the first-eighty-characters item. One defect, one render site.
• The passphrase scorer appears twice.
• The unmapped-error log appears twice, once framed as path exposure and once as full error details.
• The export screen's QR data retention appears twice, once as reset-without-zeroize and once as retention after auto-lock.
• Zeroizing carried through a cloneable message appears twice, once for the export payload and once for the message enum generally.

That is five clusters covering twelve entries, so twenty-seven listed becomes about twenty distinct items.

No longer present

• Active share file persists on disk after vault lock. Lock now removes the on-disk pointer before dropping the master key, with a note explaining the ordering requirement, since clearing it needs an unlocked vault.

Reworded rather than fixed

• Passphrase strength scoring. The linear length-plus-variety heuristic both entries describe is gone; scoring now estimates the alphabet the passphrase draws from. That does not deliver what the entries actually want, because pool-based entropy still cannot see repetition, so the cited example almost certainly still rates well. The defect stands but its description no longer matches the code, and anyone verifying against the stated formula would wrongly close it.

Confirmed still present

Descriptor coordination has no test coverage at all. The bech32 export still renders its first eighty characters with no reveal toggle, and the QR payload is still dropped without zeroizing. Copying an npub still writes to the clipboard with no clearing timer, while the neighbouring share-copy path does start one, so the inconsistency is real. The catch-all still logs the full error display. Import still accepts any non-empty passphrase with no minimum length. The friendly-error mapper still passes inner invalid-input text straight to the UI. The toast still renders as a column above the screen rather than an overlay, and the tracing subscriber still initializes with no explicit filter.

Across the four hardening umbrellas the pattern is consistent: roughly a third of entries are already done, duplicated, or describe an unbuilt feature. Real remaining work across all four is closer to fifty items than the eighty-six the checkboxes show.

[kwsantiago]

The clipboard-clearing item does not survive verification, and this one is settled by prior art rather than by reading our code alone.

The observation is accurate: copying an npub writes to the clipboard with no clearing timer, while the neighbouring sensitive-copy path starts one. The conclusion that this is an inconsistency to fix is wrong, because the two are not the same kind of data.

An npub is a public identifier. It is published on relays, handed to every app the user connects, and is meant to be shared. Clearing it after a timeout would break the ordinary reason someone copies it, which is to paste it somewhere later, and would gain nothing: there is no confidentiality to protect in a value that is already public.

Our own code already encodes that distinction. The timer sets a flag named for holding a secret, and its duration comes from a secret-clearing setting. Routing a public identifier through it would make that flag mean something else and would tell the user their clipboard held a secret when it did not.

Another established signer implementation makes the same split deliberately. It has a dedicated sensitive-clipboard helper, documented as governing how long a copied secret stays before being cleared, and uses it for exactly two things: the private key and the seed words. Public keys go through the plain copy path with no clearing. That is the same line we draw.

So the item should be closed rather than implemented. Worth noting the three duplicate entries about the export display are a different matter: that is a secret rendered on screen, and those remain valid.

[kwsantiago]

The three entries about the export display are one item, and it does not hold up.

They describe the first eighty characters of the export as being rendered in plaintext. The value is not plaintext. It is the passphrase-encrypted share export, produced by the export call whose entire purpose is to encrypt the share under a passphrase before it leaves the vault. Without that passphrase the string is not usable, so what is on screen is ciphertext, not key material.

More decisively, the same screen renders a QR code of the whole value directly above that text, by design, because scanning it is how the share is meant to reach the phone. Hiding eighty characters of a roughly kilobyte-long ciphertext behind a reveal toggle, while a scannable encoding of the complete value sits immediately above it, protects nothing. Anyone positioned to read the text can photograph the QR.

There is one narrow real case in that code: the truncation only applies above eighty characters, so a shorter value would render in full. That cannot happen for a share export, which is far longer, but it is worth keeping in mind if that screen is ever reused for a shorter secret.

So these three entries should be closed rather than implemented. Two things near them stay valid and are genuinely worth doing: the QR payload is dropped without being zeroized when the screen resets, which is real because that buffer does hold the full value, and there is no minimum length enforced on the import passphrase, which is what actually protects this ciphertext.

[kwsantiago]

Verified the QR zeroization item. It is real and it cannot be fixed from our side today.

The QR type we render comes from the widget library and stores its module matrix in a private field with no accessor and no zeroizing wrapper. Dropping it, which is what resetting the screen does, releases that buffer without clearing it. There is no API that would let us wipe it, so closing this properly needs either upstream support or not retaining the type at all and rebuilding it on every frame, which trades a persistent buffer for a stream of transient ones and is worse.

Two things bound how much this matters. The buffer holds the passphrase-encrypted export, not key material, so what lingers is ciphertext. And the source string beside it is already wrapped so that it wipes when the screen resets, which is the part we control and it is correct.

Leaving the item open, reworded to what it actually is: a residual we cannot close locally, on ciphertext, pending an upstream way to zeroize that buffer. Not a candidate for a local fix, and worth saying so in the item so the next person does not spend the same time rediscovering the private field.

[kwsantiago]

The import passphrase minimum should not be implemented, and this is the one entry on this list where implementing it as written would cause harm rather than merely waste effort.

Import does not choose a passphrase. It supplies one that must match what an export already used, so a length rule there is not a policy, it is a filter on values the user may legitimately have to enter. Anyone holding a share exported under a shorter passphrase would find that share unimportable, with the failure appearing as a validation refusal rather than anything explaining that their data is fine and the rule is new. Locking a user out of their own share is worse than the weak passphrase it would be trying to discourage.

The rule belongs where the passphrase is chosen, and it is already there. The export screen enforces a minimum length before it will proceed, and it also requires the confirmation field to match. So the property this entry wants already holds for anything exported by this application; import is simply the wrong side of it to enforce.

The only case the entry would catch is a share exported by something other than this screen, under a shorter passphrase. Refusing to import that is not obviously right either: the share is valid and the user has the passphrase.

Closing rather than implementing. Also worth noting for the earlier comment about what protects the ciphertext on the export screen: that protection is the export-side minimum, which is present, not anything on the import path.

[kwsantiago]

Both entries about error handling in the desktop error mapper do not hold up, for different reasons.

The unmapped-error log does not expose file paths. The storage error type carries a path on several variants and its display formatting deliberately leaves it out: the file-not-found, file-already-exists and permission-denied variants render as a code plus a generic phrase, with the path field present in the struct and absent from the message. So the catch-all logging the error's display prints a code and a category, not a location. That omission is an existing deliberate decision in the core error type, not an accident this entry found. I checked every variant; none renders a path.

The invalid-input entry describes the one case where passing the detail through is correct. The catch-all returns a generic string to the interface and logs the detail, which is the right split. Invalid-input is handled separately and does pass its inner message on, and that message is validation text such as an unrecognised bech32 string or a value that is not an encrypted-key string. That is the entire content of the error: suppressing it leaves the user with a refusal and no way to tell what they typed wrong. I looked for any invalid-input message carrying a path or secret and found none; they are all descriptions of the malformed input's shape.

So neither should be implemented. What would be worth doing, and is not what either entry asks for, is a rule that the core error type keeps paths out of display formatting, since that property is currently upheld by convention at each variant and nothing prevents the next one from interpolating a path.

The tracing entry beside them is also inaccurate: the environment-filter feature is enabled for the workspace, so the default initialiser does read the standard log-level variable and defaults to info rather than logging unfiltered. What differs from the command-line binary is that it writes to standard output rather than standard error and does not set an explicit fallback, which is a smaller and different point than the entry makes.