From 1dead46c90876ea22ac0b512a934fc7091b97ec2 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 15 Apr 2026 09:28:30 -0700 Subject: [PATCH] Remove exec-server fs sandbox request preflight (#17883) ## Summary - Remove the exec-server-side manual filesystem request path preflight before invoking the sandbox helper. - Keep sandbox helper policy construction and platform sandbox enforcement as the access boundary. - Add a portable local+remote regression for writing through an explicitly configured alias root. - Remove the metadata symlink-escape assertion that depended on the deleted manual preflight; no replacement metadata-specific access probe is added. ## Tests - `cargo test -p codex-exec-server --lib` - `cargo test -p codex-exec-server --test file_system` - `git diff --check` --- codex-rs/exec-server/src/fs_sandbox.rs | 170 ---------------------- codex-rs/exec-server/tests/file_system.rs | 144 ++++++++++++------ 2 files changed, 99 insertions(+), 215 deletions(-) diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index d0765afa3..7b0ac256d 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -1,10 +1,8 @@ use std::collections::HashMap; -use std::path::PathBuf; use codex_app_server_protocol::JSONRPCErrorError; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; -use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::ReadOnlyAccess; @@ -26,14 +24,6 @@ use crate::fs_helper::FsHelperPayload; use crate::fs_helper::FsHelperRequest; use crate::fs_helper::FsHelperResponse; use crate::local_file_system::current_sandbox_cwd; -use crate::local_file_system::resolve_existing_path; -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::rpc::internal_error; use crate::rpc::invalid_request; @@ -52,23 +42,16 @@ impl FileSystemSandboxRunner { sandbox: &FileSystemSandboxContext, request: FsHelperRequest, ) -> Result { - let request_sandbox_policy = - normalize_sandbox_policy_root_aliases(sandbox.sandbox_policy.clone()); let helper_sandbox_policy = normalize_sandbox_policy_root_aliases( sandbox_policy_with_helper_runtime_defaults(&sandbox.sandbox_policy), ); let cwd = current_sandbox_cwd().map_err(io_error)?; let cwd = AbsolutePathBuf::from_absolute_path(cwd.as_path()) .map_err(|err| invalid_request(format!("current directory is not absolute: {err}")))?; - let request_file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy( - &request_sandbox_policy, - cwd.as_path(), - ); let file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy( &helper_sandbox_policy, cwd.as_path(), ); - let request = resolve_request_paths(request, &request_file_system_policy, &cwd)?; let network_policy = NetworkSandboxPolicy::Restricted; let command = self.sandbox_exec_request( &helper_sandbox_policy, @@ -158,132 +141,6 @@ impl FileSystemSandboxRunner { } } -fn resolve_request_paths( - request: FsHelperRequest, - file_system_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, -) -> Result { - match request { - FsHelperRequest::ReadFile(FsReadFileParams { path, sandbox }) => { - let path = resolve_sandbox_path(&path, PreserveTerminalSymlink::No)?; - ensure_path_access(file_system_policy, cwd, &path, FileSystemAccessMode::Read)?; - Ok(FsHelperRequest::ReadFile(FsReadFileParams { - path, - sandbox, - })) - } - FsHelperRequest::WriteFile(FsWriteFileParams { - path, - data_base64, - sandbox, - }) => Ok(FsHelperRequest::WriteFile(FsWriteFileParams { - path: { - let path = resolve_sandbox_path(&path, PreserveTerminalSymlink::No)?; - ensure_path_access(file_system_policy, cwd, &path, FileSystemAccessMode::Write)?; - path - }, - data_base64, - sandbox, - })), - FsHelperRequest::CreateDirectory(FsCreateDirectoryParams { - path, - recursive, - sandbox, - }) => Ok(FsHelperRequest::CreateDirectory(FsCreateDirectoryParams { - path: { - let path = resolve_sandbox_path(&path, PreserveTerminalSymlink::No)?; - ensure_path_access(file_system_policy, cwd, &path, FileSystemAccessMode::Write)?; - path - }, - recursive, - sandbox, - })), - FsHelperRequest::GetMetadata(FsGetMetadataParams { path, sandbox }) => { - let path = resolve_sandbox_path(&path, PreserveTerminalSymlink::No)?; - ensure_path_access(file_system_policy, cwd, &path, FileSystemAccessMode::Read)?; - Ok(FsHelperRequest::GetMetadata(FsGetMetadataParams { - path, - sandbox, - })) - } - FsHelperRequest::ReadDirectory(FsReadDirectoryParams { path, sandbox }) => { - let path = resolve_sandbox_path(&path, PreserveTerminalSymlink::No)?; - ensure_path_access(file_system_policy, cwd, &path, FileSystemAccessMode::Read)?; - Ok(FsHelperRequest::ReadDirectory(FsReadDirectoryParams { - path, - sandbox, - })) - } - FsHelperRequest::Remove(FsRemoveParams { - path, - recursive, - force, - sandbox, - }) => Ok(FsHelperRequest::Remove(FsRemoveParams { - path: { - let path = resolve_sandbox_path(&path, PreserveTerminalSymlink::Yes)?; - ensure_path_access(file_system_policy, cwd, &path, FileSystemAccessMode::Write)?; - path - }, - recursive, - force, - sandbox, - })), - FsHelperRequest::Copy(FsCopyParams { - source_path, - destination_path, - recursive, - sandbox, - }) => Ok(FsHelperRequest::Copy(FsCopyParams { - source_path: { - let source_path = resolve_sandbox_path(&source_path, PreserveTerminalSymlink::Yes)?; - ensure_path_access( - file_system_policy, - cwd, - &source_path, - FileSystemAccessMode::Read, - )?; - source_path - }, - destination_path: { - let destination_path = - resolve_sandbox_path(&destination_path, PreserveTerminalSymlink::No)?; - ensure_path_access( - file_system_policy, - cwd, - &destination_path, - FileSystemAccessMode::Write, - )?; - destination_path - }, - recursive, - sandbox, - })), - } -} - -#[derive(Clone, Copy)] -enum PreserveTerminalSymlink { - Yes, - No, -} - -fn resolve_sandbox_path( - path: &AbsolutePathBuf, - preserve_terminal_symlink: PreserveTerminalSymlink, -) -> Result { - if matches!(preserve_terminal_symlink, PreserveTerminalSymlink::Yes) - && std::fs::symlink_metadata(path.as_path()) - .map(|metadata| metadata.file_type().is_symlink()) - .unwrap_or(false) - { - return Ok(normalize_top_level_alias(path.clone())); - } - - let resolved = resolve_existing_path(path.as_path()).map_err(io_error)?; - absolute_path(resolved) -} - fn normalize_sandbox_policy_root_aliases(sandbox_policy: SandboxPolicy) -> SandboxPolicy { let mut sandbox_policy = sandbox_policy; match &mut sandbox_policy { @@ -338,33 +195,6 @@ fn normalize_top_level_alias(path: AbsolutePathBuf) -> AbsolutePathBuf { path } -fn absolute_path(path: PathBuf) -> Result { - AbsolutePathBuf::from_absolute_path(path.as_path()) - .map_err(|err| invalid_request(format!("resolved sandbox path is not absolute: {err}"))) -} - -fn ensure_path_access( - file_system_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, - path: &AbsolutePathBuf, - required_access: FileSystemAccessMode, -) -> Result<(), JSONRPCErrorError> { - let actual_access = file_system_policy.resolve_access_with_cwd(path.as_path(), cwd.as_path()); - let permitted = match required_access { - FileSystemAccessMode::Read => actual_access.can_read(), - FileSystemAccessMode::Write => actual_access.can_write(), - FileSystemAccessMode::None => true, - }; - if permitted { - return Ok(()); - } - - Err(invalid_request(format!( - "{} is not permitted by filesystem sandbox", - path.display() - ))) -} - async fn run_command( command: SandboxExecRequest, request_json: Vec, diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index e24a2605f..e046c203d 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -3,6 +3,8 @@ mod common; use std::os::unix::fs::symlink; +use std::path::Path; +use std::path::PathBuf; use std::process::Command; use std::sync::Arc; @@ -17,6 +19,8 @@ use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LocalFileSystem; use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::RemoveOptions; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::ReadOnlyAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; @@ -94,20 +98,26 @@ fn workspace_write_sandbox(writable_root: std::path::PathBuf) -> FileSystemSandb } fn assert_sandbox_denied(error: &std::io::Error) { - assert!( - matches!( - error.kind(), - std::io::ErrorKind::InvalidInput | std::io::ErrorKind::PermissionDenied + match error.kind() { + std::io::ErrorKind::InvalidInput | std::io::ErrorKind::PermissionDenied => { + let message = error.to_string(); + assert!( + message.contains("is not permitted") + || message.contains("Operation not permitted") + || message.contains("Permission denied"), + "unexpected sandbox error message: {message}", + ); + } + std::io::ErrorKind::NotFound => assert!( + error.to_string().contains("No such file or directory"), + "unexpected sandbox not-found message: {error}", ), - "unexpected sandbox error kind: {error:?}", - ); - let message = error.to_string(); - assert!( - message.contains("is not permitted") - || message.contains("Operation not permitted") - || message.contains("Permission denied"), - "unexpected sandbox error message: {message}", - ); + std::io::ErrorKind::Other => assert!( + error.to_string().contains("Read-only file system"), + "unexpected sandbox other error message: {error}", + ), + other => panic!("unexpected sandbox error kind: {other:?}: {error:?}"), + } } fn assert_normalized_path_rejected(error: &std::io::Error) { @@ -129,6 +139,15 @@ fn assert_normalized_path_rejected(error: &std::io::Error) { } } +fn alias_root_candidate() -> Result> { + for root in [Path::new("/tmp").to_path_buf(), std::env::temp_dir()] { + if root.is_dir() && root.canonicalize().is_ok_and(|canonical| canonical != root) { + return Ok(Some(root)); + } + } + Ok(None) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -381,11 +400,9 @@ async fn file_system_sandboxed_write_rejects_unwritable_path(use_remote: bool) - 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 = read_only_sandbox(allowed_dir); + let sandbox = read_only_sandbox(tmp.path().to_path_buf()); let error = match file_system .write_file( &absolute_path(blocked_path.clone()), @@ -403,6 +420,72 @@ async fn file_system_sandboxed_write_rejects_unwritable_path(use_remote: bool) - Ok(()) } +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_sandboxed_write_allows_explicit_alias_roots(use_remote: bool) -> Result<()> { + let Some(alias_root) = alias_root_candidate()? else { + return Ok(()); + }; + + let context = create_file_system_context(use_remote).await?; + let file_system = context.file_system; + + let tmp = tempfile::Builder::new() + .prefix("codex-fs-sandbox-alias-") + .tempdir_in(&alias_root)?; + let file_path = tmp.path().join("note.txt"); + let sandbox = workspace_write_sandbox(alias_root.clone()); + + file_system + .write_file( + &absolute_path(file_path.clone()), + b"created".to_vec(), + Some(&sandbox), + ) + .await + .with_context(|| format!("write file through alias root mode={use_remote}"))?; + assert_eq!(std::fs::read(&file_path)?, b"created"); + + Ok(()) +} + +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_sandboxed_write_allows_additional_write_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 readable_dir = tmp.path().join("readable"); + let writable_dir = tmp.path().join("writable"); + let file_path = writable_dir.join("note.txt"); + std::fs::create_dir_all(&readable_dir)?; + std::fs::create_dir_all(&writable_dir)?; + + let mut sandbox = read_only_sandbox(readable_dir); + sandbox.additional_permissions = Some(PermissionProfile { + network: None, + file_system: Some(FileSystemPermissions { + read: None, + write: Some(vec![absolute_path(writable_dir)]), + }), + }); + + file_system + .write_file( + &absolute_path(file_path.clone()), + b"created".to_vec(), + Some(&sandbox), + ) + .await + .with_context(|| format!("write file through additional root mode={use_remote}"))?; + assert_eq!(std::fs::read(&file_path)?, b"created"); + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -528,35 +611,6 @@ async fn file_system_create_directory_rejects_symlink_escape(use_remote: bool) - Ok(()) } -#[test_case(false ; "local")] -#[test_case(true ; "remote")] -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn file_system_get_metadata_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 = read_only_sandbox(allowed_dir); - let error = match file_system - .get_metadata(&absolute_path(requested_path.clone()), Some(&sandbox)) - .await - { - Ok(_) => anyhow::bail!("get_metadata should be blocked"), - Err(error) => error, - }; - assert_sandbox_denied(&error); - - Ok(()) -} - #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]