From a60e25e1f6b6717830d884946306ffa67bc9c561 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 10 Dec 2025 18:41:46 -0800 Subject: [PATCH] fix: introduce AbsolutePathBuf as part of sandbox config --- codex-rs/Cargo.lock | 8 ++ codex-rs/app-server-protocol/Cargo.toml | 1 + .../app-server-protocol/src/protocol/v1.rs | 3 +- .../app-server-protocol/src/protocol/v2.rs | 3 +- .../suite/codex_message_processor_flow.rs | 2 +- codex-rs/app-server/tests/suite/config.rs | 2 +- .../app-server/tests/suite/v2/turn_start.rs | 2 +- codex-rs/core/src/config/mod.rs | 20 ++--- codex-rs/core/src/config/types.rs | 2 +- codex-rs/core/src/environment_context.rs | 8 +- codex-rs/core/src/safety.rs | 3 +- codex-rs/core/src/seatbelt.rs | 16 +++- codex-rs/core/tests/suite/prompt_caching.rs | 5 +- codex-rs/core/tests/suite/seatbelt.rs | 4 +- codex-rs/exec/Cargo.toml | 1 + codex-rs/exec/tests/suite/sandbox.rs | 5 +- codex-rs/linux-sandbox/Cargo.toml | 1 + codex-rs/linux-sandbox/src/landlock.rs | 6 +- codex-rs/protocol/Cargo.toml | 2 +- codex-rs/protocol/src/protocol.rs | 49 ++++++++--- codex-rs/tui/Cargo.toml | 1 + codex-rs/tui/src/chatwidget/tests.rs | 3 +- codex-rs/tui2/Cargo.toml | 1 + codex-rs/tui2/src/chatwidget/tests.rs | 3 +- codex-rs/utils/absolute-path/Cargo.toml | 5 ++ codex-rs/utils/absolute-path/src/lib.rs | 82 ++++++++++++++----- 26 files changed, 171 insertions(+), 67 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 95f4fecc48..a57b68f196 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -865,6 +865,7 @@ dependencies = [ "anyhow", "clap", "codex-protocol", + "codex-utils-absolute-path", "mcp-types", "pretty_assertions", "schemars 0.8.22", @@ -1183,6 +1184,7 @@ dependencies = [ "codex-common", "codex-core", "codex-protocol", + "codex-utils-absolute-path", "core_test_support", "libc", "mcp-types", @@ -1322,6 +1324,7 @@ version = "0.0.0" dependencies = [ "clap", "codex-core", + "codex-utils-absolute-path", "landlock", "libc", "seccompiler", @@ -1446,6 +1449,7 @@ version = "0.0.0" dependencies = [ "anyhow", "codex-git", + "codex-utils-absolute-path", "codex-utils-image", "icu_decimal", "icu_locale_core", @@ -1544,6 +1548,7 @@ dependencies = [ "codex-file-search", "codex-login", "codex-protocol", + "codex-utils-absolute-path", "codex-windows-sandbox", "color-eyre", "crossterm", @@ -1613,6 +1618,7 @@ dependencies = [ "codex-login", "codex-protocol", "codex-tui", + "codex-utils-absolute-path", "codex-windows-sandbox", "color-eyre", "crossterm", @@ -1665,9 +1671,11 @@ name = "codex-utils-absolute-path" version = "0.0.0" dependencies = [ "path-absolutize", + "schemars 0.8.22", "serde", "serde_json", "tempfile", + "ts-rs", ] [[package]] diff --git a/codex-rs/app-server-protocol/Cargo.toml b/codex-rs/app-server-protocol/Cargo.toml index c1a251c656..1c21bd6ea0 100644 --- a/codex-rs/app-server-protocol/Cargo.toml +++ b/codex-rs/app-server-protocol/Cargo.toml @@ -15,6 +15,7 @@ workspace = true anyhow = { workspace = true } clap = { workspace = true, features = ["derive"] } codex-protocol = { workspace = true } +codex-utils-absolute-path = { workspace = true } mcp-types = { workspace = true } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } diff --git a/codex-rs/app-server-protocol/src/protocol/v1.rs b/codex-rs/app-server-protocol/src/protocol/v1.rs index 853cb03b40..df39f8809e 100644 --- a/codex-rs/app-server-protocol/src/protocol/v1.rs +++ b/codex-rs/app-server-protocol/src/protocol/v1.rs @@ -16,6 +16,7 @@ use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::TurnAbortReason; +use codex_utils_absolute_path::AbsolutePathBuf; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; @@ -359,7 +360,7 @@ pub struct Tools { #[serde(rename_all = "camelCase")] pub struct SandboxSettings { #[serde(default)] - pub writable_roots: Vec, + pub writable_roots: Vec, pub network_access: Option, pub exclude_tmpdir_env_var: Option, pub exclude_slash_tmp: Option, diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 3429a4fc15..a0772a17b8 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -24,6 +24,7 @@ use codex_protocol::protocol::SessionSource as CoreSessionSource; use codex_protocol::protocol::TokenUsage as CoreTokenUsage; use codex_protocol::protocol::TokenUsageInfo as CoreTokenUsageInfo; use codex_protocol::user_input::UserInput as CoreUserInput; +use codex_utils_absolute_path::AbsolutePathBuf; use mcp_types::ContentBlock as McpContentBlock; use mcp_types::Resource as McpResource; use mcp_types::ResourceTemplate as McpResourceTemplate; @@ -420,7 +421,7 @@ pub enum SandboxPolicy { #[ts(rename_all = "camelCase")] WorkspaceWrite { #[serde(default)] - writable_roots: Vec, + writable_roots: Vec, #[serde(default)] network_access: bool, #[serde(default)] diff --git a/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs b/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs index e417198994..be94dd822e 100644 --- a/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs +++ b/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs @@ -410,7 +410,7 @@ async fn test_send_user_turn_updates_sandbox_and_cwd_between_turns() -> Result<( cwd: first_cwd.clone(), approval_policy: AskForApproval::Never, sandbox_policy: SandboxPolicy::WorkspaceWrite { - writable_roots: vec![first_cwd.clone()], + writable_roots: vec![first_cwd.try_into()?], network_access: false, exclude_tmpdir_env_var: false, exclude_slash_tmp: false, diff --git a/codex-rs/app-server/tests/suite/config.rs b/codex-rs/app-server/tests/suite/config.rs index 88e74a6fb4..29af70181b 100644 --- a/codex-rs/app-server/tests/suite/config.rs +++ b/codex-rs/app-server/tests/suite/config.rs @@ -80,7 +80,7 @@ async fn get_config_toml_parses_all_fields() -> Result<()> { approval_policy: Some(AskForApproval::OnRequest), sandbox_mode: Some(SandboxMode::WorkspaceWrite), sandbox_settings: Some(SandboxSettings { - writable_roots: vec!["/tmp".into()], + writable_roots: vec!["/tmp".try_into()?], network_access: Some(true), exclude_tmpdir_env_var: Some(true), exclude_slash_tmp: Some(true), diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index afc22c7072..1948487d14 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -532,7 +532,7 @@ async fn turn_start_updates_sandbox_and_cwd_between_turns_v2() -> Result<()> { cwd: Some(first_cwd.clone()), approval_policy: Some(codex_app_server_protocol::AskForApproval::Never), sandbox_policy: Some(codex_app_server_protocol::SandboxPolicy::WorkspaceWrite { - writable_roots: vec![first_cwd.clone()], + writable_roots: vec![first_cwd.try_into()?], network_access: false, exclude_tmpdir_env_var: false, exclude_slash_tmp: false, diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index e0e6985a39..acfc4d2284 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -40,9 +40,9 @@ use codex_protocol::config_types::TrustLevel; use codex_protocol::config_types::Verbosity; use codex_protocol::openai_models::ReasoningEffort; use codex_rmcp_client::OAuthCredentialsStoreMode; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use dirs::home_dir; -use dunce::canonicalize; use serde::Deserialize; use similar::DiffableStr; use std::collections::BTreeMap; @@ -1022,13 +1022,10 @@ impl Config { } } }; - let additional_writable_roots: Vec = additional_writable_roots + let additional_writable_roots: Vec = additional_writable_roots .into_iter() - .map(|path| { - let absolute = resolve_path(&resolved_cwd, &path); - canonicalize(&absolute).unwrap_or(absolute) - }) - .collect(); + .map(|path| AbsolutePathBuf::resolve_path_against_base(path, &resolved_cwd)) + .collect::, _>>()?; let active_project = cfg .get_active_project(&resolved_cwd) .unwrap_or(ProjectConfig { trust_level: None }); @@ -1488,7 +1485,7 @@ exclude_slash_tmp = true resolution, SandboxPolicyResolution { policy: SandboxPolicy::WorkspaceWrite { - writable_roots: vec![PathBuf::from("/my/workspace")], + writable_roots: vec!["/my/workspace".try_into().unwrap()], network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, @@ -1533,7 +1530,10 @@ trust_level = "trusted" resolution, SandboxPolicyResolution { policy: SandboxPolicy::WorkspaceWrite { - writable_roots: vec![PathBuf::from("/my/workspace")], + writable_roots: vec![ + AbsolutePathBuf::from_absolute_path("/my/workspace") + .expect("absolute path") + ], network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, @@ -1565,7 +1565,7 @@ trust_level = "trusted" temp_dir.path().to_path_buf(), )?; - let expected_backend = canonicalize(&backend).expect("canonicalize backend directory"); + let expected_backend = AbsolutePathBuf::try_from(backend).unwrap(); if cfg!(target_os = "windows") { assert!( config.forced_auto_mode_downgraded_on_windows, diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 6648e288a5..b542536ba7 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -406,7 +406,7 @@ impl Notice { #[derive(Deserialize, Debug, Clone, PartialEq, Default)] pub struct SandboxWorkspaceWrite { #[serde(default)] - pub writable_roots: Vec, + pub writable_roots: Vec, #[serde(default)] pub network_access: bool, #[serde(default)] diff --git a/codex-rs/core/src/environment_context.rs b/codex-rs/core/src/environment_context.rs index 54756bda2d..a014eefe1b 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/environment_context.rs @@ -1,3 +1,4 @@ +use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; use strum_macros::Display as DeriveDisplay; @@ -27,7 +28,7 @@ pub(crate) struct EnvironmentContext { pub approval_policy: Option, pub sandbox_mode: Option, pub network_access: Option, - pub writable_roots: Option>, + pub writable_roots: Option>, pub shell: Shell, } @@ -203,7 +204,10 @@ mod tests { fn workspace_write_policy(writable_roots: Vec<&str>, network_access: bool) -> SandboxPolicy { SandboxPolicy::WorkspaceWrite { - writable_roots: writable_roots.into_iter().map(PathBuf::from).collect(), + writable_roots: writable_roots + .into_iter() + .map(|s| AbsolutePathBuf::try_from(s).unwrap()) + .collect(), network_access, exclude_tmpdir_env_var: false, exclude_slash_tmp: false, diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index e82e107453..eaa3e2a613 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -188,6 +188,7 @@ fn is_write_patch_constrained_to_writable_paths( #[cfg(test)] mod tests { use super::*; + use codex_utils_absolute_path::AbsolutePathBuf; use tempfile::TempDir; #[test] @@ -228,7 +229,7 @@ mod tests { // With the parent dir explicitly added as a writable root, the // outside write should be permitted. let policy_with_parent = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![parent], + writable_roots: vec![AbsolutePathBuf::try_from(parent).unwrap()], network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, diff --git a/codex-rs/core/src/seatbelt.rs b/codex-rs/core/src/seatbelt.rs index 8ca7e4357e..5e01b041d0 100644 --- a/codex-rs/core/src/seatbelt.rs +++ b/codex-rs/core/src/seatbelt.rs @@ -63,7 +63,11 @@ pub(crate) fn create_seatbelt_command_args( for (index, wr) in writable_roots.iter().enumerate() { // Canonicalize to avoid mismatches like /var vs /private/var on macOS. - let canonical_root = wr.root.canonicalize().unwrap_or_else(|_| wr.root.clone()); + let canonical_root = wr + .root + .as_path() + .canonicalize() + .unwrap_or_else(|_| wr.root.to_path_buf()); let root_param = format!("WRITABLE_ROOT_{index}"); file_write_params.push((root_param.clone(), canonical_root)); @@ -75,7 +79,10 @@ pub(crate) fn create_seatbelt_command_args( let mut require_parts: Vec = Vec::new(); require_parts.push(format!("(subpath (param \"{root_param}\"))")); for (subpath_index, ro) in wr.read_only_subpaths.iter().enumerate() { - let canonical_ro = ro.canonicalize().unwrap_or_else(|_| ro.clone()); + let canonical_ro = ro + .as_path() + .canonicalize() + .unwrap_or_else(|_| ro.to_path_buf()); let ro_param = format!("WRITABLE_ROOT_{index}_RO_{subpath_index}"); require_parts .push(format!("(require-not (subpath (param \"{ro_param}\")))")); @@ -182,7 +189,10 @@ mod tests { // Build a policy that only includes the two test roots as writable and // does not automatically include defaults TMPDIR or /tmp. let policy = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![root_with_git, root_without_git], + writable_roots: vec![root_with_git, root_without_git] + .into_iter() + .map(|p| p.try_into().unwrap()) + .collect(), network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index 94158df6d8..e11139090e 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -11,6 +11,7 @@ use codex_core::shell::Shell; use codex_core::shell::default_user_shell; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::user_input::UserInput; +use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::load_sse_fixture_with_id; use core_test_support::responses::mount_sse_once; use core_test_support::responses::start_mock_server; @@ -317,7 +318,7 @@ async fn overrides_turn_context_but_keeps_cached_prefix_and_key_constant() -> an cwd: None, approval_policy: Some(AskForApproval::Never), sandbox_policy: Some(SandboxPolicy::WorkspaceWrite { - writable_roots: vec![writable.path().to_path_buf()], + writable_roots: vec![writable.path().try_into().unwrap()], network_access: true, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, @@ -507,7 +508,7 @@ async fn per_turn_overrides_keep_cached_prefix_and_key_constant() -> anyhow::Res cwd: new_cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, sandbox_policy: SandboxPolicy::WorkspaceWrite { - writable_roots: vec![writable.path().to_path_buf()], + writable_roots: vec![AbsolutePathBuf::try_from(writable.path()).unwrap()], network_access: true, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, diff --git a/codex-rs/core/tests/suite/seatbelt.rs b/codex-rs/core/tests/suite/seatbelt.rs index 52150b0511..286bc8791b 100644 --- a/codex-rs/core/tests/suite/seatbelt.rs +++ b/codex-rs/core/tests/suite/seatbelt.rs @@ -76,7 +76,7 @@ async fn if_parent_of_repo_is_writable_then_dot_git_folder_is_writable() { let tmp = TempDir::new().expect("should be able to create temp dir"); let test_scenario = create_test_scenario(&tmp); let policy = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![test_scenario.repo_parent.clone()], + writable_roots: vec![test_scenario.repo_parent.as_path().try_into().unwrap()], network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, @@ -102,7 +102,7 @@ async fn if_git_repo_is_writable_root_then_dot_git_folder_is_read_only() { let tmp = TempDir::new().expect("should be able to create temp dir"); let test_scenario = create_test_scenario(&tmp); let policy = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![test_scenario.repo_root.clone()], + writable_roots: vec![test_scenario.repo_root.as_path().try_into().unwrap()], network_access: false, exclude_tmpdir_env_var: true, exclude_slash_tmp: true, diff --git a/codex-rs/exec/Cargo.toml b/codex-rs/exec/Cargo.toml index f26bec0e67..a6e5302de1 100644 --- a/codex-rs/exec/Cargo.toml +++ b/codex-rs/exec/Cargo.toml @@ -26,6 +26,7 @@ codex-common = { workspace = true, features = [ ] } codex-core = { workspace = true } codex-protocol = { workspace = true } +codex-utils-absolute-path = { workspace = true } mcp-types = { workspace = true } opentelemetry-appender-tracing = { workspace = true } owo-colors = { workspace = true } diff --git a/codex-rs/exec/tests/suite/sandbox.rs b/codex-rs/exec/tests/suite/sandbox.rs index f0faa8b438..d995d91030 100644 --- a/codex-rs/exec/tests/suite/sandbox.rs +++ b/codex-rs/exec/tests/suite/sandbox.rs @@ -1,6 +1,7 @@ #![cfg(unix)] use codex_core::protocol::SandboxPolicy; use codex_core::spawn::StdioPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; use std::future::Future; use std::io; @@ -58,14 +59,14 @@ async fn spawn_command_under_sandbox( async fn python_multiprocessing_lock_works_under_sandbox() { core_test_support::skip_if_sandbox!(); #[cfg(target_os = "macos")] - let writable_roots = Vec::::new(); + let writable_roots = Vec::::new(); // From https://man7.org/linux/man-pages/man7/sem_overview.7.html // // > On Linux, named semaphores are created in a virtual filesystem, // > normally mounted under /dev/shm. #[cfg(target_os = "linux")] - let writable_roots = vec![PathBuf::from("/dev/shm")]; + let writable_roots: Vec = vec!["/dev/shm".try_into().unwrap()]; let policy = SandboxPolicy::WorkspaceWrite { writable_roots, diff --git a/codex-rs/linux-sandbox/Cargo.toml b/codex-rs/linux-sandbox/Cargo.toml index cac8604f5d..1009791aa0 100644 --- a/codex-rs/linux-sandbox/Cargo.toml +++ b/codex-rs/linux-sandbox/Cargo.toml @@ -18,6 +18,7 @@ workspace = true [target.'cfg(target_os = "linux")'.dependencies] clap = { workspace = true, features = ["derive"] } codex-core = { workspace = true } +codex-utils-absolute-path = { workspace = true } landlock = { workspace = true } libc = { workspace = true } seccompiler = { workspace = true } diff --git a/codex-rs/linux-sandbox/src/landlock.rs b/codex-rs/linux-sandbox/src/landlock.rs index 119d859b26..2281fa4d65 100644 --- a/codex-rs/linux-sandbox/src/landlock.rs +++ b/codex-rs/linux-sandbox/src/landlock.rs @@ -1,11 +1,11 @@ use std::collections::BTreeMap; use std::path::Path; -use std::path::PathBuf; use codex_core::error::CodexErr; use codex_core::error::Result; use codex_core::error::SandboxErr; use codex_core::protocol::SandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; use landlock::ABI; use landlock::Access; @@ -56,7 +56,9 @@ pub(crate) fn apply_sandbox_policy_to_current_thread( /// /// # Errors /// Returns [`CodexErr::Sandbox`] variants when the ruleset fails to apply. -fn install_filesystem_landlock_rules_on_current_thread(writable_roots: Vec) -> Result<()> { +fn install_filesystem_landlock_rules_on_current_thread( + writable_roots: Vec, +) -> Result<()> { let abi = ABI::V5; let access_rw = AccessFs::from_all(abi); let access_ro = AccessFs::from_read(abi); diff --git a/codex-rs/protocol/Cargo.toml b/codex-rs/protocol/Cargo.toml index 46f030c60a..19ec1fcc36 100644 --- a/codex-rs/protocol/Cargo.toml +++ b/codex-rs/protocol/Cargo.toml @@ -13,7 +13,7 @@ workspace = true [dependencies] codex-git = { workspace = true } - +codex-utils-absolute-path = { workspace = true } codex-utils-image = { workspace = true } icu_decimal = { workspace = true } icu_locale_core = { workspace = true } diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 73e2c9c877..238cd0fa26 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -23,6 +23,7 @@ use crate::openai_models::ReasoningEffort as ReasoningEffortConfig; use crate::parse_command::ParsedCommand; use crate::plan_tool::UpdatePlanArgs; use crate::user_input::UserInput; +use codex_utils_absolute_path::AbsolutePathBuf; use mcp_types::CallToolResult; use mcp_types::RequestId; use mcp_types::Resource as McpResource; @@ -34,6 +35,7 @@ use serde::Serialize; use serde_json::Value; use serde_with::serde_as; use strum_macros::Display; +use tracing::error; use ts_rs::TS; pub use crate::approvals::ApplyPatchApprovalRequestEvent; @@ -273,7 +275,7 @@ pub enum SandboxPolicy { /// Additional folders (beyond cwd and possibly TMPDIR) that should be /// writable from within the sandbox. #[serde(default, skip_serializing_if = "Vec::is_empty")] - writable_roots: Vec, + writable_roots: Vec, /// When set to `true`, outbound network access is allowed. `false` by /// default. @@ -299,11 +301,9 @@ pub enum SandboxPolicy { /// not modified by the agent. #[derive(Debug, Clone, PartialEq, Eq, JsonSchema)] pub struct WritableRoot { - /// Absolute path, by construction. - pub root: PathBuf, + pub root: AbsolutePathBuf, - /// Also absolute paths, by construction. - pub read_only_subpaths: Vec, + pub read_only_subpaths: Vec, } impl WritableRoot { @@ -385,16 +385,30 @@ impl SandboxPolicy { network_access: _, } => { // Start from explicitly configured writable roots. - let mut roots: Vec = writable_roots.clone(); + let mut roots: Vec = writable_roots.clone(); // Always include defaults: cwd, /tmp (if present on Unix), and // on macOS, the per-user TMPDIR unless explicitly excluded. - roots.push(cwd.to_path_buf()); + // TODO(mbolin): cwd param should be AbsolutePathBuf. + let cwd_absolute = AbsolutePathBuf::from_absolute_path(cwd); + match cwd_absolute { + Ok(cwd) => { + roots.push(cwd); + } + Err(e) => { + error!( + "Ignoring invalid cwd {:?} for sandbox writable root: {}", + cwd, e + ); + } + } // Include /tmp on Unix unless explicitly excluded. if cfg!(unix) && !exclude_slash_tmp { - let slash_tmp = PathBuf::from("/tmp"); - if slash_tmp.is_dir() { + #[allow(clippy::expect_used)] + let slash_tmp = + AbsolutePathBuf::from_absolute_path("/tmp").expect("/tmp is absolute"); + if slash_tmp.as_path().is_dir() { roots.push(slash_tmp); } } @@ -411,7 +425,15 @@ impl SandboxPolicy { && let Some(tmpdir) = std::env::var_os("TMPDIR") && !tmpdir.is_empty() { - roots.push(PathBuf::from(tmpdir)); + if let Ok(tmpdir_path) = + AbsolutePathBuf::from_absolute_path(PathBuf::from(&tmpdir)) + { + roots.push(tmpdir_path); + } else { + error!( + "Ignoring invalid TMPDIR value {tmpdir:?} for sandbox writable root", + ); + } } // For each root, compute subpaths that should remain read-only. @@ -419,8 +441,11 @@ impl SandboxPolicy { .into_iter() .map(|writable_root| { let mut subpaths = Vec::new(); - let top_level_git = writable_root.join(".git"); - if top_level_git.is_dir() { + #[allow(clippy::expect_used)] + let top_level_git = writable_root + .join(".git") + .expect(".git is a valid relative path"); + if top_level_git.as_path().is_dir() { subpaths.push(top_level_git); } WritableRoot { diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 4e5fad06b4..c440f09aeb 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -41,6 +41,7 @@ codex-feedback = { workspace = true } codex-file-search = { workspace = true } codex-login = { workspace = true } codex-protocol = { workspace = true } +codex-utils-absolute-path = { workspace = true } color-eyre = { workspace = true } crossterm = { workspace = true, features = ["bracketed-paste", "event-stream"] } derive_more = { workspace = true, features = ["is_variant"] } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index bd85a9edc8..4c0248d7ae 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -56,6 +56,7 @@ use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; use codex_protocol::protocol::CodexErrorInfo; +use codex_utils_absolute_path::AbsolutePathBuf; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; @@ -1805,7 +1806,7 @@ fn preset_matching_ignores_extra_writable_roots() { .find(|p| p.id == "auto") .expect("auto preset exists"); let current_sandbox = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![PathBuf::from("C:\\extra")], + writable_roots: vec![AbsolutePathBuf::try_from("C:\\extra").unwrap()], network_access: false, exclude_tmpdir_env_var: false, exclude_slash_tmp: false, diff --git a/codex-rs/tui2/Cargo.toml b/codex-rs/tui2/Cargo.toml index 06308e996c..7ac1fb859c 100644 --- a/codex-rs/tui2/Cargo.toml +++ b/codex-rs/tui2/Cargo.toml @@ -40,6 +40,7 @@ codex-feedback = { workspace = true } codex-file-search = { workspace = true } codex-login = { workspace = true } codex-protocol = { workspace = true } +codex-utils-absolute-path = { workspace = true } codex-tui = { workspace = true } color-eyre = { workspace = true } crossterm = { workspace = true, features = ["bracketed-paste", "event-stream"] } diff --git a/codex-rs/tui2/src/chatwidget/tests.rs b/codex-rs/tui2/src/chatwidget/tests.rs index d9e242674b..11a5799280 100644 --- a/codex-rs/tui2/src/chatwidget/tests.rs +++ b/codex-rs/tui2/src/chatwidget/tests.rs @@ -56,6 +56,7 @@ use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::plan_tool::UpdatePlanArgs; use codex_protocol::protocol::CodexErrorInfo; +use codex_utils_absolute_path::AbsolutePathBuf; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; @@ -1806,7 +1807,7 @@ fn preset_matching_ignores_extra_writable_roots() { .find(|p| p.id == "auto") .expect("auto preset exists"); let current_sandbox = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![PathBuf::from("C:\\extra")], + writable_roots: vec![AbsolutePathBuf::try_from("C:\\extra").unwrap()], network_access: false, exclude_tmpdir_env_var: false, exclude_slash_tmp: false, diff --git a/codex-rs/utils/absolute-path/Cargo.toml b/codex-rs/utils/absolute-path/Cargo.toml index 486051fcb4..5af9dc23aa 100644 --- a/codex-rs/utils/absolute-path/Cargo.toml +++ b/codex-rs/utils/absolute-path/Cargo.toml @@ -10,7 +10,12 @@ workspace = true [dependencies] path-absolutize = { workspace = true } +schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } +ts-rs = { workspace = true, features = [ + "serde-json-impl", + "no-serde-warnings", +] } [dev-dependencies] serde_json = { workspace = true } diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 5ea77b2b52..257a83d957 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -1,4 +1,5 @@ use path_absolutize::Absolutize; +use schemars::JsonSchema; use serde::Deserialize; use serde::Deserializer; use serde::Serialize; @@ -7,6 +8,7 @@ use std::cell::RefCell; use std::path::Display; use std::path::Path; use std::path::PathBuf; +use ts_rs::TS; /// A path that is guaranteed to be absolute and normalized (though it is not /// guaranteed to be canonicalized or exist on the filesystem). @@ -15,27 +17,27 @@ use std::path::PathBuf; /// using `AbsolutePathBufGuard::new(base_path)`. If no base path is set, the /// deserialization will fail unless the path being deserialized is already /// absolute. -#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema, TS)] pub struct AbsolutePathBuf(PathBuf); impl AbsolutePathBuf { - pub fn resolve_path_against_base(path: P, base_path: B) -> std::io::Result - where - P: AsRef, - B: AsRef, - { + pub fn resolve_path_against_base, B: AsRef>( + path: P, + base_path: B, + ) -> std::io::Result { let absolute_path = path.as_ref().absolutize_from(base_path.as_ref())?; Ok(Self(absolute_path.into_owned())) } - pub fn from_absolute_path

(path: P) -> std::io::Result - where - P: AsRef, - { + pub fn from_absolute_path>(path: P) -> std::io::Result { let absolute_path = path.as_ref().absolutize()?; Ok(Self(absolute_path.into_owned())) } + pub fn join>(&self, path: P) -> std::io::Result { + Self::resolve_path_against_base(path, &self.0) + } + pub fn as_path(&self) -> &Path { &self.0 } @@ -48,11 +50,59 @@ impl AbsolutePathBuf { self.0.clone() } + pub fn to_string_lossy(&self) -> std::borrow::Cow<'_, str> { + self.0.to_string_lossy() + } + pub fn display(&self) -> Display<'_> { self.0.display() } } +impl AsRef for AbsolutePathBuf { + fn as_ref(&self) -> &Path { + &self.0 + } +} + +impl From for PathBuf { + fn from(path: AbsolutePathBuf) -> Self { + path.into_path_buf() + } +} + +impl TryFrom<&Path> for AbsolutePathBuf { + type Error = std::io::Error; + + fn try_from(value: &Path) -> Result { + Self::from_absolute_path(value) + } +} + +impl TryFrom for AbsolutePathBuf { + type Error = std::io::Error; + + fn try_from(value: PathBuf) -> Result { + Self::from_absolute_path(value) + } +} + +impl TryFrom<&str> for AbsolutePathBuf { + type Error = std::io::Error; + + fn try_from(value: &str) -> Result { + Self::from_absolute_path(value) + } +} + +impl TryFrom for AbsolutePathBuf { + type Error = std::io::Error; + + fn try_from(value: String) -> Result { + Self::from_absolute_path(value) + } +} + thread_local! { static ABSOLUTE_PATH_BASE: RefCell> = const { RefCell::new(None) }; } @@ -96,18 +146,6 @@ impl<'de> Deserialize<'de> for AbsolutePathBuf { } } -impl AsRef for AbsolutePathBuf { - fn as_ref(&self) -> &Path { - self.as_path() - } -} - -impl From for PathBuf { - fn from(path: AbsolutePathBuf) -> Self { - path.into_path_buf() - } -} - #[cfg(test)] mod tests { use super::*;