mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
removing network proxy for yolo (#17742)
**Summary** - prevent managed requirements.toml network settings from leaking into DangerFullAccess / yolo turns by gating managed proxy attachment on sandbox mode - keep guardian/sandboxed modes on the managed proxy path, while making true yolo bypass the proxy entirely, including /shell full-access commands
This commit is contained in:
committed by
GitHub
Unverified
parent
c2bdb7812c
commit
e2dbe7dfc3
@@ -1338,6 +1338,10 @@ impl Session {
|
||||
}
|
||||
}
|
||||
|
||||
fn managed_network_proxy_active_for_sandbox_policy(sandbox_policy: &SandboxPolicy) -> bool {
|
||||
!matches!(sandbox_policy, SandboxPolicy::DangerFullAccess)
|
||||
}
|
||||
|
||||
/// Builds the `x-codex-beta-features` header value for this session.
|
||||
///
|
||||
/// `ModelClient` is session-scoped and intentionally does not depend on the full `Config`, so
|
||||
@@ -2004,10 +2008,15 @@ impl Session {
|
||||
.await;
|
||||
session_configuration.thread_name = thread_name.clone();
|
||||
let state = SessionState::new(session_configuration.clone());
|
||||
let managed_network_requirements_configured = config
|
||||
.config_layer_stack
|
||||
.requirements_toml()
|
||||
.network
|
||||
.is_some();
|
||||
let managed_network_requirements_enabled = config.managed_network_requirements_enabled();
|
||||
let network_approval = Arc::new(NetworkApprovalService::default());
|
||||
// The managed proxy can call back into core for allowlist-miss decisions.
|
||||
let network_policy_decider_session = if managed_network_requirements_enabled {
|
||||
let network_policy_decider_session = if managed_network_requirements_configured {
|
||||
config
|
||||
.permissions
|
||||
.network
|
||||
@@ -2016,7 +2025,7 @@ impl Session {
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let blocked_request_observer = if managed_network_requirements_enabled {
|
||||
let blocked_request_observer = if managed_network_requirements_configured {
|
||||
config
|
||||
.permissions
|
||||
.network
|
||||
@@ -2043,7 +2052,7 @@ impl Session {
|
||||
config.permissions.sandbox_policy.get(),
|
||||
network_policy_decider.as_ref().map(Arc::clone),
|
||||
blocked_request_observer.as_ref().map(Arc::clone),
|
||||
managed_network_requirements_enabled,
|
||||
managed_network_requirements_configured,
|
||||
network_proxy_audit_metadata,
|
||||
)
|
||||
.instrument(info_span!(
|
||||
@@ -2200,7 +2209,11 @@ impl Session {
|
||||
history_log_id,
|
||||
history_entry_count,
|
||||
initial_messages,
|
||||
network_proxy: session_network_proxy,
|
||||
network_proxy: session_network_proxy.filter(|_| {
|
||||
Self::managed_network_proxy_active_for_sandbox_policy(
|
||||
session_configuration.sandbox_policy.get(),
|
||||
)
|
||||
}),
|
||||
rollout_path,
|
||||
}),
|
||||
})
|
||||
@@ -2734,7 +2747,12 @@ impl Session {
|
||||
self.services
|
||||
.network_proxy
|
||||
.as_ref()
|
||||
.map(StartedNetworkProxy::proxy),
|
||||
.and_then(|started_proxy| {
|
||||
Self::managed_network_proxy_active_for_sandbox_policy(
|
||||
session_configuration.sandbox_policy.get(),
|
||||
)
|
||||
.then(|| started_proxy.proxy())
|
||||
}),
|
||||
self.services.environment.clone(),
|
||||
sub_id,
|
||||
Arc::clone(&self.js_repl),
|
||||
|
||||
@@ -45,6 +45,8 @@ use crate::rollout::recorder::RolloutRecorder;
|
||||
use crate::state::TaskKind;
|
||||
use crate::tasks::SessionTask;
|
||||
use crate::tasks::SessionTaskContext;
|
||||
use crate::tasks::UserShellCommandMode;
|
||||
use crate::tasks::execute_user_shell_command;
|
||||
use crate::tools::ToolRouter;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
use crate::tools::context::ToolPayload;
|
||||
@@ -740,6 +742,99 @@ async fn new_turn_refreshes_managed_network_proxy_for_sandbox_change() -> anyhow
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn danger_full_access_turns_do_not_expose_managed_network_proxy() -> anyhow::Result<()> {
|
||||
let network_spec = crate::config::NetworkProxySpec::from_config_and_constraints(
|
||||
NetworkProxyConfig::default(),
|
||||
Some(NetworkConstraints {
|
||||
enabled: Some(true),
|
||||
..Default::default()
|
||||
}),
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
)?;
|
||||
|
||||
let session = make_session_with_config(move |config| {
|
||||
config.permissions.sandbox_policy =
|
||||
codex_config::Constrained::allow_any(SandboxPolicy::DangerFullAccess);
|
||||
config.permissions.network = Some(network_spec);
|
||||
})
|
||||
.await?;
|
||||
|
||||
let turn_context = session.new_default_turn().await;
|
||||
assert!(turn_context.network.is_none());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn workspace_write_turns_continue_to_expose_managed_network_proxy() -> anyhow::Result<()> {
|
||||
let sandbox_policy = SandboxPolicy::new_workspace_write_policy();
|
||||
let network_spec = crate::config::NetworkProxySpec::from_config_and_constraints(
|
||||
NetworkProxyConfig::default(),
|
||||
Some(NetworkConstraints {
|
||||
enabled: Some(true),
|
||||
..Default::default()
|
||||
}),
|
||||
&sandbox_policy,
|
||||
)?;
|
||||
|
||||
let session = make_session_with_config(move |config| {
|
||||
config.permissions.sandbox_policy = codex_config::Constrained::allow_any(sandbox_policy);
|
||||
config.permissions.network = Some(network_spec);
|
||||
})
|
||||
.await?;
|
||||
|
||||
let turn_context = session.new_default_turn().await;
|
||||
assert!(turn_context.network.is_some());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn user_shell_commands_do_not_inherit_managed_network_proxy() -> anyhow::Result<()> {
|
||||
let sandbox_policy = SandboxPolicy::new_workspace_write_policy();
|
||||
let network_spec = crate::config::NetworkProxySpec::from_config_and_constraints(
|
||||
NetworkProxyConfig::default(),
|
||||
Some(NetworkConstraints {
|
||||
enabled: Some(true),
|
||||
..Default::default()
|
||||
}),
|
||||
&sandbox_policy,
|
||||
)?;
|
||||
|
||||
let (session, rx) = make_session_with_config_and_rx(move |config| {
|
||||
config.permissions.sandbox_policy = codex_config::Constrained::allow_any(sandbox_policy);
|
||||
config.permissions.network = Some(network_spec);
|
||||
})
|
||||
.await?;
|
||||
|
||||
let turn_context = session.new_default_turn().await;
|
||||
assert!(turn_context.network.is_some());
|
||||
|
||||
#[cfg(windows)]
|
||||
let command = r#"$val = $env:HTTP_PROXY; if ([string]::IsNullOrEmpty($val)) { $val = 'not-set' } ; [System.Console]::Write($val)"#.to_string();
|
||||
#[cfg(not(windows))]
|
||||
let command = r#"sh -c "printf '%s' \"${HTTP_PROXY:-not-set}\"""#.to_string();
|
||||
|
||||
execute_user_shell_command(
|
||||
Arc::clone(&session),
|
||||
turn_context,
|
||||
command,
|
||||
CancellationToken::new(),
|
||||
UserShellCommandMode::StandaloneTurn,
|
||||
)
|
||||
.await;
|
||||
|
||||
loop {
|
||||
let event = rx.recv().await.expect("channel open");
|
||||
if let EventMsg::ExecCommandEnd(event) = event.msg {
|
||||
assert_eq!(event.exit_code, 0);
|
||||
assert_eq!(event.stdout.trim(), "not-set");
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_base_instructions_no_user_content() {
|
||||
let prompt_with_apply_patch_instructions =
|
||||
@@ -3001,6 +3096,109 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
(session, turn_context)
|
||||
}
|
||||
|
||||
async fn make_session_with_config(
|
||||
mutator: impl FnOnce(&mut Config),
|
||||
) -> anyhow::Result<Arc<Session>> {
|
||||
let (session, _rx_event) = make_session_with_config_and_rx(mutator).await?;
|
||||
Ok(session)
|
||||
}
|
||||
|
||||
async fn make_session_with_config_and_rx(
|
||||
mutator: impl FnOnce(&mut Config),
|
||||
) -> anyhow::Result<(Arc<Session>, async_channel::Receiver<Event>)> {
|
||||
let codex_home = tempfile::tempdir().expect("create temp dir");
|
||||
let mut config = build_test_config(codex_home.path()).await;
|
||||
mutator(&mut config);
|
||||
let config = Arc::new(config);
|
||||
let auth_manager = AuthManager::from_auth_for_testing(CodexAuth::from_api_key("Test API Key"));
|
||||
let models_manager = Arc::new(ModelsManager::new(
|
||||
config.codex_home.to_path_buf(),
|
||||
auth_manager.clone(),
|
||||
/*model_catalog*/ None,
|
||||
CollaborationModesConfig::default(),
|
||||
));
|
||||
let model = ModelsManager::get_model_offline_for_tests(config.model.as_deref());
|
||||
let model_info = ModelsManager::construct_model_info_offline_for_tests(
|
||||
model.as_str(),
|
||||
&config.to_models_manager_config(),
|
||||
);
|
||||
let collaboration_mode = CollaborationMode {
|
||||
mode: ModeKind::Default,
|
||||
settings: Settings {
|
||||
model,
|
||||
reasoning_effort: config.model_reasoning_effort,
|
||||
developer_instructions: None,
|
||||
},
|
||||
};
|
||||
let session_configuration = SessionConfiguration {
|
||||
provider: config.model_provider.clone(),
|
||||
collaboration_mode,
|
||||
model_reasoning_summary: config.model_reasoning_summary,
|
||||
developer_instructions: config.developer_instructions.clone(),
|
||||
user_instructions: config.user_instructions.clone(),
|
||||
service_tier: None,
|
||||
personality: config.personality,
|
||||
base_instructions: config
|
||||
.base_instructions
|
||||
.clone()
|
||||
.unwrap_or_else(|| model_info.get_model_instructions(config.personality)),
|
||||
compact_prompt: config.compact_prompt.clone(),
|
||||
approval_policy: config.permissions.approval_policy.clone(),
|
||||
approvals_reviewer: config.approvals_reviewer,
|
||||
sandbox_policy: config.permissions.sandbox_policy.clone(),
|
||||
file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(),
|
||||
network_sandbox_policy: config.permissions.network_sandbox_policy,
|
||||
windows_sandbox_level: WindowsSandboxLevel::from_config(&config),
|
||||
cwd: config.cwd.clone(),
|
||||
codex_home: config.codex_home.clone(),
|
||||
thread_name: None,
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
session_source: SessionSource::Exec,
|
||||
dynamic_tools: Vec::new(),
|
||||
persist_extended_history: false,
|
||||
inherited_shell_snapshot: None,
|
||||
user_shell_override: None,
|
||||
};
|
||||
|
||||
let (tx_event, rx_event) = async_channel::unbounded();
|
||||
let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit);
|
||||
let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf()));
|
||||
let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager)));
|
||||
let skills_manager = Arc::new(SkillsManager::new(
|
||||
config.codex_home.clone(),
|
||||
/*bundled_skills_enabled*/ true,
|
||||
));
|
||||
|
||||
let session = Session::new(
|
||||
session_configuration,
|
||||
Arc::clone(&config),
|
||||
auth_manager,
|
||||
models_manager,
|
||||
Arc::new(ExecPolicyManager::default()),
|
||||
tx_event,
|
||||
agent_status_tx,
|
||||
InitialHistory::New,
|
||||
SessionSource::Exec,
|
||||
skills_manager,
|
||||
plugins_manager,
|
||||
mcp_manager,
|
||||
Arc::new(SkillsWatcher::noop()),
|
||||
AgentControl::default(),
|
||||
Some(Arc::new(
|
||||
codex_exec_server::Environment::create(/*exec_server_url*/ None)
|
||||
.await
|
||||
.expect("create environment"),
|
||||
)),
|
||||
/*analytics_events_client*/ None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok((session, rx_event))
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn notify_request_permissions_response_ignores_unmatched_call_id() {
|
||||
let (session, _turn_context) = make_session_and_context().await;
|
||||
|
||||
@@ -2282,7 +2282,11 @@ impl Config {
|
||||
}
|
||||
|
||||
pub fn managed_network_requirements_enabled(&self) -> bool {
|
||||
self.config_layer_stack
|
||||
!matches!(
|
||||
self.permissions.sandbox_policy.get(),
|
||||
SandboxPolicy::DangerFullAccess
|
||||
) && self
|
||||
.config_layer_stack
|
||||
.requirements_toml()
|
||||
.network
|
||||
.is_some()
|
||||
|
||||
@@ -163,7 +163,9 @@ pub(crate) async fn execute_user_shell_command(
|
||||
cwd: cwd.clone(),
|
||||
env: exec_env_map,
|
||||
exec_server_env_config: None,
|
||||
network: turn_context.network.clone(),
|
||||
// `/shell` is the explicit full-access escape hatch, so it must not
|
||||
// inherit a managed proxy from the surrounding session or turn.
|
||||
network: None,
|
||||
// TODO(zhao-oai): Now that we have ExecExpiration::Cancellation, we
|
||||
// should use that instead of an "arbitrarily large" timeout here.
|
||||
expiration: USER_SHELL_TIMEOUT_MS.into(),
|
||||
|
||||
@@ -2803,6 +2803,169 @@ allow_local_binding = true
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn network_approval_flow_survives_danger_full_access_session_start() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
fs::write(
|
||||
home.path().join("config.toml"),
|
||||
r#"default_permissions = "workspace"
|
||||
|
||||
[permissions.workspace.filesystem]
|
||||
":minimal" = "read"
|
||||
|
||||
[permissions.workspace.network]
|
||||
enabled = true
|
||||
mode = "limited"
|
||||
allow_local_binding = true
|
||||
"#,
|
||||
)?;
|
||||
let approval_policy = AskForApproval::OnFailure;
|
||||
let turn_sandbox_policy = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: Default::default(),
|
||||
network_access: true,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
};
|
||||
let mut builder = test_codex().with_home(home).with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config.permissions.sandbox_policy = Constrained::allow_any(SandboxPolicy::DangerFullAccess);
|
||||
let layers = config
|
||||
.config_layer_stack
|
||||
.get_layers(
|
||||
ConfigLayerStackOrdering::LowestPrecedenceFirst,
|
||||
/*include_disabled*/ true,
|
||||
)
|
||||
.into_iter()
|
||||
.cloned()
|
||||
.collect();
|
||||
let mut requirements = config.config_layer_stack.requirements().clone();
|
||||
requirements.network = Some(Sourced::new(
|
||||
NetworkConstraints {
|
||||
enabled: Some(true),
|
||||
allow_local_binding: Some(true),
|
||||
..Default::default()
|
||||
},
|
||||
RequirementSource::CloudRequirements,
|
||||
));
|
||||
let mut requirements_toml = config.config_layer_stack.requirements_toml().clone();
|
||||
requirements_toml.network = Some(NetworkRequirementsToml {
|
||||
enabled: Some(true),
|
||||
allow_local_binding: Some(true),
|
||||
..Default::default()
|
||||
});
|
||||
config.config_layer_stack = ConfigLayerStack::new(layers, requirements, requirements_toml)
|
||||
.expect("rebuild config layer stack with network requirements");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
assert!(
|
||||
!test.config.managed_network_requirements_enabled(),
|
||||
"expected managed network requirements to stay inactive in danger-full-access"
|
||||
);
|
||||
assert!(
|
||||
test.config.permissions.network.is_some(),
|
||||
"expected managed network proxy config to be present"
|
||||
);
|
||||
assert!(
|
||||
test.session_configured.network_proxy.is_none(),
|
||||
"expected session configured event to hide managed network proxy in danger-full-access"
|
||||
);
|
||||
|
||||
let call_id = "allow-network-after-yolo";
|
||||
let fetch_command = r#"python3 -c "import urllib.request; opener = urllib.request.build_opener(urllib.request.ProxyHandler()); print('OK:' + opener.open('http://codex-network-test.invalid', timeout=30).read().decode(errors='replace'))""#
|
||||
.to_string();
|
||||
let event = shell_event(
|
||||
call_id,
|
||||
&fetch_command,
|
||||
/*timeout_ms*/ 30_000,
|
||||
SandboxPermissions::UseDefault,
|
||||
)?;
|
||||
|
||||
let _ = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-network-after-yolo-1"),
|
||||
event,
|
||||
ev_completed("resp-network-after-yolo-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let _ = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-network-after-yolo-1", "done"),
|
||||
ev_completed("resp-network-after-yolo-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"allow-network-after-yolo",
|
||||
approval_policy,
|
||||
turn_sandbox_policy,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30);
|
||||
let approval = loop {
|
||||
let remaining = deadline
|
||||
.checked_duration_since(std::time::Instant::now())
|
||||
.expect("timed out waiting for network approval request");
|
||||
let event = wait_for_event_with_timeout(
|
||||
&test.codex,
|
||||
|event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
},
|
||||
remaining,
|
||||
)
|
||||
.await;
|
||||
match event {
|
||||
EventMsg::ExecApprovalRequest(approval) => {
|
||||
if approval.command.first().map(std::string::String::as_str)
|
||||
== Some("network-access")
|
||||
{
|
||||
break approval;
|
||||
}
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
}
|
||||
EventMsg::TurnComplete(_) => {
|
||||
panic!("expected network approval request before completion");
|
||||
}
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
};
|
||||
|
||||
let network_context = approval
|
||||
.network_approval_context
|
||||
.clone()
|
||||
.expect("expected network approval context");
|
||||
assert_eq!(network_context.protocol, NetworkApprovalProtocol::Http);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion(&test).await;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// todo(dylan) add ScenarioSpec support for rules
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(unix)]
|
||||
|
||||
Reference in New Issue
Block a user