jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-11-02 15:31:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

broker: give every identity one spelling, and the result one wire door

Browse Source 
Two P2 review findings on #6742, both reproduced from an out-of-crate consumer
before being fixed, and each fix falsified by reverting it.

1. Identity spellings broke correlation (bugs-00, correlate.rs:42).

`channel()` validated a UUID with `Uuid::parse_str` and returned the caller's
spelling untouched, while `correlate_identities` compared the request's and the
outcome's `channelId` as strings. An uppercase request against a host's
canonical lowercase echo of the same channel therefore failed `validate_for` —
a *correct* answer rejected, which is worse than the mismatch the check exists
to catch, because it makes a working host unusable.

Fixed as a class, not as a UUID case. The finding named case; the validator
also admits the unhyphenated, `{braced}`, and `urn:uuid:` forms, and all four
froze un-canonicalized. Hex has the same property. So:

- Canonicalize where a value enters. `channel()` now returns the lowercase
  hyphenated form, and every member holding an identity canonicalizes on
  deserialize too (`channel_id`, `hex64_field`, `absent_or_valued_hex64`). The
  wire is the door no `validated()` covers: those fields are public `String`s,
  so a parsed payload reaches a caller without passing through any validator.
  Both doors delegate to the same function, so they cannot drift.
- Compare parsed identities, not bytes. `correlate_identities` parses each
  `channelId` as a `Uuid`; pubkeys are compared as `PubkeyHex`, which
  lowercases in its only constructor and its serde path.

The two guards are deliberately independent and asserted separately, so neither
is the only thing holding. A genuinely different identity is still rejected.

Coverage is derived as well as enumerated: one test walks the real fixtures,
re-spells every identity-named member, and requires the canonical value back,
so a member added later with the wrong `deserialize_with` fails without anyone
extending a list.

2. `BrokerResult` was a second, lax wire door (bugs-00, mod.rs:586).

The envelope's strict reader requires the exact key set the declared status
admits, but the exported result type also derived its own reader, and that one
accepted and dropped arbitrary siblings: `status: failed` beside an `error` and
a `secretKey`, or a succeeded result beside an `error`. A consumer parsing the
result type directly got an `Ok` value whose complete wire shape had never been
vetted, from bytes that fail through the envelope.

Decision: take the type off the wire — it is no longer `Deserialize`. The
alternative, its own strict reader, would be a second copy of the per-status
contradiction rules, and two copies of a strictness check drift; this hole
exists precisely because one layer's strictness did not reach another's.
Removing the door leaves one implementation and nothing to keep in sync.
Nothing is lost: a bare `{"status": …}` object is not a payload this contract
defines, and the whole module is new in this PR, so it had no consumers.
`Serialize` is retained — it produces the envelope's flattened wire form — so
the wire form is unchanged and this is a read-side restriction only.

The absence is pinned at compile time, since a runtime test cannot call a
`Deserialize` impl that does not exist: a probe resolves to an inherent method
only when the bound holds, and it is first asserted against two types that *do*
have readers, so a broken probe cannot pass silently.

The contract docs gain a section on identity spelling, `validate_for`'s
nine-row table records that comparison is on parsed identities, and the
strictness section records the one-door rule.

Also: adding the identity docs pushed mod.rs to 1,021 lines, past the repo's
1,000-line ceiling, so the strict response reader moved to `broker/wire.rs`
(mod 913 / wire 121) — a pure move, same treatment `correlate.rs` already got.

The derived-coverage test matches identity members case-insensitively. An
earlier revision of it matched the suffix `"EventId"` exactly, which silently
skipped the outcome member spelled `eventId` and left every response-side wire
door unpinned — mutation testing caught it by removing the outcome `channelId`
door and surviving. It now walks response fixtures as well as request ones, and
floors the two directions separately, because a single combined floor would be
satisfied by the request side alone.

Validation: 299 tests pass in buzz-sdk (295 before), workspace clippy `-D
warnings` clean, fmt clean, `cargo doc` warning-free with the new section anchor
verified in the generated HTML, every touched file under 1,000 lines. Seven
mutations, one per guard, each killed by a named test. `cargo test --workspace`
is green except `test_120s_timeout` / `test_cancellation_immediate` in
buzz-pair-relay, a pre-existing flake reproduced 2-in-8 on unmodified main at
c5166f216 where no broker module exists.

Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
This commit is contained in:
ss-dev-00
2026-08-25 03:48:27 -07:00
parent 559eb306ea
commit 7c853e0fba
7 changed files with 746 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
crates/buzz-sdk/src/broker/actions/args.rs
View File

