Handle invalid security config safely across managed, project, and session layers

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<T: DeserializeOwned>(
     }
 }
 
+#[derive(Debug, Clone, PartialEq)]
+pub struct SanitizedTomlValue {
+    pub value: TomlValue,
+    pub dropped_entries: Vec<String>,
+}
+
+pub fn sanitize_toml_value<T: DeserializeOwned>(
+    mut value: TomlValue,
+) -> io::Result<SanitizedTomlValue> {
+    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<T, _> = 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<T: DeserializeOwned>(
     layers: &ConfigLayerStack,
     config_toml_file: &str,
@@ -323,6 +389,125 @@ fn span_for_config_path(contents: &str, path: &SerdePath) -> Option<std::ops::Ra
     span_for_path(contents, path)
 }
 
+#[derive(Debug, Clone, PartialEq, Eq)]
+enum RecoveryPathSegment {
+    Key(String),
+    Index(usize),
+}
+
+fn recovery_path_for_error(path: &SerdePath) -> Option<Vec<RecoveryPathSegment>> {
+    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")

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;

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 {

codex-rs/core/src/config/service_tests.rs

@@ -7,8 +7,18 @@ use codex_app_server_protocol::AskForApproval;
 use codex_utils_absolute_path::AbsolutePathBuf;
 use pretty_assertions::assert_eq;
 use std::collections::BTreeMap;
+use std::path::PathBuf;
 use tempfile::tempdir;
 
+fn test_loader_overrides(managed_config_path: Option<PathBuf>) -> LoaderOverrides {
+    LoaderOverrides {
+        managed_config_path,
+        #[cfg(target_os = "macos")]
+        managed_preferences_base64: Some(String::new()),
+        macos_managed_config_requirements_base64: Some(String::new()),
+    }
+}
+
 #[test]
 fn toml_value_to_item_handles_nested_config_tables() {
     let config = r#"
@@ -178,12 +188,7 @@ async fn read_includes_origins_and_layers() {
     let service = ConfigService::new(
         tmp.path().to_path_buf(),
         vec![],
-        LoaderOverrides {
-            managed_config_path: Some(managed_path.clone()),
-            #[cfg(target_os = "macos")]
-            managed_preferences_base64: None,
-            macos_managed_config_requirements_base64: None,
-        },
+        test_loader_overrides(Some(managed_path.clone())),
         CloudRequirementsLoader::default(),
     );
 
@@ -253,12 +258,7 @@ async fn write_value_reports_override() {
     let service = ConfigService::new(
         tmp.path().to_path_buf(),
         vec![],
-        LoaderOverrides {
-            managed_config_path: Some(managed_path.clone()),
-            #[cfg(target_os = "macos")]
-            managed_preferences_base64: None,
-            macos_managed_config_requirements_base64: None,
-        },
+        test_loader_overrides(Some(managed_path.clone())),
         CloudRequirementsLoader::default(),
     );
 
@@ -357,12 +357,7 @@ async fn invalid_user_value_rejected_even_if_overridden_by_managed() {
     let service = ConfigService::new(
         tmp.path().to_path_buf(),
         vec![],
-        LoaderOverrides {
-            managed_config_path: Some(managed_path.clone()),
-            #[cfg(target_os = "macos")]
-            managed_preferences_base64: None,
-            macos_managed_config_requirements_base64: None,
-        },
+        test_loader_overrides(Some(managed_path.clone())),
         CloudRequirementsLoader::default(),
     );
 
@@ -422,12 +417,7 @@ async fn write_value_rejects_feature_requirement_conflict() {
     let service = ConfigService::new(
         tmp.path().to_path_buf(),
         vec![],
-        LoaderOverrides {
-            managed_config_path: None,
-            #[cfg(target_os = "macos")]
-            managed_preferences_base64: None,
-            macos_managed_config_requirements_base64: None,
-        },
+        test_loader_overrides(None),
         CloudRequirementsLoader::new(async {
             Ok(Some(ConfigRequirementsToml {
                 feature_requirements: Some(crate::config_loader::FeatureRequirementsToml {
@@ -473,12 +463,7 @@ async fn write_value_rejects_profile_feature_requirement_conflict() {
     let service = ConfigService::new(
         tmp.path().to_path_buf(),
         vec![],
-        LoaderOverrides {
-            managed_config_path: None,
-            #[cfg(target_os = "macos")]
-            managed_preferences_base64: None,
-            macos_managed_config_requirements_base64: None,
-        },
+        test_loader_overrides(None),
         CloudRequirementsLoader::new(async {
             Ok(Some(ConfigRequirementsToml {
                 feature_requirements: Some(crate::config_loader::FeatureRequirementsToml {
@@ -535,12 +520,7 @@ async fn read_reports_managed_overrides_user_and_session_flags() {
     let service = ConfigService::new(
         tmp.path().to_path_buf(),
         cli_overrides,
-        LoaderOverrides {
-            managed_config_path: Some(managed_path.clone()),
-            #[cfg(target_os = "macos")]
-            managed_preferences_base64: None,
-            macos_managed_config_requirements_base64: None,
-        },
+        test_loader_overrides(Some(managed_path.clone())),
         CloudRequirementsLoader::default(),
     );
 
@@ -593,12 +573,7 @@ async fn write_value_reports_managed_override() {
     let service = ConfigService::new(
         tmp.path().to_path_buf(),
         vec![],
-        LoaderOverrides {
-            managed_config_path: Some(managed_path.clone()),
-            #[cfg(target_os = "macos")]
-            managed_preferences_base64: None,
-            macos_managed_config_requirements_base64: None,
-        },
+        test_loader_overrides(Some(managed_path.clone())),
         CloudRequirementsLoader::default(),
     );