keep-mobile: hardening & test-coverage backlog (29 items)

[kwsantiago]

Hardening and test-coverage findings in keep-mobile.

Read this before picking anything up. The original list held twenty-nine entries. A manual pass on 2026-07-30 read the code for each: eleven no longer described the tree, two were the same defect filed twice, and three of the wrong ones were errors I introduced during that pass itself, from a search that reports absence for anything it cannot match. Everything below was checked by reading the code rather than searching for it.

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

• No hook validates a signing request on mobile — Installing hooks replaces the set rather than composing with it, so this crate's hooks are the entire pre-sign policy and the ones that would refuse a raw or unstructured request never run. This is the reason the request label reaching the approval prompt is unvalidated, and requiring a structured body would also make the verified case the norm rather than the exception. The largest item on this list.
• Approval prompt still shows no amount or destination — The structured metadata is hardcoded empty on the co-sign path. Those fields can be read from the verified body rather than from anything the requester asserts, which would turn an approval from a message-type label into the actual spend being approved. The label and the requesting peer now cross the boundary correctly and the label is marked verified or claimed; this is what remains.
• No cert pinning on the bunker relay path — Retained with a correction: an earlier version of this entry, and my own audit comment repeating it, said pinning was absent. It is present. The bunker start path pins its relays through the same helper the initialization path uses and holds the pin lock across the call. What is worth checking is whether every relay path does, not this one.
• get_active_share hides storage failures — Returns Option, so a storage error is indistinguishable from no active share. The trait method it calls returns Option too, so closing this is an FFI contract change and needs the Android side in lockstep.
• frost_generate and frost_split have no rate limit or concurrency guard — Key generation runs unguarded.
• FROST peer signing path has no per-peer limit — Bounded only by a global pending-request cap, so one peer can fill the queue.
• Audit entry details carries no guidance against secrets — Bounded in length, with nothing saying what may go in it.
• clear_entries takes only a confirmation string — No authorization beyond a magic word, and the trait is foreign-implemented.
• Plaintext mnemonic persisted without application-layer encryption — Two entries described this; it is one. Stored as bytes to secure storage relying on the platform layer alone.
• Seed words cross the FFI as an unzeroizable string — Two entries described this; it is one. The foreign caller cannot wipe what it receives.
• Mnemonic is lost on restore from backup — The backup share carries no mnemonic and restore hardcodes none, so seed words do not survive a backup and restore cycle.
• Deserialized audit entries are exported without re-verifying the chain — Export converts loaded entries without checking linkage.
• Audit verify returns early on length before the constant-time compare — The comparison itself is constant time and the lengths are structural rather than secret, so this is a documentation gap rather than a leak. Worth a note saying so, since the next reader will otherwise either "fix" the lengths pointlessly or assume the compare is unsafe.
• Audit chain can be dropped or truncated by a hostile storage implementation — The storage trait is foreign-implemented and entries are not individually signed.
• do_start_bunker branch selection untested — The local versus network branch has no test.

Removed as already fixed

The now-milliseconds truncation is gone with the function that held it. The NIP-46 error mapper handles user-rejected and capacity-exceeded explicitly rather than letting them fall through. The From<KeepError> implementation that was inconsistent with it no longer exists anywhere in the crate. The rate-limit map has a hard cap with a test that pushes past it. Callback URL schemes are an explicit allowlist, with a comment recording why domain allowlisting is not meaningful when the callback comes from the calling app. The x-only prefix assumption is documented at the one place it occurs, explaining that an off-curve value fails later at the point of use before any secret is derived; two entries described that single line. Node initialization degrades to in-memory sessions rather than failing. The restore path keeps the recorded backed-up state instead of marking everything backed up. Audit timestamps can no longer move backwards along the chain, on both chains rather than only the one this list named.

Removed as wrong

NIP-49 export has no round-trip test. It has two, and the first is precisely the round trip: it exports, decrypts with the same password, checks the signing share comes back, and checks a different password fails. My error, same search flaw as above.

Duplicate rate limiting with the velocity tracker. The entry says the premise is weak in its own text. The two track different domains, per-package request counts and Bitcoin spend amounts.

Mnemonic copy before zeroize leaves an allocator residual. The entry marks itself informational and concludes the pattern is acceptable.

[kwsantiago]

Triage pass (verified against current code). Dispositions:

