mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix: Revert danger-full-access denylist-only mode (#17732)
## Summary - Reverts openai/codex#16946 and removes the danger-full-access denylist-only network mode. - Removes the corresponding config requirements, app-server protocol/schema, config API, TUI debug output, and network proxy behavior. - Drops stale tests that depended on the reverted mode while preserving newer managed allowlist-only coverage. ## Verification - `just write-app-server-schema` - `just fmt` - `cargo test -p codex-config network_requirements` - `cargo test -p codex-core network_proxy_spec` - `cargo test -p codex-core managed_network_proxy_decider_survives_full_access_start` - `cargo test -p codex-app-server map_requirements_toml_to_api` - `cargo test -p codex-tui debug_config_output` - `cargo test -p codex-app-server-protocol` - `just fix -p codex-config -p codex-core -p codex-app-server-protocol -p codex-app-server -p codex-tui` - `git diff --cached --check` Not run: full workspace `cargo test` (repo instructions ask for confirmation before that broader run).
This commit is contained in:
@@ -588,78 +588,12 @@ async fn start_managed_network_proxy_ignores_invalid_execpolicy_network_rules()
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_network_proxy_refreshes_when_sandbox_policy_changes() -> anyhow::Result<()> {
|
||||
let spec = crate::config::NetworkProxySpec::from_config_and_constraints(
|
||||
NetworkProxyConfig::default(),
|
||||
Some(NetworkConstraints {
|
||||
domains: Some(NetworkDomainPermissionsToml {
|
||||
entries: std::collections::BTreeMap::from([(
|
||||
"blocked.example.com".to_string(),
|
||||
NetworkDomainPermissionToml::Deny,
|
||||
)]),
|
||||
}),
|
||||
danger_full_access_denylist_only: Some(true),
|
||||
allow_local_binding: Some(false),
|
||||
..Default::default()
|
||||
}),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
)?;
|
||||
let exec_policy = Policy::empty();
|
||||
|
||||
let (started_proxy, _) = Session::start_managed_network_proxy(
|
||||
&spec,
|
||||
&exec_policy,
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
/*network_policy_decider*/ None,
|
||||
/*blocked_request_observer*/ None,
|
||||
/*managed_network_requirements_enabled*/ false,
|
||||
crate::config::NetworkProxyAuditMetadata::default(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert!(!started_proxy.proxy().allow_local_binding());
|
||||
let current_cfg = started_proxy.proxy().current_cfg().await?;
|
||||
assert_eq!(current_cfg.network.allowed_domains(), None);
|
||||
assert_eq!(
|
||||
current_cfg.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
|
||||
let spec = spec.recompute_for_sandbox_policy(&SandboxPolicy::DangerFullAccess)?;
|
||||
spec.apply_to_started_proxy(&started_proxy).await?;
|
||||
|
||||
assert!(started_proxy.proxy().allow_local_binding());
|
||||
let current_cfg = started_proxy.proxy().current_cfg().await?;
|
||||
assert_eq!(
|
||||
current_cfg.network.allowed_domains(),
|
||||
Some(vec!["*".to_string()])
|
||||
);
|
||||
assert_eq!(
|
||||
current_cfg.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
|
||||
let spec = spec.recompute_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy())?;
|
||||
spec.apply_to_started_proxy(&started_proxy).await?;
|
||||
|
||||
assert!(!started_proxy.proxy().allow_local_binding());
|
||||
let current_cfg = started_proxy.proxy().current_cfg().await?;
|
||||
assert_eq!(current_cfg.network.allowed_domains(), None);
|
||||
assert_eq!(
|
||||
current_cfg.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_network_proxy_decider_survives_full_access_start() -> anyhow::Result<()> {
|
||||
let spec = crate::config::NetworkProxySpec::from_config_and_constraints(
|
||||
NetworkProxyConfig::default(),
|
||||
Some(NetworkConstraints {
|
||||
enabled: Some(true),
|
||||
danger_full_access_denylist_only: Some(true),
|
||||
..Default::default()
|
||||
}),
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
|
||||
@@ -20,8 +20,6 @@ use codex_protocol::protocol::SandboxPolicy;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
const GLOBAL_ALLOWLIST_PATTERN: &str = "*";
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct NetworkProxySpec {
|
||||
base_config: NetworkProxyConfig,
|
||||
@@ -225,8 +223,6 @@ impl NetworkProxySpec {
|
||||
let allowlist_expansion_enabled =
|
||||
Self::allowlist_expansion_enabled(sandbox_policy, hard_deny_allowlist_misses);
|
||||
let denylist_expansion_enabled = Self::denylist_expansion_enabled(sandbox_policy);
|
||||
let danger_full_access_denylist_only =
|
||||
Self::danger_full_access_denylist_only_enabled(requirements, sandbox_policy);
|
||||
|
||||
if let Some(enabled) = requirements.enabled {
|
||||
config.network.enabled = enabled;
|
||||
@@ -257,43 +253,37 @@ impl NetworkProxySpec {
|
||||
constraints.dangerously_allow_all_unix_sockets =
|
||||
Some(dangerously_allow_all_unix_sockets);
|
||||
}
|
||||
if danger_full_access_denylist_only {
|
||||
config
|
||||
.network
|
||||
.set_allowed_domains(vec![GLOBAL_ALLOWLIST_PATTERN.to_string()]);
|
||||
} else {
|
||||
let managed_allowed_domains = if hard_deny_allowlist_misses {
|
||||
Some(
|
||||
requirements
|
||||
.domains
|
||||
.as_ref()
|
||||
.and_then(codex_config::NetworkDomainPermissionsToml::allowed_domains)
|
||||
.unwrap_or_default(),
|
||||
)
|
||||
} else {
|
||||
let managed_allowed_domains = if hard_deny_allowlist_misses {
|
||||
Some(
|
||||
requirements
|
||||
.domains
|
||||
.as_ref()
|
||||
.and_then(codex_config::NetworkDomainPermissionsToml::allowed_domains)
|
||||
.unwrap_or_default(),
|
||||
)
|
||||
} else {
|
||||
requirements
|
||||
.domains
|
||||
.as_ref()
|
||||
.and_then(codex_config::NetworkDomainPermissionsToml::allowed_domains)
|
||||
};
|
||||
if let Some(managed_allowed_domains) = managed_allowed_domains {
|
||||
// Managed requirements seed the baseline allowlist. User additions
|
||||
// can extend that baseline unless managed-only mode pins the
|
||||
// effective allowlist to the managed set.
|
||||
let effective_allowed_domains = if allowlist_expansion_enabled {
|
||||
Self::merge_domain_lists(
|
||||
managed_allowed_domains.clone(),
|
||||
config.network.allowed_domains().as_deref().unwrap_or(&[]),
|
||||
)
|
||||
} else {
|
||||
managed_allowed_domains.clone()
|
||||
};
|
||||
if let Some(managed_allowed_domains) = managed_allowed_domains {
|
||||
// Managed requirements seed the baseline allowlist. User additions
|
||||
// can extend that baseline unless managed-only mode pins the
|
||||
// effective allowlist to the managed set.
|
||||
let effective_allowed_domains = if allowlist_expansion_enabled {
|
||||
Self::merge_domain_lists(
|
||||
managed_allowed_domains.clone(),
|
||||
config.network.allowed_domains().as_deref().unwrap_or(&[]),
|
||||
)
|
||||
} else {
|
||||
managed_allowed_domains.clone()
|
||||
};
|
||||
config
|
||||
.network
|
||||
.set_allowed_domains(effective_allowed_domains);
|
||||
constraints.allowed_domains = Some(managed_allowed_domains);
|
||||
constraints.allowlist_expansion_enabled = Some(allowlist_expansion_enabled);
|
||||
}
|
||||
config
|
||||
.network
|
||||
.set_allowed_domains(effective_allowed_domains);
|
||||
constraints.allowed_domains = Some(managed_allowed_domains);
|
||||
constraints.allowlist_expansion_enabled = Some(allowlist_expansion_enabled);
|
||||
}
|
||||
let managed_denied_domains = requirements
|
||||
.domains
|
||||
@@ -312,7 +302,7 @@ impl NetworkProxySpec {
|
||||
constraints.denied_domains = Some(managed_denied_domains);
|
||||
constraints.denylist_expansion_enabled = Some(denylist_expansion_enabled);
|
||||
}
|
||||
if requirements.unix_sockets.is_some() && !danger_full_access_denylist_only {
|
||||
if requirements.unix_sockets.is_some() {
|
||||
let allow_unix_sockets = requirements
|
||||
.unix_sockets
|
||||
.as_ref()
|
||||
@@ -327,14 +317,6 @@ impl NetworkProxySpec {
|
||||
config.network.allow_local_binding = allow_local_binding;
|
||||
constraints.allow_local_binding = Some(allow_local_binding);
|
||||
}
|
||||
if danger_full_access_denylist_only {
|
||||
config.network.allow_upstream_proxy = true;
|
||||
constraints.allow_upstream_proxy = Some(true);
|
||||
config.network.dangerously_allow_all_unix_sockets = true;
|
||||
constraints.dangerously_allow_all_unix_sockets = Some(true);
|
||||
config.network.allow_local_binding = true;
|
||||
constraints.allow_local_binding = Some(true);
|
||||
}
|
||||
|
||||
(config, constraints)
|
||||
}
|
||||
@@ -353,16 +335,6 @@ impl NetworkProxySpec {
|
||||
requirements.managed_allowed_domains_only.unwrap_or(false)
|
||||
}
|
||||
|
||||
fn danger_full_access_denylist_only_enabled(
|
||||
requirements: &NetworkConstraints,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
) -> bool {
|
||||
matches!(sandbox_policy, SandboxPolicy::DangerFullAccess)
|
||||
&& requirements
|
||||
.danger_full_access_denylist_only
|
||||
.unwrap_or(false)
|
||||
}
|
||||
|
||||
fn denylist_expansion_enabled(sandbox_policy: &SandboxPolicy) -> bool {
|
||||
matches!(
|
||||
sandbox_policy,
|
||||
|
||||
@@ -1,11 +1,8 @@
|
||||
use super::*;
|
||||
use crate::config_loader::NetworkDomainPermissionToml;
|
||||
use crate::config_loader::NetworkDomainPermissionsToml;
|
||||
use crate::config_loader::NetworkUnixSocketPermissionToml;
|
||||
use crate::config_loader::NetworkUnixSocketPermissionsToml;
|
||||
use codex_network_proxy::NetworkDomainPermission;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
fn domain_permissions(
|
||||
entries: impl IntoIterator<Item = (&'static str, NetworkDomainPermissionToml)>,
|
||||
@@ -183,196 +180,6 @@ fn danger_full_access_keeps_managed_allowlist_and_denylist_fixed() {
|
||||
assert_eq!(spec.constraints.denylist_expansion_enabled, Some(false));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn danger_full_access_denylist_only_allows_all_domains_and_enforces_managed_denies() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
config
|
||||
.network
|
||||
.set_allowed_domains(vec!["evil.com".to_string()]);
|
||||
config
|
||||
.network
|
||||
.set_denied_domains(vec!["more-blocked.example.com".to_string()]);
|
||||
let requirements = NetworkConstraints {
|
||||
allow_upstream_proxy: Some(false),
|
||||
dangerously_allow_all_unix_sockets: Some(false),
|
||||
domains: Some(domain_permissions([
|
||||
("*.example.com", NetworkDomainPermissionToml::Allow),
|
||||
("blocked.example.com", NetworkDomainPermissionToml::Deny),
|
||||
])),
|
||||
danger_full_access_denylist_only: Some(true),
|
||||
unix_sockets: Some(NetworkUnixSocketPermissionsToml {
|
||||
entries: BTreeMap::from([(
|
||||
"/tmp/managed.sock".to_string(),
|
||||
NetworkUnixSocketPermissionToml::Allow,
|
||||
)]),
|
||||
}),
|
||||
allow_local_binding: Some(false),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
)
|
||||
.expect("denylist-only yolo mode should allow all domains except managed denies");
|
||||
|
||||
assert_eq!(
|
||||
spec.config.network.allowed_domains(),
|
||||
Some(vec!["*".to_string()])
|
||||
);
|
||||
assert_eq!(
|
||||
spec.config.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
assert!(spec.config.network.allow_upstream_proxy);
|
||||
assert!(spec.config.network.dangerously_allow_all_unix_sockets);
|
||||
assert!(spec.config.network.allow_local_binding);
|
||||
assert_eq!(spec.constraints.allow_upstream_proxy, Some(true));
|
||||
assert_eq!(
|
||||
spec.constraints.dangerously_allow_all_unix_sockets,
|
||||
Some(true)
|
||||
);
|
||||
assert_eq!(spec.constraints.allow_unix_sockets, None);
|
||||
assert_eq!(spec.constraints.allow_local_binding, Some(true));
|
||||
assert_eq!(spec.constraints.allowed_domains, None);
|
||||
assert_eq!(spec.constraints.allowlist_expansion_enabled, None);
|
||||
assert_eq!(
|
||||
spec.constraints.denied_domains,
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
assert_eq!(spec.constraints.denylist_expansion_enabled, Some(false));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn danger_full_access_denylist_only_does_not_change_workspace_write_behavior() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
config
|
||||
.network
|
||||
.set_allowed_domains(vec!["api.example.com".to_string()]);
|
||||
config
|
||||
.network
|
||||
.set_denied_domains(vec!["blocked.example.com".to_string()]);
|
||||
let requirements = NetworkConstraints {
|
||||
allow_upstream_proxy: Some(false),
|
||||
dangerously_allow_all_unix_sockets: Some(false),
|
||||
domains: Some(domain_permissions([
|
||||
("*.example.com", NetworkDomainPermissionToml::Allow),
|
||||
(
|
||||
"managed-blocked.example.com",
|
||||
NetworkDomainPermissionToml::Deny,
|
||||
),
|
||||
])),
|
||||
danger_full_access_denylist_only: Some(true),
|
||||
unix_sockets: Some(NetworkUnixSocketPermissionsToml {
|
||||
entries: BTreeMap::from([(
|
||||
"/tmp/managed.sock".to_string(),
|
||||
NetworkUnixSocketPermissionToml::Allow,
|
||||
)]),
|
||||
}),
|
||||
allow_local_binding: Some(false),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
)
|
||||
.expect("denylist-only yolo flag should not affect workspace-write mode");
|
||||
|
||||
assert_eq!(
|
||||
spec.config.network.allowed_domains(),
|
||||
Some(vec![
|
||||
"*.example.com".to_string(),
|
||||
"api.example.com".to_string()
|
||||
])
|
||||
);
|
||||
assert_eq!(
|
||||
spec.config.network.denied_domains(),
|
||||
Some(vec![
|
||||
"managed-blocked.example.com".to_string(),
|
||||
"blocked.example.com".to_string()
|
||||
])
|
||||
);
|
||||
assert!(!spec.config.network.allow_upstream_proxy);
|
||||
assert!(!spec.config.network.dangerously_allow_all_unix_sockets);
|
||||
assert_eq!(
|
||||
spec.config.network.allow_unix_sockets(),
|
||||
vec!["/tmp/managed.sock".to_string()]
|
||||
);
|
||||
assert!(!spec.config.network.allow_local_binding);
|
||||
assert_eq!(spec.constraints.allow_upstream_proxy, Some(false));
|
||||
assert_eq!(
|
||||
spec.constraints.dangerously_allow_all_unix_sockets,
|
||||
Some(false)
|
||||
);
|
||||
assert_eq!(
|
||||
spec.constraints.allow_unix_sockets,
|
||||
Some(vec!["/tmp/managed.sock".to_string()])
|
||||
);
|
||||
assert_eq!(spec.constraints.allow_local_binding, Some(false));
|
||||
assert_eq!(
|
||||
spec.constraints.allowed_domains,
|
||||
Some(vec!["*.example.com".to_string()])
|
||||
);
|
||||
assert_eq!(spec.constraints.allowlist_expansion_enabled, Some(true));
|
||||
assert_eq!(
|
||||
spec.constraints.denied_domains,
|
||||
Some(vec!["managed-blocked.example.com".to_string()])
|
||||
);
|
||||
assert_eq!(spec.constraints.denylist_expansion_enabled, Some(true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recompute_for_sandbox_policy_rebuilds_denylist_only_full_access_policy() {
|
||||
let requirements = NetworkConstraints {
|
||||
domains: Some(domain_permissions([(
|
||||
"blocked.example.com",
|
||||
NetworkDomainPermissionToml::Deny,
|
||||
)])),
|
||||
danger_full_access_denylist_only: Some(true),
|
||||
..Default::default()
|
||||
};
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
NetworkProxyConfig::default(),
|
||||
Some(requirements),
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
)
|
||||
.expect("workspace-write policy should load");
|
||||
|
||||
assert_eq!(spec.config.network.allowed_domains(), None);
|
||||
assert_eq!(
|
||||
spec.config.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
|
||||
let spec = spec
|
||||
.recompute_for_sandbox_policy(&SandboxPolicy::DangerFullAccess)
|
||||
.expect("full-access policy should load");
|
||||
|
||||
assert_eq!(
|
||||
spec.config.network.allowed_domains(),
|
||||
Some(vec!["*".to_string()])
|
||||
);
|
||||
assert_eq!(
|
||||
spec.config.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
assert!(spec.config.network.allow_local_binding);
|
||||
|
||||
let spec = spec
|
||||
.recompute_for_sandbox_policy(&SandboxPolicy::new_workspace_write_policy())
|
||||
.expect("workspace-write policy should reload");
|
||||
|
||||
assert_eq!(spec.config.network.allowed_domains(), None);
|
||||
assert_eq!(
|
||||
spec.config.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
assert!(!spec.config.network.allow_local_binding);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn managed_allowed_domains_only_disables_default_mode_allowlist_expansion() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
|
||||
@@ -201,13 +201,16 @@ async fn list_tool_suggest_discoverable_plugins_does_not_reload_marketplace_per_
|
||||
|
||||
let logs = String::from_utf8(buffer.lock().expect("buffer lock").clone()).expect("utf8 logs");
|
||||
assert_eq!(logs.matches("ignoring interface.defaultPrompt").count(), 2);
|
||||
let normalized_logs = logs.replace('\\', "/");
|
||||
assert_eq!(
|
||||
logs.matches("build-ios-apps/.codex-plugin/plugin.json")
|
||||
normalized_logs
|
||||
.matches("build-ios-apps/.codex-plugin/plugin.json")
|
||||
.count(),
|
||||
1
|
||||
);
|
||||
assert_eq!(
|
||||
logs.matches("life-science-research/.codex-plugin/plugin.json")
|
||||
normalized_logs
|
||||
.matches("life-science-research/.codex-plugin/plugin.json")
|
||||
.count(),
|
||||
1
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user