keep-frost-net: hardening & test-coverage backlog (21 items)

[kwsantiago]

Backlog of hardening / test-coverage findings in keep-frost-net, 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

• document residual brute-force risk of --oprf-auto-approve — ServeHooks comment now documents the residual risk (keep-frost-net/src/node/mod.rs:509-521), but no absolute eval cap independent of requester identity was added and the per-identity rate limiter remains bypassable (oprf.rs), so the defense-in-depth recommendation stands.
• OPRF auto-approve safety rests solely on attestation; secondary gates weak — The rationale comment was corrected, but verify_peer_share_index (keep-frost-net/src/node/mod.rs:2683) still only checks the derivable transport pubkey and does not validate the announced verifying_share against the group's canonical shares, so the recommended cryptographic binding is not implemented.
• OPRF auto-approve safety rests solely on attestation; secondary gates are weak — Duplicate of keep-fx7k; verify_peer_share_index (keep-frost-net/src/node/mod.rs:2683) does not bind the announced verifying_share to the canonical group shares, so share-index identity remains cryptographically unvalidated.
• missing tests for announce single-flight + shutdown-abort — Single-flight suppression is now covered by test_single_flight_suppresses_concurrent_announce_quotes (keep-frost-net/tests/multinode_test.rs:3944), but no test asserts the in-flight announce is aborted on shutdown/run-exit, so that gap remains.
• canonical hash duplicated across 4 sites — Free function canonical_descriptor_hash was extracted (wallet.rs:210) and adopted at 5 sites, but descriptor_session.rs:400-406 verify_descriptor_hash still inlines the framed-prefix hash computation and can drift.
• PSBT: finalize set_finalized after broadcast can desync peers — finalize_psbt_session still broadcasts PsbtFinalize before set_finalized (keep-frost-net/src/node/psbt.rs ~1128-1145) and calls set_finalized with ?, so a concurrent abort/remove_session between the read and write locks still lets peers finalize while the local session errors out.
• PersistedDescriptorLookup fails closed when Keep is locked (WDC-PSBT) — find_by_hash still returns no-match when the vault is locked and decide_descriptor_hash_verification (keep-frost-net/src/node/psbt.rs:1704) still emits the misleading 'no finalized descriptor for group' error; no PsbtPendingVaultLocked event or unlock-independent index was added.
• mixed-version nodes reject descriptor coordination after upgrade — The recovery descriptor format still uses the new older()/OP_VERIFY canonical form (keep-bitcoin/src/recovery.rs:306,353) compared by exact string equality (keep-frost-net/src/node/descriptor.rs:983), with no version negotiation added; the code comment confirms v0.3.x is incompatible, so mixed-version pools still reject each other.
• handle_signature_complete skipped participant auth for unknown sessions — keep-frost-net/src/node/signing.rs:1071-1076 still returns Ok for an unknown session before the participant check (line 1086), so the described silent-accept-for-unknown-session behavior is unchanged.
• xpub prefix validation inconsistency between protocol.rs and descriptor_session.rs — Inconsistency remains: protocol.rs:92 is_valid_xpub accepts all four prefixes network-agnostically, while descriptor_session.rs:276-288 add_contribution enforces network-specific prefixes (mainnet only 'xpub'), so a message can pass protocol validation but fail at session level.
• No unit test for reconstruct_descriptor function — reconstruct_descriptor (keep-frost-net/src/descriptor_session.rs:959) still has no dedicated unit test; it is only invoked from production code and the integration test, so the specific error-path unit tests requested do not exist.
• Group pubkey and session ID leaked in unencrypted Nostr event tags — Descriptor coordination events still attach unencrypted g/s/t tags (group pubkey, session id, message type) at keep-frost-net/src/node/descriptor.rs:118-123 and the parallel send paths; the metadata leak is unchanged.
• RefreshRequest handler missing replay window and group_pubkey checks — RefreshRequest/Round1/Round2/Complete are still unvalidated stubs that only log 'Distributed refresh not yet implemented' with no replay/group_pubkey/membership/policy checks (keep-frost-net/src/node/mod.rs:2410-2417)
• Refresh messages bypass trust check in node handler — RefreshRequest is still absent from the trusted-peer bypass allowlist so it cannot be initiated by an untrusted group member (keep-frost-net/src/node/mod.rs:2341-2346)
• RefreshRequest participants validated for max count but not for minimum threshold — validate() still only enforces participants.len()>=2, not >=threshold, and RefreshRequestPayload still carries no threshold field to check against (protocol.rs:441-455, struct at 1544).
• Refresh network messages accepted without replay window check or authentication — The Refresh* message handlers are still a warn-only no-op stub ('Distributed refresh not yet implemented') with no replay-window enforcement, authorization, or rejection (node/mod.rs:2410-2417).
• RefreshRound2 target_index not validated — validate() now checks target_index != 0 and target_index != share_index but still cannot cross-validate target_index against the RefreshRequest participant list, and the node handler remains a no-op stub that never performs that check (protocol.rs:477-482, node/mod.rs:2410-2417).
• No pin expiration or maximum age mechanism — Pins are still stored as HashMap<String, Vec> with no pinned_at timestamp or expiration/max-age (cert_pin.rs:33).
• Hostname normalization missing - case-sensitive pin lookups — Hostname from url.host_str() is still used directly with no explicit .to_ascii_lowercase() normalization for defense-in-depth (cert_pin.rs:166-169).
• cert_pin.rs missing #![forbid(unsafe_code)] attribute — cert_pin.rs still has no #![forbid(unsafe_code)] attribute; grep for 'forbid' in the file returns nothing.
• // URL port defaults to 443 via unwrap_or but may not resolve for TLS connect — Port is still derived via url.port_or_known_default().unwrap_or(443) with the same wss/https scheme mapping the finding described; behavior unchanged (cert_pin.rs:171).

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

