block / block/buzz

build_message signs mention pubkeys that are not valid hex public keys

Open
#6,291 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.