mirror of
https://github.com/openai/codex.git
synced 2026-05-29 07:19:50 +00:00
fix(permissions): preserve managed deny-read during escalation (#15977)
## Why
Managed filesystem `deny_read` requirements are administrator-enforced
restrictions on specific paths. Once those requirements are active,
Codex should not drop them just because an execution path would
otherwise leave the sandbox.
Before this change, an explicit escalation, a prefix-rule allow, a
sandbox-denial retry, or an app-server legacy sandbox override could
rebuild the runtime policy without those managed read-deny entries and
expose a path the administrator had marked unreadable.
This is narrower than general sandbox-mode constraints. If an enterprise
only sets `allowed_sandbox_modes`, a trusted `prefix_rule(..., decision
= "allow")` can still run its matching command unsandboxed; this PR only
preserves managed filesystem `deny_read` restrictions across those
paths.
## What Changed
- Mark filesystem policies built from managed `deny_read` requirements
so callers can tell when those deny entries must survive escalation.
- Preserve managed deny-read entries when runtime permission profiles
are rebuilt through protocol, app-server, or legacy sandbox-policy
compatibility paths.
- Keep managed deny-read attempts inside the selected sandbox on the
first attempt and after sandbox-denial retries.
- Preserve the same behavior in the zsh-fork escalation path, including
prefix-rule-driven escalation.
- Add a regression test showing the opposite case too: without managed
deny-read, a prefix-rule allow still chooses unsandboxed execution.
## Verification
Targeted automated verification:
```shell
cargo test -p codex-core shell_request_escalation_execution_is_explicit -- --nocapture
cargo test -p codex-core prefix_rule_uses_unsandboxed_execution_without_managed_deny_read -- --nocapture
cargo test -p codex-core prefix_rule_preserves_managed_deny_read_escalation -- --nocapture
cargo test -p codex-protocol permission_profile_round_trip_preserves_filesystem_policy_metadata -- --nocapture
cargo test -p codex-protocol preserving_deny_entries_keeps_unrestricted_policy_enforceable -- --nocapture
cargo test -p codex-app-server-protocol permission_profile_file_system_permissions_preserves_policy_metadata -- --nocapture
cargo check -p codex-app-server -p codex-tui
```
Smoke-test invocations:
```shell
# macOS exact deny + allowed control
codex exec --skip-git-repo-check -C "$ROOT" \
-c 'default_permissions="deny_read_smoke"' \
-c 'permissions.deny_read_smoke.filesystem={":minimal"="read",":project_roots"={"."="write","secrets"="none","future-secret"="none","**/*.env"="none"}}' \
'Run shell commands only. Print the contents of allowed.txt. Then test whether reading secrets/exact-secret.txt succeeds without printing that file if it does. End with exactly two lines: allowed=<contents> and exact_secret=<BLOCKED or READABLE>.'
# Linux exact deny + allowed control
codex exec --skip-git-repo-check -C "$ROOT" \
-c 'default_permissions="deny_read_smoke"' \
-c 'permissions.deny_read_smoke.filesystem={":minimal"="read",glob_scan_max_depth=3,":project_roots"={"."="write","secrets"="none","future-secret"="none","**/*.env"="none"}}' \
'Run shell commands only. Print the contents of allowed.txt. Then test whether reading secrets/exact-secret.txt succeeds without printing that file if it does. End with exactly two lines: allowed=<contents> and exact_secret=<BLOCKED or READABLE>.'
```
Observed manual smoke matrix:
| Case | macOS Seatbelt | Linux bubblewrap |
| --- | --- | --- |
| `cat allowed.txt` | Pass | Pass |
| `cat secrets/exact-secret.txt` | Blocked | Blocked |
| `cat envs/root.env` | Blocked | Blocked |
| `cat envs/nested/one.env` | Blocked | Blocked |
| `cat envs/nested/two.env` | Blocked | Blocked |
| `cat alias-to-secrets/exact-secret.txt` | Blocked | Blocked |
| Missing denied path | A file created after sandbox setup remained
unreadable | Creation was blocked by the reserved missing-path
placeholder, and the placeholder was cleaned up after exit |
| Real `codex exec` shell turn | Pass | Pass |
Notes:
- The Linux smoke run used the fallback glob walker because the devbox
did not have `rg` installed.
- The smoke matrix verifies the end-to-end filesystem behavior on macOS
and Linux; the escalation-specific behavior is covered by the focused
tests above.
---------
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Charlie Marsh <charliemarsh@openai.com>
This commit is contained in:
@@ -1777,8 +1777,12 @@ async fn proposed_execpolicy_amendment_is_present_when_heuristics_allow() {
|
||||
async fn proposed_execpolicy_amendment_is_suppressed_when_policy_matches_allow() {
|
||||
assert_exec_approval_requirement_for_command(
|
||||
ExecApprovalRequirementScenario {
|
||||
policy_src: Some(r#"prefix_rule(pattern=["echo"], decision="allow")"#.to_string()),
|
||||
command: vec!["echo".to_string(), "safe".to_string()],
|
||||
policy_src: Some(r#"prefix_rule(pattern=["python3"], decision="allow")"#.to_string()),
|
||||
command: vec![
|
||||
"python3".to_string(),
|
||||
"-c".to_string(),
|
||||
"print(1)".to_string(),
|
||||
],
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
|
||||
|
||||
@@ -27,6 +27,7 @@ use crate::tools::sandboxing::ToolCtx;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use crate::tools::sandboxing::default_exec_approval_requirement;
|
||||
use crate::tools::sandboxing::sandbox_override_for_first_attempt;
|
||||
use codex_hooks::PermissionRequestDecision;
|
||||
use codex_otel::ToolDecisionSource;
|
||||
use codex_protocol::error::CodexErr;
|
||||
@@ -149,7 +150,7 @@ impl ToolOrchestrator {
|
||||
let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| {
|
||||
default_exec_approval_requirement(approval_policy, &file_system_sandbox_policy)
|
||||
});
|
||||
match requirement {
|
||||
match &requirement {
|
||||
ExecApprovalRequirement::Skip { .. } => {
|
||||
if strict_auto_review {
|
||||
let guardian_review_id = Some(new_guardian_review_id());
|
||||
@@ -184,7 +185,7 @@ impl ToolOrchestrator {
|
||||
}
|
||||
}
|
||||
ExecApprovalRequirement::Forbidden { reason } => {
|
||||
return Err(ToolError::Rejected(reason));
|
||||
return Err(ToolError::Rejected(reason.clone()));
|
||||
}
|
||||
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
let guardian_review_id = use_guardian.then(new_guardian_review_id);
|
||||
@@ -193,7 +194,7 @@ impl ToolOrchestrator {
|
||||
turn: &tool_ctx.turn,
|
||||
call_id: &tool_ctx.call_id,
|
||||
guardian_review_id: guardian_review_id.clone(),
|
||||
retry_reason: reason,
|
||||
retry_reason: reason.clone(),
|
||||
network_approval_context: None,
|
||||
};
|
||||
let decision = Self::request_approval(
|
||||
@@ -214,8 +215,13 @@ impl ToolOrchestrator {
|
||||
}
|
||||
|
||||
// 2) First attempt under the selected sandbox.
|
||||
let sandbox_override = sandbox_override_for_first_attempt(
|
||||
tool.sandbox_permissions(req),
|
||||
&requirement,
|
||||
&file_system_sandbox_policy,
|
||||
);
|
||||
let managed_network_active = turn_ctx.network.is_some();
|
||||
let initial_sandbox = match tool.sandbox_mode_for_first_attempt(req) {
|
||||
let initial_sandbox = match sandbox_override {
|
||||
SandboxOverride::BypassSandboxFirstAttempt => SandboxType::None,
|
||||
SandboxOverride::NoOverride => self.sandbox.select_initial(
|
||||
&file_system_sandbox_policy,
|
||||
|
||||
@@ -28,13 +28,11 @@ use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::PermissionRequestPayload;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
use crate::tools::sandboxing::Sandboxable;
|
||||
use crate::tools::sandboxing::ToolCtx;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use crate::tools::sandboxing::managed_network_for_sandbox_permissions;
|
||||
use crate::tools::sandboxing::sandbox_override_for_first_attempt;
|
||||
use crate::tools::sandboxing::with_cached_approval;
|
||||
use codex_network_proxy::NetworkProxy;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
@@ -210,8 +208,8 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
))
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride {
|
||||
sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement)
|
||||
fn sandbox_permissions(&self, req: &ShellRequest) -> SandboxPermissions {
|
||||
req.sandbox_permissions
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -26,13 +26,11 @@ use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::PermissionRequestPayload;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
use crate::tools::sandboxing::Sandboxable;
|
||||
use crate::tools::sandboxing::ToolCtx;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use crate::tools::sandboxing::managed_network_for_sandbox_permissions;
|
||||
use crate::tools::sandboxing::sandbox_override_for_first_attempt;
|
||||
use crate::tools::sandboxing::with_cached_approval;
|
||||
use crate::unified_exec::NoopSpawnLifecycle;
|
||||
use crate::unified_exec::UnifiedExecError;
|
||||
@@ -212,8 +210,8 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
))
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride {
|
||||
sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement)
|
||||
fn sandbox_permissions(&self, req: &UnifiedExecRequest) -> SandboxPermissions {
|
||||
req.sandbox_permissions
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -248,18 +248,27 @@ 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 {
|
||||
// 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()
|
||||
|| matches!(
|
||||
exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
..
|
||||
}
|
||||
)
|
||||
{
|
||||
if matches!(
|
||||
exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
..
|
||||
}
|
||||
) {
|
||||
return SandboxOverride::BypassSandboxFirstAttempt;
|
||||
}
|
||||
|
||||
// Deny-read restrictions suppress explicit escalation because that path
|
||||
// would otherwise discard the filesystem policy entirely.
|
||||
if file_system_sandbox_policy.has_denied_read_restrictions() {
|
||||
return SandboxOverride::NoOverride;
|
||||
}
|
||||
|
||||
if sandbox_permissions.requires_escalated_permissions() {
|
||||
SandboxOverride::BypassSandboxFirstAttempt
|
||||
} else {
|
||||
SandboxOverride::NoOverride
|
||||
@@ -289,11 +298,10 @@ pub(crate) trait Approvable<Req> {
|
||||
// requests touching a subset can be auto-approved.
|
||||
fn approval_keys(&self, req: &Req) -> Vec<Self::ApprovalKey>;
|
||||
|
||||
/// 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 {
|
||||
SandboxOverride::NoOverride
|
||||
/// Return per-request sandbox permissions for first-attempt sandbox
|
||||
/// selection. Most tools use the ambient sandbox policy unchanged.
|
||||
fn sandbox_permissions(&self, _req: &Req) -> SandboxPermissions {
|
||||
SandboxPermissions::UseDefault
|
||||
}
|
||||
|
||||
fn should_bypass_approval(&self, policy: AskForApproval, already_approved: bool) -> bool {
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
use super::*;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::hook_names::HookToolName;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use codex_protocol::protocol::NetworkAccess;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -120,6 +123,7 @@ fn additional_permissions_allow_bypass_sandbox_first_attempt_when_execpolicy_ski
|
||||
bypass_sandbox: true,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
&FileSystemSandboxPolicy::default(),
|
||||
),
|
||||
SandboxOverride::BypassSandboxFirstAttempt
|
||||
);
|
||||
@@ -134,7 +138,43 @@ fn guardian_bypasses_sandbox_for_explicit_escalation_on_first_attempt() {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
&FileSystemSandboxPolicy::default(),
|
||||
),
|
||||
SandboxOverride::BypassSandboxFirstAttempt
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn deny_read_blocks_explicit_escalation_but_preserves_policy_bypass() {
|
||||
let file_system_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::GlobPattern {
|
||||
pattern: "**/*.env".to_string(),
|
||||
},
|
||||
access: FileSystemAccessMode::None,
|
||||
}]);
|
||||
|
||||
assert_eq!(
|
||||
sandbox_override_for_first_attempt(
|
||||
SandboxPermissions::RequireEscalated,
|
||||
&ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
&file_system_policy,
|
||||
),
|
||||
SandboxOverride::NoOverride,
|
||||
"explicit escalation would drop deny-read filesystem policy, so keep the first attempt sandboxed",
|
||||
);
|
||||
assert_eq!(
|
||||
sandbox_override_for_first_attempt(
|
||||
SandboxPermissions::WithAdditionalPermissions,
|
||||
&ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
&file_system_policy,
|
||||
),
|
||||
SandboxOverride::BypassSandboxFirstAttempt,
|
||||
"exec-policy allow rules intentionally bypass sandbox even when deny-read entries exist",
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user