safety: honor filesystem policy carveouts in apply_patch (#13445)

## Why

`apply_patch` safety approval was still checking writable paths through
the legacy `SandboxPolicy` projection.

That can hide explicit `none` carveouts when a split filesystem policy
projects back to compatibility `ExternalSandbox`, which leaves one more
approval path that can auto-approve writes inside paths that are
intentionally blocked.

## What changed

- passed `turn.file_system_sandbox_policy` into `assess_patch_safety`
- changed writable-path checks to derive effective access from
`FileSystemSandboxPolicy` instead of the legacy `SandboxPolicy`
- made those checks reject explicit unreadable roots before considering
broad write access or writable roots
- added regression coverage showing that an `ExternalSandbox`
compatibility projection still asks for approval when the split
filesystem policy blocks a subpath

## Verification

- `cargo test -p codex-core safety::tests::`
- `cargo test -p codex-core test_sandbox_config_parsing`
- `cargo clippy -p codex-core --all-targets -- -D warnings`

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13445).
* #13453
* #13452
* #13451
* #13449
* #13448
* __->__ #13445
* #13440
* #13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
This commit is contained in:
Michael Bolin
2026-03-07 00:01:08 -08:00
committed by GitHub
parent 8df4d9b3b2
commit 5ceff6588e
3 changed files with 73 additions and 17 deletions

View File

@@ -40,6 +40,7 @@ pub(crate) async fn apply_patch(
&action,
turn_context.approval_policy.value(),
turn_context.sandbox_policy.get(),
&turn_context.file_system_sandbox_policy,
&turn_context.cwd,
turn_context.windows_sandbox_level,
) {

View File

@@ -844,7 +844,6 @@ fn unsupported_windows_restricted_token_sandbox_reason(
)
})
}
/// Consumes the output of a child process, truncating it so it is suitable for
/// use as the output of a `shell` tool call. Also enforces specified timeout.
async fn consume_truncated_output(

View File

@@ -9,6 +9,7 @@ use crate::exec::SandboxType;
use crate::util::resolve_path;
use crate::protocol::AskForApproval;
use crate::protocol::FileSystemSandboxPolicy;
use crate::protocol::SandboxPolicy;
use codex_protocol::config_types::WindowsSandboxLevel;
@@ -28,6 +29,7 @@ pub fn assess_patch_safety(
action: &ApplyPatchAction,
policy: AskForApproval,
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
windows_sandbox_level: WindowsSandboxLevel,
) -> SafetyCheck {
@@ -60,7 +62,7 @@ pub fn assess_patch_safety(
// 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, sandbox_policy, cwd)
if is_write_patch_constrained_to_writable_paths(action, file_system_sandbox_policy, cwd)
|| matches!(policy, AskForApproval::OnFailure)
{
if matches!(
@@ -122,20 +124,9 @@ pub fn get_platform_sandbox(windows_sandbox_enabled: bool) -> Option<SandboxType
fn is_write_patch_constrained_to_writable_paths(
action: &ApplyPatchAction,
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> bool {
// Earlyexit if there are no declared writable roots.
let writable_roots = match sandbox_policy {
SandboxPolicy::ReadOnly { .. } => {
return false;
}
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => {
return true;
}
SandboxPolicy::WorkspaceWrite { .. } => sandbox_policy.get_writable_roots_with_cwd(cwd),
};
// 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> {
@@ -152,6 +143,9 @@ fn is_write_patch_constrained_to_writable_paths(
Some(out)
}
let unreadable_roots = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd);
let writable_roots = file_system_sandbox_policy.get_writable_roots_with_cwd(cwd);
// Determine whether `path` is inside **any** writable root. Both `path`
// and roots are converted to absolute, normalized forms before the
// prefix check.
@@ -162,6 +156,17 @@ fn is_write_patch_constrained_to_writable_paths(
None => return false,
};
if unreadable_roots
.iter()
.any(|root| abs.starts_with(root.as_path()))
{
return false;
}
if file_system_sandbox_policy.has_full_disk_write_access() {
return true;
}
writable_roots
.iter()
.any(|writable_root| writable_root.is_path_writable(&abs))
@@ -193,6 +198,10 @@ fn is_write_patch_constrained_to_writable_paths(
#[cfg(test)]
mod tests {
use super::*;
use codex_protocol::protocol::FileSystemAccessMode;
use codex_protocol::protocol::FileSystemPath;
use codex_protocol::protocol::FileSystemSandboxEntry;
use codex_protocol::protocol::FileSystemSpecialPath;
use codex_protocol::protocol::RejectConfig;
use codex_utils_absolute_path::AbsolutePathBuf;
use tempfile::TempDir;
@@ -223,13 +232,13 @@ mod tests {
assert!(is_write_patch_constrained_to_writable_paths(
&add_inside,
&policy_workspace_only,
&FileSystemSandboxPolicy::from(&policy_workspace_only),
&cwd,
));
assert!(!is_write_patch_constrained_to_writable_paths(
&add_outside,
&policy_workspace_only,
&FileSystemSandboxPolicy::from(&policy_workspace_only),
&cwd,
));
@@ -244,7 +253,7 @@ mod tests {
};
assert!(is_write_patch_constrained_to_writable_paths(
&add_outside,
&policy_with_parent,
&FileSystemSandboxPolicy::from(&policy_with_parent),
&cwd,
));
}
@@ -264,6 +273,7 @@ mod tests {
&add_inside,
AskForApproval::OnRequest,
&policy,
&FileSystemSandboxPolicy::from(&policy),
&cwd,
WindowsSandboxLevel::Disabled
),
@@ -294,6 +304,7 @@ mod tests {
&add_outside,
AskForApproval::OnRequest,
&policy_workspace_only,
&FileSystemSandboxPolicy::from(&policy_workspace_only),
&cwd,
WindowsSandboxLevel::Disabled,
),
@@ -308,6 +319,7 @@ mod tests {
mcp_elicitations: false,
}),
&policy_workspace_only,
&FileSystemSandboxPolicy::from(&policy_workspace_only),
&cwd,
WindowsSandboxLevel::Disabled,
),
@@ -339,6 +351,7 @@ mod tests {
mcp_elicitations: false,
}),
&policy_workspace_only,
&FileSystemSandboxPolicy::from(&policy_workspace_only),
&cwd,
WindowsSandboxLevel::Disabled,
),
@@ -348,4 +361,47 @@ mod tests {
},
);
}
#[test]
fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let blocked_path = cwd.join("blocked.txt");
let blocked_absolute = AbsolutePathBuf::from_absolute_path(blocked_path.clone()).unwrap();
let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string());
let sandbox_policy = SandboxPolicy::ExternalSandbox {
network_access: codex_protocol::protocol::NetworkAccess::Restricted,
};
let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
access: FileSystemAccessMode::Write,
},
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: blocked_absolute,
},
access: FileSystemAccessMode::None,
},
]);
assert!(!is_write_patch_constrained_to_writable_paths(
&action,
&file_system_sandbox_policy,
&cwd,
));
assert_eq!(
assess_patch_safety(
&action,
AskForApproval::OnRequest,
&sandbox_policy,
&file_system_sandbox_policy,
&cwd,
WindowsSandboxLevel::Disabled,
),
SafetyCheck::AskUser,
);
}
}