From 17a870b4d177b64a53dd97a025fce0a439aafa04 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 11 May 2026 10:56:56 -0400 Subject: [PATCH] fix(mcp): harden events and proxy handling --- lib/crates/fabro-mcp-server/src/run_tools.rs | 48 +++++++++++++++- lib/crates/fabro-mcp-server/src/server.rs | 58 +++++++++++++++++++- 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/lib/crates/fabro-mcp-server/src/run_tools.rs b/lib/crates/fabro-mcp-server/src/run_tools.rs index 88f03e820..681f83c93 100644 --- a/lib/crates/fabro-mcp-server/src/run_tools.rs +++ b/lib/crates/fabro-mcp-server/src/run_tools.rs @@ -830,7 +830,7 @@ fn run_event_result( .map_err(|err| ToolError::message(format!("failed to serialize event: {err}")))?; let truncated = serialized.len() > max_content_length; let event_value = if truncated { - serialized.truncate(max_content_length); + serialized.truncate(floor_char_boundary(&serialized, max_content_length)); Value::String(serialized) } else { serde_json::to_value(event) @@ -844,6 +844,14 @@ fn run_event_result( }) } +fn floor_char_boundary(value: &str, max_len: usize) -> usize { + let mut boundary = max_len.min(value.len()); + while !value.is_char_boundary(boundary) { + boundary -= 1; + } + boundary +} + fn build_mcp_run_manifest( spec: &CreateRunSpec, cwd: &Path, @@ -1028,6 +1036,7 @@ fn run_status_kind(status: RunStatus) -> &'static str { #[cfg(test)] mod tests { + use fabro_types::{EventBody, RunEvent, fixtures}; use serde_json::json; use super::*; @@ -1140,4 +1149,41 @@ mod tests { assert_eq!(args.input, vec![r"count=3", r#"decision="approve""#]); } + + #[test] + fn run_event_result_truncates_at_utf8_boundary() { + let event = EventEnvelope { + seq: 1, + event: RunEvent { + id: "evt_utf8".to_string(), + ts: Utc::now(), + run_id: fixtures::RUN_1, + node_id: None, + node_label: None, + stage_id: None, + parallel_group_id: None, + parallel_branch_id: None, + session_id: None, + parent_session_id: None, + tool_call_id: None, + actor: None, + body: EventBody::Unknown { + name: "test.utf8".to_string(), + properties: json!({ "message": "éééé" }), + }, + }, + }; + let serialized = serde_json::to_string(&event).unwrap(); + let first_multibyte = serialized + .find('é') + .expect("serialized event should contain é"); + + let result = run_event_result(&event, first_multibyte + 1).unwrap(); + + assert!(result.truncated); + let Value::String(event_json) = result.event else { + panic!("truncated events should return string payloads"); + }; + assert!(event_json.is_char_boundary(event_json.len())); + } } diff --git a/lib/crates/fabro-mcp-server/src/server.rs b/lib/crates/fabro-mcp-server/src/server.rs index aa968274a..7869de858 100644 --- a/lib/crates/fabro-mcp-server/src/server.rs +++ b/lib/crates/fabro-mcp-server/src/server.rs @@ -1,3 +1,4 @@ +use std::net::IpAddr; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Duration; @@ -11,6 +12,7 @@ use fabro_config::bind::Bind; use fabro_config::daemon::ServerDaemon; use fabro_config::{RuntimeDirectory, Storage}; use fabro_util::dev_token; +use fabro_util::version::FABRO_VERSION; use rmcp::handler::server::router::tool::ToolRouter; use rmcp::handler::server::wrapper::Parameters; use rmcp::model::{CallToolResult, ServerCapabilities, ServerInfo}; @@ -345,7 +347,10 @@ fn connect_target_transport( bearer_token: Option<&str>, ) -> Result<(fabro_http::HttpClient, String)> { if let Some(api_url) = target.as_http_url() { - let mut builder = fabro_http::HttpClientBuilder::new().no_proxy(); + let mut builder = cli_compatible_http_client_builder(); + if should_bypass_proxy_for_http_target(api_url) { + builder = builder.no_proxy(); + } if let Some(token) = bearer_token { builder = apply_bearer_token_auth(builder, token)?; } @@ -366,6 +371,29 @@ fn connect_target_transport( Ok((builder.build()?, "http://fabro".to_string())) } +fn cli_compatible_http_client_builder() -> fabro_http::HttpClientBuilder { + fabro_http::HttpClientBuilder::new().user_agent(format!("fabro-cli/{FABRO_VERSION}")) +} + +#[expect( + clippy::disallowed_types, + reason = "Proxy bypass classification parses a configured raw API target and does not log credential-bearing URLs." +)] +fn should_bypass_proxy_for_http_target(api_url: &str) -> bool { + let Ok(url) = fabro_http::Url::parse(api_url) else { + return false; + }; + let Some(host) = url.host_str() else { + return false; + }; + if host.eq_ignore_ascii_case("localhost") { + return true; + } + host.trim_matches(['[', ']']) + .parse::() + .is_ok_and(|ip| ip.is_loopback()) +} + async fn connect_bind_http_client( bind: &Bind, bearer_token: Option<&str>, @@ -400,3 +428,31 @@ async fn connect_bind_http_client( } Err(last_error.unwrap_or_else(|| anyhow!("Fabro server did not become ready in time"))) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn explicit_http_proxy_bypass_matches_cli_for_local_targets() { + assert!(should_bypass_proxy_for_http_target( + "http://localhost:3000/api/v1" + )); + assert!(should_bypass_proxy_for_http_target( + "http://127.0.0.1:3000/api/v1" + )); + assert!(should_bypass_proxy_for_http_target( + "http://[::1]:3000/api/v1" + )); + } + + #[test] + fn explicit_http_proxy_bypass_matches_cli_for_remote_targets() { + assert!(!should_bypass_proxy_for_http_target( + "https://fabro.example.test/api/v1" + )); + assert!(!should_bypass_proxy_for_http_target( + "http://192.0.2.44:3000/api/v1" + )); + } +}