From a5767c67deb951d64b8465b5da32c1dfc7a9ba40 Mon Sep 17 00:00:00 2001 From: Jason Holmes Date: Tue, 20 Oct 2026 13:44:19 -0700 Subject: [PATCH] fix(sdk): reject malformed mention pubkeys before signing p tags (#6372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `buzz_sdk::builders::mention_tags` lowercased whatever string it was handed and pushed it straight into a `p` tag, so malformed mention pubkeys were signed onto real events. A composer that mentioned someone this way looked successful while notifying nobody, and the bad tag propagated to every relay and client that read it. Reproduced before fixing: `""`, `"not-a-pubkey"`, `"zzzz"`, `"../../etc/passwd"`, and 63/65-character strings were all accepted and emitted verbatim as `p` tags by `build_message` and `build_forum_post`. Fixes #6291 ## Acceptance criteria → evidence Following @kiranmagic7's [recommendation on the issue](https://github.com/block/buzz/issues/6291): | # | Criterion | Evidence | |---|---|---| | 1 | Validate in the **private `mention_tags`**, not the public `normalize_mention_pubkeys` | `crates/buzz-sdk/src/builders.rs` — the single changed line inside `fn mention_tags`. `normalize_mention_pubkeys` is untouched, so no public signature changes. | | 2 | Reuse the existing **`check_pubkey_hex`** helper | `let lower = check_pubkey_hex(hex, "mention pubkey")?;` replaces `hex.to_ascii_lowercase()`. No new helper added. | | 3 | Reuse the existing **`SdkError::InvalidInput`** | `check_pubkey_hex` already returns `InvalidInput`; no new error variant is introduced. | | 4 | Cover all **3 builders** | `malformed_mention_pubkeys_are_refused_by_every_builder` asserts against `build_message`, `build_forum_post`, **and** `build_forum_comment` — all three share `mention_tags`. | | 5 | Reject **empty / non-hex / 63-char / 65-char** | Same test's table: `""`, `"not-a-pubkey"`, `"zzzz"`, `"../../etc/passwd"`, `"a"*63`, `"a"*65`, plus `"z"*64` and a 64-char path-like string so a length-only check cannot pass. | | 6 | Preserve valid **uppercase canonicalization** | `valid_mentions_are_canonicalized_and_deduped_across_case` — uppercase hex is accepted and the emitted `p` tag is lowercase, across all three builders. | | 7 | Preserve valid **dedupe** | Same test: the same pubkey in both spellings collapses to exactly one `p` tag. Pre-existing `message_mentions_deduped` still passes. | | 8 | Preserve **`TooManyMentions` precedence** for >50 | `too_many_mentions_takes_precedence_over_validation` — a 51-entry list that also contains a malformed entry still returns `TooManyMentions`. The cap check stays ahead of validation. `mentions_at_the_cap_still_sign` pins that the boundary itself did not move. | | 9 | Inspect **#3036 overlap**; keep the patch narrow | See the compatibility note below. | | 10 | Make these tests actually **gate** | `Justfile` + `scripts/run-tests.sh` — see "Why the runner change is here". | Out of scope by design: switching to `nostr::PublicKey::from_hex`. Its stricter on-curve rule would change every one of `check_pubkey_hex`'s ~26 call sites, which is a repo-wide consistency decision rather than part of this fix. The doc comment records that reasoning. ## Why validation lives in `mention_tags` `mention_tags` is the single choke point every mention reaches, shared by all three builders, and callers can hand those builders an explicit list without ever going through `normalize_mention_pubkeys`. Validating here closes that bypass without breaking the normalizer's public API. This also aligns the SDK with behavior already shipping elsewhere: desktop's own `events.rs::mention_tags` already validates with `check_pubkey` on the identical structural rule (64 ASCII hex). The SDK was the inconsistent one. One malformed entry refuses the whole list rather than being filtered out — a silently dropped mention is precisely the failure being fixed, and filtering would preserve it. ## Why the runner change is here Adding `buzz-sdk` to `just test-unit` and the mirrored `scripts/run-tests.sh` fallback is a CI-behavior change, so it deserves its own justification: **without it the regression tests above would never run in CI.** `buzz-sdk` is a workspace member, so it was getting `clippy` and `check`, but no CI job executed a single one of its tests. This is the exact hazard the `Justfile` already warns about for other crates — *"nothing in CI runs `cargo test --workspace` — workspace membership alone buys clippy/check, not a single executed test."* Verified with a positive/negative control rather than by reading the config: - **Negative control** — with the runner files at pristine `main` and a deliberately panicking `buzz-sdk` test in the tree, `just test-unit` exited **0** and printed "All tests passed!". The crate name never appeared in the log. - **Positive control** — same sabotaged test, with only these two runner edits applied: `just test-unit` exited **1** with `buzz-sdk tests FAILED`. The sabotage test was removed afterward; it is not part of this PR. Both entries are needed because the two lists (nextest path and the no-nextest fallback) must stay in step. This is scoped strictly to making the crate's existing tests execute — no other CI behavior is touched. ## Compatibility with #3036 #3036 (`fix/nip10-reply-p-tags`) also edits `mention_tags`: it adds `parent_author: Option` to `ThreadRef`, emits a parent-author `p` tag in `thread_tags`, and seeds `mention_tags`'s dedup set from `p` tags already on `tags`. This PR replaces only the `let lower = ...` line inside the loop, so the two changes are compatible in substance. Mechanically, whichever lands second needs the trivial merge fixup: #3036 adds `parent_author: None` to every `ThreadRef` literal, including the ones in the tests added here. One correction to the paragraph above, from an independent merge trial: the fixup is not *only* the `parent_author: None` additions. Merging #3036 into this head also produces one **textual conflict in `crates/buzz-sdk/src/builders.rs`**, confined to the `mention_tags` doc comment — both branches rewrote it, this PR to explain the validation and #3036 to explain the seeded dedup set. The resolution is to keep both blocks; there is no logic conflict, since this PR changes the `let lower = ...` line and #3036 changes the `seen` initializer. For reference, merging #3036 into plain `main` leaves `builders.rs` auto-merging cleanly, so this conflict is specific to the combination. Once the doc comment is resolved and `parent_author: None` is added to the three new `ThreadRef` literals here, `buzz-sdk` compiles and the only failing test is #3036's own `self_reply_drops_the_parent_p_tag` — which fails identically with #3036 merged into `main` **without** this PR, because `build_message` gained `.allow_self_tagging()` in #4975 after #3036's merge base. ## Test plan - [x] `cargo test -p buzz-sdk` — **267 passed, 0 failed** (5 new tests; pre-existing `message_mentions_deduped`, `message_too_many_mentions`, and the three `*_preserves_self_mention_p_tag` tests still green) - [x] `cargo fmt --all -- --check` — clean - [x] `just clippy` (workspace, `-D warnings`) — clean - [x] `just test-unit` — all 10 steps pass, now including `buzz-sdk` - [x] `just file-size-check` — pass - [x] `just desktop-tauri-fmt-check` / `desktop-tauri-check` / `desktop-tauri-test` — pass (2684 tests; `desktop/src-tauri` consumes `buzz-sdk`) - [x] Full `just ci` — the remaining steps are JS/Dart (`desktop-test`, `desktop-build`, `web-build`, `mobile-test`); this change touches only Rust + two runner scripts, so CI covers them here All gate results above were confirmed at commit `5fe5929de39388cada354593ac875d3c42c3d830`. ## Open questions for maintainers Out of draft and ready for review. Two calls I'd like from a maintainer: 1. **Do the runner entries belong here?** Adding `buzz-sdk` to `just test-unit` and `scripts/run-tests.sh` is a CI-behavior change riding along with a bug fix. I kept it because without it the regression tests above never execute in CI (controls in the section above), but I'll split it into its own PR on request. 2. **Is leaving `check_pubkey_hex` alone right for now?** Switching to `nostr::PublicKey::from_hex` would add an on-curve check but changes the contract at all ~26 call sites, which reads as a separate repo-wide decision rather than part of this fix. ### A note on CI The only check reporting here is **DCO Check** (pass). `CI`, `Desktop Release Candidate`, and `Docker image` are all sitting in `action_required` because this PR comes from a fork — a maintainer has to approve the workflow run before they execute. That's the standard first-time-contributor gate, not a failure, and I can't clear it from my side. Every gate result in the test plan above is from local runs at `5fe5929de39388cada354593ac875d3c42c3d830`, so CI has not independently confirmed them yet. Signed-off-by: rust worker Signed-off-by: Jason Holmes Co-authored-by: rust worker --- crates/buzz-sdk/src/builders.rs | 155 +++++++++++++++++++++++++++++++- 1 file changed, 153 insertions(+), 2 deletions(-) diff --git a/crates/buzz-sdk/src/builders.rs b/crates/buzz-sdk/src/builders.rs index f698598b93..a2d19f779f 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])?); } @@ -2550,6 +2570,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 @@ -2735,6 +2761,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();