mirror of
https://github.com/openai/codex.git
synced 2026-10-17 17:08:55 +00:00
Migrate legacy exec policy allow rules (#34271)
## What changed - On session startup, remove exact `allow` entries from `rules/default.rules` for command prefixes that Codex no longer suggests as policy amendments. - Record the migration in `.sandbox_migration` so it runs only once, preserving rules created after the migration. - Skip the migration when user and project exec policy rules are ignored. - Expand the protected prefix list across shells, interpreters, package runners, and destructive or privilege-related commands. ## Testing - Cover selective removal, case-insensitive matching, one-time behavior, and the startup path with ignored policy rules. GitOrigin-RevId: a0c60e3f82b9630e621fd034b40462e3ab775102
This commit is contained in:
Generated
+1
@@ -2969,6 +2969,7 @@ dependencies = [
|
||||
"starlark",
|
||||
"tempfile",
|
||||
"thiserror 2.0.18",
|
||||
"tokio",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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()],
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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 }
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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::<Vec<_>>()
|
||||
})
|
||||
.collect::<HashSet<_>>();
|
||||
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<Vec<String>>) -> 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::<Vec<String>>(pattern) else {
|
||||
return false;
|
||||
};
|
||||
let prefix = prefix
|
||||
.iter()
|
||||
.map(|token| token.to_ascii_lowercase())
|
||||
.collect::<Vec<_>>();
|
||||
banned_prefixes.contains(&prefix)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "sandbox_migration_tests.rs"]
|
||||
mod tests;
|
||||
@@ -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
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user