mirror of
https://github.com/openai/codex.git
synced 2026-10-20 08:19:50 +00:00
This is a follow-up to https://github.com/openai/codex/pull/1705, as that PR inadvertently lost the logic where `PatchApplyBeginEvent` and `PatchApplyEndEvent` events were sent when patches were auto-approved. Though as part of this fix, I believe this also makes an important safety fix to `assess_patch_safety()`, as there was a case that returned `SandboxType::None`, which arguably is the thing we were trying to avoid in #1705. On a high level, we want there to be only one codepath where `apply_patch` happens, which should be unified with the patch to run `exec`, in general, so that sandboxing is applied consistently for both cases. Prior to this change, `apply_patch()` in `core` would either: * exit early, delegating to `exec()` to shell out to `apply_patch` using the appropriate sandbox * proceed to run the logic for `apply_patch` in memory https://github.com/openai/codex/blob/549846b29ad52f6cb4f8560365a731966054a9b3/codex-rs/core/src/apply_patch.rs#L61-L63 In this implementation, only the latter would dispatch `PatchApplyBeginEvent` and `PatchApplyEndEvent`, though the former would dispatch `ExecCommandBeginEvent` and `ExecCommandEndEvent` for the `apply_patch` call (or, more specifically, the `codex --codex-run-as-apply-patch PATCH` call). To unify things in this PR, we: * Eliminate the back half of the `apply_patch()` function, and instead have it also return with `DelegateToExec`, though we add an extra field to the return value, `user_explicitly_approved_this_action`. * In `codex.rs` where we process `DelegateToExec`, we use `SandboxType::None` when `user_explicitly_approved_this_action` is `true`. This means **we no longer run the apply_patch logic in memory**, as we always `exec()`. (Note this is what allowed us to delete so much code in `apply_patch.rs`.) * In `codex.rs`, we further update `notify_exec_command_begin()` and `notify_exec_command_end()` to take additional fields to determine what type of notification to send: `ExecCommand` or `PatchApply`. Admittedly, this PR also drops some of the functionality about giving the user the opportunity to expand the set of writable roots as part of approving the `apply_patch` command. I'm not sure how much that was used, and we should probably rethink how that works as we are currently tidying up the protocol to the TUI, in general.
268 lines
8.9 KiB
Rust
268 lines
8.9 KiB
Rust
use std::collections::HashSet;
|
||
use std::path::Component;
|
||
use std::path::Path;
|
||
use std::path::PathBuf;
|
||
|
||
use codex_apply_patch::ApplyPatchAction;
|
||
use codex_apply_patch::ApplyPatchFileChange;
|
||
|
||
use crate::exec::SandboxType;
|
||
use crate::is_safe_command::is_known_safe_command;
|
||
use crate::protocol::AskForApproval;
|
||
use crate::protocol::SandboxPolicy;
|
||
|
||
#[derive(Debug)]
|
||
pub enum SafetyCheck {
|
||
AutoApprove { sandbox_type: SandboxType },
|
||
AskUser,
|
||
Reject { reason: String },
|
||
}
|
||
|
||
pub fn assess_patch_safety(
|
||
action: &ApplyPatchAction,
|
||
policy: AskForApproval,
|
||
writable_roots: &[PathBuf],
|
||
cwd: &Path,
|
||
) -> SafetyCheck {
|
||
if action.is_empty() {
|
||
return SafetyCheck::Reject {
|
||
reason: "empty patch".to_string(),
|
||
};
|
||
}
|
||
|
||
match policy {
|
||
AskForApproval::OnFailure | AskForApproval::Never => {
|
||
// Continue to see if this can be auto-approved.
|
||
}
|
||
// TODO(ragona): I'm not sure this is actually correct? I believe in this case
|
||
// we want to continue to the writable paths check before asking the user.
|
||
AskForApproval::UnlessTrusted => {
|
||
return SafetyCheck::AskUser;
|
||
}
|
||
}
|
||
|
||
// Even though the patch *appears* to be constrained to writable paths, it
|
||
// is possible that paths in the patch are hard links to files outside the
|
||
// writable roots, so we should still run `apply_patch` in a sandbox in that
|
||
// case.
|
||
if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd)
|
||
|| policy == AskForApproval::OnFailure
|
||
{
|
||
// Only auto‑approve when we can actually enforce a sandbox. Otherwise
|
||
// fall back to asking the user because the patch may touch arbitrary
|
||
// paths outside the project.
|
||
match get_platform_sandbox() {
|
||
Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type },
|
||
None => SafetyCheck::AskUser,
|
||
}
|
||
} else if policy == AskForApproval::Never {
|
||
SafetyCheck::Reject {
|
||
reason: "writing outside of the project; rejected by user approval settings"
|
||
.to_string(),
|
||
}
|
||
} else {
|
||
SafetyCheck::AskUser
|
||
}
|
||
}
|
||
|
||
/// For a command to be run _without_ a sandbox, one of the following must be
|
||
/// true:
|
||
///
|
||
/// - the user has explicitly approved the command
|
||
/// - the command is on the "known safe" list
|
||
/// - `DangerFullAccess` was specified and `UnlessTrusted` was not
|
||
pub fn assess_command_safety(
|
||
command: &[String],
|
||
approval_policy: AskForApproval,
|
||
sandbox_policy: &SandboxPolicy,
|
||
approved: &HashSet<Vec<String>>,
|
||
) -> SafetyCheck {
|
||
// A command is "trusted" because either:
|
||
// - it belongs to a set of commands we consider "safe" by default, or
|
||
// - the user has explicitly approved the command for this session
|
||
//
|
||
// Currently, whether a command is "trusted" is a simple boolean, but we
|
||
// should include more metadata on this command test to indicate whether it
|
||
// should be run inside a sandbox or not. (This could be something the user
|
||
// defines as part of `execpolicy`.)
|
||
//
|
||
// For example, when `is_known_safe_command(command)` returns `true`, it
|
||
// would probably be fine to run the command in a sandbox, but when
|
||
// `approved.contains(command)` is `true`, the user may have approved it for
|
||
// the session _because_ they know it needs to run outside a sandbox.
|
||
if is_known_safe_command(command) || approved.contains(command) {
|
||
return SafetyCheck::AutoApprove {
|
||
sandbox_type: SandboxType::None,
|
||
};
|
||
}
|
||
|
||
assess_safety_for_untrusted_command(approval_policy, sandbox_policy)
|
||
}
|
||
|
||
pub(crate) fn assess_safety_for_untrusted_command(
|
||
approval_policy: AskForApproval,
|
||
sandbox_policy: &SandboxPolicy,
|
||
) -> SafetyCheck {
|
||
use AskForApproval::*;
|
||
use SandboxPolicy::*;
|
||
|
||
match (approval_policy, sandbox_policy) {
|
||
(UnlessTrusted, _) => {
|
||
// Even though the user may have opted into DangerFullAccess,
|
||
// they also requested that we ask for approval for untrusted
|
||
// commands.
|
||
SafetyCheck::AskUser
|
||
}
|
||
(OnFailure, DangerFullAccess) | (Never, DangerFullAccess) => SafetyCheck::AutoApprove {
|
||
sandbox_type: SandboxType::None,
|
||
},
|
||
(Never, ReadOnly)
|
||
| (Never, WorkspaceWrite { .. })
|
||
| (OnFailure, ReadOnly)
|
||
| (OnFailure, WorkspaceWrite { .. }) => {
|
||
match get_platform_sandbox() {
|
||
Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type },
|
||
None => {
|
||
if matches!(approval_policy, OnFailure) {
|
||
// Since the command is not trusted, even though the
|
||
// user has requested to only ask for approval on
|
||
// failure, we will ask the user because no sandbox is
|
||
// available.
|
||
SafetyCheck::AskUser
|
||
} else {
|
||
// We are in non-interactive mode and lack approval, so
|
||
// all we can do is reject the command.
|
||
SafetyCheck::Reject {
|
||
reason: "auto-rejected because command is not on trusted list"
|
||
.to_string(),
|
||
}
|
||
}
|
||
}
|
||
}
|
||
}
|
||
}
|
||
}
|
||
|
||
pub fn get_platform_sandbox() -> Option<SandboxType> {
|
||
if cfg!(target_os = "macos") {
|
||
Some(SandboxType::MacosSeatbelt)
|
||
} else if cfg!(target_os = "linux") {
|
||
Some(SandboxType::LinuxSeccomp)
|
||
} else {
|
||
None
|
||
}
|
||
}
|
||
|
||
fn is_write_patch_constrained_to_writable_paths(
|
||
action: &ApplyPatchAction,
|
||
writable_roots: &[PathBuf],
|
||
cwd: &Path,
|
||
) -> bool {
|
||
// Early‑exit if there are no declared writable roots.
|
||
if writable_roots.is_empty() {
|
||
return false;
|
||
}
|
||
|
||
// Normalize a path by removing `.` and resolving `..` without touching the
|
||
// filesystem (works even if the file does not exist).
|
||
fn normalize(path: &Path) -> Option<PathBuf> {
|
||
let mut out = PathBuf::new();
|
||
for comp in path.components() {
|
||
match comp {
|
||
Component::ParentDir => {
|
||
out.pop();
|
||
}
|
||
Component::CurDir => { /* skip */ }
|
||
other => out.push(other.as_os_str()),
|
||
}
|
||
}
|
||
Some(out)
|
||
}
|
||
|
||
// Determine whether `path` is inside **any** writable root. Both `path`
|
||
// and roots are converted to absolute, normalized forms before the
|
||
// prefix check.
|
||
let is_path_writable = |p: &PathBuf| {
|
||
let abs = if p.is_absolute() {
|
||
p.clone()
|
||
} else {
|
||
cwd.join(p)
|
||
};
|
||
let abs = match normalize(&abs) {
|
||
Some(v) => v,
|
||
None => return false,
|
||
};
|
||
|
||
writable_roots.iter().any(|root| {
|
||
let root_abs = if root.is_absolute() {
|
||
root.clone()
|
||
} else {
|
||
normalize(&cwd.join(root)).unwrap_or_else(|| cwd.join(root))
|
||
};
|
||
|
||
abs.starts_with(&root_abs)
|
||
})
|
||
};
|
||
|
||
for (path, change) in action.changes() {
|
||
match change {
|
||
ApplyPatchFileChange::Add { .. } | ApplyPatchFileChange::Delete => {
|
||
if !is_path_writable(path) {
|
||
return false;
|
||
}
|
||
}
|
||
ApplyPatchFileChange::Update { move_path, .. } => {
|
||
if !is_path_writable(path) {
|
||
return false;
|
||
}
|
||
if let Some(dest) = move_path {
|
||
if !is_path_writable(dest) {
|
||
return false;
|
||
}
|
||
}
|
||
}
|
||
}
|
||
}
|
||
|
||
true
|
||
}
|
||
|
||
#[cfg(test)]
|
||
mod tests {
|
||
#![allow(clippy::unwrap_used)]
|
||
use super::*;
|
||
|
||
#[test]
|
||
fn test_writable_roots_constraint() {
|
||
let cwd = std::env::current_dir().unwrap();
|
||
let parent = cwd.parent().unwrap().to_path_buf();
|
||
|
||
// Helper to build a single‑entry map representing a patch that adds a
|
||
// file at `p`.
|
||
let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string());
|
||
|
||
let add_inside = make_add_change(cwd.join("inner.txt"));
|
||
let add_outside = make_add_change(parent.join("outside.txt"));
|
||
|
||
assert!(is_write_patch_constrained_to_writable_paths(
|
||
&add_inside,
|
||
&[PathBuf::from(".")],
|
||
&cwd,
|
||
));
|
||
|
||
let add_outside_2 = make_add_change(parent.join("outside.txt"));
|
||
assert!(!is_write_patch_constrained_to_writable_paths(
|
||
&add_outside_2,
|
||
&[PathBuf::from(".")],
|
||
&cwd,
|
||
));
|
||
|
||
// With parent dir added as writable root, it should pass.
|
||
assert!(is_write_patch_constrained_to_writable_paths(
|
||
&add_outside,
|
||
&[PathBuf::from("..")],
|
||
&cwd,
|
||
))
|
||
}
|
||
}
|