valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: tighten up checks against writable folders for SandboxPolicy (#2338)

Browse Source 
I was looking at the implementation of `Session::get_writable_roots()`,
which did not seem right, as it was a copy of writable roots, which is
not guaranteed to be in sync with the `sandbox_policy` field.

I looked at who was calling `get_writable_roots()` and its only call
site was `apply_patch()` in `codex-rs/core/src/apply_patch.rs`, which
took the roots and forwarded them to `assess_patch_safety()` in
`safety.rs`. I updated `assess_patch_safety()` to take `sandbox_policy:
&SandboxPolicy` instead of `writable_roots: &[PathBuf]` (and replaced
`Session::get_writable_roots()` with `Session::get_sandbox_policy()`).

Within `safety.rs`, it was fairly easy to update
`is_write_patch_constrained_to_writable_paths()` to work with
`SandboxPolicy`, and in particular, it is far more accurate because, for
better or worse, `SandboxPolicy::get_writable_roots_with_cwd()` _returns
an empty vec_ for `SandboxPolicy::DangerFullAccess`, suggesting that
_nothing_ is writable when in reality _everything_ is writable. With
this PR, `is_write_patch_constrained_to_writable_paths()` now does the
right thing for each variant of `SandboxPolicy`.

I thought this would be the end of the story, but it turned out that
`test_writable_roots_constraint()` in `safety.rs` needed to be updated,
as well. In particular, the test was writing to
`std::env::current_dir()` instead of a `TempDir`, which I suspect was a
holdover from earlier when `SandboxPolicy::WorkspaceWrite` would always
make `TMPDIR` writable on macOS, which made it hard to write tests to
verify `SandboxPolicy` in `TMPDIR`. Fortunately, we now have
`exclude_tmpdir_env_var` as an option on
`SandboxPolicy::WorkspaceWrite`, so I was able to update the test to
preserve the existing behavior, but to no longer write to
`std::env::current_dir()`.







---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2338).
* #2345
* #2329
* #2343
* #2340
* __->__ #2338
This commit is contained in:
Michael Bolin
2025-08-15 09:06:15 -07:00
committed by GitHub
parent 6df8e35314
commit 26c8373821
4 changed files with 70 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -8,7 +8,6 @@ use crate::safety::assess_patch_safety;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchFileChange;
use std::collections::HashMap;
use std::path::Path;
use std::path::PathBuf;
pub const CODEX_APPLY_PATCH_ARG1: &str = "--codex-run-as-apply-patch";
@@ -45,12 +44,10 @@ pub(crate) async fn apply_patch(
call_id: &str,
action: ApplyPatchAction,
) -> InternalApplyPatchInvocation {
let writable_roots_snapshot = sess.get_writable_roots().to_vec();
match assess_patch_safety(
&action,
sess.get_approval_policy(),
&writable_roots_snapshot,
sess.get_sandbox_policy(),
sess.get_cwd(),
) {
SafetyCheck::AutoApprove { .. } => {
@@ -124,30 +121,3 @@ pub(crate) fn convert_apply_patch_to_protocol(
}
result
}
pub(crate) fn get_writable_roots(cwd: &Path) -> Vec<PathBuf> {
let mut writable_roots = Vec::new();
if cfg!(target_os = "macos") {
// On macOS, $TMPDIR is private to the user.
writable_roots.push(std::env::temp_dir());
// Allow pyenv to update its shims directory. Without this, any tool
// that happens to be managed by `pyenv` will fail with an error like:
//
// pyenv: cannot rehash: $HOME/.pyenv/shims isn't writable
//
// which is emitted every time `pyenv` tries to run `rehash` (for
// example, after installing a new Python package that drops an entry
// point). Although the sandbox is intentionally read‑only by default,
// writing to the user's local `pyenv` directory is safe because it
// is already user‑writable and scoped to the current user account.
if let Ok(home_dir) = std::env::var("HOME") {
let pyenv_dir = PathBuf::from(home_dir).join(".pyenv");
writable_roots.push(pyenv_dir);
}
}
writable_roots.push(cwd.to_path_buf());
writable_roots
}

15
codex-rs/core/src/codex.rs
View File

@@ -31,12 +31,11 @@ use tracing::warn;
use uuid::Uuid;
use crate::ModelProviderInfo;
use crate::apply_patch;
use crate::apply_patch::ApplyPatchExec;
use crate::apply_patch::CODEX_APPLY_PATCH_ARG1;
use crate::apply_patch::InternalApplyPatchInvocation;
use crate::apply_patch::convert_apply_patch_to_protocol;
use crate::apply_patch::get_writable_roots;
use crate::apply_patch::{self};
use crate::client::ModelClient;
use crate::client_common::Prompt;
use crate::client_common::ResponseEvent;
@@ -231,7 +230,6 @@ pub(crate) struct Session {
approval_policy: AskForApproval,
sandbox_policy: SandboxPolicy,
shell_environment_policy: ShellEnvironmentPolicy,
writable_roots: Vec<PathBuf>,
disable_response_storage: bool,
tools_config: ToolsConfig,
@@ -410,8 +408,6 @@ impl Session {
state.history.record_items(&restored_items);
}
let writable_roots = get_writable_roots(&cwd);
// Handle MCP manager result and record any startup failures.
let (mcp_connection_manager, failed_clients) = match mcp_res {
Ok((mgr, failures)) => (mgr, failures),
@@ -465,7 +461,6 @@ impl Session {
sandbox_policy,
shell_environment_policy: config.shell_environment_policy.clone(),
cwd,
writable_roots,
mcp_connection_manager,
notify,
state: Mutex::new(state),
@@ -509,14 +504,14 @@ impl Session {
Ok(sess)
}
pub(crate) fn get_writable_roots(&self) -> &[PathBuf] {
&self.writable_roots
}
pub(crate) fn get_approval_policy(&self) -> AskForApproval {
self.approval_policy
}
pub(crate) fn get_sandbox_policy(&self) -> &SandboxPolicy {
&self.sandbox_policy
}
pub(crate) fn get_cwd(&self) -> &Path {
&self.cwd
}

21
codex-rs/core/src/protocol.rs
View File

@@ -156,10 +156,31 @@ pub enum SandboxPolicy {
/// not modified by the agent.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct WritableRoot {
/// Absolute path, by construction.
pub root: PathBuf,
/// Also absolute paths, by construction.
pub read_only_subpaths: Vec<PathBuf>,
}
impl WritableRoot {
pub(crate) fn is_path_writable(&self, path: &Path) -> bool {
// Check if the path is under the root.
if !path.starts_with(&self.root) {
return false;
}
// Check if the path is under any of the read-only subpaths.
for subpath in &self.read_only_subpaths {
if path.starts_with(subpath) {
return false;
}
}
true
}
}
impl FromStr for SandboxPolicy {
type Err = serde_json::Error;

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

@@ -21,7 +21,7 @@ pub enum SafetyCheck {
pub fn assess_patch_safety(
action: &ApplyPatchAction,
policy: AskForApproval,
writable_roots: &[PathBuf],
sandbox_policy: &SandboxPolicy,
cwd: &Path,
) -> SafetyCheck {
if action.is_empty() {
@@ -45,7 +45,7 @@ pub fn assess_patch_safety(
// 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, writable_roots, cwd)
if is_write_patch_constrained_to_writable_paths(action, sandbox_policy, cwd)
|| policy == AskForApproval::OnFailure
{
// Only auto‑approve when we can actually enforce a sandbox. Otherwise
@@ -171,13 +171,19 @@ pub fn get_platform_sandbox() -> Option<SandboxType> {
fn is_write_patch_constrained_to_writable_paths(
action: &ApplyPatchAction,
writable_roots: &[PathBuf],
sandbox_policy: &SandboxPolicy,
cwd: &Path,
) -> bool {
// Early‑exit if there are no declared writable roots.
if writable_roots.is_empty() {
return false;
}
let writable_roots = match sandbox_policy {
SandboxPolicy::ReadOnly => {
return false;
}
SandboxPolicy::DangerFullAccess => {
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).
@@ -209,15 +215,9 @@ fn is_write_patch_constrained_to_writable_paths(
None => return false,
};
writable_roots.iter().any(|root| {
let root_abs = if root.is_absolute() {
root.clone()
} else {
normalize(&cwd.join(root)).unwrap_or_else(|| cwd.join(root))
};
abs.starts_with(&root_abs)
})
writable_roots
.iter()
.any(|writable_root| writable_root.is_path_writable(&abs))
};
for (path, change) in action.changes() {
@@ -246,38 +246,56 @@ fn is_write_patch_constrained_to_writable_paths(
#[cfg(test)]
mod tests {
use super::*;
use tempfile::TempDir;
#[test]
fn test_writable_roots_constraint() {
let cwd = std::env::current_dir().unwrap();
// Use a temporary directory as our workspace to avoid touching
// the real current working directory.
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let parent = cwd.parent().unwrap().to_path_buf();
// Helper to build a single‑entry map representing a patch that adds a
// file at `p`.
// Helper to build a single‑entry patch that adds a file at `p`.
let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string());
let add_inside = make_add_change(cwd.join("inner.txt"));
let add_outside = make_add_change(parent.join("outside.txt"));
// Policy limited to the workspace only; exclude system temp roots so
// only `cwd` is writable by default.
let policy_workspace_only = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
network_access: false,
exclude_tmpdir_env_var: true,
exclude_slash_tmp: true,
};
assert!(is_write_patch_constrained_to_writable_paths(
&add_inside,
&[PathBuf::from(".")],
&policy_workspace_only,
&cwd,
));
let add_outside_2 = make_add_change(parent.join("outside.txt"));
assert!(!is_write_patch_constrained_to_writable_paths(
&add_outside_2,
&[PathBuf::from(".")],
&add_outside,
&policy_workspace_only,
&cwd,
));
// With parent dir added as writable root, it should pass.
// With the parent dir explicitly added as a writable root, the
// outside write should be permitted.
let policy_with_parent = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![parent.clone()],
network_access: false,
exclude_tmpdir_env_var: true,
exclude_slash_tmp: true,
};
assert!(is_write_patch_constrained_to_writable_paths(
&add_outside,
&[PathBuf::from("..")],
&policy_with_parent,
&cwd,
))
));
}
#[test]