From 5fe5929de39388cada354593ac875d3c42c3d830 Mon Sep 17 00:00:00 2001 From: rust worker Date: Wed, 19 Aug 2026 20:44:42 -0700 Subject: [PATCH] fix(sdk): reject malformed mention pubkeys before signing p tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `builders::mention_tags` lowercased whatever it was handed and pushed it straight into a `p` tag, so `""`, `"not-a-pubkey"`, `"../../etc/passwd"` and 63/65-character strings were all signed onto real events. A composer that mentioned someone this way looked successful while notifying nobody, and the malformed tag propagated to every relay and client that read it. Validate with the existing `check_pubkey_hex` inside `mention_tags`. That helper is the SDK's established structural pubkey contract (64 ASCII hex, returned lowercased) and already guards the other builders that emit `p` tags, so mentions now agree with the rest of the module and surface the existing `SdkError::InvalidInput`. Desktop's own `events.rs::mention_tags` already applies the identical rule, so this aligns the SDK with shipped behavior rather than inventing a new one. The check goes in the private `mention_tags` rather than the public `normalize_mention_pubkeys`: `mention_tags` is the single choke point shared by `build_message`, `build_forum_post` and `build_forum_comment`, and callers can hand those builders an explicit list without ever going through the normalizer. Fixing it here closes that bypass without changing a public signature. One malformed entry refuses the whole list instead of being filtered out, because a silently dropped mention is the failure being fixed. The cap check stays ahead of validation so an over-cap list still reports `TooManyMentions` rather than having that error masked by whichever entry happens to be malformed; both behaviors are pinned by tests. Adopting `nostr::PublicKey::from_hex`'s stricter on-curve check was considered and left out: it would change every `check_pubkey_hex` call site, which is a repo-wide consistency decision rather than part of this fix. Also enumerate `buzz-sdk` in `just test-unit` and the `run-tests.sh` fallback. The crate is a workspace member, so it was getting clippy and check, but no CI job ran a single one of its tests — verified with a control: a deliberately panicking buzz-sdk test still let `just test-unit` exit 0 and report "All tests passed!" before this change, and fails it after. Without those two mirrored entries the regression tests added here would never gate anything. Fixes #6291 Signed-off-by: rust worker Co-authored-by: Jason Holmes Signed-off-by: Jason Holmes --- Justfile | 7 ++ crates/buzz-sdk/src/builders.rs | 155 +++++++++++++++++++++++++++++++- scripts/run-tests.sh | 7 ++ 3 files changed, 167 insertions(+), 2 deletions(-) diff --git a/Justfile b/Justfile index ce8647cf77c..d0717eb7bdb 100644 --- a/Justfile +++ b/Justfile @@ -340,6 +340,13 @@ test-unit: # `cargo test --workspace`; without this step a manifest edit that # diverges Rust from the corpus ships green. cargo nextest run -p buzz-agent --lib + # buzz-sdk builders/mentions: pure event-construction unit tests (no + # infra) that pin the signed wire shape — tag order, NIP-10 markers, and + # the mention `p`-tag validation from #6291. Enumerated explicitly for + # the same reason as the packages above: the crate is a workspace member + # and gets clippy/check, but no CI job executed a single one of its + # tests, so a builder regression shipped green. + cargo nextest run -p buzz-sdk else ./scripts/run-tests.sh unit fi diff --git a/crates/buzz-sdk/src/builders.rs b/crates/buzz-sdk/src/builders.rs index 71c0f1e73db..56d13993e01 100644 --- a/crates/buzz-sdk/src/builders.rs +++ b/crates/buzz-sdk/src/builders.rs @@ -189,14 +189,34 @@ fn thread_tags(thread_ref: &ThreadRef, tags: &mut Vec) -> Result<(), SdkErr Ok(()) } -/// Deduplicate and cap mentions, emitting p-tags. +/// Validate, deduplicate, and cap mentions, emitting p-tags. +/// +/// Validation lives here rather than in [`crate::mentions::normalize_mention_pubkeys`] +/// because this helper is the single choke point every mention reaches: it is +/// shared by `build_message`, `build_forum_post`, and `build_forum_comment`, and +/// a caller may hand those builders an explicit list without going through the +/// normalizer at all. Without a check here, `"not-a-pubkey"`, `""`, and +/// `"../../etc/passwd"` were lowercased into `p` tags and signed, so the +/// composer reported a successful mention that could notify nobody. +/// +/// [`check_pubkey_hex`] is reused deliberately: it is the SDK's existing +/// structural pubkey contract (64 ASCII hex, returned lowercased) and is already +/// applied before other builders emit `p` tags, so mentions now agree with the +/// rest of this module and yield the same [`SdkError::InvalidInput`]. Adopting +/// `nostr::PublicKey::from_hex`'s stricter on-curve rule would be a wider +/// consistency change across every `check_pubkey_hex` call site, not a +/// mentions-only fix. +/// +/// The cap is checked before validation so an over-cap list still reports +/// [`SdkError::TooManyMentions`] rather than being masked by whichever entry +/// happens to be malformed. fn mention_tags(mentions: &[&str], tags: &mut Vec) -> Result<(), SdkError> { if mentions.len() > crate::mentions::MENTION_CAP { return Err(SdkError::TooManyMentions); } let mut seen = std::collections::HashSet::new(); for &hex in mentions { - let lower = hex.to_ascii_lowercase(); + let lower = check_pubkey_hex(hex, "mention pubkey")?; if seen.insert(lower.clone()) { tags.push(tag(&["p", &lower])?); } @@ -2355,6 +2375,12 @@ mod tests { Uuid::new_v4() } + /// A distinct well-formed 64-character hex pubkey per index, for tests that + /// need many valid mentions. + fn distinct_pubkey_hex(i: u8) -> String { + format!("{i:02x}{}", "ab".repeat(31)) + } + fn tag_values(event: &nostr::Event, key: &str) -> Vec { event .tags @@ -2539,6 +2565,131 @@ mod tests { assert_eq!(p_tags.len(), 1); } + /// Every structurally invalid mention must be refused by all three builders + /// that share `mention_tags`, rather than lowercased into a `p` tag and + /// signed. See #6291. + #[test] + fn malformed_mention_pubkeys_are_refused_by_every_builder() { + let cid = uuid(); + let root = event_id(); + let tr = ThreadRef { + root_event_id: root, + parent_event_id: root, + }; + let short = "a".repeat(63); + let long = "a".repeat(65); + // Empty, non-hex, and both off-by-one lengths. The wrong-alphabet cases + // are pinned separately from the wrong-length cases because a + // length-only check would otherwise pass: "zzzz" is caught by length, + // but a 64-character non-hex string is not. + let non_hex_64 = "z".repeat(64); + let path_like_64 = format!("../../etc/passwd{}", "0".repeat(48)); + for bad in [ + "", + "not-a-pubkey", + "zzzz", + "../../etc/passwd", + short.as_str(), + long.as_str(), + non_hex_64.as_str(), + path_like_64.as_str(), + ] { + assert!( + matches!( + build_message(cid, "hi", None, &[bad], false, &[]), + Err(SdkError::InvalidInput(_)) + ), + "build_message must refuse mention {bad:?}" + ); + assert!( + matches!( + build_forum_post(cid, "hi", &[bad], &[]), + Err(SdkError::InvalidInput(_)) + ), + "build_forum_post must refuse mention {bad:?}" + ); + assert!( + matches!( + build_forum_comment(cid, "hi", &tr, &[bad], &[]), + Err(SdkError::InvalidInput(_)) + ), + "build_forum_comment must refuse mention {bad:?}" + ); + } + } + + /// One malformed entry must refuse the whole list rather than being silently + /// dropped: a filtered mention looks successful to the composer while + /// notifying nobody. + #[test] + fn one_malformed_mention_refuses_the_whole_list() { + let cid = uuid(); + let valid = "abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234"; + let result = build_message(cid, "hi", None, &[valid, "nope", valid], false, &[]); + assert!(matches!(result, Err(SdkError::InvalidInput(_)))); + } + + /// Validation must not break the two behaviours callers already rely on: + /// uppercase hex is legitimate input and is canonicalized to lowercase, and + /// duplicates across cases collapse to a single `p` tag. + #[test] + fn valid_mentions_are_canonicalized_and_deduped_across_case() { + let cid = uuid(); + let upper = "ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234"; + let lower = upper.to_ascii_lowercase(); + let root = event_id(); + let tr = ThreadRef { + root_event_id: root, + parent_event_id: root, + }; + + let ev = sign(build_message(cid, "hi", None, &[upper], false, &[]).unwrap()); + assert_eq!( + tag_values(&ev, "p"), + vec![lower.clone()], + "uppercase hex must be accepted and lowercased" + ); + + // Same pubkey in both spellings is one person, so one p tag. + let ev = sign(build_message(cid, "hi", None, &[upper, &lower], false, &[]).unwrap()); + assert_eq!(tag_values(&ev, "p"), vec![lower.clone()]); + + let ev = sign(build_forum_post(cid, "hi", &[upper], &[]).unwrap()); + assert_eq!(tag_values(&ev, "p"), vec![lower.clone()]); + + let ev = sign(build_forum_comment(cid, "hi", &tr, &[upper], &[]).unwrap()); + assert_eq!(tag_values(&ev, "p"), vec![lower]); + } + + /// The cap is checked before per-entry validation, so an over-cap list of + /// well-formed pubkeys keeps reporting `TooManyMentions`, and a list that is + /// both over-cap *and* malformed reports `TooManyMentions` too rather than + /// changing which error callers see. + #[test] + fn too_many_mentions_takes_precedence_over_validation() { + let cid = uuid(); + let hexes: Vec = (0..51u8).map(distinct_pubkey_hex).collect(); + let mut refs: Vec<&str> = hexes.iter().map(String::as_str).collect(); + refs.push("not-a-pubkey"); + assert!(matches!( + build_message(cid, "hi", None, &refs, false, &[]), + Err(SdkError::TooManyMentions) + )); + } + + /// A list exactly at the cap still signs, so the guard cannot have moved the + /// boundary. + #[test] + fn mentions_at_the_cap_still_sign() { + let cid = uuid(); + let hexes: Vec = (0..crate::mentions::MENTION_CAP as u8) + .map(distinct_pubkey_hex) + .collect(); + let refs: Vec<&str> = hexes.iter().map(String::as_str).collect(); + let ev = sign(build_message(cid, "hi", None, &refs, false, &[]).unwrap()); + assert_eq!(tag_values(&ev, "p").len(), crate::mentions::MENTION_CAP); + } + #[test] fn message_too_many_mentions() { let cid = uuid(); diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh index 0bbdfca6a4d..2df96b500cf 100755 --- a/scripts/run-tests.sh +++ b/scripts/run-tests.sh @@ -120,6 +120,13 @@ run_unit_tests() { # `just test-unit` — the two lists must stay in step. run_test_step "buzz-agent unit tests" \ cargo test -p buzz-agent --lib -- --nocapture + + # buzz-sdk builders/mentions: pure event-construction unit tests (no infra) + # that pin the signed wire shape — tag order, NIP-10 markers, and the mention + # `p`-tag validation from #6291. Mirrors the nextest path in `just test-unit` + # — the two lists must stay in step. + run_test_step "buzz-sdk tests" \ + cargo test -p buzz-sdk -- --nocapture } # ---- DB / integration tests (infra required) --------------------------------