Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion codex-rs/core/src/codex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1250,7 +1250,14 @@ impl SessionConfiguration {

let cwd_changed = absolute_cwd.as_path() != self.cwd.as_path();
next_configuration.cwd = absolute_cwd;
if sandbox_policy_changed || (cwd_changed && file_system_policy_matches_legacy) {
if sandbox_policy_changed {
next_configuration.file_system_sandbox_policy =
FileSystemSandboxPolicy::from_legacy_sandbox_policy_preserving_read_denies(
next_configuration.sandbox_policy.get(),
&next_configuration.cwd,
&next_configuration.file_system_sandbox_policy,
);
} else if cwd_changed && file_system_policy_matches_legacy {
// Preserve richer split policies across cwd-only updates; only
// rederive when the session is already using the legacy bridge.
next_configuration.file_system_sandbox_policy =
Expand Down
10 changes: 6 additions & 4 deletions codex-rs/core/src/exec_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,12 @@ impl ExecPolicyManager {
}
}
Decision::Allow => ExecApprovalRequirement::Skip {
// Bypass sandbox if execpolicy allows the command
bypass_sandbox: evaluation.matched_rules.iter().any(|rule_match| {
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
}),
// Keep sandboxing in place when the filesystem policy
// contains explicit deny-read carveouts.
bypass_sandbox: !file_system_sandbox_policy.has_denied_read_restrictions()
&& evaluation.matched_rules.iter().any(|rule_match| {
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
}),
proposed_execpolicy_amendment: if auto_amendment_allowed {
try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules)
} else {
Expand Down
31 changes: 31 additions & 0 deletions codex-rs/core/src/exec_policy_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1644,6 +1644,37 @@ async fn dangerous_command_forbidden_in_external_sandbox_when_policy_matches() {
.await;
}

#[tokio::test]
async fn allow_rule_does_not_bypass_sandbox_when_deny_read_paths_exist() {
let policy_src = Some(r#"prefix_rule(pattern=["cat"], decision="allow")"#.to_string());
let command = vec_str(&["cat", "~/.gitconfig"]);
let deny_path = AbsolutePathBuf::try_from("/tmp/secret-config").expect("absolute path");
let file_system_sandbox_policy =
codex_protocol::permissions::FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry {
path: FileSystemPath::Path { path: deny_path },
access: FileSystemAccessMode::None,
},
]);

assert_exec_approval_requirement_for_command(
ExecApprovalRequirementScenario {
policy_src,
command,
approval_policy: AskForApproval::OnRequest,
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
file_system_sandbox_policy,
sandbox_permissions: SandboxPermissions::UseDefault,
prefix_rule: None,
},
ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: None,
},
)
.await;
}

struct ExecApprovalRequirementScenario {
/// Source for the Starlark `.rules` file.
policy_src: Option<String>,
Expand Down
44 changes: 40 additions & 4 deletions codex-rs/core/src/tools/handlers/list_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::fs::FileType;
use std::path::Path;
use std::path::PathBuf;

use codex_protocol::permissions::FileSystemReadDenyMatcher;
use codex_utils_string::take_bytes_at_char_boundary;
use serde::Deserialize;
use tokio::fs;
Expand All @@ -20,6 +21,8 @@ pub struct ListDirHandler;

const MAX_ENTRY_LENGTH: usize = 500;
const INDENTATION_SPACES: usize = 2;
const DENY_READ_POLICY_MESSAGE: &str =
"access denied: reading this path is blocked by filesystem deny_read policy";

fn default_offset() -> usize {
1
Expand Down Expand Up @@ -52,7 +55,7 @@ impl ToolHandler for ListDirHandler {
}

async fn handle(&self, invocation: ToolInvocation) -> Result<Self::Output, FunctionCallError> {
let ToolInvocation { payload, .. } = invocation;
let ToolInvocation { payload, turn, .. } = invocation;

let arguments = match payload {
ToolPayload::Function { arguments } => arguments,
Expand Down Expand Up @@ -96,23 +99,48 @@ impl ToolHandler for ListDirHandler {
"dir_path must be an absolute path".to_string(),
));
}
let read_deny_matcher = turn
.file_system_sandbox_policy
.read_deny_matcher_with_cwd(&turn.cwd);
if read_deny_matcher
.as_ref()
.is_some_and(|matcher| matcher.is_read_denied(&path))
{
return Err(FunctionCallError::RespondToModel(format!(
"{DENY_READ_POLICY_MESSAGE}: `{}`",
path.display()
)));
}

let entries = list_dir_slice(&path, offset, limit, depth).await?;
let entries =
list_dir_slice_with_policy(&path, offset, limit, depth, read_deny_matcher.as_ref())
.await?;
let mut output = Vec::with_capacity(entries.len() + 1);
output.push(format!("Absolute path: {}", path.display()));
output.extend(entries);
Ok(FunctionToolOutput::from_text(output.join("\n"), Some(true)))
}
}

#[cfg(test)]
async fn list_dir_slice(
path: &Path,
offset: usize,
limit: usize,
depth: usize,
) -> Result<Vec<String>, FunctionCallError> {
list_dir_slice_with_policy(path, offset, limit, depth, /*read_deny_matcher*/ None).await
}

async fn list_dir_slice_with_policy(
path: &Path,
offset: usize,
limit: usize,
depth: usize,
read_deny_matcher: Option<&FileSystemReadDenyMatcher>,
) -> Result<Vec<String>, FunctionCallError> {
let mut entries = Vec::new();
collect_entries(path, Path::new(""), depth, &mut entries).await?;
collect_entries(path, Path::new(""), depth, read_deny_matcher, &mut entries).await?;

if entries.is_empty() {
return Ok(Vec::new());
Expand Down Expand Up @@ -148,6 +176,7 @@ async fn collect_entries(
dir_path: &Path,
relative_prefix: &Path,
depth: usize,
read_deny_matcher: Option<&FileSystemReadDenyMatcher>,
entries: &mut Vec<DirEntry>,
) -> Result<(), FunctionCallError> {
let mut queue = VecDeque::new();
Expand All @@ -163,6 +192,13 @@ async fn collect_entries(
while let Some(entry) = read_dir.next_entry().await.map_err(|err| {
FunctionCallError::RespondToModel(format!("failed to read directory: {err}"))
})? {
let entry_path = entry.path();
if let Some(read_deny_matcher) = read_deny_matcher
&& read_deny_matcher.is_read_denied(&entry_path)
{
continue;
}

let file_type = entry.file_type().await.map_err(|err| {
FunctionCallError::RespondToModel(format!("failed to inspect entry: {err}"))
})?;
Expand All @@ -179,7 +215,7 @@ async fn collect_entries(
let sort_key = format_entry_name(&relative_path);
let kind = DirEntryKind::from(&file_type);
dir_entries.push((
entry.path(),
entry_path,
relative_path,
kind,
DirEntry {
Expand Down
61 changes: 61 additions & 0 deletions codex-rs/core/src/tools/handlers/list_dir_tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
use super::*;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use pretty_assertions::assert_eq;
use tempfile::tempdir;

Expand Down Expand Up @@ -258,3 +262,60 @@ async fn truncation_respects_sorted_order() -> anyhow::Result<()> {

Ok(())
}

#[tokio::test]
async fn hides_denied_entries_and_prunes_denied_subtrees() {
let temp = tempdir().expect("create tempdir");
let dir_path = temp.path();
let visible_dir = dir_path.join("visible");
let denied_dir = dir_path.join("private");
tokio::fs::create_dir(&visible_dir)
.await
.expect("create visible dir");
tokio::fs::create_dir(&denied_dir)
.await
.expect("create denied dir");
tokio::fs::write(visible_dir.join("ok.txt"), b"ok")
.await
.expect("write visible file");
tokio::fs::write(denied_dir.join("secret.txt"), b"secret")
.await
.expect("write denied file");
tokio::fs::write(dir_path.join("top_secret.txt"), b"secret")
.await
.expect("write denied top-level file");

let policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: denied_dir.try_into().expect("absolute denied dir"),
},
access: FileSystemAccessMode::None,
},
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: dir_path
.join("top_secret.txt")
.try_into()
.expect("absolute denied file"),
},
access: FileSystemAccessMode::None,
},
]);

let read_deny_matcher = policy.read_deny_matcher_with_cwd(dir_path);
let entries = list_dir_slice_with_policy(
dir_path,
/*offset*/ 1,
/*limit*/ 20,
/*depth*/ 3,
read_deny_matcher.as_ref(),
)
.await
.expect("list directory");

assert_eq!(
entries,
vec!["visible/".to_string(), " ok.txt".to_string(),]
);
}
1 change: 0 additions & 1 deletion codex-rs/core/src/tools/handlers/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,6 @@ impl ShellHandler {
"approval policy is {approval_policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {approval_policy:?}"
)));
}

// Intercept apply_patch if present.
if let Some(output) = intercept_apply_patch(
&exec_params.command,
Expand Down
1 change: 0 additions & 1 deletion codex-rs/core/src/tools/handlers/unified_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,6 @@ impl ToolHandler for UnifiedExecHandler {
"approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}"
)));
}