[kwsantiago]

Triage pass (verified against current code). Dispositions:

Being addressed (PR #856):

• no unit test for reconstruct_descriptor — added two error-path tests (invalid network, missing-contribution).

Already fixed (verified, postdates this issue):

• OPRF auto-approve verifying_share binding — implemented in keep-frost-net: bind an announced share index to the group's canonical verifying share #815 (handle_announce, node/mod.rs:2582-2611): the announced verifying_share is compared against the group's canonical pubkey_package.verifying_shares()[index] at admission, backed by the regression test handle_announce_rejects_non_canonical_verifying_share. Adding the check to verify_peer_share_index would be redundant (peers are already bound before it runs). Residual: an imported compact/bech32 share carries only its own verifying share, so an imported holder can enforce the binding only for indices it holds or when exported with the full pubkey package; closing that fully touches the export format (design follow-up).
• canonical hash duplication — verify_descriptor_hash now calls the shared keep_core::wallet::canonical_descriptor_hash.
• hostname normalization — normalize_pin_hostname canonicalizes via url IDNA-lowercasing; verification uses the already-lowercased host_str.
• cert_pin.rs missing #![forbid(unsafe_code)] — false: lib.rs:57 has a crate-level #![forbid(unsafe_code)] covering every module.

Not a defect (verified):

• handle_signature_complete unknown-session — returning Ok and ignoring a SignatureComplete for an unknown session is correct (no local state to accept); known sessions are guarded by the participant check that returns UntrustedPeer for a non-participant sender.
• PSBT finalize set_finalized-after-broadcast — the code does set_finalized BEFORE building/broadcasting PsbtFinalize (comment: 'Local state is already committed; notifying peers is best-effort'); on set_finalized error it returns early and never broadcasts, so peers cannot finalize while local errors out.

Remaining (keeps this open): design/feature work — the imported-compact-share full-package binding (export-format decision), the unimplemented distributed-refresh handlers (RefreshRequest/Round1/Round2/Complete validation is moot until implemented), mixed-version descriptor negotiation, pin expiration/max-age, the g/s/t tag metadata leak, an absolute per-input OPRF eval cap (#833); plus the announce single-flight shutdown-abort test.