jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-27 04:30:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
## 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](https://github.com/block/buzz/issues/6291):

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

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

Signed-off-by: rust worker <cb8459a6e4c936835834cef03b8331fe806108dd03da97d39c7036a7d710e860@buzz.block.builderlab.xyz>
Signed-off-by: Jason Holmes <holmes@squareup.com>
Co-authored-by: rust worker <cb8459a6e4c936835834cef03b8331fe806108dd03da97d39c7036a7d710e860@buzz.block.builderlab.xyz>
This commit is contained in:
Jason Holmes
2026-10-20 13:44:19 -07:00
committed by GitHub
parent cdc2b129c0
commit a5767c67de
1 changed files with 153 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

155
crates/buzz-sdk/src/builders.rs
View File

@@ -189,14 +189,34 @@ fn thread_tags(thread_ref: &ThreadRef, tags: &mut Vec<Tag>) -> Result<(), SdkErr
Ok(())
}
/// Deduplicate and cap mentions, emitting p-tags.
/// Validate, deduplicate, and cap mentions, emitting p-tags.
///
/// Validation lives here rather than in [`crate::mentions::normalize_mention_pubkeys`]
/// because this helper is the single choke point every mention reaches: it is
/// shared by `build_message`, `build_forum_post`, and `build_forum_comment`, and
/// a caller may hand those builders an explicit list without going through the
/// normalizer at all. Without a check here, `"not-a-pubkey"`, `""`, and
/// `"../../etc/passwd"` were lowercased into `p` tags and signed, so the
/// composer reported a successful mention that could notify nobody.
///
/// [`check_pubkey_hex`] is reused deliberately: it is the SDK's existing
/// structural pubkey contract (64 ASCII hex, returned lowercased) and is already
/// applied before other builders emit `p` tags, so mentions now agree with the
/// rest of this module and yield the same [`SdkError::InvalidInput`]. Adopting
/// `nostr::PublicKey::from_hex`'s stricter on-curve rule would be a wider
/// consistency change across every `check_pubkey_hex` call site, not a
/// mentions-only fix.
///
/// The cap is checked before validation so an over-cap list still reports
/// [`SdkError::TooManyMentions`] rather than being masked by whichever entry
/// happens to be malformed.
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();
let lower = check_pubkey_hex(hex, "mention pubkey")?;
if seen.insert(lower.clone()) {
tags.push(tag(&["p", &lower])?);
}
@@ -2550,6 +2570,12 @@ mod tests {
Uuid::new_v4()
}
/// A distinct well-formed 64-character hex pubkey per index, for tests that
/// need many valid mentions.
fn distinct_pubkey_hex(i: u8) -> String {
format!("{i:02x}{}", "ab".repeat(31))
}
fn tag_values(event: &nostr::Event, key: &str) -> Vec<String> {
event
.tags
@@ -2735,6 +2761,131 @@ mod tests {
assert_eq!(p_tags.len(), 1);
}
/// Every structurally invalid mention must be refused by all three builders
/// that share `mention_tags`, rather than lowercased into a `p` tag and
/// signed. See #6291.
#[test]
fn malformed_mention_pubkeys_are_refused_by_every_builder() {
let cid = uuid();
let root = event_id();
let tr = ThreadRef {
root_event_id: root,
parent_event_id: root,
};
let short = "a".repeat(63);
let long = "a".repeat(65);
// Empty, non-hex, and both off-by-one lengths. The wrong-alphabet cases
// are pinned separately from the wrong-length cases because a
// length-only check would otherwise pass: "zzzz" is caught by length,
// but a 64-character non-hex string is not.
let non_hex_64 = "z".repeat(64);
let path_like_64 = format!("../../etc/passwd{}", "0".repeat(48));
for bad in [
"",
"not-a-pubkey",
"zzzz",
"../../etc/passwd",
short.as_str(),
long.as_str(),
non_hex_64.as_str(),
path_like_64.as_str(),
] {
assert!(
matches!(
build_message(cid, "hi", None, &[bad], false, &[], &[]),
Err(SdkError::InvalidInput(_))
),
"build_message must refuse mention {bad:?}"
);
assert!(
matches!(
build_forum_post(cid, "hi", &[bad], &[]),
Err(SdkError::InvalidInput(_))
),
"build_forum_post must refuse mention {bad:?}"
);
assert!(
matches!(
build_forum_comment(cid, "hi", &tr, &[bad], &[]),
Err(SdkError::InvalidInput(_))
),
"build_forum_comment must refuse mention {bad:?}"
);
}
}
/// One malformed entry must refuse the whole list rather than being silently
/// dropped: a filtered mention looks successful to the composer while
/// notifying nobody.
#[test]
fn one_malformed_mention_refuses_the_whole_list() {
let cid = uuid();
let valid = "abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234";
let result = build_message(cid, "hi", None, &[valid, "nope", valid], false, &[], &[]);
assert!(matches!(result, Err(SdkError::InvalidInput(_))));
}
/// Validation must not break the two behaviours callers already rely on:
/// uppercase hex is legitimate input and is canonicalized to lowercase, and
/// duplicates across cases collapse to a single `p` tag.
#[test]
fn valid_mentions_are_canonicalized_and_deduped_across_case() {
let cid = uuid();
let upper = "ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234ABCD1234";
let lower = upper.to_ascii_lowercase();
let root = event_id();
let tr = ThreadRef {
root_event_id: root,
parent_event_id: root,
};
let ev = sign(build_message(cid, "hi", None, &[upper], false, &[], &[]).unwrap());
assert_eq!(
tag_values(&ev, "p"),
vec![lower.clone()],
"uppercase hex must be accepted and lowercased"
);
// Same pubkey in both spellings is one person, so one p tag.
let ev = sign(build_message(cid, "hi", None, &[upper, &lower], false, &[], &[]).unwrap());
assert_eq!(tag_values(&ev, "p"), vec![lower.clone()]);
let ev = sign(build_forum_post(cid, "hi", &[upper], &[]).unwrap());
assert_eq!(tag_values(&ev, "p"), vec![lower.clone()]);
let ev = sign(build_forum_comment(cid, "hi", &tr, &[upper], &[]).unwrap());
assert_eq!(tag_values(&ev, "p"), vec![lower]);
}
/// The cap is checked before per-entry validation, so an over-cap list of
/// well-formed pubkeys keeps reporting `TooManyMentions`, and a list that is
/// both over-cap *and* malformed reports `TooManyMentions` too rather than
/// changing which error callers see.
#[test]
fn too_many_mentions_takes_precedence_over_validation() {
let cid = uuid();
let hexes: Vec<String> = (0..51u8).map(distinct_pubkey_hex).collect();
let mut refs: Vec<&str> = hexes.iter().map(String::as_str).collect();
refs.push("not-a-pubkey");
assert!(matches!(
build_message(cid, "hi", None, &refs, false, &[], &[]),
Err(SdkError::TooManyMentions)
));
}
/// A list exactly at the cap still signs, so the guard cannot have moved the
/// boundary.
#[test]
fn mentions_at_the_cap_still_sign() {
let cid = uuid();
let hexes: Vec<String> = (0..crate::mentions::MENTION_CAP as u8)
.map(distinct_pubkey_hex)
.collect();
let refs: Vec<&str> = hexes.iter().map(String::as_str).collect();
let ev = sign(build_message(cid, "hi", None, &refs, false, &[], &[]).unwrap());
assert_eq!(tag_values(&ev, "p").len(), crate::mentions::MENTION_CAP);
}
#[test]
fn message_too_many_mentions() {
let cid = uuid();