build_message signs mention pubkeys that are not valid hex public keys

[holmes]

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:

fn mention_tags(mentions: &[&str], tags: &mut Vec<Tag>) -> 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:

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.

[kiranmagic7]

@holmes, I traced this on current main (f88cda9e). I would validate in builders::mention_tags, not normalize_mention_pubkeys. The helper is shared by build_message, build_forum_post, and build_forum_comment. Making the public normalizer return Result would break its API, and direct builder callers could still bypass it.

The same file already has check_pubkey_hex, which other SDK builders call before emitting p tags. Reusing it here preserves the SDK's existing 64-hex/lowercase contract and returns SdkError::InvalidInput, so this does not need a new error variant. CLI explicit mentions already pass through PublicKey::from_hex, Desktop uses its own check_pubkey, and the ACP call sites pass either no mentions or a pubkey parsed from an event. That means the behavior changes only for direct SDK callers that currently pass malformed strings.

Tests should reject empty, non-hex, and 63/65-character values through all three builders; preserve uppercase canonicalization and valid-mention deduplication; and keep the current TooManyMentions result for more than 50 inputs. I found no implementation for #6291. #3036 does edit mention_tags for separate reply-author deduplication, so whoever takes this should account for that overlap. If the project wants nostr::PublicKey::from_hex's stricter validation instead of the SDK's current structural 64-hex rule, I would make that a deliberate consistency change across the existing check_pubkey_hex call sites rather than only mentions.

[holmes]

Implementation is ready for maintainer review in draft PR #6372: #6372

It follows the accepted direction from @kiranmagic7: validation stays in private builders::mention_tags, reuses check_pubkey_hex / existing SdkError::InvalidInput, covers message + forum post + forum comment, preserves uppercase canonicalization/dedupe and TooManyMentions precedence, and documents the #3036 overlap. The PR also makes buzz-sdk tests execute in both just test-unit runner paths; independent positive/negative controls confirmed they were previously omitted.

Local gates and an independent review are green at 5fe5929de39388cada354593ac875d3c42c3d830. GitHub workflows are awaiting a maintainer’s first-time fork approval.