From 6f4507226fbe142a5143bc36eb60005f9656b560 Mon Sep 17 00:00:00 2001 From: Madhur Shrimal <642275+shrimalmadhur@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:18:50 -0700 Subject: [PATCH] fix: preserve self-targeted `p` tags on membership and PR status events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit nostr 0.44's `EventBuilder` silently strips any `p` tag whose value matches the signing pubkey unless the builder opts in via `.allow_self_tagging()`. Buzz treats `p` tags as protocol routing data, not a self-notify privacy scrub, so several builders whose target can legitimately be the signer were shipping events with the target missing. Two concrete failures: - kind 9000/9001 self-add and self-remove are first-class relay paths ("Self-add: always allowed regardless of policy", pinned by `test_nip29_put_user_self_add_bypasses_policy`), but neither SDK nor desktop builder could express them — the relay saw no target and rejected with `invalid: missing p tag`. - `build_merged_status_event` / `build_pull_request_status_event` derive `owner` from the signing key and then p-tag it, so *every* PR lifecycle event (kinds 1630-1633) shipped without its owner `p` tag. This one is silent: readers index those events by `#p` as `recipients`, so the owner just quietly vanished from the participant set. Existing tests sign with a key distinct from the target, so they passed either way. The added regression tests sign with the target's own key and assert the `p` tag survives; each was confirmed to fail before the fix. Co-Authored-By: Claude Signed-off-by: Madhur Shrimal <642275+shrimalmadhur@users.noreply.github.com> --- crates/buzz-sdk/src/builders.rs | 57 ++++++++++++++- .../src/commands/project_git_workflow.rs | 69 +++++++++++++++++++ desktop/src-tauri/src/events.rs | 68 +++++++++++++++++- 3 files changed, 190 insertions(+), 4 deletions(-) diff --git a/crates/buzz-sdk/src/builders.rs b/crates/buzz-sdk/src/builders.rs index 8cc9c8650a..4601deba5b 100644 --- a/crates/buzz-sdk/src/builders.rs +++ b/crates/buzz-sdk/src/builders.rs @@ -562,6 +562,12 @@ pub fn build_profile( } /// Build a NIP-29 add-member event (kind 9000). +/// +/// `.allow_self_tagging()` is required: self-add is a first-class relay path +/// (`side_effects.rs` — "Self-add: always allowed regardless of policy"), so +/// `actor == target` and the `["p", target]` tag matches the signer. nostr +/// 0.44 strips matching `p` tags by default, which would make a self-targeted +/// add reach the relay with no target at all and fail as `missing p tag`. pub fn build_add_member( channel_id: Uuid, target_pubkey: &str, @@ -575,10 +581,16 @@ pub fn build_add_member( if let Some(r) = role { tags.push(tag(&["role", r.as_str()])?); } - Ok(EventBuilder::new(Kind::Custom(9000), "").tags(tags)) + Ok(EventBuilder::new(Kind::Custom(9000), "") + .tags(tags) + .allow_self_tagging()) } /// Build a NIP-29 remove-member event (kind 9001). +/// +/// Self-remove is a supported relay path (allowed unless the actor is the last +/// owner), so this needs `.allow_self_tagging()` for the same reason as +/// [`build_add_member`]. pub fn build_remove_member( channel_id: Uuid, target_pubkey: &str, @@ -588,7 +600,9 @@ pub fn build_remove_member( tag(&["h", &channel_id.to_string()])?, tag(&["p", &target_pubkey.to_ascii_lowercase()])?, ]; - Ok(EventBuilder::new(Kind::Custom(9001), "").tags(tags)) + Ok(EventBuilder::new(Kind::Custom(9001), "") + .tags(tags) + .allow_self_tagging()) } /// Build a NIP-29 leave-request event (kind 9022). @@ -2389,6 +2403,45 @@ mod tests { assert!(tag_values(&ev, "role").is_empty()); } + /// Self-add is a supported relay path ("Self-add: always allowed + /// regardless of policy"), so the `p` tag must survive signing even when + /// it names the signer. Without `.allow_self_tagging()` nostr 0.44 strips + /// it and the relay rejects the event as `invalid: missing p tag`. + #[test] + fn add_member_self_targeted_keeps_p_tag() { + let keys = keys(); + let self_hex = keys.public_key().to_hex(); + let ev = build_add_member(uuid(), &self_hex, Some(MemberRole::Bot)) + .unwrap() + .sign_with_keys(&keys) + .unwrap(); + assert_eq!(ev.kind.as_u16(), 9000); + assert!( + has_tag(&ev, "p", &self_hex), + "self `p` tag was stripped at signing: {:?}", + ev.tags.iter().map(|t| t.as_slice()).collect::>() + ); + assert!(has_tag(&ev, "role", "bot")); + } + + /// Self-remove is allowed unless the actor is the last owner — same + /// stripping hazard as [`add_member_self_targeted_keeps_p_tag`]. + #[test] + fn remove_member_self_targeted_keeps_p_tag() { + let keys = keys(); + let self_hex = keys.public_key().to_hex(); + let ev = build_remove_member(uuid(), &self_hex) + .unwrap() + .sign_with_keys(&keys) + .unwrap(); + assert_eq!(ev.kind.as_u16(), 9001); + assert!( + has_tag(&ev, "p", &self_hex), + "self `p` tag was stripped at signing: {:?}", + ev.tags.iter().map(|t| t.as_slice()).collect::>() + ); + } + #[test] fn remove_member_happy_path() { let cid = uuid(); diff --git a/desktop/src-tauri/src/commands/project_git_workflow.rs b/desktop/src-tauri/src/commands/project_git_workflow.rs index 39832feb10..c280f8b893 100644 --- a/desktop/src-tauri/src/commands/project_git_workflow.rs +++ b/desktop/src-tauri/src/commands/project_git_workflow.rs @@ -253,9 +253,13 @@ fn build_merged_status_event( .map(Tag::parse) .collect::, _>>() .map_err(|error| format!("build merged status tags: {error}"))?; + // `.allow_self_tagging()`: `owner` is this signer's own pubkey, so nostr + // 0.44 would strip `["p", owner]` and ship the status event without the + // recipient the readers index on. EventBuilder::new(Kind::Custom(1631), "") .tags(tags) .custom_created_at(Timestamp::from(created_at)) + .allow_self_tagging() .sign_with_keys(keys) .map(|event| event.as_json()) .map_err(|error| format!("sign merged pull request status: {error}")) @@ -291,9 +295,12 @@ fn build_pull_request_status_event( .map(Tag::parse) .collect::, _>>() .map_err(|error| format!("build pull request status tags: {error}"))?; + // See `build_merged_status_event` — `owner` is the signer, so the + // `["p", owner]` tag needs `.allow_self_tagging()` to survive. EventBuilder::new(kind, "") .tags(tags) .custom_created_at(Timestamp::from(created_at.max(Timestamp::now().as_secs()))) + .allow_self_tagging() .sign_with_keys(keys) .map(|event| event.as_json()) .map_err(|error| format!("sign pull request status: {error}")) @@ -830,6 +837,68 @@ mod tests { assert!(other.recovery.is_none()); } + /// The owner `p` tag names the signer, so nostr 0.44 scrubs it unless the + /// builder opts into self-tagging. Readers index PR status events by + /// `#p` (`projectPullRequests.mjs` reads them back as `recipients`), so a + /// stripped tag silently drops the owner from the participant set. + #[test] + fn pull_request_status_events_keep_the_owner_p_tag() { + let keys = Keys::generate(); + let owner = keys.public_key().to_hex(); + let repo_address = format!("30617:{owner}:buzz"); + let author = "b".repeat(64); + + let merged = Event::from_json( + build_merged_status_event( + &keys, + &repo_address, + &"d".repeat(64), + &author, + &"e".repeat(40), + 123, + ) + .unwrap(), + ) + .unwrap(); + assert!( + merged + .tags + .iter() + .any(|tag| tag.as_slice() == ["p", owner.as_str()]), + "kind 1631 lost the owner `p` tag: {:?}", + merged.tags + ); + + for (status, kind) in [("open", 1630u16), ("closed", 1632), ("draft", 1633)] { + let event = Event::from_json( + build_pull_request_status_event( + &keys, + &repo_address, + &"d".repeat(64), + &author, + status, + 123, + ) + .unwrap(), + ) + .unwrap(); + assert_eq!(event.kind.as_u16(), kind); + assert!( + event + .tags + .iter() + .any(|tag| tag.as_slice() == ["p", owner.as_str()]), + "kind {kind} ({status}) lost the owner `p` tag: {:?}", + event.tags + ); + // The author is a distinct pubkey, so it must still ride along. + assert!(event + .tags + .iter() + .any(|tag| tag.as_slice() == ["p", author.as_str()])); + } + } + #[test] fn merged_status_is_signed_by_repository_owner() { let keys = Keys::generate(); diff --git a/desktop/src-tauri/src/events.rs b/desktop/src-tauri/src/events.rs index 777d56d02e..0dffebc3f1 100644 --- a/desktop/src-tauri/src/events.rs +++ b/desktop/src-tauri/src/events.rs @@ -266,6 +266,11 @@ pub fn build_delete_channel(channel_id: Uuid) -> Result { // ── Membership ─────────────────────────────────────────────────────────────── /// Kind 9000 — add member. +/// +/// `.allow_self_tagging()` is required: self-add is a first-class relay path, +/// so `actor == target` and the `["p", target]` tag matches the signer. nostr +/// 0.44 strips matching `p` tags by default, which would send the event with +/// no target and fail as `missing p tag`. pub fn build_add_member( channel_id: Uuid, target_pubkey: &str, @@ -279,17 +284,24 @@ pub fn build_add_member( if let Some(r) = role { tags.push(tag(vec!["role", r])?); } - Ok(EventBuilder::new(Kind::Custom(9000), "").tags(tags)) + Ok(EventBuilder::new(Kind::Custom(9000), "") + .tags(tags) + .allow_self_tagging()) } /// Kind 9001 — remove member. +/// +/// Self-remove is a supported relay path, so this needs +/// `.allow_self_tagging()` for the same reason as [`build_add_member`]. pub fn build_remove_member(channel_id: Uuid, target_pubkey: &str) -> Result { check_pubkey(target_pubkey)?; let tags = vec![ tag(vec!["h", &channel_id.to_string()])?, tag(vec!["p", &target_pubkey.to_ascii_lowercase()])?, ]; - Ok(EventBuilder::new(Kind::Custom(9001), "").tags(tags)) + Ok(EventBuilder::new(Kind::Custom(9001), "") + .tags(tags) + .allow_self_tagging()) } // ── Messages ───────────────────────────────────────────────────────────────── @@ -931,6 +943,58 @@ mod tests { assert_eq!(event.pubkey.to_hex(), TARGET_HEX); } + // ── Self-targeted membership keeps its `p` tag ────────────────────── + // + // Self-add and self-remove are supported relay paths, so `actor == + // target`. Without `.allow_self_tagging()` nostr 0.44 scrubs the `p` tag + // that names the signer and the relay rejects with `missing p tag`. + + fn self_membership_tags(builder: EventBuilder) -> (Vec>, String) { + let secret = nostr::SecretKey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000004", + ) + .unwrap(); + let keys = Keys::new(secret); + let event = builder.sign_with_keys(&keys).unwrap(); + ( + event.tags.iter().map(|t| t.as_slice().to_vec()).collect(), + keys.public_key().to_hex(), + ) + } + + #[test] + fn add_member_self_targeted_keeps_p_tag() { + let secret = nostr::SecretKey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000004", + ) + .unwrap(); + let self_hex = Keys::new(secret).public_key().to_hex(); + let channel = Uuid::parse_str(CH_ID).unwrap(); + let (tags, signer) = + self_membership_tags(build_add_member(channel, &self_hex, Some("bot")).unwrap()); + assert!( + tags.iter().any(|t| t[0] == "p" && t[1] == signer), + "self `p` tag was stripped at signing: {tags:?}" + ); + assert!(tags.iter().any(|t| t[0] == "role" && t[1] == "bot")); + } + + #[test] + fn remove_member_self_targeted_keeps_p_tag() { + let secret = nostr::SecretKey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000004", + ) + .unwrap(); + let self_hex = Keys::new(secret).public_key().to_hex(); + let channel = Uuid::parse_str(CH_ID).unwrap(); + let (tags, signer) = + self_membership_tags(build_remove_member(channel, &self_hex).unwrap()); + assert!( + tags.iter().any(|t| t[0] == "p" && t[1] == signer), + "self `p` tag was stripped at signing: {tags:?}" + ); + } + // ── build_message_edit `p`-tag emission (lane 8ace8eed) ────────────── // // The composer diffs the edited body's mentions against the original and