From f383cc980d141cdb9b208a2b08413f6e62b76b97 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Wed, 8 Apr 2026 12:10:48 -0700 Subject: [PATCH] Add sandbox support to filesystem APIs (#16751) ## Summary - add optional `sandboxPolicy` support to the app-server filesystem request surface - thread sandbox-aware filesystem options through app-server and exec-server adapters - enforce sandboxed read/write access in the filesystem abstraction with focused local and remote coverage ## Validation - `cargo test -p codex-app-server-protocol` - `cargo test -p codex-exec-server file_system` - `cargo test -p codex-app-server suite::v2::fs` --------- Co-authored-by: Codex --- codex-rs/Cargo.lock | 1 + codex-rs/exec-server/Cargo.toml | 1 + codex-rs/exec-server/src/client.rs | 28 +- codex-rs/exec-server/src/file_system.rs | 48 ++ codex-rs/exec-server/src/lib.rs | 29 +- codex-rs/exec-server/src/local_file_system.rs | 351 +++++++++++- codex-rs/exec-server/src/protocol.rs | 102 ++++ .../exec-server/src/remote_file_system.rs | 178 +++++- .../src/server/file_system_handler.rs | 47 +- codex-rs/exec-server/src/server/handler.rs | 28 +- codex-rs/exec-server/src/server/registry.rs | 14 +- codex-rs/exec-server/tests/file_system.rs | 540 ++++++++++++++++++ 12 files changed, 1271 insertions(+), 96 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 0693c73b9..b552cc796 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2094,6 +2094,7 @@ dependencies = [ "base64 0.22.1", "clap", "codex-app-server-protocol", + "codex-protocol", "codex-utils-absolute-path", "codex-utils-cargo-bin", "codex-utils-pty", diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index f064a82dc..41d65cf3d 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -20,6 +20,7 @@ async-trait = { workspace = true } base64 = { workspace = true } clap = { workspace = true, features = ["derive"] } codex-app-server-protocol = { workspace = true } +codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-pty = { workspace = true } futures = { workspace = true } diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index d73f6cf95..3121d4ce2 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -3,20 +3,6 @@ use std::sync::Arc; use std::time::Duration; use arc_swap::ArcSwap; -use codex_app_server_protocol::FsCopyParams; -use codex_app_server_protocol::FsCopyResponse; -use codex_app_server_protocol::FsCreateDirectoryParams; -use codex_app_server_protocol::FsCreateDirectoryResponse; -use codex_app_server_protocol::FsGetMetadataParams; -use codex_app_server_protocol::FsGetMetadataResponse; -use codex_app_server_protocol::FsReadDirectoryParams; -use codex_app_server_protocol::FsReadDirectoryResponse; -use codex_app_server_protocol::FsReadFileParams; -use codex_app_server_protocol::FsReadFileResponse; -use codex_app_server_protocol::FsRemoveParams; -use codex_app_server_protocol::FsRemoveResponse; -use codex_app_server_protocol::FsWriteFileParams; -use codex_app_server_protocol::FsWriteFileResponse; use codex_app_server_protocol::JSONRPCNotification; use serde_json::Value; use tokio::sync::Mutex; @@ -49,6 +35,20 @@ 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::FsCopyParams; +use crate::protocol::FsCopyResponse; +use crate::protocol::FsCreateDirectoryParams; +use crate::protocol::FsCreateDirectoryResponse; +use crate::protocol::FsGetMetadataParams; +use crate::protocol::FsGetMetadataResponse; +use crate::protocol::FsReadDirectoryParams; +use crate::protocol::FsReadDirectoryResponse; +use crate::protocol::FsReadFileParams; +use crate::protocol::FsReadFileResponse; +use crate::protocol::FsRemoveParams; +use crate::protocol::FsRemoveResponse; +use crate::protocol::FsWriteFileParams; +use crate::protocol::FsWriteFileResponse; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index b04b48089..8e68e1ab0 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -1,4 +1,5 @@ use async_trait::async_trait; +use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use tokio::io; @@ -45,27 +46,74 @@ pub trait ExecutorFileSystem: Send + Sync { String::from_utf8(bytes).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)) } + async fn read_file_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult>; + async fn write_file(&self, path: &AbsolutePathBuf, contents: Vec) -> FileSystemResult<()>; + async fn write_file_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + contents: Vec, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()>; + async fn create_directory( &self, path: &AbsolutePathBuf, options: CreateDirectoryOptions, ) -> FileSystemResult<()>; + async fn create_directory_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + create_directory_options: CreateDirectoryOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()>; + async fn get_metadata(&self, path: &AbsolutePathBuf) -> FileSystemResult; + async fn get_metadata_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult; + async fn read_directory( &self, path: &AbsolutePathBuf, ) -> FileSystemResult>; + async fn read_directory_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult>; + async fn remove(&self, path: &AbsolutePathBuf, options: RemoveOptions) -> FileSystemResult<()>; + async fn remove_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + remove_options: RemoveOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()>; + async fn copy( &self, source_path: &AbsolutePathBuf, destination_path: &AbsolutePathBuf, options: CopyOptions, ) -> FileSystemResult<()>; + + async fn copy_with_sandbox_policy( + &self, + source_path: &AbsolutePathBuf, + destination_path: &AbsolutePathBuf, + copy_options: CopyOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()>; } diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 88378da9e..eccb9c91b 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -17,20 +17,6 @@ pub use client::ExecServerClient; pub use client::ExecServerError; pub use client_api::ExecServerClientConnectOptions; pub use client_api::RemoteExecServerConnectArgs; -pub use codex_app_server_protocol::FsCopyParams; -pub use codex_app_server_protocol::FsCopyResponse; -pub use codex_app_server_protocol::FsCreateDirectoryParams; -pub use codex_app_server_protocol::FsCreateDirectoryResponse; -pub use codex_app_server_protocol::FsGetMetadataParams; -pub use codex_app_server_protocol::FsGetMetadataResponse; -pub use codex_app_server_protocol::FsReadDirectoryParams; -pub use codex_app_server_protocol::FsReadDirectoryResponse; -pub use codex_app_server_protocol::FsReadFileParams; -pub use codex_app_server_protocol::FsReadFileResponse; -pub use codex_app_server_protocol::FsRemoveParams; -pub use codex_app_server_protocol::FsRemoveResponse; -pub use codex_app_server_protocol::FsWriteFileParams; -pub use codex_app_server_protocol::FsWriteFileResponse; pub use environment::CODEX_EXEC_SERVER_URL_ENV_VAR; pub use environment::Environment; pub use environment::EnvironmentManager; @@ -52,6 +38,21 @@ pub use protocol::ExecOutputDeltaNotification; pub use protocol::ExecOutputStream; pub use protocol::ExecParams; pub use protocol::ExecResponse; +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::FsReadDirectoryEntry; +pub use protocol::FsReadDirectoryParams; +pub use protocol::FsReadDirectoryResponse; +pub use protocol::FsReadFileParams; +pub use protocol::FsReadFileResponse; +pub use protocol::FsRemoveParams; +pub use protocol::FsRemoveResponse; +pub use protocol::FsWriteFileParams; +pub use protocol::FsWriteFileResponse; pub use protocol::InitializeParams; pub use protocol::InitializeResponse; pub use protocol::ReadParams; diff --git a/codex-rs/exec-server/src/local_file_system.rs b/codex-rs/exec-server/src/local_file_system.rs index 69ab9d64b..10df5faa3 100644 --- a/codex-rs/exec-server/src/local_file_system.rs +++ b/codex-rs/exec-server/src/local_file_system.rs @@ -1,6 +1,8 @@ use async_trait::async_trait; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; -use std::path::Component; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -38,10 +40,29 @@ impl ExecutorFileSystem for LocalFileSystem { tokio::fs::read(path.as_path()).await } + async fn read_file_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult> { + enforce_read_access(path, sandbox_policy)?; + self.read_file(path).await + } + async fn write_file(&self, path: &AbsolutePathBuf, contents: Vec) -> FileSystemResult<()> { tokio::fs::write(path.as_path(), contents).await } + async fn write_file_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + contents: Vec, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + enforce_write_access(path, sandbox_policy)?; + self.write_file(path, contents).await + } + async fn create_directory( &self, path: &AbsolutePathBuf, @@ -55,6 +76,16 @@ impl ExecutorFileSystem for LocalFileSystem { Ok(()) } + async fn create_directory_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + create_directory_options: CreateDirectoryOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + enforce_write_access(path, sandbox_policy)?; + self.create_directory(path, create_directory_options).await + } + async fn get_metadata(&self, path: &AbsolutePathBuf) -> FileSystemResult { let metadata = tokio::fs::metadata(path.as_path()).await?; Ok(FileMetadata { @@ -65,6 +96,15 @@ impl ExecutorFileSystem for LocalFileSystem { }) } + async fn get_metadata_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult { + enforce_read_access(path, sandbox_policy)?; + self.get_metadata(path).await + } + async fn read_directory( &self, path: &AbsolutePathBuf, @@ -82,6 +122,15 @@ impl ExecutorFileSystem for LocalFileSystem { Ok(entries) } + async fn read_directory_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult> { + enforce_read_access(path, sandbox_policy)?; + self.read_directory(path).await + } + async fn remove(&self, path: &AbsolutePathBuf, options: RemoveOptions) -> FileSystemResult<()> { match tokio::fs::symlink_metadata(path.as_path()).await { Ok(metadata) => { @@ -102,6 +151,16 @@ impl ExecutorFileSystem for LocalFileSystem { } } + async fn remove_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + remove_options: RemoveOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + enforce_write_access_preserving_leaf(path, sandbox_policy)?; + self.remove(path, remove_options).await + } + async fn copy( &self, source_path: &AbsolutePathBuf, @@ -152,6 +211,164 @@ impl ExecutorFileSystem for LocalFileSystem { .await .map_err(|err| io::Error::other(format!("filesystem task failed: {err}")))? } + + async fn copy_with_sandbox_policy( + &self, + source_path: &AbsolutePathBuf, + destination_path: &AbsolutePathBuf, + copy_options: CopyOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + enforce_copy_source_read_access(source_path, sandbox_policy)?; + enforce_write_access(destination_path, sandbox_policy)?; + self.copy(source_path, destination_path, copy_options).await + } +} + +fn enforce_read_access( + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, +) -> FileSystemResult<()> { + enforce_access_for_current_dir( + path, + sandbox_policy, + FileSystemSandboxPolicy::can_read_path_with_cwd, + "read", + AccessPathMode::ResolveAll, + ) +} + +fn enforce_write_access( + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, +) -> FileSystemResult<()> { + enforce_access_for_current_dir( + path, + sandbox_policy, + FileSystemSandboxPolicy::can_write_path_with_cwd, + "write", + AccessPathMode::ResolveAll, + ) +} + +fn enforce_write_access_preserving_leaf( + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, +) -> FileSystemResult<()> { + enforce_access_for_current_dir( + path, + sandbox_policy, + FileSystemSandboxPolicy::can_write_path_with_cwd, + "write", + AccessPathMode::PreserveLeaf, + ) +} + +fn enforce_copy_source_read_access( + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, +) -> FileSystemResult<()> { + let path_mode = match std::fs::symlink_metadata(path.as_path()) { + Ok(metadata) if metadata.file_type().is_symlink() => AccessPathMode::PreserveLeaf, + _ => AccessPathMode::ResolveAll, + }; + enforce_access_for_current_dir( + path, + sandbox_policy, + FileSystemSandboxPolicy::can_read_path_with_cwd, + "read", + path_mode, + ) +} + +#[cfg(all(test, unix))] +fn enforce_read_access_for_cwd( + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + sandbox_cwd: &AbsolutePathBuf, +) -> FileSystemResult<()> { + enforce_access_for_cwd( + path, + sandbox_policy, + sandbox_cwd, + FileSystemSandboxPolicy::can_read_path_with_cwd, + "read", + AccessPathMode::ResolveAll, + ) +} + +fn enforce_access_for_current_dir( + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + is_allowed: fn(&FileSystemSandboxPolicy, &Path, &Path) -> bool, + access_kind: &str, + path_mode: AccessPathMode, +) -> FileSystemResult<()> { + let Some(sandbox_policy) = sandbox_policy else { + return Ok(()); + }; + let cwd = current_sandbox_cwd()?; + enforce_access( + path, + sandbox_policy, + cwd.as_path(), + is_allowed, + access_kind, + path_mode, + ) +} + +#[cfg(all(test, unix))] +fn enforce_access_for_cwd( + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + sandbox_cwd: &AbsolutePathBuf, + is_allowed: fn(&FileSystemSandboxPolicy, &Path, &Path) -> bool, + access_kind: &str, + path_mode: AccessPathMode, +) -> FileSystemResult<()> { + let Some(sandbox_policy) = sandbox_policy else { + return Ok(()); + }; + let cwd = resolve_existing_path(sandbox_cwd.as_path())?; + enforce_access( + path, + sandbox_policy, + cwd.as_path(), + is_allowed, + access_kind, + path_mode, + ) +} + +fn enforce_access( + path: &AbsolutePathBuf, + sandbox_policy: &SandboxPolicy, + sandbox_cwd: &Path, + is_allowed: fn(&FileSystemSandboxPolicy, &Path, &Path) -> bool, + access_kind: &str, + path_mode: AccessPathMode, +) -> FileSystemResult<()> { + let resolved_path = resolve_path_for_access_check(path.as_path(), path_mode)?; + let file_system_policy = + canonicalize_file_system_policy_paths(FileSystemSandboxPolicy::from(sandbox_policy))?; + if is_allowed(&file_system_policy, resolved_path.as_path(), sandbox_cwd) { + Ok(()) + } else { + Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "fs/{access_kind} is not permitted for path {}", + path.as_path().display() + ), + )) + } +} + +#[derive(Clone, Copy)] +enum AccessPathMode { + ResolveAll, + PreserveLeaf, } fn copy_dir_recursive(source: &Path, target: &Path) -> io::Result<()> { @@ -178,26 +395,30 @@ fn destination_is_same_or_descendant_of_source( destination: &Path, ) -> io::Result { let source = std::fs::canonicalize(source)?; - let destination = resolve_copy_destination_path(destination)?; + let destination = resolve_path_for_access_check(destination, AccessPathMode::ResolveAll)?; Ok(destination.starts_with(&source)) } -fn resolve_copy_destination_path(path: &Path) -> io::Result { - let mut normalized = PathBuf::new(); - for component in path.components() { - match component { - Component::Prefix(prefix) => normalized.push(prefix.as_os_str()), - Component::RootDir => normalized.push(component.as_os_str()), - Component::CurDir => {} - Component::ParentDir => { - normalized.pop(); - } - Component::Normal(part) => normalized.push(part), - } +fn resolve_path_for_access_check(path: &Path, path_mode: AccessPathMode) -> io::Result { + match path_mode { + AccessPathMode::ResolveAll => resolve_existing_path(path), + AccessPathMode::PreserveLeaf => preserve_leaf_path_for_access_check(path), } +} +fn preserve_leaf_path_for_access_check(path: &Path) -> io::Result { + let Some(file_name) = path.file_name() else { + return resolve_existing_path(path); + }; + let parent = path.parent().unwrap_or_else(|| Path::new("/")); + let mut resolved_parent = resolve_existing_path(parent)?; + resolved_parent.push(file_name); + Ok(resolved_parent) +} + +fn resolve_existing_path(path: &Path) -> io::Result { let mut unresolved_suffix = Vec::new(); - let mut existing_path = normalized.as_path(); + let mut existing_path = path; while !existing_path.exists() { let Some(file_name) = existing_path.file_name() else { break; @@ -216,6 +437,33 @@ fn resolve_copy_destination_path(path: &Path) -> io::Result { Ok(resolved) } +fn current_sandbox_cwd() -> io::Result { + let cwd = std::env::current_dir() + .map_err(|err| io::Error::other(format!("failed to read current dir: {err}")))?; + resolve_existing_path(cwd.as_path()) +} + +fn canonicalize_file_system_policy_paths( + mut file_system_policy: FileSystemSandboxPolicy, +) -> io::Result { + for entry in &mut file_system_policy.entries { + if let FileSystemPath::Path { path } = &mut entry.path { + *path = canonicalize_absolute_path(path)?; + } + } + Ok(file_system_policy) +} + +fn canonicalize_absolute_path(path: &AbsolutePathBuf) -> io::Result { + let resolved = resolve_existing_path(path.as_path())?; + AbsolutePathBuf::from_absolute_path(resolved.as_path()).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("path must stay absolute after canonicalization: {err}"), + ) + }) +} + fn copy_symlink(source: &Path, target: &Path) -> io::Result<()> { let link_target = std::fs::read_link(source)?; #[cfg(unix)] @@ -257,6 +505,79 @@ fn system_time_to_unix_ms(time: SystemTime) -> i64 { .unwrap_or(0) } +#[cfg(all(test, unix))] +mod tests { + use super::*; + use codex_protocol::protocol::ReadOnlyAccess; + use pretty_assertions::assert_eq; + use std::os::unix::fs::symlink; + + fn absolute_path(path: PathBuf) -> AbsolutePathBuf { + match AbsolutePathBuf::try_from(path) { + Ok(path) => path, + Err(err) => panic!("absolute path: {err}"), + } + } + + fn read_only_sandbox_policy(readable_roots: Vec) -> SandboxPolicy { + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: readable_roots.into_iter().map(absolute_path).collect(), + }, + network_access: false, + } + } + + #[test] + fn resolve_path_for_access_check_rejects_symlink_parent_dotdot_escape() -> io::Result<()> { + let temp_dir = tempfile::TempDir::new()?; + let allowed_dir = temp_dir.path().join("allowed"); + let outside_dir = temp_dir.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let resolved = resolve_path_for_access_check( + allowed_dir + .join("link") + .join("..") + .join("secret.txt") + .as_path(), + AccessPathMode::ResolveAll, + )?; + + assert_eq!( + resolved, + resolve_existing_path(temp_dir.path())?.join("secret.txt") + ); + Ok(()) + } + + #[test] + fn enforce_read_access_uses_explicit_sandbox_cwd() -> io::Result<()> { + let temp_dir = tempfile::TempDir::new()?; + let workspace_dir = temp_dir.path().join("workspace"); + let other_dir = temp_dir.path().join("other"); + let note_path = workspace_dir.join("note.txt"); + std::fs::create_dir_all(&workspace_dir)?; + std::fs::create_dir_all(&other_dir)?; + std::fs::write(¬e_path, "hello")?; + + let sandbox_policy = read_only_sandbox_policy(vec![]); + let sandbox_cwd = absolute_path(workspace_dir); + let other_cwd = absolute_path(other_dir); + let note_path = absolute_path(note_path); + + enforce_read_access_for_cwd(¬e_path, Some(&sandbox_policy), &sandbox_cwd)?; + + let error = enforce_read_access_for_cwd(¬e_path, Some(&sandbox_policy), &other_cwd) + .expect_err("read should be rejected outside provided cwd"); + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + Ok(()) + } +} + #[cfg(all(test, windows))] mod tests { use super::*; diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 0a0d0a4e8..8a61a9de1 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; use std::path::PathBuf; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; +use codex_protocol::protocol::SandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; @@ -131,6 +133,106 @@ pub struct TerminateResponse { pub running: bool, } +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsReadFileParams { + pub path: AbsolutePathBuf, + pub sandbox_policy: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsReadFileResponse { + pub data_base64: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsWriteFileParams { + pub path: AbsolutePathBuf, + pub data_base64: String, + pub sandbox_policy: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsWriteFileResponse {} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsCreateDirectoryParams { + pub path: AbsolutePathBuf, + pub recursive: Option, + pub sandbox_policy: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsCreateDirectoryResponse {} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsGetMetadataParams { + pub path: AbsolutePathBuf, + pub sandbox_policy: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsGetMetadataResponse { + pub is_directory: bool, + pub is_file: bool, + pub created_at_ms: i64, + pub modified_at_ms: i64, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsReadDirectoryParams { + pub path: AbsolutePathBuf, + pub sandbox_policy: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsReadDirectoryEntry { + pub file_name: String, + pub is_directory: bool, + pub is_file: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsReadDirectoryResponse { + pub entries: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsRemoveParams { + pub path: AbsolutePathBuf, + pub recursive: Option, + pub force: Option, + pub sandbox_policy: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsRemoveResponse {} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsCopyParams { + pub source_path: AbsolutePathBuf, + pub destination_path: AbsolutePathBuf, + pub recursive: bool, + pub sandbox_policy: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct FsCopyResponse {} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum ExecOutputStream { diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index 7da10b0e5..c92b460a8 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -1,13 +1,7 @@ use async_trait::async_trait; use base64::Engine as _; use base64::engine::general_purpose::STANDARD; -use codex_app_server_protocol::FsCopyParams; -use codex_app_server_protocol::FsCreateDirectoryParams; -use codex_app_server_protocol::FsGetMetadataParams; -use codex_app_server_protocol::FsReadDirectoryParams; -use codex_app_server_protocol::FsReadFileParams; -use codex_app_server_protocol::FsRemoveParams; -use codex_app_server_protocol::FsWriteFileParams; +use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use tokio::io; use tracing::trace; @@ -21,6 +15,13 @@ use crate::FileMetadata; use crate::FileSystemResult; use crate::ReadDirectoryEntry; use crate::RemoveOptions; +use crate::protocol::FsCopyParams; +use crate::protocol::FsCreateDirectoryParams; +use crate::protocol::FsGetMetadataParams; +use crate::protocol::FsReadDirectoryParams; +use crate::protocol::FsReadFileParams; +use crate::protocol::FsRemoveParams; +use crate::protocol::FsWriteFileParams; const INVALID_REQUEST_ERROR_CODE: i64 = -32600; const NOT_FOUND_ERROR_CODE: i64 = -32004; @@ -43,7 +44,32 @@ impl ExecutorFileSystem for RemoteFileSystem { trace!("remote fs read_file"); let response = self .client - .fs_read_file(FsReadFileParams { path: path.clone() }) + .fs_read_file(FsReadFileParams { + path: path.clone(), + sandbox_policy: None, + }) + .await + .map_err(map_remote_error)?; + STANDARD.decode(response.data_base64).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("remote fs/readFile returned invalid base64 dataBase64: {err}"), + ) + }) + } + + async fn read_file_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult> { + trace!("remote fs read_file_with_sandbox_policy"); + let response = self + .client + .fs_read_file(FsReadFileParams { + path: path.clone(), + sandbox_policy: sandbox_policy.cloned(), + }) .await .map_err(map_remote_error)?; STANDARD.decode(response.data_base64).map_err(|err| { @@ -60,6 +86,25 @@ impl ExecutorFileSystem for RemoteFileSystem { .fs_write_file(FsWriteFileParams { path: path.clone(), data_base64: STANDARD.encode(contents), + sandbox_policy: None, + }) + .await + .map_err(map_remote_error)?; + Ok(()) + } + + async fn write_file_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + contents: Vec, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + trace!("remote fs write_file_with_sandbox_policy"); + self.client + .fs_write_file(FsWriteFileParams { + path: path.clone(), + data_base64: STANDARD.encode(contents), + sandbox_policy: sandbox_policy.cloned(), }) .await .map_err(map_remote_error)?; @@ -76,6 +121,25 @@ impl ExecutorFileSystem for RemoteFileSystem { .fs_create_directory(FsCreateDirectoryParams { path: path.clone(), recursive: Some(options.recursive), + sandbox_policy: None, + }) + .await + .map_err(map_remote_error)?; + Ok(()) + } + + async fn create_directory_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + create_directory_options: CreateDirectoryOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + trace!("remote fs create_directory_with_sandbox_policy"); + self.client + .fs_create_directory(FsCreateDirectoryParams { + path: path.clone(), + recursive: Some(create_directory_options.recursive), + sandbox_policy: sandbox_policy.cloned(), }) .await .map_err(map_remote_error)?; @@ -86,7 +150,32 @@ impl ExecutorFileSystem for RemoteFileSystem { trace!("remote fs get_metadata"); let response = self .client - .fs_get_metadata(FsGetMetadataParams { path: path.clone() }) + .fs_get_metadata(FsGetMetadataParams { + path: path.clone(), + sandbox_policy: None, + }) + .await + .map_err(map_remote_error)?; + Ok(FileMetadata { + is_directory: response.is_directory, + is_file: response.is_file, + created_at_ms: response.created_at_ms, + modified_at_ms: response.modified_at_ms, + }) + } + + async fn get_metadata_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult { + trace!("remote fs get_metadata_with_sandbox_policy"); + let response = self + .client + .fs_get_metadata(FsGetMetadataParams { + path: path.clone(), + sandbox_policy: sandbox_policy.cloned(), + }) .await .map_err(map_remote_error)?; Ok(FileMetadata { @@ -104,7 +193,35 @@ impl ExecutorFileSystem for RemoteFileSystem { trace!("remote fs read_directory"); let response = self .client - .fs_read_directory(FsReadDirectoryParams { path: path.clone() }) + .fs_read_directory(FsReadDirectoryParams { + path: path.clone(), + sandbox_policy: None, + }) + .await + .map_err(map_remote_error)?; + Ok(response + .entries + .into_iter() + .map(|entry| ReadDirectoryEntry { + file_name: entry.file_name, + is_directory: entry.is_directory, + is_file: entry.is_file, + }) + .collect()) + } + + async fn read_directory_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult> { + trace!("remote fs read_directory_with_sandbox_policy"); + let response = self + .client + .fs_read_directory(FsReadDirectoryParams { + path: path.clone(), + sandbox_policy: sandbox_policy.cloned(), + }) .await .map_err(map_remote_error)?; Ok(response @@ -125,6 +242,26 @@ impl ExecutorFileSystem for RemoteFileSystem { path: path.clone(), recursive: Some(options.recursive), force: Some(options.force), + sandbox_policy: None, + }) + .await + .map_err(map_remote_error)?; + Ok(()) + } + + async fn remove_with_sandbox_policy( + &self, + path: &AbsolutePathBuf, + remove_options: RemoveOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + trace!("remote fs remove_with_sandbox_policy"); + self.client + .fs_remove(FsRemoveParams { + path: path.clone(), + recursive: Some(remove_options.recursive), + force: Some(remove_options.force), + sandbox_policy: sandbox_policy.cloned(), }) .await .map_err(map_remote_error)?; @@ -143,6 +280,27 @@ impl ExecutorFileSystem for RemoteFileSystem { source_path: source_path.clone(), destination_path: destination_path.clone(), recursive: options.recursive, + sandbox_policy: None, + }) + .await + .map_err(map_remote_error)?; + Ok(()) + } + + async fn copy_with_sandbox_policy( + &self, + source_path: &AbsolutePathBuf, + destination_path: &AbsolutePathBuf, + copy_options: CopyOptions, + sandbox_policy: Option<&SandboxPolicy>, + ) -> FileSystemResult<()> { + trace!("remote fs copy_with_sandbox_policy"); + self.client + .fs_copy(FsCopyParams { + source_path: source_path.clone(), + destination_path: destination_path.clone(), + recursive: copy_options.recursive, + sandbox_policy: sandbox_policy.cloned(), }) .await .map_err(map_remote_error)?; 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 2f6c679dc..84dbd4f29 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -2,21 +2,6 @@ use std::io; use base64::Engine as _; use base64::engine::general_purpose::STANDARD; -use codex_app_server_protocol::FsCopyParams; -use codex_app_server_protocol::FsCopyResponse; -use codex_app_server_protocol::FsCreateDirectoryParams; -use codex_app_server_protocol::FsCreateDirectoryResponse; -use codex_app_server_protocol::FsGetMetadataParams; -use codex_app_server_protocol::FsGetMetadataResponse; -use codex_app_server_protocol::FsReadDirectoryEntry; -use codex_app_server_protocol::FsReadDirectoryParams; -use codex_app_server_protocol::FsReadDirectoryResponse; -use codex_app_server_protocol::FsReadFileParams; -use codex_app_server_protocol::FsReadFileResponse; -use codex_app_server_protocol::FsRemoveParams; -use codex_app_server_protocol::FsRemoveResponse; -use codex_app_server_protocol::FsWriteFileParams; -use codex_app_server_protocol::FsWriteFileResponse; use codex_app_server_protocol::JSONRPCErrorError; use crate::CopyOptions; @@ -24,6 +9,21 @@ use crate::CreateDirectoryOptions; use crate::ExecutorFileSystem; use crate::RemoveOptions; use crate::local_file_system::LocalFileSystem; +use crate::protocol::FsCopyParams; +use crate::protocol::FsCopyResponse; +use crate::protocol::FsCreateDirectoryParams; +use crate::protocol::FsCreateDirectoryResponse; +use crate::protocol::FsGetMetadataParams; +use crate::protocol::FsGetMetadataResponse; +use crate::protocol::FsReadDirectoryEntry; +use crate::protocol::FsReadDirectoryParams; +use crate::protocol::FsReadDirectoryResponse; +use crate::protocol::FsReadFileParams; +use crate::protocol::FsReadFileResponse; +use crate::protocol::FsRemoveParams; +use crate::protocol::FsRemoveResponse; +use crate::protocol::FsWriteFileParams; +use crate::protocol::FsWriteFileResponse; use crate::rpc::internal_error; use crate::rpc::invalid_request; use crate::rpc::not_found; @@ -40,7 +40,7 @@ impl FileSystemHandler { ) -> Result { let bytes = self .file_system - .read_file(¶ms.path) + .read_file_with_sandbox_policy(¶ms.path, params.sandbox_policy.as_ref()) .await .map_err(map_fs_error)?; Ok(FsReadFileResponse { @@ -58,7 +58,7 @@ impl FileSystemHandler { )) })?; self.file_system - .write_file(¶ms.path, bytes) + .write_file_with_sandbox_policy(¶ms.path, bytes, params.sandbox_policy.as_ref()) .await .map_err(map_fs_error)?; Ok(FsWriteFileResponse {}) @@ -69,11 +69,12 @@ impl FileSystemHandler { params: FsCreateDirectoryParams, ) -> Result { self.file_system - .create_directory( + .create_directory_with_sandbox_policy( ¶ms.path, CreateDirectoryOptions { recursive: params.recursive.unwrap_or(true), }, + params.sandbox_policy.as_ref(), ) .await .map_err(map_fs_error)?; @@ -86,7 +87,7 @@ impl FileSystemHandler { ) -> Result { let metadata = self .file_system - .get_metadata(¶ms.path) + .get_metadata_with_sandbox_policy(¶ms.path, params.sandbox_policy.as_ref()) .await .map_err(map_fs_error)?; Ok(FsGetMetadataResponse { @@ -103,7 +104,7 @@ impl FileSystemHandler { ) -> Result { let entries = self .file_system - .read_directory(¶ms.path) + .read_directory_with_sandbox_policy(¶ms.path, params.sandbox_policy.as_ref()) .await .map_err(map_fs_error)?; Ok(FsReadDirectoryResponse { @@ -123,12 +124,13 @@ impl FileSystemHandler { params: FsRemoveParams, ) -> Result { self.file_system - .remove( + .remove_with_sandbox_policy( ¶ms.path, RemoveOptions { recursive: params.recursive.unwrap_or(true), force: params.force.unwrap_or(true), }, + params.sandbox_policy.as_ref(), ) .await .map_err(map_fs_error)?; @@ -140,12 +142,13 @@ impl FileSystemHandler { params: FsCopyParams, ) -> Result { self.file_system - .copy( + .copy_with_sandbox_policy( ¶ms.source_path, ¶ms.destination_path, CopyOptions { recursive: params.recursive, }, + params.sandbox_policy.as_ref(), ) .await .map_err(map_fs_error)?; diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 0fe2588d0..39e26548b 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -1,21 +1,21 @@ -use codex_app_server_protocol::FsCopyParams; -use codex_app_server_protocol::FsCopyResponse; -use codex_app_server_protocol::FsCreateDirectoryParams; -use codex_app_server_protocol::FsCreateDirectoryResponse; -use codex_app_server_protocol::FsGetMetadataParams; -use codex_app_server_protocol::FsGetMetadataResponse; -use codex_app_server_protocol::FsReadDirectoryParams; -use codex_app_server_protocol::FsReadDirectoryResponse; -use codex_app_server_protocol::FsReadFileParams; -use codex_app_server_protocol::FsReadFileResponse; -use codex_app_server_protocol::FsRemoveParams; -use codex_app_server_protocol::FsRemoveResponse; -use codex_app_server_protocol::FsWriteFileParams; -use codex_app_server_protocol::FsWriteFileResponse; use codex_app_server_protocol::JSONRPCErrorError; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; +use crate::protocol::FsCopyParams; +use crate::protocol::FsCopyResponse; +use crate::protocol::FsCreateDirectoryParams; +use crate::protocol::FsCreateDirectoryResponse; +use crate::protocol::FsGetMetadataParams; +use crate::protocol::FsGetMetadataResponse; +use crate::protocol::FsReadDirectoryParams; +use crate::protocol::FsReadDirectoryResponse; +use crate::protocol::FsReadFileParams; +use crate::protocol::FsReadFileResponse; +use crate::protocol::FsRemoveParams; +use crate::protocol::FsRemoveResponse; +use crate::protocol::FsWriteFileParams; +use crate::protocol::FsWriteFileResponse; use crate::protocol::InitializeResponse; use crate::protocol::ReadParams; use crate::protocol::ReadResponse; diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index 482e5ab61..19dba4b8b 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -12,6 +12,13 @@ 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::FsCopyParams; +use crate::protocol::FsCreateDirectoryParams; +use crate::protocol::FsGetMetadataParams; +use crate::protocol::FsReadDirectoryParams; +use crate::protocol::FsReadFileParams; +use crate::protocol::FsRemoveParams; +use crate::protocol::FsWriteFileParams; use crate::protocol::INITIALIZE_METHOD; use crate::protocol::INITIALIZED_METHOD; use crate::protocol::InitializeParams; @@ -20,13 +27,6 @@ use crate::protocol::TerminateParams; use crate::protocol::WriteParams; use crate::rpc::RpcRouter; use crate::server::ExecServerHandler; -use codex_app_server_protocol::FsCopyParams; -use codex_app_server_protocol::FsCreateDirectoryParams; -use codex_app_server_protocol::FsGetMetadataParams; -use codex_app_server_protocol::FsReadDirectoryParams; -use codex_app_server_protocol::FsReadFileParams; -use codex_app_server_protocol::FsRemoveParams; -use codex_app_server_protocol::FsWriteFileParams; pub(crate) fn build_router() -> RpcRouter { let mut router = RpcRouter::new(); diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index ca55587f5..a3272d4b3 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -14,6 +14,8 @@ use codex_exec_server::Environment; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::RemoveOptions; +use codex_protocol::protocol::ReadOnlyAccess; +use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::TempDir; @@ -56,6 +58,29 @@ fn absolute_path(path: std::path::PathBuf) -> AbsolutePathBuf { } } +fn read_only_sandbox_policy(readable_root: std::path::PathBuf) -> SandboxPolicy { + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![absolute_path(readable_root)], + }, + network_access: false, + } +} + +fn workspace_write_sandbox_policy(writable_root: std::path::PathBuf) -> SandboxPolicy { + SandboxPolicy::WorkspaceWrite { + writable_roots: vec![absolute_path(writable_root)], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![], + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + } +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -218,6 +243,521 @@ async fn file_system_copy_rejects_directory_without_recursive(use_remote: bool) Ok(()) } +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_read_with_sandbox_policy_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_policy = read_only_sandbox_policy(allowed_dir); + + let contents = file_system + .read_file_with_sandbox_policy(&absolute_path(file_path), Some(&sandbox_policy)) + .await + .with_context(|| format!("mode={use_remote}"))?; + assert_eq!(contents, b"sandboxed hello"); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_write_with_sandbox_policy_rejects_unwritable_path( + 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 blocked_path = tmp.path().join("blocked.txt"); + std::fs::create_dir_all(&allowed_dir)?; + + let sandbox_policy = read_only_sandbox_policy(allowed_dir); + let error = match file_system + .write_file_with_sandbox_policy( + &absolute_path(blocked_path.clone()), + b"nope".to_vec(), + Some(&sandbox_policy), + ) + .await + { + Ok(()) => anyhow::bail!("write should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/write is not permitted for path {}", + blocked_path.display() + ) + ); + assert!(!blocked_path.exists()); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_read_with_sandbox_policy_rejects_symlink_escape( + 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 outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(outside_dir.join("secret.txt"), "nope")?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_path = allowed_dir.join("link").join("secret.txt"); + let sandbox_policy = read_only_sandbox_policy(allowed_dir); + let error = match file_system + .read_file_with_sandbox_policy( + &absolute_path(requested_path.clone()), + Some(&sandbox_policy), + ) + .await + { + Ok(_) => anyhow::bail!("read should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/read is not permitted for path {}", + requested_path.display() + ) + ); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_read_with_sandbox_policy_rejects_symlink_parent_dotdot_escape( + 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 outside_dir = tmp.path().join("outside"); + let secret_path = tmp.path().join("secret.txt"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(&secret_path, "nope")?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_path = absolute_path(allowed_dir.join("link").join("..").join("secret.txt")); + let sandbox_policy = read_only_sandbox_policy(allowed_dir); + let error = match file_system + .read_file_with_sandbox_policy(&requested_path, Some(&sandbox_policy)) + .await + { + Ok(_) => anyhow::bail!("read should fail after path normalization"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::NotFound); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_write_with_sandbox_policy_rejects_symlink_escape( + 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 outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_path = allowed_dir.join("link").join("blocked.txt"); + let sandbox_policy = workspace_write_sandbox_policy(allowed_dir); + let error = match file_system + .write_file_with_sandbox_policy( + &absolute_path(requested_path.clone()), + b"nope".to_vec(), + Some(&sandbox_policy), + ) + .await + { + Ok(()) => anyhow::bail!("write should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/write is not permitted for path {}", + requested_path.display() + ) + ); + assert!(!outside_dir.join("blocked.txt").exists()); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_create_directory_with_sandbox_policy_rejects_symlink_escape( + 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 outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_path = allowed_dir.join("link").join("created"); + let sandbox_policy = workspace_write_sandbox_policy(allowed_dir); + let error = match file_system + .create_directory_with_sandbox_policy( + &absolute_path(requested_path.clone()), + CreateDirectoryOptions { recursive: false }, + Some(&sandbox_policy), + ) + .await + { + Ok(()) => anyhow::bail!("create_directory should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/write is not permitted for path {}", + requested_path.display() + ) + ); + assert!(!outside_dir.join("created").exists()); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_get_metadata_with_sandbox_policy_rejects_symlink_escape( + 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 outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(outside_dir.join("secret.txt"), "nope")?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_path = allowed_dir.join("link").join("secret.txt"); + let sandbox_policy = read_only_sandbox_policy(allowed_dir); + let error = match file_system + .get_metadata_with_sandbox_policy( + &absolute_path(requested_path.clone()), + Some(&sandbox_policy), + ) + .await + { + Ok(_) => anyhow::bail!("get_metadata should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/read is not permitted for path {}", + requested_path.display() + ) + ); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_read_directory_with_sandbox_policy_rejects_symlink_escape( + 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 outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(outside_dir.join("secret.txt"), "nope")?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_path = allowed_dir.join("link"); + let sandbox_policy = read_only_sandbox_policy(allowed_dir); + let error = match file_system + .read_directory_with_sandbox_policy( + &absolute_path(requested_path.clone()), + Some(&sandbox_policy), + ) + .await + { + Ok(_) => anyhow::bail!("read_directory should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/read is not permitted for path {}", + requested_path.display() + ) + ); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_copy_with_sandbox_policy_rejects_symlink_escape_destination( + 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 outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(allowed_dir.join("source.txt"), "hello")?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_destination = allowed_dir.join("link").join("copied.txt"); + let sandbox_policy = workspace_write_sandbox_policy(allowed_dir.clone()); + let error = match file_system + .copy_with_sandbox_policy( + &absolute_path(allowed_dir.join("source.txt")), + &absolute_path(requested_destination.clone()), + CopyOptions { recursive: false }, + Some(&sandbox_policy), + ) + .await + { + Ok(()) => anyhow::bail!("copy should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/write is not permitted for path {}", + requested_destination.display() + ) + ); + assert!(!outside_dir.join("copied.txt").exists()); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_remove_with_sandbox_policy_removes_symlink_not_target( + 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 outside_dir = tmp.path().join("outside"); + let outside_file = outside_dir.join("keep.txt"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(&outside_file, "outside")?; + let symlink_path = allowed_dir.join("link"); + symlink(&outside_file, &symlink_path)?; + + let sandbox_policy = workspace_write_sandbox_policy(allowed_dir); + file_system + .remove_with_sandbox_policy( + &absolute_path(symlink_path.clone()), + RemoveOptions { + recursive: false, + force: false, + }, + Some(&sandbox_policy), + ) + .await + .with_context(|| format!("mode={use_remote}"))?; + + assert!(!symlink_path.exists()); + assert!(outside_file.exists()); + assert_eq!(std::fs::read_to_string(outside_file)?, "outside"); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_copy_with_sandbox_policy_preserves_symlink_source( + 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 outside_dir = tmp.path().join("outside"); + let outside_file = outside_dir.join("outside.txt"); + let source_symlink = allowed_dir.join("link"); + let copied_symlink = allowed_dir.join("copied-link"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(&outside_file, "outside")?; + symlink(&outside_file, &source_symlink)?; + + let sandbox_policy = workspace_write_sandbox_policy(allowed_dir.clone()); + file_system + .copy_with_sandbox_policy( + &absolute_path(source_symlink), + &absolute_path(copied_symlink.clone()), + CopyOptions { recursive: false }, + Some(&sandbox_policy), + ) + .await + .with_context(|| format!("mode={use_remote}"))?; + + let copied_metadata = std::fs::symlink_metadata(&copied_symlink)?; + assert!(copied_metadata.file_type().is_symlink()); + assert_eq!(std::fs::read_link(copied_symlink)?, outside_file); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_remove_with_sandbox_policy_rejects_symlink_escape( + 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 outside_dir = tmp.path().join("outside"); + let outside_file = outside_dir.join("secret.txt"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(&outside_file, "outside")?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_path = allowed_dir.join("link").join("secret.txt"); + let sandbox_policy = workspace_write_sandbox_policy(allowed_dir); + let error = match file_system + .remove_with_sandbox_policy( + &absolute_path(requested_path.clone()), + RemoveOptions { + recursive: false, + force: false, + }, + Some(&sandbox_policy), + ) + .await + { + Ok(()) => anyhow::bail!("remove should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/write is not permitted for path {}", + requested_path.display() + ) + ); + assert_eq!(std::fs::read_to_string(outside_file)?, "outside"); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_copy_with_sandbox_policy_rejects_symlink_escape_source( + 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 outside_dir = tmp.path().join("outside"); + let outside_file = outside_dir.join("secret.txt"); + let requested_destination = allowed_dir.join("copied.txt"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + std::fs::write(&outside_file, "outside")?; + symlink(&outside_dir, allowed_dir.join("link"))?; + + let requested_source = allowed_dir.join("link").join("secret.txt"); + let sandbox_policy = workspace_write_sandbox_policy(allowed_dir); + let error = match file_system + .copy_with_sandbox_policy( + &absolute_path(requested_source.clone()), + &absolute_path(requested_destination.clone()), + CopyOptions { recursive: false }, + Some(&sandbox_policy), + ) + .await + { + Ok(()) => anyhow::bail!("copy should be blocked"), + Err(error) => error, + }; + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + error.to_string(), + format!( + "fs/read is not permitted for path {}", + requested_source.display() + ) + ); + assert!(!requested_destination.exists()); + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]