From 00c454ca82265d243a877700723cedd8f9681036 Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Tue, 18 Nov 2025 11:28:50 -0800 Subject: [PATCH] load execpolicy from codex_home/policy and default to empty policy --- codex-rs/core/src/codex.rs | 10 +- codex-rs/core/src/exec_policy.rs | 200 +++++++++++++----- codex-rs/core/src/tools/handlers/shell.rs | 2 +- .../core/src/unified_exec/session_manager.rs | 2 +- codex-rs/core/tests/suite/exec_policy.rs | 10 +- codex-rs/execpolicy2/src/policy.rs | 4 + 6 files changed, 166 insertions(+), 62 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 047295728e..c5700482f6 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -285,7 +285,7 @@ pub(crate) struct TurnContext { pub(crate) final_output_json_schema: Option, pub(crate) codex_linux_sandbox_exe: Option, pub(crate) tool_call_gate: Arc, - pub(crate) exec_policy: Option>, + pub(crate) exec_policy: Arc, pub(crate) truncation_policy: TruncationPolicy, } @@ -342,8 +342,8 @@ pub(crate) struct SessionConfiguration { /// Set of feature flags for this session features: Features, - /// Optional execpolicy policy, applied only when enabled by feature flag. - exec_policy: Option>, + /// Execpolicy policy, applied only when enabled by feature flag. + exec_policy: Arc, // TODO(pakrym): Remove config from here original_config_do_not_use: Arc, @@ -2618,7 +2618,7 @@ mod tests { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: Features::default(), - exec_policy: None, + exec_policy: Arc::new(codex_execpolicy2::PolicyParser::new().build()), session_source: SessionSource::Exec, }; @@ -2696,7 +2696,7 @@ mod tests { cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), features: Features::default(), - exec_policy: None, + exec_policy: Arc::new(codex_execpolicy2::PolicyParser::new().build()), session_source: SessionSource::Exec, }; diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 7920a9d906..588f248e48 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -1,4 +1,7 @@ use std::fs; +use std::fs::OpenOptions; +use std::io::ErrorKind; +use std::io::Write; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -19,6 +22,10 @@ use crate::tools::sandboxing::ApprovalRequirement; const FORBIDDEN_REASON: &str = "execpolicy forbids this command"; const PROMPT_REASON: &str = "execpolicy requires approval for this command"; +const POLICY_DIR_NAME: &str = "policy"; +const POLICY_EXTENSION: &str = "codexpolicy"; +const DEFAULT_POLICY_FILENAME: &str = "default.codexpolicy"; +const DEFAULT_POLICY_CONTENT: &str = "# Codex execpolicy rules\n"; #[derive(Debug, Error)] pub enum ExecPolicyError { @@ -39,51 +46,33 @@ pub enum ExecPolicyError { path: String, source: codex_execpolicy2::Error, }, + + #[error("failed to create execpolicy directory {path}: {source}")] + CreatePolicyDir { + path: PathBuf, + source: std::io::Error, + }, + + #[error("failed to create default execpolicy file {path}: {source}")] + CreateDefaultPolicy { + path: PathBuf, + source: std::io::Error, + }, } pub(crate) fn exec_policy_for( features: &Features, codex_home: &Path, -) -> Result>, ExecPolicyError> { +) -> Result, ExecPolicyError> { if !features.enabled(Feature::ExecPolicy) { - return Ok(None); + return Ok(Arc::new(Policy::empty())); } - let policy_dir = codex_home.to_path_buf(); - let entries = match fs::read_dir(&policy_dir) { - Ok(entries) => entries, - Err(source) if source.kind() == std::io::ErrorKind::NotFound => return Ok(None), - Err(source) => { - return Err(ExecPolicyError::ReadDir { - dir: policy_dir, - source, - }); - } - }; + let policy_dir = codex_home.join(POLICY_DIR_NAME); + ensure_policy_dir(&policy_dir)?; + ensure_default_policy_file(&policy_dir)?; - let entries = entries - .map(|entry| { - entry.map_err(|source| ExecPolicyError::ReadDir { - dir: policy_dir.clone(), - source, - }) - }) - .collect::, _>>()?; - - let mut policy_paths: Vec = Vec::with_capacity(entries.len()); - for entry in entries { - let path = entry.path(); - if path - .extension() - .and_then(|ext| ext.to_str()) - .is_some_and(|ext| ext == "codexpolicy") - && path.is_file() - { - policy_paths.push(path); - } - } - - policy_paths.sort(); + let policy_paths = collect_policy_files(&policy_dir)?; let mut parser = PolicyParser::new(); for policy_path in &policy_paths { @@ -108,7 +97,7 @@ pub(crate) fn exec_policy_for( policy_dir.display() ); - Ok(Some(policy)) + Ok(policy) } fn evaluate_with_policy( @@ -141,15 +130,13 @@ fn evaluate_with_policy( } pub(crate) fn approval_requirement_for_command( - policy: Option<&Policy>, + policy: &Policy, command: &[String], approval_policy: AskForApproval, sandbox_policy: &SandboxPolicy, with_escalated_permissions: bool, ) -> ApprovalRequirement { - if let Some(policy) = policy - && let Some(requirement) = evaluate_with_policy(policy, command, approval_policy) - { + if let Some(requirement) = evaluate_with_policy(policy, command, approval_policy) { return requirement; } @@ -165,6 +152,71 @@ pub(crate) fn approval_requirement_for_command( } } +fn collect_policy_files(dir: &Path) -> Result, ExecPolicyError> { + let entries = fs::read_dir(dir).map_err(|source| ExecPolicyError::ReadDir { + dir: dir.to_path_buf(), + source, + })?; + + let mut policy_paths = entries + .map(|entry| { + let entry = entry.map_err(|source| ExecPolicyError::ReadDir { + dir: dir.to_path_buf(), + source, + })?; + let path = entry.path(); + if path + .extension() + .and_then(|ext| ext.to_str()) + .is_some_and(|ext| ext == POLICY_EXTENSION) + && path.is_file() + { + Ok(Some(path)) + } else { + Ok(None) + } + }) + .collect::, _>>()? + .into_iter() + .flatten() + .collect::>(); + + policy_paths.sort(); + + Ok(policy_paths) +} + +fn ensure_policy_dir(policy_dir: &Path) -> Result<(), ExecPolicyError> { + match fs::create_dir_all(policy_dir) { + Ok(()) => Ok(()), + Err(source) => Err(ExecPolicyError::CreatePolicyDir { + path: policy_dir.to_path_buf(), + source, + }), + } +} + +fn ensure_default_policy_file(policy_dir: &Path) -> Result<(), ExecPolicyError> { + let default_path = policy_dir.join(DEFAULT_POLICY_FILENAME); + let maybe_file = OpenOptions::new() + .create_new(true) + .write(true) + .open(&default_path); + match maybe_file { + Ok(mut file) => file + .write_all(DEFAULT_POLICY_CONTENT.as_bytes()) + .map_err(|source| ExecPolicyError::CreateDefaultPolicy { + path: default_path.clone(), + source, + }), + Err(err) if err.kind() == ErrorKind::AlreadyExists => Ok(()), + Err(source) => Err(ExecPolicyError::CreateDefaultPolicy { + path: default_path, + source, + }), + } +} + #[cfg(test)] mod tests { use super::*; @@ -176,26 +228,69 @@ mod tests { use tempfile::tempdir; #[test] - fn returns_none_when_feature_disabled() { + fn returns_empty_policy_when_feature_disabled() { let mut features = Features::with_defaults(); features.disable(Feature::ExecPolicy); let temp_dir = tempdir().expect("create temp dir"); let policy = exec_policy_for(&features, temp_dir.path()).expect("policy result"); - assert!(policy.is_none()); + let commands = [vec!["rm".to_string()]]; + assert!(matches!( + policy.check_multiple(commands.iter()), + Evaluation::NoMatch + )); + assert!(!temp_dir.path().join(POLICY_DIR_NAME).exists()); } #[test] - fn returns_none_when_policy_dir_is_missing() { - let mut features = Features::with_defaults(); - features.enable(Feature::ExecPolicy); + fn creates_policy_dir_and_default_file_when_missing() { let temp_dir = tempdir().expect("create temp dir"); - let missing_dir = temp_dir.path().join("missing"); - let policy = exec_policy_for(&features, &missing_dir).expect("policy result"); + let _policy = + exec_policy_for(&Features::with_defaults(), temp_dir.path()).expect("policy result"); - assert!(policy.is_none()); + let policy_dir = temp_dir.path().join(POLICY_DIR_NAME); + assert!(policy_dir.exists()); + assert!(policy_dir.join(DEFAULT_POLICY_FILENAME).exists()); + } + + #[test] + fn loads_policies_from_policy_subdirectory() { + let temp_dir = tempdir().expect("create temp dir"); + let policy_dir = temp_dir.path().join(POLICY_DIR_NAME); + fs::create_dir_all(&policy_dir).expect("create policy dir"); + fs::write( + policy_dir.join("deny.codexpolicy"), + r#"prefix_rule(pattern=["rm"], decision="forbidden")"#, + ) + .expect("write policy file"); + + let policy = + exec_policy_for(&Features::with_defaults(), temp_dir.path()).expect("policy result"); + let command = [vec!["rm".to_string()]]; + assert!(matches!( + policy.check_multiple(command.iter()), + Evaluation::Match { .. } + )); + } + + #[test] + fn ignores_policies_outside_policy_dir() { + let temp_dir = tempdir().expect("create temp dir"); + fs::write( + temp_dir.path().join("root.codexpolicy"), + r#"prefix_rule(pattern=["ls"], decision="prompt")"#, + ) + .expect("write policy file"); + + let policy = + exec_policy_for(&Features::with_defaults(), temp_dir.path()).expect("policy result"); + let command = [vec!["ls".to_string()]]; + assert!(matches!( + policy.check_multiple(command.iter()), + Evaluation::NoMatch + )); } #[test] @@ -238,7 +333,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") let command = vec!["rm".to_string()]; let requirement = approval_requirement_for_command( - Some(&policy), + &policy, &command, AskForApproval::OnRequest, &SandboxPolicy::DangerFullAccess, @@ -264,7 +359,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") let command = vec!["rm".to_string()]; let requirement = approval_requirement_for_command( - Some(&policy), + &policy, &command, AskForApproval::Never, &SandboxPolicy::DangerFullAccess, @@ -283,8 +378,9 @@ prefix_rule(pattern=["rm"], decision="forbidden") fn approval_requirement_falls_back_to_heuristics() { let command = vec!["python".to_string()]; + let empty_policy = Policy::empty(); let requirement = approval_requirement_for_command( - None, + &empty_policy, &command, AskForApproval::UnlessTrusted, &SandboxPolicy::ReadOnly, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 14c750f080..30b168f7b8 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -304,7 +304,7 @@ impl ShellHandler { with_escalated_permissions: exec_params.with_escalated_permissions, justification: exec_params.justification.clone(), approval_requirement: approval_requirement_for_command( - turn.exec_policy.as_deref(), + &turn.exec_policy, &exec_params.command, turn.approval_policy, &turn.sandbox_policy, diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index a72ee9035d..df0477bc34 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -451,7 +451,7 @@ impl UnifiedExecSessionManager { with_escalated_permissions, justification, approval_requirement_for_command( - context.turn.exec_policy.as_deref(), + &context.turn.exec_policy, command, context.turn.approval_policy, &context.turn.sandbox_policy, diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 04d1e52eb8..bca917a252 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -1,7 +1,6 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Result; -use codex_core::features::Feature; use codex_core::protocol::AskForApproval; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; @@ -23,8 +22,13 @@ use std::fs; #[tokio::test] async fn execpolicy_blocks_shell_invocation() -> Result<()> { let mut builder = test_codex().with_config(|config| { - config.features.enable(Feature::ExecPolicy); - let policy_path = config.codex_home.join("policy.codexpolicy"); + let policy_path = config.codex_home.join("policy").join("policy.codexpolicy"); + fs::create_dir_all( + policy_path + .parent() + .expect("policy directory must have a parent"), + ) + .expect("create policy directory"); fs::write( &policy_path, r#"prefix_rule(pattern=["echo"], decision="forbidden")"#, diff --git a/codex-rs/execpolicy2/src/policy.rs b/codex-rs/execpolicy2/src/policy.rs index 12416b050b..a543e5c4b0 100644 --- a/codex-rs/execpolicy2/src/policy.rs +++ b/codex-rs/execpolicy2/src/policy.rs @@ -15,6 +15,10 @@ impl Policy { Self { rules_by_program } } + pub fn empty() -> Self { + Self::new(MultiMap::new()) + } + pub fn rules(&self) -> &MultiMap { &self.rules_by_program }