agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: fix fs sandbox helper for apply_patch (#18296)

Browse Source 
## Summary

- pass split filesystem sandbox policy/cwd through apply_patch contexts,
while omitting legacy-equivalent policies to keep payloads small
- keep the fs helper compatible with legacy Landlock by avoiding helper
read-root permission expansion in that mode and disabling helper network
access

## Root Cause

`d626dc38950fb40a1a5ad0a8ffab2485e3348c53` routed exec-server filesystem
operations through a sandboxed helper. That path forwarded legacy
Landlock into a helper policy shape that could require direct
split-policy enforcement. Sandboxed `apply_patch` hit that edge through
the filesystem abstraction.

The same 0.121 edit-regression path is consistent with #18354: normal
writes route through the `apply_patch` filesystem helper, fail under
sandbox, and then surface the generic retry-without-sandbox prompt.

Fixes #18069
Fixes #18354

## Validation

- `cd codex-rs && just fmt`
- earlier branch validation before merging current `origin/main` and
dropping the now-separate PATH fix:
  - `cd codex-rs && cargo test -p codex-exec-server`
- `cd codex-rs && cargo test -p codex-core file_system_sandbox_context`
  - `cd codex-rs && just fix -p codex-exec-server`
  - `cd codex-rs && just fix -p codex-core`
  - `git diff --check`
  - `cd codex-rs && cargo clean`

---------

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
viyatb-oai
2026-04-17 15:39:07 -07:00
committed by GitHub
Unverified
parent c9c4caafd8
commit f705f42ba8
5 changed files with 259 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/session/turn_context.rs
+16 -13
View File
@@ -209,6 +209,8 @@ impl TurnContext {
) -> FileSystemSandboxContext {
FileSystemSandboxContext {
sandbox_policy: self.sandbox_policy.get().clone(),
sandbox_policy_cwd: Some(self.cwd.clone()),
file_system_sandbox_policy: self.non_legacy_file_system_sandbox_policy(),
windows_sandbox_level: self.windows_sandbox_level,
windows_sandbox_private_desktop: self
.config
@@ -219,6 +221,19 @@ impl TurnContext {
}
}
fn non_legacy_file_system_sandbox_policy(&self) -> Option<FileSystemSandboxPolicy> {
// Omit the derived split filesystem policy when it is equivalent to
// the legacy sandbox policy. This keeps turn-context payloads stable
// while both fields exist; once callers consume only the split policy,
// this comparison and the legacy projection should go away.
let legacy_file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
self.sandbox_policy.get(),
&self.cwd,
);
(self.file_system_sandbox_policy != legacy_file_system_sandbox_policy)
.then(|| self.file_system_sandbox_policy.clone())
}
pub(crate) fn compact_prompt(&self) -> &str {
self.compact_prompt
.as_deref()
@@ -226,18 +241,6 @@ impl TurnContext {
}
pub(crate) fn to_turn_context_item(&self) -> TurnContextItem {
let legacy_file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
self.sandbox_policy.get(),
&self.cwd,
);
// Omit the derived split filesystem policy when it is equivalent to
// the legacy sandbox policy. This keeps turn-context payloads stable
// while both fields exist; once callers consume only the split policy,
// this comparison and the legacy projection should go away.
let file_system_sandbox_policy = (self.file_system_sandbox_policy
!= legacy_file_system_sandbox_policy)
.then(|| self.file_system_sandbox_policy.clone());
TurnContextItem {
turn_id: Some(self.sub_id.clone()),
trace_id: self.trace_id.clone(),
@@ -247,7 +250,7 @@ impl TurnContext {
approval_policy: self.approval_policy.value(),
sandbox_policy: self.sandbox_policy.get().clone(),
network: self.turn_context_network_item(),
file_system_sandbox_policy,
file_system_sandbox_policy: self.non_legacy_file_system_sandbox_policy(),
model: self.model_info.slug.clone(),
personality: self.personality,
collaboration_mode: Some(self.collaboration_mode.clone()),
codex-rs/core/src/tools/runtimes/apply_patch.rs
+11
View File
@@ -22,6 +22,7 @@ use codex_protocol::error::SandboxErr;
use codex_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::exec_output::StreamOutput;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg;
@@ -74,8 +75,18 @@ impl ApplyPatchRuntime {
return None;
}
let legacy_file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
attempt.policy,
attempt.sandbox_cwd,
);
let file_system_sandbox_policy = (attempt.file_system_policy
!= &legacy_file_system_sandbox_policy)
.then(|| attempt.file_system_policy.clone());
Some(FileSystemSandboxContext {
sandbox_policy: attempt.policy.clone(),
sandbox_policy_cwd: Some(attempt.sandbox_cwd.clone()),
file_system_sandbox_policy,
windows_sandbox_level: attempt.windows_sandbox_level,
windows_sandbox_private_desktop: attempt.windows_sandbox_private_desktop,
use_legacy_landlock: attempt.use_legacy_landlock,
codex-rs/core/src/tools/runtimes/apply_patch_tests.rs
+55 -1
View File
@@ -3,6 +3,9 @@ use crate::tools::sandboxing::SandboxAttempt;
use codex_protocol::config_types::WindowsSandboxLevel;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::GranularApprovalConfig;
@@ -99,7 +102,12 @@ fn file_system_sandbox_context_uses_active_attempt() {
permissions_preapproved: false,
};
let sandbox_policy = SandboxPolicy::new_read_only_policy();
let file_system_policy = FileSystemSandboxPolicy::from(&sandbox_policy);
let mut file_system_policy =
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, path.as_path());
file_system_policy.entries.push(FileSystemSandboxEntry {
path: FileSystemPath::Path { path: path.clone() },
access: FileSystemAccessMode::None,
});
let manager = SandboxManager::new();
let attempt = SandboxAttempt {
sandbox: SandboxType::MacosSeatbelt,
@@ -119,6 +127,11 @@ fn file_system_sandbox_context_uses_active_attempt() {
.expect("sandbox context");
assert_eq!(sandbox.sandbox_policy, sandbox_policy);
assert_eq!(sandbox.sandbox_policy_cwd, Some(path.clone()));
assert_eq!(
sandbox.file_system_sandbox_policy,
Some(file_system_policy.clone())
);
assert_eq!(sandbox.additional_permissions, Some(additional_permissions));
assert_eq!(
sandbox.windows_sandbox_level,
@@ -128,6 +141,47 @@ fn file_system_sandbox_context_uses_active_attempt() {
assert_eq!(sandbox.use_legacy_landlock, true);
}
#[test]
fn file_system_sandbox_context_omits_legacy_equivalent_policy() {
let path = std::env::temp_dir()
.join("apply-patch-runtime-legacy-equivalent.txt")
.abs();
let req = ApplyPatchRequest {
action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()),
file_paths: vec![path.clone()],
changes: HashMap::new(),
exec_approval_requirement: ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: None,
},
additional_permissions: None,
permissions_preapproved: false,
};
let sandbox_policy = SandboxPolicy::new_read_only_policy();
let file_system_policy =
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, path.as_path());
let manager = SandboxManager::new();
let attempt = SandboxAttempt {
sandbox: SandboxType::MacosSeatbelt,
policy: &sandbox_policy,
file_system_policy: &file_system_policy,
network_policy: NetworkSandboxPolicy::Restricted,
enforce_managed_network: false,
manager: &manager,
sandbox_cwd: &path,
codex_linux_sandbox_exe: None,
use_legacy_landlock: true,
windows_sandbox_level: WindowsSandboxLevel::RestrictedToken,
windows_sandbox_private_desktop: true,
};
let sandbox = ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt)
.expect("sandbox context");
assert_eq!(sandbox.sandbox_policy_cwd, Some(path));
assert_eq!(sandbox.file_system_sandbox_policy, None);
}
#[test]
fn no_sandbox_attempt_has_no_file_system_context() {
let path = std::env::temp_dir()
codex-rs/exec-server/src/file_system.rs
+7
View File
@@ -1,6 +1,7 @@
use async_trait::async_trait;
use codex_protocol::config_types::WindowsSandboxLevel;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::protocol::SandboxPolicy;
use codex_utils_absolute_path::AbsolutePathBuf;
use tokio::io;
@@ -41,6 +42,10 @@ pub struct ReadDirectoryEntry {
#[serde(rename_all = "camelCase")]
pub struct FileSystemSandboxContext {
pub sandbox_policy: SandboxPolicy,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub sandbox_policy_cwd: Option<AbsolutePathBuf>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub file_system_sandbox_policy: Option<FileSystemSandboxPolicy>,
pub windows_sandbox_level: WindowsSandboxLevel,
#[serde(default)]
pub windows_sandbox_private_desktop: bool,
@@ -53,6 +58,8 @@ impl FileSystemSandboxContext {
pub fn new(sandbox_policy: SandboxPolicy) -> Self {
Self {
sandbox_policy,
sandbox_policy_cwd: None,
file_system_sandbox_policy: None,
windows_sandbox_level: WindowsSandboxLevel::Disabled,
windows_sandbox_private_desktop: false,
use_legacy_landlock: false,
codex-rs/exec-server/src/fs_sandbox.rs
+170 -51
View File
@@ -12,6 +12,7 @@ use codex_sandboxing::SandboxExecRequest;
use codex_sandboxing::SandboxManager;
use codex_sandboxing::SandboxTransformRequest;
use codex_sandboxing::SandboxablePreference;
use codex_sandboxing::policy_transforms::merge_permission_profiles;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::canonicalize_preserving_symlinks;
use tokio::io::AsyncWriteExt;
@@ -35,6 +36,13 @@ pub(crate) struct FileSystemSandboxRunner {
helper_env: HashMap<String, String>,
}
struct HelperSandboxInputs {
sandbox_policy: SandboxPolicy,
file_system_policy: FileSystemSandboxPolicy,
network_policy: NetworkSandboxPolicy,
cwd: AbsolutePathBuf,
}
impl FileSystemSandboxRunner {
pub(crate) fn new(runtime_paths: ExecServerRuntimePaths) -> Self {
Self {
@@ -48,19 +56,14 @@ impl FileSystemSandboxRunner {
sandbox: &FileSystemSandboxContext,
request: FsHelperRequest,
) -> Result<FsHelperPayload, JSONRPCErrorError> {
let helper_sandbox_policy = normalize_sandbox_policy_root_aliases(
sandbox_policy_with_helper_runtime_defaults(&sandbox.sandbox_policy),
);
let cwd = current_sandbox_cwd().map_err(io_error)?;
let cwd = AbsolutePathBuf::from_absolute_path(cwd.as_path())
.map_err(|err| invalid_request(format!("current directory is not absolute: {err}")))?;
let file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
&helper_sandbox_policy,
cwd.as_path(),
);
let network_policy = NetworkSandboxPolicy::Restricted;
let HelperSandboxInputs {
sandbox_policy,
file_system_policy,
network_policy,
cwd,
} = helper_sandbox_inputs(sandbox)?;
let command = self.sandbox_exec_request(
&helper_sandbox_policy,
&sandbox_policy,
&file_system_policy,
network_policy,
&cwd,
@@ -92,8 +95,9 @@ impl FileSystemSandboxRunner {
args: vec![CODEX_FS_HELPER_ARG1.to_string()],
cwd: cwd.clone(),
env: self.helper_env.clone(),
additional_permissions: Some(
self.helper_permissions(sandbox_context.additional_permissions.as_ref()),
additional_permissions: self.helper_permissions(
sandbox_context.additional_permissions.as_ref(),
/*include_helper_read_root*/ !sandbox_context.use_legacy_landlock,
),
};
sandbox_manager
@@ -117,36 +121,68 @@ impl FileSystemSandboxRunner {
fn helper_permissions(
&self,
additional_permissions: Option<&PermissionProfile>,
) -> PermissionProfile {
let helper_read_root = self
.runtime_paths
.codex_self_exe
.parent()
.and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok());
let file_system =
match additional_permissions.and_then(|permissions| permissions.file_system.clone()) {
Some(mut file_system) => {
if let Some(helper_read_root) = &helper_read_root {
let read_paths = file_system.read.get_or_insert_with(Vec::new);
if !read_paths.contains(helper_read_root) {
read_paths.push(helper_read_root.clone());
}
}
Some(file_system)
}
None => helper_read_root.map(|helper_read_root| FileSystemPermissions {
include_helper_read_root: bool,
) -> Option<PermissionProfile> {
let inherited_permissions = additional_permissions
.map(|permissions| PermissionProfile {
network: None,
file_system: permissions.file_system.clone(),
})
.filter(|permissions| !permissions.is_empty());
let helper_permissions = include_helper_read_root
.then(|| {
self.runtime_paths
.codex_self_exe
.parent()
.and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok())
})
.flatten()
.map(|helper_read_root| PermissionProfile {
network: None,
file_system: Some(FileSystemPermissions {
read: Some(vec![helper_read_root]),
write: None,
}),
};
});
PermissionProfile {
network: None,
file_system,
}
merge_permission_profiles(inherited_permissions.as_ref(), helper_permissions.as_ref())
}
}
fn helper_sandbox_inputs(
sandbox: &FileSystemSandboxContext,
) -> Result<HelperSandboxInputs, JSONRPCErrorError> {
let sandbox_policy = normalize_sandbox_policy_root_aliases(
sandbox_policy_with_helper_runtime_defaults(&sandbox.sandbox_policy),
);
let cwd = match &sandbox.sandbox_policy_cwd {
Some(cwd) => cwd.clone(),
None if sandbox.file_system_sandbox_policy.is_some() => {
return Err(invalid_request(
"fileSystemSandboxPolicy requires sandboxPolicyCwd".to_string(),
));
}
None => {
let cwd = current_sandbox_cwd().map_err(io_error)?;
AbsolutePathBuf::from_absolute_path(cwd.as_path()).map_err(|err| {
invalid_request(format!("current directory is not absolute: {err}"))
})?
}
};
let file_system_policy = sandbox
.file_system_sandbox_policy
.clone()
.unwrap_or_else(|| {
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path())
});
Ok(HelperSandboxInputs {
sandbox_policy,
file_system_policy,
network_policy: NetworkSandboxPolicy::Restricted,
cwd,
})
}
fn normalize_sandbox_policy_root_aliases(sandbox_policy: SandboxPolicy) -> SandboxPolicy {
let mut sandbox_policy = sandbox_policy;
match &mut sandbox_policy {
@@ -281,10 +317,21 @@ fn spawn_command(
fn sandbox_policy_with_helper_runtime_defaults(sandbox_policy: &SandboxPolicy) -> SandboxPolicy {
let mut sandbox_policy = sandbox_policy.clone();
match &mut sandbox_policy {
SandboxPolicy::ReadOnly { access, .. } => enable_platform_defaults(access),
SandboxPolicy::ReadOnly {
access,
network_access,
} => {
enable_platform_defaults(access);
*network_access = false;
}
SandboxPolicy::WorkspaceWrite {
read_only_access, ..
} => enable_platform_defaults(read_only_access),
read_only_access,
network_access,
..
} => {
enable_platform_defaults(read_only_access);
*network_access = false;
}
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => {}
}
sandbox_policy
@@ -326,11 +373,13 @@ mod tests {
use pretty_assertions::assert_eq;
use crate::ExecServerRuntimePaths;
use crate::FileSystemSandboxContext;
use super::FileSystemSandboxRunner;
use super::helper_env;
use super::helper_env_from_vars;
use super::helper_env_key_is_allowed;
use super::helper_sandbox_inputs;
use super::sandbox_policy_with_helper_runtime_defaults;
#[test]
@@ -365,7 +414,7 @@ mod tests {
include_platform_defaults: false,
readable_roots: Vec::new(),
},
network_access: false,
network_access: true,
exclude_tmpdir_env_var: true,
exclude_slash_tmp: true,
};
@@ -387,6 +436,52 @@ mod tests {
);
}
#[test]
fn helper_sandbox_inputs_use_context_cwd_and_file_system_policy() {
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
.expect("absolute temp dir");
let sandbox_policy = SandboxPolicy::new_workspace_write_policy();
let file_system_policy =
codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy(
&sandbox_policy,
cwd.as_path(),
);
let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy.clone());
sandbox_context.sandbox_policy_cwd = Some(cwd.clone());
sandbox_context.file_system_sandbox_policy = Some(file_system_policy.clone());
let inputs = helper_sandbox_inputs(&sandbox_context).expect("helper sandbox inputs");
assert_eq!(inputs.cwd, cwd);
assert_eq!(inputs.sandbox_policy, sandbox_policy);
assert_eq!(inputs.file_system_policy, file_system_policy);
assert_eq!(inputs.network_policy, NetworkSandboxPolicy::Restricted);
}
#[test]
fn helper_sandbox_inputs_rejects_file_system_policy_without_cwd() {
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
.expect("absolute temp dir");
let sandbox_policy = SandboxPolicy::new_workspace_write_policy();
let file_system_policy =
codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy(
&sandbox_policy,
cwd.as_path(),
);
let mut sandbox_context = FileSystemSandboxContext::new(sandbox_policy);
sandbox_context.file_system_sandbox_policy = Some(file_system_policy);
let err = match helper_sandbox_inputs(&sandbox_context) {
Ok(_) => panic!("expected invalid sandbox inputs"),
Err(err) => err,
};
assert_eq!(
err.message,
"fileSystemSandboxPolicy requires sandboxPolicyCwd"
);
}
#[test]
fn helper_permissions_strip_network_grants() {
let codex_self_exe = std::env::current_exe().expect("current exe");
@@ -403,15 +498,20 @@ mod tests {
let writable = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
.expect("absolute writable path");
let permissions = runner.helper_permissions(Some(&PermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
file_system: Some(FileSystemPermissions {
read: Some(vec![]),
write: Some(vec![writable.clone()]),
}),
}));
let permissions = runner
.helper_permissions(
Some(&PermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
file_system: Some(FileSystemPermissions {
read: Some(vec![]),
write: Some(vec![writable.clone()]),
}),
}),
/*include_helper_read_root*/ true,
)
.expect("helper permissions");
assert_eq!(permissions.network, None);
assert_eq!(
@@ -537,7 +637,11 @@ mod tests {
)
.expect("absolute readable path");
let permissions = runner.helper_permissions(/*additional_permissions*/ None);
let permissions = runner
.helper_permissions(
/*additional_permissions*/ None, /*include_helper_read_root*/ true,
)
.expect("helper permissions");
assert_eq!(permissions.network, None);
assert_eq!(
@@ -548,4 +652,19 @@ mod tests {
})
);
}
#[test]
fn legacy_landlock_helper_permissions_do_not_add_helper_read_root() {
let codex_self_exe = std::env::current_exe().expect("current exe");
let runtime_paths =
ExecServerRuntimePaths::new(codex_self_exe, /*codex_linux_sandbox_exe*/ None)
.expect("runtime paths");
let runner = FileSystemSandboxRunner::new(runtime_paths);
let permissions = runner.helper_permissions(
/*additional_permissions*/ None, /*include_helper_read_root*/ false,
);
assert_eq!(permissions, None);
}
}