diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index aa1f72b4b..58ba4f2a9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1157,6 +1157,7 @@ dependencies = [ "codex-otel", "codex-protocol", "codex-rmcp-client", + "codex-utils-absolute-path", "codex-utils-pty", "codex-utils-readiness", "codex-utils-string", @@ -1464,6 +1465,7 @@ dependencies = [ "chrono", "codex-app-server-protocol", "codex-protocol", + "codex-utils-absolute-path", "eventsource-stream", "http", "opentelemetry", @@ -1650,6 +1652,16 @@ dependencies = [ "codex-tui", ] +[[package]] +name = "codex-utils-absolute-path" +version = "0.0.0" +dependencies = [ + "path-absolutize", + "serde", + "serde_json", + "tempfile", +] + [[package]] name = "codex-utils-cache" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index bd62c72d5..a2521e3bd 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -35,6 +35,7 @@ members = [ "otel", "tui", "tui2", + "utils/absolute-path", "utils/git", "utils/cache", "utils/image", @@ -90,6 +91,7 @@ codex-rmcp-client = { path = "rmcp-client" } codex-stdio-to-uds = { path = "stdio-to-uds" } codex-tui = { path = "tui" } codex-tui2 = { path = "tui2" } +codex-utils-absolute-path = { path = "utils/absolute-path" } codex-utils-cache = { path = "utils/cache" } codex-utils-image = { path = "utils/image" } codex-utils-json-to-toml = { path = "utils/json-to-toml" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index f24cc9bc6..2bc281d90 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -1,8 +1,8 @@ [package] -name = "codex-core" -version.workspace = true edition.workspace = true license.workspace = true +name = "codex-core" +version.workspace = true [lib] doctest = false @@ -18,12 +18,12 @@ askama = { workspace = true } async-channel = { workspace = true } async-trait = { workspace = true } base64 = { workspace = true } -chrono = { workspace = true, features = ["serde"] } chardetng = { workspace = true } +chrono = { workspace = true, features = ["serde"] } +codex-api = { workspace = true } codex-app-server-protocol = { workspace = true } codex-apply-patch = { workspace = true } codex-async-utils = { workspace = true } -codex-api = { workspace = true } codex-execpolicy = { workspace = true } codex-file-search = { workspace = true } codex-git = { workspace = true } @@ -31,14 +31,15 @@ codex-keyring-store = { workspace = true } codex-otel = { workspace = true, features = ["otel"] } codex-protocol = { workspace = true } codex-rmcp-client = { workspace = true } +codex-utils-absolute-path = { workspace = true } codex-utils-pty = { workspace = true } codex-utils-readiness = { workspace = true } codex-utils-string = { workspace = true } codex-windows-sandbox = { package = "codex-windows-sandbox", path = "../windows-sandbox-rs" } dirs = { workspace = true } dunce = { workspace = true } -env-flags = { workspace = true } encoding_rs = { workspace = true } +env-flags = { workspace = true } eventsource-stream = { workspace = true } futures = { workspace = true } http = { workspace = true } @@ -46,8 +47,10 @@ indexmap = { workspace = true } keyring = { workspace = true, features = ["crypto-rust"] } libc = { workspace = true } mcp-types = { workspace = true } +once_cell = { workspace = true } os_info = { workspace = true } rand = { workspace = true } +regex = { workspace = true } regex-lite = { workspace = true } reqwest = { workspace = true, features = ["json", "stream"] } serde = { workspace = true, features = ["derive"] } @@ -58,9 +61,6 @@ sha2 = { workspace = true } shlex = { workspace = true } similar = { workspace = true } strum_macros = { workspace = true } -url = { workspace = true } -once_cell = { workspace = true } -regex = { workspace = true } tempfile = { workspace = true } test-case = "3.3.1" test-log = { workspace = true } @@ -84,6 +84,7 @@ toml_edit = { workspace = true } tracing = { workspace = true, features = ["log"] } tree-sitter = { workspace = true } tree-sitter-bash = { workspace = true } +url = { workspace = true } uuid = { workspace = true, features = ["serde", "v4", "v5"] } which = { workspace = true } wildmatch = { workspace = true } @@ -94,9 +95,9 @@ test-support = [] [target.'cfg(target_os = "linux")'.dependencies] +keyring = { workspace = true, features = ["linux-native-async-persistent"] } landlock = { workspace = true } seccompiler = { workspace = true } -keyring = { workspace = true, features = ["linux-native-async-persistent"] } [target.'cfg(target_os = "macos")'.dependencies] core-foundation = "0.9" diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index df7637a30..8db08c55a 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -40,6 +40,7 @@ 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::AbsolutePathBufGuard; use dirs::home_dir; use dunce::canonicalize; use serde::Deserialize; @@ -299,9 +300,9 @@ impl Config { ) .await?; - let cfg: ConfigToml = root_value.try_into().map_err(|e| { + let cfg = deserialize_config_toml_with_base(root_value, &codex_home).map_err(|e| { tracing::error!("Failed to deserialize overridden config: {e}"); - std::io::Error::new(std::io::ErrorKind::InvalidData, e) + e })?; Self::load_from_base_config_with_overrides(cfg, overrides, codex_home) @@ -319,9 +320,9 @@ pub async fn load_config_as_toml_with_cli_overrides( ) .await?; - let cfg: ConfigToml = root_value.try_into().map_err(|e| { + let cfg = deserialize_config_toml_with_base(root_value, codex_home).map_err(|e| { tracing::error!("Failed to deserialize overridden config: {e}"); - std::io::Error::new(std::io::ErrorKind::InvalidData, e) + e })?; Ok(cfg) @@ -357,6 +358,18 @@ fn apply_overlays( base } +fn deserialize_config_toml_with_base( + root_value: TomlValue, + config_base_dir: &Path, +) -> std::io::Result { + // This guard ensures that any relative paths that is deserialized into an + // [AbsolutePathBuf] is resolved against `config_base_dir`. + let _guard = AbsolutePathBufGuard::new(config_base_dir); + root_value + .try_into() + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e)) +} + pub async fn load_global_mcp_servers( codex_home: &Path, ) -> std::io::Result> { @@ -1852,10 +1865,11 @@ trust_level = "trusted" }; let root_value = load_resolved_config(codex_home.path(), Vec::new(), overrides).await?; - let cfg: ConfigToml = root_value.try_into().map_err(|e| { - tracing::error!("Failed to deserialize overridden config: {e}"); - std::io::Error::new(std::io::ErrorKind::InvalidData, e) - })?; + let cfg = + deserialize_config_toml_with_base(root_value, codex_home.path()).map_err(|e| { + tracing::error!("Failed to deserialize overridden config: {e}"); + e + })?; assert_eq!( cfg.mcp_oauth_credentials_store, Some(OAuthCredentialsStoreMode::Keyring), @@ -1972,10 +1986,11 @@ trust_level = "trusted" ) .await?; - let cfg: ConfigToml = root_value.try_into().map_err(|e| { - tracing::error!("Failed to deserialize overridden config: {e}"); - std::io::Error::new(std::io::ErrorKind::InvalidData, e) - })?; + let cfg = + deserialize_config_toml_with_base(root_value, codex_home.path()).map_err(|e| { + tracing::error!("Failed to deserialize overridden config: {e}"); + e + })?; assert_eq!(cfg.model.as_deref(), Some("managed_config")); Ok(()) diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 5e1b78aa7..6648e288a 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -3,13 +3,14 @@ // Note this file should generally be restricted to simple struct/enum // definitions that do not contain business logic. -use serde::Deserializer; +use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; use std::path::PathBuf; use std::time::Duration; use wildmatch::WildMatchPattern; use serde::Deserialize; +use serde::Deserializer; use serde::Serialize; use serde::de::Error as SerdeError; @@ -285,9 +286,9 @@ pub enum OtelHttpProtocol { #[derive(Deserialize, Debug, Clone, PartialEq, Default)] #[serde(rename_all = "kebab-case")] pub struct OtelTlsConfig { - pub ca_certificate: Option, - pub client_certificate: Option, - pub client_private_key: Option, + pub ca_certificate: Option, + pub client_certificate: Option, + pub client_private_key: Option, } /// Which OTEL exporter to use. diff --git a/codex-rs/otel/Cargo.toml b/codex-rs/otel/Cargo.toml index 5ed6c0949..af8b72346 100644 --- a/codex-rs/otel/Cargo.toml +++ b/codex-rs/otel/Cargo.toml @@ -21,6 +21,7 @@ otel = ["opentelemetry", "opentelemetry_sdk", "opentelemetry-otlp", "tonic"] [dependencies] chrono = { workspace = true } codex-app-server-protocol = { workspace = true } +codex-utils-absolute-path = { workspace = true } codex-protocol = { workspace = true } eventsource-stream = { workspace = true } opentelemetry = { workspace = true, features = ["logs"], optional = true } diff --git a/codex-rs/otel/src/config.rs b/codex-rs/otel/src/config.rs index b6336b3a5..652a1c97b 100644 --- a/codex-rs/otel/src/config.rs +++ b/codex-rs/otel/src/config.rs @@ -1,6 +1,8 @@ use std::collections::HashMap; use std::path::PathBuf; +use codex_utils_absolute_path::AbsolutePathBuf; + #[derive(Clone, Debug)] pub struct OtelSettings { pub environment: String, @@ -20,9 +22,9 @@ pub enum OtelHttpProtocol { #[derive(Clone, Debug, Default)] pub struct OtelTlsConfig { - pub ca_certificate: Option, - pub client_certificate: Option, - pub client_private_key: Option, + pub ca_certificate: Option, + pub client_certificate: Option, + pub client_private_key: Option, } #[derive(Clone, Debug)] diff --git a/codex-rs/otel/src/otel_provider.rs b/codex-rs/otel/src/otel_provider.rs index 5495db0ad..92b1feaa1 100644 --- a/codex-rs/otel/src/otel_provider.rs +++ b/codex-rs/otel/src/otel_provider.rs @@ -2,6 +2,7 @@ use crate::config::OtelExporter; use crate::config::OtelHttpProtocol; use crate::config::OtelSettings; use crate::config::OtelTlsConfig; +use codex_utils_absolute_path::AbsolutePathBuf; use http::Uri; use opentelemetry::KeyValue; use opentelemetry_otlp::LogExporter; @@ -25,7 +26,6 @@ use std::error::Error; use std::fs; use std::io::ErrorKind; use std::io::{self}; -use std::path::Path; use std::path::PathBuf; use std::time::Duration; use tonic::metadata::MetadataMap; @@ -85,12 +85,7 @@ impl OtelProvider { .assume_http2(true); let tls_config = match tls.as_ref() { - Some(tls) => build_grpc_tls_config( - endpoint, - base_tls_config, - tls, - settings.codex_home.as_path(), - )?, + Some(tls) => build_grpc_tls_config(endpoint, base_tls_config, tls)?, None => base_tls_config, }; @@ -123,7 +118,7 @@ impl OtelProvider { .with_headers(headers.clone()); if let Some(tls) = tls.as_ref() { - let client = build_http_client(tls, settings.codex_home.as_path())?; + let client = build_http_client(tls)?; exporter_builder = exporter_builder.with_http_client(client); } @@ -149,7 +144,6 @@ fn build_grpc_tls_config( endpoint: &str, tls_config: ClientTlsConfig, tls: &OtelTlsConfig, - codex_home: &Path, ) -> Result> { let uri: Uri = endpoint.parse()?; let host = uri.host().ok_or_else(|| { @@ -161,14 +155,14 @@ fn build_grpc_tls_config( let mut config = tls_config.domain_name(host.to_owned()); if let Some(path) = tls.ca_certificate.as_ref() { - let (pem, _) = read_bytes(codex_home, path)?; + let (pem, _) = read_bytes(path)?; config = config.ca_certificate(TonicCertificate::from_pem(pem)); } match (&tls.client_certificate, &tls.client_private_key) { (Some(cert_path), Some(key_path)) => { - let (cert_pem, _) = read_bytes(codex_home, cert_path)?; - let (key_pem, _) = read_bytes(codex_home, key_path)?; + let (cert_pem, _) = read_bytes(cert_path)?; + let (key_pem, _) = read_bytes(key_path)?; config = config.identity(TonicIdentity::from_pem(cert_pem, key_pem)); } (Some(_), None) | (None, Some(_)) => { @@ -188,24 +182,20 @@ fn build_grpc_tls_config( /// `opentelemetry_sdk` `BatchLogProcessor` spawns a dedicated OS thread that uses /// `futures_executor::block_on()` rather than tokio. When the async reqwest client's /// timeout calls `tokio::time::sleep()`, it panics with "no reactor running". -fn build_http_client( - tls: &OtelTlsConfig, - codex_home: &Path, -) -> Result> { +fn build_http_client(tls: &OtelTlsConfig) -> Result> { // Wrap in block_in_place because reqwest::blocking::Client creates its own // internal tokio runtime, which would panic if built directly from an async context. - tokio::task::block_in_place(|| build_http_client_inner(tls, codex_home)) + tokio::task::block_in_place(|| build_http_client_inner(tls)) } fn build_http_client_inner( tls: &OtelTlsConfig, - codex_home: &Path, ) -> Result> { let mut builder = reqwest::blocking::Client::builder() .timeout(resolve_otlp_timeout(OTEL_EXPORTER_OTLP_LOGS_TIMEOUT)); if let Some(path) = tls.ca_certificate.as_ref() { - let (pem, location) = read_bytes(codex_home, path)?; + let (pem, location) = read_bytes(path)?; let certificate = ReqwestCertificate::from_pem(pem.as_slice()).map_err(|error| { config_error(format!( "failed to parse certificate {}: {error}", @@ -220,8 +210,8 @@ fn build_http_client_inner( match (&tls.client_certificate, &tls.client_private_key) { (Some(cert_path), Some(key_path)) => { - let (mut cert_pem, cert_location) = read_bytes(codex_home, cert_path)?; - let (key_pem, key_location) = read_bytes(codex_home, key_path)?; + let (mut cert_pem, cert_location) = read_bytes(cert_path)?; + let (key_pem, key_location) = read_bytes(key_path)?; cert_pem.extend_from_slice(key_pem.as_slice()); let identity = ReqwestIdentity::from_pem(cert_pem.as_slice()).map_err(|error| { config_error(format!( @@ -264,25 +254,16 @@ fn read_timeout_env(var: &str) -> Option { Some(Duration::from_millis(parsed as u64)) } -fn read_bytes(base: &Path, provided: &PathBuf) -> Result<(Vec, PathBuf), Box> { - let resolved = resolve_config_path(base, provided); - match fs::read(&resolved) { - Ok(bytes) => Ok((bytes, resolved)), +fn read_bytes(path: &AbsolutePathBuf) -> Result<(Vec, PathBuf), Box> { + match fs::read(path) { + Ok(bytes) => Ok((bytes, path.to_path_buf())), Err(error) => Err(Box::new(io::Error::new( error.kind(), - format!("failed to read {}: {error}", resolved.display()), + format!("failed to read {}: {error}", path.display()), ))), } } -fn resolve_config_path(base: &Path, provided: &PathBuf) -> PathBuf { - if provided.is_absolute() { - provided.clone() - } else { - base.join(provided) - } -} - fn config_error(message: impl Into) -> Box { Box::new(io::Error::new(ErrorKind::InvalidData, message.into())) } diff --git a/codex-rs/utils/absolute-path/Cargo.toml b/codex-rs/utils/absolute-path/Cargo.toml new file mode 100644 index 000000000..486051fcb --- /dev/null +++ b/codex-rs/utils/absolute-path/Cargo.toml @@ -0,0 +1,17 @@ + +[package] +name = "codex-utils-absolute-path" +version.workspace = true +edition.workspace = true +license.workspace = true + +[lints] +workspace = true + +[dependencies] +path-absolutize = { workspace = true } +serde = { workspace = true, features = ["derive"] } + +[dev-dependencies] +serde_json = { workspace = true } +tempfile = { workspace = true } diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs new file mode 100644 index 000000000..5ea77b2b5 --- /dev/null +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -0,0 +1,152 @@ +use path_absolutize::Absolutize; +use serde::Deserialize; +use serde::Deserializer; +use serde::Serialize; +use serde::de::Error as SerdeError; +use std::cell::RefCell; +use std::path::Display; +use std::path::Path; +use std::path::PathBuf; + +/// A path that is guaranteed to be absolute and normalized (though it is not +/// guaranteed to be canonicalized or exist on the filesystem). +/// +/// IMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set +/// 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)] +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, + { + 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, + { + let absolute_path = path.as_ref().absolutize()?; + Ok(Self(absolute_path.into_owned())) + } + + pub fn as_path(&self) -> &Path { + &self.0 + } + + pub fn into_path_buf(self) -> PathBuf { + self.0 + } + + pub fn to_path_buf(&self) -> PathBuf { + self.0.clone() + } + + pub fn display(&self) -> Display<'_> { + self.0.display() + } +} + +thread_local! { + static ABSOLUTE_PATH_BASE: RefCell> = const { RefCell::new(None) }; +} + +pub struct AbsolutePathBufGuard; + +impl AbsolutePathBufGuard { + pub fn new(base_path: &Path) -> Self { + ABSOLUTE_PATH_BASE.with(|cell| { + *cell.borrow_mut() = Some(base_path.to_path_buf()); + }); + Self + } +} + +impl Drop for AbsolutePathBufGuard { + fn drop(&mut self) { + ABSOLUTE_PATH_BASE.with(|cell| { + *cell.borrow_mut() = None; + }); + } +} + +impl<'de> Deserialize<'de> for AbsolutePathBuf { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let path = PathBuf::deserialize(deserializer)?; + ABSOLUTE_PATH_BASE.with(|cell| match cell.borrow().as_deref() { + Some(base) => { + Ok(Self::resolve_path_against_base(path, base).map_err(SerdeError::custom)?) + } + None if path.is_absolute() => { + Self::from_absolute_path(path).map_err(SerdeError::custom) + } + None => Err(SerdeError::custom( + "AbsolutePathBuf deserialized without a base path", + )), + }) + } +} + +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::*; + use tempfile::tempdir; + + #[test] + fn create_with_absolute_path_ignores_base_path() { + let base_dir = tempdir().expect("base dir"); + let absolute_dir = tempdir().expect("absolute dir"); + let base_path = base_dir.path(); + let absolute_path = absolute_dir.path().join("file.txt"); + let abs_path_buf = + AbsolutePathBuf::resolve_path_against_base(absolute_path.clone(), base_path) + .expect("failed to create"); + assert_eq!(abs_path_buf.as_path(), absolute_path.as_path()); + } + + #[test] + fn relative_path_is_resolved_against_base_path() { + let temp_dir = tempdir().expect("base dir"); + let base_dir = temp_dir.path(); + let abs_path_buf = AbsolutePathBuf::resolve_path_against_base("file.txt", base_dir) + .expect("failed to create"); + assert_eq!(abs_path_buf.as_path(), base_dir.join("file.txt").as_path()); + } + + #[test] + fn guard_used_in_deserialization() { + let temp_dir = tempdir().expect("base dir"); + let base_dir = temp_dir.path(); + let relative_path = "subdir/file.txt"; + let abs_path_buf = { + let _guard = AbsolutePathBufGuard::new(base_dir); + serde_json::from_str::(&format!(r#""{relative_path}""#)) + .expect("failed to deserialize") + }; + assert_eq!( + abs_path_buf.as_path(), + base_dir.join(relative_path).as_path() + ); + } +}