From 111adfbdb61ed3437ff0140633662aadb5d26dfd Mon Sep 17 00:00:00 2001 From: starr-openai Date: Fri, 29 May 2026 14:06:44 -0700 Subject: [PATCH] exec-server: make bound path operations async --- codex-rs/config/src/loader/tests.rs | 21 +++ codex-rs/exec-server/src/client.rs | 4 - codex-rs/exec-server/src/environment_path.rs | 143 ++++++++-------- codex-rs/exec-server/src/fs_helper.rs | 27 ++++ codex-rs/exec-server/src/lib.rs | 6 + codex-rs/exec-server/src/local_file_system.rs | 78 ++++----- codex-rs/exec-server/src/protocol.rs | 3 +- .../exec-server/src/remote_file_system.rs | 152 +++++------------- .../exec-server/src/sandboxed_file_system.rs | 34 ++++ .../src/server/file_system_handler.rs | 7 +- codex-rs/exec-server/tests/file_system.rs | 51 ++++-- codex-rs/file-system/src/lib.rs | 49 ++---- 12 files changed, 306 insertions(+), 269 deletions(-) diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index 2c87e1381d..9d4511a8a6 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -8,12 +8,33 @@ use codex_file_system::FileSystemSandboxContext; use codex_file_system::ReadDirectoryEntry; use codex_file_system::RemoveOptions; use pretty_assertions::assert_eq; +use std::path::Path; use tempfile::tempdir; struct TestFileSystem; #[async_trait] impl ExecutorFileSystem for TestFileSystem { + async fn canonicalize( + &self, + path: &AbsolutePathBuf, + _sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + path.canonicalize() + } + + async fn join( + &self, + base_path: &AbsolutePathBuf, + path: &Path, + ) -> FileSystemResult { + Ok(base_path.join(path)) + } + + async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { + Ok(path.parent()) + } + async fn read_file( &self, path: &AbsolutePathBuf, diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index 458693743f..b6c56e9f5e 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -214,10 +214,6 @@ impl LazyRemoteExecServerClient { } } - pub(crate) fn new_connection_client(&self) -> Self { - Self::new(self.transport_params.clone()) - } - pub(crate) async fn get(&self) -> Result { if let Some(client) = self.connected_client() { return Ok(client); diff --git a/codex-rs/exec-server/src/environment_path.rs b/codex-rs/exec-server/src/environment_path.rs index 270651cc5f..4ee022a6ed 100644 --- a/codex-rs/exec-server/src/environment_path.rs +++ b/codex-rs/exec-server/src/environment_path.rs @@ -62,26 +62,28 @@ impl EnvironmentPathRef { Self::new(Arc::clone(&self.file_system), path) } - pub fn join_relative(&self, relative_path: &Path) -> Option { + pub async fn join>(&self, path: P) -> io::Result { self.file_system - .join(&self.path, relative_path) - .ok() + .join(&self.path, path.as_ref()) + .await .map(|path| self.with_path(path)) } - pub fn parent_dir(&self) -> Option { + pub async fn parent(&self) -> io::Result> { self.file_system .parent(&self.path) - .ok() - .flatten() - .map(|path| self.with_path(path)) + .await + .map(|path| path.map(|path| self.with_path(path))) } - /// Best-effort resolves this path through its bound filesystem. - pub fn canonicalize_if_exists(&self) -> Self { + pub async fn canonicalize( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result { self.file_system - .canonicalize(&self.path) - .map_or_else(|_| self.clone(), |path| self.with_path(path)) + .canonicalize(&self.path, sandbox) + .await + .map(|path| self.with_path(path)) } } @@ -122,6 +124,8 @@ mod tests { use std::path::Path; use std::sync::Mutex; + use crate::LOCAL_FS; + #[derive(Clone, Debug, Eq, PartialEq)] enum RecordedMethod { Canonicalize, @@ -141,26 +145,17 @@ mod tests { struct RecordingFileSystem { calls: Mutex>, - canonicalize_supported: bool, } impl Default for RecordingFileSystem { fn default() -> Self { Self { calls: Mutex::new(Vec::new()), - canonicalize_supported: true, } } } impl RecordingFileSystem { - fn without_canonicalize() -> Self { - Self { - calls: Mutex::new(Vec::new()), - canonicalize_supported: false, - } - } - fn recorded_calls(&self) -> Vec { match self.calls.lock() { Ok(calls) => calls.clone(), @@ -176,37 +171,39 @@ mod tests { } } + fn local_path_ref(path: AbsolutePathBuf) -> EnvironmentPathRef { + EnvironmentPathRef::new(Arc::clone(&LOCAL_FS), path) + } + #[async_trait] impl ExecutorFileSystem for RecordingFileSystem { - fn canonicalize(&self, path: &AbsolutePathBuf) -> io::Result { + async fn canonicalize( + &self, + path: &AbsolutePathBuf, + _sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result { self.push_call(RecordedCall { method: RecordedMethod::Canonicalize, path: path.clone(), sandbox: None, }); - if !self.canonicalize_supported { - return Err(io::Error::new( - io::ErrorKind::Unsupported, - "filesystem does not support canonicalization", - )); - } Ok(path.parent().unwrap()) } - fn join( + async fn join( &self, base_path: &AbsolutePathBuf, - relative_path: &Path, + path: &Path, ) -> io::Result { self.push_call(RecordedCall { method: RecordedMethod::Join, path: base_path.clone(), sandbox: None, }); - AbsolutePathBuf::from_absolute_path_checked(base_path.as_path().join(relative_path)) + AbsolutePathBuf::from_absolute_path_checked(base_path.as_path().join(path)) } - fn parent(&self, path: &AbsolutePathBuf) -> io::Result> { + async fn parent(&self, path: &AbsolutePathBuf) -> io::Result> { self.push_call(RecordedCall { method: RecordedMethod::Parent, path: path.clone(), @@ -391,13 +388,16 @@ mod tests { let set = HashSet::from([left, same, different_path, different_fs]); assert_eq!(set.len(), 3); } - #[test] - fn canonicalize_if_exists_keeps_bound_file_system_identity() { + #[tokio::test] + async fn canonicalize_keeps_bound_file_system_identity() { let path = std::env::temp_dir().join("skills/demo").abs(); let file_system = Arc::new(RecordingFileSystem::default()); let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone()); - let canonicalized = path_ref.canonicalize_if_exists(); + let canonicalized = path_ref + .canonicalize(/*sandbox*/ None) + .await + .expect("canonicalize"); assert_eq!(canonicalized.path(), &path.parent().unwrap()); assert_eq!( @@ -414,23 +414,14 @@ mod tests { ); } - #[test] - fn canonicalize_if_exists_keeps_unsupported_path() { - let path = std::env::temp_dir().join("skills/demo").abs(); - let path_ref = - EnvironmentPathRef::new(Arc::new(RecordingFileSystem::without_canonicalize()), path); - - assert_eq!(path_ref.canonicalize_if_exists(), path_ref); - } - - #[test] - fn join_relative_keeps_bound_file_system_identity() { + #[tokio::test] + async fn join_keeps_bound_file_system_identity() { let path = std::env::temp_dir().join("skills").abs(); let file_system = Arc::new(RecordingFileSystem::default()); let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone()); assert_eq!( - path_ref.join_relative(Path::new("demo")), + path_ref.join(Path::new("demo")).await.ok(), Some(EnvironmentPathRef::new( file_system.clone(), std::env::temp_dir().join("skills/demo").abs(), @@ -446,33 +437,43 @@ mod tests { ); } - #[test] - fn join_relative_keeps_literal_tilde_under_bound_path() { - let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs()); + #[tokio::test] + async fn join_matches_absolute_path_buf_for_tilde_paths() { + let path_ref = local_path_ref(std::env::temp_dir().join("skills").abs()); assert_eq!( path_ref - .join_relative(Path::new("~")) + .join(Path::new("~")) + .await + .ok() .map(|path_ref| path_ref.path().clone()), - Some(std::env::temp_dir().join("skills/~").abs()) + Some(path_ref.path().join(Path::new("~"))) ); } - #[test] - fn join_relative_rejects_parent_dirs() { - let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs()); + #[tokio::test] + async fn join_matches_absolute_path_buf_for_parent_dirs() { + let path_ref = local_path_ref(std::env::temp_dir().join("skills").abs()); - assert_eq!(path_ref.join_relative(Path::new("../outside")), None); + assert_eq!( + path_ref + .join(Path::new("../outside")) + .await + .expect("join") + .path() + .clone(), + path_ref.path().join(Path::new("../outside")) + ); } - #[test] - fn parent_dir_keeps_bound_file_system_identity() { + #[tokio::test] + async fn parent_keeps_bound_file_system_identity() { let path = std::env::temp_dir().join("skills/demo").abs(); let file_system = Arc::new(RecordingFileSystem::default()); let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone()); assert_eq!( - path_ref.parent_dir(), + path_ref.parent().await.expect("parent"), Some(EnvironmentPathRef::new( file_system.clone(), std::env::temp_dir().join("skills").abs(), @@ -489,11 +490,27 @@ mod tests { } #[cfg(windows)] - #[test] - fn join_relative_rejects_windows_prefixed_and_rooted_paths() { - let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs()); + #[tokio::test] + async fn join_matches_absolute_path_buf_for_windows_prefixed_and_rooted_paths() { + let path_ref = local_path_ref(std::env::temp_dir().join("skills").abs()); - assert_eq!(path_ref.join_relative(Path::new(r"C:temp")), None); - assert_eq!(path_ref.join_relative(Path::new(r"\temp")), None); + assert_eq!( + path_ref + .join(Path::new(r"C:temp")) + .await + .expect("join") + .path() + .clone(), + path_ref.path().join(Path::new(r"C:temp")) + ); + assert_eq!( + path_ref + .join(Path::new(r"\temp")) + .await + .expect("join") + .path() + .clone(), + path_ref.path().join(Path::new(r"\temp")) + ); } } diff --git a/codex-rs/exec-server/src/fs_helper.rs b/codex-rs/exec-server/src/fs_helper.rs index 8d91224803..0a210175b2 100644 --- a/codex-rs/exec-server/src/fs_helper.rs +++ b/codex-rs/exec-server/src/fs_helper.rs @@ -10,6 +10,7 @@ use crate::CreateDirectoryOptions; use crate::ExecutorFileSystem; use crate::RemoveOptions; use crate::local_file_system::DirectFileSystem; +use crate::protocol::FS_CANONICALIZE_METHOD; use crate::protocol::FS_COPY_METHOD; use crate::protocol::FS_CREATE_DIRECTORY_METHOD; use crate::protocol::FS_GET_METADATA_METHOD; @@ -17,6 +18,8 @@ use crate::protocol::FS_READ_DIRECTORY_METHOD; use crate::protocol::FS_READ_FILE_METHOD; use crate::protocol::FS_REMOVE_METHOD; use crate::protocol::FS_WRITE_FILE_METHOD; +use crate::protocol::FsCanonicalizeParams; +use crate::protocol::FsCanonicalizeResponse; use crate::protocol::FsCopyParams; use crate::protocol::FsCopyResponse; use crate::protocol::FsCreateDirectoryParams; @@ -49,6 +52,8 @@ pub(crate) enum FsHelperRequest { CreateDirectory(FsCreateDirectoryParams), #[serde(rename = "fs/getMetadata")] GetMetadata(FsGetMetadataParams), + #[serde(rename = "fs/canonicalize")] + Canonicalize(FsCanonicalizeParams), #[serde(rename = "fs/readDirectory")] ReadDirectory(FsReadDirectoryParams), #[serde(rename = "fs/remove")] @@ -75,6 +80,8 @@ pub(crate) enum FsHelperPayload { CreateDirectory(FsCreateDirectoryResponse), #[serde(rename = "fs/getMetadata")] GetMetadata(FsGetMetadataResponse), + #[serde(rename = "fs/canonicalize")] + Canonicalize(FsCanonicalizeResponse), #[serde(rename = "fs/readDirectory")] ReadDirectory(FsReadDirectoryResponse), #[serde(rename = "fs/remove")] @@ -90,6 +97,7 @@ impl FsHelperPayload { Self::WriteFile(_) => FS_WRITE_FILE_METHOD, Self::CreateDirectory(_) => FS_CREATE_DIRECTORY_METHOD, Self::GetMetadata(_) => FS_GET_METADATA_METHOD, + Self::Canonicalize(_) => FS_CANONICALIZE_METHOD, Self::ReadDirectory(_) => FS_READ_DIRECTORY_METHOD, Self::Remove(_) => FS_REMOVE_METHOD, Self::Copy(_) => FS_COPY_METHOD, @@ -132,6 +140,16 @@ impl FsHelperPayload { } } + pub(crate) fn expect_canonicalize(self) -> Result { + match self { + Self::Canonicalize(response) => Ok(response), + other => Err(unexpected_response( + FS_CANONICALIZE_METHOD, + other.operation(), + )), + } + } + pub(crate) fn expect_read_directory( self, ) -> Result { @@ -219,6 +237,15 @@ pub(crate) async fn run_direct_request( modified_at_ms: metadata.modified_at_ms, })) } + FsHelperRequest::Canonicalize(params) => { + let path = file_system + .canonicalize(¶ms.path, /*sandbox*/ None) + .await + .map_err(map_fs_error)?; + Ok(FsHelperPayload::Canonicalize(FsCanonicalizeResponse { + path, + })) + } FsHelperRequest::ReadDirectory(params) => { let entries = file_system .read_directory(¶ms.path, /*sandbox*/ None) diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 01b530ba34..dea039c288 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -64,12 +64,18 @@ pub use protocol::ExecOutputDeltaNotification; pub use protocol::ExecOutputStream; pub use protocol::ExecParams; pub use protocol::ExecResponse; +pub use protocol::FsCanonicalizeParams; +pub use protocol::FsCanonicalizeResponse; pub use protocol::FsCopyParams; pub use protocol::FsCopyResponse; pub use protocol::FsCreateDirectoryParams; pub use protocol::FsCreateDirectoryResponse; pub use protocol::FsGetMetadataParams; pub use protocol::FsGetMetadataResponse; +pub use protocol::FsJoinParams; +pub use protocol::FsJoinResponse; +pub use protocol::FsParentParams; +pub use protocol::FsParentResponse; pub use protocol::FsReadDirectoryEntry; pub use protocol::FsReadDirectoryParams; pub use protocol::FsReadDirectoryResponse; diff --git a/codex-rs/exec-server/src/local_file_system.rs b/codex-rs/exec-server/src/local_file_system.rs index 3489186293..3f410f926d 100644 --- a/codex-rs/exec-server/src/local_file_system.rs +++ b/codex-rs/exec-server/src/local_file_system.rs @@ -1,6 +1,5 @@ use async_trait::async_trait; use codex_utils_absolute_path::AbsolutePathBuf; -use std::path::Component; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -80,20 +79,25 @@ impl LocalFileSystem { #[async_trait] impl ExecutorFileSystem for LocalFileSystem { - fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { - self.unsandboxed.canonicalize(path) + async fn canonicalize( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + let (file_system, sandbox) = self.file_system_for(sandbox)?; + file_system.canonicalize(path, sandbox).await } - fn join( + async fn join( &self, base_path: &AbsolutePathBuf, - relative_path: &Path, + path: &Path, ) -> FileSystemResult { - self.unsandboxed.join(base_path, relative_path) + self.unsandboxed.join(base_path, path).await } - fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { - self.unsandboxed.parent(path) + async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { + self.unsandboxed.parent(path).await } async fn read_file( @@ -169,20 +173,25 @@ impl ExecutorFileSystem for LocalFileSystem { #[async_trait] impl ExecutorFileSystem for UnsandboxedFileSystem { - fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { - self.file_system.canonicalize(path) + async fn canonicalize( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + reject_platform_sandbox_context(sandbox)?; + self.file_system.canonicalize(path, /*sandbox*/ None).await } - fn join( + async fn join( &self, base_path: &AbsolutePathBuf, - relative_path: &Path, + path: &Path, ) -> FileSystemResult { - self.file_system.join(base_path, relative_path) + self.file_system.join(base_path, path).await } - fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { - self.file_system.parent(path) + async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { + self.file_system.parent(path).await } async fn read_file( @@ -271,19 +280,24 @@ impl ExecutorFileSystem for UnsandboxedFileSystem { #[async_trait] impl ExecutorFileSystem for DirectFileSystem { - fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { - path.canonicalize() + async fn canonicalize( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + reject_sandbox_context(sandbox)?; + AbsolutePathBuf::from_absolute_path(tokio::fs::canonicalize(path.as_path()).await?) } - fn join( + async fn join( &self, base_path: &AbsolutePathBuf, - relative_path: &Path, + path: &Path, ) -> FileSystemResult { - join_bound_path(base_path, relative_path) + Ok(base_path.join(path)) } - fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { + async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { Ok(path.parent()) } @@ -456,28 +470,6 @@ fn reject_sandbox_context(sandbox: Option<&FileSystemSandboxContext>) -> io::Res Ok(()) } -fn join_bound_path( - base_path: &AbsolutePathBuf, - relative_path: &Path, -) -> FileSystemResult { - for component in relative_path.components() { - match component { - Component::CurDir | Component::Normal(_) => {} - Component::ParentDir | Component::RootDir | Component::Prefix(_) => { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - format!( - "relative path must not escape or replace the bound root: {}", - relative_path.display() - ), - )); - } - } - } - - AbsolutePathBuf::from_absolute_path_checked(base_path.as_path().join(relative_path)) -} - fn reject_platform_sandbox_context(sandbox: Option<&FileSystemSandboxContext>) -> io::Result<()> { if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) { return Err(io::Error::new( diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index fa56306c62..9ac80c188e 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -218,6 +218,7 @@ pub struct FsGetMetadataResponse { #[serde(rename_all = "camelCase")] pub struct FsCanonicalizeParams { pub path: AbsolutePathBuf, + pub sandbox: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -230,7 +231,7 @@ pub struct FsCanonicalizeResponse { #[serde(rename_all = "camelCase")] pub struct FsJoinParams { pub base_path: AbsolutePathBuf, - pub relative_path: PathBuf, + pub path: PathBuf, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index b0b39aea37..54198c93ba 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -2,13 +2,8 @@ use async_trait::async_trait; use base64::Engine as _; use base64::engine::general_purpose::STANDARD; use codex_utils_absolute_path::AbsolutePathBuf; -use std::future::Future; use std::path::Path; -use std::sync::OnceLock; -use std::thread; use tokio::io; -use tokio::runtime::Builder; -use tokio::runtime::Runtime; use tracing::trace; use crate::CopyOptions; @@ -20,7 +15,6 @@ use crate::FileSystemResult; use crate::FileSystemSandboxContext; use crate::ReadDirectoryEntry; use crate::RemoveOptions; -use crate::client::ExecServerClient; use crate::client::LazyRemoteExecServerClient; use crate::protocol::FsCanonicalizeParams; use crate::protocol::FsCopyParams; @@ -37,124 +31,60 @@ const INVALID_REQUEST_ERROR_CODE: i64 = -32600; const NOT_FOUND_ERROR_CODE: i64 = -32004; pub(crate) struct RemoteFileSystem { - async_client: LazyRemoteExecServerClient, - sync_client: LazyRemoteExecServerClient, - sync_call_runtime: SyncCallRuntime, + client: LazyRemoteExecServerClient, } impl RemoteFileSystem { pub(crate) fn new(client: LazyRemoteExecServerClient) -> Self { trace!("remote fs new"); - Self { - async_client: client, - sync_client: client.new_connection_client(), - sync_call_runtime: SyncCallRuntime::default(), - } - } - - fn run_sync_remote_call( - &self, - operation: &'static str, - call: F, - ) -> FileSystemResult - where - T: Send + 'static, - F: FnOnce(ExecServerClient) -> Fut + Send + 'static, - Fut: Future> + Send + 'static, - { - let runtime = self.sync_call_runtime.get()?; - let client = self.sync_client.clone(); - // ExecutorFileSystem exposes synchronous path operations, so remote callers have to hop - // through a dedicated exec-server client/runtime from a helper thread. Reusing the caller - // runtime can deadlock current-thread runtimes because the JSON-RPC reader also needs that - // runtime to make progress. - thread::scope(|scope| { - scope - .spawn(move || { - runtime.block_on(async move { - let client = client.get().await.map_err(map_remote_error)?; - call(client).await - }) - }) - .join() - }) - .map_err(|_| io::Error::other(format!("remote fs {operation} thread panicked")))? - } -} - -#[derive(Default)] -struct SyncCallRuntime { - runtime: OnceLock>, -} - -impl SyncCallRuntime { - fn get(&self) -> FileSystemResult<&Runtime> { - self.runtime - .get_or_init(|| { - Builder::new_multi_thread() - .worker_threads(1) - .thread_name("codex-remote-fs") - .enable_all() - .build() - .map_err(|err| err.to_string()) - }) - .as_ref() - .map_err(|err| io::Error::other(format!("failed to start remote fs runtime: {err}"))) - } -} - -impl Drop for SyncCallRuntime { - fn drop(&mut self) { - if let Some(Ok(runtime)) = self.runtime.take() { - runtime.shutdown_background(); - } + Self { client } } } #[async_trait] impl ExecutorFileSystem for RemoteFileSystem { - fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { + async fn canonicalize( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { trace!("remote fs canonicalize"); - let path = path.clone(); - self.run_sync_remote_call("canonicalize", move |client| async move { - let response = client - .fs_canonicalize(FsCanonicalizeParams { path }) - .await - .map_err(map_remote_error)?; - Ok(response.path) - }) + let client = self.client.get().await.map_err(map_remote_error)?; + let response = client + .fs_canonicalize(FsCanonicalizeParams { + path: path.clone(), + sandbox: remote_sandbox_context(sandbox), + }) + .await + .map_err(map_remote_error)?; + Ok(response.path) } - fn join( + async fn join( &self, base_path: &AbsolutePathBuf, - relative_path: &Path, + path: &Path, ) -> FileSystemResult { trace!("remote fs join"); - let base_path = base_path.clone(); - let relative_path = relative_path.to_path_buf(); - self.run_sync_remote_call("join", move |client| async move { - let response = client - .fs_join(FsJoinParams { - base_path, - relative_path, - }) - .await - .map_err(map_remote_error)?; - Ok(response.path) - }) + let client = self.client.get().await.map_err(map_remote_error)?; + let response = client + .fs_join(FsJoinParams { + base_path: base_path.clone(), + path: path.to_path_buf(), + }) + .await + .map_err(map_remote_error)?; + Ok(response.path) } - fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { + async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { trace!("remote fs parent"); - let path = path.clone(); - self.run_sync_remote_call("parent", move |client| async move { - let response = client - .fs_parent(FsParentParams { path }) - .await - .map_err(map_remote_error)?; - Ok(response.path) - }) + let client = self.client.get().await.map_err(map_remote_error)?; + let response = client + .fs_parent(FsParentParams { path: path.clone() }) + .await + .map_err(map_remote_error)?; + Ok(response.path) } async fn read_file( @@ -163,7 +93,7 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { trace!("remote fs read_file"); - let client = self.async_client.get().await.map_err(map_remote_error)?; + let client = self.client.get().await.map_err(map_remote_error)?; let response = client .fs_read_file(FsReadFileParams { path: path.clone(), @@ -186,7 +116,7 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs write_file"); - let client = self.async_client.get().await.map_err(map_remote_error)?; + let client = self.client.get().await.map_err(map_remote_error)?; client .fs_write_file(FsWriteFileParams { path: path.clone(), @@ -205,7 +135,7 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs create_directory"); - let client = self.async_client.get().await.map_err(map_remote_error)?; + let client = self.client.get().await.map_err(map_remote_error)?; client .fs_create_directory(FsCreateDirectoryParams { path: path.clone(), @@ -223,7 +153,7 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult { trace!("remote fs get_metadata"); - let client = self.async_client.get().await.map_err(map_remote_error)?; + let client = self.client.get().await.map_err(map_remote_error)?; let response = client .fs_get_metadata(FsGetMetadataParams { path: path.clone(), @@ -246,7 +176,7 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { trace!("remote fs read_directory"); - let client = self.async_client.get().await.map_err(map_remote_error)?; + let client = self.client.get().await.map_err(map_remote_error)?; let response = client .fs_read_directory(FsReadDirectoryParams { path: path.clone(), @@ -272,7 +202,7 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs remove"); - let client = self.async_client.get().await.map_err(map_remote_error)?; + let client = self.client.get().await.map_err(map_remote_error)?; client .fs_remove(FsRemoveParams { path: path.clone(), @@ -293,7 +223,7 @@ impl ExecutorFileSystem for RemoteFileSystem { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult<()> { trace!("remote fs copy"); - let client = self.async_client.get().await.map_err(map_remote_error)?; + let client = self.client.get().await.map_err(map_remote_error)?; client .fs_copy(FsCopyParams { source_path: source_path.clone(), diff --git a/codex-rs/exec-server/src/sandboxed_file_system.rs b/codex-rs/exec-server/src/sandboxed_file_system.rs index 133a3b7327..00b6966c3b 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system.rs @@ -3,6 +3,7 @@ use base64::Engine as _; use base64::engine::general_purpose::STANDARD; use codex_app_server_protocol::JSONRPCErrorError; use codex_utils_absolute_path::AbsolutePathBuf; +use std::path::Path; use tokio::io; use crate::CopyOptions; @@ -17,6 +18,7 @@ use crate::RemoveOptions; use crate::fs_helper::FsHelperPayload; use crate::fs_helper::FsHelperRequest; use crate::fs_sandbox::FileSystemSandboxRunner; +use crate::protocol::FsCanonicalizeParams; use crate::protocol::FsCopyParams; use crate::protocol::FsCreateDirectoryParams; use crate::protocol::FsGetMetadataParams; @@ -51,6 +53,38 @@ impl SandboxedFileSystem { #[async_trait] impl ExecutorFileSystem for SandboxedFileSystem { + async fn canonicalize( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + let sandbox = require_platform_sandbox(sandbox)?; + let response = self + .run_sandboxed( + sandbox, + FsHelperRequest::Canonicalize(FsCanonicalizeParams { + path: path.clone(), + sandbox: None, + }), + ) + .await? + .expect_canonicalize() + .map_err(map_sandbox_error)?; + Ok(response.path) + } + + async fn join( + &self, + base_path: &AbsolutePathBuf, + path: &Path, + ) -> FileSystemResult { + Ok(base_path.join(path)) + } + + async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult> { + Ok(path.parent()) + } + async fn read_file( &self, path: &AbsolutePathBuf, diff --git a/codex-rs/exec-server/src/server/file_system_handler.rs b/codex-rs/exec-server/src/server/file_system_handler.rs index b3e4a5cebd..b60589d020 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -118,7 +118,8 @@ impl FileSystemHandler { ) -> Result { let path = self .file_system - .canonicalize(¶ms.path) + .canonicalize(¶ms.path, params.sandbox.as_ref()) + .await .map_err(map_fs_error)?; Ok(FsCanonicalizeResponse { path }) } @@ -129,7 +130,8 @@ impl FileSystemHandler { ) -> Result { let path = self .file_system - .join(¶ms.base_path, ¶ms.relative_path) + .join(¶ms.base_path, ¶ms.path) + .await .map_err(map_fs_error)?; Ok(FsJoinResponse { path }) } @@ -141,6 +143,7 @@ impl FileSystemHandler { let path = self .file_system .parent(¶ms.path) + .await .map_err(map_fs_error)?; Ok(FsParentResponse { path }) } diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 212c6c461e..b8d1c21894 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -372,6 +372,7 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()> &absolute_path(source_link.clone()), Path::new("nested/note.txt"), ) + .await .with_context(|| format!("mode={use_remote}"))?; assert_eq!( joined_nested, @@ -379,22 +380,26 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()> ); let joined_parent = file_system .parent(&joined_nested) + .await .with_context(|| format!("mode={use_remote}"))?; assert_eq!( joined_parent, Some(absolute_path(source_link.join("nested"))) ); - let join_error = - match file_system.join(&absolute_path(source_dir.clone()), Path::new("../outside")) { - Ok(path) => panic!( - "join should reject parent traversal, got {}", - path.display() - ), - Err(err) => err, - }; - assert_eq!(join_error.kind(), std::io::ErrorKind::InvalidInput); + let joined_parent_traversal = file_system + .join(&absolute_path(source_dir.clone()), Path::new("../outside")) + .await + .with_context(|| format!("mode={use_remote}"))?; + assert_eq!( + joined_parent_traversal, + absolute_path(source_dir.join("../outside")) + ); let canonical_nested = file_system - .canonicalize(&absolute_path(source_link.join("nested").join("note.txt"))) + .canonicalize( + &absolute_path(source_link.join("nested").join("note.txt")), + /*sandbox*/ None, + ) + .await .with_context(|| format!("mode={use_remote}"))?; assert_eq!( canonical_nested, @@ -568,6 +573,32 @@ async fn file_system_sandboxed_read_allows_readable_root(use_remote: bool) -> Re Ok(()) } +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_sandboxed_canonicalize_allows_readable_root(use_remote: bool) -> Result<()> { + let context = create_file_system_context(use_remote).await?; + let file_system = context.file_system; + + let tmp = TempDir::new()?; + let allowed_dir = tmp.path().join("allowed"); + let file_path = allowed_dir.join("note.txt"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::write(&file_path, "sandboxed hello")?; + let sandbox = read_only_sandbox(allowed_dir); + + let canonical_path = file_system + .canonicalize(&absolute_path(file_path.clone()), Some(&sandbox)) + .await + .with_context(|| format!("mode={use_remote}"))?; + assert_eq!( + canonical_path, + absolute_path(std::fs::canonicalize(file_path)?) + ); + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/codex-rs/file-system/src/lib.rs b/codex-rs/file-system/src/lib.rs index fd10908887..8fad1f5b62 100644 --- a/codex-rs/file-system/src/lib.rs +++ b/codex-rs/file-system/src/lib.rs @@ -133,43 +133,22 @@ pub type FileSystemResult = io::Result; /// a remote environment. #[async_trait] pub trait ExecutorFileSystem: Send + Sync { - /// Resolves a path within this filesystem when supported. - /// - /// Implementations that cannot resolve bound paths may return - /// [`io::ErrorKind::Unsupported`]. - fn canonicalize(&self, _path: &AbsolutePathBuf) -> FileSystemResult { - Err(io::Error::new( - io::ErrorKind::Unsupported, - "filesystem does not support canonicalization", - )) - } - - /// Lexically joins a relative path onto an existing bound path when - /// supported. - /// - /// Implementations that cannot join bound paths may return - /// [`io::ErrorKind::Unsupported`]. - fn join( + /// Resolves a path within this filesystem. + async fn canonicalize( &self, - _base_path: &AbsolutePathBuf, - _relative_path: &Path, - ) -> FileSystemResult { - Err(io::Error::new( - io::ErrorKind::Unsupported, - "filesystem does not support joining bound paths", - )) - } + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult; - /// Returns the parent directory of a bound path when supported. - /// - /// Implementations that cannot inspect bound paths may return - /// [`io::ErrorKind::Unsupported`]. - fn parent(&self, _path: &AbsolutePathBuf) -> FileSystemResult> { - Err(io::Error::new( - io::ErrorKind::Unsupported, - "filesystem does not support parent lookup for bound paths", - )) - } + /// Lexically joins a path onto an existing bound path. + async fn join( + &self, + base_path: &AbsolutePathBuf, + path: &Path, + ) -> FileSystemResult; + + /// Returns the parent directory of a bound path. + async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult>; async fn read_file( &self,