diff --git a/docs/mcp.md b/docs/mcp.md index 5642c63..01a4581 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -21,7 +21,11 @@ sudo-proxy as tools over stdio JSON-RPC. Any MCP-capable AI client **`execute`** — run a command through sudo-proxy with human approval. - `argv` (required): command as an argument array. - `host`: target host (omit for localhost; must match a prior `start_server`). -- `timeout`: timeout in ms (default 120 000, max 600 000). +- `timeout`: timeout in ms (max 600 000). Defaults to 120 000 for privileged + commands; **opt-in for unprivileged** ones — if omitted, the call waits as long + as the daemon needs (the 60 s approval prompt and the executor's own bound + still apply), so an approved-but-slow command isn't falsely reported as timed + out (issue #54). - `description`: what this command does (shown in the TUI approval prompt). - `privileged`: whether to escalate privileges (default `true`). - `env`: environment variables to pass. diff --git a/src/mcp.rs b/src/mcp.rs index 7764230..fa7f21e 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -25,6 +25,27 @@ use crate::server::default_socket_path; const DEFAULT_TIMEOUT_MS: u64 = 120_000; const MAX_TIMEOUT_MS: u64 = 600_000; + +/// Choose the client-side read deadline for an `execute` request (issue #54). +/// +/// An unprivileged command runs as the invoking user with no escalation, so the +/// wall-clock `timeout` adds little safety but can falsely fail a command that +/// was approved and ran — the deadline also counts human-approval latency, and a +/// caller that retries a "timed-out" side-effecting command then double-executes. +/// So the timeout is **opt-in** for unprivileged requests: honored when the +/// caller sets it, otherwise `None` (the read waits as long as the daemon needs). +/// The daemon still bounds the command — the approval prompt is 60 s +/// default-deny and the executor has its own exec timeout — and a dead tunnel is +/// still caught by the connect/write phases and by EOF, so an unbounded read +/// cannot hang on a daemon that has died. Privileged requests keep the default +/// bound. An explicit `timeout` is always honored (and clamped to the max). +fn read_timeout_for(privileged: bool, timeout_ms: Option) -> Option { + match timeout_ms { + Some(ms) => Some(Duration::from_millis(ms.min(MAX_TIMEOUT_MS))), + None if privileged => Some(Duration::from_millis(DEFAULT_TIMEOUT_MS)), + None => None, + } +} /// Total timeout for control requests (stop/ping). They never wait on a /// human, so anything beyond connect + one round-trip means trouble. const CONTROL_TIMEOUT: Duration = Duration::from_secs(10); @@ -69,7 +90,11 @@ pub struct ExecuteParams { #[serde(default)] pub host: Option, - /// Timeout in milliseconds (default: 120000, max: 600000) + /// Timeout in milliseconds, max 600000. For privileged commands the default + /// is 120000. For unprivileged commands the timeout is opt-in: if omitted, + /// the call waits as long as the daemon needs (the 60s approval prompt and + /// the executor's own bound still apply), so an approved-but-slow command is + /// not falsely reported as timed out. Set it explicitly to bound either. #[serde(default)] pub timeout: Option, @@ -157,7 +182,7 @@ impl McpProxy { } #[tool( - description = "Execute a command (or multi-stage pipeline) on a sudo-proxy host after a human approves it at that host's terminal. Provide `argv` for a single command or `pipeline` for piped stages (e.g. [[\"ls\", \"/tmp\"], [\"wc\", \"-l\"]]); `host` targets a remote daemon started via start_server (omit for localhost); `timeout` is in milliseconds (default 120000, clamped to 600000). Blocks until the human answers, then returns the final stage's stdout plus per-stage stderr and exit codes. Errors: 'Request denied by user.' if the human declines, a timeout error if unanswered within 60s, and 'sudo-proxy is not running' if the daemon is down — call start_server first." + description = "Execute a command (or multi-stage pipeline) on a sudo-proxy host after a human approves it at that host's terminal. Provide `argv` for a single command or `pipeline` for piped stages (e.g. [[\"ls\", \"/tmp\"], [\"wc\", \"-l\"]]); `host` targets a remote daemon started via start_server (omit for localhost). Unprivileged commands on the local machine are refused — use your Bash tool, which applies the same controls. `timeout` is in milliseconds (max 600000); it defaults to 120000 for privileged commands but is opt-in for unprivileged ones (omitted = wait as long as the daemon needs, so an approved-but-slow command is not falsely timed out). Blocks until the human answers, then returns the final stage's stdout plus per-stage stderr and exit codes. Errors: 'Request denied by user.' if the human declines, a timeout error if unanswered within 60s, and 'sudo-proxy is not running' if the daemon is down — call start_server first." )] async fn execute( &self, @@ -187,7 +212,11 @@ impl McpProxy { } let socket_path = socket_for_host(target.as_deref()); - let timeout_ms = params.timeout.unwrap_or(DEFAULT_TIMEOUT_MS).min(MAX_TIMEOUT_MS); + // Issue #54: the wall-clock `timeout` is opt-in for unprivileged + // commands (see `read_timeout_for`). Computed here so the decision is + // visible next to routing; `params.privileged` is Copy so it stays + // readable after the request is built below. + let read_timeout = read_timeout_for(params.privileged, params.timeout); if !socket_path.exists() { return Ok(error_result(format!( @@ -225,8 +254,7 @@ impl McpProxy { params.forward_agent, ); - let total_timeout = Duration::from_millis(timeout_ms); - let result = send_request(&socket_path, &req, total_timeout).await; + let result = send_request(&socket_path, &req, read_timeout).await; match result { Ok(resp) => { @@ -319,7 +347,7 @@ impl McpProxy { "sudo-proxy-mcp".to_string(), Action::Stop, ); - match send_request(&socket_path, &req, CONTROL_TIMEOUT).await { + match send_request(&socket_path, &req, Some(CONTROL_TIMEOUT)).await { Ok(resp) if resp.status == Status::Ok => { // Wait for the daemon (and, remotely, the ssh tunnel) to go // away so a follow-up start_server doesn't race the old @@ -434,7 +462,7 @@ async fn probe_one(host: Option<&str>) -> String { "sudo-proxy-mcp".to_string(), Action::Ping, ); - match send_request(&sock, &req, CONTROL_TIMEOUT).await { + match send_request(&sock, &req, Some(CONTROL_TIMEOUT)).await { Ok(resp) if resp.status == Status::Ok => { touch_host(name, &resp.version); let ver = if resp.version.is_empty() { "unknown" } else { &resp.version }; @@ -796,7 +824,7 @@ const PHASE_TIMEOUT: Duration = Duration::from_secs(5); async fn send_request( socket_path: &Path, req: &Request, - total_timeout: Duration, + read_timeout: Option, ) -> Result { // Bounded retry on transient EOF — the SSH `-L` forward accepts a // local connect before the remote daemon has bound the remote @@ -810,7 +838,7 @@ async fn send_request( let mut eof_retries = 0u32; loop { - match send_request_once(socket_path, req, total_timeout).await { + match send_request_once(socket_path, req, read_timeout).await { Ok(resp) => { if eof_retries > 0 { mcp_trace!( @@ -849,9 +877,12 @@ enum SendError { async fn send_request_once( socket_path: &Path, req: &Request, - total_timeout: Duration, + read_timeout: Option, ) -> Result { - let deadline = tokio::time::Instant::now() + total_timeout; + // `None` means "wait as long as the daemon needs" (issue #54, unprivileged). + // Connect/write below still have their own PHASE_TIMEOUT, so only the read — + // the phase that waits on the human and the command — can be unbounded. + let deadline = read_timeout.map(|t| tokio::time::Instant::now() + t); // Phase 1: Connect (5s — if a Unix socket takes longer, the tunnel is dead) let stream = tokio::time::timeout(PHASE_TIMEOUT, UnixStream::connect(socket_path)) @@ -898,21 +929,34 @@ async fn send_request_once( })?; write_result?; - // Phase 3: Read (remaining time from user-specified timeout) - let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); - let remaining = remaining.max(Duration::from_secs(1)); // at least 1s - + // Phase 3: Read. With a deadline, wait the remaining budget; without one + // (opt-in-timeout unprivileged request), wait indefinitely for the daemon's + // response — the daemon always answers (approve → exec → respond, deny, or + // the 60 s prompt timeout) or drops the connection (surfaced as EOF below). let mut reader = BufReader::new(read); let mut line = String::new(); - tokio::time::timeout(remaining, reader.read_line(&mut line)) - .await - .map_err(|_| { - SendError::Hard(format!( - "server did not respond within {}s — it may be busy with another command or waiting for user approval", - total_timeout.as_secs() - )) - })? - .map_err(|e| SendError::Hard(format!("read: {e}")))?; + match deadline { + Some(dl) => { + let remaining = dl + .saturating_duration_since(tokio::time::Instant::now()) + .max(Duration::from_secs(1)); // at least 1s + tokio::time::timeout(remaining, reader.read_line(&mut line)) + .await + .map_err(|_| { + SendError::Hard(format!( + "server did not respond within {}s — it may be busy with another command or waiting for user approval", + remaining.as_secs() + )) + })? + .map_err(|e| SendError::Hard(format!("read: {e}")))?; + } + None => { + reader + .read_line(&mut line) + .await + .map_err(|e| SendError::Hard(format!("read: {e}")))?; + } + } if line.is_empty() { return Err(SendError::TransientEof); @@ -1099,6 +1143,33 @@ mod tests { /// remote stays `Some`. (The full self/loopback classification, including /// the own-hostname cases, is covered exhaustively by `is_local_host` in /// `server.rs`; here we only pin the None/Some routing decision.) + /// Issue #54: the client-side read timeout is opt-in for unprivileged + /// commands, kept for privileged, and an explicit value is always honored + /// (and clamped). + #[test] + fn read_timeout_is_opt_in_for_unprivileged() { + // Unprivileged, no explicit timeout -> unbounded (daemon-bounded). + assert_eq!(read_timeout_for(false, None), None); + // Privileged, no explicit timeout -> default bound. + assert_eq!( + read_timeout_for(true, None), + Some(Duration::from_millis(DEFAULT_TIMEOUT_MS)) + ); + // An explicit timeout is honored for both, and clamped to the max. + assert_eq!( + read_timeout_for(false, Some(5_000)), + Some(Duration::from_millis(5_000)) + ); + assert_eq!( + read_timeout_for(true, Some(5_000)), + Some(Duration::from_millis(5_000)) + ); + assert_eq!( + read_timeout_for(false, Some(u64::MAX)), + Some(Duration::from_millis(MAX_TIMEOUT_MS)) + ); + } + #[test] fn normalize_host_folds_loopback_to_local() { assert_eq!(normalize_host(None), None);