build_message signs mention pubkeys that are not valid hex public keys
- Dominant language
- Rust
- Stars
- 32.7k
- Forks
- 4.3k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 253
Description
`buzz_sdk::builders::build_message` accepts mention pubkeys that are not valid hex public keys and signs them into `p` tags unchanged. Reported from a downstream consumer (a UniFFI mobile core); filing here rather than working around it locally, since the decision affects desktop and CLI consumers too.
## Behavior
`mention_tags` (`crates/buzz-sdk/src/builders.rs:192-205`) lowercases each entry and pushes `["p", lower]` with no hex/length check:
```rust
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();
if seen.insert(lower.clone()) {
tags.push(tag(&["p", &lower])?);
}
}
Ok(())
}
```
`Tag::parse` does not validate that a `p` value is a 32-byte hex key, and `mentions::normalize_mention_pubkeys` (`crates/buzz-sdk/src/mentions.rs:228`) only lowercases, dedupes, and drops the sender — so nothing on this path rejects a malformed key. Note the asymmetry: the NIP-27 path *does* validate, via `PublicKey::from_bech32` at `mentions.rs:378`. Only explicitly-supplied pubkeys skip validation.
## Reproduction
Against `93114c9c` (`crates/buzz-sdk`), calling `build_message` with four invalid mentions and signing:
```rust
let garbage = ["not-a-pubkey", "ZZZZ", "", "../../etc/passwd"];
let builder = buzz_sdk::builders::build_message(
channel, "hello", None, &garbage, false, &[],
).expect("build_message accepted non-hex mentions");
let event = builder.sign_with_keys(&keys).expect("signed");
```
`build_message` returns `Ok`, the event signs, and the signed tags are:
```
[["p", "not-a-pubkey"], ["p", "zzzz"], ["p", ""], ["p", "../../etc/passwd"]]
```
An empty `p` value and a path-shaped `p` value both survive into a signed event.
## Why it matters downstream
For SDK consumers whose mention list crosses a boundary from user-influenced data (an FFI surface, an MCP tool call, a CLI flag), the builder is the natural validation point: a caller reasonably assumes a function taking "mention pubkeys" rejects things that are not pubkeys. Instead the invalid tag is signed and published, and every reader has to defend against `p` tags that cannot be parsed as keys. It also silently burns mention-cap budget on entries that can never address anyone.
I have no evidence of a live exploit — the practical impact I can demonstrate is malformed signed events and wasted cap, not a relay-side vulnerability.
## Suggested fix (upstream's call)
Validate each entry through `nostr::PublicKey::from_hex` in `mention_tags` and return a typed error (e.g. an `SdkError::InvalidMentionPubkey { value }`, alongside the existing `TooManyMentions`) rather than silently dropping — a silent drop would make a mention vanish with no feedback to the composer.
Two things worth deciding deliberately, which is why I am not sending a patch:
1. **Whether it belongs in `mention_tags` or in `normalize_mention_pubkeys`.** Putting it in `normalize_*` catches more callers but changes a currently-infallible signature.
2. **Whether rejecting is a breaking change for existing consumers.** If any caller today passes non-key values through `p` tags deliberately, validation would start failing their sends.
Happy to send a PR once you have a preference on those two points.
Contributor guide
Research direction
Read crates/buzz-sdk/src/builders.rs:192-205 and crates/buzz-sdk/src/mentions.rs:228, then compare the explicit-pubkey path with the NIP-27 validation at mentions.rs:378. Run the supplied reproduction against the referenced commit and inspect existing SDK error and builder tests. Done means the upstream validation location, error behavior, compatibility choice, and regression coverage are agreed and verified.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- api
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100