gitops/optimclaw
1
0
Fork 0
mirror of https://github.com/outbackdingo/optimclaw.git synced 2026-08-26 23:50:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat(mcp): transport abstraction, stdio/UDS transports, and OAuth fixes (#721)

Browse Source 
* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
This commit is contained in:
Illia Polosukhin
2026-03-09 02:47:42 +00:00
committed by GitHub
co-authored by Claude Opus 4.6
parent 9401ab0d58
commit 02f85a8ad5
15 changed files with 2937 additions and 474 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/cli/mcp.rs
+241 -65
View File
@@ -2,10 +2,11 @@
//!
//! Commands for adding, removing, authenticating, and testing MCP servers.
use std::collections::HashMap;
use std::io::Write;
use std::sync::Arc;
use clap::Subcommand;
use clap::{Args, Subcommand};
use crate::config::Config;
use crate::db::Database;
@@ -15,39 +16,67 @@ use crate::secrets::{SecretsCrypto, SecretsStore};
use crate::tools::mcp::{
McpClient, McpServerConfig, McpSessionManager, OAuthConfig,
auth::{authorize_mcp_server, is_authenticated},
config::{self, McpServersFile},
config::{self, EffectiveTransport, McpServersFile},
};
/// Arguments for the `mcp add` subcommand.
#[derive(Args, Debug, Clone)]
pub struct McpAddArgs {
/// Server name (e.g., "notion", "github")
pub name: String,
/// Server URL (e.g., "https://mcp.notion.com") -- required for http transport
pub url: Option<String>,
/// Transport type: http (default), stdio, unix
#[arg(long, default_value = "http")]
pub transport: String,
/// Command to run (stdio transport)
#[arg(long)]
pub command: Option<String>,
/// Command arguments (stdio transport, can be repeated)
#[arg(long = "arg", num_args = 1..)]
pub cmd_args: Vec<String>,
/// Environment variables (stdio transport, KEY=VALUE format, can be repeated)
#[arg(long = "env", value_parser = parse_env_var)]
pub env: Vec<(String, String)>,
/// Unix socket path (unix transport)
#[arg(long)]
pub socket: Option<String>,
/// Custom HTTP headers (KEY:VALUE format, can be repeated)
#[arg(long = "header", value_parser = parse_header)]
pub headers: Vec<(String, String)>,
/// OAuth client ID (if authentication is required)
#[arg(long)]
pub client_id: Option<String>,
/// OAuth authorization URL (optional, can be discovered)
#[arg(long)]
pub auth_url: Option<String>,
/// OAuth token URL (optional, can be discovered)
#[arg(long)]
pub token_url: Option<String>,
/// Scopes to request (comma-separated)
#[arg(long)]
pub scopes: Option<String>,
/// Server description
#[arg(long)]
pub description: Option<String>,
}
#[derive(Subcommand, Debug, Clone)]
pub enum McpCommand {
/// Add an MCP server
Add {
/// Server name (e.g., "notion", "github")
name: String,
/// Server URL (e.g., "https://mcp.notion.com")
url: String,
/// OAuth client ID (if authentication is required)
#[arg(long)]
client_id: Option<String>,
/// OAuth authorization URL (optional, can be discovered)
#[arg(long)]
auth_url: Option<String>,
/// OAuth token URL (optional, can be discovered)
#[arg(long)]
token_url: Option<String>,
/// Scopes to request (comma-separated)
#[arg(long)]
scopes: Option<String>,
/// Server description
#[arg(long)]
description: Option<String>,
},
Add(Box<McpAddArgs>),
/// Remove an MCP server
Remove {
@@ -97,29 +126,24 @@ pub enum McpCommand {
},
}
fn parse_header(s: &str) -> Result<(String, String), String> {
let pos = s
.find(':')
.ok_or_else(|| format!("invalid header format '{}', expected KEY:VALUE", s))?;
Ok((s[..pos].trim().to_string(), s[pos + 1..].trim().to_string()))
}
fn parse_env_var(s: &str) -> Result<(String, String), String> {
let pos = s
.find('=')
.ok_or_else(|| format!("invalid env var format '{}', expected KEY=VALUE", s))?;
Ok((s[..pos].to_string(), s[pos + 1..].to_string()))
}
/// Run an MCP command.
pub async fn run_mcp_command(cmd: McpCommand) -> anyhow::Result<()> {
match cmd {
McpCommand::Add {
name,
url,
client_id,
auth_url,
token_url,
scopes,
description,
} => {
add_server(
name,
url,
client_id,
auth_url,
token_url,
scopes,
description,
)
.await
}
McpCommand::Add(args) => add_server(*args).await,
McpCommand::Remove { name } => remove_server(name).await,
McpCommand::List { verbose } => list_servers(verbose).await,
McpCommand::Auth { name, user } => auth_server(name, user).await,
@@ -133,16 +157,58 @@ pub async fn run_mcp_command(cmd: McpCommand) -> anyhow::Result<()> {
}
/// Add a new MCP server.
async fn add_server(
name: String,
url: String,
client_id: Option<String>,
auth_url: Option<String>,
token_url: Option<String>,
scopes: Option<String>,
description: Option<String>,
) -> anyhow::Result<()> {
let mut config = McpServerConfig::new(&name, &url);
async fn add_server(args: McpAddArgs) -> anyhow::Result<()> {
let McpAddArgs {
name,
url,
transport,
command,
cmd_args,
env,
socket,
headers,
client_id,
auth_url,
token_url,
scopes,
description,
} = args;
let transport_lower = transport.to_lowercase();
let mut config = match transport_lower.as_str() {
"stdio" => {
let cmd = command
.clone()
.ok_or_else(|| anyhow::anyhow!("--command is required for stdio transport"))?;
let env_map: HashMap<String, String> = env.into_iter().collect();
McpServerConfig::new_stdio(&name, &cmd, cmd_args.clone(), env_map)
}
"unix" => {
let socket_path = socket
.clone()
.ok_or_else(|| anyhow::anyhow!("--socket is required for unix transport"))?;
McpServerConfig::new_unix(&name, &socket_path)
}
"http" => {
let url_val = url
.as_deref()
.ok_or_else(|| anyhow::anyhow!("URL is required for http transport"))?;
McpServerConfig::new(&name, url_val)
}
other => {
anyhow::bail!(
"Unknown transport type '{}'. Supported: http, stdio, unix",
other
);
}
};
// Apply headers if any
if !headers.is_empty() {
let headers_map: HashMap<String, String> = headers.into_iter().collect();
config = config.with_headers(headers_map);
}
if let Some(desc) = description {
config = config.with_description(desc);
@@ -151,8 +217,12 @@ async fn add_server(
// Track if auth is required
let requires_auth = client_id.is_some();
// Set up OAuth if client_id is provided
// Set up OAuth if client_id is provided (HTTP transport only)
if let Some(client_id) = client_id {
if transport_lower != "http" {
anyhow::bail!("OAuth authentication is only supported with http transport");
}
let mut oauth = OAuthConfig::new(client_id);
if let (Some(auth), Some(token)) = (auth_url, token_url) {
@@ -181,7 +251,24 @@ async fn add_server(
println!();
println!(" ✓ Added MCP server '{}'", name);
println!(" URL: {}", url);
match transport_lower.as_str() {
"stdio" => {
println!(
" Transport: stdio (command: {})",
command.as_deref().unwrap_or("")
);
}
"unix" => {
println!(
" Transport: unix (socket: {})",
socket.as_deref().unwrap_or("")
);
}
_ => {
println!(" URL: {}", url.as_deref().unwrap_or(""));
}
}
if requires_auth {
println!();
@@ -236,9 +323,40 @@ async fn list_servers(verbose: bool) -> anyhow::Result<()> {
""
};
let effective = server.effective_transport();
let transport_label = match &effective {
EffectiveTransport::Http => "http".to_string(),
EffectiveTransport::Stdio { command, .. } => {
format!("stdio ({})", command)
}
EffectiveTransport::Unix { socket_path } => {
format!("unix ({})", socket_path)
}
};
if verbose {
println!(" {} {}{}", status, server.name, auth_status);
println!(" URL: {}", server.url);
println!(" Transport: {}", transport_label);
match &effective {
EffectiveTransport::Http => {
println!(" URL: {}", server.url);
}
EffectiveTransport::Stdio { command, args, env } => {
println!(" Command: {}", command);
if !args.is_empty() {
println!(" Args: {}", args.join(", "));
}
if !env.is_empty() {
// Only print env var names, not values (may contain secrets).
let env_keys: Vec<&str> = env.keys().map(|k| k.as_str()).collect();
println!(" Env: {}", env_keys.join(", "));
}
}
EffectiveTransport::Unix { socket_path } => {
println!(" Socket: {}", socket_path);
}
}
if let Some(ref desc) = server.description {
println!(" Description: {}", desc);
}
@@ -248,11 +366,27 @@ async fn list_servers(verbose: bool) -> anyhow::Result<()> {
println!(" Scopes: {}", oauth.scopes.join(", "));
}
}
if !server.headers.is_empty() {
let header_keys: Vec<&String> = server.headers.keys().collect();
println!(
" Headers: {}",
header_keys
.iter()
.map(|k| k.as_str())
.collect::<Vec<_>>()
.join(", ")
);
}
println!();
} else {
let display = match &effective {
EffectiveTransport::Http => server.url.clone(),
EffectiveTransport::Stdio { command, .. } => command.to_string(),
EffectiveTransport::Unix { socket_path } => socket_path.to_string(),
};
println!(
" {} {} - {}{}",
status, server.name, server.url, auth_status
" {} {} - {} [{}]{}",
status, server.name, display, transport_label, auth_status
);
}
}
@@ -374,7 +508,7 @@ async fn test_server(name: String, user_id: String) -> anyhow::Result<()> {
return Ok(());
} else {
// No OAuth and no tokens - try unauthenticated
McpClient::new_with_name(&server.name, &server.url)
McpClient::new_with_config(server.clone())
};
// Test connection
@@ -579,4 +713,46 @@ mod tests {
TestCli::command().debug_assert();
}
#[test]
fn test_parse_header_valid() {
let result = parse_header("Authorization: Bearer token123").unwrap();
assert_eq!(result.0, "Authorization");
assert_eq!(result.1, "Bearer token123");
}
#[test]
fn test_parse_header_no_spaces() {
let result = parse_header("X-Api-Key:abc123").unwrap();
assert_eq!(result.0, "X-Api-Key");
assert_eq!(result.1, "abc123");
}
#[test]
fn test_parse_header_invalid() {
let result = parse_header("no-colon-here");
assert!(result.is_err());
assert!(result.unwrap_err().contains("invalid header format"));
}
#[test]
fn test_parse_env_var_valid() {
let result = parse_env_var("NODE_ENV=production").unwrap();
assert_eq!(result.0, "NODE_ENV");
assert_eq!(result.1, "production");
}
#[test]
fn test_parse_env_var_with_equals_in_value() {
let result = parse_env_var("KEY=value=with=equals").unwrap();
assert_eq!(result.0, "KEY");
assert_eq!(result.1, "value=with=equals");
}
#[test]
fn test_parse_env_var_invalid() {
let result = parse_env_var("no-equals-here");
assert!(result.is_err());
assert!(result.unwrap_err().contains("invalid env var format"));
}
}
src/cli/mod.rs
+1 -1
View File
@@ -132,7 +132,7 @@ pub enum Command {
about = "Manage MCP servers",
long_about = "Add, auth, list, or test MCP servers.\nExample: ironclaw mcp add notion https://mcp.notion.com"
)]
Mcp(McpCommand),
Mcp(Box<McpCommand>),
/// Query and manage workspace memory
#[command(