diff --git a/crates/fs-operation/src/glob.rs b/crates/fs-operation/src/glob.rs index 1567eee66..fe5791a3e 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, resolve_access_path}; +use crate::{FsAccessPolicy, FsError, FsPath, GlobRequest, GlobResult}; /// Execute a bounded glob entirely inside the provider process. pub fn run_glob( @@ -19,10 +19,12 @@ pub fn run_glob( path: PathBuf::from(request.path.as_str()), source, })?; - let base_resolved = resolve_access_path(base).map_err(|error| FsError::Io { - path: PathBuf::from(request.path.as_str()), - source: error, - })?; + let base_resolved = access + .resolve_access_path(base) + .map_err(|error| FsError::Io { + path: PathBuf::from(request.path.as_str()), + source: error, + })?; if !access.is_readable_paths(base, &base_resolved) { return Err(FsError::OutOfScope(PathBuf::from(request.path.as_str()))); } @@ -32,8 +34,33 @@ pub fn run_glob( let mut matches = Vec::new(); let mut retained_path_bytes = 0_usize; let mut provider_truncated = false; - let mut walker = WalkBuilder::new(base); + let traversal = access + .open_traversal_root(base, &base_resolved) + .map_err(|source| FsError::Io { + path: PathBuf::from(request.path.as_str()), + source, + })?; + let walker_root = traversal + .as_ref() + .map(|traversal| traversal.path()) + .unwrap_or(base); + let mut walker = WalkBuilder::new(walker_root); walker.hidden(false).follow_links(false); + if traversal.is_some() { + let search_relative = base.strip_prefix(root).map_err(|_| { + FsError::InvalidArgument("glob base is outside its provider root".to_string()) + })?; + let search_relative = search_relative.to_path_buf(); + let filter_root = walker_root.to_path_buf(); + walker.filter_entry(move |entry| { + entry + .path() + .strip_prefix(&filter_root) + .is_ok_and(|relative| { + relative.starts_with(&search_relative) || search_relative.starts_with(relative) + }) + }); + } let mut visited = 0_usize; for entry in walker.build().flatten() { access.check_cancelled().map_err(|source| FsError::Io { @@ -47,19 +74,28 @@ pub fn run_glob( crate::MAX_TRAVERSAL_ENTRIES ))); } - let path = entry.path(); - let readable = resolve_access_path(path) - .is_ok_and(|resolved| access.is_readable_paths(path, &resolved)); - if !path.is_file() || !readable { - continue; - } - let Ok(file) = access.open_read_file(path, path) else { + let walked_path = entry.path(); + let path = if traversal.is_some() { + let relative = walked_path.strip_prefix(walker_root).map_err(|_| { + FsError::InvalidArgument( + "descriptor traversal returned a path outside its root".to_string(), + ) + })?; + root.join(relative) + } else { + walked_path.to_path_buf() + }; + let resolved = match access.resolve_access_path(&path) { + Ok(resolved) if access.is_readable_paths(&path, &resolved) => resolved, + _ => continue, + }; + let Ok(file) = access.open_read_file(&path, &resolved) else { continue; }; if !file.metadata().is_ok_and(|metadata| metadata.is_file()) { continue; } - let relative = path.strip_prefix(base).unwrap_or(path); + let relative = path.strip_prefix(base).unwrap_or(&path); if !matcher.is_match(relative) { continue; } diff --git a/crates/fs-operation/src/lib.rs b/crates/fs-operation/src/lib.rs index 2631b3a80..51f4cd7ea 100644 --- a/crates/fs-operation/src/lib.rs +++ b/crates/fs-operation/src/lib.rs @@ -110,7 +110,14 @@ pub fn open_beneath_no_symlinks_at( ) as RawFd }; if fd < 0 { - return Err(std::io::Error::last_os_error()); + let error = std::io::Error::last_os_error(); + if error.raw_os_error() == Some(libc::ELOOP) { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "symbolic links are not permitted by this provider", + )); + } + return Err(error); } // SAFETY: fd is a new owned descriptor returned by openat2. Ok(unsafe { std::fs::File::from_raw_fd(fd) }) @@ -147,6 +154,26 @@ pub fn open_beneath_no_symlinks(_root: &Path, _path: &Path) -> std::io::Result Self { + Self { + path, + _directory: directory, + } + } + + pub fn path(&self) -> &Path { + &self.path + } +} + /// Provider-owned access policy used by local filesystem operations. pub trait FsAccessPolicy: Send + Sync { fn is_readable(&self, path: &Path) -> bool; @@ -158,6 +185,23 @@ pub trait FsAccessPolicy: Send + Sync { Ok(()) } + /// Resolve a logical path for authorization. Descriptor-confined providers + /// may retain the lexical path because the subsequent open is the authority. + fn resolve_access_path(&self, logical: &Path) -> std::io::Result { + resolve_access_path(logical) + } + + /// Return a descriptor-backed traversal root when pathname walking would + /// violate provider confinement. The retained directory keeps procfs-style + /// descriptor paths valid for the complete walk. + fn open_traversal_root( + &self, + _logical: &Path, + _resolved: &Path, + ) -> std::io::Result> { + Ok(None) + } + /// Open an already-authorized readable file. Capability providers override /// this to bind path resolution and open into one root-confined operation. fn open_read_file(&self, _logical: &Path, resolved: &Path) -> std::io::Result { @@ -168,10 +212,10 @@ pub trait FsAccessPolicy: Send + Sync { /// override this to prevent a path swap between authorization and stat. fn read_metadata( &self, - logical: &Path, - _resolved: &Path, + _logical: &Path, + resolved: &Path, ) -> std::io::Result { - std::fs::symlink_metadata(logical) + std::fs::metadata(resolved) } /// Open an already-authorized directory for bounded enumeration. diff --git a/crates/fs-operation/src/local.rs b/crates/fs-operation/src/local.rs index 191b10832..553271171 100644 --- a/crates/fs-operation/src/local.rs +++ b/crates/fs-operation/src/local.rs @@ -24,7 +24,9 @@ pub fn run_stat( .check_cancelled() .map_err(|error| map_io(&logical, error))?; let path = resolve(root, &logical)?; - let resolved = resolve_access_path(&path).map_err(|error| map_io(&logical, error))?; + let resolved = access + .resolve_access_path(&path) + .map_err(|error| map_io(&logical, error))?; if !access.is_readable_paths(&path, &resolved) { return Err(FsError::OutOfScope(PathBuf::from(logical.as_str()))); } @@ -308,7 +310,7 @@ pub fn run_list( } let entry = entry.map_err(|error| map_io(&logical, error))?; let logical_absolute = logical_base.join(entry.file_name()); - let resolved = match resolve_access_path(&logical_absolute) { + let resolved = match access.resolve_access_path(&logical_absolute) { Ok(resolved) => resolved, Err(_) => continue, }; @@ -380,7 +382,10 @@ fn require_access( write: bool, allow_symlink_directory: bool, ) -> Result { - let symlink = direct_symlink(path); + let resolved = access + .resolve_access_path(path) + .map_err(|error| map_io(logical, error))?; + let symlink = (resolved != path).then(|| direct_symlink(path)).flatten(); if let Some(info) = symlink.as_ref() && !info.target_exists { @@ -390,7 +395,6 @@ fn require_access( target: PathBuf::from(""), }); } - let resolved = resolve_access_path(path).map_err(|error| map_io(logical, error))?; if let Some(info) = symlink { let allowed = if write { access.is_writable_paths(path, &resolved) diff --git a/crates/fs-operation/src/search.rs b/crates/fs-operation/src/search.rs index 41b437f2f..ce91636de 100644 --- a/crates/fs-operation/src/search.rs +++ b/crates/fs-operation/src/search.rs @@ -1,6 +1,6 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use std::fmt::Write as _; -use std::io::Read; +use std::io::{BufRead, BufReader, Read}; use std::path::{Path, PathBuf}; use crate::FsAccessPolicy; @@ -8,12 +8,11 @@ use grep_regex::RegexMatcherBuilder; use grep_searcher::sinks::UTF8 as UTF8Sink; use grep_searcher::{BinaryDetection, Searcher, SearcherBuilder, Sink, SinkContext, SinkMatch}; use ignore::WalkBuilder; +use ignore::gitignore::{Gitignore, GitignoreBuilder}; use ignore::overrides::{Override, OverrideBuilder}; use ignore::types::{Types, TypesBuilder}; -use crate::{ - FsError, GrepOutputMode, GrepRequest, GrepResult, direct_symlink, resolve_access_path, -}; +use crate::{FsError, GrepOutputMode, GrepRequest, GrepResult, direct_symlink}; struct SourceBoundedReader<'a, R> { inner: R, @@ -208,6 +207,118 @@ fn direct_file_selected(path: &Path, overrides: Option<&Override>, types: Option && !types.is_some_and(|filter| filter.matched(path, false).is_ignore()) } +#[derive(Default)] +struct DescriptorIgnoreMatchers { + loaded_directories: HashSet, + by_directory: BTreeMap, +} + +impl DescriptorIgnoreMatchers { + fn load_directory( + &mut self, + directory: &Path, + explicit_base: &Path, + source_bytes_remaining: &mut u64, + access: &dyn FsAccessPolicy, + ) -> Result<(), FsError> { + if !self.loaded_directories.insert(directory.to_path_buf()) { + return Ok(()); + } + let mut builder = GitignoreBuilder::new(directory); + let mut has_patterns = false; + for name in [".gitignore", ".ignore"] { + let path = directory.join(name); + let resolved = access + .resolve_access_path(&path) + .map_err(|error| FsError::io(&path, error))?; + if !access.is_readable_paths(&path, &resolved) { + continue; + } + let file = match access.open_read_file(&path, &resolved) { + Ok(file) => file, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => return Err(FsError::io(&path, error)), + }; + if !file + .metadata() + .map_err(|error| FsError::io(&path, error))? + .is_file() + { + continue; + } + let bounded = SourceBoundedReader { + inner: file, + remaining: source_bytes_remaining, + access, + }; + let mut reader = BufReader::new(bounded); + let mut line = String::new(); + let mut line_number = 0_u64; + loop { + line.clear(); + let read = reader + .read_line(&mut line) + .map_err(|error| FsError::io(&path, error))?; + if read == 0 { + break; + } + line_number = line_number.saturating_add(1); + if line.len() > 1024 * 1024 { + return Err(FsError::InvalidArgument(format!( + "ignore pattern line {line_number} exceeds provider limit" + ))); + } + let line = line.trim_end_matches(['\r', '\n']); + // Match the ignore crate's partial-error behavior: one invalid + // pattern does not discard the remaining valid lines. + if builder.add_line(Some(path.clone()), line).is_ok() { + has_patterns = true; + } + } + } + if !has_patterns { + return Ok(()); + } + let matcher = builder.build().map_err(|_| { + FsError::InvalidArgument("ignore patterns could not be compiled".to_string()) + })?; + // An explicitly selected search root is traversed even when an ancestor + // ignores that directory, matching path-backed Grep behavior. + if directory != explicit_base + && matcher + .matched_path_or_any_parents(explicit_base, true) + .is_ignore() + { + return Ok(()); + } + self.by_directory.insert(directory.to_path_buf(), matcher); + Ok(()) + } + + fn is_ignored(&self, root: &Path, path: &Path) -> bool { + let mut directories = path + .parent() + .into_iter() + .flat_map(Path::ancestors) + .take_while(|directory| directory.starts_with(root)) + .collect::>(); + directories.reverse(); + let mut ignored = false; + for directory in directories { + let Some(matcher) = self.by_directory.get(directory) else { + continue; + }; + let matched = matcher.matched_path_or_any_parents(path, false); + if matched.is_ignore() { + ignored = true; + } else if matched.is_whitelist() { + ignored = false; + } + } + ignored + } +} + struct GrepParams { pattern: String, path: Option, @@ -272,7 +383,22 @@ pub fn run_grep( if !base.is_absolute() { return Err(FsError::RelativePath(base)); } - let symlink = direct_symlink(&base); + let resolved_base = match access.resolve_access_path(&base) { + Ok(resolved) => resolved, + Err(error) => { + if let Some(info) = direct_symlink(&base).filter(|info| !info.target_exists) { + return Err(FsError::BrokenSymlink { + path: base.clone(), + link: info.link_path, + target: info.resolved_path, + }); + } + return Err(FsError::io(&base, error)); + } + }; + let symlink = (resolved_base != base) + .then(|| direct_symlink(&base)) + .flatten(); if let Some(info) = symlink.as_ref() && !info.target_exists { @@ -282,14 +408,14 @@ pub fn run_grep( target: info.resolved_path.clone(), }); } - let resolved_base = resolve_access_path(&base).map_err(|error| FsError::io(&base, error))?; if !access.is_readable_paths(&base, &resolved_base) { return Err(if let Some(info) = symlink.as_ref() { let link_parent_readable = info .link_path .parent() .and_then(|parent| { - resolve_access_path(parent) + access + .resolve_access_path(parent) .ok() .map(|resolved| access.is_readable_paths(parent, &resolved)) }) @@ -307,10 +433,12 @@ pub fn run_grep( FsError::OutOfScope(base.clone()) }); } - let base_meta = std::fs::metadata(&base).map_err(|e| match e.kind() { - std::io::ErrorKind::NotFound => FsError::NotFound(base.clone()), - _ => FsError::io(&base, e), - })?; + let base_meta = access + .read_metadata(&base, &resolved_base) + .map_err(|e| match e.kind() { + std::io::ErrorKind::NotFound => FsError::NotFound(base.clone()), + _ => FsError::io(&base, e), + })?; if !base_meta.is_file() && !base_meta.is_dir() { return Err(FsError::InvalidArgument(format!( "grep search path must be a regular file or directory: {}", @@ -358,24 +486,82 @@ pub fn run_grep( return Ok(report.into_result(root)); } - let mut walker = WalkBuilder::new(&base); - walker - .hidden(true) - .git_ignore(true) - .git_global(true) - .git_exclude(true) - .ignore(true) - .parents(true) - .follow_links(false); - if let Some(types) = types { - walker.types(types); - } - if let Some(overrides) = overrides { - walker.overrides(overrides); + let traversal = access + .open_traversal_root(&base, &resolved_base) + .map_err(|error| FsError::io(&base, error))?; + let walker_root = traversal + .as_ref() + .map(|traversal| traversal.path()) + .unwrap_or(&base); + let mut walker = WalkBuilder::new(walker_root); + if traversal.is_some() { + walker + .hidden(false) + // Path-backed ignore discovery may follow an ignore-file symlink. + // Descriptor providers load ignore files through `access` below. + .git_ignore(false) + .git_global(false) + .git_exclude(false) + .ignore(false) + .parents(false) + .follow_links(false); + let search_relative = base.strip_prefix(root).map_err(|_| { + FsError::InvalidArgument("grep base is outside its provider root".to_string()) + })?; + let search_relative = search_relative.to_path_buf(); + let filter_root = walker_root.to_path_buf(); + walker.filter_entry(move |entry| { + entry + .path() + .strip_prefix(&filter_root) + .is_ok_and(|relative| { + relative.starts_with(&search_relative) || search_relative.starts_with(relative) + }) + }); + } else { + walker + .hidden(true) + .git_ignore(true) + .git_global(true) + .git_exclude(true) + .ignore(true) + .parents(true) + .follow_links(false); + if let Some(types) = types.clone() { + walker.types(types); + } + if let Some(overrides) = overrides.clone() { + walker.overrides(overrides); + } } let mut visited = 0_usize; let mut source_bytes_remaining = crate::MAX_GREP_SOURCE_BYTES; + let mut descriptor_ignores = DescriptorIgnoreMatchers::default(); + if traversal.is_some() { + let mut directory = root.to_path_buf(); + descriptor_ignores.load_directory( + &directory, + &base, + &mut source_bytes_remaining, + access, + )?; + for component in base + .strip_prefix(root) + .map_err(|_| { + FsError::InvalidArgument("grep base is outside its provider root".to_string()) + })? + .components() + { + directory.push(component); + descriptor_ignores.load_directory( + &directory, + &base, + &mut source_bytes_remaining, + access, + )?; + } + } for entry in walker.build().flatten() { access .check_cancelled() @@ -387,23 +573,50 @@ pub fn run_grep( crate::MAX_TRAVERSAL_ENTRIES ))); } - if !entry - .file_type() - .map(|kind| kind.is_file()) - .unwrap_or(false) - { + let walked_path = entry.path(); + let path = if traversal.is_some() { + let relative = walked_path.strip_prefix(walker_root).map_err(|_| { + FsError::InvalidArgument( + "descriptor traversal returned a path outside its root".to_string(), + ) + })?; + root.join(relative) + } else { + walked_path.to_path_buf() + }; + let file_type = entry.file_type(); + if traversal.is_some() && file_type.as_ref().is_some_and(|kind| kind.is_dir()) { + descriptor_ignores.load_directory(&path, &base, &mut source_bytes_remaining, access)?; continue; } - let path = entry.path(); - let readable = resolve_access_path(path) - .is_ok_and(|resolved| access.is_readable_paths(path, &resolved)); + if !file_type.map(|kind| kind.is_file()).unwrap_or(false) { + continue; + } + if traversal.is_some() { + let relative = path.strip_prefix(&base).map_err(|_| { + FsError::InvalidArgument( + "descriptor traversal returned a path outside its search base".to_string(), + ) + })?; + if relative + .components() + .any(|component| component.as_os_str().to_string_lossy().starts_with('.')) + || descriptor_ignores.is_ignored(root, &path) + || !direct_file_selected(&path, overrides.as_ref(), types.as_ref()) + { + continue; + } + } + let readable = access + .resolve_access_path(&path) + .is_ok_and(|resolved| access.is_readable_paths(&path, &resolved)); if !readable { continue; } if scan_path( &mut searcher, &matcher, - path, + &path, mode, &mut report, &mut matching_files_seen, @@ -434,8 +647,11 @@ fn scan_path( source_bytes_remaining: &mut u64, access: &dyn FsAccessPolicy, ) -> Result { + let resolved = access + .resolve_access_path(path) + .map_err(|error| FsError::io(path, error))?; let reader = access - .open_read_file(path, path) + .open_read_file(path, &resolved) .map_err(|error| FsError::io(path, error))?; let metadata = reader .metadata() diff --git a/crates/workdir/src/external_local_tests.rs b/crates/workdir/src/external_local_tests.rs index ae30efc52..3fe822993 100644 --- a/crates/workdir/src/external_local_tests.rs +++ b/crates/workdir/src/external_local_tests.rs @@ -2,9 +2,10 @@ use crate::http::{ WorkdirSessionOperation, WorkdirSessionOperationResult, dispatch_workdir_session_operation, }; use crate::{ - BoundedReadLimits, CommandRequest, EditRequest, ExternalWorkdirRoot, LocalWorkdirSession, - ReadRequest, Workdir, WorkdirError, WorkdirPath, WorkdirSession, WorkdirSessionCapabilities, - WorkdirSessionCapability, WriteRequest, + BoundedReadLimits, CommandRequest, EditRequest, ExternalWorkdirRoot, GlobRequest, + GrepOutputMode, GrepRequest, LocalWorkdirSession, ReadRequest, Workdir, WorkdirError, + WorkdirPath, WorkdirSession, WorkdirSessionCapabilities, WorkdirSessionCapability, + WriteRequest, }; fn external_session(root: &tempfile::TempDir, limits: BoundedReadLimits) -> LocalWorkdirSession { @@ -206,10 +207,13 @@ async fn external_local_provider_rejects_symlink_roots_and_traversal() { ) .await .unwrap_err(); - assert!(matches!( - error, - WorkdirError::SymlinkOutOfScope { .. } | WorkdirError::OutOfScope(_) - )); + assert!( + matches!( + error, + WorkdirError::SymlinkOutOfScope { .. } | WorkdirError::OutOfScope(_) + ), + "unexpected error for {path}: {error:?}" + ); } let parent = tempfile::tempdir().unwrap(); @@ -224,20 +228,103 @@ async fn external_local_provider_rejects_symlink_roots_and_traversal() { assert!(matches!(error, WorkdirError::Denied(_))); } +#[cfg(target_os = "linux")] +#[tokio::test] +async fn external_grep_honors_gitignore_above_a_nested_search_root() { + let root = tempfile::tempdir().unwrap(); + std::fs::create_dir(root.path().join("nested")).unwrap(); + std::fs::write(root.path().join(".gitignore"), "nested/ignored.txt\n").unwrap(); + std::fs::write(root.path().join("nested/ignored.txt"), "needle ignored\n").unwrap(); + std::fs::write(root.path().join("nested/visible.txt"), "needle visible\n").unwrap(); + let session = external_session(&root, BoundedReadLimits::new(4096, 1024).unwrap()); + + let result = WorkdirSession::grep( + &session, + GrepRequest { + pattern: "needle".to_string(), + path: WorkdirPath::new("nested").unwrap(), + glob: Some("*.txt".to_string()), + file_type: None, + case_insensitive: false, + before_context: 0, + after_context: 0, + multiline: false, + output_mode: GrepOutputMode::FilesWithMatches, + limit: 10, + offset: 0, + }, + ) + .await + .unwrap(); + + assert!(result.output.contains("nested/visible.txt")); + assert!(!result.output.contains("nested/ignored.txt")); + + std::fs::write(root.path().join(".gitignore"), "nested/\n").unwrap(); + let explicit_ignored_root = WorkdirSession::grep( + &session, + GrepRequest { + pattern: "needle".to_string(), + path: WorkdirPath::new("nested").unwrap(), + glob: Some("*.txt".to_string()), + file_type: None, + case_insensitive: false, + before_context: 0, + after_context: 0, + multiline: false, + output_mode: GrepOutputMode::FilesWithMatches, + limit: 10, + offset: 0, + }, + ) + .await + .unwrap(); + assert!(explicit_ignored_root.output.contains("nested/visible.txt")); + assert!(explicit_ignored_root.output.contains("nested/ignored.txt")); + + let outside = tempfile::tempdir().unwrap(); + std::fs::write(outside.path().join("ignore-rules"), "nested/visible.txt\n").unwrap(); + std::fs::remove_file(root.path().join(".gitignore")).unwrap(); + std::os::unix::fs::symlink( + outside.path().join("ignore-rules"), + root.path().join(".gitignore"), + ) + .unwrap(); + let symlink_error = WorkdirSession::grep( + &session, + GrepRequest { + pattern: "needle".to_string(), + path: WorkdirPath::new("nested").unwrap(), + glob: Some("*.txt".to_string()), + file_type: None, + case_insensitive: false, + before_context: 0, + after_context: 0, + multiline: false, + output_mode: GrepOutputMode::FilesWithMatches, + limit: 10, + offset: 0, + }, + ) + .await + .unwrap_err(); + assert!(matches!(symlink_error, WorkdirError::OutOfScope(_))); +} + #[cfg(target_os = "linux")] #[tokio::test] async fn external_local_provider_keeps_pre_grant_root_when_path_changes_before_session_creation() { let parent = tempfile::tempdir().unwrap(); let approved = parent.path().join("shared"); std::fs::create_dir(&approved).unwrap(); - std::fs::write(approved.join("item.txt"), "approved").unwrap(); + std::fs::write(approved.join("approved.txt"), "approved needle").unwrap(); // This is the CLI's approval/open boundary. Remote grant creation happens // only after this descriptor has been pinned. let pinned = ExternalWorkdirRoot::pin(&approved).unwrap(); std::fs::rename(&approved, parent.path().join("approved-original")).unwrap(); std::fs::create_dir(&approved).unwrap(); - std::fs::write(approved.join("item.txt"), "replacement").unwrap(); + std::fs::write(approved.join("replacement.txt"), "replacement needle").unwrap(); let session = LocalWorkdirSession::external_read_only_pinned( Workdir::new("external-workdir-pre-grant-pin"), @@ -248,7 +335,7 @@ async fn external_local_provider_keeps_pre_grant_root_when_path_changes_before_s let read = WorkdirSession::read( &session, ReadRequest { - path: WorkdirPath::new("item.txt").unwrap(), + path: WorkdirPath::new("approved.txt").unwrap(), offset: 0, limit: 10, max_bytes: 1024, @@ -256,7 +343,91 @@ async fn external_local_provider_keeps_pre_grant_root_when_path_changes_before_s ) .await .unwrap(); - assert_eq!(read.bytes, b"approved"); + assert_eq!(read.bytes, b"approved needle"); + + let glob = WorkdirSession::glob( + &session, + GlobRequest { + pattern: "*.txt".to_string(), + path: WorkdirPath::new("").unwrap(), + limit: 10, + }, + ) + .await + .unwrap(); + assert_eq!( + glob.paths + .iter() + .map(|path| path.as_str()) + .collect::>(), + vec!["approved.txt"] + ); + + let grep = WorkdirSession::grep( + &session, + GrepRequest { + pattern: "needle".to_string(), + path: WorkdirPath::new("").unwrap(), + glob: Some("*.txt".to_string()), + file_type: None, + case_insensitive: false, + before_context: 0, + after_context: 0, + multiline: false, + output_mode: GrepOutputMode::FilesWithMatches, + limit: 10, + offset: 0, + }, + ) + .await + .unwrap(); + assert!(grep.output.contains("approved.txt")); + assert!(!grep.output.contains("replacement.txt")); + + let outside = tempfile::tempdir().unwrap(); + std::fs::write(outside.path().join("secret.txt"), "secret needle").unwrap(); + std::fs::remove_file(approved.join("replacement.txt")).unwrap(); + std::fs::remove_dir(&approved).unwrap(); + std::os::unix::fs::symlink(outside.path(), &approved).unwrap(); + + let glob_after_symlink_swap = WorkdirSession::glob( + &session, + GlobRequest { + pattern: "*.txt".to_string(), + path: WorkdirPath::new("").unwrap(), + limit: 10, + }, + ) + .await + .unwrap(); + assert_eq!( + glob_after_symlink_swap + .paths + .iter() + .map(|path| path.as_str()) + .collect::>(), + vec!["approved.txt"] + ); + let grep_after_symlink_swap = WorkdirSession::grep( + &session, + GrepRequest { + pattern: "needle".to_string(), + path: WorkdirPath::new("").unwrap(), + glob: Some("*.txt".to_string()), + file_type: None, + case_insensitive: false, + before_context: 0, + after_context: 0, + multiline: false, + output_mode: GrepOutputMode::FilesWithMatches, + limit: 10, + offset: 0, + }, + ) + .await + .unwrap(); + assert!(grep_after_symlink_swap.output.contains("approved.txt")); + assert!(!grep_after_symlink_swap.output.contains("secret.txt")); } #[cfg(target_os = "linux")] diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 5e93ad21f..3a48f227f 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -246,9 +246,7 @@ impl ScopeAccess { fn path_is_confined(&self, logical: &Path, resolved: &Path) -> bool { !self.reject_symlinks - || (logical.starts_with(&self.root) - && resolved.starts_with(&self.root) - && fs_operation::first_symlink(logical).is_none()) + || (logical.starts_with(&self.root) && resolved.starts_with(&self.root)) } } @@ -281,6 +279,54 @@ impl fs_operation::FsAccessPolicy for ScopeAccess { Ok(()) } + fn resolve_access_path(&self, logical: &Path) -> std::io::Result { + if !self.reject_symlinks { + return fs_operation::resolve_access_path(logical); + } + if logical.starts_with(&self.root) { + Ok(logical.to_path_buf()) + } else { + Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "path is outside provider root", + )) + } + } + + fn open_traversal_root( + &self, + _logical: &Path, + resolved: &Path, + ) -> std::io::Result> { + if !self.reject_symlinks { + return Ok(None); + } + #[cfg(target_os = "linux")] + { + use std::os::fd::AsRawFd; + + let directory = self.open_confined(resolved)?; + if !directory.metadata()?.is_dir() { + return Err(std::io::Error::new( + std::io::ErrorKind::NotADirectory, + "path is not a directory", + )); + } + let root = self + .pinned_root + .as_ref() + .expect("confined traversal requires a pinned root") + .try_clone()?; + let path = PathBuf::from(format!("/proc/self/fd/{}", root.as_raw_fd())); + return Ok(Some(fs_operation::FsTraversalRoot::new(path, root))); + } + #[cfg(not(target_os = "linux"))] + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "descriptor-rooted traversal is unavailable on this platform", + )) + } + fn open_read_file(&self, _logical: &Path, resolved: &Path) -> std::io::Result { if self.reject_symlinks { self.open_confined(resolved) @@ -289,11 +335,15 @@ impl fs_operation::FsAccessPolicy for ScopeAccess { } } - fn read_metadata(&self, logical: &Path, resolved: &Path) -> std::io::Result { + fn read_metadata( + &self, + _logical: &Path, + resolved: &Path, + ) -> std::io::Result { if self.reject_symlinks { self.open_confined(resolved)?.metadata() } else { - std::fs::symlink_metadata(logical) + std::fs::metadata(resolved) } } @@ -1367,6 +1417,11 @@ fn sanitize_error(error: WorkdirError, logical: &WorkdirPath) -> WorkdirError { WorkdirError::ReadOnly(_) => WorkdirError::ReadOnly(path), WorkdirError::IsDirectory(_) => WorkdirError::IsDirectory(path), WorkdirError::NotFound(_) => WorkdirError::NotFound(path), + WorkdirError::Io { source, .. } + if source.kind() == std::io::ErrorKind::PermissionDenied => + { + WorkdirError::OutOfScope(path) + } WorkdirError::Io { source, .. } => WorkdirError::Unavailable(format!( "I/O operation failed for {logical}: {}", source.kind() diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 9f8e49ca4..12090b437 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -723,6 +723,7 @@ struct ExternalProviderConnection { } struct ExternalWorkdirExpiryTask { + task_id: Uuid, handle: tokio::task::JoinHandle<()>, } @@ -8991,7 +8992,7 @@ async fn open_current_worker_workdir_session_locked( })?; if let WorkdirRegistrySource::ExternalGrant { grant_id } = &workdir.source { close_current_worker_attachment_session_locked(api, worker).await?; - let grant = api + let mut grant = api .store .get_external_workdir_grant(&api.config.workspace_id, grant_id)? .ok_or_else(|| Error::RuntimeOperationFailed { @@ -9003,45 +9004,15 @@ async fn open_current_worker_workdir_session_locked( .map(|expires_at| expires_at.with_timezone(&Utc) <= Utc::now()) .unwrap_or(true); if expired { - let updated_at = Utc::now().to_rfc3339_opts(SecondsFormat::Nanos, true); - api.store.update_external_workdir_grant_state( - &api.config.workspace_id, - grant_id, - &grant.provider_instance_id, - grant.generation, - "expired", - &updated_at, - )?; + grant = fence_external_workdir_grant_expired(api, grant_id) + .await + .map_err(|error| error.error)?; + } + if matches!(grant.status.as_str(), "revoked" | "expired") { + cleanup_external_workdir_attachments_with_lock(api, &grant.workdir_id, Some(worker)) + .await + .map_err(|error| error.error)?; cancel_external_workdir_expiry(api, grant_id); - let connection = { - api.external_workdir_providers - .lock() - .expect("External Workdir provider registry poisoned") - .disconnect(grant_id, grant.generation) - }; - if let Some(connection) = connection { - let _ = connection - .sender - .send(ExternalProviderCommand::Revoke { - reason: workdir::external::ExternalWorkdirRevokeReason::Expired, - }) - .await; - } - close_current_worker_command_sessions_for_alias_locked(api, worker, &link.alias) - .await?; - api.store.detach_worker_workdir( - &api.config.workspace_id, - worker, - Some(&link.workdir_id), - &updated_at, - )?; - sync_runtime_worker_workdir_attachments(api, worker).map_err(|error| error.error)?; - api.worker_projection.refresh(worker)?; - return Err(Error::RuntimeOperationFailed { - runtime_id: "workspace-backend".to_string(), - code: "external_workdir_provider_expired".to_string(), - message: "External Workdir grant has expired".to_string(), - }); } if grant.status != "online" { return Err(Error::RuntimeOperationFailed { @@ -11950,22 +11921,30 @@ fn schedule_external_workdir_expiry( let grant_id = grant.grant_id.clone(); let task_api = api.clone(); let task_grant_id = grant_id.clone(); + let task_id = Uuid::now_v7(); + let task_instance_id = task_id; + let (start_sender, start_receiver) = tokio::sync::oneshot::channel(); let handle = tokio::spawn(async move { + if start_receiver.await.is_err() { + return; + } let delay = (expires_at - Utc::now()).to_std().unwrap_or_default(); tokio::time::sleep(delay).await; loop { match expire_external_workdir_grant(&task_api, &task_grant_id).await { - Ok(()) => break, + Ok(true) => break, + Ok(false) => { + // Wall time may have moved backwards after the monotonic + // sleep was calculated. Keep ownership and recheck without + // detaching an active grant. + tokio::time::sleep(std::time::Duration::from_secs(1)).await; + } Err(error) => { tracing::error!(grant_id = %task_grant_id, error = ?error, "Backend External Workdir expiry cleanup requires retry"); let retryable = task_api .store .get_external_workdir_grant(&task_api.config.workspace_id, &task_grant_id) - .ok() - .flatten() - .is_some_and(|grant| { - matches!(grant.status.as_str(), "revoked" | "expired") - }); + .map_or(true, |grant| grant.is_some()); if !retryable { break; } @@ -11973,14 +11952,26 @@ fn schedule_external_workdir_expiry( } } } + let mut tasks = task_api + .external_workdir_expiry_tasks + .lock() + .expect("External Workdir expiry task registry poisoned"); + if tasks + .get(&task_grant_id) + .is_some_and(|task| task.task_id == task_instance_id) + { + tasks.remove(&task_grant_id); + } }); let mut tasks = api .external_workdir_expiry_tasks .lock() .expect("External Workdir expiry task registry poisoned"); - if let Some(existing) = tasks.insert(grant_id, ExternalWorkdirExpiryTask { handle }) { + if let Some(existing) = tasks.insert(grant_id, ExternalWorkdirExpiryTask { task_id, handle }) { existing.handle.abort(); } + drop(tasks); + let _ = start_sender.send(()); Ok(()) } @@ -11995,20 +11986,23 @@ fn cancel_external_workdir_expiry(api: &WorkspaceApi, grant_id: &str) { } } -async fn expire_external_workdir_grant(api: &WorkspaceApi, grant_id: &str) -> ApiResult<()> { +async fn fence_external_workdir_grant_expired( + api: &WorkspaceApi, + grant_id: &str, +) -> ApiResult { loop { - let grant = api + let mut grant = api .store .get_external_workdir_grant(&api.config.workspace_id, grant_id)? .ok_or_else(|| Error::InvalidInput("Unknown External Workdir grant".to_string()))?; if matches!(grant.status.as_str(), "revoked" | "expired") { - break; + return Ok(grant); } let expires_at = chrono::DateTime::parse_from_rfc3339(&grant.expires_at) .map(|value| value.with_timezone(&Utc)) .map_err(|_| Error::Store("External Workdir grant has invalid expiry".to_string()))?; if expires_at > Utc::now() { - return Ok(()); + return Ok(grant); } let updated_at = Utc::now().to_rfc3339_opts(SecondsFormat::Nanos, true); if !api.store.update_external_workdir_grant_state( @@ -12050,20 +12044,19 @@ async fn expire_external_workdir_grant(api: &WorkspaceApi, grant_id: &str) -> Ap ), &updated_at, )?; - break; + grant.status = "expired".to_string(); + grant.updated_at = updated_at; + return Ok(grant); } - cleanup_external_workdir_attachments_for_grant(api, grant_id).await } -async fn cleanup_external_workdir_attachments_for_grant( - api: &WorkspaceApi, - grant_id: &str, -) -> ApiResult<()> { - let grant = api - .store - .get_external_workdir_grant(&api.config.workspace_id, grant_id)? - .ok_or_else(|| Error::InvalidInput("Unknown External Workdir grant".to_string()))?; - cleanup_external_workdir_attachments(api, &grant.workdir_id).await +async fn expire_external_workdir_grant(api: &WorkspaceApi, grant_id: &str) -> ApiResult { + let grant = fence_external_workdir_grant_expired(api, grant_id).await?; + if !matches!(grant.status.as_str(), "revoked" | "expired") { + return Ok(false); + } + cleanup_external_workdir_attachments(api, &grant.workdir_id).await?; + Ok(true) } async fn scoped_create_external_workdir_grant( @@ -12166,7 +12159,7 @@ async fn scoped_external_workdir_provider_ws( ws: WebSocketUpgrade, ) -> ApiResult { validate_workspace_scope(&api, &workspace_id)?; - let grant = api + let mut grant = api .store .get_external_workdir_grant(&workspace_id, &grant_id)? .ok_or_else(|| Error::InvalidInput("Unknown External Workdir grant".to_string()))?; @@ -12180,17 +12173,14 @@ async fn scoped_external_workdir_provider_ws( .map(|expires_at| expires_at.with_timezone(&Utc) <= now) .unwrap_or(true); if expired { - api.store.update_external_workdir_grant_state( - &workspace_id, - &grant_id, - &grant.provider_instance_id, - grant.generation, - "expired", - &now.to_rfc3339_opts(SecondsFormat::Nanos, true), - )?; - cancel_external_workdir_expiry(&api, &grant_id); + grant = fence_external_workdir_grant_expired(&api, &grant_id).await?; + } + if matches!(grant.status.as_str(), "revoked" | "expired") { cleanup_external_workdir_attachments(&api, &grant.workdir_id).await?; - return Err(Error::InvalidInput("External Workdir grant has expired".to_string()).into()); + cancel_external_workdir_expiry(&api, &grant_id); + return Err( + Error::InvalidInput(format!("External Workdir grant is {}", grant.status)).into(), + ); } Ok(ws .max_message_size(workdir::external::MAX_EXTERNAL_PROVIDER_FRAME_BYTES) @@ -12551,24 +12541,13 @@ async fn scoped_get_external_workdir_grant( .map(|expires_at| expires_at.with_timezone(&Utc) <= now) .unwrap_or(true); if expired { - let updated_at = now.to_rfc3339_opts(SecondsFormat::Nanos, true); - if api.store.update_external_workdir_grant_state( - &workspace_id, - &grant_id, - &grant.provider_instance_id, - grant.generation, - "expired", - &updated_at, - )? { - grant.status = "expired".to_string(); - grant.updated_at = updated_at; - } - cancel_external_workdir_expiry(&api, &grant_id); + grant = fence_external_workdir_grant_expired(&api, &grant_id).await?; } // Terminal cleanup is retryable: a prior expiry/revoke may have committed // its fence before Runtime/session projection completed. if matches!(grant.status.as_str(), "expired" | "revoked") { cleanup_external_workdir_attachments(&api, &grant.workdir_id).await?; + cancel_external_workdir_expiry(&api, &grant_id); } Ok(Json(external_workdir_grant_response(&grant))) } @@ -12615,7 +12594,6 @@ async fn scoped_revoke_external_workdir_grant( } grant.status = "revoked".to_string(); grant.updated_at = updated_at; - cancel_external_workdir_expiry(&api, &grant_id); api.store.record_external_workdir_audit( &workspace_id, "account", @@ -12634,12 +12612,21 @@ async fn scoped_revoke_external_workdir_grant( // Cleanup is deliberately retried for terminal grants. A prior revoke may // have committed its fence before a Runtime/session projection failure. cleanup_external_workdir_attachments(&api, &grant.workdir_id).await?; + cancel_external_workdir_expiry(&api, &grant_id); Ok(Json(external_workdir_grant_response(&grant))) } async fn cleanup_external_workdir_attachments( api: &WorkspaceApi, workdir_id: &str, +) -> ApiResult<()> { + cleanup_external_workdir_attachments_with_lock(api, workdir_id, None).await +} + +async fn cleanup_external_workdir_attachments_with_lock( + api: &WorkspaceApi, + workdir_id: &str, + already_locked_worker: Option<&RuntimeWorkerRef>, ) -> ApiResult<()> { let workdir = api .store @@ -12657,7 +12644,7 @@ async fn cleanup_external_workdir_attachments( &now, )?; - match release_external_workdir_attachments(api, workdir_id).await { + match release_external_workdir_attachments(api, workdir_id, already_locked_worker).await { Ok(()) => { let completed_at = Utc::now().to_rfc3339_opts(SecondsFormat::Nanos, true); api.store.update_external_workdir_cleanup_state( @@ -12705,16 +12692,66 @@ async fn cleanup_external_workdir_attachments( } } -async fn release_external_workdir_attachments( +#[cfg(test)] +fn external_workdir_cleanup_failure_slot() -> &'static Mutex> { + use std::sync::OnceLock; + + static INJECTED_WORKDIR: OnceLock>> = OnceLock::new(); + INJECTED_WORKDIR.get_or_init(|| Mutex::new(None)) +} + +#[cfg(test)] +fn inject_external_workdir_cleanup_failure(workdir_id: &str) { + *external_workdir_cleanup_failure_slot() + .lock() + .expect("External cleanup injection lock poisoned") = Some(workdir_id.to_string()); +} + +#[cfg(test)] +fn take_external_workdir_cleanup_failure(workdir_id: &str) -> bool { + let mut injected = external_workdir_cleanup_failure_slot() + .lock() + .expect("External cleanup injection lock poisoned"); + if injected.as_deref() == Some(workdir_id) { + *injected = None; + true + } else { + false + } +} + +#[cfg(test)] +fn external_workdir_cleanup_reconciliations() -> &'static Mutex> { + use std::sync::OnceLock; + + static RECONCILIATIONS: OnceLock>> = OnceLock::new(); + RECONCILIATIONS.get_or_init(|| Mutex::new(HashMap::new())) +} + +#[cfg(test)] +fn record_external_workdir_cleanup_reconciliation(workdir_id: &str) { + let mut reconciliations = external_workdir_cleanup_reconciliations() + .lock() + .expect("External cleanup reconciliation lock poisoned"); + *reconciliations.entry(workdir_id.to_string()).or_default() += 1; +} + +#[cfg(test)] +fn external_workdir_cleanup_reconciliation_count(workdir_id: &str) -> usize { + external_workdir_cleanup_reconciliations() + .lock() + .expect("External cleanup reconciliation lock poisoned") + .get(workdir_id) + .copied() + .unwrap_or_default() +} + +async fn release_external_workdir_attachment_locked( api: &WorkspaceApi, workdir_id: &str, + link: &WorkerWorkdirLinkRecord, ) -> ApiResult<()> { - let links = api - .store - .list_workdir_worker_links(&api.config.workspace_id, workdir_id)?; - for link in links.into_iter().filter(|link| link.unlinked_at.is_none()) { - let lock = current_worker_session_lock(api, &link.worker); - let _guard = lock.lock().await; + if link.unlinked_at.is_none() { close_current_worker_attachment_session_locked(api, &link.worker).await?; close_current_worker_command_sessions_for_alias_locked(api, &link.worker, &link.alias) .await?; @@ -12724,10 +12761,47 @@ async fn release_external_workdir_attachments( Some(workdir_id), &Utc::now().to_rfc3339_opts(SecondsFormat::Nanos, true), )?; - sync_runtime_worker_workdir_attachments(api, &link.worker)?; - api.worker_projection - .refresh(&link.worker) - .map_err(ApiError::from)?; + #[cfg(test)] + if take_external_workdir_cleanup_failure(workdir_id) { + return Err(Error::RuntimeOperationFailed { + runtime_id: "workspace-backend".to_string(), + code: "injected_external_workdir_cleanup_failure".to_string(), + message: "injected External Workdir attachment cleanup failure".to_string(), + } + .into()); + } + } + // Reconcile even an already-unlinked durable link. A previous cleanup may + // have committed detach before Runtime persistence or projection failed. + #[cfg(test)] + record_external_workdir_cleanup_reconciliation(workdir_id); + sync_runtime_worker_workdir_attachments(api, &link.worker)?; + api.worker_projection + .refresh(&link.worker) + .map_err(ApiError::from)?; + Ok(()) +} + +async fn release_external_workdir_attachments( + api: &WorkspaceApi, + workdir_id: &str, + already_locked_worker: Option<&RuntimeWorkerRef>, +) -> ApiResult<()> { + let Some(link) = api + .store + .latest_workdir_worker_link(&api.config.workspace_id, workdir_id)? + else { + return Ok(()); + }; + // The newest link is the only possible active attachment and remains the + // durable reconciliation target if detach committed before Runtime update. + // Missing/ambiguous Runtime state remains a retryable cleanup failure. + if already_locked_worker == Some(&link.worker) { + release_external_workdir_attachment_locked(api, workdir_id, &link).await?; + } else { + let lock = current_worker_session_lock(api, &link.worker); + let _guard = lock.lock().await; + release_external_workdir_attachment_locked(api, workdir_id, &link).await?; } Ok(()) } @@ -21226,7 +21300,13 @@ mod tests { .map_err(Error::from) }) .unwrap(); - if cleanup_state == "completed" { + if cleanup_state == "completed" + && !api + .external_workdir_expiry_tasks + .lock() + .unwrap() + .contains_key(&grant.grant_id) + { break; } } @@ -21237,6 +21317,219 @@ mod tests { .expect("Backend-owned expiry must run without a provider socket"); } + #[tokio::test] + async fn session_open_expiry_persists_cleanup_retry_without_deadlocking() { + let dir = tempfile::tempdir().unwrap(); + init_clean_git_workspace(dir.path()); + let api = test_api(dir.path()).await; + let Json(created_worker) = create_workspace_worker( + State(api.clone()), + HeaderMap::new(), + Json(CreateWorkspaceWorkerRequest { + runtime_id: EMBEDDED_WORKER_RUNTIME_ID.to_string(), + display_name: "External expiry Worker".to_string(), + profile: Some("builtin:coder".to_string()), + ticket_assignment: None, + initial_submit: Vec::new(), + workdir_attachments: Vec::new(), + control_operation_id: None, + }), + ) + .await + .unwrap(); + let worker = RuntimeWorkerRef::new(&created_worker.runtime_id, &created_worker.worker_id); + let now = Utc::now(); + let created_at = now.to_rfc3339_opts(SecondsFormat::Nanos, true); + let grant = ExternalWorkdirGrantRecord { + grant_id: "session-open-expiry-grant".to_string(), + workspace_id: TEST_WORKSPACE_ID.to_string(), + workdir_id: "session-open-expiry-workdir".to_string(), + provider_instance_id: "session-open-expiry-provider".to_string(), + display_name: "Session-open expiry".to_string(), + permissions: "read_only".to_string(), + created_by: test_browser_request_actor().account_id, + created_at: created_at.clone(), + expires_at: (now + Duration::minutes(5)).to_rfc3339_opts(SecondsFormat::Nanos, true), + generation: 1, + status: "online".to_string(), + updated_at: created_at.clone(), + }; + api.store + .create_external_workdir_grant( + &grant, + &WorkdirRegistryRecord { + workspace_id: TEST_WORKSPACE_ID.to_string(), + workdir_id: grant.workdir_id.clone(), + display_name: Some(grant.display_name.clone()), + source: WorkdirRegistrySource::ExternalGrant { + grant_id: grant.grant_id.clone(), + }, + creation_selector: None, + creation_ref: None, + creation_tree: None, + current_selector: None, + current_ref: None, + current_tree: None, + observed_at_epoch_seconds: None, + materialization_status: "present".to_string(), + cleanliness: "unknown".to_string(), + created_at: created_at.clone(), + updated_at: created_at.clone(), + }, + ) + .unwrap(); + schedule_external_workdir_expiry(&api, &grant).unwrap(); + let link = WorkerWorkdirLinkRecord { + workspace_id: TEST_WORKSPACE_ID.to_string(), + worker: worker.clone(), + workdir_id: grant.workdir_id.clone(), + alias: "expired-external".to_string(), + linked_at: created_at, + unlinked_at: None, + }; + api.store.attach_worker_workdir(&link).unwrap(); + sync_runtime_worker_workdir_attachments(&api, &worker).unwrap(); + assert!( + !expire_external_workdir_grant(&api, &grant.grant_id) + .await + .unwrap(), + "an early scheduler wake must retain ownership without cleanup" + ); + assert!( + api.store + .list_workdir_worker_links(TEST_WORKSPACE_ID, &grant.workdir_id) + .unwrap() + .iter() + .any(|current| current.unlinked_at.is_none()) + ); + api.config_store + .with_conn(|conn| { + conn.execute( + "UPDATE external_workdir_grants SET expires_at = ?1 WHERE workspace_id = ?2 AND grant_id = ?3", + rusqlite::params![ + (now - Duration::seconds(1)) + .to_rfc3339_opts(SecondsFormat::Nanos, true), + TEST_WORKSPACE_ID, + grant.grant_id, + ], + )?; + Ok(()) + }) + .unwrap(); + + inject_external_workdir_cleanup_failure(&grant.workdir_id); + let session_lock = current_worker_session_lock(&api, &worker); + let session_guard = session_lock.lock().await; + let error = tokio::time::timeout( + std::time::Duration::from_secs(1), + open_current_worker_workdir_session_locked(&api, &worker, &link), + ) + .await + .expect("session-open expiry cleanup must not reacquire its Worker lock") + .expect_err("injected attachment cleanup must surface"); + assert!(matches!( + error, + Error::RuntimeOperationFailed { ref code, .. } + if code == "injected_external_workdir_cleanup_failure" + )); + let (status, cleanup_state, cleanup_error): (String, String, Option) = api + .config_store + .with_conn(|conn| { + conn.query_row( + "SELECT status, cleanup_state, cleanup_error FROM external_workdir_grants WHERE workspace_id = ?1 AND grant_id = ?2", + rusqlite::params![TEST_WORKSPACE_ID, grant.grant_id], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + ) + .map_err(Error::from) + }) + .unwrap(); + assert_eq!(status, "expired"); + assert_eq!(cleanup_state, "retry_required"); + assert_eq!(cleanup_error.as_deref(), Some("attachment_release_failed")); + assert!( + api.external_workdir_expiry_tasks + .lock() + .unwrap() + .contains_key(&grant.grant_id), + "retry ownership must remain until cleanup completes" + ); + assert!( + api.store + .list_workdir_worker_links(TEST_WORKSPACE_ID, &grant.workdir_id) + .unwrap() + .iter() + .all(|current| current.unlinked_at.is_some()), + "durable detach commits before the injected Runtime reconciliation failure" + ); + assert_eq!( + external_workdir_cleanup_reconciliation_count(&grant.workdir_id), + 0, + "injected failure occurs after durable detach but before Runtime reconciliation" + ); + drop(session_guard); + + assert!( + expire_external_workdir_grant(&api, &grant.grant_id) + .await + .unwrap() + ); + cancel_external_workdir_expiry(&api, &grant.grant_id); + let (cleanup_state, cleanup_error): (String, Option) = api + .config_store + .with_conn(|conn| { + conn.query_row( + "SELECT cleanup_state, cleanup_error FROM external_workdir_grants WHERE workspace_id = ?1 AND grant_id = ?2", + rusqlite::params![TEST_WORKSPACE_ID, grant.grant_id], + |row| Ok((row.get(0)?, row.get(1)?)), + ) + .map_err(Error::from) + }) + .unwrap(); + assert_eq!(cleanup_state, "completed"); + assert_eq!(cleanup_error, None); + assert!( + api.store + .list_workdir_worker_links(TEST_WORKSPACE_ID, &grant.workdir_id) + .unwrap() + .iter() + .all(|current| current.unlinked_at.is_some()) + ); + assert_eq!( + external_workdir_cleanup_reconciliation_count(&grant.workdir_id), + 1, + "retry must reconcile an already-unlinked durable attachment" + ); + assert!( + api.runtime + .worker(&worker) + .unwrap() + .workdir_attachments + .iter() + .all( + |attachment| attachment.working_directory.summary.working_directory_id + != grant.workdir_id + ), + "retry must reconcile Runtime state for an already-unlinked durable attachment" + ); + let audit_actions: Vec = api + .config_store + .with_conn(|conn| { + let mut statement = conn.prepare( + "SELECT action FROM audit_events WHERE workspace_id = ?1 AND target_kind = 'external_workdir_grant' AND target_id = ?2 ORDER BY created_at", + )?; + statement + .query_map(rusqlite::params![TEST_WORKSPACE_ID, grant.grant_id], |row| { + row.get(0) + })? + .collect::>>() + .map_err(Error::from) + }) + .unwrap(); + assert!(audit_actions.contains(&"external_workdir_grant_expired".to_string())); + assert!(audit_actions.contains(&"external_workdir_cleanup_attention_required".to_string())); + assert!(audit_actions.contains(&"external_workdir_cleanup_completed".to_string())); + } + #[tokio::test] async fn external_workdirs_are_launchable_with_real_multiple_attachments() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index 677ec66ee..c27422561 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -1743,6 +1743,11 @@ pub trait ControlPlaneStore: Send + Sync + WorkspaceDeletionStore { workspace_id: &str, workdir_id: &str, ) -> Result>; + fn latest_workdir_worker_link( + &self, + workspace_id: &str, + workdir_id: &str, + ) -> Result>; } #[derive(Clone)] @@ -8293,6 +8298,26 @@ impl ControlPlaneStore for SqliteWorkspaceStore { .map_err(Error::from) }) } + + fn latest_workdir_worker_link( + &self, + workspace_id: &str, + workdir_id: &str, + ) -> Result> { + self.with_conn(|conn| { + conn.query_row( + r#"SELECT workspace_id, runtime_id, worker_id, workdir_id, alias, linked_at, unlinked_at + FROM worker_workdir_links + WHERE workspace_id = ?1 AND workdir_id = ?2 + ORDER BY linked_at DESC, rowid DESC + LIMIT 1"#, + params![workspace_id, workdir_id], + read_worker_workdir_link_record, + ) + .optional() + .map_err(Error::from) + }) + } } fn flow_source_name(path: &str) -> Result { diff --git a/crates/yoi/src/workdir_share.rs b/crates/yoi/src/workdir_share.rs index 82cdccc87..72438a218 100644 --- a/crates/yoi/src/workdir_share.rs +++ b/crates/yoi/src/workdir_share.rs @@ -77,11 +77,29 @@ impl Drop for OperationTask { } } +fn pin_selected_root(path: &std::path::Path) -> Result { + let cwd = std::env::current_dir() + .map_err(|error| format!("failed to resolve current directory: {error}"))?; + pin_selected_root_from(path, &cwd) +} + +fn pin_selected_root_from( + path: &std::path::Path, + cwd: &std::path::Path, +) -> Result { + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + cwd.join(path) + }; + ExternalWorkdirRoot::pin(absolute).map_err(|error| error.to_string()) +} + pub(crate) async fn run(options: WorkdirShareOptions) -> Result<(), String> { // Pin before creating remote authority: the Backend grant can never outlive // a failed local approval/open race, and subsequent path replacement cannot // redirect the provider. - let pinned_root = ExternalWorkdirRoot::pin(&options.path).map_err(|error| error.to_string())?; + let pinned_root = pin_selected_root(&options.path)?; let client = BackendApiClient::from_stored_token(&options.backend_url) .map_err(|error| error.to_string())?; @@ -619,6 +637,16 @@ fn encode_path_segment(value: &str) -> String { mod tests { use super::*; + #[test] + fn relative_share_path_is_resolved_and_pinned_before_backend_access() { + let directory = tempfile::tempdir().unwrap(); + let pinned = pin_selected_root_from(std::path::Path::new("."), directory.path()).unwrap(); + assert_eq!( + pinned.canonical_path(), + directory.path().canonicalize().unwrap() + ); + } + #[tokio::test] async fn operation_shutdown_waits_until_executor_has_stopped() { let operation_id = ExternalWorkdirOperationId::new("op-cancel-test").unwrap();