From 7aaf189247f4fd758af1082bd48658835a1148a6 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 27 Aug 2026 11:11:06 +0900 Subject: [PATCH] fix: scope delegated commands by cwd --- crates/tools/src/bash.rs | 15 +- crates/tools/tests/integration.rs | 12 + crates/workdir/src/delegation.rs | 330 ++++++++++++++++++++++--- crates/workdir/src/local.rs | 42 +++- crates/workdir/src/operation.rs | 5 + crates/worker/tests/controller_test.rs | 4 + crates/workspace-server/src/server.rs | 1 + 7 files changed, 370 insertions(+), 39 deletions(-) diff --git a/crates/tools/src/bash.rs b/crates/tools/src/bash.rs index 8224472b..a8cff604 100644 --- a/crates/tools/src/bash.rs +++ b/crates/tools/src/bash.rs @@ -5,7 +5,9 @@ use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; use async_trait::async_trait; use schemars::JsonSchema; use serde::Deserialize; -use workdir::{CommandHandle, CommandOutputRequest, CommandRequest, WorkdirSessionHandle}; +use workdir::{ + CommandHandle, CommandOutputRequest, CommandRequest, WorkdirPath, WorkdirSessionHandle, +}; const DEFAULT_TIMEOUT_SECS: u64 = 120; const MAX_TIMEOUT_SECS: u64 = 600; @@ -14,6 +16,10 @@ const INLINE_BYTE_BUDGET: usize = 12 * 1024; #[derive(Debug, Deserialize, JsonSchema)] struct BashParams { command: String, + /// Optional logical working directory relative to the bound session cwd. + /// Supplying it lets delegation guards prove the command is disjoint from child write scopes. + #[serde(default)] + cwd: Option, #[serde(default)] timeout: Option, } @@ -51,11 +57,18 @@ impl Tool for BashTool { .timeout .unwrap_or(DEFAULT_TIMEOUT_SECS) .clamp(1, MAX_TIMEOUT_SECS); + let cwd = params + .cwd + .as_deref() + .map(WorkdirPath::new) + .transpose() + .map_err(crate::ToolsError::from)?; let cmd_summary = truncate_for_summary(¶ms.command); let handle = self .session .start_command(CommandRequest { command: params.command, + cwd, timeout_secs, output_limit: INLINE_BYTE_BUDGET, tool_call_id: Some(ctx.call_id), diff --git a/crates/tools/tests/integration.rs b/crates/tools/tests/integration.rs index f8761aa8..fd444c7a 100644 --- a/crates/tools/tests/integration.rs +++ b/crates/tools/tests/integration.rs @@ -390,6 +390,18 @@ async fn bash_inherits_workdir_cwd() { assert_eq!(actual, expected); } +#[tokio::test] +async fn bash_uses_explicit_logical_cwd() { + let (dir, _spill, reg) = setup(); + std::fs::create_dir_all(dir.path().join("nested")).unwrap(); + let bash = reg.get("Bash"); + let out = call(&bash, json!({ "command": "pwd", "cwd": "nested" })).await; + let body = out.content.unwrap(); + let actual = std::fs::canonicalize(body.trim()).unwrap(); + let expected = std::fs::canonicalize(dir.path().join("nested")).unwrap(); + assert_eq!(actual, expected); +} + #[tokio::test] async fn bash_provider_output_does_not_expose_internal_paths() { let (_dir, spill, reg) = setup(); diff --git a/crates/workdir/src/delegation.rs b/crates/workdir/src/delegation.rs index 4a5b313f..4ee2b356 100644 --- a/crates/workdir/src/delegation.rs +++ b/crates/workdir/src/delegation.rs @@ -251,14 +251,43 @@ impl DelegatingWorkdirSession { self.ensure_path(path, WorkdirDelegationPermission::Write) } - fn ensure_command(&self, starting: bool) -> Result<(), WorkdirError> { + fn resolve_command_cwd(&self, cwd: Option<&FsPath>) -> Result { + match cwd { + Some(cwd) => self.resolve_path(cwd), + None => Ok(self.cwd.clone()), + } + } + + fn ensure_command_start(&self, cwd: &FsPath) -> Result<(), WorkdirError> { self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; - if starting && self.has_active_write_lease() { - return Err(WorkdirError::Denied( - "command execution is denied while a child holds a write delegation".into(), - )); + if let Some(scope) = &self.scope + && !scope.iter().any(|rule| { + rule.permission == WorkdirDelegationPermission::Write + && rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) + }) + { + return Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` is outside the delegated write scope" + ))); + } + + let mut leases = self + .child_write_leases + .lock() + .expect("workdir delegation lease mutex poisoned"); + leases.retain(|_, lease| lease.validity.upgrade().is_some_and(|v| v.is_active())); + if leases.values().any(|lease| { + lease.rules.iter().any(|rule| { + rule.permission == WorkdirDelegationPermission::Write + && command_cwd_overlaps_rule(cwd, rule) + }) + }) { + Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` overlaps a child write delegation" + ))) + } else { + Ok(()) } - Ok(()) } fn ensure_parent_write_available(&self, path: &FsPath) -> Result<(), WorkdirError> { @@ -281,20 +310,6 @@ impl DelegatingWorkdirSession { } } - fn has_active_write_lease(&self) -> bool { - let mut leases = self - .child_write_leases - .lock() - .expect("workdir delegation lease mutex poisoned"); - leases.retain(|_, lease| lease.validity.upgrade().is_some_and(|v| v.is_active())); - leases.values().any(|lease| { - lease - .rules - .iter() - .any(|rule| rule.permission == WorkdirDelegationPermission::Write) - }) - } - fn validate_delegation_rules( &self, rules: &[WorkdirDelegationRule], @@ -502,13 +517,20 @@ impl WorkdirSession for DelegatingWorkdirSession { self.source.grep(request).await } - async fn start_command(&self, request: CommandRequest) -> Result { - self.ensure_command(true)?; + async fn start_command( + &self, + mut request: CommandRequest, + ) -> Result { + let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; + self.ensure_command_start(&cwd)?; + if !self.source.transports_delegation_context() { + request.cwd = Some(cwd); + } self.source.start_command(request).await } async fn command_status(&self, handle: CommandHandle) -> Result { - self.ensure_command(false)?; + self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; self.source.command_status(handle).await } @@ -516,12 +538,12 @@ impl WorkdirSession for DelegatingWorkdirSession { &self, request: CommandOutputRequest, ) -> Result { - self.ensure_command(false)?; + self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; self.source.command_output(request).await } async fn cancel_command(&self, handle: CommandHandle) -> Result<(), WorkdirError> { - self.ensure_command(false)?; + self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; self.source.cancel_command(handle).await } @@ -649,6 +671,11 @@ impl WorkdirSession for ReadOnlyWorkdirSession { } } +fn command_cwd_overlaps_rule(cwd: &FsPath, rule: &WorkdirDelegationRule) -> bool { + rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) + || Path::new(rule.target.as_str()).starts_with(Path::new(cwd.as_str())) +} + fn rule_allows_path( rule: &WorkdirDelegationRule, path: &FsPath, @@ -763,6 +790,7 @@ mod tests { let handle = parent .start_command(CommandRequest { command: "printf ready; sleep 0.2; printf done".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-delegated".into()), @@ -917,6 +945,18 @@ mod tests { .await, Err(WorkdirError::Denied(_)) )); + assert!(matches!( + parent + .start_command(CommandRequest { + command: "printf escaped".into(), + cwd: Some(fs_path("granted/outside")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("symlink-cwd".into()), + }) + .await, + Err(WorkdirError::Denied(message)) if message.contains("traverses a symlink") + )); parent .write(write("secret/parent", "still-authoritative")) .await @@ -924,7 +964,7 @@ mod tests { } #[tokio::test] - async fn write_lease_blocks_parent_region_until_release() { + async fn write_lease_blocks_only_overlapping_parent_command_cwds_until_release() { let root = TempDir::new().unwrap(); fs::create_dir_all(root.path().join("leased")).unwrap(); fs::create_dir_all(root.path().join("other")).unwrap(); @@ -941,7 +981,8 @@ mod tests { let command = child .scoped_session .start_command(CommandRequest { - command: "printf child-command".into(), + command: "pwd; printf child-command".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("delegated-child-command".into()), @@ -958,17 +999,65 @@ mod tests { }) .await .unwrap(); - assert_eq!(command_output.content, "child-command"); assert!( - parent - .start_command(CommandRequest { - command: "printf parent-command".into(), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("blocked-parent-command".into()), - }) - .await - .is_err() + command_output.content.ends_with("leased\nchild-command"), + "child command must run from its delegated cwd: {}", + command_output.content + ); + let denied = parent + .start_command(CommandRequest { + command: "printf parent-command".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("blocked-parent-command".into()), + }) + .await + .unwrap_err(); + assert!(matches!( + denied, + WorkdirError::Denied(message) + if message.contains("command cwd `.` overlaps a child write delegation") + )); + let denied = parent + .start_command(CommandRequest { + command: "printf still-denied".into(), + cwd: Some(fs_path("leased")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("overlapping-parent-command".into()), + }) + .await + .unwrap_err(); + assert!(matches!( + denied, + WorkdirError::Denied(message) + if message.contains("command cwd `leased` overlaps a child write delegation") + )); + + let unrelated = parent + .start_command(CommandRequest { + command: "pwd; printf parent-command".into(), + cwd: Some(fs_path("other")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("unrelated-parent-command".into()), + }) + .await + .unwrap(); + let unrelated_output = parent + .command_output(CommandOutputRequest { + handle: unrelated, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!( + unrelated_output.content.ends_with("other\nparent-command"), + "parent command must run from its explicit disjoint cwd: {}", + unrelated_output.content ); assert!(matches!( @@ -982,6 +1071,26 @@ mod tests { .await .unwrap(); child.release(); + let resumed = parent + .start_command(CommandRequest { + command: "printf resumed".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("resumed-parent-command".into()), + }) + .await + .unwrap(); + let resumed_output = parent + .command_output(CommandOutputRequest { + handle: resumed, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert_eq!(resumed_output.content, "resumed"); parent .write(write("leased/parent", "parent")) .await @@ -1033,6 +1142,153 @@ mod tests { )); } + #[tokio::test] + async fn nested_write_delegation_uses_each_session_cwd_without_widening_scope() { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("top/nested")).unwrap(); + fs::create_dir_all(root.path().join("top/peer")).unwrap(); + fs::create_dir_all(root.path().join("other")).unwrap(); + let root_session = session(root.path()); + let child = root_session + .delegate(request("top", WorkdirDelegationPermission::Write)) + .await + .unwrap(); + let nested = child + .scoped_session + .delegate(WorkdirDelegationRequest { + rules: vec![WorkdirDelegationRule { + target: fs_path("top/nested"), + permission: WorkdirDelegationPermission::Write, + recursive: true, + }], + cwd: fs_path("top/nested"), + }) + .await + .unwrap(); + + let nested_handle = nested + .scoped_session + .start_command(CommandRequest { + command: "pwd; printf nested".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("nested-command".into()), + }) + .await + .unwrap(); + let nested_output = nested + .scoped_session + .command_output(CommandOutputRequest { + handle: nested_handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(nested_output.content.ends_with("top/nested\nnested")); + + let denied = child + .scoped_session + .start_command(CommandRequest { + command: "printf blocked".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("nested-overlap".into()), + }) + .await + .unwrap_err(); + assert!(matches!( + denied, + WorkdirError::Denied(message) + if message.contains("command cwd `top` overlaps a child write delegation") + )); + + let peer_handle = child + .scoped_session + .start_command(CommandRequest { + command: "pwd; printf peer".into(), + cwd: Some(fs_path("peer")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("nested-peer".into()), + }) + .await + .unwrap(); + let peer_output = child + .scoped_session + .command_output(CommandOutputRequest { + handle: peer_handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(peer_output.content.ends_with("top/peer\npeer")); + + let outside_handle = root_session + .start_command(CommandRequest { + command: "pwd; printf outside".into(), + cwd: Some(fs_path("other")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("root-outside".into()), + }) + .await + .unwrap(); + let outside_output = root_session + .command_output(CommandOutputRequest { + handle: outside_handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(outside_output.content.ends_with("other\noutside")); + + nested.release(); + child.release(); + } + + #[tokio::test] + async fn reapplied_delegation_chain_preserves_command_cwd() { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("delegated")).unwrap(); + let parent = session(root.path()); + let applied = apply_delegation_chain( + parent, + [request("delegated", WorkdirDelegationPermission::Write)], + ) + .await + .unwrap(); + let scoped = &applied.scoped_session; + + let handle = scoped + .start_command(CommandRequest { + command: "pwd; printf reapplied".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("reapplied-command".into()), + }) + .await + .unwrap(); + let output = scoped + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(output.content.ends_with("delegated\nreapplied")); + } + #[tokio::test] async fn applied_chain_cannot_replace_outer_provider_attenuation() { let root = TempDir::new().unwrap(); diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 16686cb4..8b422970 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -548,6 +548,40 @@ impl LocalWorkdirSession { self.inner.root.join(path.as_str()) } } + + fn resolve_command_cwd(&self, cwd: Option<&WorkdirPath>) -> Result { + let Some(cwd) = cwd else { + return Ok(self.inner.cwd.clone()); + }; + let host_cwd = self.resolve(cwd); + let canonical_root = self + .inner + .root + .canonicalize() + .map_err(|error| WorkdirError::io(&self.inner.root, error))?; + let expected = if cwd.is_root() { + canonical_root + } else { + canonical_root.join(cwd.as_str()) + }; + let resolved = host_cwd + .canonicalize() + .map_err(|error| WorkdirError::io(&host_cwd, error))?; + if resolved != expected { + return Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` traverses a symlink" + ))); + } + let scope = self.inner.scope.snapshot(); + if !scope.is_readable(&resolved) + || !std::fs::metadata(&resolved).is_ok_and(|metadata| metadata.is_dir()) + { + return Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` is not a readable Workdir directory" + ))); + } + Ok(resolved) + } } #[async_trait] @@ -693,7 +727,7 @@ impl WorkdirSession for LocalWorkdirSession { self.ensure_open()?; let id = self.inner.next_command_id.fetch_add(1, Ordering::Relaxed); let handle = CommandHandle(format!("command-{id}")); - let cwd = self.inner.cwd.clone(); + let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; let (completion_tx, completion) = watch::channel(false); let command_id = handle.0.clone(); let telemetry = self.inner.command_telemetry.clone(); @@ -1438,6 +1472,7 @@ mod tests { &session, CommandRequest { command: "sleep 30".to_owned(), + cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -1964,6 +1999,7 @@ mod tests { &workdir, CommandRequest { command: "pwd && printf provider-command".into(), + cwd: None, timeout_secs: 5, output_limit: 4096, tool_call_id: None, @@ -1999,6 +2035,7 @@ mod tests { &workdir, CommandRequest { command: "printf 'aéz'".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, @@ -2222,6 +2259,7 @@ mod tests { &workdir, CommandRequest { command: "printf ready; printf warning >&2; sleep 0.2; printf done".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-7".into()), @@ -2325,6 +2363,7 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), + cwd: None, timeout_secs: 1, output_limit: 1024, tool_call_id: None, @@ -2394,6 +2433,7 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), + cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, diff --git a/crates/workdir/src/operation.rs b/crates/workdir/src/operation.rs index 47527ad8..fe19763c 100644 --- a/crates/workdir/src/operation.rs +++ b/crates/workdir/src/operation.rs @@ -1,3 +1,4 @@ +use fs_operation::FsPath; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -7,6 +8,10 @@ pub struct CommandHandle(pub String); #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CommandRequest { pub command: String, + /// Optional logical working directory relative to the calling session's cwd. + /// Providers must resolve and validate it before starting the process. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub cwd: Option, pub timeout_secs: u64, pub output_limit: usize, /// Optional caller-owned correlation id. Bash supplies its tool-call id so diff --git a/crates/worker/tests/controller_test.rs b/crates/worker/tests/controller_test.rs index 3e7e750b..c4a226a3 100644 --- a/crates/worker/tests/controller_test.rs +++ b/crates/worker/tests/controller_test.rs @@ -230,6 +230,7 @@ async fn shutdown_closes_bound_workdir_session() { let command = session .start_command(CommandRequest { command: "sleep 30".to_owned(), + cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -271,6 +272,7 @@ async fn controller_projects_workdir_command_events_and_snapshot_state() { let command = session .start_command(CommandRequest { command: "printf ready; sleep 0.3; printf done".to_owned(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-command-1".into()), @@ -378,6 +380,7 @@ async fn controller_refreshes_command_snapshot_after_high_output_provider_lag() .start_command(CommandRequest { command: "dd if=/dev/zero bs=8192 count=300 2>/dev/null | tr '\\0' x; sleep 5" .to_owned(), + cwd: None, timeout_secs: 10, output_limit: 1024, tool_call_id: Some("tool-high-output".into()), @@ -452,6 +455,7 @@ async fn controller_startup_failure_closes_bound_workdir_session() { session .start_command(CommandRequest { command: "printf unreachable".to_owned(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 49909a57..c18546fb 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -15271,6 +15271,7 @@ mod tests { let provider_handle = source .start_command(workdir::CommandRequest { command: "printf ready; sleep 30".to_string(), + cwd: None, timeout_secs: 60, output_limit: 4096, tool_call_id: Some("tool-call-command-session".to_string()),