fix(sdk): reject malformed mention pubkeys before signing p tags

[holmes]

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:

#	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<PublicKey> 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

• 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)
• cargo fmt --all -- --check — clean
• just clippy (workspace, -D warnings) — clean
• just test-unit — all 10 steps pass, now including buzz-sdk
• just file-size-check — pass
• just desktop-tauri-fmt-check / desktop-tauri-check / desktop-tauri-test — pass (2684 tests; desktop/src-tauri consumes buzz-sdk)
• 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.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Non-blocking documentation correction: nostr::PublicKey::from_hex is not stricter and does not perform an on-curve check in nostr 0.44.7; it only hex-decodes 32 bytes into PublicKey. Curve validation happens only when PublicKey::xonly() is called. The implementation here is still correct for the SDK’s established structural 64-hex contract, but the mention_tags comment should describe the deferred xonly() check rather than attributing it to from_hex.

No blocking defects found at 5fe5929de39388cada354593ac875d3c42c3d830. I traced all three SDK builders and repository call sites, verified Desktop parity, cap/error precedence, case canonicalization, deduplication and tag ordering, and checked the test-runner additions against current main. cargo test -p buzz-sdk passed 267 tests; cargo fmt --check -p buzz-sdk, git diff --check, the file-size ratchet, and a conflict-free merge-tree with current origin/main also passed. GitHub CI remains unexecuted pending maintainer approval of the fork workflows.