From bf718e644f4d2b5d186a3f77ffc28c279f5271fc Mon Sep 17 00:00:00 2001 From: Hayt <41ea58f1e64c243627e8acde7c89be667052ee6e17d8f021c1195be4324ebf04@buzz.block.builderlab.xyz> Date: Fri, 14 Aug 2026 13:10:56 -0400 Subject: [PATCH] fix(buzz-agent): reject structurally invalid permission response frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wire::classify normalized malformed JSON-RPC responses into well-formed allow results before the fail-closed broker could see them. A present non-string `method` collapsed to "no method" via `as_str`, and a frame with both `result` and `error` forwarded `result` unconditionally — so a `selected`/`allow_once` payload in either shape was laundered into an approval upstream of every authorization check. classify now forwards `result` only for a structurally valid response: no `method` member and exactly one of `result`/`error`. Any other shape normalizes to Null, which the broker denies. The adversarial-response tests attacked the result payload; these attack the frame structure. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-agent/src/permission.rs | 68 ++++++++++++++++++++++ crates/buzz-agent/src/wire.rs | 87 ++++++++++++++++++++++++++--- 2 files changed, 146 insertions(+), 9 deletions(-) diff --git a/crates/buzz-agent/src/permission.rs b/crates/buzz-agent/src/permission.rs index 1dde42a27..01dea078c 100644 --- a/crates/buzz-agent/src/permission.rs +++ b/crates/buzz-agent/src/permission.rs @@ -519,6 +519,74 @@ mod tests { assert_eq!(broker.available_permits(), 4); } + // ── Malformed response frames deny (Carl's review) ──────────────────────── + + /// Route Carl's frame through the real `classify` → `deliver` path against a + /// live waiter and assert the tool is denied. Delivers on the exact id the + /// broker minted, so the only reason the waiter denies is that `classify` + /// refused to forward the ambiguous/malformed `result`. `provider_id` + /// carries which shape is under test so a failure names the mutant. + async fn assert_malformed_frame_denies(provider_id: &str, frame: Value) { + let broker = Arc::new(PermissionBroker::new(4, LONG)); + let (tx, mut rx) = mpsc::channel(8); + let (_cancel_tx, mut cancel_rx) = watch::channel(false); + + let b = Arc::clone(&broker); + let call = tool_call(); + let task = tokio::spawn(async move { + b.request_permission(&tx, 2, "ses_a", &call, &mut cancel_rx) + .await + }); + + let id = next_request_id(&mut rx).await; + // Stamp the broker's minted id onto Carl's frame, then classify it + // exactly as the dispatch loop would before handing `result` to deliver. + let mut frame = frame; + frame["id"] = id.clone(); + match crate::wire::classify(&frame) { + crate::wire::Inbound::Response { id, result } => broker.deliver(&id, result), + other => panic!("[{provider_id}] expected Response, got {other:?}"), + } + + assert_eq!( + task.await.unwrap(), + PermissionDecision::Denied(PERMISSION_DENIED_MSG), + "[{provider_id}] malformed frame must deny, not authorize the tool", + ); + assert_eq!(broker.pending_count(), 0); + assert_eq!(broker.available_permits(), 4); + } + + /// Carl frame #1: `result` (well-formed `selected`/`allow_once`) AND `error` + /// both present. The tool must not run — the ambiguous frame denies. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_frame_with_result_and_error_denies_tool() { + assert_malformed_frame_denies( + "result+error", + json!({ + "jsonrpc": "2.0", + "result": { "outcome": { "outcome": "selected", "optionId": ALLOW_OPTION_ID } }, + "error": { "code": -32603, "message": "internal" }, + }), + ) + .await; + } + + /// Carl frame #2: present non-string `method: 7` alongside a well-formed + /// `selected` `result`. It is not a valid response — the tool must not run. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_frame_with_non_string_method_denies_tool() { + assert_malformed_frame_denies( + "non-string-method", + json!({ + "jsonrpc": "2.0", + "method": 7, + "result": { "outcome": { "outcome": "selected", "optionId": ALLOW_OPTION_ID } }, + }), + ) + .await; + } + // ── Stale / unknown id ignored ──────────────────────────────────────────── #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/crates/buzz-agent/src/wire.rs b/crates/buzz-agent/src/wire.rs index cb959ea56..e6fe89e07 100644 --- a/crates/buzz-agent/src/wire.rs +++ b/crates/buzz-agent/src/wire.rs @@ -30,8 +30,12 @@ pub enum Inbound { /// A bare JSON-RPC response (id present, no method) — the client's answer /// to a request buzz-agent issued. Today the only such request is /// `session/request_permission`. `result` carries the JSON-RPC `result` - /// field, or `Null` for an `error`/malformed response; every non-`selected` - /// shape fails the broker's authorization predicate and denies. + /// field ONLY when the frame is a structurally valid response — no `method` + /// member and exactly one of `result`/`error`. Any malformed shape (present + /// non-string `method`, both `result` and `error`, or neither) is normalized + /// to `Null` so a possibly-`selected` payload is never laundered into an + /// approval; every non-`selected` shape fails the broker's authorization + /// predicate and denies. Response { id: Value, result: Value, @@ -118,13 +122,26 @@ pub fn classify(msg: &Value) -> Inbound { }, (Some(m), None) => Inbound::Notification { method: m, params }, // Bare responses (id present, no method) answer a request buzz-agent - // issued — today only `session/request_permission`. Route the `result` - // (or `Null` on an `error`/absent result) to the permission broker, - // which matches it to a live correlation id or ignores it if unknown. - (None, Some(id)) => Inbound::Response { - id, - result: msg.get("result").cloned().unwrap_or(Value::Null), - }, + // issued — today only `session/request_permission`. Route to the + // permission broker, which matches a live correlation id or ignores an + // unknown one. Forward the `result` ONLY when the frame is a + // structurally valid response — the exactly-one-of invariant: no + // `method` member at all, and `result` present with `error` absent. A + // present non-string `method` (which `as_str` above collapsed to + // `None`), both `result` and `error`, or neither is malformed; forward + // `Null` so the broker fails closed (deny) rather than laundering a + // possibly-`selected` payload into an approval. + (None, Some(id)) => { + let well_formed = msg.get("method").is_none() + && msg.get("result").is_some() + && msg.get("error").is_none(); + let result = if well_formed { + msg.get("result").cloned().unwrap_or(Value::Null) + } else { + Value::Null + }; + Inbound::Response { id, result } + } (None, None) => Inbound::Invalid { id: Value::Null, code: INVALID_REQUEST, @@ -752,6 +769,58 @@ mod tests { } } + /// Carl's frame #1: a response carrying BOTH `result` and `error` is + /// structurally ambiguous and must NOT deliver the `result`, even when that + /// `result` is a well-formed `selected`/`allow_once` payload. The wire layer + /// normalizes it to `Null` so the broker denies instead of the frame + /// laundering an approval upstream of every fail-closed check. + #[test] + fn classify_response_with_both_result_and_error_denies() { + let msg = json!({ + "jsonrpc": "2.0", + "id": "perm-3", + "result": { "outcome": { "outcome": "selected", "optionId": ALLOW_OPTION_ID } }, + "error": { "code": -32603, "message": "internal" }, + }); + match classify(&msg) { + Inbound::Response { id, result } => { + assert_eq!(id, json!("perm-3")); + assert_eq!( + result, + Value::Null, + "result+error is malformed → Null → deny, never forward the allow payload" + ); + } + other => panic!("expected Response, got {other:?}"), + } + } + + /// Carl's frame #2: a present but non-string `method` is NOT "method + /// absent". `as_str` collapses `method: 7` to `None`, which lands the frame + /// in the response arm, but it is not a valid response and must not forward + /// its `result` (a well-formed `selected` payload here). The structural + /// check sees the present `method` member and normalizes to `Null` → deny. + #[test] + fn classify_response_with_non_string_method_denies() { + let msg = json!({ + "jsonrpc": "2.0", + "id": "perm-3", + "method": 7, + "result": { "outcome": { "outcome": "selected", "optionId": ALLOW_OPTION_ID } }, + }); + match classify(&msg) { + Inbound::Response { id, result } => { + assert_eq!(id, json!("perm-3")); + assert_eq!( + result, + Value::Null, + "present non-string method → not a valid response → Null → deny" + ); + } + other => panic!("expected Response, got {other:?}"), + } + } + // ── send_checked: observable wire closure ──────────────────────────────── /// `send_checked` reports `Ok` while the writer's receiver is alive and