Being fixed (PR #854):

• rate-limit HashMap unbounded growth — hard cap added with a flood test.
• mobile fails node init on session-store error — now degrades to in-memory sessions.
• nip46_to_mobile_error drops UserRejected/CapacityExceeded — now mapped to InvalidInput/RateLimited.

Already fixed in current code:

• get_seed_words non-zeroizing String / unzeroized Option over UniFFI — now returns a wipeable Option<Vec<u8>>.
• NIP-49 export_ncryptsec round-trip test — export_ncryptsec_accepts_utf8_password_bytes covers encrypt + decrypt + wrong-password.

Not a defect (verified):

• duplicate rate limiting — SigningRateLimiter (per-caller request counts) and VelocityTracker (Bitcoin spend amounts) are distinct domains.
• audit timestamp-ordering validation — integrity is hash-chain linkage via prev_hash, not wall-clock; enforcing monotonic timestamps would false-positive on NTP skew.
• audit verify() length-check-before-ct_eq timing — compared values are fixed-length public integrity tags, not secrets.
• hardcoded 0x02 x-only prefix — on-curve validation happens downstream when secp256k1 parses the compressed key.

Intentional / by-design:

• did_backup forced true on restore (a restored share provably came from a backup); export_json dumps raw entries so forensics succeed on a tampered log (verify_chain is the separate integrity check); plaintext_mnemonic relies on Keystore/Keychain at-rest encryption (documented); no rate-limit on owner-initiated local frost_generate/frost_split; foreign AuditStorage is the app's own TCB (at-rest tampering is detectable via verify_chain); MAX_PENDING_REQUESTS bounds the peer signing path; clear_entries confirm-token guards accidental clears with auth enforced at the native layer.

Remaining (keeps this open): bunker-relay cert pinning (defense-in-depth, medium), do_start_bunker branch-selection test, and the optional From unification (deferred: changes error variants seen across the FFI boundary).

[kwsantiago]

Re-verified this list against current main by reading the code. Nine of the twenty-nine items no longer describe the tree. Recording which, so the list stops being re-mined and the genuine remainder is visible.

No longer present

• now_ms as u64 truncation. The function and the .as_millis() as u64 pattern are both gone from the signing policy module.
• nip46_to_mobile_error drops UserRejected and CapacityExceeded. Both are mapped explicitly now, UserRejected to an input error and CapacityExceeded to the rate-limited variant, alongside Timeout and RateLimited. The stringified fall-through no longer swallows them.
• From<KeepError> for KeepMobileError inconsistent with the per-variant mapper. That impl no longer exists anywhere in the crate, so the two mappers cannot disagree.
• Rate-limit map unbounded growth. There is a hard cap constant with an eviction check against it, plus a test that pushes past the cap.
• Callback URL allows https without validation. Schemes are now an explicit allowlist, with a comment recording why domain allowlisting is not meaningful here: the callback is supplied by the calling app under NIP-55.
• Hardcoded x-only prefix assumption, and the second item describing the same line. There is one occurrence and it now carries the explanation the finding asked for: an off-curve x fails later at the point of use, before any secret is derived, rather than being silently trusted at parse time. Both items are the same defect and both are addressed.
• Mobile fails node init on session store error. It degrades now, warning and continuing with in-memory sessions instead of propagating, which is the desktop behavior the item wanted parity with.

Confirmed still present

Cert pinning is still absent on the bunker relay path, where only URL validation runs. frost_generate and frost_split still have no rate limit or concurrency guard, and frost_generate still constructs the trusted dealer with no warning surfaced through the FFI. Audit entries still take a wall-clock timestamp with no ordering validation, verify still does early-return length checks with no note that it is therefore not fully constant-time, the details field is bounded but carries no no-secrets guidance, and clear_entries still takes only a confirmation string. get_active_share still collapses storage errors into the same None as a genuine absence. Restore still hardcodes did_backup: true, overriding what the backup recorded. export_ncryptsec still has no test of any kind. The FROST peer signing path is still bounded only by the pending-request cap, with no per-peer limit. The plaintext mnemonic is still stored, and the seed-words accessor still returns a String the foreign caller cannot zeroize.

Nine of twenty-nine were already done and never ticked. Worth noting the same ratio held on the keep-cli list, which suggests the other hardening umbrellas are similarly overstated rather than this one being unusual.

[kwsantiago]

The restore flag item is done in #925, both halves.

Shares now carry through what the backup recorded, at both places restore writes it. Keys pass false rather than true, which needed separate reasoning: the format records no flag for a key, so there is nothing to preserve and it is a choice. False matches what a fresh import passes and keeps the prompt to save the seed words, which restoring a vault file does not establish the user has done.

Two things surfaced while verifying that are worth keeping with the item. The mobile site originally read the value from the backup and a later change replaced it with a hardcoded true with no stated reason, so this restores prior behavior rather than inventing it, and explains why keep-core and mobile disagreed on the same field. And the flag is serde-defaulted with no format version bump when it was added, so a backup predating it is indistinguishable from one recording false. That is acceptable here because the only consumers are an advisory prompt and a seed-words confirm button, with nothing destructive gated on it, but it is worth knowing before anything else starts trusting that field.

[kwsantiago]

Correcting another error in my audit above.

I wrote that the NIP-49 export has no test of any kind. It has two, and the first is precisely the round-trip the item asks for: it exports, decrypts with the same password, checks the signing share comes back, and checks a different password fails. The item is fully stale.

The cause is the same faulty search that produced my earlier false claim about descriptor reconstruction: looking for lines containing both the symbol and the word "test", which a test function's own name line does not contain. I identified that flaw, corrected the other issue for it, and then repeated it here in the same audit. Both false negatives are mine and both went in the direction of inventing work rather than hiding it, which is the less damaging direction but still wasted verification effort.

I re-checked the two remaining claims in that audit that rested on similar searches rather than on reading the code. Both hold: the FROST generation entry point genuinely has no doc comment, so the trusted-dealer caveat is not surfaced to FFI callers, and there is genuinely no guidance against putting secrets in the audit details field. The rest of that audit's "still present" list was established by reading the code and stands.

Net for this list: ten of twenty-nine were already done, not nine.