From 5ab860f8677eb0a0b4bdf7053af5c724dff8549a Mon Sep 17 00:00:00 2001 From: starr-openai Date: Fri, 29 May 2026 13:32:43 -0700 Subject: [PATCH] exec-server: canonicalize bound filesystem paths --- codex-rs/exec-server/src/client.rs | 10 ++ codex-rs/exec-server/src/environment_path.rs | 93 +++++++++++++++++++ codex-rs/exec-server/src/local_file_system.rs | 12 +++ codex-rs/exec-server/src/protocol.rs | 13 +++ .../exec-server/src/remote_file_system.rs | 25 +++++ .../src/server/file_system_handler.rs | 13 +++ codex-rs/exec-server/src/server/handler.rs | 10 ++ codex-rs/exec-server/src/server/registry.rs | 8 ++ codex-rs/exec-server/tests/file_system.rs | 12 +++ codex-rs/file-system/src/lib.rs | 11 +++ 10 files changed, 207 insertions(+) diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index e06a736c17..b362238a04 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -41,6 +41,7 @@ use crate::protocol::ExecExitedNotification; use crate::protocol::ExecOutputDeltaNotification; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; +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; @@ -48,6 +49,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; @@ -414,6 +417,13 @@ impl ExecServerClient { self.call(FS_GET_METADATA_METHOD, ¶ms).await } + pub async fn fs_canonicalize( + &self, + params: FsCanonicalizeParams, + ) -> Result { + self.call(FS_CANONICALIZE_METHOD, ¶ms).await + } + pub async fn fs_read_directory( &self, params: FsReadDirectoryParams, diff --git a/codex-rs/exec-server/src/environment_path.rs b/codex-rs/exec-server/src/environment_path.rs index 49e2cef91f..8a2995a600 100644 --- a/codex-rs/exec-server/src/environment_path.rs +++ b/codex-rs/exec-server/src/environment_path.rs @@ -2,6 +2,8 @@ use std::fmt; use std::hash::Hash; use std::hash::Hasher; use std::io; +use std::path::Component; +use std::path::Path; use std::sync::Arc; use codex_utils_absolute_path::AbsolutePathBuf; @@ -60,6 +62,13 @@ impl EnvironmentPathRef { pub fn with_path(&self, path: AbsolutePathBuf) -> Self { Self::new(Arc::clone(&self.file_system), path) } + + /// Best-effort resolves this path through its bound filesystem. + pub fn canonicalize_if_exists(&self) -> Self { + self.file_system + .canonicalize(&self.path) + .map_or_else(|_| self.clone(), |path| self.with_path(path)) + } } impl PartialEq for EnvironmentPathRef { @@ -100,6 +109,7 @@ mod tests { #[derive(Clone, Debug, Eq, PartialEq)] enum RecordedMethod { + Canonicalize, ReadFileText, Metadata, ReadDirectory, @@ -114,17 +124,26 @@ 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(), @@ -142,6 +161,21 @@ mod tests { #[async_trait] impl ExecutorFileSystem for RecordingFileSystem { + fn canonicalize(&self, path: &AbsolutePathBuf) -> 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()) + } + async fn read_file( &self, path: &AbsolutePathBuf, @@ -318,4 +352,63 @@ 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() { + 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(); + + assert_eq!(canonicalized.path(), &path.parent().unwrap()); + assert_eq!( + canonicalized, + EnvironmentPathRef::new(file_system.clone(), path.parent().unwrap()) + ); + assert_eq!( + file_system.recorded_calls(), + vec![RecordedCall { + method: RecordedMethod::Canonicalize, + path, + sandbox: None, + }] + ); + } + + #[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_literal_tilde_under_bound_path() { + let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs()); + + assert_eq!( + path_ref + .join_relative(Path::new("~")) + .map(|path_ref| path_ref.path().clone()), + Some(std::env::temp_dir().join("skills/~").abs()) + ); + } + + #[test] + fn join_relative_rejects_parent_dirs() { + let path_ref = EnvironmentPathRef::local(std::env::temp_dir().join("skills").abs()); + + assert_eq!(path_ref.join_relative(Path::new("../outside")), None); + } + + #[cfg(windows)] + #[test] + fn join_relative_rejects_windows_prefixed_and_rooted_paths() { + let path_ref = EnvironmentPathRef::local(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); + } } diff --git a/codex-rs/exec-server/src/local_file_system.rs b/codex-rs/exec-server/src/local_file_system.rs index c7d628ce8f..dd09c9950d 100644 --- a/codex-rs/exec-server/src/local_file_system.rs +++ b/codex-rs/exec-server/src/local_file_system.rs @@ -79,6 +79,10 @@ impl LocalFileSystem { #[async_trait] impl ExecutorFileSystem for LocalFileSystem { + fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { + self.unsandboxed.canonicalize(path) + } + async fn read_file( &self, path: &AbsolutePathBuf, @@ -152,6 +156,10 @@ impl ExecutorFileSystem for LocalFileSystem { #[async_trait] impl ExecutorFileSystem for UnsandboxedFileSystem { + fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { + self.file_system.canonicalize(path) + } + async fn read_file( &self, path: &AbsolutePathBuf, @@ -238,6 +246,10 @@ impl ExecutorFileSystem for UnsandboxedFileSystem { #[async_trait] impl ExecutorFileSystem for DirectFileSystem { + fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { + path.canonicalize() + } + async fn read_file( &self, path: &AbsolutePathBuf, diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index e801a7f437..2de5c423f3 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -23,6 +23,7 @@ pub const FS_READ_FILE_METHOD: &str = "fs/readFile"; pub const FS_WRITE_FILE_METHOD: &str = "fs/writeFile"; pub const FS_CREATE_DIRECTORY_METHOD: &str = "fs/createDirectory"; pub const FS_GET_METADATA_METHOD: &str = "fs/getMetadata"; +pub const FS_CANONICALIZE_METHOD: &str = "fs/canonicalize"; pub const FS_READ_DIRECTORY_METHOD: &str = "fs/readDirectory"; pub const FS_REMOVE_METHOD: &str = "fs/remove"; pub const FS_COPY_METHOD: &str = "fs/copy"; @@ -211,6 +212,18 @@ pub struct FsGetMetadataResponse { pub modified_at_ms: i64, } +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsCanonicalizeParams { + pub path: AbsolutePathBuf, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsCanonicalizeResponse { + pub path: AbsolutePathBuf, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct FsReadDirectoryParams { diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index 4251fa33ec..1ad0769bdf 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -2,6 +2,7 @@ use async_trait::async_trait; use base64::Engine as _; use base64::engine::general_purpose::STANDARD; use codex_utils_absolute_path::AbsolutePathBuf; +use std::thread; use tokio::io; use tracing::trace; @@ -15,6 +16,7 @@ use crate::FileSystemSandboxContext; use crate::ReadDirectoryEntry; use crate::RemoveOptions; use crate::client::LazyRemoteExecServerClient; +use crate::protocol::FsCanonicalizeParams; use crate::protocol::FsCopyParams; use crate::protocol::FsCreateDirectoryParams; use crate::protocol::FsGetMetadataParams; @@ -40,6 +42,29 @@ impl RemoteFileSystem { #[async_trait] impl ExecutorFileSystem for RemoteFileSystem { + fn canonicalize(&self, path: &AbsolutePathBuf) -> FileSystemResult { + trace!("remote fs canonicalize"); + let runtime = tokio::runtime::Handle::try_current().map_err(|err| { + io::Error::other(format!("remote fs canonicalize requires runtime: {err}")) + })?; + let client = self.client.clone(); + let path = path.clone(); + // ExecutorFileSystem::canonicalize is synchronous, so remote callers have to hop through + // the active runtime from a helper thread to reach the JSON-RPC filesystem surface. + thread::spawn(move || { + runtime.block_on(async move { + let client = client.get().await.map_err(map_remote_error)?; + let response = client + .fs_canonicalize(FsCanonicalizeParams { path }) + .await + .map_err(map_remote_error)?; + Ok(response.path) + }) + }) + .join() + .map_err(|_| io::Error::other("remote fs canonicalize thread panicked"))? + } + 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 bef30e975f..473ecb7843 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -11,6 +11,8 @@ use crate::ExecutorFileSystem; use crate::RemoveOptions; use crate::local_file_system::LocalFileSystem; 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; @@ -106,6 +108,17 @@ impl FileSystemHandler { }) } + pub(crate) async fn canonicalize( + &self, + params: FsCanonicalizeParams, + ) -> Result { + let path = self + .file_system + .canonicalize(¶ms.path) + .map_err(map_fs_error)?; + Ok(FsCanonicalizeResponse { path }) + } + pub(crate) async fn read_directory( &self, params: FsReadDirectoryParams, diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index d0645724c4..3d4849329c 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -16,6 +16,8 @@ use crate::client::http_client::PendingReqwestHttpBodyStream; use crate::client::http_client::ReqwestHttpRequestRunner; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; +use crate::protocol::FsCanonicalizeParams; +use crate::protocol::FsCanonicalizeResponse; use crate::protocol::FsCopyParams; use crate::protocol::FsCopyResponse; use crate::protocol::FsCreateDirectoryParams; @@ -240,6 +242,14 @@ impl ExecServerHandler { self.file_system.get_metadata(params).await } + pub(crate) async fn fs_canonicalize( + &self, + params: FsCanonicalizeParams, + ) -> Result { + self.require_initialized_for("filesystem")?; + self.file_system.canonicalize(params).await + } + pub(crate) async fn fs_read_directory( &self, params: FsReadDirectoryParams, diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index 87dee6aa58..44d0562382 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -5,6 +5,7 @@ use crate::protocol::EXEC_READ_METHOD; use crate::protocol::EXEC_TERMINATE_METHOD; use crate::protocol::EXEC_WRITE_METHOD; use crate::protocol::ExecParams; +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; @@ -12,6 +13,7 @@ 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::FsCopyParams; use crate::protocol::FsCreateDirectoryParams; use crate::protocol::FsGetMetadataParams; @@ -96,6 +98,12 @@ pub(crate) fn build_router() -> RpcRouter { handler.fs_get_metadata(params).await }, ); + router.request( + FS_CANONICALIZE_METHOD, + |handler: Arc, params: FsCanonicalizeParams| async move { + handler.fs_canonicalize(params).await + }, + ); router.request( FS_READ_DIRECTORY_METHOD, |handler: Arc, params: FsReadDirectoryParams| async move { diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 3da796df24..2e59308065 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -365,6 +365,18 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()> .await .with_context(|| format!("mode={use_remote}"))?; + let source_link = tmp.path().join("source-link"); + symlink(&source_dir, &source_link)?; + let canonical_nested = file_system + .canonicalize(&absolute_path(source_link.join("nested").join("note.txt"))) + .with_context(|| format!("mode={use_remote}"))?; + assert_eq!( + canonical_nested, + absolute_path(std::fs::canonicalize( + source_dir.join("nested").join("note.txt") + )?) + ); + let nested_file_contents = file_system .read_file(&absolute_path(nested_file.clone()), /*sandbox*/ None) .await diff --git a/codex-rs/file-system/src/lib.rs b/codex-rs/file-system/src/lib.rs index 606f987cbd..1d7ef33d3c 100644 --- a/codex-rs/file-system/src/lib.rs +++ b/codex-rs/file-system/src/lib.rs @@ -133,6 +133,17 @@ 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", + )) + } + async fn read_file( &self, path: &AbsolutePathBuf,