mirror of
https://github.com/openai/codex.git
synced 2026-05-23 20:44:50 +00:00
## Why Runtime decisions should not infer permissions from the lossy legacy sandbox projection once `PermissionProfile` is available. In particular, `Disabled` and `External` need to remain distinct, and managed profiles with split filesystem or deny-read rules should not be collapsed before approval, network, safety, or analytics code makes decisions. ## What Changed - Changes managed network proxy setup and network approval logic to use `PermissionProfile` when deciding whether a managed sandbox is active. - Migrates patch safety, Guardian/user-shell approval paths, Landlock helper setup, analytics sandbox classification, and selected turn/session code to profile-backed permissions. - Validates command-level profile overrides against the constrained `PermissionProfile` rather than a strict `SandboxPolicy` round trip. - Preserves configured deny-read restrictions when command profiles are narrowed. - Adds coverage for profile-backed trust, network proxy/approval behavior, patch safety, analytics classification, and command-profile narrowing. ## Verification - `cargo test -p codex-core direct_write_roots` - `cargo test -p codex-core runtime_roots_to_legacy_projection` - `cargo test -p codex-app-server requested_permissions_trust_project_uses_permission_profile_intent` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19393). * #19395 * #19394 * __->__ #19393
198 lines
6.7 KiB
Rust
198 lines
6.7 KiB
Rust
use std::path::Component;
|
||
use std::path::Path;
|
||
use std::path::PathBuf;
|
||
|
||
use crate::util::resolve_path;
|
||
use codex_apply_patch::ApplyPatchAction;
|
||
use codex_apply_patch::ApplyPatchFileChange;
|
||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||
use codex_protocol::models::PermissionProfile;
|
||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||
use codex_protocol::protocol::AskForApproval;
|
||
use codex_sandboxing::SandboxType;
|
||
use codex_sandboxing::get_platform_sandbox;
|
||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||
|
||
const PATCH_REJECTED_OUTSIDE_PROJECT_REASON: &str =
|
||
"writing outside of the project; rejected by user approval settings";
|
||
const PATCH_REJECTED_READ_ONLY_REASON: &str =
|
||
"writing is blocked by read-only sandbox; rejected by user approval settings";
|
||
|
||
#[derive(Debug, PartialEq)]
|
||
pub enum SafetyCheck {
|
||
AutoApprove {
|
||
sandbox_type: SandboxType,
|
||
user_explicitly_approved: bool,
|
||
},
|
||
AskUser,
|
||
Reject {
|
||
reason: String,
|
||
},
|
||
}
|
||
|
||
pub fn assess_patch_safety(
|
||
action: &ApplyPatchAction,
|
||
policy: AskForApproval,
|
||
permission_profile: &PermissionProfile,
|
||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||
cwd: &AbsolutePathBuf,
|
||
windows_sandbox_level: WindowsSandboxLevel,
|
||
) -> SafetyCheck {
|
||
if action.is_empty() {
|
||
return SafetyCheck::Reject {
|
||
reason: "empty patch".to_string(),
|
||
};
|
||
}
|
||
|
||
match policy {
|
||
AskForApproval::OnFailure
|
||
| AskForApproval::Never
|
||
| AskForApproval::OnRequest
|
||
| AskForApproval::Granular(_) => {
|
||
// 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;
|
||
}
|
||
}
|
||
|
||
let rejects_sandbox_approval = matches!(policy, AskForApproval::Never)
|
||
|| matches!(
|
||
policy,
|
||
AskForApproval::Granular(granular_config) if !granular_config.sandbox_approval
|
||
);
|
||
|
||
// 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, file_system_sandbox_policy, cwd)
|
||
|| matches!(policy, AskForApproval::OnFailure)
|
||
{
|
||
if matches!(
|
||
permission_profile,
|
||
PermissionProfile::Disabled | PermissionProfile::External { .. }
|
||
) {
|
||
// Disabled and External profiles intentionally do not apply an
|
||
// outer Codex filesystem sandbox.
|
||
SafetyCheck::AutoApprove {
|
||
sandbox_type: SandboxType::None,
|
||
user_explicitly_approved: false,
|
||
}
|
||
} else {
|
||
// 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(windows_sandbox_level != WindowsSandboxLevel::Disabled) {
|
||
Some(sandbox_type) => SafetyCheck::AutoApprove {
|
||
sandbox_type,
|
||
user_explicitly_approved: false,
|
||
},
|
||
None => {
|
||
if rejects_sandbox_approval {
|
||
SafetyCheck::Reject {
|
||
reason: patch_rejection_reason(
|
||
permission_profile,
|
||
file_system_sandbox_policy,
|
||
cwd,
|
||
)
|
||
.to_string(),
|
||
}
|
||
} else {
|
||
SafetyCheck::AskUser
|
||
}
|
||
}
|
||
}
|
||
}
|
||
} else if rejects_sandbox_approval {
|
||
SafetyCheck::Reject {
|
||
reason: patch_rejection_reason(permission_profile, file_system_sandbox_policy, cwd)
|
||
.to_string(),
|
||
}
|
||
} else {
|
||
SafetyCheck::AskUser
|
||
}
|
||
}
|
||
|
||
fn patch_rejection_reason(
|
||
permission_profile: &PermissionProfile,
|
||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||
cwd: &AbsolutePathBuf,
|
||
) -> &'static str {
|
||
match permission_profile {
|
||
PermissionProfile::Managed { .. }
|
||
if !file_system_sandbox_policy.has_full_disk_write_access()
|
||
&& file_system_sandbox_policy
|
||
.get_writable_roots_with_cwd(cwd.as_path())
|
||
.is_empty() =>
|
||
{
|
||
PATCH_REJECTED_READ_ONLY_REASON
|
||
}
|
||
PermissionProfile::Managed { .. }
|
||
| PermissionProfile::Disabled
|
||
| PermissionProfile::External { .. } => PATCH_REJECTED_OUTSIDE_PROJECT_REASON,
|
||
}
|
||
}
|
||
|
||
fn is_write_patch_constrained_to_writable_paths(
|
||
action: &ApplyPatchAction,
|
||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||
cwd: &AbsolutePathBuf,
|
||
) -> bool {
|
||
// 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 = resolve_path(cwd, p);
|
||
let abs = match normalize(&abs) {
|
||
Some(v) => v,
|
||
None => return false,
|
||
};
|
||
|
||
file_system_sandbox_policy.can_write_path_with_cwd(&abs, cwd)
|
||
};
|
||
|
||
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
|
||
&& !is_path_writable(dest)
|
||
{
|
||
return false;
|
||
}
|
||
}
|
||
}
|
||
}
|
||
|
||
true
|
||
}
|
||
|
||
#[cfg(test)]
|
||
#[path = "safety_tests.rs"]
|
||
mod tests;
|