From 3dd5d7c7c3956e53fb295f10e439a13826523aa1 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Wed, 17 Jun 2026 09:39:44 +0000 Subject: [PATCH] fix(nip09): bind deletion tombstones to author to prevent cross-author censorship A kind-5 deletion that arrives before its target laid down an author-agnostic tombstone: is_event_deleted matched any kind-5 carrying the e-tag id, and is_coordinate_deleted matched any kind-5 carrying the a-tag coordinate, regardless of who signed the deletion. This let any pubkey pre-emptively censor another's events by racing a kind-5 ahead of the target (out-of-order delete). Bind both checks to the rightful author: - is_event_deleted now takes the candidate event's author and only honours a kind-5 signed by that same pubkey; - is_coordinate_deleted requires the kind-5 author to match the pubkey embedded in the coordinate (::). Add integration test out_of_order_deletion_by_other_pubkey_not_actioned and unit tests covering both the e-tag and a-tag cross-author cases. --- src/nostr/builder.rs | 10 +++- src/nostr/policy/deletion.rs | 4 +- src/nostr/tombstones.rs | 89 ++++++++++++++++++++++++++--- tests/nip09_validation.rs | 106 +++++++++++++++++++++++++++++++---- 4 files changed, 185 insertions(+), 24 deletions(-) diff --git a/src/nostr/builder.rs b/src/nostr/builder.rs index e13a682..83e0bdd 100644 --- a/src/nostr/builder.rs +++ b/src/nostr/builder.rs @@ -634,8 +634,14 @@ impl Nip34WritePolicy { return Some(reject_invalid("this pubkey has requested to vanish")); } - // 2. Deleted event id - if self.ctx.tombstones.is_event_deleted(&event.id).await { + // 2. Deleted event id (author-bound: only the event's own author may + // have deleted it) + if self + .ctx + .tombstones + .is_event_deleted(&event.id, &event.pubkey) + .await + { tracing::debug!( event_id = %event.id.to_hex(), "Rejected re-submission of deleted event" diff --git a/src/nostr/policy/deletion.rs b/src/nostr/policy/deletion.rs index 33b7348..f9f31b1 100644 --- a/src/nostr/policy/deletion.rs +++ b/src/nostr/policy/deletion.rs @@ -728,7 +728,9 @@ mod tests { assert!(matches!(result, WritePolicyResult::Accept)); // No tombstone recorded -> re-submission of the target is NOT blocked. assert!( - !ctx.tombstones.is_event_deleted(&target).await, + !ctx.tombstones + .is_event_deleted(&target, &keys.public_key()) + .await, "Disrespector mode must NOT record a deletion tombstone" ); } diff --git a/src/nostr/tombstones.rs b/src/nostr/tombstones.rs index ed70622..fbf396d 100644 --- a/src/nostr/tombstones.rs +++ b/src/nostr/tombstones.rs @@ -131,14 +131,21 @@ impl Tombstones { Ok(()) } - /// Has this event id been deleted by a recorded kind-5 (`e` tag)? + /// Has this event id been deleted by a recorded kind-5 (`e` tag) authored by + /// `author`? /// /// Mirrors the backend's `is_deleted` check that previously fed the - /// relay-builder `check_id` gate. - pub async fn is_event_deleted(&self, id: &EventId) -> bool { - // Any stored kind-5 carrying this id in an `e` tag means it was deleted. + /// relay-builder `check_id` gate, but with explicit author binding: only the + /// event's own author may delete it (NIP-09). A deletion request signed by a + /// different pubkey must NOT block (re-)submission of `author`'s event — + /// otherwise any pubkey could pre-emptively censor another's events by + /// racing a kind-5 ahead of the target (out-of-order delete). + pub async fn is_event_deleted(&self, id: &EventId, author: &PublicKey) -> bool { + // A stored kind-5 authored by `author` carrying this id in an `e` tag + // means `author` deleted their own event. let filter = Filter::new() .kind(Kind::EventDeletion) + .author(*author) .custom_tag(SingleLetterTag::lowercase(Alphabet::E), id.to_hex()); match self.db.query(filter).await { Ok(events) => !events.is_empty(), @@ -159,19 +166,31 @@ impl Tombstones { /// to the deletion request's `created_at`. So a candidate event is blocked /// only if a recorded deletion for the same coordinate has /// `deletion.created_at >= event_created_at`. + /// + /// Author binding: a coordinate is `::`, so only the pubkey + /// embedded in the coordinate may delete it. A kind-5 signed by a different + /// pubkey referencing this coordinate is ignored, otherwise any pubkey could + /// pre-emptively censor another's addressable events. pub async fn is_coordinate_deleted( &self, coordinate: &str, event_created_at: Timestamp, ) -> bool { + // The coordinate owner (pubkey hex) is the only party allowed to delete + // it. Extract it so we can require the kind-5 author to match. + let coord_owner_hex = coordinate.split(':').nth(1); + let filter = Filter::new().kind(Kind::EventDeletion).custom_tag( SingleLetterTag::lowercase(Alphabet::A), coordinate.to_string(), ); match self.db.query(filter).await { - Ok(events) => events - .iter() - .any(|deletion| deletion.created_at >= event_created_at), + Ok(events) => events.iter().any(|deletion| { + deletion.created_at >= event_created_at + && coord_owner_hex + .map(|owner| owner == deletion.pubkey.to_hex()) + .unwrap_or(false) + }), Err(e) => { tracing::error!(error = %e, "Tombstone query failed for is_coordinate_deleted"); false @@ -212,12 +231,39 @@ mod tests { let keys = Keys::generate(); let target = EventId::all_zeros(); - assert!(!store.is_event_deleted(&target).await); + assert!(!store.is_event_deleted(&target, &keys.public_key()).await); let deletion = deletion_by_event(&keys, target); store.record_deletion(&deletion).await.unwrap(); - assert!(store.is_event_deleted(&target).await); + assert!(store.is_event_deleted(&target, &keys.public_key()).await); + } + + #[tokio::test] + async fn deletion_does_not_block_a_different_authors_event() { + // A kind-5 from pubkey B targeting an event id must NOT mark that id as + // deleted for a different author A: only the event's own author may + // delete it (NIP-09). Guards against pre-emptive cross-author censorship + // when the deletion races ahead of the target (out-of-order delete). + let store = Tombstones::in_memory(); + let attacker = Keys::generate(); + let victim = Keys::generate(); + let target = EventId::all_zeros(); + + let deletion = deletion_by_event(&attacker, target); + store.record_deletion(&deletion).await.unwrap(); + + // The attacker "deleted" it for themselves... + assert!( + store + .is_event_deleted(&target, &attacker.public_key()) + .await + ); + // ...but the victim's identical id is NOT considered deleted. + assert!( + !store.is_event_deleted(&target, &victim.public_key()).await, + "a deletion from another pubkey must not block the victim's event" + ); } #[tokio::test] @@ -252,6 +298,31 @@ mod tests { ); } + #[tokio::test] + async fn coordinate_deletion_by_other_pubkey_is_ignored() { + // A kind-5 from pubkey B carrying an `a` tag for a coordinate owned by + // pubkey A must NOT mark that coordinate as deleted: only the pubkey + // embedded in the coordinate may delete it. + let store = Tombstones::in_memory(); + let victim = Keys::generate(); + let attacker = Keys::generate(); + let coord = format!("30618:{}:my-repo", victim.public_key().to_hex()); + + let deletion = EventBuilder::new(Kind::EventDeletion, "") + .tags(vec![Tag::custom("a", vec![coord.clone()])]) + .custom_created_at(Timestamp::from_secs(1000)) + .finalize(&attacker) + .unwrap(); + store.record_deletion(&deletion).await.unwrap(); + + assert!( + !store + .is_coordinate_deleted(&coord, Timestamp::from_secs(1000)) + .await, + "a coordinate deletion signed by a different pubkey must be ignored" + ); + } + #[tokio::test] async fn detects_pubkey_vanish() { let store = Tombstones::in_memory(); diff --git a/tests/nip09_validation.rs b/tests/nip09_validation.rs index 9836592..936de08 100644 --- a/tests/nip09_validation.rs +++ b/tests/nip09_validation.rs @@ -278,10 +278,9 @@ async fn deletion_of_nonexistent_event_is_accepted() { .expect("create audit client"); // A random id the relay has never stored. - let nonexistent = EventId::from_hex( - "0000000000000000000000000000000000000000000000000000000000000001", - ) - .expect("parse nonexistent id"); + let nonexistent = + EventId::from_hex("0000000000000000000000000000000000000000000000000000000000000001") + .expect("parse nonexistent id"); let deletion = build_deletion(&client, &[nonexistent], &[]); @@ -317,7 +316,12 @@ async fn deletion_before_target_rejects_later_submission() { let (announcement, _repo_id) = publish_served_repo(&client, "delete-first").await; let issue = client - .create_issue(&announcement, "Issue deleted before it arrives", "delete me", vec![]) + .create_issue( + &announcement, + "Issue deleted before it arrives", + "delete me", + vec![], + ) .expect("build issue"); let issue_id = issue.id; @@ -371,6 +375,84 @@ async fn deletion_before_target_rejects_later_submission() { ); } +/// 2c. Out-of-order deletion by a DIFFERENT pubkey must NOT be actioned. +/// +/// Author B sends a kind-5 targeting (by `e` tag) an event id that the relay +/// has never seen — an id that will later belong to author A's event. Because +/// B is not the author of the target, the deletion must not lay down a +/// tombstone that censors A: when A subsequently submits the genuine event it +/// must be accepted and served. +/// +/// This is the cross-author counterpart of +/// `deletion_before_target_rejects_later_submission`. The earlier +/// `deletion_author_mismatch_rejected` test only covers the case where the +/// target already exists; this one covers the harder out-of-order case where B +/// races ahead of A and could otherwise pre-emptively block A's event. +#[tokio::test] +async fn out_of_order_deletion_by_other_pubkey_not_actioned() { + let relay = TestRelay::start().await; + + let author_a = AuditClient::new(relay.url(), AuditConfig::isolated()) + .await + .expect("create author A client"); + let author_b = AuditClient::new(relay.url(), AuditConfig::isolated()) + .await + .expect("create author B client"); + + // A promotes a repo and builds (but does NOT yet send) an issue, so we know + // the id of A's future event. + let (announcement_a, _repo_id) = publish_served_repo(&author_a, "other-pubkey-race").await; + let issue = author_a + .create_issue(&announcement_a, "A's issue", "owned by A", vec![]) + .expect("build issue"); + let issue_id = issue.id; + + // Sanity: the relay has never seen the issue. + assert!( + !author_a + .is_event_on_relay(issue_id) + .await + .expect("query issue before anything is sent"), + "issue should not be served before it is ever submitted" + ); + + // B races ahead and sends a deletion targeting A's not-yet-seen event id. + // The relay cannot yet know the author of the target. Whether the relay + // accepts B's kind-5 (as a pre-emptive no-op) or rejects it outright, the + // critical requirement is that it MUST NOT lay down a tombstone that blocks + // A's genuine event. + let deletion = build_deletion(&author_b, &[issue_id], &[]); + let _ = author_b.send_event(deletion).await; + + tokio::time::sleep(Duration::from_millis(200)).await; + + // A now submits the genuine event. It must be accepted: B's deletion of an + // event B does not own must never have been actioned. + let result = author_a.send_event(issue.clone()).await; + + tokio::time::sleep(Duration::from_millis(200)).await; + + let served = author_a + .is_event_on_relay(issue_id) + .await + .expect("query A's issue after submission"); + + relay.stop().await; + + assert!( + result.is_ok(), + "A's genuine event must be accepted: a deletion from another pubkey \ + must not block it, but submission was rejected: {:?}", + result.err() + ); + assert!( + served, + "A's issue {} must be served: a deletion request from another pubkey \ + must not be actioned against A's events", + issue_id + ); +} + /// 3. Author B cannot delete an event owned by author A: the relay rejects the /// kind-5 with a message mentioning the authorship problem. #[tokio::test] @@ -583,9 +665,9 @@ async fn deletion_invalid_address_format_handled_gracefully() { .expect("create audit client"); let malformed = [ - "invalid:format", // only two parts - "notanumber:pk:id", // unparseable kind - "30617:badhex:id", // unparseable pubkey hex + "invalid:format", // only two parts + "notanumber:pk:id", // unparseable kind + "30617:badhex:id", // unparseable pubkey hex ]; for coord in malformed { @@ -612,10 +694,10 @@ async fn deletion_invalid_address_format_handled_gracefully() { // prove it by round-tripping a normal acceptance. let probe = build_deletion( &client, - &[EventId::from_hex( - "0000000000000000000000000000000000000000000000000000000000000002", - ) - .expect("parse probe id")], + &[ + EventId::from_hex("0000000000000000000000000000000000000000000000000000000000000002") + .expect("parse probe id"), + ], &[], ); let probe_result = client.send_event(probe).await;