diff --git a/crates/agent-tui-adapter/src/manifest.rs b/crates/agent-tui-adapter/src/manifest.rs index 8fc4c23..0044764 100644 --- a/crates/agent-tui-adapter/src/manifest.rs +++ b/crates/agent-tui-adapter/src/manifest.rs @@ -481,7 +481,7 @@ fn resolve_rows( } RowSpec::Dynamic(RowDynamic::AboveLastNonEmpty) => { let row = last_non_empty_row(snap, rows, cols)?; - (row > 0).then_some((0, row - 1)) + row.checked_sub(1).map(|above| (0, above)) } RowSpec::Dynamic(RowDynamic::Cursor) => { let row = usize::from(snap.grid.cursor.0).min(rows - 1); @@ -489,7 +489,7 @@ fn resolve_rows( } RowSpec::Dynamic(RowDynamic::AboveCursor) => { let row = usize::from(snap.grid.cursor.0).min(rows - 1); - (row > 0).then_some((0, row - 1)) + row.checked_sub(1).map(|above| (0, above)) } } } @@ -793,6 +793,32 @@ mod tests { assert_eq!(resolve_range([-100, 100], 24), (0, 23)); } + #[test] + fn dynamic_above_row_selectors_do_not_underflow_on_first_row() { + let snap = snap("> "); + let rows = usize::from(snap.grid.rows); + let cols = usize::from(snap.grid.cols); + + assert_eq!( + resolve_rows( + &RowSpec::Dynamic(RowDynamic::AboveLastNonEmpty), + &snap, + rows, + cols, + ), + None + ); + assert_eq!( + resolve_rows( + &RowSpec::Dynamic(RowDynamic::AboveCursor), + &snap, + rows, + cols + ), + None + ); + } + fn snap(content: &str) -> EngineSnapshot { let lines: Vec<&str> = content.split('\n').collect(); let cols = u16::try_from( diff --git a/crates/agent-tui-daemon/src/governance.rs b/crates/agent-tui-daemon/src/governance.rs index be6c5fc..68e5336 100644 --- a/crates/agent-tui-daemon/src/governance.rs +++ b/crates/agent-tui-daemon/src/governance.rs @@ -17,6 +17,7 @@ //! OPA-WASM (`agent-tui --policy `) lands in a follow-on cycle. use std::collections::HashSet; +use std::path::{Path, PathBuf}; use std::sync::Arc; use agent_tui_protocol::action::{Action, ActionDetail, ActionKind, CallerInfo, Decision, Verdict}; @@ -53,11 +54,16 @@ impl Evaluator for AllowAllEvaluator { } /// Binary allowlist enforcement on `Spawn`; pass-through for other action -/// kinds. Empty allowlist means "everything allowed" (effective baseline -/// for development); the explicit `*` wildcard means "anything but I want -/// the audit log to know about it". +/// kinds. Entries without path separators are command names and only match +/// bare `argv[0]` values such as `git`; entries with path separators are +/// canonical executable paths and only match path-style invocations such as +/// `/usr/bin/git` or `./tool`. Empty allowlist means "everything allowed" +/// (effective baseline for development); the explicit `*` wildcard means +/// "anything but I want the audit log to know about it". pub struct AllowlistEvaluator { binaries: HashSet, + paths: HashSet, + unresolved_paths: HashSet, wildcard: bool, } @@ -67,12 +73,29 @@ impl AllowlistEvaluator { #[must_use] pub fn new(binaries: Vec) -> Self { let wildcard = binaries.iter().any(|s| s == "*"); - let binaries = binaries - .into_iter() - .filter(|s| s != "*") - .map(|s| basename(&s).to_string()) - .collect(); - Self { binaries, wildcard } + let mut command_names = HashSet::new(); + let mut paths = HashSet::new(); + let mut unresolved_paths = HashSet::new(); + for binary in binaries.into_iter().filter(|s| s != "*") { + if contains_path_separator(&binary) { + match canonicalize_allowlist_path(&binary) { + Ok(path) => { + paths.insert(path); + } + Err(_) => { + unresolved_paths.insert(binary); + } + } + } else { + command_names.insert(binary); + } + } + Self { + binaries: command_names, + paths, + unresolved_paths, + wildcard, + } } /// Parse a CSV string into an `AllowlistEvaluator`. @@ -92,11 +115,9 @@ impl AllowlistEvaluator { impl Evaluator for AllowlistEvaluator { async fn evaluate(&self, action: &Action) -> Decision { let audit_id = Uuid::new_v4(); - if let ActionDetail::Spawn { argv, .. } = &action.detail { - let comm = argv - .first() - .map(|s| basename(s).to_string()) - .unwrap_or_default(); + if let ActionDetail::Spawn { argv, cwd } = &action.detail { + let requested = argv.first().map(String::as_str).unwrap_or_default(); + let comm = spawn_display_name(requested); if self.wildcard { return Decision { audit_id, @@ -104,18 +125,44 @@ impl Evaluator for AllowlistEvaluator { reason: format!("wildcard allowlist; spawn={comm}"), }; } - if self.binaries.is_empty() { + let empty_allowlist = self.binaries.is_empty() + && self.paths.is_empty() + && self.unresolved_paths.is_empty(); + if empty_allowlist { return Decision { audit_id, verdict: Verdict::Allow, reason: "empty allowlist (development mode)".into(), }; } - if self.binaries.contains(&comm) { + if contains_path_separator(requested) { + return match canonicalize_spawn_path(requested, cwd) { + Ok(path) if self.paths.contains(&path) => Decision { + audit_id, + verdict: Verdict::Allow, + reason: format!("allowlisted path: {}", path.display()), + }, + Ok(path) => Decision { + audit_id, + verdict: Verdict::Deny, + reason: format!( + "binary path {} not in allowlist; permitted: {}", + path.display(), + self.allowed_summary() + ), + }, + Err(e) => Decision { + audit_id, + verdict: Verdict::Deny, + reason: format!("binary path {requested} could not be resolved: {e}"), + }, + }; + } + if self.binaries.contains(requested) { return Decision { audit_id, verdict: Verdict::Allow, - reason: format!("allowlisted: {comm}"), + reason: format!("allowlisted command: {requested}"), }; } return Decision { @@ -123,7 +170,7 @@ impl Evaluator for AllowlistEvaluator { verdict: Verdict::Deny, reason: format!( "binary {comm} not in allowlist; permitted: {}", - self.binaries.iter().cloned().collect::>().join(", ") + self.allowed_summary() ), }; } @@ -135,8 +182,52 @@ impl Evaluator for AllowlistEvaluator { } } -fn basename(path: &str) -> &str { - path.rsplit('/').next().unwrap_or(path) +impl AllowlistEvaluator { + fn allowed_summary(&self) -> String { + let mut entries = self.binaries.iter().cloned().collect::>(); + entries.extend(self.paths.iter().map(|p| p.display().to_string())); + entries.extend( + self.unresolved_paths + .iter() + .map(|p| format!("{p} (unresolved)")), + ); + entries.sort(); + if entries.is_empty() { + "(none)".to_string() + } else { + entries.join(", ") + } + } +} + +fn spawn_display_name(path: &str) -> String { + path.rsplit(['/', '\\']).next().unwrap_or(path).to_string() +} + +fn contains_path_separator(path: &str) -> bool { + path.contains('/') || path.contains('\\') +} + +fn canonicalize_allowlist_path(path: &str) -> std::io::Result { + let path = Path::new(path); + if path.is_absolute() { + path.canonicalize() + } else { + std::env::current_dir()?.join(path).canonicalize() + } +} + +fn canonicalize_spawn_path(path: &str, cwd: &str) -> std::io::Result { + let path = Path::new(path); + if path.is_absolute() { + return path.canonicalize(); + } + let base = if cwd.trim().is_empty() { + std::env::current_dir()? + } else { + PathBuf::from(cwd) + }; + base.join(path).canonicalize() } /// Per-daemon governance state. @@ -251,14 +342,38 @@ mod tests { async fn allowlist_allows_known_binary_by_basename() { let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["bash".into()]))); let d = g - .check(build::spawn( - vec!["/bin/bash".into(), "-i".into()], - "/".into(), - )) + .check(build::spawn(vec!["bash".into(), "-i".into()], "/".into())) .await; assert_eq!(d.verdict, Verdict::Allow); } + #[tokio::test] + async fn basename_allowlist_does_not_allow_path_invocation() { + let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["git".into()]))); + let d = g + .check(build::spawn(vec!["./git".into()], "/".into())) + .await; + assert_eq!(d.verdict, Verdict::Deny); + } + + #[tokio::test] + async fn unresolved_path_entry_does_not_make_allowlist_permissive() { + let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec![ + "/definitely/missing/agent-tui-test-binary".into(), + ]))); + let d = g.check(build::spawn(vec!["bash".into()], "/".into())).await; + assert_eq!(d.verdict, Verdict::Deny); + } + + #[tokio::test] + async fn path_allowlist_allows_exact_canonical_path() { + let exe = std::env::current_exe().expect("current test binary"); + let exe_str = exe.to_string_lossy().into_owned(); + let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec![exe_str.clone()]))); + let d = g.check(build::spawn(vec![exe_str], "/".into())).await; + assert_eq!(d.verdict, Verdict::Allow); + } + #[tokio::test] async fn allowlist_wildcard_allows_anything() { let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["*".into()]))); @@ -281,9 +396,7 @@ mod tests { async fn audit_event_emitted_on_check() { let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["bash".into()]))); let mut sub = g.subscribe(); - let _ = g - .check(build::spawn(vec!["/bin/bash".into()], "/".into())) - .await; + let _ = g.check(build::spawn(vec!["bash".into()], "/".into())).await; let evt = sub.recv().await.expect("event"); assert_eq!(evt.action_kind, ActionKind::Spawn); assert_eq!(evt.verdict, Verdict::Allow); diff --git a/crates/agent-tui-daemon/src/handlers/snapshot.rs b/crates/agent-tui-daemon/src/handlers/snapshot.rs index e18c019..74a43f9 100644 --- a/crates/agent-tui-daemon/src/handlers/snapshot.rs +++ b/crates/agent-tui-daemon/src/handlers/snapshot.rs @@ -6,6 +6,7 @@ //! built-in `generic_outline` heuristic so the agent never gets `null`. use std::collections::HashMap; +use std::path::{Component, Path}; use std::sync::Arc; use agent_tui_engine::{Cell, EngineSnapshot}; @@ -16,6 +17,7 @@ use agent_tui_protocol::{ Selector, Snapshot, Warning, format_selector_parse_error, outline_all_refs, }; use base64::Engine as _; +use tokio::io::AsyncWriteExt; use crate::classifier; use crate::hash_window::HashWindow; @@ -299,13 +301,45 @@ async fn render_png_artifact( "report a bug", )) })?; - tokio::fs::write(path, &rendered.bytes).await.map_err(|e| { + + validate_png_artifact_path(path).map_err(|reason| { + Response::err(ErrorBody::new( + ErrorCode::InvalidArgs, + reason, + "use a new relative path under the daemon's current directory", + )) + })?; + let mut file = tokio::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path) + .await + .map_err(|e| { + let code = if e.kind() == std::io::ErrorKind::AlreadyExists { + ErrorCode::InvalidArgs + } else { + ErrorCode::Internal + }; + Response::err(ErrorBody::new( + code, + format!("failed to create PNG at {path}: {e}"), + "choose a new writable path", + )) + })?; + file.write_all(&rendered.bytes).await.map_err(|e| { Response::err(ErrorBody::new( ErrorCode::Internal, format!("failed to write PNG to {path}: {e}"), "check the path is writable", )) })?; + file.flush().await.map_err(|e| { + Response::err(ErrorBody::new( + ErrorCode::Internal, + format!("failed to flush PNG to {path}: {e}"), + "check the path is writable", + )) + })?; Ok(PngInfo { path: path.to_string(), width: rendered.width, @@ -314,6 +348,54 @@ async fn render_png_artifact( }) } +fn validate_png_artifact_path(path: &str) -> Result<(), String> { + let path = Path::new(path); + if path.as_os_str().is_empty() { + return Err("PNG path cannot be empty".to_string()); + } + if path.is_absolute() { + return Err(format!("PNG path {} must be relative", path.display())); + } + + let mut has_normal_component = false; + for component in path.components() { + match component { + Component::Normal(_) => has_normal_component = true, + Component::CurDir => {} + Component::ParentDir | Component::RootDir | Component::Prefix(_) => { + return Err(format!( + "PNG path {} must not contain traversal or root components", + path.display() + )); + } + } + } + if !has_normal_component { + return Err(format!("PNG path {} must name a file", path.display())); + } + + let cwd = std::env::current_dir() + .and_then(|p| p.canonicalize()) + .map_err(|e| format!("cannot resolve daemon current directory: {e}"))?; + let parent = path.parent().filter(|p| !p.as_os_str().is_empty()); + let parent = parent.unwrap_or_else(|| Path::new(".")); + let parent = parent.canonicalize().map_err(|e| { + format!( + "cannot resolve PNG parent directory {}: {e}", + parent.display() + ) + })?; + if !parent.starts_with(&cwd) { + return Err(format!( + "PNG path {} escapes daemon current directory {}", + path.display(), + cwd.display() + )); + } + + Ok(()) +} + /// Resolve the pane's outline: the attached adapter's, or the generic /// heuristic when the adapter fails or returns nothing — so callers (and the /// `--annotate` overlay) never face a `null` outline. @@ -621,6 +703,34 @@ mod tests { // are >= 256, so a fully-default cell carries no color. const DEF: u32 = 256; + #[test] + fn png_path_validation_accepts_relative_file() { + validate_png_artifact_path("snapshot.png").expect("relative file path should be accepted"); + } + + #[test] + fn png_path_validation_rejects_absolute_path() { + let absolute = std::env::current_dir() + .expect("current dir") + .join("snapshot.png"); + let err = validate_png_artifact_path(&absolute.to_string_lossy()) + .expect_err("absolute path must be rejected"); + assert!(err.contains("must be relative")); + } + + #[test] + fn png_path_validation_rejects_parent_traversal() { + let err = + validate_png_artifact_path("../snapshot.png").expect_err("traversal must be rejected"); + assert!(err.contains("must not contain traversal")); + } + + #[test] + fn png_path_validation_rejects_non_file_path() { + let err = validate_png_artifact_path(".").expect_err("directory path must be rejected"); + assert!(err.contains("must name a file")); + } + #[test] fn plain_mode_emits_no_escapes() { let snap = row(&[("h", 1, DEF, 0), ("i", DEF, DEF, 0)]); diff --git a/crates/agent-tui-daemon/src/paths.rs b/crates/agent-tui-daemon/src/paths.rs index f94e2cc..36d17ee 100644 --- a/crates/agent-tui-daemon/src/paths.rs +++ b/crates/agent-tui-daemon/src/paths.rs @@ -104,10 +104,49 @@ impl SocketLayout { env::temp_dir().join("agent-tui") } + /// Create the root directory if missing. Idempotent. + /// + /// # Errors + /// Filesystem errors from `create_dir_all`, ownership checks, or + /// permission tightening. + #[cfg(unix)] + pub fn ensure_root(&self) -> std::io::Result<()> { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + std::fs::create_dir_all(&self.root)?; + let meta = std::fs::symlink_metadata(&self.root)?; + if !meta.file_type().is_dir() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("socket root {} is not a directory", self.root.display()), + )); + } + let euid = nix::unistd::Uid::effective().as_raw(); + if meta.uid() != euid { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!( + "socket root {} is owned by uid {}, not current uid {}", + self.root.display(), + meta.uid(), + euid + ), + )); + } + let mode = meta.permissions().mode(); + if mode & 0o077 != 0 { + let mut perms = meta.permissions(); + perms.set_mode(mode & !0o077); + std::fs::set_permissions(&self.root, perms)?; + } + Ok(()) + } + /// Create the root directory if missing. Idempotent. /// /// # Errors /// Filesystem errors from `create_dir_all`. + #[cfg(not(unix))] pub fn ensure_root(&self) -> std::io::Result<()> { std::fs::create_dir_all(&self.root) } @@ -137,6 +176,30 @@ mod tests { assert!(l.version.to_string_lossy().ends_with("hello.version")); } + #[cfg(unix)] + #[test] + fn ensure_root_tightens_group_and_other_permissions() { + use std::os::unix::fs::PermissionsExt; + + let root = + std::env::temp_dir().join(format!("agent-tui-paths-{}-tighten", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).expect("create temp socket root"); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o755)) + .expect("make root too broad"); + + let layout = SocketLayout::for_session_in(&SessionId("tight".to_string()), root.clone()); + layout.ensure_root().expect("tighten socket root"); + + let mode = std::fs::metadata(&root) + .expect("stat socket root") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o700); + let _ = std::fs::remove_dir_all(&root); + } + // Note: the env-driven `explicit_socket_dir_wins` path is exercised by // integration tests under `tests/` (which can opt into `unsafe` env // mutation), not here — `#![forbid(unsafe_code)]` is non-negotiable for diff --git a/crates/agent-tui-daemon/src/server.rs b/crates/agent-tui-daemon/src/server.rs index 938016b..bfb4f75 100644 --- a/crates/agent-tui-daemon/src/server.rs +++ b/crates/agent-tui-daemon/src/server.rs @@ -13,7 +13,7 @@ use agent_tui_protocol::{ use interprocess::local_socket::ListenerOptions; use interprocess::local_socket::tokio::{Listener, Stream}; use interprocess::local_socket::traits::tokio::Listener as _; -use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; +use tokio::io::{AsyncBufRead, AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}; use tokio::sync::Notify; use tracing::{debug, error, info, warn}; @@ -62,6 +62,11 @@ pub struct DaemonConfig { /// CI run. pub const DEFAULT_IDLE_TIMEOUT_SECS: u64 = 900; +/// Maximum bytes accepted for one daemon request line, including the +/// terminating newline. Normal JSON-RPC commands are tiny; 1 MiB leaves room +/// for structured payloads while bounding memory spent on a single client. +const MAX_REQUEST_LINE_BYTES: usize = 1024 * 1024; + fn build_governance(cfg: &DaemonConfig) -> Governance { use super::governance::{AllowlistEvaluator, Governance}; if let Some(csv) = cfg.allowed_binaries.as_ref() { @@ -277,9 +282,9 @@ pub async fn run_daemon(cfg: DaemonConfig) -> std::io::Result { async fn handle_conn(sock: Stream, state: DaemonState) { let (reader, mut writer) = tokio::io::split(sock); - let mut lines = BufReader::new(reader).lines(); + let mut reader = BufReader::new(reader); loop { - match lines.next_line().await { + match read_request_line(&mut reader).await { Ok(Some(line)) => { if line.trim().is_empty() { continue; @@ -306,7 +311,7 @@ async fn handle_conn(sock: Stream, state: DaemonState) { let disconnected = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); let watcher_flag = disconnected.clone(); - let read_half = lines.into_inner().into_inner(); + let read_half = reader.into_inner(); let watcher = tokio::spawn(async move { use tokio::io::AsyncReadExt as _; let mut r = read_half; @@ -365,6 +370,32 @@ async fn handle_conn(sock: Stream, state: DaemonState) { } } +async fn read_request_line(reader: &mut R) -> std::io::Result> +where + R: AsyncBufRead + Unpin, +{ + let mut buf = Vec::new(); + let read = { + let mut limited = (&mut *reader).take((MAX_REQUEST_LINE_BYTES + 1) as u64); + limited.read_until(b'\n', &mut buf).await? + }; + if read == 0 && buf.is_empty() { + return Ok(None); + } + if buf.len() > MAX_REQUEST_LINE_BYTES { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("request line exceeds {MAX_REQUEST_LINE_BYTES} bytes"), + )); + } + String::from_utf8(buf).map(Some).map_err(|e| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("request line is not valid UTF-8: {e}"), + ) + }) +} + /// Stream the child's output to the client as new bytes arrive. /// Emits one envelope per chunk plus a final `{type:"eof"}` envelope. /// @@ -1109,6 +1140,44 @@ async fn dispatch_snapshot( .await } +async fn daemon_upgrade_policy_response( + state: &DaemonState, + binary: Option<&str>, +) -> Option { + let executable = match binary { + Some(binary) => binary.to_string(), + None => match std::env::current_exe() { + Ok(path) => path.to_string_lossy().into_owned(), + Err(_) => return None, + }, + }; + let decision = state + .governance + .check(super::governance::build::spawn( + vec![executable], + String::new(), + )) + .await; + policy_response_for_daemon_command(&decision) +} + +fn policy_response_for_daemon_command(decision: &agent_tui_protocol::Decision) -> Option { + use agent_tui_protocol::Verdict; + match decision.verdict { + Verdict::Allow => None, + Verdict::Deny => Some(Response::err(ErrorBody::new( + ErrorCode::PolicyDenied, + decision.reason.clone(), + "daemon command blocked; loosen --allowed-binaries or choose an allowed binary", + ))), + Verdict::RequireConfirm => Some(Response::err(ErrorBody::new( + ErrorCode::PolicyPending, + decision.reason.clone(), + "human confirmation required; `policy confirm ` lands in a follow-on cycle", + ))), + } +} + #[allow(clippy::too_many_lines)] async fn dispatch_command(state: &DaemonState, cmd: agent_tui_protocol::Command) -> Response { use agent_tui_protocol::Command; @@ -1237,7 +1306,12 @@ async fn dispatch_command(state: &DaemonState, cmd: agent_tui_protocol::Command) "status": "shutting_down", })) } - Command::DaemonUpgrade { binary } => crate::upgrade::run(state, binary).await, + Command::DaemonUpgrade { binary } => { + if let Some(resp) = daemon_upgrade_policy_response(state, binary.as_deref()).await { + return resp; + } + crate::upgrade::run(state, binary).await + } Command::Focus { pane } => handlers::focus::run(&state.registry, pane).await, Command::Eval { .. } => Response::err(ErrorBody::new( ErrorCode::Internal, @@ -1314,3 +1388,28 @@ async fn idle_timeout_watcher( // let _ = nix::sys::prctl::set_pdeathsig( // Some(nix::sys::signal::Signal::SIGTERM), // ); + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn read_request_line_accepts_normal_line() { + let mut reader = BufReader::new(std::io::Cursor::new(b"{\"op\":\"list\"}\n".to_vec())); + let line = read_request_line(&mut reader) + .await + .expect("read request") + .expect("line"); + assert_eq!(line, "{\"op\":\"list\"}\n"); + } + + #[tokio::test] + async fn read_request_line_rejects_oversized_frame() { + let mut reader = + BufReader::new(std::io::Cursor::new(vec![b'a'; MAX_REQUEST_LINE_BYTES + 1])); + let err = read_request_line(&mut reader) + .await + .expect_err("oversized request must fail"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + } +} diff --git a/crates/agent-tui-daemon/tests/round_trip.rs b/crates/agent-tui-daemon/tests/round_trip.rs index 36680f6..4e8a1b8 100644 --- a/crates/agent-tui-daemon/tests/round_trip.rs +++ b/crates/agent-tui-daemon/tests/round_trip.rs @@ -17,7 +17,9 @@ use std::time::{Duration, Instant}; use agent_tui_daemon::{DaemonConfig, SocketLayout, run_daemon}; use agent_tui_protocol::request::SnapshotMode; -use agent_tui_protocol::{Command, PROTOCOL_VERSION, PaneId, Request, ResponseEnvelope, SessionId}; +use agent_tui_protocol::{ + Command, ErrorCode, PROTOCOL_VERSION, PaneId, Request, ResponseEnvelope, SessionId, +}; use base64::Engine as _; use interprocess::local_socket::tokio::Stream; use interprocess::local_socket::traits::tokio::Stream as _; @@ -65,6 +67,14 @@ fn short_temp_root(prefix: &str) -> PathBuf { PathBuf::from(format!("/tmp/{prefix}-{h}")) } +fn relative_artifact_dir(prefix: &str) -> PathBuf { + let mut h = Uuid::new_v4().simple().to_string(); + h.truncate(8); + PathBuf::from("target") + .join("test-artifacts") + .join(format!("{prefix}-{h}")) +} + async fn round_trip(cfg: &DaemonConfig, command: Command) -> ResponseEnvelope { let name = agent_tui_daemon::paths::socket_name(&cfg.layout).expect("name"); let stream = Stream::connect(name).await.expect("connect"); @@ -1201,7 +1211,10 @@ async fn snapshot_png_writes_valid_image() { ) .await; - let dir = short_temp_root("at-png"); + let abs_dir = short_temp_root("at-png-abs"); + std::fs::create_dir_all(&abs_dir).expect("mkdir absolute png dir"); + let abs_path = abs_dir.join("shot.png"); + let dir = relative_artifact_dir("at-png"); std::fs::create_dir_all(&dir).expect("mkdir png dir"); let path = dir.join("shot.png"); @@ -1211,6 +1224,29 @@ async fn snapshot_png_writes_valid_image() { "pane never displayed 'hello'" ); + let rejected = round_trip( + &cfg, + Command::Snapshot { + pane: None, + mode: SnapshotMode::Outline, + png: Some(abs_path.to_string_lossy().into_owned()), + annotate: None, + chrome: None, + select: None, + all: false, + keep_color: false, + }, + ) + .await; + assert!( + rejected.response.is_failure(), + "absolute PNG path should be rejected: {rejected:?}" + ); + assert_eq!( + rejected.response.error.expect("error").code, + ErrorCode::InvalidArgs + ); + let snap = round_trip( &cfg, Command::Snapshot { @@ -1250,6 +1286,7 @@ async fn snapshot_png_writes_valid_image() { ); let _ = std::fs::remove_dir_all(&dir); + let _ = std::fs::remove_dir_all(&abs_dir); let _ = round_trip( &cfg, Command::Die { @@ -1282,7 +1319,7 @@ async fn snapshot_png_annotate_overlays_refs() { ) .await; - let dir = short_temp_root("at-png-an"); + let dir = relative_artifact_dir("at-png-an"); std::fs::create_dir_all(&dir).expect("mkdir png dir"); let plain = dir.join("plain.png"); let annot = dir.join("annot.png"); diff --git a/crates/agent-tui/src/cli.rs b/crates/agent-tui/src/cli.rs index 56fe9e0..aedffdf 100644 --- a/crates/agent-tui/src/cli.rs +++ b/crates/agent-tui/src/cli.rs @@ -50,8 +50,10 @@ pub struct GlobalArgs { /// Truncate snapshot payloads at N characters. #[arg(long, value_name = "N", global = true)] pub max_output: Option, - /// Comma-separated allowlist of binary basenames `spawn` will accept. - /// `*` allows everything (audit-only). Empty / unset = no restriction. + /// Comma-separated allowlist of binary basenames or executable paths + /// `spawn` will accept. Bare names only match bare argv[0] values; path + /// invocations require an exact canonical path entry. `*` allows + /// everything (audit-only). Empty / unset = no restriction. /// Env: `AGENT_TUI_ALLOWED_BINARIES`. #[arg( long, diff --git a/crates/agent-tui/src/client.rs b/crates/agent-tui/src/client.rs index 93d9e64..969aaa5 100644 --- a/crates/agent-tui/src/client.rs +++ b/crates/agent-tui/src/client.rs @@ -20,14 +20,49 @@ use tokio::time::timeout; use tracing::debug; use uuid::Uuid; +/// Policy values the client must copy into a daemon it lazily starts. +/// +/// Foreground `daemon run` receives these from clap directly. Lazy-spawn is +/// different: the parent CLI command already parsed the globals, then starts a +/// fresh `agent-tui daemon run` child. Keep the policy payload explicit so the +/// spawned daemon cannot silently fall back to permissive defaults. +#[derive(Debug, Clone, Default)] +pub struct LazySpawnConfig { + pub allowed_binaries: Option, +} + +impl LazySpawnConfig { + #[must_use] + pub fn from_allowed_binaries(allowed_binaries: Option<&str>) -> Self { + Self { + allowed_binaries: allowed_binaries.map(str::to_string), + } + } + + fn resolved_allowed_binaries(&self) -> Option { + self.allowed_binaries + .clone() + .or_else(|| std::env::var("AGENT_TUI_ALLOWED_BINARIES").ok()) + } +} + /// Connect, send one request, read one response. Spawns the daemon if no /// socket is listening. pub async fn one_shot(layout: &SocketLayout, command: Command) -> Result { + one_shot_with_config(layout, command, &LazySpawnConfig::default()).await +} + +/// [`one_shot`] with explicit lazy-spawn policy propagation. +pub async fn one_shot_with_config( + layout: &SocketLayout, + command: Command, + lazy_spawn: &LazySpawnConfig, +) -> Result { let stream = match connect(layout).await { Ok(s) => s, Err(e) if is_unreachable(&e) => { debug!(socket = %layout.socket.display(), "daemon unreachable, spawning"); - spawn_daemon(layout)?; + spawn_daemon(layout, lazy_spawn)?; wait_for_socket(layout, Duration::from_secs(3)).await? } Err(e) => return Err(e), @@ -63,7 +98,7 @@ fn is_unreachable(err: &anyhow::Error) -> bool { }) } -fn spawn_daemon(layout: &SocketLayout) -> Result<()> { +fn spawn_daemon(layout: &SocketLayout, lazy_spawn: &LazySpawnConfig) -> Result<()> { let exe = std::env::current_exe().context("current_exe")?; let session = layout .socket @@ -77,10 +112,10 @@ fn spawn_daemon(layout: &SocketLayout) -> Result<()> { .arg("--socket-dir") .arg(&layout.root); // Forward governance settings to the lazily-spawned daemon child. The - // CLI's `--allowed-binaries` value lives on the *parent* invocation; we - // propagate it via the env-var binding clap already declares so the - // daemon process sees the same allowlist. - if let Ok(csv) = std::env::var("AGENT_TUI_ALLOWED_BINARIES") { + // CLI's `--allowed-binaries` value lives on the *parent* invocation; copy + // the parsed value explicitly, falling back to the env-var binding clap + // declares for callers that configured policy through the environment. + if let Some(csv) = lazy_spawn.resolved_allowed_binaries() { cmd.env("AGENT_TUI_ALLOWED_BINARIES", csv); } cmd.arg("daemon").arg("run"); @@ -125,13 +160,23 @@ async fn wait_for_socket(layout: &SocketLayout, max_wait: Duration) -> Result Result, +) -> Result<()> { + stream_with_config(layout, command, &LazySpawnConfig::default(), on_envelope).await +} + +/// [`stream`] with explicit lazy-spawn policy propagation. +pub async fn stream_with_config( + layout: &SocketLayout, + command: Command, + lazy_spawn: &LazySpawnConfig, mut on_envelope: impl FnMut(&ResponseEnvelope) -> Result, ) -> Result<()> { let stream = match connect(layout).await { Ok(s) => s, Err(e) if is_unreachable(&e) => { debug!(socket = %layout.socket.display(), "daemon unreachable, spawning"); - spawn_daemon(layout)?; + spawn_daemon(layout, lazy_spawn)?; wait_for_socket(layout, Duration::from_secs(3)).await? } Err(e) => return Err(e), diff --git a/crates/agent-tui/src/commands.rs b/crates/agent-tui/src/commands.rs index f91b468..8dfb05a 100644 --- a/crates/agent-tui/src/commands.rs +++ b/crates/agent-tui/src/commands.rs @@ -203,6 +203,35 @@ async fn run_foreground_daemon( Ok(()) } +fn lazy_spawn_config(g: &crate::cli::GlobalArgs) -> client::LazySpawnConfig { + client::LazySpawnConfig::from_allowed_binaries(g.allowed_binaries.as_deref()) +} + +async fn client_one_shot( + g: &crate::cli::GlobalArgs, + layout: &agent_tui_daemon::SocketLayout, + cmd: Command, +) -> Result { + if g.allowed_binaries.is_none() { + return client::one_shot(layout, cmd).await; + } + let lazy_spawn = lazy_spawn_config(g); + client::one_shot_with_config(layout, cmd, &lazy_spawn).await +} + +async fn client_stream( + g: &crate::cli::GlobalArgs, + layout: &agent_tui_daemon::SocketLayout, + cmd: Command, + on_envelope: impl FnMut(&agent_tui_protocol::ResponseEnvelope) -> Result, +) -> Result<()> { + if g.allowed_binaries.is_none() { + return client::stream(layout, cmd, on_envelope).await; + } + let lazy_spawn = lazy_spawn_config(g); + client::stream_with_config(layout, cmd, &lazy_spawn, on_envelope).await +} + #[allow(clippy::too_many_lines)] /// Orchestrate the `run` sugar verb. Bundles: /// spawn --stdin pipe → optionally stdin --text + close-stdin → @@ -261,7 +290,8 @@ async fn run_orchestrate( StdinMode::Closed }; let env_pairs = parse_env_pairs(&env)?; - let spawn_env = client::one_shot( + let spawn_env = client_one_shot( + g, &layout, Command::Spawn { argv: argv.clone(), @@ -287,7 +317,8 @@ async fn run_orchestrate( // 2. Optionally write stdin bytes + close-stdin. if let Some(bytes) = &stdin_bytes { - let write_env = client::one_shot( + let write_env = client_one_shot( + g, &layout, Command::Stdin { pane: pane.clone(), @@ -298,7 +329,8 @@ async fn run_orchestrate( .await?; if write_env.response.is_failure() { // Best-effort cleanup before surfacing. - let _ = client::one_shot( + let _ = client_one_shot( + g, &layout, Command::Die { pane: pane.clone(), @@ -309,11 +341,12 @@ async fn run_orchestrate( println!("{}", serde_json::to_string(&write_env)?); std::process::exit(2); } - let _ = client::one_shot(&layout, Command::CloseStdin { pane: pane.clone() }).await?; + let _ = client_one_shot(g, &layout, Command::CloseStdin { pane: pane.clone() }).await?; } // 3. Wait for the child to exit. - let wait_env = client::one_shot( + let wait_env = client_one_shot( + g, &layout, Command::Wait { pane: pane.clone(), @@ -330,7 +363,8 @@ async fn run_orchestrate( .and_then(serde_json::Value::as_i64); // 4. Tail the bytes the child wrote. - let tail_env = client::one_shot( + let tail_env = client_one_shot( + g, &layout, Command::Tail { pane: pane.clone(), @@ -359,7 +393,8 @@ async fn run_orchestrate( }; // 5. Cleanup: best-effort die on the pane. - let _ = client::one_shot( + let _ = client_one_shot( + g, &layout, Command::Die { pane: pane.clone(), @@ -440,7 +475,7 @@ async fn tail_follow( }; let mut stdout = std::io::stdout().lock(); let mut child_exit: Option = None; - client::stream(&layout, cmd, |env| { + client_stream(g, &layout, cmd, |env| { if env.response.is_failure() { // Print the error envelope and stop. let line = serde_json::to_string(env)?; @@ -524,7 +559,7 @@ async fn attach_stream( }; let mut stdout = std::io::stdout().lock(); let mut child_exit: Option = None; - client::stream(&layout, cmd, |env| { + client_stream(g, &layout, cmd, |env| { if env.response.is_failure() { writeln!(stdout, "{}", serde_json::to_string(env)?).ok(); return Ok(false); @@ -883,7 +918,8 @@ async fn run_capture( } else { StdinMode::Closed }; - let spawn_env = client::one_shot( + let spawn_env = client_one_shot( + g, &layout, Command::Spawn { argv: argv.clone(), @@ -913,7 +949,8 @@ async fn run_capture( .map(|s| PaneId(s.to_string())); if let Some(bytes) = &stdin_bytes { - let _ = client::one_shot( + let _ = client_one_shot( + g, &layout, Command::Stdin { pane: pane.clone(), @@ -922,9 +959,10 @@ async fn run_capture( }, ) .await?; - let _ = client::one_shot(&layout, Command::CloseStdin { pane: pane.clone() }).await?; + let _ = client_one_shot(g, &layout, Command::CloseStdin { pane: pane.clone() }).await?; } - let _ = client::one_shot( + let _ = client_one_shot( + g, &layout, Command::Wait { pane: pane.clone(), @@ -933,7 +971,8 @@ async fn run_capture( }, ) .await?; - let tail_env = client::one_shot( + let tail_env = client_one_shot( + g, &layout, Command::Tail { pane: pane.clone(), @@ -951,7 +990,7 @@ async fn run_capture( .and_then(serde_json::Value::as_str) .unwrap_or_default() .to_string(); - let _ = client::one_shot(&layout, Command::Die { pane, grace: None }).await; + let _ = client_one_shot(g, &layout, Command::Die { pane, grace: None }).await; Ok(text) } @@ -983,7 +1022,8 @@ async fn watch_sugar(g: &crate::cli::GlobalArgs, argv: Vec) -> Result<() return Err(anyhow!("watch requires at least one positional argv")); } let layout = client::layout_for(&g.session, g.socket_dir.as_deref()); - let spawn_env = client::one_shot( + let spawn_env = client_one_shot( + g, &layout, Command::Spawn { argv: argv.clone(), @@ -1007,7 +1047,8 @@ async fn watch_sugar(g: &crate::cli::GlobalArgs, argv: Vec) -> Result<() .map(str::to_string); tail_follow(g, pane.clone(), 0, true).await?; // Best-effort die when streaming ends. - let _ = client::one_shot( + let _ = client_one_shot( + g, &layout, Command::Die { pane: pane.map(agent_tui_protocol::PaneId), @@ -1100,7 +1141,7 @@ async fn one_shot_print_with( exit_override: impl Fn(&agent_tui_protocol::ResponseEnvelope) -> Option, ) -> Result<()> { let layout = client::layout_for(&g.session, g.socket_dir.as_deref()); - let env = match client::one_shot(&layout, cmd).await { + let env = match client_one_shot(g, &layout, cmd).await { Ok(e) => e, Err(e) => { eprintln!("{e:#}"); @@ -1133,7 +1174,7 @@ async fn events_stream( debounce_ms: Some(debounce), }; let mut stdout = std::io::stdout().lock(); - client::stream(&layout, cmd, |env| { + client_stream(g, &layout, cmd, |env| { if env.response.is_failure() { writeln!(stdout, "{}", serde_json::to_string(env)?).ok(); return Ok(false); @@ -1375,7 +1416,7 @@ async fn doctor(g: &crate::cli::GlobalArgs, args: &crate::cli::DoctorArgs) -> Re "diagnostic_bundle": args.diagnostic_bundle.as_ref().map(|p| p.display().to_string()), }); - match client::one_shot(&layout, Command::DaemonStatus).await { + match client_one_shot(g, &layout, Command::DaemonStatus).await { Ok(env) if env.response.success => { report["daemon"] = serde_json::json!({ "reachable": true, diff --git a/crates/agent-tui/src/mcp.rs b/crates/agent-tui/src/mcp.rs index a0027d1..d5d1f53 100644 --- a/crates/agent-tui/src/mcp.rs +++ b/crates/agent-tui/src/mcp.rs @@ -16,7 +16,7 @@ //! //! Each tool is a thin wrapper around the existing agent-tui CLI //! command path — the MCP layer parses the MCP envelope, builds the -//! corresponding `Command`, dispatches via `client::one_shot`, and +//! corresponding `Command`, dispatches via the daemon client, and //! returns the response envelope as the tool's result text. The //! daemon's lazy-spawn machinery is transparent: the first tool call //! that needs a daemon starts one. @@ -33,7 +33,7 @@ use agent_tui_daemon::SocketLayout; use agent_tui_protocol::{Command, PaneId}; use anyhow::Result; use serde_json::{Value, json}; -use tokio::io::AsyncBufReadExt; +use tokio::io::{AsyncBufRead, AsyncBufReadExt, AsyncReadExt, BufReader}; use crate::cli::GlobalArgs; use crate::client; @@ -42,13 +42,24 @@ use crate::client; /// 2024-11-05 spec — broad enough that current Claude clients accept it. const MCP_PROTOCOL_VERSION: &str = "2024-11-05"; +/// Maximum bytes accepted for one MCP JSON-RPC line, including newline. +const MAX_MCP_LINE_BYTES: usize = 1024 * 1024; + /// Run the MCP server loop on stdio until EOF on stdin. pub async fn serve(globals: GlobalArgs) -> Result<()> { let stdin = tokio::io::stdin(); - let mut reader = tokio::io::BufReader::new(stdin).lines(); + let mut reader = BufReader::new(stdin); let layout = client::layout_for(&globals.session, globals.socket_dir.as_deref()); - while let Some(line) = reader.next_line().await? { + loop { + let line = match read_mcp_line(&mut reader).await { + Ok(Some(line)) => line, + Ok(None) => break, + Err(e) => { + send_response(&parse_error(None, &format!("invalid MCP frame: {e}")))?; + break; + } + }; if line.trim().is_empty() { continue; } @@ -84,6 +95,32 @@ pub async fn serve(globals: GlobalArgs) -> Result<()> { Ok(()) } +async fn read_mcp_line(reader: &mut R) -> std::io::Result> +where + R: AsyncBufRead + Unpin, +{ + let mut buf = Vec::new(); + let read = { + let mut limited = (&mut *reader).take((MAX_MCP_LINE_BYTES + 1) as u64); + limited.read_until(b'\n', &mut buf).await? + }; + if read == 0 && buf.is_empty() { + return Ok(None); + } + if buf.len() > MAX_MCP_LINE_BYTES { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("MCP frame exceeds {MAX_MCP_LINE_BYTES} bytes"), + )); + } + String::from_utf8(buf).map(Some).map_err(|e| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("MCP frame is not valid UTF-8: {e}"), + ) + }) +} + /// Dispatch one method. Returns the `result` field on success, or an /// `(error_code, message)` pair on failure. async fn dispatch( @@ -123,7 +160,7 @@ async fn dispatch( /// it via the daemon client, and wrap the response envelope in MCP /// `content` format. async fn call_tool( - _globals: &GlobalArgs, + globals: &GlobalArgs, layout: &SocketLayout, params: Value, ) -> Result { @@ -137,7 +174,9 @@ async fn call_tool( .unwrap_or(Value::Object(serde_json::Map::new())); let command = build_command(name, &args).map_err(McpError::invalid_params)?; - let envelope = client::one_shot(layout, command) + let lazy_spawn = + client::LazySpawnConfig::from_allowed_binaries(globals.allowed_binaries.as_deref()); + let envelope = client::one_shot_with_config(layout, command, &lazy_spawn) .await .map_err(|e| McpError::internal(&e.to_string()))?; @@ -527,6 +566,25 @@ mod tests { use super::*; use agent_tui_protocol::request::SnapshotMode; + #[tokio::test] + async fn read_mcp_line_accepts_normal_line() { + let mut reader = BufReader::new(std::io::Cursor::new(b"{\"jsonrpc\":\"2.0\"}\n".to_vec())); + let line = read_mcp_line(&mut reader) + .await + .expect("read MCP line") + .expect("line"); + assert_eq!(line, "{\"jsonrpc\":\"2.0\"}\n"); + } + + #[tokio::test] + async fn read_mcp_line_rejects_oversized_frame() { + let mut reader = BufReader::new(std::io::Cursor::new(vec![b'a'; MAX_MCP_LINE_BYTES + 1])); + let err = read_mcp_line(&mut reader) + .await + .expect_err("oversized MCP frame must fail"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + } + #[test] fn build_command_spawn() { let args = json!({ "argv": ["bash", "-l"] }); diff --git a/crates/agent-tui/tests/daemon_supervision.rs b/crates/agent-tui/tests/daemon_supervision.rs index f582684..1bab492 100644 --- a/crates/agent-tui/tests/daemon_supervision.rs +++ b/crates/agent-tui/tests/daemon_supervision.rs @@ -327,3 +327,42 @@ fn daemon_shutdown_reaps_pane_then_lazily_respawns() { // Tidy up the respawned daemon. let _ = s.cmd(&["daemon", "shutdown", "--force"], None).output(); } + +/// A lazily-spawned daemon must receive the parent CLI's global governance +/// flags. Without this propagation, the daemon starts permissive and accepts +/// `/bin/sh` even though this client invocation only allows the test binary. +#[test] +fn lazy_spawn_forwards_allowed_binaries_to_daemon() { + let s = Supervised::new("lazyallow"); + let owner_pid = s.owner.id(); + let out = s + .cmd( + &[ + "--allowed-binaries", + bin(), + "spawn", + "--", + "/bin/sh", + "-c", + "true", + ], + Some(owner_pid), + ) + .output() + .expect("run lazy-spawned policy check"); + + assert!( + !out.status.success(), + "spawn unexpectedly succeeded; stdout={} stderr={}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + let data = json(&out); + assert_eq!( + data.get("error").and_then(|e| e.get("code")), + Some(&Value::String("POLICY_DENIED".into())), + "lazy-spawned daemon should enforce the forwarded allowlist: {data:?}" + ); + + let _ = s.cmd(&["daemon", "shutdown", "--force"], None).output(); +}