Files
llmx/codex-rs/core/src/safety.rs
Michael Bolin a9adb4175c fix: enable clippy on tests (#870)
https://github.com/openai/codex/pull/855 added the clippy warning to
disallow `unwrap()`, but apparently we were not verifying that tests
were "clippy clean" in CI, so I ended up with a lot of local errors in
VS Code.

This turns on the check in CI and fixes the offenders.
2025-05-08 16:02:56 -07:00

228 lines
7.2 KiB
Rust
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
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::AutoEdit | 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::UnlessAllowListed => {
return SafetyCheck::AskUser;
}
}
if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) {
SafetyCheck::AutoApprove {
sandbox_type: SandboxType::None,
}
} else if policy == AskForApproval::OnFailure {
// Only autoapprove 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
}
}
pub fn assess_command_safety(
command: &[String],
approval_policy: AskForApproval,
sandbox_policy: &SandboxPolicy,
approved: &HashSet<Vec<String>>,
) -> SafetyCheck {
let approve_without_sandbox = || SafetyCheck::AutoApprove {
sandbox_type: SandboxType::None,
};
// Previously approved or allow-listed commands
// All approval modes allow these commands to continue without sandboxing
if is_known_safe_command(command) || approved.contains(command) {
// TODO(ragona): I think we should consider running even these inside the sandbox, but it's
// a change in behavior so I'm keeping it at parity with upstream for now.
return approve_without_sandbox();
}
// Command was not known-safe or allow-listed
if sandbox_policy.is_unrestricted() {
approve_without_sandbox()
} else {
match get_platform_sandbox() {
// We have a sandbox, so we can approve the command in all modes
Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type },
None => {
// We do not have a sandbox, so we need to consider the approval policy
match approval_policy {
// Never is our "non-interactive" mode; it must automatically reject
AskForApproval::Never => SafetyCheck::Reject {
reason: "auto-rejected by user approval settings".to_string(),
},
// Otherwise, we ask the user for approval
_ => SafetyCheck::AskUser,
}
}
}
}
}
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 {
// Earlyexit 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 singleentry 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,
))
}
}