diff --git a/crates/buzz-sdk/src/broker/actions/args.rs b/crates/buzz-sdk/src/broker/actions/args.rs index a5a75ed2cc..0fff4b09be 100644 --- a/crates/buzz-sdk/src/broker/actions/args.rs +++ b/crates/buzz-sdk/src/broker/actions/args.rs @@ -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, @@ -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, diff --git a/crates/buzz-sdk/src/broker/actions/mod.rs b/crates/buzz-sdk/src/broker/actions/mod.rs index b45b2af170..654a8dd346 100644 --- a/crates/buzz-sdk/src/broker/actions/mod.rs +++ b/crates/buzz-sdk/src/broker/actions/mod.rs @@ -278,11 +278,45 @@ fn optional(value: Option<&String>, label: &str, max: usize) -> Result Result { 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 +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 { @@ -295,6 +329,47 @@ fn event_id(value: &str, label: &str) -> Result { 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 +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, D::Error> +where + D: serde::Deserializer<'de>, +{ + use serde::de::Error as _; + + match Option::::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 { if value.trim().is_empty() { return Err(SdkError::InvalidInput("content must not be empty".into())); diff --git a/crates/buzz-sdk/src/broker/actions/outcomes.rs b/crates/buzz-sdk/src/broker/actions/outcomes.rs index 2590722749..61a3b08d02 100644 --- a/crates/buzz-sdk/src/broker/actions/outcomes.rs +++ b/crates/buzz-sdk/src/broker/actions/outcomes.rs @@ -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, } diff --git a/crates/buzz-sdk/src/broker/correlate.rs b/crates/buzz-sdk/src/broker/correlate.rs index f9f7bf735c..0662cf60b5 100644 --- a/crates/buzz-sdk/src/broker/correlate.rs +++ b/crates/buzz-sdk/src/broker/correlate.rs @@ -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(()), } } diff --git a/crates/buzz-sdk/src/broker/mod.rs b/crates/buzz-sdk/src/broker/mod.rs index 3823047221..2efdda57a5 100644 --- a/crates/buzz-sdk/src/broker/mod.rs +++ b/crates/buzz-sdk/src/broker/mod.rs @@ -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` 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, - #[serde(default, deserialize_with = "absent_or_valued")] - outcome: Option>, - #[serde(default, deserialize_with = "absent_or_valued")] - error: Option, - #[serde(default)] - replayed: bool, -} - -impl<'de> Deserialize<'de> for BrokerResponse { - fn deserialize(deserializer: D) -> Result - 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 diff --git a/crates/buzz-sdk/src/broker/tests.rs b/crates/buzz-sdk/src/broker/tests.rs index c5d518b56f..35660ce2c8 100644 --- a/crates/buzz-sdk/src/broker/tests.rs +++ b/crates/buzz-sdk/src/broker/tests.rs @@ -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::(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::(serde_json::json!({ + "action": "channel.read", + "args": { "channelId": CHANNEL, "rootEventId": bad }, + })) + .is_err(), + "rootEventId \"{bad}\" must not deserialize" + ); + } + assert!( + serde_json::from_value::(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(std::marker::PhantomData); + + trait NoReader { + fn absence_of_a_reader() -> bool { + true + } + } + impl NoReader for Probe {} + + impl Probe { + 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::::absence_of_a_reader(), + "probe is broken: it reports no reader for a type that has one" + ); + assert!( + !Probe::::absence_of_a_reader(), + "probe is broken: it reports no reader for a type that has one" + ); + assert!( + Probe::::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::(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(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 diff --git a/crates/buzz-sdk/src/broker/wire.rs b/crates/buzz-sdk/src/broker/wire.rs new file mode 100644 index 0000000000..73cc1bbdd1 --- /dev/null +++ b/crates/buzz-sdk/src/broker/wire.rs @@ -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` 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, + #[serde(default, deserialize_with = "absent_or_valued")] + outcome: Option>, + #[serde(default, deserialize_with = "absent_or_valued")] + error: Option, + #[serde(default)] + replayed: bool, +} + +impl<'de> Deserialize<'de> for BrokerResponse { + fn deserialize(deserializer: D) -> Result + 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, + }) + } +}