Skip to content

Commit 02f85a8

Browse files
feat(mcp): transport abstraction, stdio/UDS transports, and OAuth fixes (#721)
* feat(mcp): transport abstraction, stdio/UDS transports, and OAuth fixes Extract McpTransport trait from HTTP-coupled McpClient, enabling pluggable transport backends. Implements stdio and Unix domain socket transports for local MCP server integration, fixes OAuth discovery per RFC 9728, and adds SSRF protection. Transport abstraction (Step 2): - McpTransport trait with send(), shutdown(), supports_http_features() - HttpMcpTransport extracted from McpClient with SSE parsing, session tracking - Shared JSON-RPC framing helpers (write_jsonrpc_line, spawn_jsonrpc_reader) - McpClient refactored to hold Arc<dyn McpTransport> Stdio transport (#652, Step 4): - StdioMcpTransport spawns child process, communicates via stdin/stdout - McpProcessManager for lifecycle management with exponential backoff restart - Background stderr drain task for debug logging Unix domain socket transport (#134, Step 5): - UnixMcpTransport connects to existing Unix sockets - Reuses shared JSON-RPC framing from transport.rs HTML error body sanitization (#263, Step 1): - sanitize_error_body() detects HTML, strips control chars, truncates to 500 Custom headers (#639, Step 3): - headers field on McpServerConfig, merged into every HTTP request - --header CLI arg for `mcp add` Config and CLI updates (Step 6): - McpTransportConfig tagged enum (Http/Stdio/Unix) with serde support - EffectiveTransport for zero-copy config dispatch - CLI: --transport, --command, --arg, --env, --socket flags for `mcp add` - `mcp list` shows transport type OAuth fixes (#299, Step 8): - Multi-strategy discovery (401-based, RFC 9728, direct) - RFC 8707 resource parameter in auth and refresh flows - SSRF protection with IPv4-mapped IPv6 bypass detection - Well-known URI construction per RFC 8414 Closes #652, #134, #639, #263, #299 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): address audit findings from crate review - Fix SSRF bypass: make validate_url_safe async with DNS resolution to block hostnames that resolve to private/link-local IPs - Fix UTF-8 truncation: use char-based truncation in sanitize_error_body to avoid panicking on multi-byte characters - Fix SSE parser: process only complete lines to handle chunks split across boundaries, add 10MB buffer size limit - Add debug_assert for transport type mismatch in new_with_config - Propagate custom headers in new_with_transport constructor - Deduplicate effective_transport() calls in CLI list command - Gate test-only accessors with #[cfg(test)] to eliminate dead_code warnings - Document JSON-RPC notification id:0 limitation in protocol.rs - Document total backoff wait time (31s) in process.rs - Add regression test for multi-byte UTF-8 truncation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): address PR review findings from Copilot, Gemini, and zmanian Moderate/High fixes: - Plumb custom headers through new_authenticated constructor - Restrict HTTP to localhost only in validate_url_safe (prevent plaintext credential leaks over non-localhost HTTP) - Add mcp_process_manager.shutdown_all() to app shutdown path to prevent orphaning stdio child processes - Validate discovered authorization_url before opening browser (prevent malicious MCP server redirecting to phishing page) Medium fixes: - Upgrade debug_assert to assert in new_with_config (fires in release) - Remove pending map entry on Ok(Err(_)) in stdio/unix send() to avoid stale entries and unnecessary 30s waits - Shut down old transport in try_restart() before spawning replacement - Redact env var values in mcp list --verbose (may contain secrets) - Drain pending requests on shutdown to wake waiters immediately - Add IPv6 link-local, site-local, unique-local, and documentation ranges to is_dangerous_ip SSRF protection Low fixes: - Truncate logged JSON parse error lines to 200 chars (prevent sensitive data in logs) - Remove misleading shutdown comment in unix_transport - Use tempfile::tempdir() instead of hardcoded /tmp/ path in test - Adopt main's improved sanitize_error_body (HTML tag stripping, 200-char truncation with char_indices) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): gate unix_transport with #[cfg(unix)] for Windows compat - Add #[cfg(unix)] to unix_transport module declaration - Add #[cfg(unix)]/#[cfg(not(unix))] branches in app.rs for Unix socket MCP server setup - Remove unused sanitize_error_body import in client.rs tests [skip-regression-check] --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9401ab0 commit 02f85a8

15 files changed

Lines changed: 2934 additions & 471 deletions

File tree

src/app.rs

Lines changed: 161 additions & 75 deletions
Large diffs are not rendered by default.

src/cli/mcp.rs

Lines changed: 238 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,11 @@
22
//!
33
//! Commands for adding, removing, authenticating, and testing MCP servers.
44
5+
use std::collections::HashMap;
56
use std::io::Write;
67
use std::sync::Arc;
78

8-
use clap::Subcommand;
9+
use clap::{Args, Subcommand};
910

1011
use crate::config::Config;
1112
use crate::db::Database;
@@ -15,39 +16,67 @@ use crate::secrets::{SecretsCrypto, SecretsStore};
1516
use crate::tools::mcp::{
1617
McpClient, McpServerConfig, McpSessionManager, OAuthConfig,
1718
auth::{authorize_mcp_server, is_authenticated},
18-
config::{self, McpServersFile},
19+
config::{self, EffectiveTransport, McpServersFile},
1920
};
2021

21-
#[derive(Subcommand, Debug, Clone)]
22-
pub enum McpCommand {
23-
/// Add an MCP server
24-
Add {
25-
/// Server name (e.g., "notion", "github")
26-
name: String,
22+
/// Arguments for the `mcp add` subcommand.
23+
#[derive(Args, Debug, Clone)]
24+
pub struct McpAddArgs {
25+
/// Server name (e.g., "notion", "github")
26+
pub name: String,
2727

28-
/// Server URL (e.g., "https://mcp.notion.com")
29-
url: String,
28+
/// Server URL (e.g., "https://mcp.notion.com") -- required for http transport
29+
pub url: Option<String>,
3030

31-
/// OAuth client ID (if authentication is required)
32-
#[arg(long)]
33-
client_id: Option<String>,
31+
/// Transport type: http (default), stdio, unix
32+
#[arg(long, default_value = "http")]
33+
pub transport: String,
3434

35-
/// OAuth authorization URL (optional, can be discovered)
36-
#[arg(long)]
37-
auth_url: Option<String>,
35+
/// Command to run (stdio transport)
36+
#[arg(long)]
37+
pub command: Option<String>,
3838

39-
/// OAuth token URL (optional, can be discovered)
40-
#[arg(long)]
41-
token_url: Option<String>,
39+
/// Command arguments (stdio transport, can be repeated)
40+
#[arg(long = "arg", num_args = 1..)]
41+
pub cmd_args: Vec<String>,
4242

43-
/// Scopes to request (comma-separated)
44-
#[arg(long)]
45-
scopes: Option<String>,
43+
/// Environment variables (stdio transport, KEY=VALUE format, can be repeated)
44+
#[arg(long = "env", value_parser = parse_env_var)]
45+
pub env: Vec<(String, String)>,
4646

47-
/// Server description
48-
#[arg(long)]
49-
description: Option<String>,
50-
},
47+
/// Unix socket path (unix transport)
48+
#[arg(long)]
49+
pub socket: Option<String>,
50+
51+
/// Custom HTTP headers (KEY:VALUE format, can be repeated)
52+
#[arg(long = "header", value_parser = parse_header)]
53+
pub headers: Vec<(String, String)>,
54+
55+
/// OAuth client ID (if authentication is required)
56+
#[arg(long)]
57+
pub client_id: Option<String>,
58+
59+
/// OAuth authorization URL (optional, can be discovered)
60+
#[arg(long)]
61+
pub auth_url: Option<String>,
62+
63+
/// OAuth token URL (optional, can be discovered)
64+
#[arg(long)]
65+
pub token_url: Option<String>,
66+
67+
/// Scopes to request (comma-separated)
68+
#[arg(long)]
69+
pub scopes: Option<String>,
70+
71+
/// Server description
72+
#[arg(long)]
73+
pub description: Option<String>,
74+
}
75+
76+
#[derive(Subcommand, Debug, Clone)]
77+
pub enum McpCommand {
78+
/// Add an MCP server
79+
Add(Box<McpAddArgs>),
5180

5281
/// Remove an MCP server
5382
Remove {
@@ -97,29 +126,24 @@ pub enum McpCommand {
97126
},
98127
}
99128

129+
fn parse_header(s: &str) -> Result<(String, String), String> {
130+
let pos = s
131+
.find(':')
132+
.ok_or_else(|| format!("invalid header format '{}', expected KEY:VALUE", s))?;
133+
Ok((s[..pos].trim().to_string(), s[pos + 1..].trim().to_string()))
134+
}
135+
136+
fn parse_env_var(s: &str) -> Result<(String, String), String> {
137+
let pos = s
138+
.find('=')
139+
.ok_or_else(|| format!("invalid env var format '{}', expected KEY=VALUE", s))?;
140+
Ok((s[..pos].to_string(), s[pos + 1..].to_string()))
141+
}
142+
100143
/// Run an MCP command.
101144
pub async fn run_mcp_command(cmd: McpCommand) -> anyhow::Result<()> {
102145
match cmd {
103-
McpCommand::Add {
104-
name,
105-
url,
106-
client_id,
107-
auth_url,
108-
token_url,
109-
scopes,
110-
description,
111-
} => {
112-
add_server(
113-
name,
114-
url,
115-
client_id,
116-
auth_url,
117-
token_url,
118-
scopes,
119-
description,
120-
)
121-
.await
122-
}
146+
McpCommand::Add(args) => add_server(*args).await,
123147
McpCommand::Remove { name } => remove_server(name).await,
124148
McpCommand::List { verbose } => list_servers(verbose).await,
125149
McpCommand::Auth { name, user } => auth_server(name, user).await,
@@ -133,16 +157,58 @@ pub async fn run_mcp_command(cmd: McpCommand) -> anyhow::Result<()> {
133157
}
134158

135159
/// Add a new MCP server.
136-
async fn add_server(
137-
name: String,
138-
url: String,
139-
client_id: Option<String>,
140-
auth_url: Option<String>,
141-
token_url: Option<String>,
142-
scopes: Option<String>,
143-
description: Option<String>,
144-
) -> anyhow::Result<()> {
145-
let mut config = McpServerConfig::new(&name, &url);
160+
async fn add_server(args: McpAddArgs) -> anyhow::Result<()> {
161+
let McpAddArgs {
162+
name,
163+
url,
164+
transport,
165+
command,
166+
cmd_args,
167+
env,
168+
socket,
169+
headers,
170+
client_id,
171+
auth_url,
172+
token_url,
173+
scopes,
174+
description,
175+
} = args;
176+
177+
let transport_lower = transport.to_lowercase();
178+
179+
let mut config = match transport_lower.as_str() {
180+
"stdio" => {
181+
let cmd = command
182+
.clone()
183+
.ok_or_else(|| anyhow::anyhow!("--command is required for stdio transport"))?;
184+
let env_map: HashMap<String, String> = env.into_iter().collect();
185+
McpServerConfig::new_stdio(&name, &cmd, cmd_args.clone(), env_map)
186+
}
187+
"unix" => {
188+
let socket_path = socket
189+
.clone()
190+
.ok_or_else(|| anyhow::anyhow!("--socket is required for unix transport"))?;
191+
McpServerConfig::new_unix(&name, &socket_path)
192+
}
193+
"http" => {
194+
let url_val = url
195+
.as_deref()
196+
.ok_or_else(|| anyhow::anyhow!("URL is required for http transport"))?;
197+
McpServerConfig::new(&name, url_val)
198+
}
199+
other => {
200+
anyhow::bail!(
201+
"Unknown transport type '{}'. Supported: http, stdio, unix",
202+
other
203+
);
204+
}
205+
};
206+
207+
// Apply headers if any
208+
if !headers.is_empty() {
209+
let headers_map: HashMap<String, String> = headers.into_iter().collect();
210+
config = config.with_headers(headers_map);
211+
}
146212

147213
if let Some(desc) = description {
148214
config = config.with_description(desc);
@@ -151,8 +217,12 @@ async fn add_server(
151217
// Track if auth is required
152218
let requires_auth = client_id.is_some();
153219

154-
// Set up OAuth if client_id is provided
220+
// Set up OAuth if client_id is provided (HTTP transport only)
155221
if let Some(client_id) = client_id {
222+
if transport_lower != "http" {
223+
anyhow::bail!("OAuth authentication is only supported with http transport");
224+
}
225+
156226
let mut oauth = OAuthConfig::new(client_id);
157227

158228
if let (Some(auth), Some(token)) = (auth_url, token_url) {
@@ -181,7 +251,24 @@ async fn add_server(
181251

182252
println!();
183253
println!(" ✓ Added MCP server '{}'", name);
184-
println!(" URL: {}", url);
254+
255+
match transport_lower.as_str() {
256+
"stdio" => {
257+
println!(
258+
" Transport: stdio (command: {})",
259+
command.as_deref().unwrap_or("")
260+
);
261+
}
262+
"unix" => {
263+
println!(
264+
" Transport: unix (socket: {})",
265+
socket.as_deref().unwrap_or("")
266+
);
267+
}
268+
_ => {
269+
println!(" URL: {}", url.as_deref().unwrap_or(""));
270+
}
271+
}
185272

186273
if requires_auth {
187274
println!();
@@ -236,9 +323,40 @@ async fn list_servers(verbose: bool) -> anyhow::Result<()> {
236323
""
237324
};
238325

326+
let effective = server.effective_transport();
327+
328+
let transport_label = match &effective {
329+
EffectiveTransport::Http => "http".to_string(),
330+
EffectiveTransport::Stdio { command, .. } => {
331+
format!("stdio ({})", command)
332+
}
333+
EffectiveTransport::Unix { socket_path } => {
334+
format!("unix ({})", socket_path)
335+
}
336+
};
337+
239338
if verbose {
240339
println!(" {} {}{}", status, server.name, auth_status);
241-
println!(" URL: {}", server.url);
340+
println!(" Transport: {}", transport_label);
341+
match &effective {
342+
EffectiveTransport::Http => {
343+
println!(" URL: {}", server.url);
344+
}
345+
EffectiveTransport::Stdio { command, args, env } => {
346+
println!(" Command: {}", command);
347+
if !args.is_empty() {
348+
println!(" Args: {}", args.join(", "));
349+
}
350+
if !env.is_empty() {
351+
// Only print env var names, not values (may contain secrets).
352+
let env_keys: Vec<&str> = env.keys().map(|k| k.as_str()).collect();
353+
println!(" Env: {}", env_keys.join(", "));
354+
}
355+
}
356+
EffectiveTransport::Unix { socket_path } => {
357+
println!(" Socket: {}", socket_path);
358+
}
359+
}
242360
if let Some(ref desc) = server.description {
243361
println!(" Description: {}", desc);
244362
}
@@ -248,11 +366,27 @@ async fn list_servers(verbose: bool) -> anyhow::Result<()> {
248366
println!(" Scopes: {}", oauth.scopes.join(", "));
249367
}
250368
}
369+
if !server.headers.is_empty() {
370+
let header_keys: Vec<&String> = server.headers.keys().collect();
371+
println!(
372+
" Headers: {}",
373+
header_keys
374+
.iter()
375+
.map(|k| k.as_str())
376+
.collect::<Vec<_>>()
377+
.join(", ")
378+
);
379+
}
251380
println!();
252381
} else {
382+
let display = match &effective {
383+
EffectiveTransport::Http => server.url.clone(),
384+
EffectiveTransport::Stdio { command, .. } => command.to_string(),
385+
EffectiveTransport::Unix { socket_path } => socket_path.to_string(),
386+
};
253387
println!(
254-
" {} {} - {}{}",
255-
status, server.name, server.url, auth_status
388+
" {} {} - {} [{}]{}",
389+
status, server.name, display, transport_label, auth_status
256390
);
257391
}
258392
}
@@ -374,7 +508,7 @@ async fn test_server(name: String, user_id: String) -> anyhow::Result<()> {
374508
return Ok(());
375509
} else {
376510
// No OAuth and no tokens - try unauthenticated
377-
McpClient::new_with_name(&server.name, &server.url)
511+
McpClient::new_with_config(server.clone())
378512
};
379513

380514
// Test connection
@@ -579,4 +713,46 @@ mod tests {
579713

580714
TestCli::command().debug_assert();
581715
}
716+
717+
#[test]
718+
fn test_parse_header_valid() {
719+
let result = parse_header("Authorization: Bearer token123").unwrap();
720+
assert_eq!(result.0, "Authorization");
721+
assert_eq!(result.1, "Bearer token123");
722+
}
723+
724+
#[test]
725+
fn test_parse_header_no_spaces() {
726+
let result = parse_header("X-Api-Key:abc123").unwrap();
727+
assert_eq!(result.0, "X-Api-Key");
728+
assert_eq!(result.1, "abc123");
729+
}
730+
731+
#[test]
732+
fn test_parse_header_invalid() {
733+
let result = parse_header("no-colon-here");
734+
assert!(result.is_err());
735+
assert!(result.unwrap_err().contains("invalid header format"));
736+
}
737+
738+
#[test]
739+
fn test_parse_env_var_valid() {
740+
let result = parse_env_var("NODE_ENV=production").unwrap();
741+
assert_eq!(result.0, "NODE_ENV");
742+
assert_eq!(result.1, "production");
743+
}
744+
745+
#[test]
746+
fn test_parse_env_var_with_equals_in_value() {
747+
let result = parse_env_var("KEY=value=with=equals").unwrap();
748+
assert_eq!(result.0, "KEY");
749+
assert_eq!(result.1, "value=with=equals");
750+
}
751+
752+
#[test]
753+
fn test_parse_env_var_invalid() {
754+
let result = parse_env_var("no-equals-here");
755+
assert!(result.is_err());
756+
assert!(result.unwrap_err().contains("invalid env var format"));
757+
}
582758
}

src/cli/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ pub enum Command {
132132
about = "Manage MCP servers",
133133
long_about = "Add, auth, list, or test MCP servers.\nExample: ironclaw mcp add notion https://mcp.notion.com"
134134
)]
135-
Mcp(McpCommand),
135+
Mcp(Box<McpCommand>),
136136

137137
/// Query and manage workspace memory
138138
#[command(

0 commit comments

Comments
 (0)