[codex] allowlist safe find arguments

codex-rs/core/src/exec_policy.rs

@@ -250,7 +250,16 @@ impl ExecPolicyManager {
         // Keep heredoc prefix parsing for rule evaluation so existing
         // allow/prompt/forbidden rules still apply, but avoid auto-derived
         // amendments when only the heredoc fallback parser matched.
-        let auto_amendment_allowed = !used_complex_parsing;
+        let sandbox_writable_executable_requires_review = commands.iter().any(|cmd| {
+            sandbox_writable_executable_path_requires_review(
+                cmd,
+                file_system_sandbox_policy,
+                sandbox_cwd,
+                sandbox_permissions,
+            )
+        });
+        let auto_amendment_allowed =
+            !used_complex_parsing && !sandbox_writable_executable_requires_review;
         let exec_policy_fallback = |cmd: &[String]| {
             render_decision_for_unmatched_command(
                 approval_policy,
@@ -271,14 +280,18 @@ impl ExecPolicyManager {
             &match_options,
         );
 
-        let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule(
-            prefix_rule.as_ref(),
-            &evaluation.matched_rules,
-            exec_policy.as_ref(),
-            &commands,
-            &exec_policy_fallback,
-            &match_options,
-        );
+        let requested_amendment = if auto_amendment_allowed {
+            derive_requested_execpolicy_amendment_from_prefix_rule(
+                prefix_rule.as_ref(),
+                &evaluation.matched_rules,
+                exec_policy.as_ref(),
+                &commands,
+                &exec_policy_fallback,
+                &match_options,
+            )
+        } else {
+            None
+        };
 
         match evaluation.decision {
             Decision::Forbidden => ExecApprovalRequirement::Forbidden {
@@ -590,6 +603,15 @@ pub fn render_decision_for_unmatched_command(
     sandbox_permissions: SandboxPermissions,
     used_complex_parsing: bool,
 ) -> Decision {
+    if sandbox_writable_executable_path_requires_review(
+        command,
+        file_system_sandbox_policy,
+        sandbox_cwd,
+        sandbox_permissions,
+    ) {
+        return Decision::Prompt;
+    }
+
     if is_known_safe_command(command) && !used_complex_parsing {
         return Decision::Allow;
     }
@@ -678,6 +700,33 @@ pub fn render_decision_for_unmatched_command(
     }
 }
 
+fn sandbox_writable_executable_path_requires_review(
+    command: &[String],
+    file_system_sandbox_policy: &FileSystemSandboxPolicy,
+    sandbox_cwd: &Path,
+    sandbox_permissions: SandboxPermissions,
+) -> bool {
+    if !sandbox_permissions.requests_sandbox_override()
+        || !matches!(
+            file_system_sandbox_policy.kind,
+            FileSystemSandboxKind::Restricted
+        )
+        || file_system_sandbox_policy.has_full_disk_write_access()
+    {
+        return false;
+    }
+
+    let Some(program) = command.first() else {
+        return false;
+    };
+    let program_path = Path::new(program);
+    if !program_path.is_absolute() && program_path.components().count() <= 1 {
+        return false;
+    }
+
+    file_system_sandbox_policy.can_write_path_with_cwd(program_path, sandbox_cwd)
+}
+
 fn profile_is_managed_read_only(
     permission_profile: &PermissionProfile,
     file_system_sandbox_policy: &FileSystemSandboxPolicy,

codex-rs/core/src/exec_policy_tests.rs

@@ -997,6 +997,34 @@ fn unmatched_on_request_uses_split_filesystem_policy_for_escalation_prompts() {
     );
 }
 
+#[tokio::test]
+async fn sandbox_writable_safe_command_path_requires_approval_for_escalation() {
+    let command = vec!["/tmp/cat".to_string(), "/dev/null".to_string()];
+    let manager = ExecPolicyManager::default();
+
+    let requirement = manager
+        .create_exec_approval_requirement_for_command(ExecApprovalRequest {
+            command: &command,
+            approval_policy: AskForApproval::OnRequest,
+            permission_profile: permission_profile_from_sandbox_policy(
+                &SandboxPolicy::new_workspace_write_policy(),
+            ),
+            file_system_sandbox_policy: &workspace_write_file_system_sandbox_policy(),
+            sandbox_cwd: Path::new("/tmp"),
+            sandbox_permissions: SandboxPermissions::RequireEscalated,
+            prefix_rule: None,
+        })
+        .await;
+
+    assert_eq!(
+        requirement,
+        ExecApprovalRequirement::NeedsApproval {
+            reason: None,
+            proposed_execpolicy_amendment: None,
+        }
+    );
+}
+
 #[test]
 fn managed_cwd_write_profile_is_not_read_only() {
     let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![

codex-rs/shell-command/src/bash.rs

@@ -149,10 +149,10 @@ fn parse_plain_command_from_node(cmd: tree_sitter::Node, src: &str) -> Option<Ve
                 if word_node.kind() != "word" {
                     return None;
                 }
-                words.push(word_node.utf8_text(src.as_bytes()).ok()?.to_owned());
+                words.push(decode_unquoted_shell_word(word_node, src)?);
             }
             "word" | "number" => {
-                words.push(child.utf8_text(src.as_bytes()).ok()?.to_owned());
+                words.push(decode_unquoted_shell_word(child, src)?);
             }
             "string" => {
                 let parsed = parse_double_quoted_string(child, src)?;
@@ -169,8 +169,7 @@ fn parse_plain_command_from_node(cmd: tree_sitter::Node, src: &str) -> Option<Ve
                 for part in child.named_children(&mut concat_cursor) {
                     match part.kind() {
                         "word" | "number" => {
-                            concatenated
-                                .push_str(part.utf8_text(src.as_bytes()).ok()?.to_owned().as_str());
+                            concatenated.push_str(&decode_unquoted_shell_word(part, src)?);
                         }
                         "string" => {
                             let parsed = parse_double_quoted_string(part, src)?;
@@ -194,6 +193,23 @@ fn parse_plain_command_from_node(cmd: tree_sitter::Node, src: &str) -> Option<Ve
     Some(words)
 }
 
+fn decode_unquoted_shell_word(node: Node, src: &str) -> Option<String> {
+    let raw = node.utf8_text(src.as_bytes()).ok()?;
+    let mut decoded = String::with_capacity(raw.len());
+    let mut chars = raw.chars();
+    while let Some(ch) = chars.next() {
+        if ch == '\\' {
+            let next = chars.next()?;
+            if next != '\n' {
+                decoded.push(next);
+            }
+        } else {
+            decoded.push(ch);
+        }
+    }
+    Some(decoded)
+}
+
 fn parse_heredoc_command_words(cmd: Node<'_>, src: &str) -> Option<Vec<String>> {
     if cmd.kind() != "command" {
         return None;
@@ -482,6 +498,26 @@ mod tests {
         );
     }
 
+    #[test]
+    fn decodes_backslash_escapes_in_unquoted_words() {
+        let cmds = parse_seq(r#"find /app -maxdepth 0 -e\xec sh -c 'echo ok' sh \;"#).unwrap();
+        assert_eq!(
+            cmds,
+            vec![vec![
+                "find".to_string(),
+                "/app".to_string(),
+                "-maxdepth".to_string(),
+                "0".to_string(),
+                "-exec".to_string(),
+                "sh".to_string(),
+                "-c".to_string(),
+                "echo ok".to_string(),
+                "sh".to_string(),
+                ";".to_string(),
+            ]]
+        );
+    }
+
     #[test]
     fn rejects_concatenation_with_variable_substitution() {
         // Environment variables in concatenated strings should be rejected

codex-rs/shell-command/src/command_safety/is_safe_command.rs

@@ -91,24 +91,7 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
             })
         }
 
-        Some("find") => {
-            // Certain options to `find` can delete files, write to files, or
-            // execute arbitrary commands, so we cannot auto-approve the
-            // invocation of `find` in such cases.
-            #[rustfmt::skip]
-            const UNSAFE_FIND_OPTIONS: &[&str] = &[
-                // Options that can execute arbitrary commands.
-                "-exec", "-execdir", "-ok", "-okdir",
-                // Option that deletes matching files.
-                "-delete",
-                // Options that write pathnames to a file.
-                "-fls", "-fprint", "-fprint0", "-fprintf",
-            ];
-
-            !command
-                .iter()
-                .any(|arg| UNSAFE_FIND_OPTIONS.contains(&arg.as_str()))
-        }
+        Some("find") => find_args_are_read_only(command),
 
         // Ripgrep
         Some("rg") => {
@@ -217,6 +200,110 @@ fn git_has_unsafe_global_option(command: &[String]) -> bool {
         .any(git_global_option_requires_prompt)
 }
 
+fn find_args_are_read_only(command: &[String]) -> bool {
+    const FIND_OPTIONS_BEFORE_PATHS: &[&str] = &["-H", "-L", "-P"];
+    const FIND_READ_ONLY_OPERATORS: &[&str] = &["!", "(", ")", "-a", "-and", "-o", "-or", "-not"];
+    const FIND_READ_ONLY_TESTS_WITH_ARGS: &[&str] = &[
+        "-amin",
+        "-anewer",
+        "-atime",
+        "-cmin",
+        "-cnewer",
+        "-ctime",
+        "-fstype",
+        "-gid",
+        "-group",
+        "-ilname",
+        "-iname",
+        "-inum",
+        "-ipath",
+        "-iregex",
+        "-iwholename",
+        "-links",
+        "-lname",
+        "-mmin",
+        "-mtime",
+        "-name",
+        "-newer",
+        "-path",
+        "-perm",
+        "-regex",
+        "-samefile",
+        "-size",
+        "-type",
+        "-uid",
+        "-user",
+        "-wholename",
+    ];
+    const FIND_READ_ONLY_TESTS_WITHOUT_ARGS: &[&str] = &[
+        "-depth",
+        "-empty",
+        "-executable",
+        "-false",
+        "-mount",
+        "-nogroup",
+        "-nouser",
+        "-prune",
+        "-quit",
+        "-readable",
+        "-true",
+        "-writable",
+        "-xdev",
+    ];
+    const FIND_READ_ONLY_ACTIONS_WITH_ARGS: &[&str] = &["-printf"];
+    const FIND_READ_ONLY_ACTIONS_WITHOUT_ARGS: &[&str] = &["-ls", "-print", "-print0"];
+
+    let mut args = command.iter().skip(1).map(String::as_str).peekable();
+    let mut saw_expression = false;
+
+    while let Some(arg) = args.next() {
+        if !saw_expression && FIND_OPTIONS_BEFORE_PATHS.contains(&arg) {
+            continue;
+        }
+
+        if !saw_expression && !arg_starts_find_expression(arg) {
+            continue;
+        }
+
+        saw_expression = true;
+
+        if FIND_READ_ONLY_OPERATORS.contains(&arg)
+            || FIND_READ_ONLY_TESTS_WITHOUT_ARGS.contains(&arg)
+            || FIND_READ_ONLY_ACTIONS_WITHOUT_ARGS.contains(&arg)
+        {
+            continue;
+        }
+
+        if FIND_READ_ONLY_TESTS_WITH_ARGS.contains(&arg)
+            || FIND_READ_ONLY_ACTIONS_WITH_ARGS.contains(&arg)
+            || arg.starts_with("-newer")
+        {
+            if args.next().is_none() {
+                return false;
+            }
+            continue;
+        }
+
+        if matches!(arg, "-maxdepth" | "-mindepth") {
+            let Some(depth) = args.next() else {
+                return false;
+            };
+            if depth.is_empty() || !depth.chars().all(|ch| ch.is_ascii_digit()) {
+                return false;
+            }
+            continue;
+        }
+
+        return false;
+    }
+
+    true
+}
+
+fn arg_starts_find_expression(arg: &str) -> bool {
+    arg == "!" || arg == "(" || arg == ")" || arg.starts_with('-')
+}
+
 fn git_subcommand_args_are_read_only(args: &[String]) -> bool {
     // Flags that can write to disk or execute external tools should never be
     // auto-approved on an unsandboxed machine.
@@ -307,6 +394,15 @@ mod tests {
         assert!(is_safe_to_call_with_exec(&vec_str(&[
             "find", ".", "-name", "file.txt"
         ])));
+        assert!(is_safe_to_call_with_exec(&vec_str(&[
+            "find",
+            "/app/test-data",
+            "-maxdepth",
+            "2",
+            "-type",
+            "f",
+            "-print0",
+        ])));
 
         if cfg!(target_os = "linux") {
             assert!(is_safe_to_call_with_exec(&vec_str(&["numfmt", "1000"])));
@@ -461,6 +557,8 @@ mod tests {
             vec_str(&["find", ".", "-fprint", "/etc/passwd"]),
             vec_str(&["find", ".", "-fprint0", "/etc/passwd"]),
             vec_str(&["find", ".", "-fprintf", "/root/suid.txt", "%#m %u %p\n"]),
+            vec_str(&["find", ".", "-unknown-primary", "value"]),
+            vec_str(&["find", ".", "-maxdepth", "one"]),
         ] {
             assert!(
                 !is_safe_to_call_with_exec(&args),
@@ -469,6 +567,15 @@ mod tests {
         }
     }
 
+    #[test]
+    fn shell_escaped_find_exec_is_not_safe() {
+        assert!(!is_known_safe_command(&vec_str(&[
+            "bash",
+            "-lc",
+            r#"find /app -maxdepth 0 -e\xec sh -c '/usr/bin/su\do -n id' sh \;"#,
+        ])));
+    }
+
     #[test]
     fn base64_output_options_are_unsafe() {
         for args in [