fix(mcp): harden events and proxy handling

This commit is contained in:
Bryan Helmkamp 2026-05-11 10:56:56 -04:00
parent feec197dec
commit 17a870b4d1
No known key found for this signature in database
2 changed files with 104 additions and 2 deletions

View file

@ -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()));
}
}

View file

@ -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::<IpAddr>()
.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"
));
}
}