fix(desktop): harden private-config tombstones and equal-second retention convergence

Three review fixes for relay-primary private managed-agent config:

- Tombstone owner-binding (review issue 4): resolve_inbound_tombstone now
  rejects any kind:5 whose signer is not the workspace owner, in Rust,
  so a foreign-owner tombstone with a colliding d-tag cannot delete
  local state through direct IPC.

- Tombstone-aware head deletion (review issue 2): the covered retained
  kind:30177/30179 heads are deleted in the same transaction as the
  kind:5 retain, so boot rehydration can no longer resurrect a deleted
  secret-bearing agent. A head strictly newer than the tombstone is a
  recreation/late replay and is preserved (NIP-09 bound).

- Equal-second convergence (review issue 3): inbound_event_outcome
  applies the NIP-01 addressable-event tiebreak at equal created_at —
  inbound wins only with a lexicographically lower event id — so two
  devices authoring same-second successors converge on the relay's
  winner instead of diverging forever. A losing pending local edit has
  pending_sync cleared since the relay would refuse it anyway.

Co-authored-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
This commit is contained in:
Brain
2026-08-20 13:01:20 -06:00
parent a6d4b30a1c
commit 6c7bb84f01
3 changed files with 426 additions and 44 deletions

View File

@@ -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<Option<(u32, String)>, 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`.

View File

@@ -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.

View File

@@ -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<String> {
serde_json::from_str::<serde_json::Value>(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();