mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
Preserve managed deny-read rules across permission updates (#40004)
## Why Runtime permission updates must not weaken managed filesystem `deny_read` requirements. ## What changed - Retain managed deny-read rules separately and merge them into updated permission profiles. - Reject permission profiles and legacy sandbox policies that conflict with a managed denied path. - Apply the same constraint when `command/exec` handles a request-specific sandbox policy. ## Testing - Cover thread permission updates with managed deny-read requirements. - Cover `command/exec` enforcement for managed and user-defined denies, including conflicting policy and profile overrides. GitOrigin-RevId: 5e387b9c1bf1650a21753a74a3338bd33df7d0ce
This commit is contained in:
@@ -238,26 +238,12 @@ impl CommandExecRequestProcessor {
|
||||
config.effective_workspace_roots(),
|
||||
)
|
||||
} else if let Some(policy) = sandbox_policy.map(|policy| policy.to_core()) {
|
||||
self.config
|
||||
.permissions
|
||||
.can_set_legacy_sandbox_policy(&policy, &sandbox_cwd)
|
||||
.map_err(|err| invalid_request(format!("invalid sandbox policy: {err}")))?;
|
||||
let file_system_sandbox_policy =
|
||||
codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, &sandbox_cwd);
|
||||
let network_sandbox_policy =
|
||||
codex_protocol::permissions::NetworkSandboxPolicy::from(&policy);
|
||||
let permission_profile =
|
||||
codex_protocol::models::PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
codex_protocol::models::SandboxEnforcement::from_legacy_sandbox_policy(&policy),
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
);
|
||||
self.config
|
||||
.permissions
|
||||
.can_set_permission_profile(&permission_profile)
|
||||
let mut permissions = self.config.permissions.clone();
|
||||
permissions
|
||||
.set_legacy_sandbox_policy(policy, &sandbox_cwd)
|
||||
.map_err(|err| invalid_request(format!("invalid sandbox policy: {err}")))?;
|
||||
(
|
||||
permission_profile,
|
||||
permissions.effective_permission_profile(),
|
||||
self.config.permissions.network.clone(),
|
||||
self.config.permissions.permission_profile().clone(),
|
||||
self.config.managed_network_requirements_enabled(),
|
||||
|
||||
@@ -330,6 +330,119 @@ async fn command_exec_accepts_permission_profile() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn command_exec_enforces_managed_deny_read_requirements() -> Result<()> {
|
||||
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri(), "never")?;
|
||||
let denied_root = codex_home.path().join("private");
|
||||
let nested_root = denied_root.join("nested");
|
||||
std::fs::create_dir_all(&nested_root)?;
|
||||
let denied_path = nested_root.join("secret.txt");
|
||||
std::fs::write(&denied_path, "managed secret")?;
|
||||
let user_denied_root = codex_home.path().join("user-private");
|
||||
std::fs::create_dir_all(&user_denied_root)?;
|
||||
let user_denied_path = user_denied_root.join("secret.txt");
|
||||
std::fs::write(&user_denied_path, "user secret")?;
|
||||
std::fs::write(
|
||||
codex_home.path().join("requirements.toml"),
|
||||
format!("[permissions.filesystem]\ndeny_read = [{denied_root:?}]\n"),
|
||||
)?;
|
||||
insert_command_exec_config(
|
||||
codex_home.path(),
|
||||
&format!(
|
||||
"default_permissions = \"thread-policy\"\n\n[permissions.thread-policy.filesystem]\n\":root\" = \"read\"\n{user_denied_root:?} = \"deny\"\n\n[permissions.nested.filesystem]\n\":root\" = \"read\"\n{nested_root:?} = \"write\"\n"
|
||||
),
|
||||
)?;
|
||||
let mut app_server = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.build_initialized_with_timeout(DEFAULT_READ_TIMEOUT)
|
||||
.await?;
|
||||
|
||||
let request = CommandExecParams {
|
||||
command: vec![
|
||||
"sh".to_string(),
|
||||
"-c".to_string(),
|
||||
"cat \"$1\"".to_string(),
|
||||
"sh".to_string(),
|
||||
denied_path.to_string_lossy().into_owned(),
|
||||
],
|
||||
process_id: None,
|
||||
tty: false,
|
||||
stream_stdin: false,
|
||||
stream_stdout_stderr: false,
|
||||
output_bytes_cap: None,
|
||||
disable_output_cap: false,
|
||||
disable_timeout: false,
|
||||
timeout_ms: None,
|
||||
cwd: Some(codex_home.path().to_path_buf()),
|
||||
env: None,
|
||||
size: None,
|
||||
sandbox_policy: Some(SandboxPolicy::ReadOnly {
|
||||
network_access: false,
|
||||
}),
|
||||
permission_profile: None,
|
||||
};
|
||||
let request_id = app_server
|
||||
.send_command_exec_request(request.clone())
|
||||
.await?;
|
||||
let response: CommandExecResponse = app_server.read_response(request_id).await?;
|
||||
assert_ne!(response.exit_code, 0);
|
||||
assert!(!response.stdout.contains("managed secret"));
|
||||
|
||||
let mut user_request = request.clone();
|
||||
*user_request
|
||||
.command
|
||||
.last_mut()
|
||||
.expect("cat command includes a file path") = user_denied_path.to_string_lossy().into();
|
||||
let request_id = app_server.send_command_exec_request(user_request).await?;
|
||||
let response: CommandExecResponse = app_server.read_response(request_id).await?;
|
||||
assert_eq!(
|
||||
response,
|
||||
CommandExecResponse {
|
||||
exit_code: 0,
|
||||
stdout: "user secret".to_string(),
|
||||
stderr: String::new(),
|
||||
}
|
||||
);
|
||||
|
||||
for sandbox_policy in [
|
||||
SandboxPolicy::DangerFullAccess,
|
||||
SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![nested_root.try_into()?],
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
},
|
||||
] {
|
||||
let request_id = app_server
|
||||
.send_command_exec_request(CommandExecParams {
|
||||
sandbox_policy: Some(sandbox_policy),
|
||||
..request.clone()
|
||||
})
|
||||
.await?;
|
||||
let error = app_server
|
||||
.read_stream_until_error_message(RequestId::Integer(request_id))
|
||||
.await?;
|
||||
assert!(error.error.message.contains("invalid sandbox policy"));
|
||||
}
|
||||
|
||||
let request_id = app_server
|
||||
.send_command_exec_request(CommandExecParams {
|
||||
sandbox_policy: None,
|
||||
permission_profile: Some("nested".to_string()),
|
||||
..request
|
||||
})
|
||||
.await?;
|
||||
let error = app_server
|
||||
.read_stream_until_error_message(RequestId::Integer(request_id))
|
||||
.await?;
|
||||
assert!(error.error.message.contains("invalid permission profile"));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn command_exec_permission_profile_starts_selected_network_proxy() -> Result<()> {
|
||||
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
||||
|
||||
@@ -114,8 +114,10 @@ use codex_protocol::models::SandboxEnforcement;
|
||||
use codex_protocol::openai_models::ModelMessages;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::permissions::ReadDenyMatcher;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::MultiAgentVersion;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
@@ -297,6 +299,8 @@ pub struct Permissions {
|
||||
/// Constrained permission profile plus its selected profile identity, if
|
||||
/// the profile came from a built-in or named config profile.
|
||||
permission_profile_state: PermissionProfileState,
|
||||
/// Managed deny-read rules retained independently from user-defined denies.
|
||||
managed_deny_read_policy: Option<Arc<FileSystemSandboxPolicy>>,
|
||||
/// Thread-scoped runtime workspace roots. Symbolic `:workspace_roots`
|
||||
/// entries in the permission profile are materialized against these roots.
|
||||
workspace_roots: Vec<AbsolutePathBuf>,
|
||||
@@ -332,6 +336,7 @@ impl Permissions {
|
||||
permission_profile_state: PermissionProfileState::from_constrained_legacy(
|
||||
permission_profile,
|
||||
)?,
|
||||
managed_deny_read_policy: None,
|
||||
workspace_roots: Vec::new(),
|
||||
network: None,
|
||||
allow_login_shell: true,
|
||||
@@ -388,8 +393,10 @@ impl Permissions {
|
||||
&self,
|
||||
permission_profile: &PermissionProfile,
|
||||
) -> ConstraintResult<()> {
|
||||
let permission_profile =
|
||||
self.permission_profile_preserving_managed_denied_reads(permission_profile.clone());
|
||||
self.permission_profile_state
|
||||
.can_set_legacy_permission_profile(permission_profile)
|
||||
.can_set_legacy_permission_profile(&permission_profile)
|
||||
}
|
||||
|
||||
pub fn set_workspace_roots(&mut self, workspace_roots: Vec<AbsolutePathBuf>) {
|
||||
@@ -455,8 +462,7 @@ impl Permissions {
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
);
|
||||
self.permission_profile_state
|
||||
.can_set_legacy_permission_profile(&permission_profile)
|
||||
self.can_set_permission_profile(&permission_profile)
|
||||
}
|
||||
|
||||
/// Set permissions from a legacy sandbox policy and keep every permission
|
||||
@@ -496,9 +502,7 @@ impl Permissions {
|
||||
],
|
||||
};
|
||||
|
||||
self.permission_profile_state
|
||||
.set_legacy_permission_profile(permission_profile)?;
|
||||
Ok(())
|
||||
self.set_permission_profile(permission_profile)
|
||||
}
|
||||
|
||||
/// Set permissions from the canonical profile.
|
||||
@@ -506,9 +510,34 @@ impl Permissions {
|
||||
&mut self,
|
||||
permission_profile: PermissionProfile,
|
||||
) -> ConstraintResult<()> {
|
||||
let permission_profile =
|
||||
self.permission_profile_preserving_managed_denied_reads(permission_profile);
|
||||
self.permission_profile_state
|
||||
.set_legacy_permission_profile(permission_profile)
|
||||
}
|
||||
|
||||
fn permission_profile_preserving_managed_denied_reads(
|
||||
&self,
|
||||
permission_profile: PermissionProfile,
|
||||
) -> PermissionProfile {
|
||||
let Some(managed_deny_read_policy) = self.managed_deny_read_policy.as_ref() else {
|
||||
return permission_profile;
|
||||
};
|
||||
if matches!(
|
||||
sandbox_mode_requirement_for_permission_profile(&permission_profile),
|
||||
SandboxModeRequirement::DangerFullAccess | SandboxModeRequirement::ExternalSandbox
|
||||
) {
|
||||
return permission_profile;
|
||||
}
|
||||
let enforcement = permission_profile.enforcement();
|
||||
let (mut file_system_policy, network_policy) = permission_profile.to_runtime_permissions();
|
||||
file_system_policy.preserve_deny_read_restrictions_from(managed_deny_read_policy);
|
||||
PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
enforcement,
|
||||
&file_system_policy,
|
||||
network_policy,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// A profile override only inherits the selected profile's proxy/allowlist config
|
||||
@@ -3985,15 +4014,17 @@ impl Config {
|
||||
effective_file_system_sandbox_policy
|
||||
.preserve_deny_read_restrictions_from(&file_system_sandbox_policy);
|
||||
}
|
||||
if let Some(Sourced {
|
||||
value: filesystem_requirements,
|
||||
..
|
||||
}) = filesystem_requirements.as_ref()
|
||||
{
|
||||
apply_managed_filesystem_constraints(
|
||||
&mut effective_file_system_sandbox_policy,
|
||||
filesystem_requirements,
|
||||
);
|
||||
let managed_deny_read_policy = filesystem_requirements
|
||||
.as_ref()
|
||||
.filter(|Sourced { value, .. }| !value.deny_read.is_empty())
|
||||
.map(|Sourced { value, .. }| {
|
||||
let mut policy = FileSystemSandboxPolicy::restricted(Vec::new());
|
||||
apply_managed_filesystem_constraints(&mut policy, value);
|
||||
Arc::new(policy)
|
||||
});
|
||||
if let Some(managed_deny_read_policy) = managed_deny_read_policy.as_ref() {
|
||||
effective_file_system_sandbox_policy
|
||||
.preserve_deny_read_restrictions_from(managed_deny_read_policy);
|
||||
}
|
||||
let effective_file_system_sandbox_policy = effective_file_system_sandbox_policy
|
||||
.with_additional_readable_roots(resolved_cwd.as_path(), &helper_readable_roots);
|
||||
@@ -4006,6 +4037,58 @@ impl Config {
|
||||
.value
|
||||
.set(effective_permission_profile)
|
||||
.map_err(std::io::Error::from)?;
|
||||
if let Some(Sourced {
|
||||
source: requirement_source,
|
||||
..
|
||||
}) = filesystem_requirements.as_ref()
|
||||
&& let Some(managed_file_system_policy) = managed_deny_read_policy.as_ref()
|
||||
{
|
||||
let _initial_matcher =
|
||||
ReadDenyMatcher::try_new(managed_file_system_policy, resolved_cwd.as_path())
|
||||
.map_err(std::io::Error::other)?;
|
||||
let managed_file_system_policy = Arc::clone(managed_file_system_policy);
|
||||
let permission_cwd = resolved_cwd.clone();
|
||||
let requirement_source = requirement_source.clone();
|
||||
constrained_permission_profile
|
||||
.value
|
||||
.add_validator(move |permission_profile| {
|
||||
let managed_deny_matcher =
|
||||
ReadDenyMatcher::new(&managed_file_system_policy, permission_cwd.as_path());
|
||||
let file_system_policy = permission_profile.file_system_sandbox_policy();
|
||||
let missing_required_deny = managed_file_system_policy
|
||||
.entries
|
||||
.iter()
|
||||
.any(|entry| !file_system_policy.entries.contains(entry));
|
||||
let violating_root = file_system_policy
|
||||
.entries
|
||||
.iter()
|
||||
.filter(|entry| entry.access.can_read())
|
||||
.find_map(|entry| {
|
||||
let FileSystemPath::Path { path } = &entry.path else {
|
||||
return None;
|
||||
};
|
||||
let path = path.to_abs_path().ok()?;
|
||||
managed_deny_matcher
|
||||
.as_ref()
|
||||
.is_some_and(|matcher| matcher.is_read_denied(path.as_path()))
|
||||
.then_some(path)
|
||||
});
|
||||
if missing_required_deny || violating_root.is_some() {
|
||||
return Err(ConstraintError::InvalidValue {
|
||||
field_name: "permissions.filesystem",
|
||||
candidate: violating_root
|
||||
.map_or_else(|| "missing managed deny".to_string(), |path| {
|
||||
path.to_string_lossy().into_owned()
|
||||
}),
|
||||
allowed: "all managed deny_read restrictions".to_string(),
|
||||
requirement_source: requirement_source.clone(),
|
||||
});
|
||||
}
|
||||
|
||||
Ok(())
|
||||
})
|
||||
.map_err(std::io::Error::from)?;
|
||||
}
|
||||
let permission_profile_state = PermissionProfileState::from_constrained_active_profile(
|
||||
constrained_permission_profile.value,
|
||||
active_permission_profile,
|
||||
@@ -4031,6 +4114,7 @@ impl Config {
|
||||
permissions: Permissions {
|
||||
approval_policy: constrained_approval_policy.value,
|
||||
permission_profile_state,
|
||||
managed_deny_read_policy,
|
||||
workspace_roots,
|
||||
network,
|
||||
allow_login_shell,
|
||||
|
||||
@@ -1,9 +1,17 @@
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use codex_config::CloudConfigBundleLoader;
|
||||
use codex_config::test_support::CloudConfigBundleFixture;
|
||||
use codex_core::CodexThreadSettingsOverrides;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::ThreadSettingsOverrides;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_target_windows;
|
||||
use core_test_support::submit_thread_settings;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::sync::Arc;
|
||||
@@ -71,6 +79,72 @@ async fn refreshed_cloud_bundle_updates_later_sessions() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_deny_read_requirements_follow_thread_permission_updates() -> Result<()> {
|
||||
skip_if_target_windows!(
|
||||
Ok(()),
|
||||
"Windows restricted-token sandbox cannot enforce deny-read policies"
|
||||
);
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
let denied_root = home.path().join("managed-private");
|
||||
let nested_root = denied_root.join("nested");
|
||||
std::fs::create_dir_all(&nested_root)?;
|
||||
let nested_root = AbsolutePathBuf::from_absolute_path(nested_root)?;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_home(home)
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(format!(
|
||||
"[permissions.filesystem]\ndeny_read = [{denied_root:?}]\n"
|
||||
)),
|
||||
)
|
||||
.with_config(|config| {
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile(PermissionProfile::workspace_write())
|
||||
.expect("safe workspace permissions should preserve managed denies");
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
submit_thread_settings(
|
||||
&test.codex,
|
||||
ThreadSettingsOverrides {
|
||||
permission_profile: Some(PermissionProfile::read_only()),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
let snapshot = test.codex.config_snapshot().await;
|
||||
assert!(
|
||||
!snapshot
|
||||
.permission_profile
|
||||
.file_system_sandbox_policy()
|
||||
.can_read_path_with_cwd(nested_root.as_path(), snapshot.cwd().as_path()),
|
||||
"a live permission change must preserve managed deny-read rules"
|
||||
);
|
||||
|
||||
let conflicting_profile = PermissionProfile::workspace_write_with(
|
||||
std::slice::from_ref(&nested_root),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
/*exclude_tmpdir_env_var*/ false,
|
||||
/*exclude_slash_tmp*/ false,
|
||||
);
|
||||
let error = test
|
||||
.codex
|
||||
.preview_thread_settings_overrides(CodexThreadSettingsOverrides {
|
||||
permission_profile: Some(conflicting_profile),
|
||||
..Default::default()
|
||||
})
|
||||
.await
|
||||
.err()
|
||||
.context("a concrete writable root must not override a managed deny")?;
|
||||
assert!(error.to_string().contains("permissions.filesystem"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_guardian_v1_requirements_disable_guardian_v2() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
|
||||
Reference in New Issue
Block a user