mirror of
https://github.com/openai/codex.git
synced 2026-11-08 20:40:33 +00:00
Sandbox remote apply_patch operations (#38043)
## Why Cross-platform remote `apply_patch` calls were rejected when filesystem writes were restricted because patch verification and writes could not be safely performed against executor files. ## What changed - Route intercepted and direct remote patches through the executor-managed filesystem sandbox, including the configured workspace roots. - Select the restricted-token sandbox for Windows executor paths when no Windows sandbox level was configured. - Fail closed when an executor cannot enforce the requested sandbox, and treat executor-managed access failures as sandbox denials so approval can retry the patch without sandboxing. ## Testing - Cover sandboxed remote patches, denied writes, approval retries, Windows sandbox selection, and executor filesystem enforcement. GitOrigin-RevId: caddeed0b266c456a689080a14a3a58e2bd7887c
This commit is contained in:
@@ -238,23 +238,6 @@ impl ExecCommandHandler {
|
||||
)
|
||||
.map_err(FunctionCallError::RespondToModel)?;
|
||||
let command = resolved_command.command;
|
||||
if environment.is_remote()
|
||||
&& !cwd_uses_native_convention
|
||||
&& !turn_environment
|
||||
.permission_profile()
|
||||
.file_system_sandbox_policy()
|
||||
.has_full_disk_write_access()
|
||||
&& matches!(
|
||||
codex_apply_patch::maybe_parse_apply_patch(&command, &cwd),
|
||||
codex_apply_patch::MaybeApplyPatch::Body(_)
|
||||
)
|
||||
{
|
||||
// CA-781: patch verification reads executor files before process sandboxing applies.
|
||||
manager.release_process_id(process_id).await;
|
||||
return Err(FunctionCallError::RespondToModel(
|
||||
"cross-platform remote apply_patch is unavailable until executor-side filesystem sandboxing is supported".to_string(),
|
||||
));
|
||||
}
|
||||
let shell_type = resolved_command.shell_type;
|
||||
let command_for_display = codex_shell_command::parse_command::shlex_join(&command);
|
||||
|
||||
@@ -329,7 +312,7 @@ impl ExecCommandHandler {
|
||||
}
|
||||
};
|
||||
|
||||
if let Some(output) = intercept_apply_patch(
|
||||
let intercepted_patch = intercept_apply_patch(
|
||||
&command,
|
||||
&cwd,
|
||||
fs.as_ref(),
|
||||
@@ -340,8 +323,13 @@ impl ExecCommandHandler {
|
||||
&context.call_id,
|
||||
"exec_command",
|
||||
)
|
||||
.await?
|
||||
{
|
||||
.await;
|
||||
// Keep the reservation when interception returns `Ok(None)`: the normal command below
|
||||
// still needs this process ID.
|
||||
if intercepted_patch.is_err() {
|
||||
manager.release_process_id(process_id).await;
|
||||
}
|
||||
if let Some(output) = intercepted_patch? {
|
||||
manager.release_process_id(process_id).await;
|
||||
return Ok(boxed_tool_output(ExecCommandToolOutput {
|
||||
event_call_id: String::new(),
|
||||
|
||||
@@ -13,6 +13,7 @@ use crate::tools::sandboxing::Sandboxable;
|
||||
use crate::tools::sandboxing::ToolCtx;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use crate::tools::sandboxing::executor_windows_sandbox_level;
|
||||
use codex_apply_patch::AppliedPatchDelta;
|
||||
use codex_apply_patch::ApplyPatchAction;
|
||||
use codex_exec_server::FileSystemSandboxContext;
|
||||
@@ -25,6 +26,7 @@ use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::FileChange;
|
||||
use codex_sandboxing::SandboxType;
|
||||
use codex_sandboxing::SandboxablePreference;
|
||||
use codex_sandboxing::is_likely_executor_managed_sandbox_denied;
|
||||
use codex_sandboxing::policy_transforms::effective_permission_profile;
|
||||
use codex_sandboxing::record_filesystem_sandbox_violation;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -85,7 +87,7 @@ impl ApplyPatchRuntime {
|
||||
req: &ApplyPatchRequest,
|
||||
attempt: &SandboxAttempt<'_>,
|
||||
) -> Option<FileSystemSandboxContext> {
|
||||
if attempt.sandbox == SandboxType::None {
|
||||
if !attempt.sandbox_requested {
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -97,7 +99,10 @@ impl ApplyPatchRuntime {
|
||||
permissions: permissions.into(),
|
||||
cwd: Some(attempt.sandbox_cwd.clone()),
|
||||
workspace_roots: attempt.workspace_roots.to_vec(),
|
||||
windows_sandbox_level: attempt.windows_sandbox_level,
|
||||
windows_sandbox_level: executor_windows_sandbox_level(
|
||||
attempt.windows_sandbox_level,
|
||||
attempt.sandbox_cwd,
|
||||
),
|
||||
windows_sandbox_private_desktop: attempt.windows_sandbox_private_desktop,
|
||||
windows_sandbox_proxy_settings_mode: None,
|
||||
use_legacy_landlock: attempt.use_legacy_landlock,
|
||||
@@ -149,6 +154,10 @@ impl ToolRuntime<ApplyPatchRequest, ApplyPatchRuntimeOutput> for ApplyPatchRunti
|
||||
&req.turn_environment
|
||||
}
|
||||
|
||||
fn uses_executor_managed_process_sandbox(&self, req: &ApplyPatchRequest) -> bool {
|
||||
req.turn_environment.environment.is_remote()
|
||||
}
|
||||
|
||||
fn sandbox_cwd<'a>(&self, req: &'a ApplyPatchRequest) -> Option<&'a PathUri> {
|
||||
Some(&req.action.cwd)
|
||||
}
|
||||
@@ -191,8 +200,18 @@ impl ToolRuntime<ApplyPatchRequest, ApplyPatchRuntimeOutput> for ApplyPatchRunti
|
||||
duration: started_at.elapsed(),
|
||||
timed_out: false,
|
||||
};
|
||||
if failed && is_likely_sandbox_denied(attempt.sandbox, &output) {
|
||||
record_filesystem_sandbox_violation(attempt.sandbox, &output);
|
||||
let sandbox_denied = failed
|
||||
&& if attempt.sandbox == SandboxType::None {
|
||||
attempt.sandbox_requested && is_likely_executor_managed_sandbox_denied(&output)
|
||||
} else {
|
||||
is_likely_sandbox_denied(attempt.sandbox, &output)
|
||||
};
|
||||
if sandbox_denied {
|
||||
// TODO(iceweasel): Report executor filesystem sandbox backends like process/start so
|
||||
// executor-managed apply_patch denials can emit backend-specific violation telemetry.
|
||||
if attempt.sandbox != SandboxType::None {
|
||||
record_filesystem_sandbox_violation(attempt.sandbox, &output);
|
||||
}
|
||||
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
|
||||
output: Box::new(output),
|
||||
network_policy_decision: None,
|
||||
|
||||
@@ -267,7 +267,7 @@ async fn file_system_sandbox_context_preserves_executor_workspace_permissions()
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn no_sandbox_attempt_has_no_file_system_context() {
|
||||
async fn file_system_sandbox_context_respects_sandbox_request() {
|
||||
let path = std::env::temp_dir()
|
||||
.join("apply-patch-runtime-none.txt")
|
||||
.abs();
|
||||
@@ -310,4 +310,28 @@ async fn no_sandbox_attempt_has_no_file_system_context() {
|
||||
ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt),
|
||||
None
|
||||
);
|
||||
|
||||
let cwd = PathUri::parse("file:///C:/workspace").expect("Windows workspace URI");
|
||||
let permissions = PermissionProfile::workspace_write();
|
||||
let attempt = SandboxAttempt {
|
||||
sandbox_requested: true,
|
||||
permissions: &permissions,
|
||||
exec_server_permissions: &permissions,
|
||||
sandbox_cwd: &cwd,
|
||||
workspace_roots: std::slice::from_ref(&cwd),
|
||||
..attempt
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt),
|
||||
Some(FileSystemSandboxContext {
|
||||
permissions: permissions.into(),
|
||||
cwd: Some(cwd.clone()),
|
||||
workspace_roots: vec![cwd],
|
||||
windows_sandbox_level: WindowsSandboxLevel::RestrictedToken,
|
||||
windows_sandbox_private_desktop: false,
|
||||
windows_sandbox_proxy_settings_mode: None,
|
||||
use_legacy_landlock: false,
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
@@ -401,6 +401,19 @@ pub(crate) struct SandboxAttempt<'a> {
|
||||
pub(crate) network_proxy: Option<&'a NetworkProxy>,
|
||||
}
|
||||
|
||||
pub(crate) fn executor_windows_sandbox_level(
|
||||
windows_sandbox_level: WindowsSandboxLevel,
|
||||
cwd: &PathUri,
|
||||
) -> WindowsSandboxLevel {
|
||||
if windows_sandbox_level == WindowsSandboxLevel::Disabled
|
||||
&& cwd.infer_path_convention() == Some(PathConvention::Windows)
|
||||
{
|
||||
WindowsSandboxLevel::RestrictedToken
|
||||
} else {
|
||||
windows_sandbox_level
|
||||
}
|
||||
}
|
||||
|
||||
impl<'a> SandboxAttempt<'a> {
|
||||
pub(crate) fn network_proxy<'b>(
|
||||
&'b self,
|
||||
@@ -478,24 +491,14 @@ impl<'a> SandboxAttempt<'a> {
|
||||
crate::sandboxing::ExecRequest::from_sandbox_exec_request(request, options, Vec::new());
|
||||
exec_request.exec_server_managed_network = managed_network;
|
||||
if self.sandbox_requested {
|
||||
// This level comes from the orchestrator's config, so `Disabled` means Windows
|
||||
// sandboxing is irrelevant on a non-Windows host. A Windows executor would instead
|
||||
// treat it as unable to enforce the requested sandbox. Select its baseline restricted
|
||||
// token backend while preserving explicitly configured levels and same-OS behavior.
|
||||
let windows_sandbox_level = if self.windows_sandbox_level
|
||||
== WindowsSandboxLevel::Disabled
|
||||
&& self.sandbox_cwd.infer_path_convention() == Some(PathConvention::Windows)
|
||||
&& PathConvention::native() != PathConvention::Windows
|
||||
{
|
||||
WindowsSandboxLevel::RestrictedToken
|
||||
} else {
|
||||
self.windows_sandbox_level
|
||||
};
|
||||
exec_request.exec_server_sandbox = Some(FileSystemSandboxContext {
|
||||
permissions: exec_server_permissions.into(),
|
||||
cwd: Some(exec_request.windows_sandbox_policy_cwd.clone()),
|
||||
workspace_roots: self.workspace_roots.to_vec(),
|
||||
windows_sandbox_level,
|
||||
windows_sandbox_level: executor_windows_sandbox_level(
|
||||
self.windows_sandbox_level,
|
||||
self.sandbox_cwd,
|
||||
),
|
||||
windows_sandbox_private_desktop: self.windows_sandbox_private_desktop,
|
||||
windows_sandbox_proxy_settings_mode: None,
|
||||
use_legacy_landlock: self.use_legacy_landlock,
|
||||
|
||||
@@ -266,7 +266,11 @@ fn exec_server_env_keeps_command_native_and_carries_sandbox_context() {
|
||||
permissions: exec_server_permissions.clone().into(),
|
||||
cwd: Some(cwd_uri.clone()),
|
||||
workspace_roots: vec![cwd_uri.clone()],
|
||||
windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled,
|
||||
windows_sandbox_level: if cfg!(windows) {
|
||||
codex_protocol::config_types::WindowsSandboxLevel::RestrictedToken
|
||||
} else {
|
||||
codex_protocol::config_types::WindowsSandboxLevel::Disabled
|
||||
},
|
||||
windows_sandbox_private_desktop: false,
|
||||
windows_sandbox_proxy_settings_mode: None,
|
||||
use_legacy_landlock: false,
|
||||
|
||||
Reference in New Issue
Block a user