let workdir = workdir.filter(|value| !value.is_empty());

let workdir = workdir.map(|dir| context.turn.resolve_path(Some(dir)));
Expand Down
30 changes: 20 additions & 10 deletions codex-rs/core/src/tools/orchestrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,16 +182,17 @@ impl ToolOrchestrator {
.requirements_toml()
.network
.is_some();
let initial_sandbox = match tool.sandbox_mode_for_first_attempt(req) {
SandboxOverride::BypassSandboxFirstAttempt => SandboxType::None,
SandboxOverride::NoOverride => self.sandbox.select_initial(
&turn_ctx.file_system_sandbox_policy,
turn_ctx.network_sandbox_policy,
tool.sandbox_preference(),
turn_ctx.windows_sandbox_level,
has_managed_network_requirements,
),
};
let initial_sandbox =
match tool.sandbox_mode_for_first_attempt(req, &turn_ctx.file_system_sandbox_policy) {
SandboxOverride::BypassSandboxFirstAttempt => SandboxType::None,
SandboxOverride::NoOverride => self.sandbox.select_initial(
&turn_ctx.file_system_sandbox_policy,
turn_ctx.network_sandbox_policy,
tool.sandbox_preference(),
turn_ctx.windows_sandbox_level,
has_managed_network_requirements,
),
};

// Platform-specific flag gating is handled by SandboxManager::select_initial.
let use_legacy_landlock = turn_ctx.features.use_legacy_landlock();
Expand Down Expand Up @@ -245,6 +246,15 @@ impl ToolOrchestrator {
network_policy_decision,
})));
}
if turn_ctx
.file_system_sandbox_policy
.has_denied_read_restrictions()
{
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
output,
network_policy_decision,
})));
}
if !tool.escalate_on_failure() {
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
output,
Expand Down
13 changes: 11 additions & 2 deletions codex-rs/core/src/tools/runtimes/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use crate::tools::sandboxing::with_cached_approval;
use codex_network_proxy::NetworkProxy;
use codex_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxablePreference;
use codex_shell_command::powershell::prefix_powershell_script_with_utf8;
Expand Down Expand Up @@ -197,8 +198,16 @@ impl Approvable<ShellRequest> for ShellRuntime {
Some(req.exec_approval_requirement.clone())
}

fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride {
sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement)
fn sandbox_mode_for_first_attempt(
&self,
req: &ShellRequest,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
) -> SandboxOverride {
sandbox_override_for_first_attempt(
req.sandbox_permissions,
&req.exec_approval_requirement,
file_system_sandbox_policy,
)
}
}

