diff --git a/crates/fs-operation/src/glob.rs b/crates/fs-operation/src/glob.rs index c4f3ed65..a3d2ba4f 100644 --- a/crates/fs-operation/src/glob.rs +++ b/crates/fs-operation/src/glob.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use globset::Glob; use ignore::WalkBuilder; -use crate::{FsAccessPolicy, FsError, FsPath, GlobRequest, GlobResult, direct_symlink}; +use crate::{FsAccessPolicy, FsError, FsPath, GlobRequest, GlobResult}; /// Execute a bounded glob entirely inside the provider process. pub fn run_glob( @@ -18,21 +18,13 @@ pub fn run_glob( if !access.is_readable(base) { return Err(FsError::OutOfScope(PathBuf::from(request.path.as_str()))); } - if let Some(info) = direct_symlink(base) - && info.target_exists - && info.resolved_path.is_dir() - { - return Err(FsError::SymlinkDirectoryNotTraversed { - tool: "Glob", - path: PathBuf::from(request.path.as_str()), - target: PathBuf::from(""), - }); - } let matcher = Glob::new(&request.pattern) .map_err(|error| FsError::InvalidGlob(error.to_string()))? .compile_matcher(); let mut matches = Vec::new(); - for entry in WalkBuilder::new(base).hidden(false).build().flatten() { + let mut walker = WalkBuilder::new(base); + walker.hidden(false).follow_links(false); + for entry in walker.build().flatten() { let path = entry.path(); if !path.is_file() || !access.is_readable(path) { continue; diff --git a/crates/fs-operation/src/lib.rs b/crates/fs-operation/src/lib.rs index 1488d505..19800ba7 100644 --- a/crates/fs-operation/src/lib.rs +++ b/crates/fs-operation/src/lib.rs @@ -477,13 +477,14 @@ mod tests { #[cfg(unix)] #[test] - fn grep_keeps_direct_symlink_directory_and_broken_path_guards() { + fn grep_traverses_a_direct_symlink_directory_and_rejects_a_broken_path() { use std::os::unix::fs::symlink; let temp = tempfile::tempdir().unwrap(); let root = temp.path().canonicalize().unwrap(); let readable = RootAccess(root.clone()); std::fs::create_dir(root.join("target-dir")).unwrap(); + std::fs::write(root.join("target-dir/nested.rs"), "needle nested\n").unwrap(); std::fs::write(root.join("target-file.rs"), "needle file\n").unwrap(); symlink(root.join("target-file.rs"), root.join("file-link.rs")).unwrap(); symlink(root.join("target-dir"), root.join("directory-link")).unwrap(); @@ -501,18 +502,35 @@ mod tests { assert_eq!(file_result.match_count, 1); assert!(file_result.output.starts_with("file-link.rs\n")); - let directory_error = run_grep( + let directory_result = run_grep( &root, root.join("directory-link"), request("directory-link"), &readable, ) - .unwrap_err(); - assert!(matches!( - directory_error, - FsError::SymlinkDirectoryNotTraversed { tool: "Grep", path, .. } - if path == root.join("directory-link") - )); + .unwrap(); + assert_eq!(directory_result.match_count, 1); + assert!( + directory_result + .output + .starts_with("directory-link/nested.rs\n") + ); + + let glob_result = run_glob( + &root, + &root.join("directory-link"), + GlobRequest { + pattern: "**/*.rs".to_string(), + path: FsPath::new("directory-link").unwrap(), + limit: 10, + }, + &readable, + ) + .unwrap(); + assert_eq!( + glob_result.paths, + vec![FsPath::new("directory-link/nested.rs").unwrap()] + ); let broken_error = run_grep( &root, diff --git a/crates/fs-operation/src/local.rs b/crates/fs-operation/src/local.rs index b554d8bb..4f91c9f3 100644 --- a/crates/fs-operation/src/local.rs +++ b/crates/fs-operation/src/local.rs @@ -45,7 +45,7 @@ pub fn run_read( ) -> Result { let logical = request.path; let path = resolve(root, &logical)?; - let path = require_access(&path, &logical, access, false)?; + let path = require_access(&path, &logical, access, false, false)?; let metadata = fs::metadata(&path).map_err(|error| map_io(&logical, error))?; if metadata.is_dir() { return Err(FsError::IsDirectory(PathBuf::from(logical.as_str()))); @@ -99,7 +99,7 @@ pub fn run_write( let path = resolve(root, &logical)?; let created = !path.exists(); if path.exists() { - let target = require_access(&path, &logical, access, true)?; + let target = require_access(&path, &logical, access, true, false)?; let metadata = fs::metadata(&target).map_err(|error| map_io(&logical, error))?; if metadata.is_dir() { return Err(FsError::IsDirectory(PathBuf::from(logical.as_str()))); @@ -117,7 +117,7 @@ pub fn run_write( FsError::InvalidArgument(format!("{} has no parent", logical.as_str())) })?; let parent_logical = logical_parent(&logical); - require_access(parent, &parent_logical, access, true)?; + require_access(parent, &parent_logical, access, true, true)?; atomic_write(&path, &request.content, &logical)?; } Ok(WriteResult { @@ -133,7 +133,7 @@ pub fn run_edit( ) -> Result { let logical = request.path; let path = resolve(root, &logical)?; - let target = require_access(&path, &logical, access, true)?; + let target = require_access(&path, &logical, access, true, false)?; let bytes = fs::read(&target).map_err(|error| map_io(&logical, error))?; let actual_hash = hash_bytes(&bytes); if actual_hash != request.expected_hash { @@ -173,7 +173,7 @@ pub fn run_list( ) -> Result { let logical = request.path; let path = resolve(root, &logical)?; - let path = require_access(&path, &logical, access, false)?; + let path = require_access(&path, &logical, access, false, true)?; let metadata = fs::metadata(&path).map_err(|error| map_io(&logical, error))?; if !metadata.is_dir() { return Err(FsError::NotDirectory(PathBuf::from(logical.as_str()))); @@ -247,6 +247,7 @@ fn require_access( logical: &FsPath, access: &dyn FsAccessPolicy, write: bool, + allow_symlink_directory: bool, ) -> Result { if let Some(info) = direct_symlink(path) { if !info.target_exists { @@ -257,9 +258,9 @@ fn require_access( }); } let allowed = if write { - access.is_writable(&info.resolved_path) + access.is_writable(path) } else { - access.is_readable(&info.resolved_path) + access.is_readable(path) }; if !allowed { return Err(FsError::SymlinkOutOfScope { @@ -268,7 +269,7 @@ fn require_access( required_permission: if write { "write" } else { "read" }, }); } - if write && info.resolved_path.is_dir() { + if !allow_symlink_directory && info.resolved_path.is_dir() { return Err(FsError::SymlinkTargetIsDirectory { path: PathBuf::from(logical.as_str()), target: PathBuf::from(""), diff --git a/crates/fs-operation/src/search.rs b/crates/fs-operation/src/search.rs index a6d9d336..2f219721 100644 --- a/crates/fs-operation/src/search.rs +++ b/crates/fs-operation/src/search.rs @@ -259,16 +259,6 @@ pub fn run_grep( base.display() ))); } - if base_meta.is_dir() - && let Some(info) = symlink.as_ref() - { - return Err(FsError::SymlinkDirectoryNotTraversed { - tool: "Grep", - path: base.clone(), - target: info.resolved_path.clone(), - }); - } - let filter_base = if base_meta.is_file() { root } else { &base }; let types = build_types(p.file_type.as_deref())?; let overrides = build_overrides(filter_base, p.glob.as_deref())?; diff --git a/crates/manifest/src/scope.rs b/crates/manifest/src/scope.rs index 21fab0d4..ad6e5def 100644 --- a/crates/manifest/src/scope.rs +++ b/crates/manifest/src/scope.rs @@ -3,11 +3,11 @@ //! Built from [`crate::ScopeConfig`] via [`Scope::from_config`]. Every //! rule `target` must already be an absolute path — per-layer path //! resolution runs earlier, inside [`crate::WorkerManifestConfig::resolve_paths`]. -//! All rule `target` paths inside the [`Scope`] are canonicalised (where -//! possible) so access checks are pure path comparisons. +//! All rule `target` paths inside the [`Scope`] are normalized lexically so +//! access authority follows the path presented through the Workdir, not a +//! symbolic-link target outside that logical tree. -use std::ffi::OsString; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::sync::{Arc, Mutex}; use arc_swap::{ArcSwap, Guard}; @@ -26,7 +26,7 @@ pub struct Scope { #[derive(Debug, Clone, PartialEq, Eq)] struct ResolvedRule { - /// Absolute, canonicalized-or-normalized target directory/file. + /// Absolute, lexically normalized target directory/file. target: PathBuf, permission: Permission, recursive: bool, @@ -201,9 +201,14 @@ impl Scope { } /// Convenience constructor for tests and simple setups: a single - /// recursive `allow(Write)` rule rooted at `root`. + /// recursive `allow(Write)` rule rooted at the lexical path `root`. pub fn writable(root: impl AsRef) -> std::io::Result { - let root = root.as_ref().canonicalize()?; + let root = normalize_path(root.as_ref()).ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "scope root must be an absolute path without root traversal", + ) + })?; Ok(Self { allow: vec![ResolvedRule { target: root, @@ -214,8 +219,7 @@ impl Scope { }) } - /// Resolve one rule target with the same symlink and missing-tail semantics - /// used by scope matching. + /// Return one rule's lexically normalized target without resolving symlinks. pub fn resolved_target(rule: &ScopeRule) -> Result { Ok(resolve_rule(rule)?.target) } @@ -244,7 +248,7 @@ impl Scope { /// Returns `None` when `path` is outside every allow rule, or when /// deny rules have knocked it below `Read`. pub fn permission_at(&self, path: &Path) -> Option { - let resolved = resolve_path(path)?; + let resolved = normalize_path(path)?; let mut effective: Option = None; for rule in &self.allow { if rule.matches(&resolved) { @@ -523,7 +527,7 @@ fn resolve_rule(rule: &ScopeRule) -> Result { if !rule.target.is_absolute() { return Err(ScopeError::RelativeTarget(rule.target.clone())); } - let target = resolve_path(&rule.target).ok_or_else(|| ScopeError::ResolveTarget { + let target = normalize_path(&rule.target).ok_or_else(|| ScopeError::ResolveTarget { path: rule.target.clone(), source: std::io::Error::new(std::io::ErrorKind::Other, "could not absolutize target"), })?; @@ -534,37 +538,27 @@ fn resolve_rule(rule: &ScopeRule) -> Result { }) } -/// Convert `path` to an absolute form suitable for prefix comparison. -/// -/// Tries `canonicalize` on the full path first (resolves symlinks). If -/// the path doesn't exist yet, climbs to the closest existing ancestor, -/// canonicalizes it, then rejoins the missing tail. Returns `None` for -/// relative inputs that have no existing ancestor to anchor against. -fn resolve_path(path: &Path) -> Option { +/// Normalize an absolute path for lexical scope comparison without consulting +/// filesystem metadata or resolving symbolic links. +fn normalize_path(path: &Path) -> Option { if !path.is_absolute() { return None; } - if let Ok(canonical) = path.canonicalize() { - return Some(canonical); - } - let mut tail: Vec = Vec::new(); - let mut cur = path.to_path_buf(); - loop { - if let Ok(canonical) = cur.canonicalize() { - let mut out = canonical; - for segment in tail.iter().rev() { - out.push(segment); + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::Prefix(prefix) => normalized.push(prefix.as_os_str()), + Component::RootDir => normalized.push(component.as_os_str()), + Component::CurDir => {} + Component::ParentDir => { + if !normalized.pop() { + return None; + } } - return Some(out); + Component::Normal(part) => normalized.push(part), } - let name = cur.file_name()?.to_os_string(); - tail.push(name); - let parent = cur.parent()?.to_path_buf(); - if parent == cur { - return None; - } - cur = parent; } + normalized.is_absolute().then_some(normalized) } #[cfg(test)] @@ -805,6 +799,23 @@ mod tests { assert!(!scope.is_readable(&traversal)); } + #[cfg(unix)] + #[test] + fn scope_authorizes_symlink_paths_lexically_without_authorizing_targets() { + use std::os::unix::fs::symlink; + + let dir = TempDir::new().unwrap(); + let outside = TempDir::new().unwrap(); + std::fs::write(outside.path().join("outside.txt"), "visible through link").unwrap(); + symlink(outside.path(), dir.path().join("external")).unwrap(); + let scope = Scope::writable(dir.path()).unwrap(); + + assert!(scope.is_readable(&dir.path().join("external/outside.txt"))); + assert!(scope.is_writable(&dir.path().join("external/new.txt"))); + assert!(!scope.is_readable(&outside.path().join("outside.txt"))); + assert!(!scope.is_writable(&outside.path().join("new.txt"))); + } + #[test] fn summary_lists_readable_and_writable() { let dir = TempDir::new().unwrap(); diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 94efeceb..06ad09ff 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -1635,7 +1635,7 @@ mod tests { #[cfg(unix)] #[test] - fn read_bytes_reports_symlink_target_outside_scope() { + fn read_bytes_allows_logical_symlink_path_with_target_outside_scope() { use std::os::unix::fs::symlink; let dir = TempDir::new().unwrap(); @@ -1646,15 +1646,7 @@ mod tests { symlink(&target, &link).unwrap(); let fs = make_fs(&dir); - let err = fs.read_bytes(&link).unwrap_err(); - assert!( - matches!( - err, - WorkdirError::SymlinkOutOfScope { ref path, target: ref err_target, required_permission: "read" } - if path == &link && err_target == &target.canonicalize().unwrap() - ), - "expected symlink out-of-scope diagnostic, got {err:?}" - ); + assert_eq!(fs.read_bytes(&link).unwrap(), b"secret"); } #[cfg(unix)] @@ -1746,7 +1738,7 @@ mod tests { #[cfg(unix)] #[test] - fn write_reports_symlink_target_outside_scope() { + fn write_allows_logical_symlink_path_with_target_outside_scope() { use std::os::unix::fs::symlink; let dir = TempDir::new().unwrap(); @@ -1757,14 +1749,13 @@ mod tests { symlink(&target, &link).unwrap(); let fs = make_fs(&dir); - let err = fs.write(&link, b"new").unwrap_err(); + fs.write(&link, b"new").unwrap(); + assert_eq!(fs::read(&target).unwrap(), b"new"); assert!( - matches!( - err, - WorkdirError::SymlinkOutOfScope { ref path, target: ref err_target, required_permission: "write" } - if path == &link && err_target == &target.canonicalize().unwrap() - ), - "expected write symlink out-of-scope diagnostic, got {err:?}" + fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink() ); } @@ -1942,6 +1933,70 @@ mod tests { )); } + #[cfg(unix)] + #[tokio::test] + async fn provider_uses_logical_paths_through_symlinked_directories() { + use std::os::unix::fs::symlink; + + let dir = TempDir::new().unwrap(); + let outside = TempDir::new().unwrap(); + std::fs::write(outside.path().join("worker.json"), "scope-needle\n").unwrap(); + symlink(outside.path(), dir.path().join("yoi.local")).unwrap(); + let workdir = make_fs(&dir); + + let read = WorkdirSession::read( + &workdir, + ReadRequest { + path: WorkdirPath::new("yoi.local/worker.json").unwrap(), + offset: 0, + limit: 100, + max_bytes: 1024, + }, + ) + .await + .unwrap(); + assert_eq!(read.bytes, b"scope-needle\n"); + let glob = WorkdirSession::glob( + &workdir, + GlobRequest { + pattern: "**/*.json".into(), + path: WorkdirPath::new("yoi.local").unwrap(), + limit: 10, + }, + ) + .await + .unwrap(); + assert_eq!( + glob.paths, + [WorkdirPath::new("yoi.local/worker.json").unwrap()] + ); + let grep = WorkdirSession::grep( + &workdir, + GrepRequest { + pattern: "scope-needle".into(), + path: WorkdirPath::new("yoi.local").unwrap(), + glob: Some("*.json".into()), + file_type: None, + case_insensitive: false, + before_context: 0, + after_context: 0, + multiline: false, + output_mode: crate::GrepOutputMode::Content, + limit: 10, + offset: 0, + }, + ) + .await + .unwrap(); + assert_eq!(grep.match_count, 1); + assert!(grep.output.contains("yoi.local/worker.json")); + assert!( + !workdir + .scope() + .is_readable(&outside.path().join("worker.json")) + ); + } + #[tokio::test] async fn provider_executes_glob_grep_and_command_at_the_materialization() { let dir = TempDir::new().unwrap(); diff --git a/crates/workdir/src/scope.rs b/crates/workdir/src/scope.rs index 131ae6cf..3137ddba 100644 --- a/crates/workdir/src/scope.rs +++ b/crates/workdir/src/scope.rs @@ -492,51 +492,9 @@ impl ScopedWorkdirSession { } } - async fn ensure_source_path_has_no_symlink(&self, path: &FsPath) -> Result<(), WorkdirError> { - let mut current = String::new(); - for component in Path::new(path.as_str()).components() { - let component = component.as_os_str().to_string_lossy(); - if component.is_empty() || component == "." { - continue; - } - if !current.is_empty() { - current.push('/'); - } - current.push_str(&component); - let current = FsPath::new(¤t).map_err(|error| { - WorkdirError::Denied(format!("invalid scoped Workdir path: {error}")) - })?; - match self.source.stat(StatRequest { path: current }).await { - Ok(result) if result.kind == fs_operation::EntryKind::Symlink => { - return Err(WorkdirError::Denied(format!( - "scoped Workdir path `{path}` traverses a symlink" - ))); - } - Ok(_) => {} - Err(WorkdirError::NotFound(_)) => break, - Err(error) => return Err(error), - } - } - Ok(()) - } - - async fn ensure_scope_targets_do_not_traverse_symlinks( - &self, - rules: &[WorkdirToolScopeRule], - ) -> Result<(), WorkdirError> { - for rule in rules { - self.ensure_source_path_has_no_symlink(&rule.target).await?; - } - Ok(()) - } - - async fn resolve_operation_path(&self, path: &FsPath) -> Result { + fn resolve_operation_path(&self, path: &FsPath) -> Result { self.ensure_active()?; - let resolved = self.resolve_path(path)?; - if self.scope.is_some() { - self.ensure_source_path_has_no_symlink(&resolved).await?; - } - Ok(resolved) + self.resolve_path(path) } fn validate_scope( @@ -624,8 +582,6 @@ impl ScopedWorkdirSession { request.cwd ))); } - self.ensure_scope_targets_do_not_traverse_symlinks(&request.rules) - .await?; 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); @@ -736,49 +692,49 @@ impl WorkdirSession for ScopedWorkdirSession { } async fn stat(&self, mut request: StatRequest) -> Result { - let path = self.resolve_operation_path(&request.path).await?; + let path = self.resolve_operation_path(&request.path)?; self.ensure_read(&path, WorkdirSessionCapability::Read)?; request.path = path; self.source.stat(request).await } async fn read(&self, mut request: ReadRequest) -> Result { - let path = self.resolve_operation_path(&request.path).await?; + let path = self.resolve_operation_path(&request.path)?; self.ensure_read(&path, WorkdirSessionCapability::Read)?; request.path = path; self.source.read(request).await } async fn write(&self, mut request: WriteRequest) -> Result { - let path = self.resolve_operation_path(&request.path).await?; + let path = self.resolve_operation_path(&request.path)?; self.ensure_write(&path, WorkdirSessionCapability::Write)?; request.path = path; self.source.write(request).await } async fn edit(&self, mut request: EditRequest) -> Result { - let path = self.resolve_operation_path(&request.path).await?; + let path = self.resolve_operation_path(&request.path)?; self.ensure_write(&path, WorkdirSessionCapability::Edit)?; request.path = path; self.source.edit(request).await } async fn list(&self, mut request: ListRequest) -> Result { - let path = self.resolve_operation_path(&request.path).await?; + let path = self.resolve_operation_path(&request.path)?; self.ensure_read(&path, WorkdirSessionCapability::Read)?; request.path = path; self.source.list(request).await } async fn glob(&self, mut request: GlobRequest) -> Result { - let path = self.resolve_operation_path(&request.path).await?; + let path = self.resolve_operation_path(&request.path)?; self.ensure_read(&path, WorkdirSessionCapability::Glob)?; request.path = path; self.source.glob(request).await } async fn grep(&self, mut request: GrepRequest) -> Result { - let path = self.resolve_operation_path(&request.path).await?; + let path = self.resolve_operation_path(&request.path)?; self.ensure_read(&path, WorkdirSessionCapability::Grep)?; request.path = path; self.source.grep(request).await @@ -1487,7 +1443,7 @@ mod tests { #[cfg(unix)] #[tokio::test] - async fn provider_scope_denies_read_through_symlink_outside_grant() { + async fn provider_scope_allows_read_through_its_logical_symlink_path() { use std::os::unix::fs::symlink; let root = TempDir::new().unwrap(); @@ -1501,16 +1457,12 @@ mod tests { .await .unwrap(); - let result = child.read(read("link")).await; - assert!( - result.is_err(), - "symlink read escaped provider scope: {result:?}" - ); + assert_eq!(child.read(read("link")).await.unwrap().bytes, b"hidden"); } #[cfg(unix)] #[tokio::test] - async fn provider_scope_denies_write_through_symlink_outside_grant() { + async fn provider_scope_allows_write_through_its_logical_symlink_path() { use std::os::unix::fs::symlink; let root = TempDir::new().unwrap(); @@ -1523,17 +1475,19 @@ mod tests { .await .unwrap(); - let result = child.write(write("outside/new", "forbidden")).await; - assert!( - result.is_err(), - "symlink write escaped provider scope: {result:?}" + child + .write(write("outside/new", "through-logical-path")) + .await + .unwrap(); + assert_eq!( + fs::read_to_string(root.path().join("secret/new")).unwrap(), + "through-logical-path" ); - assert!(!root.path().join("secret/new").exists()); } #[cfg(unix)] #[tokio::test] - async fn write_delegation_rejects_symlink_target_before_lease() { + async fn write_delegation_leases_the_logical_symlink_path() { use std::os::unix::fs::symlink; let root = TempDir::new().unwrap(); @@ -1542,19 +1496,25 @@ mod tests { symlink("../secret", root.path().join("granted/outside")).unwrap(); let parent = session(root.path()); - assert!(matches!( - parent - .scope(request( - "granted/outside", - WorkdirToolScopePermission::Write - )) - .await, - Err(WorkdirError::Denied(_)) - )); + let child = parent + .scope(request( + "granted/outside", + WorkdirToolScopePermission::Write, + )) + .await + .unwrap(); + child + .write(write("from-child", "child-authoritative")) + .await + .unwrap(); parent .write(write("secret/parent", "still-authoritative")) .await .unwrap(); + assert_eq!( + fs::read_to_string(root.path().join("secret/from-child")).unwrap(), + "child-authoritative" + ); } #[tokio::test]