From 878f7728ebe6accd4bf4892b183f1e0f50a46870 Mon Sep 17 00:00:00 2001 From: 2ro <17595647+2ro@users.noreply.github.com> Date: Fri, 12 Jun 2026 12:48:34 -0400 Subject: [PATCH] Build 34: harden the nostr ingest path (audit Medium items) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - N-F1: commit the wrap/rumor/slate dedup markers immediately after the durable receive/finalize, before the reply+sync tail, so a crash there can't re-trigger the action on catch-up. (grin + decide() already backstopped it; this closes the window cleanly.) - N-F3: prune the processed-dedup store hourly in the heartbeat, not only at startup — a long-lived session could otherwise grow it unbounded under fresh-keypair spam since the 30-day TTL never re-applied. - N-F2: kept Created/SendFailed in the finalize allow-set (removing them would strand a real send whose S1 reached the peer before we persisted AwaitingS2) and documented why it is not a forgery vector; added a test. - Update replay_check e2e: a same-pubkey second register is now blocked by the name-change cooldown (fires before the one-name rule); accept either. Validated: 35 lib tests + live nip17_slatepack_roundtrip + replay_check green. --- src/nostr/client.rs | 22 ++++++++++++++++++++++ src/nostr/ingest.rs | 24 ++++++++++++++++++++++++ tests/replay_check.rs | 13 ++++++++++--- 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/nostr/client.rs b/src/nostr/client.rs index 70043781..edd83e90 100644 --- a/src/nostr/client.rs +++ b/src/nostr/client.rs @@ -450,6 +450,10 @@ async fn run_service(svc: Arc, wallet: Wallet) { svc.store.prune_processed(); let mut notifications = client.notifications(); + // Re-run TTL pruning periodically, not just at startup: a session that + // never restarts would otherwise let the processed-dedup store grow + // unbounded under fresh-keypair spam (the 30-day TTL never applied). + let mut last_prune = unix_time(); loop { if svc.shutdown.load(Ordering::SeqCst) || !wallet.is_open() { break; @@ -473,6 +477,10 @@ async fn run_service(svc: Arc, wallet: Wallet) { let connected = client.relays().await.values() .any(|r| r.status() == RelayStatus::Connected); svc.connected.store(connected, Ordering::Relaxed); + if unix_time() - last_prune >= 3600 { + svc.store.prune_processed(); + last_prune = unix_time(); + } } } } @@ -704,6 +712,13 @@ async fn handle_wrap(svc: &Arc, wallet: &Wallet, client: &Client, created_at: now, updated_at: now, }); + // Commit dedup markers now the receive is durable, BEFORE + // the reply + sync tail. A crash there must not let this + // wrap re-trigger a second receive on catch-up (decide() + // and grin's TransactionAlreadyReceived also backstop it). + svc.store.mark_processed(&wrap_id); + svc.store.mark_processed(&rumor_id); + svc.store.mark_processed(&slate_marker); match svc .send_payment_dm(&sender_hex, &reply_text, None, &[]) .await @@ -744,6 +759,13 @@ async fn handle_wrap(svc: &Arc, wallet: &Wallet, client: &Client, Ok(()) => { svc.store .update_tx_status(&slate.id.to_string(), NostrSendStatus::Finalized); + // Finalize+post committed; mark dedup before the sync tail so a + // crash can't re-finalize on catch-up (grin rejects a second + // finalize and the meta is now Finalized, which decide() drops — + // this just avoids the redundant attempt). + svc.store.mark_processed(&wrap_id); + svc.store.mark_processed(&rumor_id); + svc.store.mark_processed(&slate_marker); if let Some(mut contact) = svc.store.contact(&sender_hex) { contact.last_paid_at = Some(unix_time()); svc.store.save_contact(&contact); diff --git a/src/nostr/ingest.rs b/src/nostr/ingest.rs index 257f827f..8bf8c32f 100644 --- a/src/nostr/ingest.rs +++ b/src/nostr/ingest.rs @@ -80,6 +80,16 @@ pub fn decide(ctx: &IngestContext) -> IngestDecision { AcceptPolicy::Ask => IngestDecision::SurfaceIncoming, } } + // Standard2 is the counterparty's reply to a send WE initiated. The + // status allow-set below includes Created/SendFailed (not just + // AwaitingS2) on purpose: our send records intent as Created BEFORE + // dispatch and only flips to AwaitingS2 after a relay accepts S1, so + // a crash in that gap leaves a legitimate pending send marked + // Created/SendFailed even though the counterparty did receive S1. + // This is NOT a forgery vector: a finalizing S2 must carry our S1 + // partial signature over our locked outputs, which the counterparty + // can only have if we actually sent it. Sender + tx_exists are still + // required, and grin rejects finalizing an already-finalized tx. SlateState::Standard2 => match ctx.meta { Some(meta) if meta.direction == NostrTxDirection::Sent @@ -251,6 +261,20 @@ mod tests { assert!(matches!(decide(&c), IngestDecision::Drop(_))); } + #[test] + fn s2_finalizes_from_pre_dispatch_states() { + // Created/SendFailed are deliberately accepted: a crash between + // relay-accept and the AwaitingS2 write must not strand a real send. + for status in [NostrSendStatus::Created, NostrSendStatus::SendFailed] { + let m = meta(NostrTxDirection::Sent, status, ALICE); + let c = ctx(SlateState::Standard2, 100, ALICE, Some(&m), true); + assert_eq!(decide(&c), IngestDecision::FinalizePost); + // Still bound to the counterparty: a stranger's S2 drops. + let c2 = ctx(SlateState::Standard2, 100, MALLORY, Some(&m), true); + assert!(matches!(decide(&c2), IngestDecision::Drop(_))); + } + } + #[test] fn i1_never_pays_automatically() { // Even from a contact under the most permissive policy. diff --git a/tests/replay_check.rs b/tests/replay_check.rs index f1332d5c..f01243b2 100644 --- a/tests/replay_check.rs +++ b/tests/replay_check.rs @@ -56,7 +56,12 @@ async fn replay_and_double_name_rejected() { "replay should be rejected, got: {r2}" ); - // Second DISTINCT name with a FRESH signature but same pubkey -> one-name rule. + // Second DISTINCT name with a FRESH signature but same pubkey -> blocked. + // Two protections can fire here: the per-pubkey name-change cooldown (one + // change per 10 min, which the just-completed register of name_a triggers) + // and the one-active-name-per-pubkey rule. The cooldown is checked first, so + // within 10 min of a successful register a same-pubkey second register is + // rejected with name_change_cooldown; either rejection is a valid block. let name_b = format!("s{}", &pk[..7]); let body_b = serde_json::json!({"name": name_b, "pubkey": pk}).to_string(); let ph_b = hex::encode(Sha256::digest(body_b.as_bytes())); @@ -73,8 +78,10 @@ async fn replay_and_double_name_rejected() { let r3 = post(&auth_b, &body_b); println!("2nd name: {r3}"); assert!( - r3.contains("already has a name") || r3.contains("pubkey already"), - "one-name rule should block, got: {r3}" + r3.contains("already has a name") + || r3.contains("pubkey already") + || r3.contains("name_change_cooldown"), + "a same-pubkey second name should be blocked (one-name rule or cooldown), got: {r3}" ); // Cleanup name A.