Expand Down
13 changes: 11 additions & 2 deletions codex-rs/core/src/tools/runtimes/unified_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use codex_network_proxy::NetworkProxy;
use codex_protocol::error::CodexErr;
use codex_protocol::error::SandboxErr;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxablePreference;
use codex_shell_command::powershell::prefix_powershell_script_with_utf8;
Expand Down Expand Up @@ -177,8 +178,16 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
Some(req.exec_approval_requirement.clone())
}

fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride {
sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement)
fn sandbox_mode_for_first_attempt(
&self,
req: &UnifiedExecRequest,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
) -> SandboxOverride {
sandbox_override_for_first_attempt(
req.sandbox_permissions,
&req.exec_approval_requirement,
file_system_sandbox_policy,
)
}
}

Expand Down
13 changes: 12 additions & 1 deletion codex-rs/core/src/tools/sandboxing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,14 @@ pub(crate) enum SandboxOverride {
pub(crate) fn sandbox_override_for_first_attempt(
sandbox_permissions: SandboxPermissions,
exec_approval_requirement: &ExecApprovalRequirement,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
) -> SandboxOverride {
// Approval may widen execution, but it must not silently discard explicit
// read-deny carveouts by running the first attempt outside the sandbox.
if file_system_sandbox_policy.has_denied_read_restrictions() {
return SandboxOverride::NoOverride;
}

// ExecPolicy `Allow` can intentionally imply full trust (Skip + bypass_sandbox=true),
// which supersedes `with_additional_permissions` sandboxed execution hints.
if sandbox_permissions.requires_escalated_permissions()
Expand Down Expand Up @@ -255,7 +262,11 @@ pub(crate) trait Approvable<Req> {
/// Some tools may request to skip the sandbox on the first attempt
/// (e.g., when the request explicitly asks for escalated permissions).
/// Defaults to `NoOverride`.
fn sandbox_mode_for_first_attempt(&self, _req: &Req) -> SandboxOverride {
fn sandbox_mode_for_first_attempt(
&self,
_req: &Req,
_file_system_sandbox_policy: &FileSystemSandboxPolicy,
) -> SandboxOverride {
SandboxOverride::NoOverride
}

Expand Down
Loading
Loading