diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 471c46b94e..54f68da232 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2969,6 +2969,7 @@ dependencies = [ "starlark", "tempfile", "thiserror 2.0.18", + "tokio", ] [[package]] diff --git a/codex-rs/app-server/tests/suite/v2/remote_thread_store.rs b/codex-rs/app-server/tests/suite/v2/remote_thread_store.rs index f78b253b3b..1cb7e53a0b 100644 --- a/codex-rs/app-server/tests/suite/v2/remote_thread_store.rs +++ b/codex-rs/app-server/tests/suite/v2/remote_thread_store.rs @@ -432,6 +432,7 @@ fn assert_no_local_persistence_artifacts(codex_home: &Path) -> Result<()> { assert_eq!( entries, BTreeSet::from([ + ".sandbox_migration".to_string(), "config.toml".to_string(), "installation_id".to_string(), "skills".to_string(), diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 7aae784d34..92b54d135f 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -50,53 +50,95 @@ const REJECT_RULES_APPROVAL_REASON: &str = const RULES_DIR_NAME: &str = "rules"; const RULE_EXTENSION: &str = "rules"; const DEFAULT_POLICY_FILE: &str = "default.rules"; -static BANNED_PREFIX_SUGGESTIONS: &[&[&str]] = &[ - &["python3"], - &["python3", "-"], - &["python3", "-c"], - &["python"], - &["python", "-"], - &["python", "-c"], - &["py"], - &["py", "-3"], - &["pythonw"], - &["pyw"], - &["pypy"], - &["pypy3"], - &["git"], - &["bash"], - &["bash", "-lc"], - &["sh"], - &["sh", "-c"], - &["sh", "-lc"], - &["zsh"], - &["zsh", "-lc"], - &["/bin/zsh"], - &["/bin/zsh", "-lc"], +pub(crate) static BANNED_PREFIX_SUGGESTIONS: &[&[&str]] = &[ &["/bin/bash"], + &["/bin/bash", "-c"], &["/bin/bash", "-lc"], - &["pwsh"], - &["pwsh", "-Command"], - &["pwsh", "-c"], + &["/bin/sh"], + &["/bin/sh", "-c"], + &["/bin/sh", "-lc"], + &["/bin/zsh"], + &["/bin/zsh", "-c"], + &["/bin/zsh", "-lc"], + &["Rscript"], + &["bash"], + &["bash", "-c"], + &["bash", "-lc"], + &["bun"], + &["bun", "-e"], + &["bun", "run"], + &["cmd"], + &["cmd", "/c"], + &["cmd", "/k"], + &["cmd.exe"], + &["cmd.exe", "/c"], + &["cmd.exe", "/k"], + &["dash"], + &["dash", "-c"], + &["deno"], + &["deno", "eval"], + &["env"], + &["fish"], + &["fish", "-c"], + &["git"], + &["julia"], + &["julia", "-e"], + &["ksh"], + &["ksh", "-c"], + &["lua"], + &["lua", "-e"], + &["node"], + &["node", "-e"], + &["nodejs"], + &["nodejs", "-e"], + &["npm", "run"], + &["osascript"], + &["perl"], + &["perl", "-e"], + &["php"], + &["php", "-r"], + &["pnpm", "run"], &["powershell"], &["powershell", "-Command"], + &["powershell", "-EncodedCommand"], + &["powershell", "-File"], &["powershell", "-c"], &["powershell.exe"], &["powershell.exe", "-Command"], + &["powershell.exe", "-EncodedCommand"], + &["powershell.exe", "-File"], &["powershell.exe", "-c"], - &["env"], - &["sudo"], - &["node"], - &["node", "-e"], - &["perl"], - &["perl", "-e"], + &["pwsh"], + &["pwsh", "-Command"], + &["pwsh", "-EncodedCommand"], + &["pwsh", "-File"], + &["pwsh", "-c"], + &["pwsh", "-e"], + &["pwsh", "-ec"], + &["pwsh", "-f"], + &["py"], + &["py", "-3"], + &["pypy"], + &["pypy3"], + &["python"], + &["python", "-"], + &["python", "-c"], + &["python3"], + &["python3", "-"], + &["python3", "-c"], + &["pythonw"], + &["pyw"], + &["rm"], &["ruby"], &["ruby", "-e"], - &["php"], - &["php", "-r"], - &["lua"], - &["lua", "-e"], - &["osascript"], + &["sh"], + &["sh", "-c"], + &["sh", "-lc"], + &["sudo"], + &["yarn", "run"], + &["zsh"], + &["zsh", "-c"], + &["zsh", "-lc"], ]; /// Describes which unmatched-command heuristics should classify the command @@ -796,7 +838,7 @@ fn profile_has_managed_filesystem_restrictions(permission_profile: &PermissionPr && !file_system_sandbox_policy.has_full_disk_write_access() } -fn default_policy_path(codex_home: &Path) -> PathBuf { +pub(crate) fn default_policy_path(codex_home: &Path) -> PathBuf { codex_home.join(RULES_DIR_NAME).join(DEFAULT_POLICY_FILE) } diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 4b47cdab3d..5b3e33d3a3 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1946,6 +1946,7 @@ fn derive_requested_execpolicy_amendment_returns_none_for_shell_and_powershell_v vec!["pwsh".to_string()], vec!["pwsh".to_string(), "-Command".to_string()], vec!["pwsh".to_string(), "-c".to_string()], + vec!["pwsh".to_string(), "-ec".to_string()], vec!["powershell".to_string()], vec!["powershell".to_string(), "-Command".to_string()], vec!["powershell".to_string(), "-c".to_string()], diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index d434666594..b96c190e7b 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -33,7 +33,9 @@ use crate::context::world_state::WorldState; use crate::current_time::TimeProvider; use crate::default_skill_metadata_budget; use crate::environment_selection::TurnEnvironmentSnapshot; +use crate::exec_policy::BANNED_PREFIX_SUGGESTIONS; use crate::exec_policy::ExecPolicyManager; +use crate::exec_policy::default_policy_path; use crate::image_preparation::prepare_response_items as prepare_image_response_items; use crate::parse_turn_item; use crate::realtime_conversation::RealtimeConversationManager; @@ -57,6 +59,7 @@ use codex_connectors::connector_runtime_context_key; use codex_core_skills::injection::HostSkillsCatalogInWorldState; use codex_exec_server::Environment; use codex_exec_server::EnvironmentManager; +use codex_execpolicy::prefix_rule_migration; use codex_extension_api::ExtensionDataInit; use codex_extension_api::LoadedUserInstructions; use codex_extension_api::PromptFragment; @@ -549,6 +552,22 @@ impl Session { } else if let Some(exec_policy) = &inherited_exec_policy { Arc::clone(exec_policy) } else { + if !config + .config_layer_stack + .ignore_user_and_project_exec_policy_rules() + { + let codex_home = config.codex_home.clone(); + let policy_path = default_policy_path(codex_home.as_path()); + if let Err(err) = prefix_rule_migration( + codex_home.as_path(), + policy_path.as_path(), + BANNED_PREFIX_SUGGESTIONS, + ) + .await + { + tracing::warn!(error = %err, "failed to run prefix rule migration"); + } + } Arc::new( ExecPolicyManager::load(&config.config_layer_stack) .await diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index b699f6db1b..d2783b7a02 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -94,6 +94,55 @@ fn assert_no_matched_rules_invariant(output_item: &Value) { ); } +#[tokio::test] +async fn startup_migrates_default_policy_and_honors_ignore_rules() -> Result<()> { + const LEGACY_POLICY: &str = r#"prefix_rule(pattern=["rm"], decision="allow") +prefix_rule(pattern=["git", "status"], decision="allow") +"#; + const MIGRATED_POLICY: &str = r#"prefix_rule(pattern=["git", "status"], decision="allow") +"#; + const MIGRATION_MARKER_FILENAME: &str = ".sandbox_migration"; + + let server = start_mock_server().await; + let mut migrated_builder = test_codex().with_config(|config| { + let policy_path = config.codex_home.join("rules/default.rules"); + fs::create_dir_all(policy_path.parent().expect("rules directory")) + .expect("create rules directory"); + fs::write(policy_path, LEGACY_POLICY).expect("write legacy policy"); + }); + let migrated = migrated_builder.build_with_auto_env(&server).await?; + let migrated_policy_path = migrated.codex_home_path().join("rules/default.rules"); + assert_eq!(fs::read_to_string(&migrated_policy_path)?, MIGRATED_POLICY); + assert_eq!( + fs::read_to_string(migrated.codex_home_path().join(MIGRATION_MARKER_FILENAME))?, + "v1\n" + ); + + let mut ignored_builder = test_codex().with_config(|config| { + let policy_path = config.codex_home.join("rules/default.rules"); + fs::create_dir_all(policy_path.parent().expect("rules directory")) + .expect("create rules directory"); + fs::write(policy_path, LEGACY_POLICY).expect("write legacy policy"); + config.config_layer_stack = config + .config_layer_stack + .clone() + .with_user_and_project_exec_policy_rules_ignored( + /*ignore_user_and_project_exec_policy_rules*/ true, + ); + }); + let ignored = ignored_builder.build_with_auto_env(&server).await?; + let ignored_policy_path = ignored.codex_home_path().join("rules/default.rules"); + assert_eq!(fs::read_to_string(&ignored_policy_path)?, LEGACY_POLICY); + assert!( + !ignored + .codex_home_path() + .join(MIGRATION_MARKER_FILENAME) + .exists() + ); + + Ok(()) +} + #[tokio::test] async fn granular_complex_forced_rm_denial_explains_why_the_command_was_rejected() -> Result<()> { skip_if_target_windows!(Ok(()), "uses a POSIX shell command fixture"); diff --git a/codex-rs/execpolicy/Cargo.toml b/codex-rs/execpolicy/Cargo.toml index b22226a79e..4204022233 100644 --- a/codex-rs/execpolicy/Cargo.toml +++ b/codex-rs/execpolicy/Cargo.toml @@ -26,8 +26,9 @@ serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } shlex = { workspace = true } starlark = { workspace = true } +tempfile = { workspace = true } thiserror = { workspace = true } +tokio = { workspace = true, features = ["fs", "io-util", "macros", "rt"] } [dev-dependencies] pretty_assertions = { workspace = true } -tempfile = { workspace = true } diff --git a/codex-rs/execpolicy/src/lib.rs b/codex-rs/execpolicy/src/lib.rs index 45d3642415..495c0d74a3 100644 --- a/codex-rs/execpolicy/src/lib.rs +++ b/codex-rs/execpolicy/src/lib.rs @@ -6,6 +6,7 @@ mod executable_name; pub(crate) mod parser; pub(crate) mod policy; pub mod rule; +mod sandbox_migration; pub use amend::AmendError; pub use amend::blocking_append_allow_prefix_rule; @@ -28,3 +29,4 @@ pub use rule::PrefixRule; pub use rule::Rule; pub use rule::RuleMatch; pub use rule::RuleRef; +pub use sandbox_migration::prefix_rule_migration; diff --git a/codex-rs/execpolicy/src/sandbox_migration.rs b/codex-rs/execpolicy/src/sandbox_migration.rs new file mode 100644 index 0000000000..a2a57c1c4c --- /dev/null +++ b/codex-rs/execpolicy/src/sandbox_migration.rs @@ -0,0 +1,123 @@ +use std::collections::HashSet; +use std::io; +use std::io::SeekFrom; +use std::io::Write as _; +use std::path::Path; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncSeekExt; +use tokio::io::AsyncWriteExt; + +const MIGRATION_MARKER_FILENAME: &str = ".sandbox_migration"; + +/// removes legacy allow rules that newer codex versions no longer offer. +/// +/// this migration is intentionally one-shot. once complete, a marker in `codex_home` prevents +/// policies saved by newer codex versions from being removed on later startups. +pub async fn prefix_rule_migration( + codex_home: &Path, + policy_path: &Path, + banned_prefixes: &[&[&str]], +) -> io::Result<()> { + let marker_path = codex_home.join(MIGRATION_MARKER_FILENAME); + if tokio::fs::try_exists(&marker_path).await? { + return Ok(()); + } + clean_rules_file(policy_path, banned_prefixes).await?; + + write_migration_marker(codex_home, &marker_path).await?; + Ok(()) +} + +// atomically writes the marker after creating codex home when needed. +async fn write_migration_marker(codex_home: &Path, marker_path: &Path) -> io::Result<()> { + tokio::fs::create_dir_all(codex_home).await?; + let codex_home = codex_home.to_owned(); + let marker_path = marker_path.to_owned(); + tokio::task::spawn_blocking(move || { + let mut marker = tempfile::NamedTempFile::new_in(codex_home)?; + marker.write_all(b"v1\n")?; + match marker.persist_noclobber(marker_path) { + Ok(_) => Ok(()), + Err(err) if err.error.kind() == io::ErrorKind::AlreadyExists => Ok(()), + Err(err) => Err(err.error), + } + }) + .await + .map_err(io::Error::other)? +} + +// removes exact banned allow rules only when the policy needs changing. +async fn clean_rules_file(policy_path: &Path, banned_prefixes: &[&[&str]]) -> io::Result<()> { + let contents = match tokio::fs::read_to_string(policy_path).await { + Ok(contents) => contents, + Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(err) => return Err(err), + }; + if strip_banned_allow_rules(&contents, banned_prefixes) == contents { + return Ok(()); + } + + let mut file = match tokio::fs::OpenOptions::new() + .read(true) + .write(true) + .open(policy_path) + .await + { + Ok(file) => file, + Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(err) => return Err(err), + }; + + let mut contents = String::new(); + file.read_to_string(&mut contents).await?; + let retained = strip_banned_allow_rules(&contents, banned_prefixes); + if retained == contents { + return Ok(()); + } + + file.seek(SeekFrom::Start(0)).await?; + file.write_all(retained.as_bytes()).await?; + file.set_len(retained.len() as u64).await?; + Ok(()) +} + +// returns the policy text without exact banned allow rules. +fn strip_banned_allow_rules(contents: &str, banned_prefixes: &[&[&str]]) -> String { + let banned_prefixes = banned_prefixes + .iter() + .map(|prefix| { + prefix + .iter() + .map(|token| token.to_ascii_lowercase()) + .collect::>() + }) + .collect::>(); + contents + .split_inclusive('\n') + .filter(|line| !should_remove_rule(line, &banned_prefixes)) + .collect() +} + +// checks whether a line is an exact banned allow rule. +fn should_remove_rule(line: &str, banned_prefixes: &HashSet>) -> bool { + let line = line.strip_suffix('\n').unwrap_or(line); + let line = line.strip_suffix('\r').unwrap_or(line); + let Some(pattern) = line + .strip_prefix("prefix_rule(pattern=") + .and_then(|line| line.strip_suffix(r#", decision="allow")"#)) + else { + return false; + }; + let Ok(prefix) = serde_json::from_str::>(pattern) else { + return false; + }; + let prefix = prefix + .iter() + .map(|token| token.to_ascii_lowercase()) + .collect::>(); + banned_prefixes.contains(&prefix) +} + +#[cfg(test)] +#[path = "sandbox_migration_tests.rs"] +mod tests; diff --git a/codex-rs/execpolicy/src/sandbox_migration_tests.rs b/codex-rs/execpolicy/src/sandbox_migration_tests.rs new file mode 100644 index 0000000000..5bb6af6e25 --- /dev/null +++ b/codex-rs/execpolicy/src/sandbox_migration_tests.rs @@ -0,0 +1,58 @@ +use super::*; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +#[tokio::test] +async fn removes_banned_allow_rules_once() { + const BANNED_PREFIXES: &[&[&str]] = &[ + &["cmd.exe", "/k"], + &["git"], + &["pwsh", "-ec"], + &["pwsh", "-f"], + ]; + let codex_home = tempdir().expect("create codex home"); + let policy_path = codex_home.path().join("rules/default.rules"); + std::fs::create_dir_all(policy_path.parent().expect("rules directory")) + .expect("create rules directory"); + std::fs::write( + &policy_path, + r#"prefix_rule(pattern=["git"], decision="allow") +prefix_rule(pattern=["git"], decision="prompt") +prefix_rule(pattern=["git"], decision="deny") +prefix_rule(pattern=["git", "status"], decision="allow") +prefix_rule(pattern=["CMD.EXE", "/K"], decision="allow") +prefix_rule(pattern=["PWSH", "-EC"], decision="allow") +prefix_rule(pattern=["PwSh", "-F"], decision="allow") +network_rule(host="api.github.com", protocol="https", decision="allow") +"#, + ) + .expect("write legacy policy"); + + prefix_rule_migration(codex_home.path(), &policy_path, BANNED_PREFIXES) + .await + .expect("run sandbox migration"); + assert_eq!( + std::fs::read_to_string(&policy_path).expect("read migrated policy"), + r#"prefix_rule(pattern=["git"], decision="prompt") +prefix_rule(pattern=["git"], decision="deny") +prefix_rule(pattern=["git", "status"], decision="allow") +network_rule(host="api.github.com", protocol="https", decision="allow") +"# + ); + assert_eq!( + std::fs::read_to_string(codex_home.path().join(MIGRATION_MARKER_FILENAME)) + .expect("read migration marker"), + "v1\n" + ); + + let post_migration_policy = r#"prefix_rule(pattern=["git"], decision="allow") +"#; + std::fs::write(&policy_path, post_migration_policy).expect("write post-migration policy"); + prefix_rule_migration(codex_home.path(), &policy_path, BANNED_PREFIXES) + .await + .expect("rerun sandbox migration"); + assert_eq!( + std::fs::read_to_string(&policy_path).expect("read post-migration policy"), + post_migration_policy + ); +}