diff --git a/crates/manifest/src/scope.rs b/crates/manifest/src/scope.rs index 67756c61..29bec5c8 100644 --- a/crates/manifest/src/scope.rs +++ b/crates/manifest/src/scope.rs @@ -214,6 +214,25 @@ impl Scope { }) } + /// Return whether this effective scope fully contains a requested rule. + /// This is used when attenuating provider authority without mutating the + /// parent scope. + pub fn allows_rule(&self, requested: &ScopeRule) -> Result { + let requested = resolve_rule(requested)?; + let covered = self + .allow + .iter() + .any(|candidate| rule_covers(candidate, &requested)); + if !covered { + return Ok(false); + } + let denied = self + .deny + .iter() + .any(|deny| denial_overlaps_requested(deny, &requested)); + Ok(!denied) + } + /// Effective permission for `path`. /// /// Returns `None` when `path` is outside every allow rule, or when diff --git a/crates/workdir/src/delegation.rs b/crates/workdir/src/delegation.rs index 19c3beea..cdbe7fdd 100644 --- a/crates/workdir/src/delegation.rs +++ b/crates/workdir/src/delegation.rs @@ -15,20 +15,23 @@ use crate::{ WorkdirSessionHandle, }; -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "snake_case")] pub enum WorkdirDelegationPermission { Read, Write, } -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] +#[serde(deny_unknown_fields)] pub struct WorkdirDelegationRule { pub target: FsPath, pub permission: WorkdirDelegationPermission, pub recursive: bool, } -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] +#[serde(deny_unknown_fields)] pub struct WorkdirDelegationRequest { pub rules: Vec, pub cwd: FsPath, @@ -101,6 +104,7 @@ struct ActiveWriteLease { struct DelegatingWorkdirSession { source: WorkdirSessionHandle, + cwd: FsPath, scope: Option>, capabilities: WorkdirSessionCapabilities, validity: Arc, @@ -125,6 +129,7 @@ pub fn delegation_capable_session(source: WorkdirSessionHandle) -> WorkdirSessio let capabilities = source.capabilities(); Arc::new(DelegatingWorkdirSession { source, + cwd: FsPath::new("").expect("empty Workdir path is valid"), scope: None, capabilities, validity: SessionValidity::root(), @@ -180,6 +185,17 @@ impl DelegatingWorkdirSession { Ok(()) } + fn resolve_path(&self, path: &FsPath) -> Result { + if self.cwd.as_str().is_empty() { + return Ok(path.clone()); + } + let joined = Path::new(self.cwd.as_str()).join(path.as_str()); + let joined = joined.to_str().ok_or_else(|| { + WorkdirError::Denied("logical Workdir path is not valid UTF-8".into()) + })?; + FsPath::new(joined).map_err(|error| WorkdirError::Denied(error.to_string())) + } + fn ensure_read( &self, path: &FsPath, @@ -308,14 +324,17 @@ impl WorkdirSession for DelegatingWorkdirSession { true } - async fn capture_delegation_source(&self) -> Result { + async fn capture_delegation_source( + &self, + request: &WorkdirDelegationRequest, + ) -> Result { self.ensure_active()?; if self.scope.is_some() { return Err(WorkdirError::Denied( "scoped Workdir sessions cannot expose their provider source".into(), )); } - self.source.capture_delegation_source().await + self.source.capture_delegation_source(request).await } async fn delegate( @@ -333,7 +352,7 @@ impl WorkdirSession for DelegatingWorkdirSession { request.cwd ))); } - let source = self.source.capture_delegation_source().await?; + let source = self.source.capture_delegation_source(&request).await?; let validity = SessionValidity::child(self.validity.clone()); let id = self.next_lease_id.fetch_add(1, Ordering::Relaxed); if request @@ -354,6 +373,7 @@ impl WorkdirSession for DelegatingWorkdirSession { } let child: WorkdirSessionHandle = Arc::new(DelegatingWorkdirSession { source, + cwd: request.cwd, scope: Some(request.rules), capabilities, validity: validity.clone(), @@ -374,37 +394,44 @@ impl WorkdirSession for DelegatingWorkdirSession { }) } - async fn stat(&self, request: StatRequest) -> Result { + async fn stat(&self, mut request: StatRequest) -> Result { + request.path = self.resolve_path(&request.path)?; self.ensure_read(&request.path, WorkdirSessionCapability::Read)?; self.source.stat(request).await } - async fn read(&self, request: ReadRequest) -> Result { + async fn read(&self, mut request: ReadRequest) -> Result { + request.path = self.resolve_path(&request.path)?; self.ensure_read(&request.path, WorkdirSessionCapability::Read)?; self.source.read(request).await } - async fn write(&self, request: WriteRequest) -> Result { + async fn write(&self, mut request: WriteRequest) -> Result { + request.path = self.resolve_path(&request.path)?; self.ensure_write(&request.path, WorkdirSessionCapability::Write)?; self.source.write(request).await } - async fn edit(&self, request: EditRequest) -> Result { + async fn edit(&self, mut request: EditRequest) -> Result { + request.path = self.resolve_path(&request.path)?; self.ensure_write(&request.path, WorkdirSessionCapability::Edit)?; self.source.edit(request).await } - async fn list(&self, request: ListRequest) -> Result { + async fn list(&self, mut request: ListRequest) -> Result { + request.path = self.resolve_path(&request.path)?; self.ensure_read(&request.path, WorkdirSessionCapability::Read)?; self.source.list(request).await } - async fn glob(&self, request: GlobRequest) -> Result { + async fn glob(&self, mut request: GlobRequest) -> Result { + request.path = self.resolve_path(&request.path)?; self.ensure_read(&request.path, WorkdirSessionCapability::Glob)?; self.source.glob(request).await } - async fn grep(&self, request: GrepRequest) -> Result { + async fn grep(&self, mut request: GrepRequest) -> Result { + request.path = self.resolve_path(&request.path)?; self.ensure_read(&request.path, WorkdirSessionCapability::Grep)?; self.source.grep(request).await } @@ -571,7 +598,10 @@ fn rule_contains_rule(parent: &WorkdirDelegationRule, child: &WorkdirDelegationR if !path_in_rule(parent, &child.target) { return false; } - parent.recursive || !child.recursive + if parent.recursive { + return true; + } + !child.recursive && parent.target == child.target } #[cfg(test)] @@ -650,7 +680,7 @@ mod tests { } #[tokio::test] - async fn read_only_delegation_allows_prefix_and_denies_siblings_and_mutation() { + async fn read_only_delegation_allows_prefix_and_denies_mutation() { let root = TempDir::new().unwrap(); fs::create_dir_all(root.path().join("docs")).unwrap(); fs::create_dir_all(root.path().join("secret")).unwrap(); @@ -666,18 +696,14 @@ mod tests { assert_eq!( child .scoped_session - .read(read("docs/readme.md")) + .read(read("readme.md")) .await .unwrap() .bytes, b"visible" ); assert!(matches!( - child.scoped_session.read(read("secret/key")).await, - Err(WorkdirError::Denied(_)) - )); - assert!(matches!( - child.scoped_session.write(write("docs/new.md", "no")).await, + child.scoped_session.write(write("new.md", "no")).await, Err(WorkdirError::Denied(_)) )); assert!( @@ -687,6 +713,55 @@ mod tests { ); } + #[cfg(unix)] + #[tokio::test] + async fn provider_scope_denies_read_through_symlink_outside_grant() { + use std::os::unix::fs::symlink; + + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("granted")).unwrap(); + fs::create_dir_all(root.path().join("secret")).unwrap(); + fs::write(root.path().join("secret/key"), "hidden").unwrap(); + symlink("../secret/key", root.path().join("granted/link")).unwrap(); + let parent = session(root.path()); + let child = parent + .delegate(request("granted", WorkdirDelegationPermission::Read)) + .await + .unwrap(); + + let result = child.scoped_session.read(read("link")).await; + assert!( + result.is_err(), + "symlink read escaped provider scope: {result:?}" + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn provider_scope_denies_write_through_symlink_outside_grant() { + use std::os::unix::fs::symlink; + + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("granted")).unwrap(); + fs::create_dir_all(root.path().join("secret")).unwrap(); + symlink("../secret", root.path().join("granted/outside")).unwrap(); + let parent = session(root.path()); + let child = parent + .delegate(request("granted", WorkdirDelegationPermission::Write)) + .await + .unwrap(); + + let result = child + .scoped_session + .write(write("outside/new", "forbidden")) + .await; + assert!( + result.is_err(), + "symlink write escaped provider scope: {result:?}" + ); + assert!(!root.path().join("secret/new").exists()); + } + #[tokio::test] async fn write_lease_blocks_parent_region_until_release() { let root = TempDir::new().unwrap(); @@ -705,7 +780,7 @@ mod tests { parent.write(write("other/file", "parent")).await.unwrap(); child .scoped_session - .write(write("leased/file", "child")) + .write(write("file", "child")) .await .unwrap(); child.release(); @@ -714,7 +789,7 @@ mod tests { .await .unwrap(); assert!(matches!( - child.scoped_session.read(read("leased/file")).await, + child.scoped_session.read(read("file")).await, Err(WorkdirError::SessionClosed) )); } @@ -737,15 +812,14 @@ mod tests { .await .unwrap(); - nested - .scoped_session - .read(read("docs/sub/a")) - .await - .unwrap(); - assert!(matches!( - nested.scoped_session.read(read("docs/peer/b")).await, - Err(WorkdirError::Denied(_)) - )); + nested.scoped_session.read(read("a")).await.unwrap(); + assert!( + child + .scoped_session + .delegate(request("other", WorkdirDelegationPermission::Read)) + .await + .is_err() + ); assert!( child .scoped_session @@ -756,7 +830,7 @@ mod tests { child.release(); assert!(matches!( - nested.scoped_session.read(read("docs/sub/a")).await, + nested.scoped_session.read(read("a")).await, Err(WorkdirError::SessionClosed) )); } @@ -774,7 +848,7 @@ mod tests { parent.close().await.unwrap(); assert!(matches!( - child.scoped_session.read(read("docs/a")).await, + child.scoped_session.read(read("a")).await, Err(WorkdirError::SessionClosed) )); } diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index c6855751..3483d30c 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -68,6 +68,15 @@ pub enum WorkdirSessionOperation { CommandCancel(CommandHandle), } +/// Wire envelope for an operation and its optional provider-enforced child scope. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct WorkdirSessionOperationRequest { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub delegation: Option, + pub operation: WorkdirSessionOperation, +} + /// Typed result paired with [`WorkdirSessionOperation`]. #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "operation", content = "result", rename_all = "snake_case")] @@ -207,6 +216,7 @@ mod client { workdir: Workdir, session_id: WorkdirSessionId, capabilities: WorkdirSessionCapabilities, + delegation: Option, closed: AtomicBool, } @@ -259,6 +269,7 @@ mod client { workdir: Workdir::new(opened.workdir_id.as_str()), session_id: opened.session_id, capabilities: opened.capabilities, + delegation: None, closed: AtomicBool::new(false), }) } @@ -285,6 +296,10 @@ mod client { "operations", ], )?; + let operation = WorkdirSessionOperationRequest { + delegation: self.delegation.clone(), + operation, + }; let response = self .client .post(url) @@ -313,7 +328,10 @@ mod client { self.capabilities } - async fn capture_delegation_source(&self) -> Result { + async fn capture_delegation_source( + &self, + request: &crate::WorkdirDelegationRequest, + ) -> Result { if self.closed.load(Ordering::Acquire) { return Err(WorkdirError::SessionClosed); } @@ -324,6 +342,7 @@ mod client { workdir: self.workdir.clone(), session_id: self.session_id.clone(), capabilities: self.capabilities, + delegation: Some(request.clone()), closed: AtomicBool::new(false), })) } diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index d165e67f..c68f8b52 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -151,7 +151,10 @@ pub trait WorkdirSession: std::fmt::Debug + Send + Sync { /// Capture a provider-specific source for a delegated child session. /// Remote providers use this boundary to pin attachment identity without /// exposing transport handles or host paths. - async fn capture_delegation_source(&self) -> Result { + async fn capture_delegation_source( + &self, + _request: &WorkdirDelegationRequest, + ) -> Result { Err(WorkdirError::Denied( "workdir provider does not support delegated sessions".into(), )) diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 6fdfc99c..c644b1fd 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -19,7 +19,7 @@ use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::time::Duration; use async_trait::async_trait; -use manifest::{Scope, SharedScope}; +use manifest::{Permission, Scope, ScopeConfig, ScopeRule, SharedScope}; use sha2::{Digest, Sha256}; use tokio::process::Command; use tokio::sync::{Mutex, Notify}; @@ -28,9 +28,10 @@ use tokio::task::JoinHandle; use crate::{ CommandHandle, CommandOutput, CommandOutputRequest, CommandRequest, CommandStatus, EditRequest, EditResult, GlobRequest, GlobResult, GrepRequest, GrepResult, ListRequest, ListResult, - ReadRequest, ReadResult, StatRequest, StatResult, Workdir, WorkdirError, WorkdirPath, - WorkdirSession, WorkdirSessionCapabilities, WorkdirSessionCapability, WorkdirSessionHandle, - WriteRequest, WriteResult, + ReadRequest, ReadResult, StatRequest, StatResult, Workdir, WorkdirDelegationPermission, + WorkdirDelegationRequest, WorkdirError, WorkdirPath, WorkdirSession, + WorkdirSessionCapabilities, WorkdirSessionCapability, WorkdirSessionHandle, WriteRequest, + WriteResult, }; #[cfg(test)] use crate::{EntryKind, WriteOutcome}; @@ -371,8 +372,46 @@ impl WorkdirSession for LocalWorkdirSession { self.inner.capabilities } - async fn capture_delegation_source(&self) -> Result { - Ok(Arc::new(self.clone())) + async fn capture_delegation_source( + &self, + request: &WorkdirDelegationRequest, + ) -> Result { + let host_rules = request + .rules + .iter() + .map(|rule| ScopeRule { + target: self.inner.root.join(rule.target.as_str()), + permission: match rule.permission { + WorkdirDelegationPermission::Read => Permission::Read, + WorkdirDelegationPermission::Write => Permission::Write, + }, + recursive: rule.recursive, + }) + .collect::>(); + let parent_scope = self.inner.scope.snapshot(); + for rule in &host_rules { + if !parent_scope + .allows_rule(rule) + .map_err(|error| WorkdirError::Denied(error.to_string()))? + { + return Err(WorkdirError::Denied(format!( + "delegated provider scope `{}` exceeds the parent session", + rule.target.display() + ))); + } + } + let child_scope = Scope::from_config(&ScopeConfig { + allow: host_rules, + deny: Vec::new(), + }) + .map_err(|error| WorkdirError::Denied(error.to_string()))?; + Ok(Arc::new(LocalWorkdirSession::materialized_bound( + self.inner.workdir.clone(), + self.inner.root.clone(), + self.inner.cwd.clone(), + SharedScope::new(child_scope), + self.inner.capabilities, + ))) } async fn stat(&self, request: StatRequest) -> Result { diff --git a/crates/workdir/src/workspace.rs b/crates/workdir/src/workspace.rs index ce2ed61c..1f070bdc 100644 --- a/crates/workdir/src/workspace.rs +++ b/crates/workdir/src/workspace.rs @@ -320,6 +320,8 @@ mod tests { pub struct WorkspaceWorkdirSessionOperationRequest { #[serde(default, skip_serializing_if = "Option::is_none")] pub expected_session_fence: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub delegation: Option, pub operation: crate::http::WorkdirSessionOperation, } diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index 481c60fc..48d31c86 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -59,8 +59,8 @@ use workdir::{ CommandOutput, CommandStatus, WorkdirSessionHandle, http::{ OpenWorkdirSessionRequest, OpenWorkdirSessionResponse, WorkdirSessionId, - WorkdirSessionOperation, WorkdirSessionOperationResult, WorkdirTransportError, - WorkdirTransportErrorCode, + WorkdirSessionOperation, WorkdirSessionOperationRequest, WorkdirSessionOperationResult, + WorkdirTransportError, WorkdirTransportErrorCode, }, }; @@ -596,11 +596,11 @@ async fn run_workdir_session_operation( State(state): State, Path(session_id): Path, auth: Option>, - body: Result, JsonRejection>, + body: Result, JsonRejection>, ) -> Result, RuntimeHttpWorkdirError> { - let Json(operation) = body.map_err(|_| RuntimeHttpWorkdirError::invalid_request())?; + let Json(request) = body.map_err(|_| RuntimeHttpWorkdirError::invalid_request())?; let owner = required_workdir_owner(auth)?; - let session = { + let source = { let sessions = state .workdir_sessions .lock() @@ -611,6 +611,19 @@ async fn run_workdir_session_operation( .ok_or_else(RuntimeHttpWorkdirError::not_found)?; record.session.clone() }; + let delegation = if let Some(delegation) = request.delegation { + Some( + workdir::delegation_capable_session(source.clone()) + .delegate(delegation) + .await?, + ) + } else { + None + }; + let session = delegation.as_ref().map_or(source.as_ref(), |delegation| { + delegation.scoped_session.as_ref() + }); + let operation = request.operation; let result = match operation { WorkdirSessionOperation::Stat(request) => { @@ -1836,7 +1849,8 @@ mod tests { use manifest::{Scope, SharedScope}; use tower::ServiceExt; use workdir::{ - LocalWorkdirSession, StatRequest, Workdir, WorkdirPath, WorkdirSessionCapabilities, + LocalWorkdirSession, ReadRequest, StatRequest, Workdir, WorkdirPath, + WorkdirSessionCapabilities, }; fn test_bundle(profile: ProfileSelector) -> ConfigBundle { @@ -2224,6 +2238,14 @@ mod tests { async fn workdir_session_operations_enforce_owner_and_close_terminally() { let temp = tempfile::tempdir().expect("tempdir"); std::fs::write(temp.path().join("hello.txt"), "hello").expect("write fixture"); + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + std::fs::create_dir(temp.path().join("granted")).expect("granted directory"); + std::fs::create_dir(temp.path().join("secret")).expect("secret directory"); + std::fs::write(temp.path().join("secret/key"), "hidden").expect("secret fixture"); + symlink("../secret/key", temp.path().join("granted/link")).expect("symlink fixture"); + } let scope = SharedScope::new(Scope::writable(temp.path()).expect("scope")); let session: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::materialized_bound( Workdir::new("wd-1"), @@ -2256,9 +2278,12 @@ mod tests { token_id: "token-a".to_string(), expires_at: u64::MAX, }; - let operation = WorkdirSessionOperation::Stat(StatRequest { - path: WorkdirPath::new("hello.txt").expect("logical path"), - }); + let operation = WorkdirSessionOperationRequest { + delegation: None, + operation: WorkdirSessionOperation::Stat(StatRequest { + path: WorkdirPath::new("hello.txt").expect("logical path"), + }), + }; let Json(result) = run_workdir_session_operation( State(state.clone()), @@ -2270,6 +2295,39 @@ mod tests { .expect("owned operation"); assert!(matches!(result, WorkdirSessionOperationResult::Stat(_))); + #[cfg(unix)] + { + let delegated_read = WorkdirSessionOperationRequest { + delegation: Some(workdir::WorkdirDelegationRequest { + rules: vec![workdir::WorkdirDelegationRule { + target: WorkdirPath::new("granted").unwrap(), + permission: workdir::WorkdirDelegationPermission::Read, + recursive: true, + }], + cwd: WorkdirPath::new("granted").unwrap(), + }), + operation: WorkdirSessionOperation::Read(ReadRequest { + path: WorkdirPath::new("link").unwrap(), + offset: 0, + limit: 20, + max_bytes: 1024, + }), + }; + let error = run_workdir_session_operation( + State(state.clone()), + Path("session-1".to_string()), + Some(Extension(auth.clone())), + Ok(Json(delegated_read)), + ) + .await + .expect_err("provider must reject delegated symlink escape"); + assert_ne!(error.status, StatusCode::OK); + assert_eq!( + std::fs::read_to_string(temp.path().join("secret/key")).unwrap(), + "hidden" + ); + } + let wrong_owner = RuntimeAuthContext { workspace_id: "workspace-b".to_string(), ..auth.clone() diff --git a/crates/worker/src/controller.rs b/crates/worker/src/controller.rs index 5c96edbb..443cd79c 100644 --- a/crates/worker/src/controller.rs +++ b/crates/worker/src/controller.rs @@ -910,10 +910,6 @@ where // without one so invocation fails deterministically until the parent // attaches a Workdir. if feature_config.sub_worker.enabled { - let spawner_cwd = local_filesystem - .as_ref() - .map(|local| local.cwd.clone()) - .unwrap_or_else(|| PathBuf::from("/")); let spawner_workspace_root = local_workspace_root .clone() .unwrap_or_else(|| PathBuf::from("/")); @@ -923,7 +919,6 @@ where parent_notifications, runtime_base.clone(), spawner_workspace_root, - spawner_cwd.clone(), source_workdir_session, spawned_registry.clone(), spawner_manifest, diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index 57cf359b..2318b77c 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -155,6 +155,7 @@ pub struct WorkspaceAttachedWorkdirSession { client: Arc, workdir: Workdir, expected_session_fence: Option, + delegation: Option, } impl WorkspaceAttachedWorkdirSession { @@ -163,6 +164,7 @@ impl WorkspaceAttachedWorkdirSession { client, workdir: Workdir::new("workspace-attachment"), expected_session_fence: None, + delegation: None, }) } @@ -181,6 +183,7 @@ impl WorkspaceAttachedWorkdirSession { ), serde_json::to_string(&WorkspaceWorkdirSessionOperationRequest { expected_session_fence: self.expected_session_fence.clone(), + delegation: self.delegation.clone(), operation, }) .map_err(|error| { @@ -231,7 +234,10 @@ impl WorkdirSession for WorkspaceAttachedWorkdirSession { WorkdirSessionCapabilities::ALL } - async fn capture_delegation_source(&self) -> Result { + async fn capture_delegation_source( + &self, + request: &workdir::WorkdirDelegationRequest, + ) -> Result { let expected_session_fence = if let Some(fence) = &self.expected_session_fence { fence.clone() } else { @@ -265,6 +271,7 @@ impl WorkdirSession for WorkspaceAttachedWorkdirSession { client: self.client.clone(), workdir: self.workdir.clone(), expected_session_fence: Some(expected_session_fence), + delegation: Some(request.clone()), })) } @@ -1067,11 +1074,11 @@ mod tests { let delegation = parent .delegate(workdir::WorkdirDelegationRequest { rules: vec![workdir::WorkdirDelegationRule { - target: workdir::WorkdirPath::new("visible.txt").unwrap(), + target: workdir::WorkdirPath::new("").unwrap(), permission: workdir::WorkdirDelegationPermission::Read, recursive: false, }], - cwd: workdir::WorkdirPath::new("visible.txt").unwrap(), + cwd: workdir::WorkdirPath::new("").unwrap(), }) .await .unwrap(); @@ -1093,6 +1100,7 @@ mod tests { serde_json::from_str(requests[1].body.as_deref().unwrap()).unwrap(); assert_eq!(body["expected_session_fence"], "attachment-fence"); assert_eq!(body["operation"]["operation"], "stat"); + assert_eq!(body["delegation"]["rules"][0]["target"], ""); } #[test] diff --git a/crates/worker/src/spawn/tool.rs b/crates/worker/src/spawn/tool.rs index 93ec63f2..eac2ff86 100644 --- a/crates/worker/src/spawn/tool.rs +++ b/crates/worker/src/spawn/tool.rs @@ -13,7 +13,7 @@ use async_trait::async_trait; use fs_operation::FsPath; use llm_engine::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; use manifest::{ - CompactionConfigPartial, EngineManifestConfig, FileUploadLimitsPartial, Permission, + CompactionConfigPartial, EngineManifestConfig, FileUploadLimitsPartial, PermissionConfigPartial, ProfileDiscovery, ProfileError, ProfileRegistry, ProfileRegistrySource, ProfileResolveOptions, ProfileResolver, ProfileSelector, ScopeConfig, ScopeRule, SessionConfigPartial, ToolOutputLimitsPartial, WorkerManifest, WorkerManifestConfig, @@ -53,11 +53,10 @@ struct SubWorkerSpawnInput { /// Exact catalog-root dotted Prompt name (for example `default` or `role.coder`). #[serde(default)] instruction: Option, - /// Child process/tool working directory. This is not the runtime workspace - /// root and grants no filesystem authority. When omitted, the spawned SubWorker - /// starts in the spawner's current working directory. + /// Logical Workdir-relative child tool working directory. This path is not + /// a host path and grants no authority. When omitted, the Workdir root is used. #[serde(default)] - cwd: Option, + cwd: Option, /// First message sent to the spawned SubWorker via `Method::Run`. task: String, /// Allow rules delegated to the spawned SubWorker. Must be a subset of the @@ -77,8 +76,9 @@ struct ReviewerHandoffInput { #[derive(Debug, Deserialize, schemars::JsonSchema)] struct ScopeRuleInput { - /// Absolute target path. Relative paths are rejected. - target: PathBuf, + /// Logical Workdir-relative target such as `.` or `src`. Absolute host + /// paths and parent traversal are rejected. + target: String, /// `"read"` or `"write"`. permission: PermissionInput, /// When `false`, the rule matches the target itself and its direct @@ -98,15 +98,6 @@ fn default_true() -> bool { true } -impl From for Permission { - fn from(p: PermissionInput) -> Self { - match p { - PermissionInput::Read => Permission::Read, - PermissionInput::Write => Permission::Write, - } - } -} - #[derive(Debug, Clone)] struct AvailableProfiles { registry: Option, @@ -274,7 +265,6 @@ pub struct SubWorkerSpawnTool { workspace_root: PathBuf, /// Directory the spawned SubWorker's tools should use when the LLM did not /// override it. Defaults to the spawner's cwd. - spawner_cwd: PathBuf, /// Active provider-backed Workdir session from which child leases are captured. source_workdir_session: Option, /// Parent-owned in-memory registry shared by the five SubWorker tools. @@ -302,7 +292,6 @@ impl SubWorkerSpawnTool { parent_notifications: ParentNotificationTarget, runtime_base: PathBuf, workspace_root: PathBuf, - spawner_cwd: PathBuf, source_workdir_session: Option, registry: Arc, spawner_manifest: WorkerManifest, @@ -315,7 +304,6 @@ impl SubWorkerSpawnTool { parent_notifications, runtime_base, workspace_root, - spawner_cwd, source_workdir_session, registry, spawner_manifest, @@ -378,11 +366,10 @@ impl Tool for SubWorkerSpawnTool { .reserve_internal_name(input.name.clone()) .map_err(|error| ToolError::InvalidArgument(error.to_string()))?; - let scope_allow = parse_scope(&input.scope)?; + let workdir_rules = parse_workdir_scope(&input.scope)?; let source_workdir_session = require_active_workdir_session(self.source_workdir_session.as_ref())?; - let delegation_request = - self.workdir_delegation_request(input.cwd.as_deref(), &scope_allow)?; + let delegation_request = workdir_delegation_request(input.cwd.as_deref(), workdir_rules)?; let workdir_delegation = source_workdir_session .delegate(delegation_request) .await @@ -397,6 +384,7 @@ impl Tool for SubWorkerSpawnTool { self.available_profiles.error_suffix() )) })?; + let scope_allow = Vec::new(); let spawn_config_json = self .build_spawn_config_json( &input.name, @@ -618,63 +606,57 @@ impl Tool for SubWorkerSpawnTool { } } -impl SubWorkerSpawnTool { - fn workdir_delegation_request( - &self, - cwd: Option<&Path>, - scope_allow: &[ScopeRule], - ) -> Result { - let rules = scope_allow - .iter() - .map(|rule| { - Ok(WorkdirDelegationRule { - target: self.logical_workdir_path(&rule.target)?, - permission: match rule.permission { - Permission::Read => WorkdirDelegationPermission::Read, - Permission::Write => WorkdirDelegationPermission::Write, - }, - recursive: rule.recursive, - }) - }) - .collect::, ToolError>>()?; - let cwd = cwd.unwrap_or(&self.spawner_cwd); - if !cwd.is_absolute() { - return Err(ToolError::InvalidArgument(format!( - "cwd must be absolute, got `{}`", - cwd.display() - ))); - } - Ok(WorkdirDelegationRequest { - rules, - cwd: self.logical_workdir_path(cwd)?, - }) +fn logical_workdir_path(value: &str, field: &str) -> Result { + let path = Path::new(value); + if path.is_absolute() { + return Err(ToolError::InvalidArgument(format!( + "{field} must be Workdir-relative, got `{value}`" + ))); } + let normalized = path + .components() + .filter_map(|component| match component { + std::path::Component::CurDir => None, + other => Some(other.as_os_str()), + }) + .collect::(); + let normalized = normalized.to_str().ok_or_else(|| { + ToolError::InvalidArgument(format!("{field} `{value}` is not valid UTF-8")) + })?; + FsPath::new(normalized).map_err(|error| { + ToolError::InvalidArgument(format!( + "{field} `{value}` is not a valid logical Workdir path: {error}" + )) + }) +} - fn logical_workdir_path(&self, path: &Path) -> Result { - let logical = if self.workspace_root == Path::new("/") { - path.strip_prefix(Path::new("/")) - } else { - path.strip_prefix(&self.workspace_root) - } - .map_err(|_| { - ToolError::InvalidArgument(format!( - "scope target `{}` is not a Workdir-owned logical path", - path.display() - )) - })?; - let logical = logical.to_str().ok_or_else(|| { - ToolError::InvalidArgument(format!( - "scope target `{}` is not valid UTF-8", - path.display() - )) - })?; - FsPath::new(logical).map_err(|error| { - ToolError::InvalidArgument(format!( - "scope target `{}` is not a valid logical Workdir path: {error}", - path.display() - )) - }) +fn parse_workdir_scope(rules: &[ScopeRuleInput]) -> Result, ToolError> { + if rules.is_empty() { + return Err(ToolError::InvalidArgument("scope must not be empty".into())); } + rules + .iter() + .map(|rule| { + Ok(WorkdirDelegationRule { + target: logical_workdir_path(&rule.target, "scope.target")?, + permission: match rule.permission { + PermissionInput::Read => WorkdirDelegationPermission::Read, + PermissionInput::Write => WorkdirDelegationPermission::Write, + }, + recursive: rule.recursive, + }) + }) + .collect() +} + +fn workdir_delegation_request( + cwd: Option<&str>, + rules: Vec, +) -> Result { + Ok(WorkdirDelegationRequest { + rules, + cwd: logical_workdir_path(cwd.unwrap_or("."), "cwd")?, + }) } fn require_active_workdir_session( @@ -688,28 +670,6 @@ fn require_active_workdir_session( }) } -fn parse_scope(rules: &[ScopeRuleInput]) -> Result, ToolError> { - if rules.is_empty() { - return Err(ToolError::InvalidArgument("scope must not be empty".into())); - } - rules - .iter() - .map(|r| { - if !r.target.is_absolute() { - return Err(ToolError::InvalidArgument(format!( - "scope.target must be absolute: {}", - r.target.display() - ))); - } - Ok(ScopeRule { - target: r.target.clone(), - permission: r.permission.into(), - recursive: r.recursive, - }) - }) - .collect() -} - /// Serialise the internal manifest config that gets handed to the child /// Worker runtime process via the hidden `--spawn-config-json` flag. /// `WorkerManifestConfig`'s `Serialize` impl is the single source of truth for the @@ -915,7 +875,6 @@ pub(crate) fn sub_worker_spawn_tool( parent_notifications: ParentNotificationTarget, runtime_base: PathBuf, workspace_root: PathBuf, - spawner_cwd: PathBuf, source_workdir_session: Option, registry: Arc, spawner_manifest: WorkerManifest, @@ -927,7 +886,6 @@ pub(crate) fn sub_worker_spawn_tool( parent_notifications, runtime_base, workspace_root, - spawner_cwd, source_workdir_session, registry, spawner_manifest, @@ -941,7 +899,6 @@ fn sub_worker_spawn_tool_impl( parent_notifications: ParentNotificationTarget, runtime_base: PathBuf, workspace_root: PathBuf, - spawner_cwd: PathBuf, source_workdir_session: Option, registry: Arc, spawner_manifest: WorkerManifest, @@ -973,7 +930,6 @@ fn sub_worker_spawn_tool_impl( parent_notifications.clone(), runtime_base.clone(), workspace_root.clone(), - spawner_cwd.clone(), source_workdir_session.clone(), registry.clone(), spawner_manifest.clone(), @@ -987,7 +943,7 @@ fn sub_worker_spawn_tool_impl( #[cfg(test)] mod tests { use super::*; - use manifest::{DelegationScope, Scope, SharedScope}; + use manifest::{DelegationScope, Permission, Scope, SharedScope}; use std::pin::Pin; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::time::Duration; @@ -1016,25 +972,53 @@ mod tests { )); } + #[test] + fn workdir_scope_uses_logical_relative_paths() { + let rules = parse_workdir_scope(&[ + ScopeRuleInput { + target: ".".to_string(), + permission: PermissionInput::Read, + recursive: true, + }, + ScopeRuleInput { + target: "src".to_string(), + permission: PermissionInput::Write, + recursive: false, + }, + ]) + .unwrap(); + assert_eq!(rules[0].target.as_str(), ""); + assert_eq!(rules[1].target.as_str(), "src"); + for target in ["/host/path", "../escape"] { + let error = parse_workdir_scope(&[ScopeRuleInput { + target: target.to_string(), + permission: PermissionInput::Read, + recursive: true, + }]) + .unwrap_err(); + assert!(matches!(error, ToolError::InvalidArgument(_))); + } + } + #[test] fn reviewer_handoff_requires_explicit_builtin_profile_and_read_only_scope() { let valid: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ "name":"reviewer","task":"review","profile":"builtin:reviewer", - "scope":[{"target":"/tmp/work","permission":"read"}], + "scope":[{"target":"work","permission":"read"}], "review":{"ticket_id":"T1"} })) .unwrap(); assert!(validate_reviewer_handoff(&valid).is_ok()); let wrong_profile: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ "name":"reviewer","task":"review","profile":"builtin:coder", - "scope":[{"target":"/tmp/work","permission":"read"}], + "scope":[{"target":"work","permission":"read"}], "review":{"ticket_id":"T1"} })) .unwrap(); assert!(validate_reviewer_handoff(&wrong_profile).is_err()); let writable: SubWorkerSpawnInput = serde_json::from_value(serde_json::json!({ "name":"reviewer","task":"review","profile":"builtin:reviewer", - "scope":[{"target":"/tmp/work","permission":"write"}], + "scope":[{"target":"work","permission":"write"}], "review":{"ticket_id":"T1"} })) .unwrap(); @@ -1128,7 +1112,6 @@ extract_threshold = 4000 ParentNotificationTarget::Controller(parent_method_tx.downgrade()), runtime.path().to_path_buf(), workspace_root.clone(), - workspace_root.clone(), Some(source_workdir_session), registry.clone(), manifest.clone(), @@ -1149,7 +1132,7 @@ extract_threshold = 4000 "instruction": "role.reviewer", "task": "review immutable commit", "scope": [{ - "target": workspace_root.clone(), + "target": ".", "permission": "read", "recursive": true }] diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 01c5a983..0db12f5f 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -5138,8 +5138,25 @@ async fn scoped_execute_current_worker_workdir_operation( &link, request.expected_session_fence.as_deref(), )?; - let session = open_current_worker_workdir_session_locked(&api, &worker, &link).await?; - let result = execute_workdir_session_operation(&session, request.operation) + let source = open_current_worker_workdir_session_locked(&api, &worker, &link).await?; + let delegation = if let Some(delegation) = request.delegation { + Some( + workdir::delegation_capable_session(source.clone()) + .delegate(delegation) + .await + .map_err(|error| Error::RuntimeOperationFailed { + runtime_id: worker.runtime_id.clone(), + code: "workdir_session_delegation_failed".to_string(), + message: error.to_string(), + })?, + ) + } else { + None + }; + let session = delegation + .as_ref() + .map_or(&source, |delegation| &delegation.scoped_session); + let result = execute_workdir_session_operation(session, request.operation) .await .map_err(|error| Error::RuntimeOperationFailed { runtime_id: worker.runtime_id.clone(),