From e448073b98f35800c901d14f71dbdd1b6c011050 Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 14 Sep 2026 20:05:27 +0900 Subject: [PATCH] fix: fence resolved Workdir lease aliases --- crates/workdir/src/http.rs | 18 +- crates/workdir/src/lib.rs | 14 +- crates/workdir/src/local.rs | 62 ++++- crates/workdir/src/scope.rs | 234 ++++++++++++------ crates/worker-runtime/src/http_server.rs | 31 +++ .../src/feature/builtin/manage_workdir.rs | 14 +- crates/workspace-server/src/server.rs | 5 + 7 files changed, 292 insertions(+), 86 deletions(-) diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index a12cc250..eb3620d1 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -11,7 +11,8 @@ use crate::{ CommandHandle, CommandOutput, CommandOutputRequest, CommandRequest, CommandStatus, EditRequest, EditResult, GlobRequest, GlobResult, GrepRequest, GrepResult, ListRequest, ListResult, ReadRequest, ReadResult, StatRequest, StatResult, WorkdirError, WorkdirId, - WorkdirScopeAuthorizationRequest, WorkdirSessionCapabilities, WriteRequest, WriteResult, + WorkdirScopeAuthorizationRequest, WorkdirScopeOverlapRequest, WorkdirSessionCapabilities, + WriteRequest, WriteResult, }; /// Opaque Runtime-owned identifier for one ephemeral Workdir session. @@ -56,6 +57,7 @@ pub struct OpenWorkdirSessionResponse { #[serde(tag = "operation", content = "request", rename_all = "snake_case")] pub enum WorkdirSessionOperation { AuthorizeScope(WorkdirScopeAuthorizationRequest), + ScopeRulesOverlap(WorkdirScopeOverlapRequest), Stat(StatRequest), Read(ReadRequest), Write(WriteRequest), @@ -81,6 +83,7 @@ pub struct WorkdirSessionOperationRequest { #[serde(tag = "operation", content = "result", rename_all = "snake_case")] pub enum WorkdirSessionOperationResult { AuthorizeScope, + ScopeRulesOverlap { overlaps: bool }, Stat(StatResult), Read(ReadResult), Write(WriteResult), @@ -462,6 +465,19 @@ mod client { } } + async fn scope_rules_overlap( + &self, + request: WorkdirScopeOverlapRequest, + ) -> Result { + match self + .operate(WorkdirSessionOperation::ScopeRulesOverlap(request)) + .await? + { + WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps } => Ok(overlaps), + _ => Err(Self::mismatch("scope_rules_overlap")), + } + } + async fn stat(&self, request: StatRequest) -> Result { match self.operate(WorkdirSessionOperation::Stat(request)).await? { WorkdirSessionOperationResult::Stat(result) => Ok(result), diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index 4abd3c1d..3e3ae0de 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -28,8 +28,9 @@ pub use local::{ }; pub use operation::*; pub use scope::{ - ReadOnlyWorkdirSession, WorkdirScopeAuthorizationRequest, WorkdirScopeLease, WorkdirToolBroker, - WorkdirToolScope, WorkdirToolScopePermission, WorkdirToolScopeRule, + ReadOnlyWorkdirSession, WorkdirScopeAuthorizationRequest, WorkdirScopeLease, + WorkdirScopeOverlapRequest, WorkdirToolBroker, WorkdirToolScope, WorkdirToolScopePermission, + WorkdirToolScopeRule, }; /// Persistent, opaque identity of one materialized Workdir. @@ -166,6 +167,15 @@ pub trait WorkdirSession: std::fmt::Debug + Send + Sync { } } + async fn scope_rules_overlap( + &self, + _request: WorkdirScopeOverlapRequest, + ) -> Result { + Err(WorkdirError::Denied( + "Workdir provider cannot compare resolved scope authority".to_string(), + )) + } + async fn stat(&self, request: StatRequest) -> Result; async fn read(&self, request: ReadRequest) -> Result; async fn write(&self, request: WriteRequest) -> Result; diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 4df7c5c8..6bbb0738 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -29,8 +29,9 @@ use crate::{ CommandSnapshot, CommandStatus, CommandStream, CommandStreamSlice, EditRequest, EditResult, GlobRequest, GlobResult, GrepRequest, GrepResult, ListRequest, ListResult, ReadRequest, ReadResult, StatRequest, StatResult, Workdir, WorkdirError, WorkdirPath, - WorkdirScopeAuthorizationRequest, WorkdirSession, WorkdirSessionCapabilities, - WorkdirSessionCapability, WorkdirToolScopePermission, WriteRequest, WriteResult, + WorkdirScopeAuthorizationRequest, WorkdirScopeOverlapRequest, WorkdirSession, + WorkdirSessionCapabilities, WorkdirSessionCapability, WorkdirToolScopePermission, WriteRequest, + WriteResult, }; #[cfg(test)] use crate::{EntryKind, WriteOutcome}; @@ -225,6 +226,41 @@ impl fs_operation::FsAccessPolicy for ScopeAccess { } } +fn path_sets_overlap( + left: &Path, + left_recursive: bool, + right: &Path, + right_recursive: bool, +) -> bool { + match (left_recursive, right_recursive) { + (true, true) => left.starts_with(right) || right.starts_with(left), + (true, false) => { + right.starts_with(left) + || left == right + || left.parent().is_some_and(|parent| parent == right) + } + (false, true) => { + left.starts_with(right) + || left == right + || right.parent().is_some_and(|parent| parent == left) + } + (false, false) => { + left == right + || left.parent().is_some_and(|parent| parent == right) + || right.parent().is_some_and(|parent| parent == left) + } + } +} + +fn rule_targets( + root: &Path, + rule: &crate::WorkdirToolScopeRule, +) -> std::io::Result<(PathBuf, PathBuf)> { + let logical = root.join(rule.target.as_str()); + let resolved = fs_operation::resolve_access_path(&logical)?; + Ok((logical, resolved)) +} + #[derive(Debug)] struct LocalWorkdirSessionInner { workdir: Workdir, @@ -626,6 +662,28 @@ impl WorkdirSession for LocalWorkdirSession { } } + async fn scope_rules_overlap( + &self, + request: WorkdirScopeOverlapRequest, + ) -> Result { + self.ensure_open()?; + let (left_logical, left_resolved) = rule_targets(&self.inner.root, &request.left) + .map_err(|error| WorkdirError::io(&self.inner.root, error))?; + let (right_logical, right_resolved) = rule_targets(&self.inner.root, &request.right) + .map_err(|error| WorkdirError::io(&self.inner.root, error))?; + Ok(path_sets_overlap( + &left_logical, + request.left.recursive, + &right_logical, + request.right.recursive, + ) || path_sets_overlap( + &left_resolved, + request.left.recursive, + &right_resolved, + request.right.recursive, + )) + } + async fn stat(&self, request: StatRequest) -> Result { self.ensure_capability(WorkdirSessionCapability::Read)?; let logical = request.path.clone(); diff --git a/crates/workdir/src/scope.rs b/crates/workdir/src/scope.rs index 0a659007..104c58ad 100644 --- a/crates/workdir/src/scope.rs +++ b/crates/workdir/src/scope.rs @@ -45,6 +45,14 @@ pub struct WorkdirScopeAuthorizationRequest { pub permission: WorkdirToolScopePermission, } +/// Provider-side overlap comparison that keeps resolved host paths private. +#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] +#[serde(deny_unknown_fields)] +pub struct WorkdirScopeOverlapRequest { + pub left: WorkdirToolScopeRule, + pub right: WorkdirToolScopeRule, +} + #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] #[serde(deny_unknown_fields)] pub struct WorkdirToolScope { @@ -82,6 +90,7 @@ impl WorkdirToolBroker { capabilities, validity: SessionValidity::root(), child_write_leases: Mutex::new(HashMap::new()), + scope_lock: tokio::sync::Mutex::new(()), next_lease_id: AtomicU64::new(1), close_lock: Arc::new(tokio::sync::Mutex::new(())), owned_commands: Arc::new(Mutex::new(HashSet::new())), @@ -322,6 +331,7 @@ struct ScopedWorkdirSession { capabilities: WorkdirSessionCapabilities, validity: Arc, child_write_leases: Mutex>, + scope_lock: tokio::sync::Mutex<()>, next_lease_id: AtomicU64, close_lock: Arc>, owned_commands: Arc>>, @@ -386,9 +396,6 @@ impl ScopedWorkdirSession { ))); } } - if permission == WorkdirToolScopePermission::Write { - self.ensure_parent_write_available(path)?; - } Ok(()) } @@ -475,33 +482,48 @@ impl ScopedWorkdirSession { }); } - fn ensure_parent_write_available(&self, path: &FsPath) -> Result<(), WorkdirError> { - let mut leases = self - .child_write_leases - .lock() - .expect("Workdir tool scope lease mutex poisoned"); - leases.retain(|_, lease| { - lease - .validity - .upgrade() - .is_some_and(|validity| validity.is_active()) - || lease - .cleanup_pending + async fn ensure_parent_write_available(&self, path: &FsPath) -> Result<(), WorkdirError> { + let active_write_rules = { + let mut leases = self + .child_write_leases + .lock() + .expect("Workdir tool scope lease mutex poisoned"); + leases.retain(|_, lease| { + lease + .validity .upgrade() - .is_some_and(|pending| pending.load(Ordering::Acquire)) - }); - if leases.values().any(|lease| { - lease.rules.iter().any(|rule| { - rule.permission == WorkdirToolScopePermission::Write - && rule_allows_path(rule, path, WorkdirToolScopePermission::Write) - }) - }) { - Err(WorkdirError::Denied(format!( - "logical workdir path `{path}` is leased to child Workdir tools" - ))) - } else { - Ok(()) + .is_some_and(|validity| validity.is_active()) + || lease + .cleanup_pending + .upgrade() + .is_some_and(|pending| pending.load(Ordering::Acquire)) + }); + leases + .values() + .flat_map(|lease| lease.rules.iter().cloned()) + .collect::>() + }; + let requested = WorkdirToolScopeRule { + target: path.clone(), + permission: WorkdirToolScopePermission::Write, + recursive: false, + symlink_policy: SymlinkPolicy::Resolved, + }; + for active in active_write_rules { + if self + .source + .scope_rules_overlap(WorkdirScopeOverlapRequest { + left: active, + right: requested.clone(), + }) + .await? + { + return Err(WorkdirError::Denied(format!( + "path `{path}` is leased to child Workdir tools" + ))); + } } + Ok(()) } async fn ensure_scope_targets_are_authorized( @@ -527,6 +549,9 @@ impl ScopedWorkdirSession { ) -> Result { self.ensure_active()?; let resolved = self.resolve_path(path)?; + if permission == WorkdirToolScopePermission::Write { + self.ensure_parent_write_available(&resolved).await?; + } if let Some(rules) = self.scope.as_ref() { self.source .authorize_scope_path(WorkdirScopeAuthorizationRequest { @@ -613,6 +638,7 @@ impl ScopedWorkdirSession { self: &Arc, request: WorkdirToolScope, ) -> Result { + let _scope_guard = self.scope_lock.lock().await; let capabilities = self.validate_scope(&request.rules, request.command)?; if !request .rules @@ -629,50 +655,61 @@ impl ScopedWorkdirSession { let validity = SessionValidity::child(self.validity.clone()); let cleanup_pending = Arc::new(AtomicBool::new(true)); let id = self.next_lease_id.fetch_add(1, Ordering::Relaxed); - if request + let write_rules = request .rules .iter() - .any(|rule| rule.permission == WorkdirToolScopePermission::Write) - { - let mut leases = self - .child_write_leases - .lock() - .expect("Workdir tool scope lease mutex poisoned"); - leases.retain(|_, lease| { - lease - .validity - .upgrade() - .is_some_and(|validity| validity.is_active()) - || lease - .cleanup_pending - .upgrade() - .is_some_and(|pending| pending.load(Ordering::Acquire)) - }); - let requested_write_rules = request - .rules - .iter() - .filter(|rule| rule.permission == WorkdirToolScopePermission::Write); - for requested in requested_write_rules { - if leases.values().any(|lease| { + .filter(|rule| rule.permission == WorkdirToolScopePermission::Write) + .cloned() + .collect::>(); + if !write_rules.is_empty() { + let active_write_rules = { + let mut leases = self + .child_write_leases + .lock() + .expect("Workdir tool scope lease mutex poisoned"); + leases.retain(|_, lease| { lease - .rules - .iter() - .any(|active| rules_overlap(active, requested)) - }) { - return Err(WorkdirError::Denied(format!( - "scoped write path `{}` overlaps an active child scope", - requested.target - ))); + .validity + .upgrade() + .is_some_and(|validity| validity.is_active()) + || lease + .cleanup_pending + .upgrade() + .is_some_and(|pending| pending.load(Ordering::Acquire)) + }); + leases + .values() + .flat_map(|lease| lease.rules.iter().cloned()) + .collect::>() + }; + for requested in &write_rules { + for active in &active_write_rules { + if self + .source + .scope_rules_overlap(WorkdirScopeOverlapRequest { + left: active.clone(), + right: requested.clone(), + }) + .await? + { + return Err(WorkdirError::Denied(format!( + "scoped write path `{}` overlaps an active child scope after provider resolution", + requested.target + ))); + } } } - leases.insert( - id, - ActiveWriteLease { - validity: Arc::downgrade(&validity), - cleanup_pending: Arc::downgrade(&cleanup_pending), - rules: request.rules.clone(), - }, - ); + self.child_write_leases + .lock() + .expect("Workdir tool scope lease mutex poisoned") + .insert( + id, + ActiveWriteLease { + validity: Arc::downgrade(&validity), + cleanup_pending: Arc::downgrade(&cleanup_pending), + rules: request.rules.clone(), + }, + ); } let owned_commands = Arc::new(Mutex::new(HashSet::new())); let pending_command_events = Arc::new(Mutex::new(HashMap::new())); @@ -698,6 +735,7 @@ impl ScopedWorkdirSession { capabilities, validity: validity.clone(), child_write_leases: Mutex::new(HashMap::new()), + scope_lock: tokio::sync::Mutex::new(()), next_lease_id: AtomicU64::new(1), close_lock: close_lock.clone(), owned_commands, @@ -1011,6 +1049,13 @@ impl WorkdirSession for ReadOnlyWorkdirSession { self.inner.authorize_scope_path(request).await } + async fn scope_rules_overlap( + &self, + request: WorkdirScopeOverlapRequest, + ) -> Result { + self.inner.scope_rules_overlap(request).await + } + async fn stat(&self, request: StatRequest) -> Result { self.inner.stat(request).await } @@ -1167,13 +1212,6 @@ fn unix_timestamp_ms() -> u64 { .min(u128::from(u64::MAX)) as u64 } -fn rules_overlap(left: &WorkdirToolScopeRule, right: &WorkdirToolScopeRule) -> bool { - left.permission == WorkdirToolScopePermission::Write - && right.permission == WorkdirToolScopePermission::Write - && (rule_allows_path(left, &right.target, WorkdirToolScopePermission::Write) - || rule_allows_path(right, &left.target, WorkdirToolScopePermission::Write)) -} - pub(crate) fn rule_allows_path( rule: &WorkdirToolScopeRule, path: &FsPath, @@ -1578,6 +1616,41 @@ mod tests { )); } + #[cfg(unix)] + #[tokio::test] + async fn sibling_write_scopes_reject_distinct_aliases_to_same_resolved_target() { + use std::os::unix::fs::symlink; + + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("target")).unwrap(); + symlink("target", root.path().join("alias-a")).unwrap(); + symlink("target", root.path().join("alias-b")).unwrap(); + let parent = session(root.path()); + let _first = parent + .scope(request("alias-a", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + + assert!(matches!( + parent + .scope(request("alias-b", WorkdirToolScopePermission::Write)) + .await, + Err(WorkdirError::Denied(message)) + if message.contains("overlaps an active child scope after provider resolution") + )); + assert!(matches!( + parent + .write(WriteRequest { + path: FsPath::new("target/from-parent").unwrap(), + content: b"blocked".to_vec(), + expected_hash: None, + }) + .await, + Err(WorkdirError::Denied(message)) + if message.contains("leased to child Workdir tools") + )); + } + #[tokio::test] async fn nested_scope_cannot_expand_resolved_policy_to_logical() { let root = TempDir::new().unwrap(); @@ -1641,7 +1714,7 @@ mod tests { #[cfg(unix)] #[tokio::test] - async fn write_delegation_leases_the_logical_symlink_path() { + async fn write_delegation_leases_logical_alias_and_resolved_target() { use std::os::unix::fs::symlink; let root = TempDir::new().unwrap(); @@ -1657,10 +1730,13 @@ mod tests { .write(write("from-child", "child-authoritative")) .await .unwrap(); - parent - .write(write("secret/parent", "still-authoritative")) - .await - .unwrap(); + assert!(matches!( + parent + .write(write("secret/parent", "must-be-blocked")) + .await, + Err(WorkdirError::Denied(message)) + if message.contains("leased to child Workdir tools") + )); assert_eq!( fs::read_to_string(root.path().join("secret/from-child")).unwrap(), "child-authoritative" diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index 298deb72..191ac9b3 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -1016,6 +1016,10 @@ async fn run_workdir_session_operation( session.authorize_scope_path(request).await?; WorkdirSessionOperationResult::AuthorizeScope } + WorkdirSessionOperation::ScopeRulesOverlap(request) => { + let overlaps = session.scope_rules_overlap(request).await?; + WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps } + } WorkdirSessionOperation::Stat(request) => { WorkdirSessionOperationResult::Stat(session.stat(request).await?) } @@ -3014,6 +3018,33 @@ mod tests { WorkdirSessionOperationResult::AuthorizeScope )); + let overlap_rule = workdir::WorkdirToolScopeRule { + target: WorkdirPath::new("hello.txt").unwrap(), + permission: workdir::WorkdirToolScopePermission::Write, + recursive: false, + symlink_policy: Default::default(), + }; + let overlap = WorkdirSessionOperationRequest { + operation: WorkdirSessionOperation::ScopeRulesOverlap( + workdir::WorkdirScopeOverlapRequest { + left: overlap_rule.clone(), + right: overlap_rule, + }, + ), + }; + let Json(result) = run_workdir_session_operation( + State(state.clone()), + Path("session-1".to_string()), + Some(Extension(auth.clone())), + Ok(Json(overlap)), + ) + .await + .expect("provider-side resolved overlap check"); + assert!(matches!( + result, + WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps: true } + )); + let grep = WorkdirSessionOperationRequest { operation: WorkdirSessionOperation::Grep(GrepRequest { pattern: "hello".into(), diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index 4302ab76..33a93de8 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -19,8 +19,8 @@ use workdir::{ CommandHandle, CommandOutput, CommandOutputRequest, CommandRequest, CommandStatus, EditRequest, EditResult, GlobRequest, GlobResult, GrepRequest, GrepResult, ListRequest, ListResult, ReadRequest, ReadResult, StatRequest, StatResult, Workdir, WorkdirError, - WorkdirScopeAuthorizationRequest, WorkdirSession, WorkdirSessionCapabilities, - WorkdirSessionHandle, WriteRequest, WriteResult, + WorkdirScopeAuthorizationRequest, WorkdirScopeOverlapRequest, WorkdirSession, + WorkdirSessionCapabilities, WorkdirSessionHandle, WriteRequest, WriteResult, }; use workspace_api::{ @@ -294,6 +294,16 @@ impl WorkdirSession for WorkspaceAttachedWorkdirSession { } } + async fn scope_rules_overlap( + &self, + request: WorkdirScopeOverlapRequest, + ) -> Result { + match self.operate(WorkdirSessionOperation::ScopeRulesOverlap(request))? { + WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps } => Ok(overlaps), + _ => Err(Self::mismatch("scope_rules_overlap")), + } + } + async fn stat(&self, request: StatRequest) -> Result { match self.operate(WorkdirSessionOperation::Stat(request))? { WorkdirSessionOperationResult::Stat(result) => Ok(result), diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 5a3de4d5..2b266417 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -8434,6 +8434,7 @@ async fn scoped_execute_current_worker_workdir_operation( .map_err(|error| current_worker_workdir_operation_error(&worker, error))? } operation @ (WorkdirSessionOperation::AuthorizeScope(_) + | WorkdirSessionOperation::ScopeRulesOverlap(_) | WorkdirSessionOperation::Stat(_) | WorkdirSessionOperation::Read(_) | WorkdirSessionOperation::Write(_) @@ -8489,6 +8490,10 @@ async fn execute_workdir_session_operation( .authorize_scope_path(request) .await .map(|()| WorkdirSessionOperationResult::AuthorizeScope), + WorkdirSessionOperation::ScopeRulesOverlap(request) => session + .scope_rules_overlap(request) + .await + .map(|overlaps| WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps }), WorkdirSessionOperation::Stat(request) => session .stat(request) .await