From 74f29c7e2d12b594d4620f7d400d8a42baaa419a Mon Sep 17 00:00:00 2001 From: Eva Wong Date: Mon, 27 Apr 2026 13:49:40 -0700 Subject: [PATCH 1/2] Add workspace metadata shell preflight --- codex-rs/Cargo.lock | 1 + codex-rs/cli/Cargo.toml | 1 + codex-rs/cli/src/debug_sandbox.rs | 97 ++++++++ codex-rs/core/src/tools/handlers/shell.rs | 8 + codex-rs/shell-command/src/bash.rs | 224 +++++++++++++++++++ codex-rs/shell-command/src/lib.rs | 2 + codex-rs/shell-command/src/metadata_write.rs | 136 +++++++++++ 7 files changed, 469 insertions(+) create mode 100644 codex-rs/shell-command/src/metadata_write.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 51667c60132b..aafcbdd1b906 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2126,6 +2126,7 @@ dependencies = [ "codex-rmcp-client", "codex-rollout-trace", "codex-sandboxing", + "codex-shell-command", "codex-state", "codex-stdio-to-uds", "codex-terminal-detection", diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index cdee241b4252..8601de5851e6 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -44,6 +44,7 @@ codex-responses-api-proxy = { workspace = true } codex-rmcp-client = { workspace = true } codex-rollout-trace = { workspace = true } codex-sandboxing = { workspace = true } +codex-shell-command = { workspace = true } codex-state = { workspace = true } codex-stdio-to-uds = { workspace = true } codex-terminal-detection = { workspace = true } diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index c85da0f5f2d0..73ca85502c63 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -35,6 +35,10 @@ use crate::exit_status::handle_exit_status; #[cfg(target_os = "macos")] use seatbelt::DenialLogger; +#[cfg(target_os = "linux")] +const LINUX_SANDBOX_FORWARDED_SIGNALS: &[libc::c_int] = + &[libc::SIGHUP, libc::SIGINT, libc::SIGQUIT, libc::SIGTERM]; + #[cfg(target_os = "macos")] pub async fn run_command_under_seatbelt( command: SeatbeltCommand, @@ -142,6 +146,13 @@ async fn run_command_under_sandbox( // sandbox policy. In the future, we could add a CLI option to set them // separately. let sandbox_policy_cwd = cwd.clone(); + if let Some(reason) = codex_shell_command::metadata_write_forbidden_reason( + &command, + cwd.as_path(), + &config.permissions.file_system_sandbox_policy(), + ) { + anyhow::bail!("{reason}"); + } let env = create_env( &config.permissions.shell_environment_policy, @@ -261,6 +272,9 @@ async fn run_command_under_sandbox( denial_logger.on_child_spawn(&child); } + #[cfg(target_os = "linux")] + let status = wait_for_debug_sandbox_child(&mut child).await?; + #[cfg(not(target_os = "linux"))] let status = child.wait().await?; #[cfg(target_os = "macos")] @@ -438,6 +452,27 @@ async fn spawn_debug_sandbox_child( cmd.env(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR, "1"); } + #[cfg(target_os = "linux")] + { + let parent_pid = unsafe { libc::getpid() }; + // SAFETY: `pre_exec` runs in the child immediately before exec. The + // closure only adjusts the child signal mask, installs a parent-death + // signal, and checks the inherited parent pid to close the fork/exec + // race. + unsafe { + cmd.pre_exec(move || { + block_linux_sandbox_forwarded_signals()?; + if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 { + return Err(std::io::Error::last_os_error()); + } + if libc::getppid() != parent_pid { + libc::raise(libc::SIGTERM); + } + Ok(()) + }); + } + } + cmd.stdin(Stdio::inherit()) .stdout(Stdio::inherit()) .stderr(Stdio::inherit()) @@ -445,6 +480,68 @@ async fn spawn_debug_sandbox_child( .spawn() } +#[cfg(target_os = "linux")] +async fn wait_for_debug_sandbox_child( + child: &mut Child, +) -> std::io::Result { + let child_pid = child.id().map(|pid| pid as libc::pid_t); + tokio::select! { + status = child.wait() => status, + signal = recv_linux_sandbox_forwarded_signal() => { + let signal = signal?; + if let Some(child_pid) = child_pid { + signal_debug_sandbox_child(child_pid, signal)?; + } + child.wait().await + } + } +} + +#[cfg(target_os = "linux")] +async fn recv_linux_sandbox_forwarded_signal() -> std::io::Result { + use tokio::signal::unix::SignalKind; + use tokio::signal::unix::signal; + + let mut sighup = signal(SignalKind::hangup())?; + let mut sigint = signal(SignalKind::interrupt())?; + let mut sigquit = signal(SignalKind::quit())?; + let mut sigterm = signal(SignalKind::terminate())?; + + let signal = tokio::select! { + _ = sighup.recv() => libc::SIGHUP, + _ = sigint.recv() => libc::SIGINT, + _ = sigquit.recv() => libc::SIGQUIT, + _ = sigterm.recv() => libc::SIGTERM, + }; + Ok(signal) +} + +#[cfg(target_os = "linux")] +fn signal_debug_sandbox_child(pid: libc::pid_t, signal: libc::c_int) -> std::io::Result<()> { + if unsafe { libc::kill(pid, signal) } < 0 { + let err = std::io::Error::last_os_error(); + if err.raw_os_error() != Some(libc::ESRCH) { + return Err(err); + } + } + Ok(()) +} + +#[cfg(target_os = "linux")] +fn block_linux_sandbox_forwarded_signals() -> std::io::Result<()> { + let mut blocked: libc::sigset_t = unsafe { std::mem::zeroed() }; + unsafe { + libc::sigemptyset(&mut blocked); + for signal in LINUX_SANDBOX_FORWARDED_SIGNALS { + libc::sigaddset(&mut blocked, *signal); + } + if libc::sigprocmask(libc::SIG_BLOCK, &blocked, std::ptr::null_mut()) < 0 { + return Err(std::io::Error::last_os_error()); + } + } + Ok(()) +} + #[cfg(target_os = "windows")] mod windows_stdio_bridge { use std::io::Read; diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index b7512b707618..c3e101bf17b6 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -531,6 +531,14 @@ impl ShellHandler { prefix_rule, }) .await; + let exec_approval_requirement = codex_shell_command::metadata_write_forbidden_reason( + &exec_params.command, + &exec_params.cwd, + &file_system_sandbox_policy, + ) + .map_or(exec_approval_requirement, |reason| { + crate::tools::sandboxing::ExecApprovalRequirement::Forbidden { reason } + }); let req = ShellRequest { command: exec_params.command.clone(), diff --git a/codex-rs/shell-command/src/bash.rs b/codex-rs/shell-command/src/bash.rs index 2fe299108c20..9a10ba458be9 100644 --- a/codex-rs/shell-command/src/bash.rs +++ b/codex-rs/shell-command/src/bash.rs @@ -119,6 +119,55 @@ pub fn parse_shell_lc_plain_commands(command: &[String]) -> Option Option>> { + let (_, script) = extract_bash_command(command)?; + let tree = try_parse_shell(script)?; + let root = tree.root_node(); + if root.has_error() { + return None; + } + + let mut commands = Vec::new(); + for command_node in find_command_nodes(root) { + if let Some(words) = parse_command_word_prefix_from_node(command_node, script) + && !words.is_empty() + { + commands.push(words); + } + } + + (!commands.is_empty()).then_some(commands) +} + +/// Returns literal write redirection targets within a `bash -lc "..."` or +/// `zsh -lc "..."` invocation. +pub fn parse_shell_lc_write_redirection_targets(command: &[String]) -> Option> { + let (_, script) = extract_bash_command(command)?; + let tree = try_parse_shell(script)?; + let root = tree.root_node(); + if root.has_error() { + return None; + } + + let mut targets = Vec::new(); + for redirect_node in find_nodes_by_kind(root, "file_redirect") { + if file_redirect_uses_write_operator(redirect_node) + && let Some(target) = parse_redirection_target(redirect_node, script) + { + targets.push(target); + } + } + + (!targets.is_empty()).then_some(targets) +} + /// Returns the parsed argv for a single shell command in a here-doc style /// script (`<<`), as long as the script contains exactly one command node. pub fn parse_shell_lc_single_command_prefix(command: &[String]) -> Option> { @@ -194,6 +243,100 @@ fn parse_plain_command_from_node(cmd: tree_sitter::Node, src: &str) -> Option, src: &str) -> Option> { + if cmd.kind() != "command" { + return None; + } + let mut words = Vec::new(); + let mut cursor = cmd.walk(); + for child in cmd.named_children(&mut cursor) { + match child.kind() { + "command_name" => { + let word_node = child.named_child(0)?; + if !matches!(word_node.kind(), "word" | "number") { + return None; + } + words.push(word_node.utf8_text(src.as_bytes()).ok()?.to_owned()); + } + "word" | "number" => { + words.push(child.utf8_text(src.as_bytes()).ok()?.to_owned()); + } + "string" => { + let parsed = parse_double_quoted_string(child, src)?; + words.push(parsed); + } + "raw_string" => { + let parsed = parse_raw_string(child, src)?; + words.push(parsed); + } + "concatenation" => { + let parsed = parse_concatenation(child, src)?; + words.push(parsed); + } + "variable_assignment" | "comment" => {} + kind if is_allowed_heredoc_attachment_kind(kind) => {} + _ => {} + } + } + Some(words) +} + +fn file_redirect_uses_write_operator(node: Node<'_>) -> bool { + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + if !child.is_named() && child.kind().contains('>') { + return true; + } + } + false +} + +fn parse_redirection_target(node: Node<'_>, src: &str) -> Option { + let mut target = None; + let mut cursor = node.walk(); + for child in node.named_children(&mut cursor) { + if let Some(parsed) = parse_literal_shell_word(child, src) { + target = Some(parsed); + } + } + target +} + +fn parse_literal_shell_word(node: Node<'_>, src: &str) -> Option { + match node.kind() { + "word" | "number" => Some(node.utf8_text(src.as_bytes()).ok()?.to_owned()), + "string" => parse_double_quoted_string(node, src), + "raw_string" => parse_raw_string(node, src), + "concatenation" => parse_concatenation(node, src), + _ => None, + } +} + +fn parse_concatenation(node: Node<'_>, src: &str) -> Option { + let mut concatenated = String::new(); + let mut cursor = node.walk(); + for part in node.named_children(&mut cursor) { + match part.kind() { + "word" | "number" => { + concatenated.push_str(part.utf8_text(src.as_bytes()).ok()?.to_owned().as_str()); + } + "string" => { + let parsed = parse_double_quoted_string(part, src)?; + concatenated.push_str(&parsed); + } + "raw_string" => { + let parsed = parse_raw_string(part, src)?; + concatenated.push_str(&parsed); + } + _ => return None, + } + } + if concatenated.is_empty() { + return None; + } + Some(concatenated) +} + fn parse_heredoc_command_words(cmd: Node<'_>, src: &str) -> Option> { if cmd.kind() != "command" { return None; @@ -268,6 +411,29 @@ fn find_single_command_node(root: Node<'_>) -> Option> { single_command } +fn find_command_nodes(root: Node<'_>) -> Vec> { + let mut command_nodes = find_nodes_by_kind(root, "command"); + command_nodes.sort_by_key(Node::start_byte); + command_nodes +} + +fn find_nodes_by_kind<'a>(root: Node<'a>, kind: &str) -> Vec> { + let mut stack = vec![root]; + let mut matches = Vec::new(); + while let Some(node) = stack.pop() { + if node.kind() == kind { + matches.push(node); + } + + let mut cursor = node.walk(); + for child in node.named_children(&mut cursor) { + stack.push(child); + } + } + matches.sort_by_key(Node::start_byte); + matches +} + fn has_named_descendant_kind(node: Node<'_>, kind: &str) -> bool { let mut stack = vec![node]; while let Some(current) = stack.pop() { @@ -453,6 +619,64 @@ mod tests { assert_eq!(parsed, vec![vec!["ls".to_string()]]); } + #[test] + fn parse_shell_lc_command_word_prefixes_extracts_complex_script_commands() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + r#"set -e +top=$(git rev-parse --show-toplevel) +if git init -q; then + exit 22 +fi +if mkdir .codex; then + exit 23 +fi +printf pwned > .git/config +"# + .to_string(), + ]; + + let parsed = parse_shell_lc_command_word_prefixes(&command).expect("parse script"); + + assert!(parsed.contains(&vec![ + "git".to_string(), + "rev-parse".to_string(), + "--show-toplevel".to_string() + ])); + assert!(parsed.contains(&vec![ + "git".to_string(), + "init".to_string(), + "-q".to_string() + ])); + assert!(parsed.contains(&vec!["mkdir".to_string(), ".codex".to_string()])); + } + + #[test] + fn parse_shell_lc_write_redirection_targets_extracts_literal_writes() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + r#"printf pwned > .git +cat < .git/config +printf ok >> .codex/log +printf ok > ".agents/config" +"# + .to_string(), + ]; + + let parsed = parse_shell_lc_write_redirection_targets(&command).expect("parse redirects"); + + assert_eq!( + parsed, + vec![ + ".git".to_string(), + ".codex/log".to_string(), + ".agents/config".to_string() + ] + ); + } + #[test] fn accepts_concatenated_flag_and_value() { // Test case: -g"*.py" (flag directly concatenated with quoted value) diff --git a/codex-rs/shell-command/src/lib.rs b/codex-rs/shell-command/src/lib.rs index 1d9e302a4e70..7311f22c2814 100644 --- a/codex-rs/shell-command/src/lib.rs +++ b/codex-rs/shell-command/src/lib.rs @@ -4,8 +4,10 @@ mod shell_detect; pub mod bash; pub(crate) mod command_safety; +mod metadata_write; pub mod parse_command; pub mod powershell; pub use command_safety::is_dangerous_command; pub use command_safety::is_safe_command; +pub use metadata_write::metadata_write_forbidden_reason; diff --git a/codex-rs/shell-command/src/metadata_write.rs b/codex-rs/shell-command/src/metadata_write.rs new file mode 100644 index 000000000000..ee15786feb1e --- /dev/null +++ b/codex-rs/shell-command/src/metadata_write.rs @@ -0,0 +1,136 @@ +use std::path::Path; + +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::forbidden_agent_metadata_write; + +pub fn metadata_write_forbidden_reason( + command: &[String], + cwd: &Path, + file_system_sandbox_policy: &FileSystemSandboxPolicy, +) -> Option { + if let Some(targets) = crate::bash::parse_shell_lc_write_redirection_targets(command) { + for target in targets { + if let Some(name) = + forbidden_agent_metadata_write(Path::new(&target), cwd, file_system_sandbox_policy) + { + return Some(metadata_write_reason(name)); + } + } + } + None +} + +fn metadata_write_reason(name: &str) -> String { + format!("command targets protected workspace metadata path `{name}`") +} + +#[cfg(test)] +mod tests { + use std::path::Path; + use std::path::PathBuf; + + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_protocol::protocol::SandboxPolicy; + use pretty_assertions::assert_eq; + + use super::metadata_write_forbidden_reason; + + struct TestDir { + path: PathBuf, + } + + impl TestDir { + fn new(name: &str) -> Self { + let path = std::env::temp_dir().join(format!( + "codex-metadata-write-{name}-{}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&path); + std::fs::create_dir(&path).expect("create tempdir"); + Self { path } + } + + fn path(&self) -> &Path { + &self.path + } + } + + impl Drop for TestDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } + } + + fn legacy_workspace_write_policy(cwd: &Path) -> FileSystemSandboxPolicy { + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, cwd) + } + + #[test] + fn metadata_write_detector_allows_normal_git_under_parent_repo() { + let repo = TestDir::new("normal-git-under-parent-repo"); + std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); + let cwd = repo.path().join("sub"); + std::fs::create_dir(&cwd).expect("create cwd"); + let policy = legacy_workspace_write_policy(&cwd); + + let reason = metadata_write_forbidden_reason( + &[ + "/bin/bash".to_string(), + "-lc".to_string(), + "git status --short".to_string(), + ], + &cwd, + &policy, + ); + + assert_eq!(reason, None); + } + + #[test] + fn metadata_write_detector_leaves_direct_writes_to_sandbox_policy() { + let cwd = TestDir::new("direct-metadata-writes"); + let policy = legacy_workspace_write_policy(cwd.path()); + + let reason = metadata_write_forbidden_reason( + &[ + "/bin/bash".to_string(), + "-lc".to_string(), + "touch .git && mkdir -p .codex".to_string(), + ], + cwd.path(), + &policy, + ); + + assert_eq!(reason, None); + } + + #[test] + fn metadata_write_detector_blocks_metadata_redirections() { + let repo = TestDir::new("metadata-write-redirections"); + std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); + let cwd = repo.path().join("sub"); + std::fs::create_dir(&cwd).expect("create cwd"); + let policy = legacy_workspace_write_policy(&cwd); + + let reason = metadata_write_forbidden_reason( + &[ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf pwned > .git".to_string(), + ], + &cwd, + &policy, + ); + + assert_eq!( + reason, + Some("command targets protected workspace metadata path `.git`".to_string()) + ); + } +} From 321204f3d0f9339a61a5922c48132067456400b6 Mon Sep 17 00:00:00 2001 From: Eva Wong Date: Tue, 28 Apr 2026 13:28:00 -0700 Subject: [PATCH 2/2] Apply metadata preflight consistently --- codex-rs/cli/src/debug_sandbox.rs | 91 +-------------- codex-rs/core/src/tools/handlers/shell.rs | 18 ++- .../core/src/tools/handlers/unified_exec.rs | 40 ++++++- codex-rs/core/src/tools/sandboxing.rs | 45 ++++++++ codex-rs/core/src/tools/sandboxing_tests.rs | 105 ++++++++++++++++++ codex-rs/core/src/unified_exec/mod.rs | 4 +- .../core/src/unified_exec/process_manager.rs | 22 +--- codex-rs/protocol/src/permissions.rs | 12 +- codex-rs/shell-command/src/metadata_write.rs | 88 ++++++++++++--- 9 files changed, 285 insertions(+), 140 deletions(-) diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index 73ca85502c63..838cdcfcece8 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -35,10 +35,6 @@ use crate::exit_status::handle_exit_status; #[cfg(target_os = "macos")] use seatbelt::DenialLogger; -#[cfg(target_os = "linux")] -const LINUX_SANDBOX_FORWARDED_SIGNALS: &[libc::c_int] = - &[libc::SIGHUP, libc::SIGINT, libc::SIGQUIT, libc::SIGTERM]; - #[cfg(target_os = "macos")] pub async fn run_command_under_seatbelt( command: SeatbeltCommand, @@ -149,6 +145,7 @@ async fn run_command_under_sandbox( if let Some(reason) = codex_shell_command::metadata_write_forbidden_reason( &command, cwd.as_path(), + sandbox_policy_cwd.as_path(), &config.permissions.file_system_sandbox_policy(), ) { anyhow::bail!("{reason}"); @@ -272,9 +269,6 @@ async fn run_command_under_sandbox( denial_logger.on_child_spawn(&child); } - #[cfg(target_os = "linux")] - let status = wait_for_debug_sandbox_child(&mut child).await?; - #[cfg(not(target_os = "linux"))] let status = child.wait().await?; #[cfg(target_os = "macos")] @@ -452,27 +446,6 @@ async fn spawn_debug_sandbox_child( cmd.env(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR, "1"); } - #[cfg(target_os = "linux")] - { - let parent_pid = unsafe { libc::getpid() }; - // SAFETY: `pre_exec` runs in the child immediately before exec. The - // closure only adjusts the child signal mask, installs a parent-death - // signal, and checks the inherited parent pid to close the fork/exec - // race. - unsafe { - cmd.pre_exec(move || { - block_linux_sandbox_forwarded_signals()?; - if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 { - return Err(std::io::Error::last_os_error()); - } - if libc::getppid() != parent_pid { - libc::raise(libc::SIGTERM); - } - Ok(()) - }); - } - } - cmd.stdin(Stdio::inherit()) .stdout(Stdio::inherit()) .stderr(Stdio::inherit()) @@ -480,68 +453,6 @@ async fn spawn_debug_sandbox_child( .spawn() } -#[cfg(target_os = "linux")] -async fn wait_for_debug_sandbox_child( - child: &mut Child, -) -> std::io::Result { - let child_pid = child.id().map(|pid| pid as libc::pid_t); - tokio::select! { - status = child.wait() => status, - signal = recv_linux_sandbox_forwarded_signal() => { - let signal = signal?; - if let Some(child_pid) = child_pid { - signal_debug_sandbox_child(child_pid, signal)?; - } - child.wait().await - } - } -} - -#[cfg(target_os = "linux")] -async fn recv_linux_sandbox_forwarded_signal() -> std::io::Result { - use tokio::signal::unix::SignalKind; - use tokio::signal::unix::signal; - - let mut sighup = signal(SignalKind::hangup())?; - let mut sigint = signal(SignalKind::interrupt())?; - let mut sigquit = signal(SignalKind::quit())?; - let mut sigterm = signal(SignalKind::terminate())?; - - let signal = tokio::select! { - _ = sighup.recv() => libc::SIGHUP, - _ = sigint.recv() => libc::SIGINT, - _ = sigquit.recv() => libc::SIGQUIT, - _ = sigterm.recv() => libc::SIGTERM, - }; - Ok(signal) -} - -#[cfg(target_os = "linux")] -fn signal_debug_sandbox_child(pid: libc::pid_t, signal: libc::c_int) -> std::io::Result<()> { - if unsafe { libc::kill(pid, signal) } < 0 { - let err = std::io::Error::last_os_error(); - if err.raw_os_error() != Some(libc::ESRCH) { - return Err(err); - } - } - Ok(()) -} - -#[cfg(target_os = "linux")] -fn block_linux_sandbox_forwarded_signals() -> std::io::Result<()> { - let mut blocked: libc::sigset_t = unsafe { std::mem::zeroed() }; - unsafe { - libc::sigemptyset(&mut blocked); - for signal in LINUX_SANDBOX_FORWARDED_SIGNALS { - libc::sigaddset(&mut blocked, *signal); - } - if libc::sigprocmask(libc::SIG_BLOCK, &blocked, std::ptr::null_mut()) < 0 { - return Err(std::io::Error::last_os_error()); - } - } - Ok(()) -} - #[cfg(target_os = "windows")] mod windows_stdio_bridge { use std::io::Read; diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index c3e101bf17b6..22e81b4d13dc 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -35,9 +35,11 @@ use crate::tools::runtimes::shell::ShellRequest; use crate::tools::runtimes::shell::ShellRuntime; use crate::tools::runtimes::shell::ShellRuntimeBackend; use crate::tools::sandboxing::ToolCtx; +use crate::tools::sandboxing::apply_protected_metadata_write_preflight; use codex_features::Feature; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::ExecCommandSource; +use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ShellCommandBackendConfig; @@ -513,7 +515,11 @@ impl ShellHandler { ); emitter.begin(event_ctx).await; - let file_system_sandbox_policy = turn.file_system_sandbox_policy(); + let base_file_system_sandbox_policy = turn.file_system_sandbox_policy(); + let file_system_sandbox_policy = effective_file_system_sandbox_policy( + &base_file_system_sandbox_policy, + normalized_additional_permissions.as_ref(), + ); let exec_approval_requirement = session .services .exec_policy @@ -531,14 +537,14 @@ impl ShellHandler { prefix_rule, }) .await; - let exec_approval_requirement = codex_shell_command::metadata_write_forbidden_reason( + let exec_approval_requirement = apply_protected_metadata_write_preflight( + exec_approval_requirement, + effective_additional_permissions.sandbox_permissions, &exec_params.command, &exec_params.cwd, + turn.cwd.as_path(), &file_system_sandbox_policy, - ) - .map_or(exec_approval_requirement, |reason| { - crate::tools::sandboxing::ExecApprovalRequirement::Forbidden { reason } - }); + ); let req = ShellRequest { command: exec_params.command.clone(), diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 10c8deeb3f6a..1119a2e62ccd 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -1,3 +1,4 @@ +use crate::exec_policy::ExecApprovalRequest; use crate::function_tool::FunctionCallError; use crate::maybe_emit_implicit_skill_invocation; use crate::sandboxing::SandboxPermissions; @@ -19,6 +20,7 @@ use crate::tools::registry::PostToolUsePayload; use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; +use crate::tools::sandboxing::apply_protected_metadata_write_preflight; use crate::unified_exec::ExecCommandRequest; use crate::unified_exec::UnifiedExecContext; use crate::unified_exec::UnifiedExecError; @@ -32,6 +34,7 @@ use codex_otel::TOOL_CALL_UNIFIED_EXEC_METRIC; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::TerminalInteractionEvent; +use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::UnifiedExecShellMode; use codex_utils_output_truncation::TruncationPolicy; @@ -330,6 +333,40 @@ impl ToolHandler for UnifiedExecHandler { }); } + let base_file_system_sandbox_policy = context.turn.file_system_sandbox_policy(); + let file_system_sandbox_policy = effective_file_system_sandbox_policy( + &base_file_system_sandbox_policy, + normalized_additional_permissions.as_ref(), + ); + let exec_approval_requirement = context + .session + .services + .exec_policy + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: context.turn.approval_policy.value(), + permission_profile: context.turn.permission_profile(), + file_system_sandbox_policy: &file_system_sandbox_policy, + sandbox_cwd: context.turn.cwd.as_path(), + sandbox_permissions: if effective_additional_permissions + .permissions_preapproved + { + SandboxPermissions::UseDefault + } else { + effective_additional_permissions.sandbox_permissions + }, + prefix_rule: prefix_rule.clone(), + }) + .await; + let exec_approval_requirement = apply_protected_metadata_write_preflight( + exec_approval_requirement, + effective_additional_permissions.sandbox_permissions, + &command, + &cwd, + context.turn.cwd.as_path(), + &file_system_sandbox_policy, + ); + emit_unified_exec_tty_metric(&turn.session_telemetry, tty); match manager .exec_command( @@ -345,10 +382,11 @@ impl ToolHandler for UnifiedExecHandler { sandbox_permissions: effective_additional_permissions .sandbox_permissions, additional_permissions: normalized_additional_permissions, + #[cfg(unix)] additional_permissions_preapproved: effective_additional_permissions .permissions_preapproved, justification, - prefix_rule, + exec_approval_requirement, }, &context, ) diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index f6b960ee1ac6..8846df4df95c 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -34,6 +34,7 @@ use serde::Serialize; use std::collections::HashMap; use std::fmt::Debug; use std::hash::Hash; +use std::path::Path; use std::sync::Arc; #[derive(Clone, Default, Debug)] @@ -194,6 +195,50 @@ impl ExecApprovalRequirement { } } +/// Applies an early UX check for protected metadata writes only to commands +/// that would otherwise run in the sandbox. Explicit sandbox-bypass approval +/// paths keep their existing approval semantics. +pub(crate) fn apply_protected_metadata_write_preflight( + exec_approval_requirement: ExecApprovalRequirement, + sandbox_permissions: SandboxPermissions, + command: &[String], + command_cwd: &Path, + sandbox_policy_cwd: &Path, + file_system_sandbox_policy: &FileSystemSandboxPolicy, +) -> ExecApprovalRequirement { + let bypasses_sandbox = matches!( + &exec_approval_requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + .. + } + ); + let approval_is_for_sandbox_override = sandbox_permissions.requests_sandbox_override() + && matches!( + &exec_approval_requirement, + ExecApprovalRequirement::NeedsApproval { .. } + ); + if bypasses_sandbox + || approval_is_for_sandbox_override + || matches!( + &exec_approval_requirement, + ExecApprovalRequirement::Forbidden { .. } + ) + { + return exec_approval_requirement; + } + + codex_shell_command::metadata_write_forbidden_reason( + command, + command_cwd, + sandbox_policy_cwd, + file_system_sandbox_policy, + ) + .map_or(exec_approval_requirement, |reason| { + ExecApprovalRequirement::Forbidden { reason } + }) +} + /// - Never, OnFailure: do not ask /// - OnRequest: ask unless filesystem access is unrestricted /// - Granular: ask unless filesystem access is unrestricted, but auto-reject diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 19eb7a67d967..9fc3f7cbcbdc 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -5,6 +5,7 @@ use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::NetworkAccess; use pretty_assertions::assert_eq; use serde_json::json; +use tempfile::tempdir; #[test] fn bash_permission_request_payload_omits_missing_description() { @@ -138,3 +139,107 @@ fn guardian_bypasses_sandbox_for_explicit_escalation_on_first_attempt() { SandboxOverride::BypassSandboxFirstAttempt ); } + +fn metadata_redirect_command(target: &str) -> Vec { + vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + format!("printf pwned > {target}"), + ] +} + +fn workspace_write_file_system_sandbox_policy() -> FileSystemSandboxPolicy { + FileSystemSandboxPolicy::workspace_write(&[], false, false) +} + +#[test] +fn protected_metadata_write_preflight_forbids_only_sandboxed_skip() { + let repo = tempdir().expect("create tempdir"); + std::fs::create_dir(repo.path().join(".git")).expect("create .git"); + let file_system_sandbox_policy = workspace_write_file_system_sandbox_policy(); + + let requirement = apply_protected_metadata_write_preflight( + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + SandboxPermissions::UseDefault, + &metadata_redirect_command(".git/config"), + repo.path(), + repo.path(), + &file_system_sandbox_policy, + ); + + assert_eq!( + requirement, + ExecApprovalRequirement::Forbidden { + reason: "command targets protected workspace metadata path `.git`".to_string() + } + ); +} + +#[test] +fn protected_metadata_write_preflight_preserves_approval_and_bypass_paths() { + let repo = tempdir().expect("create tempdir"); + std::fs::create_dir(repo.path().join(".git")).expect("create .git"); + let file_system_sandbox_policy = workspace_write_file_system_sandbox_policy(); + let command = metadata_redirect_command(".git/config"); + + let needs_approval = ExecApprovalRequirement::NeedsApproval { + reason: Some("requires approval".to_string()), + proposed_execpolicy_amendment: None, + }; + assert_eq!( + apply_protected_metadata_write_preflight( + needs_approval.clone(), + SandboxPermissions::RequireEscalated, + &command, + repo.path(), + repo.path(), + &file_system_sandbox_policy, + ), + needs_approval + ); + + let bypass_sandbox = ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }; + assert_eq!( + apply_protected_metadata_write_preflight( + bypass_sandbox.clone(), + SandboxPermissions::UseDefault, + &command, + repo.path(), + repo.path(), + &file_system_sandbox_policy, + ), + bypass_sandbox + ); +} + +#[test] +fn protected_metadata_write_preflight_forbids_sandboxed_approval_prompts() { + let repo = tempdir().expect("create tempdir"); + std::fs::create_dir(repo.path().join(".git")).expect("create .git"); + let file_system_sandbox_policy = workspace_write_file_system_sandbox_policy(); + + let requirement = apply_protected_metadata_write_preflight( + ExecApprovalRequirement::NeedsApproval { + reason: Some("dangerous command".to_string()), + proposed_execpolicy_amendment: None, + }, + SandboxPermissions::UseDefault, + &metadata_redirect_command(".git/config"), + repo.path(), + repo.path(), + &file_system_sandbox_policy, + ); + + assert_eq!( + requirement, + ExecApprovalRequirement::Forbidden { + reason: "command targets protected workspace metadata path `.git`".to_string() + } + ); +} diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index acf361f94b8c..9d523ba8a40e 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -37,6 +37,7 @@ use tokio::sync::Mutex; use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::tools::sandboxing::ExecApprovalRequirement; mod async_watcher; mod errors; @@ -97,9 +98,10 @@ pub(crate) struct ExecCommandRequest { pub tty: bool, pub sandbox_permissions: SandboxPermissions, pub additional_permissions: Option, + #[cfg(unix)] pub additional_permissions_preapproved: bool, pub justification: Option, - pub prefix_rule: Option>, + pub exec_approval_requirement: ExecApprovalRequirement, } #[derive(Debug)] diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 76d8021d3d93..8ca4c99bc3f8 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -13,7 +13,6 @@ use tokio_util::sync::CancellationToken; use crate::exec_env::CODEX_THREAD_ID_ENV_VAR; use crate::exec_env::create_env; -use crate::exec_policy::ExecApprovalRequest; use crate::sandboxing::ExecRequest; use crate::sandboxing::ExecServerEnvConfig; use crate::tools::context::ExecCommandToolOutput; @@ -789,25 +788,6 @@ impl UnifiedExecProcessManager { self, context.turn.tools_config.unified_exec_shell_mode.clone(), ); - let file_system_sandbox_policy = context.turn.file_system_sandbox_policy(); - let exec_approval_requirement = context - .session - .services - .exec_policy - .create_exec_approval_requirement_for_command(ExecApprovalRequest { - command: &request.command, - approval_policy: context.turn.approval_policy.value(), - permission_profile: context.turn.permission_profile(), - file_system_sandbox_policy: &file_system_sandbox_policy, - sandbox_cwd: context.turn.cwd.as_path(), - sandbox_permissions: if request.additional_permissions_preapproved { - crate::sandboxing::SandboxPermissions::UseDefault - } else { - request.sandbox_permissions - }, - prefix_rule: request.prefix_rule.clone(), - }) - .await; let req = UnifiedExecToolRequest { command: request.command.clone(), hook_command: request.hook_command.clone(), @@ -823,7 +803,7 @@ impl UnifiedExecProcessManager { #[cfg(unix)] additional_permissions_preapproved: request.additional_permissions_preapproved, justification: request.justification.clone(), - exec_approval_requirement, + exec_approval_requirement: request.exec_approval_requirement.clone(), }; let tool_ctx = ToolCtx { session: context.session.clone(), diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index c1f3d0724b8a..c7f0db256c15 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -46,7 +46,8 @@ pub fn is_protected_metadata_directory_name(name: &OsStr) -> bool { /// should be blocked before execution. pub fn forbidden_agent_metadata_write( path: &Path, - cwd: &Path, + path_cwd: &Path, + policy_cwd: &Path, file_system_sandbox_policy: &FileSystemSandboxPolicy, ) -> Option<&'static str> { if !matches!( @@ -56,19 +57,19 @@ pub fn forbidden_agent_metadata_write( return None; } - let target = resolve_candidate_path(path, cwd)?; + let target = resolve_candidate_path(path, path_cwd)?; let (protected_metadata_path, metadata_name) = - metadata_child_of_writable_root(file_system_sandbox_policy, target.as_path(), cwd)?; + metadata_child_of_writable_root(file_system_sandbox_policy, target.as_path(), policy_cwd)?; if has_explicit_write_entry_for_metadata_path( file_system_sandbox_policy, &protected_metadata_path, target.as_path(), - cwd, + policy_cwd, ) { return None; } - if !file_system_sandbox_policy.can_write_path_with_cwd(target.as_path(), cwd) { + if !file_system_sandbox_policy.can_write_path_with_cwd(target.as_path(), policy_cwd) { return Some(metadata_name); } @@ -2007,6 +2008,7 @@ mod tests { forbidden_agent_metadata_write( Path::new(".git/config"), relative_cwd, + relative_cwd, &file_system_policy, ), Some(".git") diff --git a/codex-rs/shell-command/src/metadata_write.rs b/codex-rs/shell-command/src/metadata_write.rs index ee15786feb1e..8b819de0eb02 100644 --- a/codex-rs/shell-command/src/metadata_write.rs +++ b/codex-rs/shell-command/src/metadata_write.rs @@ -5,14 +5,18 @@ use codex_protocol::permissions::forbidden_agent_metadata_write; pub fn metadata_write_forbidden_reason( command: &[String], - cwd: &Path, + command_cwd: &Path, + sandbox_policy_cwd: &Path, file_system_sandbox_policy: &FileSystemSandboxPolicy, ) -> Option { if let Some(targets) = crate::bash::parse_shell_lc_write_redirection_targets(command) { for target in targets { - if let Some(name) = - forbidden_agent_metadata_write(Path::new(&target), cwd, file_system_sandbox_policy) - { + if let Some(name) = forbidden_agent_metadata_write( + Path::new(&target), + command_cwd, + sandbox_policy_cwd, + file_system_sandbox_policy, + ) { return Some(metadata_write_reason(name)); } } @@ -29,8 +33,10 @@ mod tests { use std::path::Path; use std::path::PathBuf; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; - use codex_protocol::protocol::SandboxPolicy; use pretty_assertions::assert_eq; use super::metadata_write_forbidden_reason; @@ -61,14 +67,8 @@ mod tests { } } - fn legacy_workspace_write_policy(cwd: &Path) -> FileSystemSandboxPolicy { - let policy = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![], - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }; - FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, cwd) + fn workspace_write_policy() -> FileSystemSandboxPolicy { + FileSystemSandboxPolicy::workspace_write(&[], false, false) } #[test] @@ -77,7 +77,7 @@ mod tests { std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); let cwd = repo.path().join("sub"); std::fs::create_dir(&cwd).expect("create cwd"); - let policy = legacy_workspace_write_policy(&cwd); + let policy = workspace_write_policy(); let reason = metadata_write_forbidden_reason( &[ @@ -86,6 +86,7 @@ mod tests { "git status --short".to_string(), ], &cwd, + &cwd, &policy, ); @@ -95,7 +96,7 @@ mod tests { #[test] fn metadata_write_detector_leaves_direct_writes_to_sandbox_policy() { let cwd = TestDir::new("direct-metadata-writes"); - let policy = legacy_workspace_write_policy(cwd.path()); + let policy = workspace_write_policy(); let reason = metadata_write_forbidden_reason( &[ @@ -104,6 +105,7 @@ mod tests { "touch .git && mkdir -p .codex".to_string(), ], cwd.path(), + cwd.path(), &policy, ); @@ -116,7 +118,7 @@ mod tests { std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); let cwd = repo.path().join("sub"); std::fs::create_dir(&cwd).expect("create cwd"); - let policy = legacy_workspace_write_policy(&cwd); + let policy = workspace_write_policy(); let reason = metadata_write_forbidden_reason( &[ @@ -125,6 +127,7 @@ mod tests { "printf pwned > .git".to_string(), ], &cwd, + &cwd, &policy, ); @@ -133,4 +136,57 @@ mod tests { Some("command targets protected workspace metadata path `.git`".to_string()) ); } + + #[test] + fn metadata_write_detector_resolves_targets_against_command_cwd() { + let repo = TestDir::new("metadata-write-command-cwd"); + let command_cwd = repo.path().join("sub"); + std::fs::create_dir(&command_cwd).expect("create command cwd"); + let policy = workspace_write_policy(); + + let reason = metadata_write_forbidden_reason( + &[ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf pwned > ../.codex/config.toml".to_string(), + ], + &command_cwd, + repo.path(), + &policy, + ); + + assert_eq!( + reason, + Some("command targets protected workspace metadata path `.codex`".to_string()) + ); + } + + #[test] + fn metadata_write_detector_honors_explicit_metadata_write_entry() { + let repo = TestDir::new("metadata-write-explicit-entry"); + let mut policy = workspace_write_policy(); + policy.entries.push(FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: repo + .path() + .join(".codex") + .try_into() + .expect("absolute path"), + }, + access: FileSystemAccessMode::Write, + }); + + let reason = metadata_write_forbidden_reason( + &[ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf pwned > .codex/config.toml".to_string(), + ], + repo.path(), + repo.path(), + &policy, + ); + + assert_eq!(reason, None); + } }