mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
permissions: migrate approval and sandbox consumers to profiles (#19393)
## Why Runtime decisions should not infer permissions from the lossy legacy sandbox projection once `PermissionProfile` is available. In particular, `Disabled` and `External` need to remain distinct, and managed profiles with split filesystem or deny-read rules should not be collapsed before approval, network, safety, or analytics code makes decisions. ## What Changed - Changes managed network proxy setup and network approval logic to use `PermissionProfile` when deciding whether a managed sandbox is active. - Migrates patch safety, Guardian/user-shell approval paths, Landlock helper setup, analytics sandbox classification, and selected turn/session code to profile-backed permissions. - Validates command-level profile overrides against the constrained `PermissionProfile` rather than a strict `SandboxPolicy` round trip. - Preserves configured deny-read restrictions when command profiles are narrowed. - Adds coverage for profile-backed trust, network proxy/approval behavior, patch safety, analytics classification, and command-profile narrowing. ## Verification - `cargo test -p codex-core direct_write_roots` - `cargo test -p codex-core runtime_roots_to_legacy_projection` - `cargo test -p codex-app-server requested_permissions_trust_project_uses_permission_profile_intent` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19393). * #19395 * #19394 * __->__ #19393
This commit is contained in:
@@ -58,6 +58,7 @@ use codex_model_provider_info::WireApi;
|
||||
use codex_models_manager::bundled_models_response;
|
||||
use codex_protocol::models::ManagedFileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::SandboxEnforcement;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
@@ -6775,6 +6776,62 @@ async fn permission_profile_override_falls_back_when_disallowed_by_requirements(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn permission_profile_override_preserves_split_write_roots() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let cwd = codex_home.path().join("workspace");
|
||||
let outside_root = codex_home.path().join("outside-write");
|
||||
std::fs::create_dir_all(&cwd)?;
|
||||
std::fs::create_dir_all(&outside_root)?;
|
||||
let outside_root =
|
||||
AbsolutePathBuf::from_absolute_path(outside_root).expect("outside root is absolute");
|
||||
let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::Root,
|
||||
},
|
||||
access: FileSystemAccessMode::Read,
|
||||
},
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path {
|
||||
path: outside_root.clone(),
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
},
|
||||
]);
|
||||
let permission_profile = PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
SandboxEnforcement::Managed,
|
||||
&file_system_sandbox_policy,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
);
|
||||
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(cwd))
|
||||
.harness_overrides(ConfigOverrides {
|
||||
permission_profile: Some(permission_profile),
|
||||
..Default::default()
|
||||
})
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert!(
|
||||
config
|
||||
.permissions
|
||||
.file_system_sandbox_policy()
|
||||
.can_write_path_with_cwd(outside_root.as_path(), config.cwd.as_path())
|
||||
);
|
||||
assert!(matches!(
|
||||
config.permissions.sandbox_policy.get(),
|
||||
SandboxPolicy::WorkspaceWrite { .. }
|
||||
));
|
||||
assert_eq!(
|
||||
config.permissions.network_sandbox_policy(),
|
||||
NetworkSandboxPolicy::Restricted
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn requirements_web_search_mode_overrides_danger_full_access_default() -> std::io::Result<()>
|
||||
{
|
||||
|
||||
@@ -2396,10 +2396,17 @@ impl Config {
|
||||
None => (None, None),
|
||||
};
|
||||
let has_network_requirements = network_requirements.is_some();
|
||||
let network_permission_profile = if *constrained_sandbox_policy.get()
|
||||
== original_sandbox_policy
|
||||
{
|
||||
permission_profile.clone()
|
||||
} else {
|
||||
PermissionProfile::from_legacy_sandbox_policy(constrained_sandbox_policy.get())
|
||||
};
|
||||
let network = NetworkProxySpec::from_config_and_constraints(
|
||||
configured_network_proxy_config,
|
||||
network_requirements,
|
||||
constrained_sandbox_policy.get(),
|
||||
&network_permission_profile,
|
||||
)
|
||||
.map_err(|err| {
|
||||
if let Some(source) = network_requirements_source.as_ref() {
|
||||
|
||||
@@ -16,7 +16,7 @@ use codex_network_proxy::build_config_state;
|
||||
use codex_network_proxy::host_and_port_from_network_addr;
|
||||
use codex_network_proxy::normalize_host;
|
||||
use codex_network_proxy::validate_policy_against_constraints;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
@@ -89,7 +89,7 @@ impl NetworkProxySpec {
|
||||
pub(crate) fn from_config_and_constraints(
|
||||
config: NetworkProxyConfig,
|
||||
requirements: Option<NetworkConstraints>,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
permission_profile: &PermissionProfile,
|
||||
) -> std::io::Result<Self> {
|
||||
let base_config = config.clone();
|
||||
let hard_deny_allowlist_misses = requirements
|
||||
@@ -99,7 +99,7 @@ impl NetworkProxySpec {
|
||||
Self::apply_requirements(
|
||||
config,
|
||||
requirements,
|
||||
sandbox_policy,
|
||||
permission_profile,
|
||||
hard_deny_allowlist_misses,
|
||||
)
|
||||
} else {
|
||||
@@ -122,7 +122,7 @@ impl NetworkProxySpec {
|
||||
|
||||
pub async fn start_proxy(
|
||||
&self,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
permission_profile: &PermissionProfile,
|
||||
policy_decider: Option<Arc<dyn NetworkPolicyDecider>>,
|
||||
blocked_request_observer: Option<Arc<dyn BlockedRequestObserver>>,
|
||||
enable_network_approval_flow: bool,
|
||||
@@ -133,10 +133,7 @@ impl NetworkProxySpec {
|
||||
if enable_network_approval_flow && !self.hard_deny_allowlist_misses {
|
||||
if let Some(policy_decider) = policy_decider {
|
||||
builder = builder.policy_decider_arc(policy_decider);
|
||||
} else if matches!(
|
||||
sandbox_policy,
|
||||
SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. }
|
||||
) {
|
||||
} else if Self::managed_sandbox_active(permission_profile) {
|
||||
builder = builder
|
||||
.policy_decider(|_request| async { NetworkDecision::ask("not_allowed") });
|
||||
}
|
||||
@@ -154,14 +151,14 @@ impl NetworkProxySpec {
|
||||
Ok(StartedNetworkProxy::new(proxy, handle))
|
||||
}
|
||||
|
||||
pub(crate) fn recompute_for_sandbox_policy(
|
||||
pub(crate) fn recompute_for_permission_profile(
|
||||
&self,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
permission_profile: &PermissionProfile,
|
||||
) -> std::io::Result<Self> {
|
||||
Self::from_config_and_constraints(
|
||||
self.base_config.clone(),
|
||||
self.requirements.clone(),
|
||||
sandbox_policy,
|
||||
permission_profile,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -216,13 +213,13 @@ impl NetworkProxySpec {
|
||||
fn apply_requirements(
|
||||
mut config: NetworkProxyConfig,
|
||||
requirements: &NetworkConstraints,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
permission_profile: &PermissionProfile,
|
||||
hard_deny_allowlist_misses: bool,
|
||||
) -> (NetworkProxyConfig, NetworkProxyConstraints) {
|
||||
let mut constraints = NetworkProxyConstraints::default();
|
||||
let allowlist_expansion_enabled =
|
||||
Self::allowlist_expansion_enabled(sandbox_policy, hard_deny_allowlist_misses);
|
||||
let denylist_expansion_enabled = Self::denylist_expansion_enabled(sandbox_policy);
|
||||
Self::allowlist_expansion_enabled(permission_profile, hard_deny_allowlist_misses);
|
||||
let denylist_expansion_enabled = Self::denylist_expansion_enabled(permission_profile);
|
||||
|
||||
if let Some(enabled) = requirements.enabled {
|
||||
config.network.enabled = enabled;
|
||||
@@ -322,24 +319,22 @@ impl NetworkProxySpec {
|
||||
}
|
||||
|
||||
fn allowlist_expansion_enabled(
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
permission_profile: &PermissionProfile,
|
||||
hard_deny_allowlist_misses: bool,
|
||||
) -> bool {
|
||||
matches!(
|
||||
sandbox_policy,
|
||||
SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. }
|
||||
) && !hard_deny_allowlist_misses
|
||||
Self::managed_sandbox_active(permission_profile) && !hard_deny_allowlist_misses
|
||||
}
|
||||
|
||||
fn managed_allowed_domains_only(requirements: &NetworkConstraints) -> bool {
|
||||
requirements.managed_allowed_domains_only.unwrap_or(false)
|
||||
}
|
||||
|
||||
fn denylist_expansion_enabled(sandbox_policy: &SandboxPolicy) -> bool {
|
||||
matches!(
|
||||
sandbox_policy,
|
||||
SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. }
|
||||
)
|
||||
fn denylist_expansion_enabled(permission_profile: &PermissionProfile) -> bool {
|
||||
Self::managed_sandbox_active(permission_profile)
|
||||
}
|
||||
|
||||
fn managed_sandbox_active(permission_profile: &PermissionProfile) -> bool {
|
||||
matches!(permission_profile, PermissionProfile::Managed { .. })
|
||||
}
|
||||
|
||||
fn merge_domain_lists(mut managed: Vec<String>, user_entries: &[String]) -> Vec<String> {
|
||||
|
||||
@@ -2,8 +2,16 @@ use super::*;
|
||||
use codex_config::NetworkDomainPermissionToml;
|
||||
use codex_config::NetworkDomainPermissionsToml;
|
||||
use codex_network_proxy::NetworkDomainPermission;
|
||||
use codex_protocol::models::ManagedFileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn permission_profile_for_sandbox_policy(sandbox_policy: &SandboxPolicy) -> PermissionProfile {
|
||||
PermissionProfile::from_legacy_sandbox_policy(sandbox_policy)
|
||||
}
|
||||
|
||||
fn domain_permissions(
|
||||
entries: impl IntoIterator<Item = (&'static str, NetworkDomainPermissionToml)>,
|
||||
) -> NetworkDomainPermissionsToml {
|
||||
@@ -54,7 +62,7 @@ fn requirements_allowed_domains_are_a_baseline_for_user_allowlist() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_read_only_policy()),
|
||||
)
|
||||
.expect("config should stay within the managed allowlist");
|
||||
|
||||
@@ -89,7 +97,7 @@ fn requirements_allowed_domains_do_not_override_user_denies_for_same_pattern() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy()),
|
||||
)
|
||||
.expect("managed allowlist should not erase a user deny");
|
||||
|
||||
@@ -121,7 +129,7 @@ fn requirements_allowlist_expansion_keeps_user_entries_mutable() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy()),
|
||||
)
|
||||
.expect("managed baseline should still allow user edits");
|
||||
|
||||
@@ -144,6 +152,41 @@ fn requirements_allowlist_expansion_keeps_user_entries_mutable() {
|
||||
.expect("user allowlist entries should not become managed constraints");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn managed_unrestricted_profile_allows_domain_expansion() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
config
|
||||
.network
|
||||
.set_allowed_domains(vec!["api.example.com".to_string()]);
|
||||
let requirements = NetworkConstraints {
|
||||
domains: Some(domain_permissions([(
|
||||
"*.example.com",
|
||||
NetworkDomainPermissionToml::Allow,
|
||||
)])),
|
||||
..Default::default()
|
||||
};
|
||||
let permission_profile = PermissionProfile::Managed {
|
||||
file_system: ManagedFileSystemPermissions::Unrestricted,
|
||||
network: NetworkSandboxPolicy::Restricted,
|
||||
};
|
||||
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&permission_profile,
|
||||
)
|
||||
.expect("managed unrestricted filesystem should still use managed network constraints");
|
||||
|
||||
assert_eq!(
|
||||
spec.config.network.allowed_domains(),
|
||||
Some(vec![
|
||||
"*.example.com".to_string(),
|
||||
"api.example.com".to_string()
|
||||
])
|
||||
);
|
||||
assert_eq!(spec.constraints.allowlist_expansion_enabled, Some(true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn danger_full_access_keeps_managed_allowlist_and_denylist_fixed() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
@@ -164,7 +207,7 @@ fn danger_full_access_keeps_managed_allowlist_and_denylist_fixed() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::DangerFullAccess),
|
||||
)
|
||||
.expect("yolo mode should pin the effective policy to the managed baseline");
|
||||
|
||||
@@ -198,7 +241,7 @@ fn managed_allowed_domains_only_disables_default_mode_allowlist_expansion() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy()),
|
||||
)
|
||||
.expect("managed baseline should still load");
|
||||
|
||||
@@ -227,7 +270,7 @@ fn managed_allowed_domains_only_ignores_user_allowlist_and_hard_denies_misses()
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy()),
|
||||
)
|
||||
.expect("managed-only allowlist should still load");
|
||||
|
||||
@@ -257,7 +300,7 @@ fn managed_allowed_domains_only_without_managed_allowlist_blocks_all_user_domain
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy()),
|
||||
)
|
||||
.expect("managed-only mode should treat missing managed allowlist as empty");
|
||||
|
||||
@@ -281,7 +324,7 @@ fn managed_allowed_domains_only_blocks_all_user_domains_in_full_access_without_m
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::DangerFullAccess),
|
||||
)
|
||||
.expect("managed-only mode should treat missing managed allowlist as empty");
|
||||
|
||||
@@ -308,7 +351,7 @@ fn deny_only_requirements_do_not_create_allow_constraints_in_full_access() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::DangerFullAccess),
|
||||
)
|
||||
.expect("deny-only requirements should not constrain the allowlist");
|
||||
|
||||
@@ -341,7 +384,7 @@ fn allow_only_requirements_do_not_create_deny_constraints_in_full_access() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::DangerFullAccess),
|
||||
)
|
||||
.expect("allow-only requirements should not constrain the denylist");
|
||||
|
||||
@@ -374,7 +417,7 @@ fn requirements_denied_domains_are_a_baseline_for_default_mode() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy()),
|
||||
)
|
||||
.expect("default mode should merge managed and user deny entries");
|
||||
|
||||
@@ -409,7 +452,7 @@ fn requirements_denylist_expansion_keeps_user_entries_mutable() {
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
&permission_profile_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy()),
|
||||
)
|
||||
.expect("managed baseline should still allow user edits");
|
||||
|
||||
|
||||
Reference in New Issue
Block a user