buzz-sdk: build_add_member/build_remove_member accept overlong hex pubkeys and sign an unparseable p tag

[holmes]

buzz_sdk::builders::build_add_member and build_remove_member validate their target_pubkey with a minimum-length check where the wire format requires an exact length, so an overlong all-hex string is accepted and signed into the p tag. The relay then cannot parse that p tag and rejects the write with missing p tag.

Found while reviewing #6372 (which touches a different builder path and is not affected). This is pre-existing on main — reproduced at f88cda9eb886500ec7d205e1d265ac6f654aa433.

Note for anyone who saw my earlier informal note on this: my first characterization ("no hex-alphabet check") was wrong, and the probe below is what corrected it. check_hex_len does check the hex alphabet. The defect is min-vs-exact length only.

The check

crates/buzz-sdk/src/builders.rs:44-52:

/// Validate hex string has at least `min_len` hex characters.
fn check_hex_len(s: &str, min_len: usize, field: &str) -> Result<(), SdkError> {
    if s.len() < min_len || !s.chars().all(|c| c.is_ascii_hexdigit()) {
        return Err(SdkError::InvalidDiffMeta(format!(
            "{field} must be at least {min_len} hex characters (got {:?})",
            s
        )));
    }
    Ok(())
}

It is the right helper for its two other call sites — abbreviated git shas at builders.rs:333 (commit_sha, min 7) and :335 (parent_commit, min 7), where "at least" is genuinely the contract. It is the wrong helper for a pubkey:

// builders.rs:576-590
pub fn build_add_member(channel_id: Uuid, target_pubkey: &str, role: Option<MemberRole>) -> Result<EventBuilder, SdkError> {
    check_hex_len(target_pubkey, 64, "target_pubkey")?;     // >= 64, not == 64
    let mut tags = vec![
        tag(&["h", &channel_id.to_string()])?,
        tag(&["p", &target_pubkey.to_ascii_lowercase()])?,
    ];
    ...
}

// builders.rs:593-603 — build_remove_member, identical check

Reproduction

Throwaway test in crates/buzz-sdk/src/builders.rs at f88cda9eb, cargo test -p buzz-sdk -- --nocapture, reverted after:

ADD  len=100 hex     -> ACCEPTED, p tag len 100 = ["abab…abab"]
RM   len=100 hex     -> ACCEPTED, p tag len 100 = ["abab…abab"]
ADD  len=65  hex     -> ACCEPTED
ADD  len=64  non-hex -> REJECTED: InvalidDiffMeta("target_pubkey must be at least 64 hex characters (got \"zzzz…\")")
ADD  len=63  hex     -> REJECTED: InvalidDiffMeta("target_pubkey must be at least 64 hex characters (got \"aaa…\")")
BAN  len=65  hex     -> REJECTED: InvalidInput("target_pubkey must be a 64-character hex pubkey")

So: too short is caught, non-hex is caught, too long is signed. The last line is the same input against build_moderation_ban, which uses the exact-length helper and rejects it.

Why exact length is the contract

1. The relay requires exactly 64. crates/buzz-relay/src/handlers/side_effects.rs:2366-2379 (extract_p_tag, used by the kind-9000/9001 admin validator at :363 and :455) hex-decodes and requires 32 bytes. crates/buzz-relay/src/handlers/moderation_commands.rs:561-575 (extract_p_tag_bytes) requires val.len() == 64 && all ascii hexdigit. A 100-char tag decodes to 50 bytes and a 65-char tag fails to decode; both yield None, and the 9000 validator turns that into Err("missing p tag") — a confusing error for a caller that did supply a p tag.
2. The SDK already documents this contract in its own test. crates/buzz-sdk/src/builders.rs:4383-4388:

#[test]
fn moderation_ban_rejects_overlong_pubkey() {
    // Relay `extract_p_tag_bytes` requires exactly 64 hex; the SDK must
    // reject 65+ hex here rather than sign a `p` tag the relay drops.
    let err = build_moderation_ban(&"a".repeat(65), None, None).unwrap_err();
    assert!(matches!(err, SdkError::InvalidInput(_)));
}

build_add_member/build_remove_member are the two p-tag builders that do not honor it.

3. Every sibling p-tag builder uses the exact-length helper, check_pubkey_hex (builders.rs:69-76, len() != 64 || !is_ascii_hexdigit, returns the value lowercased, errors InvalidInput): build_moderation_ban/unban/timeout/untimeout (:1751, :1764, and following), build_dm_open (:269-270), issue-assignee tags (:1231), repo owner/recipient tags (:988, :1036, :1045, …). check_hex_len at 64 appears only in these two functions.

Secondary nit visible in the probe output: the error variant is SdkError::InvalidDiffMeta, which is a git-diff-metadata error, for a channel member pubkey. check_pubkey_hex returns InvalidInput.

Exposure

Scoped to what I searched (rg 'build_add_member|build_remove_member' -g '*.rs' across the repo):

• crates/buzz-cli/src/commands/channels.rs:991 and :1008 — pre-validate with validate_hex64 (exact 64, crates/buzz-cli/src/validate.rs:29-36). Not exposed.
• crates/buzz-cli/src/commands/channels.rs:747 (template apply / roster) — passes agent.pubkey straight through with no length check. That value comes from the d tag of a relay-stored KIND_MANAGED_AGENT event via extract_d_tag, and build_roster_resolution only rejects it when empty (channels.rs:453-455). This is the one in-repo caller that would reach the builder with an unvalidated length; I did not construct a malformed managed-agent record, so I am claiming "unguarded path", not "demonstrated exploit".
• desktop/src-tauri/src/** calls its own events::build_add_member/build_remove_member (desktop/src-tauri/src/events.rs:222, :239), which use a local check_pubkey with an exact-64 check (events.rs:84-92). Desktop does not reach the SDK helper on this path.

No security claim: the relay rejects the event, so the practical impact is a signed-but-unusable event and a misleading missing p tag error rather than a bad membership write. I found no path where an overlong p tag results in a member being added.

Suggested fix

Swap both call sites to the existing exact-length helper — no new helper, and it also lowercases so the manual to_ascii_lowercase() on the tag can use its return value:

let target_pubkey = check_pubkey_hex(target_pubkey, "target_pubkey")?;
let mut tags = vec![
    tag(&["h", &channel_id.to_string()])?,
    tag(&["p", &target_pubkey])?,
];

That also moves the error from InvalidDiffMeta to InvalidInput, matching the sibling builders. It is a behavior change for any caller currently passing 65+ hex chars, but such a caller's event is already rejected by the relay, so the change converts a remote failure into a local one.

Tests worth adding alongside, mirroring moderation_ban_rejects_overlong_pubkey: overlong rejected for both builders, exact-64 still accepted and lowercased, and the existing short/non-hex rejections preserved.

Happy to send that PR if you want it — it is a two-line change plus tests. Flagging rather than patching unilaterally since it tightens a public SDK signature's accepted input.