diff --git a/desktop/src-tauri/src/commands/personas/inbound.rs b/desktop/src-tauri/src/commands/personas/inbound.rs index b6e4b845da..7b13473463 100644 --- a/desktop/src-tauri/src/commands/personas/inbound.rs +++ b/desktop/src-tauri/src/commands/personas/inbound.rs @@ -487,59 +487,61 @@ fn parse_deletion_coordinate(event: &nostr::Event) -> Option<(u32, String)> { }) } -/// Apply an inbound kind:5 NIP-09 deletion: remove the local record at the -/// tombstone's target coordinate, scoped per-kind. Mirrors the upsert spine — -/// arrival-scoped retention resolution under the store lock, then a per-kind -/// store mutation — but removes rather than patches. Unknown/malformed -/// coordinates no-op, as does a tombstone whose arrival community is no longer -/// active. -fn reconcile_inbound_tombstone( +/// Resolve an inbound kind:5 tombstone against the scope owner and the +/// retention store: authorize it, retain it, and return the +/// `(target_kind, target_d_tag)` the caller must delete locally — or `None` +/// when the tombstone must no-op. +/// +/// Authorization is TWO bindings, both enforced in Rust because the frontend +/// owner filter reads attacker-controlled fields and callable IPC bypasses it +/// anyway: `parse_deletion_coordinate` proves the signer owns the coordinate +/// it NAMES, and the check here proves that signer is the active workspace +/// owner. The local stores match by d-tag alone, so without the second +/// binding a validly signed foreign-owner tombstone naming its OWN coordinate +/// with a colliding d-tag would delete this owner's record. +/// +/// The covered retained upsert head is deleted in the SAME transaction as the +/// kind:5 retain, mirroring the local delete path +/// (`tombstone_managed_agent_pending`). Without this the retained kind:30179 +/// head survives the tombstone and the next boot's `hydrate_from_retention` +/// rematerializes the deleted secret-bearing config into the overlay +/// (head → tombstone → restart resurrection). +fn resolve_inbound_tombstone( event: &nostr::Event, - arrival_relay_url: &str, - app: &AppHandle, - state: &AppState, -) -> Result<(), String> { - use crate::managed_agents::{ - load_managed_agents, load_teams, - retention::{ - open_retention_db, retain_inbound_event, tombstone_retention_d_tag, InboundOutcome, - RetainedEvent, - }, - save_managed_agents, save_teams, + scope_owner: &nostr::PublicKey, + conn: &rusqlite::Connection, +) -> Result, String> { + use crate::managed_agents::retention::{ + delete_retained_event, get_retained_event, retain_inbound_event, tombstone_retention_d_tag, + InboundOutcome, RetainedEvent, }; use buzz_core_pkg::kind::{ KIND_DELETION, KIND_MANAGED_AGENT, KIND_PERSONA, KIND_PRIVATE_MANAGED_AGENT, KIND_TEAM, }; use nostr::JsonUtil; + if event.pubkey != *scope_owner { + return Ok(None); // foreign-owner tombstone: not authorized in this scope + } let Some((target_kind, target_d_tag)) = parse_deletion_coordinate(event) else { - return Ok(()); // no routable coordinate — nothing to delete + return Ok(None); // no routable coordinate — nothing to delete }; if !matches!( target_kind, KIND_PERSONA | KIND_TEAM | KIND_MANAGED_AGENT | KIND_PRIVATE_MANAGED_AGENT ) { - return Ok(()); // deletion for a kind we don't track locally + return Ok(None); // deletion for a kind we don't track locally } - let _store_guard = state - .managed_agents_store_lock - .lock() - .map_err(|error| error.to_string())?; - // Resolve against the retained tombstone row (keyed by the target // coordinate, F2c) so a re-received tombstone or one older than a pending - // local edit is a no-op. Scoped to the arrival community, so a workspace - // switch since arrival drops the tombstone instead of retaining it — and - // deleting a record — in the wrong community's store. - let Some(scope) = - crate::managed_agents::retention::arrival_retention_scope(app, state, arrival_relay_url)? - else { - return Ok(()); - }; - let conn = open_retention_db(&scope.db_path)?; + // local edit is a no-op. Retain + head delete commit atomically so a crash + // between them cannot leave a consumed tombstone with a live head. + let transaction = conn + .unchecked_transaction() + .map_err(|error| format!("failed to begin inbound tombstone transaction: {error}"))?; let outcome = retain_inbound_event( - &conn, + &transaction, &RetainedEvent { kind: KIND_DELETION, pubkey: event.pubkey.to_hex(), @@ -551,8 +553,85 @@ fn reconcile_inbound_tombstone( }, )?; if outcome == InboundOutcome::Skipped { - return Ok(()); + return Ok(None); } + // The two agent kinds are one record locally (the match arm below removes + // the disk record AND the overlay entry for either), so both retained + // heads are covered — the local delete path tombstones both coordinates, + // but the sibling kind:5 may be lost or unsent, and a surviving 30179 head + // would rematerialize the agent at the next boot hydration. + let covered_kinds: &[u32] = + if matches!(target_kind, KIND_MANAGED_AGENT | KIND_PRIVATE_MANAGED_AGENT) { + &[KIND_MANAGED_AGENT, KIND_PRIVATE_MANAGED_AGENT] + } else { + std::slice::from_ref(&target_kind) + }; + // NIP-09: a deletion covers events up to its own `created_at`. A retained + // head NEWER than the tombstone means the record was recreated after the + // deletion (or the tombstone is a late replay) — keep the record and its + // heads. The kind:5 row retained above still dedupes future replays. + let owner_hex = event.pubkey.to_hex(); + let tombstone_created_at = event.created_at.as_secs() as i64; + for covered_kind in covered_kinds { + if let Some(head) = + get_retained_event(&transaction, *covered_kind, &owner_hex, &target_d_tag)? + { + if head.created_at > tombstone_created_at { + transaction.commit().map_err(|error| { + format!("failed to commit inbound tombstone transaction: {error}") + })?; + return Ok(None); + } + } + } + for covered_kind in covered_kinds { + delete_retained_event(&transaction, *covered_kind, &owner_hex, &target_d_tag)?; + } + transaction + .commit() + .map_err(|error| format!("failed to commit inbound tombstone transaction: {error}"))?; + Ok(Some((target_kind, target_d_tag))) +} + +/// Apply an inbound kind:5 NIP-09 deletion: remove the local record at the +/// tombstone's target coordinate, scoped per-kind. Mirrors the upsert spine — +/// arrival-scoped retention resolution under the store lock, then a per-kind +/// store mutation — but removes rather than patches. Unknown/malformed +/// coordinates no-op, as does a tombstone whose arrival community is no longer +/// active or whose signer is not the scope owner. +fn reconcile_inbound_tombstone( + event: &nostr::Event, + arrival_relay_url: &str, + app: &AppHandle, + state: &AppState, +) -> Result<(), String> { + use crate::managed_agents::{ + load_managed_agents, load_teams, retention::open_retention_db, save_managed_agents, + save_teams, + }; + use buzz_core_pkg::kind::{ + KIND_MANAGED_AGENT, KIND_PERSONA, KIND_PRIVATE_MANAGED_AGENT, KIND_TEAM, + }; + + let _store_guard = state + .managed_agents_store_lock + .lock() + .map_err(|error| error.to_string())?; + + // Scoped to the arrival community, so a workspace switch since arrival + // drops the tombstone instead of retaining it — and deleting a record — + // in the wrong community's store. + let Some(scope) = + crate::managed_agents::retention::arrival_retention_scope(app, state, arrival_relay_url)? + else { + return Ok(()); + }; + let conn = open_retention_db(&scope.db_path)?; + let Some((target_kind, target_d_tag)) = + resolve_inbound_tombstone(event, &scope.owner_keys.public_key(), &conn)? + else { + return Ok(()); + }; // Remove the local record using the SAME per-kind match rule the apply fns // use: persona by `persona_d_tag`, team by `id`, managed-agent by `pubkey`. diff --git a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs index b19f779fdb..35ee9d8a8a 100644 --- a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs @@ -924,6 +924,167 @@ fn owned_deletion_event(kind: u32, d_tag: &str) -> nostr::Event { deletion_event_with_keys(&format!("{kind}:{owner}:{d_tag}"), &keys) } +/// Like [`deletion_event_with_keys`] but with a controlled `created_at`, for +/// tests that race a tombstone against a retained head's timestamp. +fn owned_deletion_event_at( + keys: &nostr::Keys, + kind: u32, + d_tag: &str, + created_at: u64, +) -> nostr::Event { + use nostr::{EventBuilder, JsonUtil, Kind, Tag, Timestamp}; + let owner = keys.public_key().to_hex(); + let event = EventBuilder::new(Kind::Custom(5), "") + .tags(vec![ + Tag::parse(["a", &format!("{kind}:{owner}:{d_tag}")]).unwrap() + ]) + .custom_created_at(Timestamp::from(created_at)) + .sign_with_keys(keys) + .unwrap(); + nostr::Event::from_json(event.as_json()).unwrap() +} + +// ── resolve_inbound_tombstone: authorization + retained-head coverage ────── +// +// The seam is deliberately a pure function over (event, scope_owner, conn) so +// the security decision is testable WITHOUT a live AppHandle — the production +// caller (`reconcile_inbound_tombstone`) is a `#[tauri::command]` body that +// only adds scope resolution and the per-kind store mutation around it. These +// are the direct-IPC regressions: the frontend owner filter never runs here. + +/// Issue-4 regression: a VALIDLY SIGNED tombstone whose signer owns the +/// coordinate it names — so it clears `parse_deletion_coordinate` — but who is +/// not the workspace owner must no-op. Local stores match by d-tag alone, so +/// without the scope-owner binding this would delete the victim's record. +#[test] +fn resolve_inbound_tombstone_rejects_foreign_owner() { + use crate::managed_agents::retention::open_retention_db; + use std::path::Path; + + let conn = open_retention_db(Path::new(":memory:")).unwrap(); + let scope_owner_keys = nostr::Keys::generate(); + let attacker_keys = nostr::Keys::generate(); + + // The attacker names their OWN coordinate (passes the NIP-09 signer == + // coordinate-owner check) with a d-tag colliding with the victim's agent. + let forged = owned_deletion_event_at(&attacker_keys, 30177, AGENT_PUBKEY, 100); + let resolved = + resolve_inbound_tombstone(&forged, &scope_owner_keys.public_key(), &conn).unwrap(); + assert_eq!(resolved, None, "foreign-owner tombstone must not authorize"); + + // Nothing was retained: the attacker cannot even park a kind:5 row in the + // victim's scoped store. + assert!( + !crate::managed_agents::retention::has_retained_personas( + &conn, + &attacker_keys.public_key().to_hex() + ) + .unwrap(), + "a rejected tombstone must leave no retained row" + ); + + // POSITIVE CONTROL: the same event shape signed by the scope owner + // resolves — proving the rejection above is the owner binding, not a + // broken fixture. + let owned = owned_deletion_event_at(&scope_owner_keys, 30177, AGENT_PUBKEY, 100); + assert_eq!( + resolve_inbound_tombstone(&owned, &scope_owner_keys.public_key(), &conn).unwrap(), + Some((30177, AGENT_PUBKEY.to_string())), + "control: the scope owner's tombstone authorizes" + ); +} + +/// Issue-2 regression: head → tombstone → restart. The tombstone must remove +/// the retained kind:30179 head so a fresh boot's `hydrate_from_retention` +/// cannot rematerialize the deleted secret-bearing agent. +#[test] +fn tombstoned_private_head_does_not_survive_restart_hydration() { + use crate::managed_agents::{ + private_config_overlay::{hydrate_from_retention, PrivateConfigOverlay}, + retention::open_retention_db, + }; + use buzz_core_pkg::private_managed_agent; + use tempfile::TempDir; + + let dir = TempDir::new().unwrap(); + let db_path = dir.path().join("retention.db"); + let owner_keys = nostr::Keys::generate(); + let agent_keys = nostr::Keys::generate(); + let agent_hex = agent_keys.public_key().to_hex(); + + // Session 1: a private head lands and is retained durably. + { + let conn = open_retention_db(&db_path).unwrap(); + let payload = private_agent_payload(&owner_keys, &agent_keys, "doomed", 4); + let event = private_managed_agent::build_event(&owner_keys, &payload, 20).unwrap(); + let mut overlay = PrivateConfigOverlay::default(); + apply_inbound_private_managed_agent_event(&event, &owner_keys, &conn, &mut overlay) + .unwrap(); + + // The delete arrives: a kind:5 covering the PUBLIC coordinate (either + // agent kind must cover both heads — the sibling tombstone may be + // lost). + let tombstone = owned_deletion_event_at(&owner_keys, 30177, &agent_hex, 30); + let resolved = + resolve_inbound_tombstone(&tombstone, &owner_keys.public_key(), &conn).unwrap(); + assert_eq!(resolved, Some((30177, agent_hex.clone()))); + } + + // Session 2 (restart, fresh overlay): hydration must NOT resurrect it. + let conn = open_retention_db(&db_path).unwrap(); + let overlay = hydrate_from_retention(&conn, &owner_keys).unwrap(); + assert!( + overlay.resolved_records(&[]).is_empty(), + "a tombstoned 30179 head must not rehydrate after restart" + ); + + // Replay: the same tombstone re-received dedupes to a no-op instead of + // re-deleting (retention row already at its created_at). + let replay = owned_deletion_event_at(&owner_keys, 30177, &agent_hex, 30); + assert_eq!( + resolve_inbound_tombstone(&replay, &owner_keys.public_key(), &conn).unwrap(), + None, + "a replayed tombstone must dedupe" + ); +} + +/// NIP-09 bound: a deletion covers events up to its own `created_at`. A head +/// NEWER than the tombstone is a recreation (or the tombstone is a late +/// replay) — the record and its heads must survive. +#[test] +fn tombstone_older_than_the_retained_head_preserves_it() { + use crate::managed_agents::{ + private_config_overlay::{hydrate_from_retention, PrivateConfigOverlay}, + retention::open_retention_db, + }; + use buzz_core_pkg::private_managed_agent; + use std::path::Path; + + let conn = open_retention_db(Path::new(":memory:")).unwrap(); + let owner_keys = nostr::Keys::generate(); + let agent_keys = nostr::Keys::generate(); + let agent_hex = agent_keys.public_key().to_hex(); + + // Head at t=50; tombstone authored earlier at t=30 arrives late. + let payload = private_agent_payload(&owner_keys, &agent_keys, "survivor", 4); + let event = private_managed_agent::build_event(&owner_keys, &payload, 50).unwrap(); + let mut overlay = PrivateConfigOverlay::default(); + apply_inbound_private_managed_agent_event(&event, &owner_keys, &conn, &mut overlay).unwrap(); + + let stale_tombstone = owned_deletion_event_at(&owner_keys, 30179, &agent_hex, 30); + assert_eq!( + resolve_inbound_tombstone(&stale_tombstone, &owner_keys.public_key(), &conn).unwrap(), + None, + "a tombstone older than the head must not delete it" + ); + let overlay = hydrate_from_retention(&conn, &owner_keys).unwrap(); + assert_eq!( + overlay.resolved_records(&[]).len(), + 1, + "the newer head survives the stale tombstone" + ); +} + #[test] fn parse_deletion_coordinate_extracts_kind_and_d_tag() { // Persona / team / agent coordinates all route by their leading kind. diff --git a/desktop/src-tauri/src/managed_agents/retention.rs b/desktop/src-tauri/src/managed_agents/retention.rs index f669c210c0..4bd8f7b4a5 100644 --- a/desktop/src-tauri/src/managed_agents/retention.rs +++ b/desktop/src-tauri/src/managed_agents/retention.rs @@ -255,11 +255,17 @@ pub enum InboundOutcome { /// - No local row, or inbound strictly newer (`created_at >`): apply the /// inbound event, clearing `pending_sync`. Inbound wins; a stale local edit /// the relay already superseded stops republishing instead of looping. -/// - Equal `created_at`: skip. Nostr time is seconds-granularity, so a pending -/// local edit and an inbound event can share a timestamp; applying here would -/// clear `pending_sync` and drop the local publish. Skipping leaves the -/// pending row intact so the flush republishes and the relay resolves -/// last-writer-wins. (A re-received echo at equal time is also a no-op.) +/// - Equal `created_at`: NIP-01 addressable-event tiebreak — the event with +/// the lexicographically LOWEST id wins, exactly the head the relay itself +/// retains. Nostr time is seconds-granularity, so two devices can retain +/// distinct successors in the same second; without a shared deterministic +/// winner each side skips the other's head on every replay and the devices +/// diverge permanently. A pending local edit that WINS the tie stays +/// pending and republishes; one that LOSES is superseded — the relay would +/// refuse it as the head anyway, so clearing its `pending_sync` is what +/// converges both devices onto the relay's answer. (A re-received echo has +/// an equal id and stays a no-op; if either id is unavailable the inbound +/// event is skipped, preserving any pending local publish.) /// - Inbound older: skip — nothing to change. /// /// Decide whether an inbound event is newer than the retained coordinate without @@ -274,12 +280,39 @@ pub fn inbound_event_outcome( Ok(match existing { None => InboundOutcome::Applied, Some(row) if event.created_at > row.created_at => InboundOutcome::Applied, - // Equal or older: skip. Equal time may collide with a pending local - // edit, so we never clear its `pending_sync`; older is stale. + Some(row) + if event.created_at == row.created_at + && equal_second_inbound_wins(&event.raw_event, &row.raw_event) => + { + InboundOutcome::Applied + } + // Older, or an equal-second loser/echo: skip. A pending local edit + // that won (or an undecidable tie) keeps its `pending_sync`. Some(_) => InboundOutcome::Skipped, }) } +/// NIP-01 addressable-event tiebreak at equal `created_at`: the event with the +/// lexicographically lowest id is the head the relay retains. Returns `true` +/// only when BOTH ids are present and the inbound id is strictly lower — +/// an undecidable or equal comparison must not clobber the retained row (or a +/// pending local publish riding on it). +fn equal_second_inbound_wins(inbound_raw: &str, retained_raw: &str) -> bool { + match (raw_event_id(inbound_raw), raw_event_id(retained_raw)) { + (Some(inbound_id), Some(retained_id)) => inbound_id < retained_id, + _ => false, + } +} + +/// Extract the `id` field from a raw event JSON string, if present. +fn raw_event_id(raw_event: &str) -> Option { + serde_json::from_str::(raw_event) + .ok()? + .get("id")? + .as_str() + .map(str::to_owned) +} + pub fn retain_inbound_event( conn: &Connection, event: &RetainedEvent, @@ -853,7 +886,8 @@ mod tests { #[test] fn inbound_equal_second_skips_and_preserves_pending() { let conn = test_db(); - // Pending local edit at t=1000. + // Pending local edit at t=1000. Same raw-event id as the inbound below + // (an echo / undecidable tie), so the tiebreak cannot decide a winner. let local = sample_event(); retain_event(&conn, &local).unwrap(); @@ -877,6 +911,114 @@ mod tests { assert!(row.content.contains("Test")); } + /// Issue-3 regression: two devices retain DISTINCT successors in the same + /// second, then each receives the other's. Without a deterministic + /// equal-second winner both sides skip forever and diverge permanently. + /// The NIP-01 tiebreak (lowest event id wins) makes opposite delivery + /// orders converge on the SAME head — the one the relay itself retains. + #[test] + fn inbound_equal_second_opposite_delivery_orders_converge() { + let event_low = RetainedEvent { + content: r#"{"display_name":"Low"}"#.to_string(), + raw_event: r#"{"id":"0aaa"}"#.to_string(), + pending_sync: false, + ..sample_event() + }; + let event_high = RetainedEvent { + content: r#"{"display_name":"High"}"#.to_string(), + raw_event: r#"{"id":"0bbb"}"#.to_string(), + pending_sync: false, + ..sample_event() + }; + + // Device A: low first, then high. High loses the tie — skipped. + let device_a = test_db(); + assert_eq!( + retain_inbound_event(&device_a, &event_low).unwrap(), + InboundOutcome::Applied + ); + assert_eq!( + retain_inbound_event(&device_a, &event_high).unwrap(), + InboundOutcome::Skipped + ); + + // Device B: high first, then low. Low wins the tie — applied. + let device_b = test_db(); + assert_eq!( + retain_inbound_event(&device_b, &event_high).unwrap(), + InboundOutcome::Applied + ); + assert_eq!( + retain_inbound_event(&device_b, &event_low).unwrap(), + InboundOutcome::Applied + ); + + // Both devices converge on the lexically-lowest id. + for conn in [&device_a, &device_b] { + let row = get_retained_event(conn, 30175, "abc123", "test-persona") + .unwrap() + .unwrap(); + assert!( + row.content.contains("Low"), + "both delivery orders must converge on the lowest event id" + ); + } + } + + /// A pending local edit that WINS the equal-second tie keeps its + /// `pending_sync` (the flush republishes it); one that LOSES is superseded + /// by the relay's head and stops republishing a refused event. + #[test] + fn inbound_equal_second_pending_local_winner_and_loser() { + // Local pending edit with the LOWER id: inbound loses, pending stays. + let conn = test_db(); + let local_low = RetainedEvent { + raw_event: r#"{"id":"0aaa"}"#.to_string(), + ..sample_event() + }; + retain_event(&conn, &local_low).unwrap(); + let inbound_high = RetainedEvent { + content: r#"{"display_name":"Remote"}"#.to_string(), + raw_event: r#"{"id":"0bbb"}"#.to_string(), + pending_sync: false, + ..sample_event() + }; + assert_eq!( + retain_inbound_event(&conn, &inbound_high).unwrap(), + InboundOutcome::Skipped + ); + let row = get_retained_event(&conn, 30175, "abc123", "test-persona") + .unwrap() + .unwrap(); + assert!(row.pending_sync, "the winning local edit keeps its publish"); + + // Local pending edit with the HIGHER id: inbound wins, pending clears. + let conn = test_db(); + let local_high = RetainedEvent { + raw_event: r#"{"id":"0bbb"}"#.to_string(), + ..sample_event() + }; + retain_event(&conn, &local_high).unwrap(); + let inbound_low = RetainedEvent { + content: r#"{"display_name":"Remote"}"#.to_string(), + raw_event: r#"{"id":"0aaa"}"#.to_string(), + pending_sync: false, + ..sample_event() + }; + assert_eq!( + retain_inbound_event(&conn, &inbound_low).unwrap(), + InboundOutcome::Applied + ); + let row = get_retained_event(&conn, 30175, "abc123", "test-persona") + .unwrap() + .unwrap(); + assert!( + !row.pending_sync, + "the losing local edit stops republishing a head the relay refused" + ); + assert!(row.content.contains("Remote")); + } + #[test] fn inbound_strictly_newer_applies_and_clears_pending() { let conn = test_db();