diff --git a/codex-rs/config/src/diagnostics.rs b/codex-rs/config/src/diagnostics.rs index 899114d6d7..819a8d6b4d 100644 --- a/codex-rs/config/src/diagnostics.rs +++ b/codex-rs/config/src/diagnostics.rs @@ -9,11 +9,13 @@ use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::de::DeserializeOwned; use serde_path_to_error::Path as SerdePath; use serde_path_to_error::Segment as SerdeSegment; +use std::collections::HashSet; use std::fmt; use std::fmt::Write; use std::io; use std::path::Path; use std::path::PathBuf; +use toml::Value as TomlValue; use toml_edit::Document; use toml_edit::Item; use toml_edit::Table; @@ -134,6 +136,70 @@ pub fn config_error_from_typed_toml( } } +#[derive(Debug, Clone, PartialEq)] +pub struct SanitizedTomlValue { + pub value: TomlValue, + pub dropped_entries: Vec, +} + +pub fn sanitize_toml_value( + mut value: TomlValue, +) -> io::Result { + let mut dropped_entries = Vec::new(); + let mut seen_paths = HashSet::new(); + + loop { + let contents = toml::to_string(&value).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("failed to serialize TOML while sanitizing: {err}"), + ) + })?; + let deserializer = toml::de::Deserializer::parse(&contents).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("failed to parse TOML while sanitizing: {err}"), + ) + })?; + + let result: Result = serde_path_to_error::deserialize(deserializer); + match result { + Ok(_) => { + return Ok(SanitizedTomlValue { + value, + dropped_entries, + }); + } + Err(err) => { + let path_hint = err.path().clone(); + let toml_err: toml::de::Error = err.into_inner(); + let Some(recovery_path) = recovery_path_for_error(&path_hint) else { + return Err(io::Error::new(io::ErrorKind::InvalidData, toml_err)); + }; + let recovery_path_display = display_recovery_path(&recovery_path); + if !seen_paths.insert(recovery_path_display.clone()) { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "unable to recover from repeated invalid TOML path {recovery_path_display}: {toml_err}", + ), + )); + } + if !remove_value_at_recovery_path(&mut value, &recovery_path) { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "unable to remove invalid TOML path {recovery_path_display}: {toml_err}", + ), + )); + } + prune_empty_containers(&mut value); + dropped_entries.push(format!("{recovery_path_display}: {}", toml_err.message())); + } + } + } +} + pub async fn first_layer_config_error( layers: &ConfigLayerStack, config_toml_file: &str, @@ -323,6 +389,125 @@ fn span_for_config_path(contents: &str, path: &SerdePath) -> Option Option> { + let mut recovery_path = Vec::new(); + let mut last_seq_recovery_len = None; + + for segment in path.iter() { + match segment { + SerdeSegment::Map { key } => { + recovery_path.push(RecoveryPathSegment::Key(key.to_string())); + } + SerdeSegment::Enum { variant } => { + recovery_path.push(RecoveryPathSegment::Key(variant.to_string())); + } + SerdeSegment::Seq { index } => { + recovery_path.push(RecoveryPathSegment::Index(*index)); + last_seq_recovery_len = Some(recovery_path.len()); + } + SerdeSegment::Unknown => {} + } + } + + if recovery_path.is_empty() { + return None; + } + + if let Some(len) = last_seq_recovery_len { + recovery_path.truncate(len); + } + + Some(recovery_path) +} + +fn display_recovery_path(path: &[RecoveryPathSegment]) -> String { + let mut rendered = String::new(); + + for segment in path { + match segment { + RecoveryPathSegment::Key(key) => { + if !rendered.is_empty() { + rendered.push('.'); + } + rendered.push_str(key); + } + RecoveryPathSegment::Index(index) => { + let _ = write!(rendered, "[{index}]"); + } + } + } + + rendered +} + +fn remove_value_at_recovery_path(value: &mut TomlValue, path: &[RecoveryPathSegment]) -> bool { + remove_value_at_recovery_path_inner(value, path, 0) +} + +fn prune_empty_containers(value: &mut TomlValue) { + let _ = prune_empty_containers_inner(value); +} + +fn remove_value_at_recovery_path_inner( + value: &mut TomlValue, + path: &[RecoveryPathSegment], + index: usize, +) -> bool { + let Some(segment) = path.get(index) else { + return false; + }; + let is_leaf = index + 1 == path.len(); + + match segment { + RecoveryPathSegment::Key(key) => { + let Some(table) = value.as_table_mut() else { + return false; + }; + if is_leaf { + table.remove(key).is_some() + } else { + table.get_mut(key).is_some_and(|child| { + remove_value_at_recovery_path_inner(child, path, index + 1) + }) + } + } + RecoveryPathSegment::Index(seq_index) => { + let Some(array) = value.as_array_mut() else { + return false; + }; + if *seq_index >= array.len() { + return false; + } + if is_leaf { + array.remove(*seq_index); + true + } else { + remove_value_at_recovery_path_inner(&mut array[*seq_index], path, index + 1) + } + } + } +} + +fn prune_empty_containers_inner(value: &mut TomlValue) -> bool { + match value { + TomlValue::Table(table) => { + table.retain(|_, child| !prune_empty_containers_inner(child)); + table.is_empty() + } + TomlValue::Array(array) => { + array.retain_mut(|child| !prune_empty_containers_inner(child)); + array.is_empty() + } + _ => false, + } +} + fn is_features_table_path(path: &SerdePath) -> bool { let mut segments = path.iter(); matches!(segments.next(), Some(SerdeSegment::Map { key }) if key == "features") diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 995a5b8db7..19a1d77e8c 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -34,6 +34,7 @@ pub use constraint::ConstraintError; pub use constraint::ConstraintResult; pub use diagnostics::ConfigError; pub use diagnostics::ConfigLoadError; +pub use diagnostics::SanitizedTomlValue; pub use diagnostics::TextPosition; pub use diagnostics::TextRange; pub use diagnostics::config_error_from_toml; @@ -43,6 +44,7 @@ pub use diagnostics::first_layer_config_error_from_entries; pub use diagnostics::format_config_error; pub use diagnostics::format_config_error_with_source; pub use diagnostics::io_error_from_config_error; +pub use diagnostics::sanitize_toml_value; pub use fingerprint::version_for_toml; pub use merge::merge_toml_values; pub use overrides::build_cli_overrides_layer; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index a1f270458a..21b6cf606d 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -646,6 +646,17 @@ impl ConfigBuilder { } let cli_overrides = cli_overrides.unwrap_or_default(); let mut harness_overrides = harness_overrides.unwrap_or_default(); + #[cfg(all(test, target_os = "macos"))] + let loader_overrides = { + let loader_overrides_provided = loader_overrides.is_some(); + let mut loader_overrides = loader_overrides.unwrap_or_default(); + if !loader_overrides_provided { + loader_overrides.managed_preferences_base64 = Some(String::new()); + loader_overrides.macos_managed_config_requirements_base64 = Some(String::new()); + } + loader_overrides + }; + #[cfg(not(all(test, target_os = "macos")))] let loader_overrides = loader_overrides.unwrap_or_default(); let cwd_override = harness_overrides.cwd.as_deref().or(fallback_cwd.as_deref()); let cwd = match cwd_override { diff --git a/codex-rs/core/src/config_loader/macos.rs b/codex-rs/core/src/config_loader/macos.rs index d94b1930b7..87f6ddd3c7 100644 --- a/codex-rs/core/src/config_loader/macos.rs +++ b/codex-rs/core/src/config_loader/macos.rs @@ -1,8 +1,10 @@ use super::ConfigRequirementsToml; use super::ConfigRequirementsWithSources; use super::RequirementSource; +use crate::config::ConfigToml; use base64::Engine; use base64::prelude::BASE64_STANDARD; +use codex_config::sanitize_toml_value; use core_foundation::base::TCFType; use core_foundation::string::CFString; use core_foundation::string::CFStringRef; @@ -36,7 +38,7 @@ pub(crate) async fn load_managed_admin_config_layer( return if trimmed.is_empty() { Ok(None) } else { - parse_managed_config_base64(trimmed).map(Some) + parse_managed_config_base64(trimmed) }; } @@ -59,6 +61,7 @@ fn load_managed_admin_config() -> io::Result> { .map(str::trim) .map(parse_managed_config_base64) .transpose() + .map(Option::flatten) } pub(crate) async fn load_managed_admin_requirements_toml( @@ -71,10 +74,9 @@ pub(crate) async fn load_managed_admin_requirements_toml( return Ok(()); } - target.merge_unset_fields( - managed_preferences_requirements_source(), - parse_managed_requirements_base64(trimmed)?, - ); + if let Some(requirements) = parse_managed_requirements_base64(trimmed)? { + target.merge_unset_fields(managed_preferences_requirements_source(), requirements); + } return Ok(()); } @@ -102,6 +104,7 @@ fn load_managed_admin_requirements() -> io::Result io::Result> { @@ -128,34 +131,106 @@ fn load_managed_preference(key_name: &str) -> io::Result> { Ok(Some(value)) } -fn parse_managed_config_base64(encoded: &str) -> io::Result { - let raw_toml = decode_managed_preferences_base64(encoded)?; +fn parse_managed_config_base64(encoded: &str) -> io::Result> { + let raw_toml = match decode_managed_preferences_base64(encoded) { + Ok(raw_toml) => raw_toml, + Err(err) => { + tracing::warn!( + error = %err, + "Ignoring invalid MDM managed config payload", + ); + return Ok(None); + } + }; match toml::from_str::(&raw_toml) { - Ok(TomlValue::Table(parsed)) => Ok(ManagedAdminConfigLayer { - config: TomlValue::Table(parsed), - raw_toml, - }), + Ok(TomlValue::Table(parsed)) => { + let sanitized = match sanitize_toml_value::(TomlValue::Table(parsed)) { + Ok(sanitized) => sanitized, + Err(err) => { + tracing::warn!( + error = %err, + "Ignoring invalid MDM managed config payload", + ); + return Ok(None); + } + }; + for dropped_entry in &sanitized.dropped_entries { + tracing::warn!( + dropped_entry = %dropped_entry, + "Ignoring invalid MDM managed config entry", + ); + } + Ok(Some(ManagedAdminConfigLayer { + config: sanitized.value, + raw_toml, + })) + } Ok(other) => { - tracing::error!("Managed config TOML must have a table at the root, found {other:?}",); - Err(io::Error::new( - io::ErrorKind::InvalidData, - "managed config root must be a table", - )) + tracing::warn!( + managed_value = ?other, + "Ignoring invalid MDM managed config payload: root must be a table", + ); + Ok(None) } Err(err) => { - tracing::error!("Failed to parse managed config TOML: {err}"); - Err(io::Error::new(io::ErrorKind::InvalidData, err)) + tracing::warn!( + error = %err, + "Ignoring invalid MDM managed config payload", + ); + Ok(None) } } } -fn parse_managed_requirements_base64(encoded: &str) -> io::Result { - toml::from_str::(&decode_managed_preferences_base64(encoded)?).map_err( - |err| { - tracing::error!("Failed to parse managed requirements TOML: {err}"); - io::Error::new(io::ErrorKind::InvalidData, err) - }, - ) +fn parse_managed_requirements_base64(encoded: &str) -> io::Result> { + let raw_toml = match decode_managed_preferences_base64(encoded) { + Ok(raw_toml) => raw_toml, + Err(err) => { + tracing::warn!( + error = %err, + "Ignoring invalid MDM managed requirements payload", + ); + return Ok(None); + } + }; + let parsed = match toml::from_str::(&raw_toml) { + Ok(TomlValue::Table(parsed)) => TomlValue::Table(parsed), + Ok(other) => { + tracing::warn!( + managed_value = ?other, + "Ignoring invalid MDM managed requirements payload: root must be a table", + ); + return Ok(None); + } + Err(err) => { + tracing::warn!( + error = %err, + "Ignoring invalid MDM managed requirements payload", + ); + return Ok(None); + } + }; + let sanitized = match sanitize_toml_value::(parsed) { + Ok(sanitized) => sanitized, + Err(err) => { + tracing::warn!( + error = %err, + "Ignoring invalid MDM managed requirements payload", + ); + return Ok(None); + } + }; + for dropped_entry in &sanitized.dropped_entries { + tracing::warn!( + dropped_entry = %dropped_entry, + "Ignoring invalid MDM managed requirements entry", + ); + } + let requirements = sanitized + .value + .try_into() + .map_err(|err: toml::de::Error| io::Error::new(io::ErrorKind::InvalidData, err))?; + Ok(Some(requirements)) } fn decode_managed_preferences_base64(encoded: &str) -> io::Result { diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 03be02ebfe..6875aeeb4f 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -380,6 +380,144 @@ flag = false assert!(raw.contains("value = \"managed\"")); } +#[cfg(target_os = "macos")] +#[tokio::test] +async fn managed_preferences_expand_home_directory_in_workspace_write_roots() -> anyhow::Result<()> +{ + use base64::Engine; + + let Some(home) = dirs::home_dir() else { + return Ok(()); + }; + let tmp = tempdir()?; + + let config = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides { + managed_config_path: Some(tmp.path().join("managed_config.toml")), + managed_preferences_base64: Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +sandbox_mode = "workspace-write" +[sandbox_workspace_write] +writable_roots = ["~/code"] +"# + .as_bytes(), + ), + ), + macos_managed_config_requirements_base64: None, + }) + .build() + .await?; + + let expected_root = AbsolutePathBuf::from_absolute_path(home.join("code"))?; + match config.permissions.sandbox_policy.get() { + SandboxPolicy::WorkspaceWrite { writable_roots, .. } => { + assert_eq!( + writable_roots + .iter() + .filter(|root| **root == expected_root) + .count(), + 1, + ); + } + other => panic!("expected workspace-write policy, got {other:?}"), + } + + Ok(()) +} + +#[cfg(target_os = "macos")] +#[tokio::test] +async fn managed_preferences_ignore_invalid_config_entry_without_dropping_valid_entries() +-> anyhow::Result<()> { + use base64::Engine; + + let tmp = tempdir()?; + + let config = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides { + managed_config_path: Some(tmp.path().join("managed_config.toml")), + managed_preferences_base64: Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +model = "managed" +sandbox_mode = "workspace-write" +[sandbox_workspace_write] +writable_roots = ["relative/path"] +"# + .as_bytes(), + ), + ), + macos_managed_config_requirements_base64: None, + }) + .build() + .await?; + + assert_eq!(config.model.as_deref(), Some("managed")); + let effective_config = config.config_layer_stack.effective_config(); + let writable_roots = effective_config + .get("sandbox_workspace_write") + .and_then(|value| value.get("writable_roots")) + .and_then(TomlValue::as_array); + assert_eq!(writable_roots, None); + + Ok(()) +} + +#[cfg(target_os = "macos")] +#[tokio::test] +async fn managed_preferences_ignore_invalid_payload() -> anyhow::Result<()> { + let tmp = tempdir()?; + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "model = \"user\"\n")?; + + let config = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides { + managed_config_path: Some(tmp.path().join("managed_config.toml")), + managed_preferences_base64: Some("%%%".to_string()), + macos_managed_config_requirements_base64: None, + }) + .build() + .await?; + + assert_eq!(config.model.as_deref(), Some("user")); + + Ok(()) +} + +#[cfg(target_os = "macos")] +#[tokio::test] +async fn managed_preferences_ignore_invalid_requirements_payload() -> anyhow::Result<()> { + use base64::Engine; + + let tmp = tempdir()?; + + let state = load_config_layers_state( + tmp.path(), + Some(AbsolutePathBuf::try_from(tmp.path())?), + &[] as &[(String, TomlValue)], + LoaderOverrides { + managed_config_path: Some(tmp.path().join("managed_config.toml")), + managed_preferences_base64: Some(String::new()), + macos_managed_config_requirements_base64: Some( + base64::prelude::BASE64_STANDARD + .encode("allowed_sandbox_modes = [\"bogus\"]".as_bytes()), + ), + }, + CloudRequirementsLoader::default(), + ) + .await?; + + assert_eq!(state.requirements_toml().allowed_sandbox_modes, None); + + Ok(()) +} + #[cfg(target_os = "macos")] #[tokio::test] async fn managed_preferences_requirements_are_applied() -> anyhow::Result<()> { diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 1ba1c40cfb..c8ea210fe3 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -179,12 +179,16 @@ impl<'de> Deserialize<'de> for AbsolutePathBuf { 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 => { + let expanded = Self::maybe_expand_home_directory(&path); + if expanded.is_absolute() { + Self::from_absolute_path(expanded).map_err(SerdeError::custom) + } else { + Err(SerdeError::custom( + "AbsolutePathBuf deserialized without a base path", + )) + } } - None => Err(SerdeError::custom( - "AbsolutePathBuf deserialized without a base path", - )), }) } } @@ -246,6 +250,17 @@ mod tests { assert_eq!(abs_path_buf.as_path(), home.as_path()); } + #[cfg(not(target_os = "windows"))] + #[test] + fn home_directory_root_on_non_windows_is_expanded_without_base_path() { + let Some(home) = home_dir() else { + return; + }; + let abs_path_buf = + serde_json::from_str::("\"~\"").expect("failed to deserialize"); + assert_eq!(abs_path_buf.as_path(), home.as_path()); + } + #[cfg(not(target_os = "windows"))] #[test] fn home_directory_subpath_on_non_windows_is_expanded_in_deserialization() { @@ -260,6 +275,17 @@ mod tests { assert_eq!(abs_path_buf.as_path(), home.join("code").as_path()); } + #[cfg(not(target_os = "windows"))] + #[test] + fn home_directory_subpath_on_non_windows_is_expanded_without_base_path() { + let Some(home) = home_dir() else { + return; + }; + let abs_path_buf = + serde_json::from_str::("\"~/code\"").expect("failed to deserialize"); + assert_eq!(abs_path_buf.as_path(), home.join("code").as_path()); + } + #[cfg(not(target_os = "windows"))] #[test] fn home_directory_double_slash_on_non_windows_is_expanded_in_deserialization() { @@ -288,4 +314,15 @@ mod tests { base_dir.join("~").join("code").as_path() ); } + + #[test] + fn relative_path_without_base_path_fails_in_deserialization() { + let err = serde_json::from_str::("\"subdir/file.txt\"") + .expect_err("relative path without a base path should fail"); + assert!( + err.to_string() + .contains("AbsolutePathBuf deserialized without a base path"), + "unexpected error: {err}", + ); + } }