openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-06 15:29:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

Authorize apply_patch in the executor path context (#42391)

Browse Source 
## Why

Patch targets can use a different path convention from the Codex host, so
host-native path conversion can misclassify writable roots and requested
permissions.

## What changed

- Evaluate patch targets as `PathUri` values with the active filesystem policy
  context, including workspace roots and the executor's path convention.
- Distinguish executor-managed sandboxing from local platform sandboxing when
  deciding whether a patch can be auto-approved and how to normalize additional
  write permissions.
- Make full-disk and special-path policy checks honor the selected executor's
  Windows or POSIX convention.

## Testing

Add coverage for Windows executor URIs, full-disk policy aliases, remote patch
permission requests, sandbox availability, and owner-provided workspace roots.

GitOrigin-RevId: 1a054ea443efd342623c67432762f85c53d20c15
This commit is contained in:
iceweasel-oai
2026-09-02 22:33:13 +00:00
committed by copyberry
parent e6249b5296
commit fe140d4c8e
8 changed files with 389 additions and 202 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
codex-rs/core/src/apply_patch.rs
View File

@@ -1,4 +1,5 @@
use crate::function_tool::FunctionCallError;
use crate::safety::PatchSandboxRoute;
use crate::safety::SafetyCheck;
use crate::safety::assess_patch_safety;
use crate::session::step_context::StepContext;
@@ -6,6 +7,7 @@ use crate::session::turn_context::TurnEnvironment;
use crate::tools::sandboxing::ExecApprovalRequirement;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchFileChange;
use codex_protocol::permissions::FileSystemSandboxPolicyContext;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::FileSystemSandboxPolicy;
use codex_utils_path_uri::PathUri;
@@ -23,6 +25,8 @@ pub(crate) fn prepare_apply_patch(
step_context: &StepContext,
turn_environment: &TurnEnvironment,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
context: &FileSystemSandboxPolicyContext<'_>,
sandbox_route: PatchSandboxRoute,
action: ApplyPatchAction,
) -> Result<ApplyPatchRuntimeInvocation, FunctionCallError> {
match assess_patch_safety(
@@ -30,8 +34,8 @@ pub(crate) fn prepare_apply_patch(
step_context.settings.approval_policy(),
turn_environment.permission_profile(),
file_system_sandbox_policy,
&action.cwd,
turn_environment.config().windows_sandbox_level,
context,
sandbox_route,
) {
SafetyCheck::AutoApprove => Ok(ApplyPatchRuntimeInvocation {
action,

13
codex-rs/core/src/apply_patch_tests.rs
View File

@@ -58,9 +58,18 @@ async fn prepare_apply_patch_uses_action_policy_before_turn_policy() {
.clone();
environment.config_mut().permission_profile =
PermissionProfileSnapshot::legacy(permission_profile);
let sandbox = environment.sandbox_context(/*additional_permissions*/ None);
let context = sandbox.policy_context().expect("local sandbox context");
let prepared = prepare_apply_patch(&step, &environment, &file_system_policy, action)
.expect("issuing action policy should request approval");
let prepared = prepare_apply_patch(
&step,
&environment,
&file_system_policy,
&context,
PatchSandboxRoute::Platform(codex_protocol::config_types::WindowsSandboxLevel::Disabled),
action,
)
.expect("issuing action policy should request approval");
assert!(!prepared.auto_approved);
assert!(matches!(

107
codex-rs/core/src/safety.rs
View File

@@ -1,12 +1,9 @@
use std::path::Component;
use std::path::Path;
use std::path::PathBuf;
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::permissions::FileSystemSandboxPolicyContext;
use codex_protocol::protocol::AskForApproval;
use codex_sandboxing::get_platform_sandbox;
use codex_utils_path_uri::PathUri;
@@ -23,13 +20,19 @@ pub enum SafetyCheck {
Reject { reason: String },
}
#[derive(Debug, Clone, Copy)]
pub(crate) enum PatchSandboxRoute {
ExecutorManaged,
Platform(WindowsSandboxLevel),
}
pub fn assess_patch_safety(
action: &ApplyPatchAction,
policy: AskForApproval,
permission_profile: &PermissionProfile,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &PathUri,
windows_sandbox_level: WindowsSandboxLevel,
context: &FileSystemSandboxPolicyContext<'_>,
sandbox_route: PatchSandboxRoute,
) -> SafetyCheck {
if action.is_empty() {
return SafetyCheck::Reject {
@@ -53,43 +56,27 @@ pub fn assess_patch_safety(
policy,
AskForApproval::Granular(granular_config) if !granular_config.sandbox_approval
);
let sandbox_available = match sandbox_route {
PatchSandboxRoute::ExecutorManaged => true,
PatchSandboxRoute::Platform(windows_sandbox_level) => {
get_platform_sandbox(windows_sandbox_level != WindowsSandboxLevel::Disabled).is_some()
}
};
// 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) {
if matches!(
// Disabled and External profiles intentionally do not apply an outer sandbox.
if is_write_patch_constrained_to_writable_paths(action, file_system_sandbox_policy, context)
&& (matches!(
permission_profile,
PermissionProfile::Disabled | PermissionProfile::External { .. }
) {
// Disabled and External profiles intentionally do not apply an
// outer Codex filesystem sandbox.
SafetyCheck::AutoApprove
} 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(_) => SafetyCheck::AutoApprove,
None => {
if rejects_sandbox_approval {
SafetyCheck::Reject {
reason: patch_rejection_reason(
permission_profile,
file_system_sandbox_policy,
cwd,
)
.to_string(),
}
} else {
SafetyCheck::AskUser
}
}
}
}
) || sandbox_available)
{
SafetyCheck::AutoApprove
} else if rejects_sandbox_approval {
SafetyCheck::Reject {
reason: patch_rejection_reason(permission_profile, file_system_sandbox_policy, cwd)
reason: patch_rejection_reason(permission_profile, file_system_sandbox_policy, context)
.to_string(),
}
} else {
@@ -100,14 +87,12 @@ pub fn assess_patch_safety(
fn patch_rejection_reason(
permission_profile: &PermissionProfile,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &PathUri,
context: &FileSystemSandboxPolicyContext<'_>,
) -> &'static str {
let has_no_writable_roots = cwd
.to_abs_path()
.is_ok_and(|cwd| !file_system_sandbox_policy.has_writable_roots_with_cwd(cwd.as_path()));
let has_no_writable_roots = !file_system_sandbox_policy.has_configured_writable_roots(context);
match permission_profile {
PermissionProfile::Managed { .. }
if !file_system_sandbox_policy.has_full_disk_write_access()
if !file_system_sandbox_policy.has_full_disk_write_access_with_context(context)
&& has_no_writable_roots =>
{
PATCH_REJECTED_READ_ONLY_REASON
@@ -121,49 +106,15 @@ fn patch_rejection_reason(
fn is_write_patch_constrained_to_writable_paths(
action: &ApplyPatchAction,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &PathUri,
context: &FileSystemSandboxPolicyContext<'_>,
) -> bool {
// A full-disk policy permits every patch target, so no per-path writable-root check can
// further constrain the result.
if file_system_sandbox_policy.has_full_disk_write_access() {
if file_system_sandbox_policy.has_full_disk_write_access_with_context(context) {
return true;
}
// TODO(anp): Make filesystem sandbox policies operate on PathUri.
let Ok(native_cwd) = cwd.to_abs_path() else {
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 = |path: &PathUri| {
// TODO(anp): Make sandbox policy path checks accept PathUri without host projection.
let Ok(path) = path.to_abs_path() else {
return false;
};
let abs = path.into_path_buf();
let abs = match normalize(&abs) {
Some(v) => v,
None => return false,
};
file_system_sandbox_policy.can_write_path_with_cwd(&abs, &native_cwd)
};
let is_path_writable =
|path: &PathUri| file_system_sandbox_policy.can_write_path(path, context);
for (path, change) in action.changes() {
match change {

150
codex-rs/core/src/safety_tests.rs
View File

@@ -1,5 +1,6 @@
use super::*;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemSandboxPolicyContext;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::FileSystemAccessMode;
use codex_protocol::protocol::FileSystemPath;
@@ -12,6 +13,109 @@ use core_test_support::PathExt;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
fn local_context(cwd: &PathUri) -> FileSystemSandboxPolicyContext<'_> {
FileSystemSandboxPolicyContext {
cwd,
workspace_roots: std::slice::from_ref(cwd),
user_home_dir: None,
temporary_directories: None,
}
}
#[test]
fn windows_patch_matching_is_uri_native() {
let cwd = PathUri::parse("file:///C:/workspace").expect("Windows cwd");
let context = local_context(&cwd);
let permission_profile = PermissionProfile::workspace_write();
let policy = permission_profile.file_system_sandbox_policy();
let inside = ApplyPatchAction::new_add_for_test(
&PathUri::parse("file:///C:/workspace/in.txt").expect("inside"),
String::new(),
);
let outside = ApplyPatchAction::new_add_for_test(
&PathUri::parse("file:///C:/outside.txt").expect("outside"),
String::new(),
);
assert!(!is_write_patch_constrained_to_writable_paths(
&outside, &policy, &context,
));
assert_eq!(
assess_patch_safety(
&inside,
AskForApproval::Never,
&permission_profile,
&policy,
&context,
PatchSandboxRoute::ExecutorManaged,
),
SafetyCheck::AutoApprove,
);
}
#[test]
fn full_disk_write_uses_executor_path_convention() {
use FileSystemAccessMode::Deny;
use FileSystemAccessMode::Read;
use FileSystemAccessMode::Write;
use FileSystemSpecialPath::Root;
use FileSystemSpecialPath::SlashTmp;
for (root, full_disk_write) in [("file:///", false), ("file:///C:/", true)] {
let cwd = PathUri::parse(root).expect("executor root");
let context = local_context(&cwd);
let mut policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry::new(FileSystemPath::Special { value: Root }, Write),
FileSystemSandboxEntry::new(FileSystemPath::Special { value: SlashTmp }, Deny),
]);
let action = ApplyPatchAction::new_add_for_test(
&cwd.join("tmp/blocked.txt").expect("patch target"),
String::new(),
);
assert_eq!(
policy.has_full_disk_write_access_with_context(&context),
full_disk_write
);
assert_eq!(
assess_patch_safety(
&action,
AskForApproval::OnRequest,
&PermissionProfile::from_runtime_permissions(
&policy,
NetworkSandboxPolicy::Restricted
),
&policy,
&context,
PatchSandboxRoute::ExecutorManaged,
),
if full_disk_write {
SafetyCheck::AutoApprove
} else {
SafetyCheck::AskUser
},
);
// Literal aliases can override equally specific read grants, but never denies.
policy.entries[1].access = Read;
policy.entries.push(FileSystemSandboxEntry::new(
cwd.join("tmp").expect("tmp alias").into(),
Write,
));
assert!(policy.has_full_disk_write_access_with_context(&context));
policy.entries[1].access = Deny;
assert_eq!(
policy.has_full_disk_write_access_with_context(&context),
full_disk_write
);
policy.entries[1].path = cwd.clone().into();
policy.entries[1].access = Read;
assert!(policy.has_full_disk_write_access_with_context(&context));
policy.entries[1].access = Deny;
assert!(!policy.has_full_disk_write_access_with_context(&context));
}
}
#[test]
fn test_writable_roots_constraint() {
// Use a temporary directory as our workspace to avoid touching
@@ -40,13 +144,13 @@ fn test_writable_roots_constraint() {
assert!(is_write_patch_constrained_to_writable_paths(
&add_inside,
&workspace_only_file_system_policy,
&cwd_uri,
&local_context(&cwd_uri),
));
assert!(!is_write_patch_constrained_to_writable_paths(
&add_outside,
&workspace_only_file_system_policy,
&cwd_uri,
&local_context(&cwd_uri),
));
// With the parent dir explicitly added as a writable root, the
@@ -59,7 +163,7 @@ fn test_writable_roots_constraint() {
assert!(is_write_patch_constrained_to_writable_paths(
&add_outside,
&file_system_policy_with_parent,
&cwd_uri,
&local_context(&cwd_uri),
));
}
@@ -85,8 +189,8 @@ fn external_sandbox_auto_approves_in_on_request() {
AskForApproval::OnRequest,
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled)
),
SafetyCheck::AutoApprove
);
@@ -115,8 +219,8 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() {
AskForApproval::OnRequest,
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled,
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
),
SafetyCheck::AskUser,
);
@@ -132,8 +236,8 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() {
}),
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled,
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
),
SafetyCheck::AskUser,
);
@@ -168,8 +272,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() {
}),
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled,
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
),
SafetyCheck::Reject {
reason: PATCH_REJECTED_OUTSIDE_PROJECT_REASON.to_string(),
@@ -191,7 +295,7 @@ fn read_only_policy_rejects_patch_with_read_only_reason() {
assert!(!is_write_patch_constrained_to_writable_paths(
&action,
&file_system_sandbox_policy,
&cwd_uri,
&local_context(&cwd_uri),
));
assert_eq!(
assess_patch_safety(
@@ -199,8 +303,8 @@ fn read_only_policy_rejects_patch_with_read_only_reason() {
AskForApproval::Never,
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled,
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
),
SafetyCheck::Reject {
reason: PATCH_REJECTED_READ_ONLY_REASON.to_string(),
@@ -241,7 +345,7 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() {
assert!(!is_write_patch_constrained_to_writable_paths(
&action,
&file_system_sandbox_policy,
&cwd_uri,
&local_context(&cwd_uri),
));
assert_eq!(
assess_patch_safety(
@@ -249,8 +353,8 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() {
AskForApproval::OnRequest,
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled,
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
),
SafetyCheck::AskUser,
);
@@ -291,7 +395,7 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() {
assert!(!is_write_patch_constrained_to_writable_paths(
&action,
&file_system_sandbox_policy,
&cwd_uri,
&local_context(&cwd_uri),
));
assert_eq!(
assess_patch_safety(
@@ -299,8 +403,8 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() {
AskForApproval::OnRequest,
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled,
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
),
SafetyCheck::AskUser,
);
@@ -334,7 +438,7 @@ fn missing_project_dot_codex_config_requires_approval() {
assert!(!is_write_patch_constrained_to_writable_paths(
&action,
&file_system_sandbox_policy,
&cwd_uri,
&local_context(&cwd_uri),
));
assert_eq!(
assess_patch_safety(
@@ -342,8 +446,8 @@ fn missing_project_dot_codex_config_requires_approval() {
AskForApproval::OnRequest,
&permission_profile,
&file_system_sandbox_policy,
&cwd_uri,
WindowsSandboxLevel::Disabled,
&local_context(&cwd_uri),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
),
SafetyCheck::AskUser,
);

122
codex-rs/core/src/tools/handlers/apply_patch.rs
View File

@@ -1,4 +1,3 @@
use std::collections::BTreeSet;
use std::collections::HashMap;
use std::path::Path;
use std::path::PathBuf;
@@ -10,6 +9,7 @@ use tokio_util::sync::CancellationToken;
use crate::apply_patch;
use crate::apply_patch::convert_apply_patch_to_protocol;
use crate::function_tool::FunctionCallError;
use crate::safety::PatchSandboxRoute;
use crate::session::session::Session;
use crate::session::step_context::StepContext;
use crate::session::turn_context::TurnContext;
@@ -24,6 +24,7 @@ use crate::tools::events::ToolEmitter;
use crate::tools::events::ToolEventCtx;
use crate::tools::handlers::apply_granted_turn_permissions;
use crate::tools::handlers::apply_patch_spec::create_apply_patch_freeform_tool;
use crate::tools::handlers::file_system_sandbox_policy_context_for_cwd;
use crate::tools::handlers::resolve_tool_environment;
use crate::tools::handlers::updated_hook_command;
use crate::tools::hook_names::HookToolName;
@@ -45,15 +46,16 @@ use codex_exec_server::ExecutorFileSystem;
use codex_features::Feature;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::permissions::FileSystemSandboxPolicyContext;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::PatchApplyUpdatedEvent;
use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy;
use codex_sandboxing::policy_transforms::merge_permission_profiles;
use codex_sandboxing::policy_transforms::normalize_additional_permissions;
use codex_sandboxing::policy_transforms::normalize_additional_permissions_with_context;
use codex_tools::ToolName;
use codex_tools::ToolSpec;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
const APPLY_PATCH_ARGUMENT_DIFF_BUFFER_INTERVAL: Duration = Duration::from_millis(500);
@@ -234,40 +236,47 @@ fn file_paths_for_action(action: &ApplyPatchAction) -> Vec<PathUri> {
}
fn write_permissions_for_paths(
file_paths: &[AbsolutePathBuf],
file_paths: &[PathUri],
file_system_sandbox_policy: &codex_protocol::permissions::FileSystemSandboxPolicy,
cwd: &AbsolutePathBuf,
context: &FileSystemSandboxPolicyContext<'_>,
sandbox_route: PatchSandboxRoute,
) -> Option<AdditionalPermissionProfile> {
let write_paths = file_paths
let mut write_paths = file_paths
.iter()
// Skip already-writable targets before deriving parent permissions.
// Otherwise, a writable directory could grant access to its parent.
.filter(|path| {
!file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path())
})
.filter(|path| !file_system_sandbox_policy.can_write_path(path, context))
.map(|path| {
path.parent()
.or_else(|| match sandbox_route {
PatchSandboxRoute::Platform(_) => {
// Host path rules can recover parents of opaque local paths.
// `to_abs_path` verifies that the target round-trips losslessly.
path.to_abs_path().ok()?.parent().map(PathUri::from)
}
PatchSandboxRoute::ExecutorManaged => None,
})
.unwrap_or_else(|| path.clone())
.into_path_buf()
})
.filter(|path| {
!file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path())
})
.collect::<BTreeSet<_>>()
.into_iter()
.map(AbsolutePathBuf::from_absolute_path)
.collect::<Result<Vec<_>, _>>()
.ok()?;
.filter(|path| !file_system_sandbox_policy.can_write_path(path, context))
.collect::<Vec<_>>();
write_paths.sort_by_key(PathUri::to_string);
write_paths.dedup();
let permissions = (!write_paths.is_empty()).then_some(AdditionalPermissionProfile {
file_system: Some(FileSystemPermissions::from_read_write_roots(
file_system: Some(FileSystemPermissions::from_read_write_path_uris(
Some(vec![]),
Some(write_paths),
)),
..Default::default()
})?;
normalize_additional_permissions(permissions).ok()
match sandbox_route {
PatchSandboxRoute::Platform(_) => normalize_additional_permissions(permissions).ok(),
PatchSandboxRoute::ExecutorManaged => {
normalize_additional_permissions_with_context(permissions, context).ok()
}
}
}
/// Extracts the raw patch text used as the command-shaped hook input for apply_patch.
@@ -282,15 +291,15 @@ async fn effective_patch_permissions(
session: &Session,
environment: &TurnEnvironment,
action: &ApplyPatchAction,
cwd: &PathUri,
) -> std::io::Result<(
context: &FileSystemSandboxPolicyContext<'_>,
sandbox_route: PatchSandboxRoute,
) -> (
Vec<PathUri>,
crate::tools::handlers::EffectiveAdditionalPermissions,
codex_protocol::permissions::FileSystemSandboxPolicy,
)> {
) {
let environment_id = environment.selection.environment_id.as_str();
let file_paths = file_paths_for_action(action);
let native_cwd = cwd.to_abs_path()?;
let granted_permissions = merge_permission_profiles(
session
.granted_session_permissions(environment_id)
@@ -302,49 +311,30 @@ async fn effective_patch_permissions(
.as_ref(),
);
let base_file_system_sandbox_policy = environment
.permission_profile_with_workspace_roots()
.permission_profile()
.file_system_sandbox_policy();
let file_system_sandbox_policy = effective_file_system_sandbox_policy(
&base_file_system_sandbox_policy,
granted_permissions.as_ref(),
);
let native_file_paths = file_paths
.iter()
.map(PathUri::to_abs_path)
.collect::<Result<Vec<_>, _>>()?;
let effective_additional_permissions = apply_granted_turn_permissions(
session,
environment,
cwd,
context.cwd,
crate::sandboxing::SandboxPermissions::UseDefault,
write_permissions_for_paths(&native_file_paths, &file_system_sandbox_policy, &native_cwd),
write_permissions_for_paths(
&file_paths,
&file_system_sandbox_policy,
context,
sandbox_route,
),
)
.await;
Ok((
(
file_paths,
effective_additional_permissions,
file_system_sandbox_policy,
))
}
fn patch_permissions_without_path_matching(
action: &ApplyPatchAction,
) -> (
Vec<PathUri>,
crate::tools::handlers::EffectiveAdditionalPermissions,
codex_protocol::permissions::FileSystemSandboxPolicy,
) {
// TODO(anp): Make permission matching operate on PathUri. Until then, foreign paths skip
// permission matching; a managed turn still fails closed at the platform sandbox boundary.
(
file_paths_for_action(action),
crate::tools::handlers::EffectiveAdditionalPermissions {
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
additional_permissions: None,
permissions_preapproved: false,
},
codex_protocol::permissions::FileSystemSandboxPolicy::unrestricted(),
)
}
@@ -429,7 +419,6 @@ impl ApplyPatchHandler {
};
let content = execute_verified_patch(
changes,
turn_environment.cwd(),
turn_environment.clone(),
Some(&tracker),
tool_ctx,
@@ -539,7 +528,7 @@ pub(crate) async fn intercept_apply_patch(
tool_name: ToolName::plain(tool_name),
};
let content =
execute_verified_patch(changes, cwd, turn_environment, tracker, tool_ctx).await?;
execute_verified_patch(changes, turn_environment, tracker, tool_ctx).await?;
Ok(Some(FunctionToolOutput::from_text(content, Some(true))))
}
codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => {
@@ -557,19 +546,38 @@ pub(crate) async fn intercept_apply_patch(
async fn execute_verified_patch(
action: ApplyPatchAction,
cwd: &PathUri,
turn_environment: TurnEnvironment,
tracker: Option<&SharedTurnDiffTracker>,
tool_ctx: ToolCtx,
) -> Result<String, FunctionCallError> {
let cwd = action.cwd.clone();
let sandbox_context = turn_environment.sandbox_context(/*additional_permissions*/ None);
let Some(policy_context) = file_system_sandbox_policy_context_for_cwd(&sandbox_context, &cwd)
else {
return Err(FunctionCallError::RespondToModel(
"apply_patch requires an executor cwd".to_string(),
));
};
let sandbox_route = if turn_environment.environment.is_remote() {
PatchSandboxRoute::ExecutorManaged
} else {
PatchSandboxRoute::Platform(turn_environment.config().windows_sandbox_level)
};
let (file_paths, effective_additional_permissions, file_system_sandbox_policy) =
effective_patch_permissions(tool_ctx.session.as_ref(), &turn_environment, &action, cwd)
.await
.unwrap_or_else(|_| patch_permissions_without_path_matching(&action));
effective_patch_permissions(
tool_ctx.session.as_ref(),
&turn_environment,
&action,
&policy_context,
sandbox_route,
)
.await;
let apply = apply_patch::prepare_apply_patch(
&tool_ctx.step_context,
&turn_environment,
&file_system_sandbox_policy,
&policy_context,
sandbox_route,
action,
)?;
let changes = convert_apply_patch_to_protocol(&apply.action);

71
codex-rs/core/src/tools/handlers/apply_patch_tests.rs
View File

@@ -1,10 +1,13 @@
use super::*;
use codex_apply_patch::MaybeApplyPatchVerified;
use codex_exec_server::LOCAL_FS;
use codex_protocol::config_types::WindowsSandboxLevel;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::FileSystemSandboxPolicyContext;
use codex_protocol::protocol::FileChange;
use codex_utils_absolute_path::AbsolutePathBuf;
use core_test_support::PathBufExt;
use core_test_support::PathExt;
use pretty_assertions::assert_eq;
@@ -15,6 +18,15 @@ use std::sync::Arc;
use tempfile::TempDir;
use tokio::sync::Mutex;
fn local_context(cwd: &PathUri) -> FileSystemSandboxPolicyContext<'_> {
FileSystemSandboxPolicyContext {
cwd,
workspace_roots: std::slice::from_ref(cwd),
user_home_dir: None,
temporary_directories: None,
}
}
use crate::session::step_context::StepContext;
use crate::session::tests::make_session_and_context;
use crate::tools::context::ToolInvocation;
@@ -281,7 +293,12 @@ fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root()
/*exclude_slash_tmp*/ false,
);
let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd);
let permissions = write_permissions_for_paths(
&[file_path.into()],
&sandbox_policy,
&local_context(&cwd.into()),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
);
assert_eq!(permissions, None);
}
@@ -302,7 +319,12 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() {
/*exclude_slash_tmp*/ true,
);
let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd_abs);
let permissions = write_permissions_for_paths(
&[file_path.into()],
&sandbox_policy,
&local_context(&cwd_abs.into()),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
);
let expected_outside = outside.abs();
assert_eq!(
@@ -324,9 +346,12 @@ fn write_permissions_for_paths_do_not_widen_workspace_root_target() {
/*exclude_tmpdir_env_var*/ true,
/*exclude_slash_tmp*/ true,
);
let permissions =
write_permissions_for_paths(std::slice::from_ref(&cwd), &sandbox_policy, &cwd);
let permissions = write_permissions_for_paths(
&[cwd.clone().into()],
&sandbox_policy,
&local_context(&cwd.into()),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
);
assert_eq!(permissions, None);
}
@@ -341,7 +366,41 @@ fn write_permissions_for_paths_do_not_regrant_an_already_writable_parent() {
FileSystemSandboxEntry::new(file_path.clone().into(), FileSystemAccessMode::Read),
]);
let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd);
let permissions = write_permissions_for_paths(
&[file_path.into()],
&sandbox_policy,
&local_context(&cwd.into()),
PatchSandboxRoute::Platform(WindowsSandboxLevel::Disabled),
);
assert_eq!(permissions, None);
}
#[test]
fn write_permissions_for_windows_paths_uses_executor_uris() {
let cwd = PathUri::parse("file:///C:/workspace").expect("Windows cwd");
let context = local_context(&cwd);
let policy = FileSystemSandboxPolicy::workspace_write(
&[],
/*exclude_tmpdir_env_var*/ true,
/*exclude_slash_tmp*/ true,
);
let outside = PathUri::parse("file:///C:/outside/out.txt").expect("outside");
assert_eq!(
write_permissions_for_paths(
&[outside],
&policy,
&context,
PatchSandboxRoute::ExecutorManaged,
)
.and_then(|profile| profile.file_system)
.map(|permissions| permissions.entries),
Some(vec![FileSystemSandboxEntry::new(
PathUri::parse("file:///C:/outside")
.expect("outside parent")
.into(),
FileSystemAccessMode::Write,
)]),
);
}

6
codex-rs/core/tests/suite/workspace_roots.rs
View File

@@ -1,7 +1,6 @@
use anyhow::Context;
use anyhow::Result;
use codex_core::EnvironmentConfig;
use codex_core::windows_sandbox::WindowsSandboxLevelExt;
use codex_exec_server::CreateDirectoryOptions;
use codex_exec_server::RemoveOptions;
use codex_protocol::config_types::WindowsSandboxLevel;
@@ -234,7 +233,8 @@ async fn workspace_roots_allow_file_and_command_writes_in_secondary_root(
if !owner_resolved_roots {
config.workspace_roots.push(secondary_root);
}
config.set_windows_sandbox_enabled(/*value*/ true);
// Owner-provided sandbox settings must work independently of thread defaults.
config.set_windows_sandbox_enabled(!owner_resolved_roots);
})
.with_workspace_setup(|cwd, fs| async move {
let secondary_root = cwd
@@ -276,7 +276,7 @@ async fn workspace_roots_allow_file_and_command_writes_in_secondary_root(
workspace_roots_profile(),
),
shell_environment_policy: Default::default(),
windows_sandbox_level: WindowsSandboxLevel::from_config(&test.config),
windows_sandbox_level: WindowsSandboxLevel::RestrictedToken,
windows_sandbox_private_desktop: test
.config
.permissions

114
codex-rs/protocol/src/permissions.rs
View File

@@ -743,7 +743,7 @@ impl FileSystemSandboxPolicy {
/// entry for the same target wins under the normal precedence rules, so a
/// shadowed `read` entry must not downgrade the policy out of full-disk
/// write mode.
fn has_write_narrowing_entries(&self) -> bool {
fn has_write_narrowing_entries(&self, convention: PathConvention) -> bool {
matches!(self.kind, FileSystemSandboxKind::Restricted)
&& self.entries.iter().any(|entry| {
if entry.access.can_write() {
@@ -751,15 +751,21 @@ impl FileSystemSandboxPolicy {
}
match &entry.path {
FileSystemPath::Path { .. } => !self.has_same_target_write_override(entry),
FileSystemPath::Path { .. } => {
!self.has_same_target_write_override(entry, convention)
}
FileSystemPath::GlobPattern { .. } => true,
FileSystemPath::Special { value } => match value {
FileSystemSpecialPath::Root => entry.access == FileSystemAccessMode::Deny,
FileSystemSpecialPath::SlashTmp if !cfg!(unix) => false,
FileSystemSpecialPath::SlashTmp
if convention == PathConvention::Windows =>
{
false
}
FileSystemSpecialPath::Minimal | FileSystemSpecialPath::Unknown { .. } => {
false
}
_ => !self.has_same_target_write_override(entry),
_ => !self.has_same_target_write_override(entry, convention),
},
}
})
@@ -767,11 +773,15 @@ impl FileSystemSandboxPolicy {
/// Returns true when a higher-priority `write` entry targets the same
/// location as `entry`, so `entry` cannot narrow effective write access.
fn has_same_target_write_override(&self, entry: &FileSystemSandboxEntry) -> bool {
fn has_same_target_write_override(
&self,
entry: &FileSystemSandboxEntry,
convention: PathConvention,
) -> bool {
self.entries.iter().any(|candidate| {
candidate.access.can_write()
&& candidate.access > entry.access
&& file_system_paths_share_target(&candidate.path, &entry.path)
&& file_system_paths_share_target(&candidate.path, &entry.path, convention)
})
}
@@ -880,14 +890,29 @@ impl FileSystemSandboxPolicy {
}
}
/// Returns true when filesystem writes are unrestricted.
/// Returns true when filesystem writes are unrestricted on this host.
pub fn has_full_disk_write_access(&self) -> bool {
self.has_full_disk_write_access_for_convention(Some(PathConvention::native()))
}
/// Returns true when filesystem writes are unrestricted for the selected executor.
pub fn has_full_disk_write_access_with_context(
&self,
context: &FileSystemSandboxPolicyContext<'_>,
) -> bool {
self.has_full_disk_write_access_for_convention(context.cwd.infer_path_convention())
}
fn has_full_disk_write_access_for_convention(
&self,
convention: Option<PathConvention>,
) -> bool {
match self.kind {
FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => true,
FileSystemSandboxKind::Restricted => {
FileSystemSandboxKind::Restricted => convention.is_some_and(|convention| {
self.has_root_access(FileSystemAccessMode::can_write)
&& !self.has_write_narrowing_entries()
}
&& !self.has_write_narrowing_entries(convention)
}),
}
}
@@ -967,11 +992,16 @@ impl FileSystemSandboxPolicy {
.unwrap_or(FileSystemAccessMode::Deny)
}
fn can_write_path(&self, path: &PathUri, context: &FileSystemSandboxPolicyContext<'_>) -> bool {
pub fn can_write_path(
&self,
path: &PathUri,
context: &FileSystemSandboxPolicyContext<'_>,
) -> bool {
if !self.resolve_access(path, context).can_write() {
return false;
}
self.has_full_disk_write_access() || self.metadata_write_denial(path, context).is_none()
self.has_full_disk_write_access_with_context(context)
|| self.metadata_write_denial(path, context).is_none()
}
fn metadata_write_denial(
@@ -1453,13 +1483,23 @@ impl FileSystemSandboxPolicy {
/// do not currently exist (including `/tmp`). Do not use this result to authorize
/// filesystem access or replace the resolution needed for sandbox enforcement.
pub fn has_configured_writable_roots_with_cwd(&self, cwd: &Path) -> bool {
!self.has_full_disk_write_access()
&& with_local_policy_context(cwd, cwd, |_, context| {
self.resolved_entries(context)
.into_iter()
.any(|(path, access)| access.can_write() && self.can_write_path(&path, context))
})
.unwrap_or(false)
with_local_policy_context(cwd, cwd, |_, context| {
self.has_configured_writable_roots(context)
})
.unwrap_or(false)
}
/// Reports configured writable roots for executor-context diagnostics, excluding
/// full-disk policies and without inspecting the filesystem.
pub fn has_configured_writable_roots(
&self,
context: &FileSystemSandboxPolicyContext<'_>,
) -> bool {
!self.has_full_disk_write_access_with_context(context)
&& self
.resolved_entries(context)
.into_iter()
.any(|(path, access)| access.can_write() && self.can_write_path(&path, context))
}
/// Returns writable roots without following attacker-mutable path components.
@@ -1979,7 +2019,11 @@ fn local_temporary_directories() -> Vec<PathUri> {
/// This is intentionally narrower than full path resolution: it only answers
/// the "can one entry shadow another at the same specificity?" question used
/// by `has_write_narrowing_entries`.
fn file_system_paths_share_target(left: &FileSystemPath, right: &FileSystemPath) -> bool {
fn file_system_paths_share_target(
left: &FileSystemPath,
right: &FileSystemPath,
convention: PathConvention,
) -> bool {
match (left, right) {
(FileSystemPath::Path { path: left }, FileSystemPath::Path { path: right }) => {
left == right
@@ -1988,9 +2032,10 @@ fn file_system_paths_share_target(left: &FileSystemPath, right: &FileSystemPath)
special_paths_share_target(left, right)
}
(FileSystemPath::Path { path }, FileSystemPath::Special { value })
| (FileSystemPath::Special { value }, FileSystemPath::Path { path }) => path
.to_abs_path()
.is_ok_and(|path| special_path_matches_absolute_path(value, &path)),
| (FileSystemPath::Special { value }, FileSystemPath::Path { path }) => {
path.infer_path_convention() == Some(convention)
&& special_path_matches_path_uri(value, path)
}
(
FileSystemPath::GlobPattern { pattern: left },
FileSystemPath::GlobPattern { pattern: right },
@@ -2025,18 +2070,19 @@ fn special_paths_share_target(left: &FileSystemSpecialPath, right: &FileSystemSp
}
}
/// Matches cwd-independent special paths against absolute `Path` entries when
/// Matches cwd-independent special paths against `PathUri` entries when
/// they name the same location.
///
/// We intentionally only fold the special paths whose concrete meaning is
/// stable without a cwd, such as `/` and `/tmp`.
fn special_path_matches_absolute_path(
value: &FileSystemSpecialPath,
path: &AbsolutePathBuf,
) -> bool {
fn special_path_matches_path_uri(value: &FileSystemSpecialPath, path: &PathUri) -> bool {
match value {
FileSystemSpecialPath::Root => path.as_path().parent().is_none(),
FileSystemSpecialPath::SlashTmp => path.as_path() == Path::new("/tmp"),
FileSystemSpecialPath::Root => path.lexical_depth().is_some() && path.parent().is_none(),
FileSystemSpecialPath::SlashTmp => {
path.infer_path_convention() == Some(PathConvention::Posix)
&& path.lexical_depth() == Some(1)
&& path.basename().as_deref() == Some("tmp")
}
_ => false,
}
}
@@ -2329,7 +2375,7 @@ fn append_default_read_only_entry_if_no_explicit_rule(
) {
if entries
.iter()
.any(|entry| file_system_paths_share_target(&entry.path, &path))
.any(|entry| file_system_paths_share_target(&entry.path, &path, PathConvention::native()))
{
return;
}
@@ -2509,6 +2555,12 @@ mod tests {
FileSystemSandboxPolicy::read_only(),
FileSystemSandboxPolicy::unrestricted(),
FileSystemSandboxPolicy::external_sandbox(),
FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new(
FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
FileSystemAccessMode::Write,
)]),
FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new(
writable_root.clone().into(),
FileSystemAccessMode::Write,