From dbb36b892ad3ab6c97d2979d30346215ec8fbfc2 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Mon, 2 Mar 2026 11:32:43 -0800 Subject: [PATCH] fix(permissions): make deny_read sandbox constraints explicit --- codex-rs/config/src/constraint.rs | 51 +++++++++++++++ codex-rs/core/src/config/mod.rs | 105 +++++++++++++++++++++++++----- codex-rs/protocol/src/protocol.rs | 24 +------ 3 files changed, 141 insertions(+), 39 deletions(-) diff --git a/codex-rs/config/src/constraint.rs b/codex-rs/config/src/constraint.rs index cddccb7d1f..899f2c5b50 100644 --- a/codex-rs/config/src/constraint.rs +++ b/codex-rs/config/src/constraint.rs @@ -138,6 +138,27 @@ impl Constrained { (self.validator)(candidate) } + /// Composes an additional validator onto the current constraint. + /// + /// The existing value must satisfy the combined validator before it is installed. + pub fn add_validator( + &mut self, + validator: impl Fn(&T) -> ConstraintResult<()> + Send + Sync + 'static, + ) -> ConstraintResult<()> + where + T: 'static, + { + let existing_validator = self.validator.clone(); + let combined_validator: Arc> = Arc::new(move |candidate| { + existing_validator(candidate)?; + validator(candidate) + }); + + combined_validator(&self.value)?; + self.validator = combined_validator; + Ok(()) + } + pub fn set(&mut self, value: T) -> ConstraintResult<()> { let value = if let Some(normalizer) = &self.normalizer { normalizer(value) @@ -264,6 +285,36 @@ mod tests { Ok(()) } + #[test] + fn constrained_add_validator_composes_with_existing_validator() -> anyhow::Result<()> { + let mut constrained = Constrained::new(5, |value: &i32| { + if *value >= 0 { + Ok(()) + } else { + Err(ConstraintError::empty_field("value")) + } + })?; + constrained.add_validator(|value| { + if *value <= 10 { + Ok(()) + } else { + Err(ConstraintError::empty_field("value")) + } + })?; + + assert_eq!(constrained.can_set(&7), Ok(())); + assert_eq!( + constrained.can_set(&11), + Err(ConstraintError::empty_field("value")) + ); + assert_eq!( + constrained.can_set(&-1), + Err(ConstraintError::empty_field("value")) + ); + + Ok(()) + } + #[test] fn constrained_new_rejects_invalid_initial_value() { let result = Constrained::new(0, |value| { diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index d91776a020..5f6e5ebd35 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1897,12 +1897,6 @@ impl Config { &mut constrained_approval_policy, &mut startup_warnings, )?; - apply_requirement_constrained_value( - "sandbox_mode", - sandbox_policy, - &mut constrained_sandbox_policy, - &mut startup_warnings, - )?; if let Some(Sourced { value: filesystem_requirements, source: filesystem_requirements_source, @@ -1910,6 +1904,21 @@ impl Config { && !filesystem_requirements.deny_read.is_empty() { let deny_read_paths = expand_deny_read_patterns(&filesystem_requirements.deny_read); + let requirement_source = filesystem_requirements_source.clone(); + constrained_sandbox_policy + .value + .add_validator(move |policy| match policy { + SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. } => Ok(()), + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { + Err(ConstraintError::InvalidValue { + field_name: "sandbox_mode", + candidate: policy.to_string(), + allowed: "[read-only, workspace-write]".to_string(), + requirement_source: requirement_source.clone(), + }) + } + }) + .map_err(std::io::Error::from)?; constrained_sandbox_policy .value .add_normalizer(move |mut policy| { @@ -1924,6 +1933,12 @@ impl Config { )); } } + apply_requirement_constrained_value( + "sandbox_mode", + sandbox_policy, + &mut constrained_sandbox_policy, + &mut startup_warnings, + )?; apply_requirement_constrained_value( "web_search_mode", web_search_mode, @@ -5667,7 +5682,6 @@ mcp_oauth_callback_url = "https://example.com/callback" config.permissions.sandbox_policy.get().denied_read_paths(), vec![top_match, nested_match] ); - Ok(()) } @@ -5715,12 +5729,10 @@ mcp_oauth_callback_url = "https://example.com/callback" } #[tokio::test] - async fn requirements_filesystem_deny_read_normalizes_danger_full_access() -> std::io::Result<()> - { + async fn requirements_filesystem_deny_read_rejects_unsupported_sandbox_mode_changes() + -> std::io::Result<()> { let codex_home = TempDir::new()?; let denied_path = codex_home.path().join("sensitive").join("secret.txt"); - let denied_path = - AbsolutePathBuf::try_from(denied_path).expect("deny_read test path should be absolute"); let mut config = ConfigBuilder::default() .codex_home(codex_home.path().to_path_buf()) @@ -5743,18 +5755,75 @@ mcp_oauth_callback_url = "https://example.com/callback" .build() .await?; - config + let err = config .permissions .sandbox_policy .set(SandboxPolicy::DangerFullAccess) - .map_err(std::io::Error::from)?; + .expect_err("danger-full-access should be rejected"); - assert_eq!( - *config.permissions.sandbox_policy.get(), - SandboxPolicy::ExternalSandbox { + assert!(err.to_string().contains("sandbox_mode")); + assert!(err.to_string().contains("read-only, workspace-write")); + + let err = config + .permissions + .sandbox_policy + .set(SandboxPolicy::ExternalSandbox { network_access: crate::protocol::NetworkAccess::Enabled, - deny_read_paths: vec![denied_path], - } + deny_read_paths: vec![], + }) + .expect_err("external-sandbox should be rejected"); + + assert!(err.to_string().contains("sandbox_mode")); + assert!(err.to_string().contains("read-only, workspace-write")); + Ok(()) + } + + #[tokio::test] + async fn explicit_sandbox_mode_falls_back_when_deny_read_rejects_full_access() + -> std::io::Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"sandbox_mode = "danger-full-access" +"#, + )?; + + let denied_path = + AbsolutePathBuf::try_from(codex_home.path().join("sensitive").join("secret.txt")) + .expect("deny_read test path should be absolute"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_requirements(CloudRequirementsLoader::new({ + let denied_path = denied_path.clone(); + async move { + Some( + toml::from_str::(&format!( + r#" + [permissions.filesystem] + deny_read = [{:?}] + "#, + denied_path.as_path().display().to_string() + )) + .expect("parse requirements toml"), + ) + } + })) + .build() + .await?; + + let policy = config.permissions.sandbox_policy.get(); + + assert!(matches!( + policy, + SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. } + )); + assert_eq!(policy.denied_read_paths(), vec![denied_path]); + assert!( + config.startup_warnings.iter().any( + |warning| warning.contains("Configured value for `sandbox_mode` is disallowed") + ) ); Ok(()) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 082c508f94..ca88ed3d9c 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -738,19 +738,7 @@ impl SandboxPolicy { | SandboxPolicy::WorkspaceWrite { deny_read_paths, .. } => deny_read_paths, - SandboxPolicy::DangerFullAccess => { - *self = SandboxPolicy::ExternalSandbox { - network_access: NetworkAccess::Enabled, - deny_read_paths: Vec::new(), - }; - let SandboxPolicy::ExternalSandbox { - deny_read_paths, .. - } = self - else { - unreachable!("danger-full-access should normalize to external-sandbox"); - }; - deny_read_paths - } + SandboxPolicy::DangerFullAccess => return, }; target_paths.extend(new_paths.iter().cloned()); @@ -3037,7 +3025,7 @@ mod tests { } #[test] - fn append_deny_read_paths_normalizes_danger_full_access() { + fn append_deny_read_paths_ignores_danger_full_access() { let denied_path = if cfg!(windows) { AbsolutePathBuf::try_from(r"C:\sensitive\secret.txt").expect("absolute path") } else { @@ -3047,13 +3035,7 @@ mod tests { policy.append_deny_read_paths(std::slice::from_ref(&denied_path)); - assert_eq!( - policy, - SandboxPolicy::ExternalSandbox { - network_access: NetworkAccess::Enabled, - deny_read_paths: vec![denied_path], - } - ); + assert_eq!(policy, SandboxPolicy::DangerFullAccess); } #[test]