@@ -11,9 +11,10 @@
use serde::{Deserialize, Serialize};
use super::{
absent_or_valued, channel, content, cursor, event_id, is_false, limit, mentions, optional,
required, respond_to, validate_slug, Action, PubkeyHex, DEFAULT_PAGE_LIMIT, MAX_ABOUT_CHARS,
MAX_EMOJI_CHARS, MAX_NAME_CHARS, MAX_PROMPT_CHARS, MAX_SCALAR_CHARS,
absent_or_valued, absent_or_valued_hex64, channel, channel_id, content, cursor, event_id,
hex64_field, is_false, limit, mentions, optional, required, respond_to, validate_slug, Action,
PubkeyHex, DEFAULT_PAGE_LIMIT, MAX_ABOUT_CHARS, MAX_EMOJI_CHARS, MAX_NAME_CHARS,
MAX_PROMPT_CHARS, MAX_SCALAR_CHARS,
};
use crate::SdkError;
@@ -29,11 +30,12 @@ use crate::SdkError;
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct ChannelReadArgs {
/// Channel to read.
#[serde(deserialize_with = "channel_id")]
pub channel_id: String,
/// Narrow to one thread by its root event.
#[serde(
default,
deserialize_with = "absent_or_valued",
deserialize_with = "absent_or_valued_hex64",
skip_serializing_if = "Option::is_none"
)]
pub root_event_id: Option<String>,
@@ -124,6 +126,7 @@ impl ChannelReadArgs {
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct MessagePostArgs {
/// Channel to post in.
#[serde(deserialize_with = "channel_id")]
pub channel_id: String,
/// Message body.
pub content: String,
@@ -154,8 +157,10 @@ impl MessagePostArgs {
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct MessageReplyArgs {
/// Channel containing the parent.
#[serde(deserialize_with = "channel_id")]
pub channel_id: String,
/// Event being replied to.
#[serde(deserialize_with = "hex64_field")]
pub reply_to_event_id: String,
/// Reply body.
pub content: String,
@@ -187,8 +192,10 @@ impl MessageReplyArgs {
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct ReactionAddArgs {
/// Channel containing the target.
#[serde(deserialize_with = "channel_id")]
pub channel_id: String,
/// Event being reacted to.
#[serde(deserialize_with = "hex64_field")]
pub target_event_id: String,
/// Reaction payload — an emoji or a `:shortcode:`.
pub reaction: String,
@@ -335,6 +342,7 @@ impl AgentTarget {
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct AgentsCreateArgs {
/// Channel the new agent is attached to.
#[serde(deserialize_with = "channel_id")]
pub channel_id: String,
/// Name for the new agent.
pub display_name: String,

79
crates/buzz-sdk/src/broker/actions/mod.rs
View File

@@ -278,11 +278,45 @@ fn optional(value: Option<&String>, label: &str, max: usize) -> Result<Option<St
value.map(|value| required(value, label, max)).transpose()
}
/// Validate a channel id and return its **canonical** spelling.
///
/// A UUID has several legal spellings — `Uuid::parse_str` accepts uppercase, the
/// unhyphenated 32-character form, `{braced}`, and `urn:uuid:` — and they all
/// name the same channel. Returning the caller's spelling would freeze a request
/// body that disagrees with the host's canonical echo of the same identity, and
/// the response correlation in
/// [`crate::broker::BrokerResponse::validate_for`] would then reject a correct
/// answer. So this returns the lowercase hyphenated form and nothing else: one
/// identity, one spelling, chosen at the only point that sees the value before it
/// is frozen.
///
/// This is the same treatment [`PubkeyHex::parse`] gives the other identity in
/// this contract, and it is applied at both doors — the validators, and the
/// [`channel_id`] deserializer — so a value cannot reach a caller
/// un-canonicalized whether it was built or parsed.
fn channel(value: &str) -> Result<String, SdkError> {
let value = required(value, "channel", 128)?;
uuid::Uuid::parse_str(&value)
.map_err(|_| SdkError::InvalidInput(format!("invalid channel UUID: {value}")))?;
Ok(value)
.map(|id| id.as_hyphenated().to_string())
.map_err(|_| SdkError::InvalidInput(format!("invalid channel UUID: {value}")))
}
/// Deserialize a `channelId`, canonicalizing it and rejecting a non-UUID.
///
/// The wire is the one door a validator cannot cover: every args and outcome
/// field holding a channel id is a public `String`, so a payload parsed from
/// JSON reaches a caller without passing through any `validated()`. Delegating
/// to [`channel`] means the wire form and the constructed form are canonicalized
/// by the same code, so the two cannot drift, and a malformed channel id never
/// becomes a value at all — the same rule the no-null guard follows.
pub(super) fn channel_id<'de, D>(deserializer: D) -> Result<String, D::Error>
where
D: serde::Deserializer<'de>,
{
use serde::de::Error as _;
let raw = String::deserialize(deserializer)?;
channel(&raw).map_err(D::Error::custom)
}
fn event_id(value: &str, label: &str) -> Result<String, SdkError> {
@@ -295,6 +329,47 @@ fn event_id(value: &str, label: &str) -> Result<String, SdkError> {
Ok(value.to_ascii_lowercase())
}
/// Deserialize a 64-hex identifier (`eventId`, `dTag`), lowercasing it and
/// rejecting anything that is not 64 hex characters.
///
/// Hex admits two spellings of one identifier, so this is the [`channel_id`] rule
/// applied to the contract's other multi-spelling identities. See that function
/// for why the wire needs a door of its own.
///
/// The label is generic because serde reports which member failed; naming the
/// member here as well would be a second copy to keep true.
pub(super) fn hex64_field<'de, D>(deserializer: D) -> Result<String, D::Error>
where
D: serde::Deserializer<'de>,
{
use serde::de::Error as _;
let raw = String::deserialize(deserializer)?;
event_id(&raw, "identifier").map_err(D::Error::custom)
}
/// [`hex64_field`] for an optional member: `null` is still rejected.
///
/// A dedicated function rather than a composition of the two guards, because
/// `deserialize_with` takes one function and both rules — reject `null`,
/// canonicalize the value — apply to the same member. Reaching the `None` arm
/// means the key was present and `null`; see [`absent_or_valued`].
pub(super) fn absent_or_valued_hex64<'de, D>(deserializer: D) -> Result<Option<String>, D::Error>
where
D: serde::Deserializer<'de>,
{
use serde::de::Error as _;
match Option::<String>::deserialize(deserializer)? {
Some(raw) => Ok(Some(
event_id(&raw, "identifier").map_err(D::Error::custom)?,
)),
None => Err(D::Error::custom(
"must not be null; omit the member to mean absent",
)),
}
}
fn content(value: &str) -> Result<String, SdkError> {
if value.trim().is_empty() {
return Err(SdkError::InvalidInput("content must not be empty".into()));

7
crates/buzz-sdk/src/broker/actions/outcomes.rs
View File

@@ -10,8 +10,8 @@
use serde::{Deserialize, Serialize};
use super::{
absent_or_valued, channel, cursor, event_id, required, Action, PubkeyHex, MAX_NAME_CHARS,
MAX_PAGE_LIMIT,
absent_or_valued, channel, channel_id, cursor, event_id, hex64_field, required, Action,
PubkeyHex, MAX_NAME_CHARS, MAX_PAGE_LIMIT,
};
use crate::SdkError;
use nostr::{Event, EventId, Kind, PublicKey, Tags, Timestamp};
@@ -156,6 +156,7 @@ pub struct MessagePage {
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct EventPublished {
/// The published event's id (hex).
#[serde(deserialize_with = "hex64_field")]
pub event_id: String,
/// The published event's kind.
pub kind: u32,
@@ -175,6 +176,7 @@ pub struct StorageAddress {
/// Event kind holding the record.
pub kind: u32,
/// Derived `d` tag (64 hex characters).
#[serde(deserialize_with = "hex64_field")]
pub d_tag: String,
}
@@ -192,6 +194,7 @@ pub struct AgentsCreateOutcome {
/// The new agent's name as stored.
pub display_name: String,
/// Channel the agent was attached to.
#[serde(deserialize_with = "channel_id")]
pub channel_id: String,
}

66
crates/buzz-sdk/src/broker/correlate.rs
View File

@@ -13,20 +13,64 @@ use super::{ActionArgs, ActionOutcome, AgentTarget, PubkeyHex, SdkError};
/// public entry point a host author reads. It is documented there rather than
/// here so there is one copy to keep true.
///
/// # Identities are compared parsed, never as strings
///
/// Both identities in this contract have more than one legal spelling: a UUID
/// may be uppercase, unhyphenated, braced, or `urn:uuid:`-prefixed, and hex may
/// be either case. A byte comparison would therefore reject a *correct* answer
/// whenever the caller and the host spelled the same identity differently —
/// which is a worse failure than the one this function exists to catch, because
/// it makes a working host unusable.
///
/// Canonicalization at the validators and at the wire doors (see
/// [`channel_id`][super::actions::channel_id]) means both sides normally arrive
/// canonical already. This function does not rely on that: it parses each side
/// into the identity's own type and compares those, so a spelling variant that
/// reached here by any route still correlates. The two guards are deliberately
/// independent — neither is load-bearing alone.
///
/// The match below is exhaustive over [`ActionArgs`], so adding an action is a
/// compile error here rather than a silent default to "not compared".
pub(super) fn correlate_identities(
args: &ActionArgs,
outcome: &ActionOutcome,
) -> Result<(), SdkError> {
/// Compare one echoed identity, naming the field in the error.
fn same(field: &str, requested: &str, returned: &str) -> Result<(), SdkError> {
/// Compare two channel ids as UUIDs, so two spellings of one channel match.
///
/// An unparseable id on either side is a mismatch rather than an error:
/// `validate`/`validated` already reject a malformed channel id with a
/// precise message, and duplicating that verdict here would report a
/// correlation failure for what is really a malformed payload.
fn same_channel(requested: &str, returned: &str) -> Result<(), SdkError> {
let parse = |value: &str| uuid::Uuid::parse_str(value).ok();
match (parse(requested), parse(returned)) {
(Some(requested_id), Some(returned_id)) if requested_id == returned_id => Ok(()),
_ => Err(mismatch("channelId", requested, returned)),
}
}
/// Compare two pubkeys.
///
/// [`PubkeyHex`] lowercases in [`PubkeyHex::parse`], which is its only
/// constructor and also its `Deserialize` path, so a value of this type is
/// already canonical however it was built. Comparing the typed values is
/// therefore the parsed comparison, not a string one.
fn same_pubkey(requested: &PubkeyHex, returned: &PubkeyHex) -> Result<(), SdkError> {
if requested == returned {
return Ok(());
}
Err(SdkError::InvalidInput(format!(
Err(mismatch(
"agentPubkey",
requested.as_str(),
returned.as_str(),
))
}
/// The error every mismatch reports, naming the field and both spellings.
fn mismatch(field: &str, requested: &str, returned: &str) -> SdkError {
SdkError::InvalidInput(format!(
"response {field} \"{returned}\" does not match the requested \"{requested}\""
)))
))
}
/// The pubkey a target names immutably, if it names one.
@@ -39,25 +83,17 @@ pub(super) fn correlate_identities(
match (args, outcome) {
(ActionArgs::AgentsCreate(args), ActionOutcome::AgentsCreate(outcome)) => {
same("channelId", &args.channel_id, &outcome.channel_id)
same_channel(&args.channel_id, &outcome.channel_id)
}
(ActionArgs::AgentsUpdate(args), ActionOutcome::AgentsUpdate(outcome)) => {
match targeted(&args.target) {
Some(requested) => same(
"agentPubkey",
requested.as_str(),
outcome.agent_pubkey.as_str(),
),
Some(requested) => same_pubkey(requested, &outcome.agent_pubkey),
None => Ok(()),
}
}
(ActionArgs::AgentsDelete(args), ActionOutcome::AgentsDelete(outcome)) => {
match targeted(&args.target) {
Some(requested) => same(
"agentPubkey",
requested.as_str(),
outcome.agent_pubkey.as_str(),
),
Some(requested) => same_pubkey(requested, &outcome.agent_pubkey),
None => Ok(()),
}
}

181
crates/buzz-sdk/src/broker/mod.rs
View File

@@ -69,6 +69,33 @@
//! nested action object — a host cannot ship a `secretKey` beside `sig`, or
//! beside `args`, and have it silently trimmed.
//!
//! There is also exactly **one** wire door per payload, so no lax reader sits
//! beside a strict one. [`BrokerResult`] is the case that needed work: its
//! members reach the wire only flattened into [`BrokerResponse`], but it also
//! derived a reader of its own, and that reader accepted and dropped siblings the
//! envelope rejects. It is no longer [`Deserialize`] — see the reasoning on that
//! type. Removing the second door is what keeps the rule single-sourced, since two
//! copies of a strictness check are exactly how this hole arose.
//!
//! # Identities have exactly one spelling
//!
//! Both identity types in this contract admit several legal spellings. A UUID may
//! be uppercase, unhyphenated, `{braced}`, or `urn:uuid:`-prefixed; hex may be
//! either case. Two spellings of one identity are the same identity, so the
//! contract picks one and normalizes to it: **lowercase hyphenated** for a
//! `channelId`, **lowercase** for a pubkey, `eventId`, or `dTag`.
//!
//! Normalization happens at both doors — each `validated()` and the
//! [`Deserialize`] impl of every member that holds an identity — so a value is
//! canonical whether it was built in Rust or parsed from JSON, and the frozen
//! request body carries the canonical spelling rather than the caller's. A host
//! may send any legal spelling and will be read as having sent the canonical one.
//!
//! Without this, [`BrokerResponse::validate_for`] would have compared the
//! caller's spelling of a `channelId` against the host's canonical echo of the
//! same channel and rejected a correct answer. That check compares parsed
//! identities as well, so neither guard is load-bearing alone.
//!
//! # Duplicate members are rejected
//!
//! A key may appear at most once in any object. serde's derived readers reject a
@@ -155,6 +182,7 @@ use crate::SdkError;
pub mod actions;
pub mod client;
mod correlate;
mod wire;
use actions::absent_or_valued;
pub use actions::{
@@ -582,7 +610,32 @@ impl BrokerError {
/// `Failed` promises no side effects took hold, while `Indeterminate` promises
/// nothing at all and demands reconciliation. Which [`BrokerErrorCode`] may
/// carry which status is a closed table, documented on that type.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
///
/// # Why this type is not [`Deserialize`]
///
/// It is deliberately **not readable from the wire**, and that is the point:
/// [`BrokerResponse`] is the only door, and it is strict.
///
/// This type has no wire form of its own. Its members only ever appear flattened
/// into the response envelope, whose strict reader requires the exact key set the
/// declared status admits — so `status: failed` beside an `error` and a
/// `secretKey`, or a succeeded result beside an `error`, fail to parse. A derived
/// reader on this type answered the same bytes with `Ok`, dropping the members it
/// could not represent, so a consumer that parsed the result directly got a value
/// whose complete wire shape had never been checked.
///
/// The two ways to close that were to give this type its own strict reader, or to
/// take it off the wire. A second strict reader would be a second copy of the
/// per-status contradiction rules, and two copies of a security check drift — the
/// hole above exists precisely because one layer's strictness did not reach
/// another's. Removing the door leaves one implementation of the rule and nothing
/// to keep in sync. Nothing is lost: a bare `{"status": …}` object is not a
/// payload this contract defines, so no host or client had a legitimate reason to
/// parse one.
///
/// [`Serialize`] is retained — it is what produces the envelope's flattened wire
/// form — so this is a read-side restriction only, and the wire form is unchanged.
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
#[serde(tag = "status", rename_all = "snake_case")]
pub enum BrokerResult {
/// The action completed and produced this outcome.
@@ -683,116 +736,6 @@ pub struct BrokerResponse {
pub replayed: bool,
}
/// The strict wire form of a [`BrokerResponse`]: every key spelled out, no
/// `flatten`, so `deny_unknown_fields` is actually in force.
///
/// The status-specific members are `Option` here only because one struct has to
/// describe three shapes; [`BrokerResponse::deserialize`] then requires the exact
/// set for the declared status, so `error` beside a succeeded outcome — or a
/// missing `outcome` under `succeeded` — is a parse failure rather than a field
/// nobody reads.
///
/// Those members deserialize through [`absent_or_valued`] rather than plain
/// `#[serde(default)]`, because the status match below reads `None` as *absent*
/// and `#[serde(default)] Option<T>` also produces `None` for an explicit
/// `null`. Without it, `{"status":"failed","action":null,"outcome":null}` — or a
/// succeeded response with `"error":null` — parsed as well-formed and skipped the
/// contradiction check entirely. `null` is now rejected for these members whether
/// or not the declared status admits them, so there is exactly one way for a
/// member to be absent.
///
/// `outcome` is held as a `serde_json::Value` and re-deserialized under its
/// action tag, so this reader is JSON-specific. That is not a narrowing: the HTTP
/// binding in [`client`] declares `application/json`, and JSON is the only
/// encoding this contract has ever specified.
#[derive(Deserialize)]
#[serde(deny_unknown_fields, rename_all = "camelCase")]
struct WireResponse {
r#type: String,
protocol_version: u16,
request_id: String,
status: String,
#[serde(default, deserialize_with = "absent_or_valued")]
action: Option<String>,
#[serde(default, deserialize_with = "absent_or_valued")]
outcome: Option<Box<serde_json::value::RawValue>>,
#[serde(default, deserialize_with = "absent_or_valued")]
error: Option<BrokerError>,
#[serde(default)]
replayed: bool,
}
impl<'de> Deserialize<'de> for BrokerResponse {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
use serde::de::Error as _;
let wire = WireResponse::deserialize(deserializer)?;
// One arm per status, each naming the members that status may carry.
// Anything present that this status does not admit is rejected here, so
// "succeeded with an error" cannot be parsed and then ignored.
let result = match wire.status.as_str() {
"succeeded" => {
if wire.error.is_some() {
return Err(D::Error::custom(
"a succeeded response must not carry an error",
));
}
let action = wire
.action
.ok_or_else(|| D::Error::missing_field("action"))?;
let outcome = wire
.outcome
.ok_or_else(|| D::Error::missing_field("outcome"))?;
// Re-deserialize the outcome under its action tag, which is how
// the adjacently-tagged `ActionOutcome` — and the
// `deny_unknown_fields` on each outcome type — get applied.
// Re-deserialize from the original bytes, not from a
// `serde_json::Value`: buffering through `Value` collapses
// duplicate keys last-wins, which would let `outcome` carry a
// duplicate that no other part of this contract accepts.
let tagged = format!(
"{{\"action\":{},\"outcome\":{}}}",
serde_json::to_string(&action).map_err(D::Error::custom)?,
outcome.get()
);
let outcome: ActionOutcome =
serde_json::from_str(&tagged).map_err(D::Error::custom)?;
BrokerResult::Succeeded { outcome }
}
status @ ("failed" | "indeterminate") => {
if wire.action.is_some() || wire.outcome.is_some() {
return Err(D::Error::custom(format!(
"a {status} response must not carry an action or outcome"
)));
}
let error = wire.error.ok_or_else(|| D::Error::missing_field("error"))?;
if status == "failed" {
BrokerResult::Failed { error }
} else {
BrokerResult::Indeterminate { error }
}
}
other => {
return Err(D::Error::custom(format!(
"unknown broker result status \"{other}\""
)))
}
};
Ok(Self {
r#type: wire.r#type,
protocol_version: wire.protocol_version,
request_id: wire.request_id,
result,
replayed: wire.replayed,
})
}
}
fn is_false(value: &bool) -> bool {
!*value
}
@@ -894,8 +837,8 @@ impl BrokerResponse {
///
/// `requestId` plus action is not enough: a host routing bug can return a
/// well-formed success for the wrong *subject*. Every identity the request
/// supplies and the outcome echoes must match exactly. This is the whole
/// table:
/// supplies and the outcome echoes must name the same thing. This is the
/// whole table:
///
/// | Action | compared | not compared, and why |
/// |---|---|---|
@@ -905,10 +848,20 @@ impl BrokerResponse {
/// | `reaction.add` | — | same |
/// | `profile.set` | — | same |
/// | `storage.address` | — | `slug` is deliberately absent from the outcome: a `d` tag is a keyed hash of it, and echoing the slug would defeat that |
/// | `agents.create` | `channelId` | pubkey and name are newly minted, so there is nothing prior to compare |
/// | `agents.create` | `channelId`, as UUIDs | pubkey and name are newly minted, so there is nothing prior to compare |
/// | `agents.update` | `agentPubkey` when targeted by pubkey | a name target is resolved host-side; `displayName` may be exactly what this call changed |
/// | `agents.delete` | `agentPubkey` when targeted by pubkey | ditto for a name target |
///
/// **Comparison is on parsed identities, never on bytes.** Both identity
/// types here admit more than one legal spelling — a UUID may be uppercase,
/// unhyphenated, braced or `urn:uuid:`-prefixed; hex may be either case — and
/// two spellings of one identity are the same identity. A byte comparison
/// would reject a correct answer whenever the caller and the host spelled it
/// differently, which is a worse failure than the one this check exists to
/// catch. Values are also canonicalized where they enter — at each
/// `validated()` and at the wire — so both sides normally arrive canonical;
/// the parsed comparison does not depend on that having happened.
///
/// A name-targeted `agents.update`/`agents.delete` is the one case where the
/// caller asked for an identity it cannot verify in the reply. That is
/// inherent: the host resolves the name, and a rename may be the very thing

415
crates/buzz-sdk/src/broker/tests.rs
View File

@@ -1512,6 +1512,421 @@ fn correlation_rejects_an_outcome_naming_a_different_subject() {
}
}
// ── Identities have one spelling ────────────────────────────────────────────
/// Every legal spelling of a channel UUID names one channel, so a request and a
/// response that spell it differently must still correlate.
///
/// The bug: `Uuid::parse_str` accepts uppercase, unhyphenated, braced, and
/// `urn:uuid:` forms, `channel()` returned the caller's spelling untouched, and
/// correlation compared bytes — so an uppercase request against a host's
/// canonical lowercase echo of the *same* channel failed `validate_for`. That is
/// worse than the mismatch the check exists to catch: it makes a correct host
/// unusable.
///
/// Two independent guards close it, and each is asserted separately below so
/// neither can be the only thing holding: canonicalize where a value enters, and
/// compare parsed identities rather than bytes.
#[test]
fn one_identity_spelled_two_ways_still_correlates() {
let spellings = [
CHANNEL.to_ascii_uppercase(),
CHANNEL.replace('-', ""),
format!("{{{CHANNEL}}}"),
format!("urn:uuid:{CHANNEL}"),
CHANNEL.to_string(),
];
let create = |channel_id: &str| {
ActionArgs::AgentsCreate(AgentsCreateArgs {
channel_id: channel_id.into(),
display_name: "Helper".into(),
system_prompt: "be useful".into(),
runtime: None,
provider: None,
model: None,
respond_to: None,
})
};
let echo = |channel_id: &str| {
BrokerResult::succeeded(ActionOutcome::AgentsCreate(AgentsCreateOutcome {
agent_pubkey: pubkey(),
display_name: "Helper".into(),
channel_id: channel_id.into(),
}))
};
for spelling in &spellings {
// Guard 1: the frozen body carries the canonical spelling, not the
// caller's, so what the host receives is what correlation will compare.
let request = BrokerRequest::new("req-spelling", create(spelling))
.expect("every legal UUID spelling validates");
let body = String::from_utf8(request.prepare().expect("prepares").body().to_vec())
.expect("body is utf8");
assert!(
body.contains(&format!("\"channelId\":\"{CHANNEL}\"")),
"frozen body did not canonicalize \"{spelling}\": {body}"
);
// And through the wire door too, which no `validated()` covers: a parsed
// request reaches a caller canonical.
let parsed: BrokerRequest = serde_json::from_value(serde_json::json!({
"type": BROKER_REQUEST_TYPE,
"protocolVersion": 1,
"requestId": "req-spelling",
"actionVersion": 1,
"action": "channel.read",
"args": { "channelId": spelling },
}))
.unwrap_or_else(|e| panic!("\"{spelling}\" must parse: {e}"));
assert_eq!(
parsed.action,
ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL)),
"the wire door did not canonicalize \"{spelling}\""
);
// Guard 2: correlation compares parsed identities, so every spelling on
// either side correlates even if guard 1 were absent.
let prepared = BrokerRequest::new("req-spelling", create(spelling))
.expect("validates")
.prepare()
.expect("prepares");
for returned in &spellings {
BrokerResponse::new("req-spelling", echo(returned))
.validate_for(&prepared)
.unwrap_or_else(|e| {
panic!("request \"{spelling}\" vs echo \"{returned}\" must correlate: {e}")
});
}
}
// A genuinely different channel is still rejected, so the fix widened what
// counts as equal without weakening the check.
let prepared = BrokerRequest::new("req-spelling", create(CHANNEL))
.expect("validates")
.prepare()
.expect("prepares");
let err = BrokerResponse::new("req-spelling", echo("c2c38ca8-9ec3-411e-bab5-f9deab34d52e"))
.validate_for(&prepared)
.expect_err("a different channel must still be rejected");
assert!(matches!(err, SdkError::InvalidInput(_)), "{err:?}");
}
/// The same treatment for the contract's other multi-spelling identities: hex.
///
/// A pubkey was already canonicalized by `PubkeyHex::parse`, which is also its
/// serde path — this pins that it is, so the `agentPubkey` rows of the
/// correlation table cannot regress into a byte comparison of two cases. Event
/// ids and `d` tags are plain `String`s and were *not* normalized on the wire,
/// only in `validated()`, so those are the ones this changes.
#[test]
fn hex_identities_are_canonical_through_every_door() {
// Pubkey: mixed-case target vs lowercase echo correlates, both directions.
let upper = PubkeyHex::parse(PUBKEY.to_ascii_uppercase()).expect("valid hex");
let request = BrokerRequest::new(
"req-hex",
ActionArgs::AgentsDelete(AgentsDeleteArgs {
target: AgentTarget::Pubkey(upper),
}),
)
.expect("validates")
.prepare()
.expect("prepares");
BrokerResponse::new(
"req-hex",
BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome {
agent_pubkey: pubkey(),
display_name: "Gone".into(),
})),
)
.validate_for(&request)
.expect("two cases of one pubkey are one identity");
// Event ids and d tags: the wire door lowercases, so a parsed value equals a
// constructed one and neither carries the sender's case.
let parsed: ActionArgs = serde_json::from_value(serde_json::json!({
"action": "reaction.add",
"args": {
"channelId": CHANNEL,
"targetEventId": EVENT.to_ascii_uppercase(),
"reaction": "\u{1f41d}",
},
}))
.expect("an uppercase event id parses");
assert_eq!(
parsed,
ActionArgs::ReactionAdd(ReactionAddArgs {
channel_id: CHANNEL.into(),
target_event_id: EVENT.into(),
reaction: "\u{1f41d}".into(),
}),
"the wire door did not lowercase targetEventId"
);
let parsed: ActionOutcome = serde_json::from_value(serde_json::json!({
"action": "storage.address",
"outcome": {
"authorPubkey": PUBKEY.to_ascii_uppercase(),
"kind": 30078,
"dTag": EVENT.to_ascii_uppercase(),
},
}))
.expect("an uppercase d tag parses");
assert_eq!(
parsed,
ActionOutcome::StorageAddress(StorageAddress {
author_pubkey: pubkey(),
kind: 30078,
d_tag: EVENT.into(),
}),
"the wire door did not lowercase dTag or authorPubkey"
);
// The optional identity member takes the same door, and still rejects null.
let read: ActionArgs = serde_json::from_value(serde_json::json!({
"action": "channel.read",
"args": { "channelId": CHANNEL, "rootEventId": EVENT.to_ascii_uppercase() },
}))
.expect("an uppercase root event id parses");
assert_eq!(
read,
ActionArgs::ChannelRead(ChannelReadArgs {
channel_id: CHANNEL.into(),
root_event_id: Some(EVENT.into()),
..ChannelReadArgs::default()
}),
);
assert!(
serde_json::from_value::<ActionArgs>(serde_json::json!({
"action": "channel.read",
"args": { "channelId": CHANNEL, "rootEventId": serde_json::Value::Null },
}))
.is_err(),
"canonicalizing must not have replaced the null guard"
);
// A malformed identity is still a parse failure, so the new doors reject
// rather than merely normalize.
for bad in ["nothex", "", &EVENT[..40], &format!("{EVENT}00")] {
assert!(
serde_json::from_value::<ActionArgs>(serde_json::json!({
"action": "channel.read",
"args": { "channelId": CHANNEL, "rootEventId": bad },
}))
.is_err(),
"rootEventId \"{bad}\" must not deserialize"
);
}
assert!(
serde_json::from_value::<ActionArgs>(serde_json::json!({
"action": "channel.read",
"args": { "channelId": "not-a-uuid" },
}))
.is_err(),
"a non-UUID channelId must not deserialize"
);
}
/// `BrokerResult` must have **no wire door of its own**, so the strict envelope is
/// the only way to read a result.
///
/// The bug: the exported result type derived its own reader, which accepted and
/// dropped arbitrary siblings — `status: failed` beside an `error` and a
/// `secretKey`, or a succeeded result beside an `error`. A consumer parsing the
/// result type directly therefore got an `Ok` value whose complete wire shape had
/// never been vetted, while the identical bytes failed through the envelope.
///
/// Removing the door is checked at compile time, because a runtime test cannot
/// call a `Deserialize` impl that does not exist. `absence_of_a_reader` resolves to
/// the inherent function only when the bound holds, so this is a genuine negative
/// assertion rather than a comment.
#[test]
fn the_result_type_has_no_deserializer_of_its_own() {
struct Probe<T>(std::marker::PhantomData<T>);
trait NoReader {
fn absence_of_a_reader() -> bool {
true
}
}
impl<T> NoReader for Probe<T> {}
impl<T: serde::de::DeserializeOwned> Probe<T> {
fn absence_of_a_reader() -> bool {
false
}
}
// The probe must be able to see a reader that *is* there, or its `true`
// means nothing.
assert!(
!Probe::<BrokerResponse>::absence_of_a_reader(),
"probe is broken: it reports no reader for a type that has one"
);
assert!(
!Probe::<ActionOutcome>::absence_of_a_reader(),
"probe is broken: it reports no reader for a type that has one"
);
assert!(
Probe::<BrokerResult>::absence_of_a_reader(),
"BrokerResult must not be Deserialize: it is a second, lax wire door"
);
// And the exact byte sequences the old direct reader accepted are rejected
// through the one door that remains. Each is the envelope form of what
// bugs-00 reported, since a bare result object is no longer parseable at all.
let envelope = |extra: serde_json::Value| {
let mut json = serde_json::json!({
"type": BROKER_RESULT_TYPE,
"protocolVersion": 1,
"requestId": "req-1",
});
for (key, value) in extra.as_object().expect("object").clone() {
json[key] = value;
}
json
};
let reported = [
(
"failed with an error and a secretKey",
serde_json::json!({
"status": "failed",
"error": { "code": "action_failed", "message": "no" },
"secretKey": "nsec1deadbeef",
}),
),
(
"succeeded beside an error",
serde_json::json!({
"status": "succeeded",
"action": "agents.delete",
"outcome": { "agentPubkey": PUBKEY, "displayName": "Gone" },
"error": { "code": "action_failed", "message": "no" },
}),
),
];
for (what, body) in reported {
let json = envelope(body);
assert!(
serde_json::from_value::<BrokerResponse>(json.clone()).is_err(),
"{what} must not deserialize through the envelope either: {json}"
);
}
}
/// Derived coverage for the canonicalization rule, so a *newly added* identity
/// member is covered without anyone remembering to extend a list.
///
/// The two tests above name the members that exist today. This one walks the real
/// fixtures — requests *and* responses, since both directions carry identities
/// through separate code — finds every member whose name marks it as an identity,
/// re-spells its value, and requires the payload to parse back to the canonical
/// value. A field added later with the wrong (or no) `deserialize_with` fails here.
///
/// Matching on the member *name* is the point: the naming convention is what a
/// reviewer sees, so if a member is named like an identity it is held to the
/// identity rule. A member holding an identity under some other name would escape
/// this, which is why the audit above is by type as well.
///
/// The suffix match is case-insensitive on purpose. An earlier revision matched
/// `"EventId"` exactly, which silently skipped the outcome member spelled
/// `eventId` and left every response-side door unpinned — a mutation removing
/// that door survived. Matching how a *reader* groups these names, rather than
/// how one of them happens to be capitalized, is what closes that gap.
#[test]
fn every_identity_shaped_member_is_canonicalized_on_the_wire() {
/// A member-name suffix and how a sender might legally re-spell its value.
type Respelling = (&'static str, fn(&str) -> String);
// Every identity in this contract is hex or a UUID, so case is the
// re-spelling they all admit; `channelId` additionally admits the forms
// covered by `one_identity_spelled_two_ways_still_correlates`.
let respellings: [Respelling; 4] = [
("channelid", |v| v.to_ascii_uppercase()),
("eventid", |v| v.to_ascii_uppercase()),
("pubkey", |v| v.to_ascii_uppercase()),
("dtag", |v| v.to_ascii_uppercase()),
];
/// Re-spell every identity-named member of `valid` in turn and require the
/// payload to parse back to `original`. Returns how many members it checked.
fn respell_each<T>(valid: &serde_json::Value, original: &T, respellings: &[Respelling]) -> usize
where
T: serde::de::DeserializeOwned + PartialEq + std::fmt::Debug,
{
let mut checked = 0;
let mut paths = Vec::new();
member_paths(valid, "", &mut paths);
for path in paths {
let Some(name) = path.rsplit('/').next() else {
continue;
};
let lowered = name.to_ascii_lowercase();
let Some((_, respell)) = respellings
.iter()
.find(|(suffix, _)| lowered.ends_with(suffix))
else {
continue;
};
let Some(current) = valid
.pointer(&path)
.expect("path addresses a member")
.as_str()
else {
continue;
};
let respelled = respell(current);
if respelled == current {
continue;
}
let mut json = valid.clone();
*json.pointer_mut(&path).expect("path addresses a member") =
serde_json::Value::String(respelled.clone());
let parsed: T = serde_json::from_value(json)
.unwrap_or_else(|e| panic!("\"{respelled}\" at {path} must parse: {e}"));
assert_eq!(
&parsed, original,
"member {path} did not canonicalize \"{respelled}\" back to \"{current}\""
);
checked += 1;
}
checked
}
let mut request_members = 0;
for args in action_fixtures() {
let request = BrokerRequest::new("req-canon", args).expect("fixture request builds");
let valid = serde_json::to_value(&request).expect("request serializes");
request_members += respell_each(&valid, &request, &respellings);
}
// The response side carries identities too — `agents.create` echoes a
// `channelId`, `storage.address` a `dTag`, the publishing outcomes an
// `eventId` and an `authorPubkey` — and those doors are separate code from
// the request side's.
let keys = Keys::generate();
let mut response_members = 0;
for outcome in outcome_fixtures(&keys) {
let response = BrokerResponse::new("req-canon", BrokerResult::succeeded(outcome));
let valid = serde_json::to_value(&response).expect("response serializes");
response_members += respell_each(&valid, &response, &respellings);
}
// Guard the guard: a rule that silently matched nothing would pass forever.
// The two directions are floored *separately* on purpose — one combined
// total would be satisfied by the request side alone, which is exactly the
// blind spot that let a response-side door go unpinned.
assert!(
request_members >= 8,
"expected identity members across the request fixtures, checked {request_members}"
);
assert!(
response_members >= 6,
"expected identity members across the response fixtures, checked {response_members}"
);
}
// ── Reads carry verifiable provenance ───────────────────────────────────────
/// A read returns the signed event, so a keyless caller can check authorship

119
crates/buzz-sdk/src/broker/wire.rs Normal file
View File

@@ -0,0 +1,119 @@
//! The strict wire form of a [`BrokerResponse`].
//!
//! Split from [`super`] to keep that file within the repo's 1,000-line ceiling.
//! This is the response side's only reader, so it is the one place the envelope's
//! strictness is defined.
use serde::Deserialize;
use super::{absent_or_valued, ActionOutcome, BrokerError, BrokerResponse, BrokerResult};
/// The strict wire form of a [`BrokerResponse`]: every key spelled out, no
/// `flatten`, so `deny_unknown_fields` is actually in force.
///
/// The status-specific members are `Option` here only because one struct has to
/// describe three shapes; [`BrokerResponse::deserialize`] then requires the exact
/// set for the declared status, so `error` beside a succeeded outcome — or a
/// missing `outcome` under `succeeded` — is a parse failure rather than a field
/// nobody reads.
///
/// Those members deserialize through [`absent_or_valued`] rather than plain
/// `#[serde(default)]`, because the status match below reads `None` as *absent*
/// and `#[serde(default)] Option<T>` also produces `None` for an explicit
/// `null`. Without it, `{"status":"failed","action":null,"outcome":null}` — or a
/// succeeded response with `"error":null` — parsed as well-formed and skipped the
/// contradiction check entirely. `null` is now rejected for these members whether
/// or not the declared status admits them, so there is exactly one way for a
/// member to be absent.
///
/// `outcome` is held as a `serde_json::Value` and re-deserialized under its
/// action tag, so this reader is JSON-specific. That is not a narrowing: the HTTP
/// binding in [`client`] declares `application/json`, and JSON is the only
/// encoding this contract has ever specified.
#[derive(Deserialize)]
#[serde(deny_unknown_fields, rename_all = "camelCase")]
struct WireResponse {
r#type: String,
protocol_version: u16,
request_id: String,
status: String,
#[serde(default, deserialize_with = "absent_or_valued")]
action: Option<String>,
#[serde(default, deserialize_with = "absent_or_valued")]
outcome: Option<Box<serde_json::value::RawValue>>,
#[serde(default, deserialize_with = "absent_or_valued")]
error: Option<BrokerError>,
#[serde(default)]
replayed: bool,
}
impl<'de> Deserialize<'de> for BrokerResponse {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
use serde::de::Error as _;
let wire = WireResponse::deserialize(deserializer)?;
// One arm per status, each naming the members that status may carry.
// Anything present that this status does not admit is rejected here, so
// "succeeded with an error" cannot be parsed and then ignored.
let result = match wire.status.as_str() {
"succeeded" => {
if wire.error.is_some() {
return Err(D::Error::custom(
"a succeeded response must not carry an error",
));
}
let action = wire
.action
.ok_or_else(|| D::Error::missing_field("action"))?;
let outcome = wire
.outcome
.ok_or_else(|| D::Error::missing_field("outcome"))?;
// Re-deserialize the outcome under its action tag, which is how
// the adjacently-tagged `ActionOutcome` — and the
// `deny_unknown_fields` on each outcome type — get applied.
// Re-deserialize from the original bytes, not from a
// `serde_json::Value`: buffering through `Value` collapses
// duplicate keys last-wins, which would let `outcome` carry a
// duplicate that no other part of this contract accepts.
let tagged = format!(
"{{\"action\":{},\"outcome\":{}}}",
serde_json::to_string(&action).map_err(D::Error::custom)?,
outcome.get()
);
let outcome: ActionOutcome =
serde_json::from_str(&tagged).map_err(D::Error::custom)?;
BrokerResult::Succeeded { outcome }
}
status @ ("failed" | "indeterminate") => {
if wire.action.is_some() || wire.outcome.is_some() {
return Err(D::Error::custom(format!(
"a {status} response must not carry an action or outcome"
)));
}
let error = wire.error.ok_or_else(|| D::Error::missing_field("error"))?;
if status == "failed" {
BrokerResult::Failed { error }
} else {
BrokerResult::Indeterminate { error }
}
}
other => {
return Err(D::Error::custom(format!(
"unknown broker result status \"{other}\""
)))
}
};
Ok(Self {
r#type: wire.r#type,
protocol_version: wire.protocol_version,
request_id: wire.request_id,
result,
replayed: wire.replayed,
})
}
}