mirror of
https://github.com/openai/codex.git
synced 2026-10-23 18:08:45 +00:00
Preserve foreign paths in managed network approvals (#39040)
## Why Unified exec could drop network approval registration when a selected environment used a Windows working directory on a non-Windows host because the trigger path could not be converted to a host absolute path. ## What changed - Keep network approval trigger working directories as `PathUri` values and render them with their inferred native path syntax for Guardian requests. - Fall back to the selected turn environment only when a trigger path can be converted to a host absolute path. - Reject selected environment working directories larger than 8 KiB at the thread-manager validation boundary. ## Testing - Cover managed-network approval registration for a foreign Windows working directory. - Verify that oversized working directories are rejected as invalid `thread/start` requests. GitOrigin-RevId: 646fbf12115a35a7e5cb04b511464797bee6647b
This commit is contained in:
@@ -704,6 +704,44 @@ async fn thread_start_rejects_relative_environment_cwd_as_invalid_request() -> R
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_rejects_oversized_environment_cwd_as_invalid_request() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.build_initialized()
|
||||
.await?;
|
||||
let environment_id = mcp.auto_env_params()?.environment_id;
|
||||
|
||||
let request_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
environments: Some(vec![TurnEnvironmentParams {
|
||||
environment_id,
|
||||
cwd: serde_json::from_value(json!(format!("C:\\{}", "x".repeat(9_000))))?,
|
||||
runtime_workspace_roots: None,
|
||||
}]),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let error: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(request_id)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
assert_eq!(error.id, RequestId::Integer(request_id));
|
||||
assert_eq!(error.error.code, INVALID_REQUEST_ERROR_CODE);
|
||||
assert_eq!(
|
||||
error.error.message,
|
||||
"turn environment working directory exceeds the maximum size"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_response_includes_loaded_instruction_sources() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
|
||||
@@ -7,6 +7,7 @@ use codex_protocol::approvals::NetworkApprovalProtocol;
|
||||
use codex_protocol::models::AdditionalPermissionProfile;
|
||||
use codex_protocol::request_permissions::RequestPermissionProfile;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use serde::Serialize;
|
||||
use serde_json::Value;
|
||||
|
||||
@@ -83,7 +84,7 @@ pub(crate) struct GuardianNetworkAccessTrigger {
|
||||
pub(crate) call_id: String,
|
||||
pub(crate) tool_name: String,
|
||||
pub(crate) command: Vec<String>,
|
||||
pub(crate) cwd: AbsolutePathBuf,
|
||||
pub(crate) cwd: PathUri,
|
||||
pub(crate) sandbox_permissions: crate::sandboxing::SandboxPermissions,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub(crate) additional_permissions: Option<AdditionalPermissionProfile>,
|
||||
@@ -330,14 +331,20 @@ pub(crate) fn guardian_approval_request_to_json(
|
||||
protocol,
|
||||
port,
|
||||
trigger,
|
||||
} => serialize_guardian_action(NetworkAccessApprovalAction {
|
||||
tool: "network_access",
|
||||
target,
|
||||
host,
|
||||
protocol: *protocol,
|
||||
port: *port,
|
||||
trigger: trigger.as_ref(),
|
||||
}),
|
||||
} => {
|
||||
let mut value = serialize_guardian_action(NetworkAccessApprovalAction {
|
||||
tool: "network_access",
|
||||
target,
|
||||
host,
|
||||
protocol: *protocol,
|
||||
port: *port,
|
||||
trigger: trigger.as_ref(),
|
||||
})?;
|
||||
if let Some(trigger) = trigger {
|
||||
value["trigger"]["cwd"] = trigger.cwd.inferred_native_path_string().into();
|
||||
}
|
||||
Ok(value)
|
||||
}
|
||||
GuardianApprovalRequest::McpToolCall {
|
||||
id: _,
|
||||
server,
|
||||
|
||||
@@ -1207,7 +1207,7 @@ async fn build_guardian_prompt_items_keeps_other_requests_generic() -> anyhow::R
|
||||
|
||||
#[test]
|
||||
fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_json::Result<()> {
|
||||
let cwd = test_path_buf("/repo").abs();
|
||||
let cwd = PathUri::parse("file:///C:/repo").expect("valid Windows path URI");
|
||||
let action = GuardianApprovalRequest::NetworkAccess {
|
||||
id: "network-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
@@ -1219,7 +1219,7 @@ fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_j
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: cwd.clone(),
|
||||
cwd,
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("Fetch the release metadata.".to_string()),
|
||||
@@ -1239,7 +1239,7 @@ fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_j
|
||||
"callId": "call-1",
|
||||
"toolName": "shell",
|
||||
"command": ["curl", "https://example.com"],
|
||||
"cwd": cwd.to_string_lossy().to_string(),
|
||||
"cwd": "C:\\repo",
|
||||
"sandboxPermissions": "use_default",
|
||||
"justification": "Fetch the release metadata.",
|
||||
},
|
||||
@@ -1253,7 +1253,7 @@ fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_j
|
||||
async fn build_guardian_prompt_items_explains_network_access_review_scope() -> anyhow::Result<()> {
|
||||
let (session, turn) = guardian_test_session_and_turn_with_base_url("http://localhost").await;
|
||||
seed_guardian_parent_history(&session, &turn).await;
|
||||
let cwd = test_path_buf("/repo").abs();
|
||||
let cwd = PathUri::from_abs_path(&test_path_buf("/repo").abs());
|
||||
|
||||
let prompt = build_guardian_prompt_items(
|
||||
session.as_ref(),
|
||||
@@ -1382,7 +1382,7 @@ fn guardian_request_target_item_id_omits_network_access_trigger_call_id() {
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: test_path_buf("/repo").abs(),
|
||||
cwd: PathUri::from_abs_path(&test_path_buf("/repo").abs()),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: None,
|
||||
|
||||
@@ -98,6 +98,8 @@ use tracing::instrument;
|
||||
use tracing::warn;
|
||||
|
||||
const THREAD_CREATED_CHANNEL_CAPACITY: usize = 1024;
|
||||
// Reject pathological selected cwd values at the environment-selection boundary.
|
||||
const MAX_TURN_ENVIRONMENT_CWD_BYTES: usize = 8 * 1024;
|
||||
|
||||
/// Test-only override for enabling thread-manager behaviors used by integration
|
||||
/// tests.
|
||||
@@ -724,6 +726,12 @@ impl ThreadManager {
|
||||
) -> CodexResult<()> {
|
||||
let mut environment_ids = HashSet::with_capacity(environments.len());
|
||||
for environment in environments {
|
||||
if environment.cwd.inferred_native_path_string().len() > MAX_TURN_ENVIRONMENT_CWD_BYTES
|
||||
{
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
"turn environment working directory exceeds the maximum size".to_string(),
|
||||
));
|
||||
}
|
||||
if !environment_ids.insert(environment.environment_id.as_str()) {
|
||||
return Err(CodexErr::InvalidRequest(format!(
|
||||
"duplicate turn environment id `{}`",
|
||||
|
||||
@@ -699,8 +699,11 @@ impl NetworkApprovalService {
|
||||
let command = owner_call
|
||||
.as_ref()
|
||||
.map_or_else(|| prompt_command.join(" "), |call| call.command.clone());
|
||||
let cwd = if let Some(owner_call) = owner_call.as_ref() {
|
||||
owner_call.trigger.cwd.clone()
|
||||
let cwd = if let Some(cwd) = owner_call
|
||||
.as_ref()
|
||||
.and_then(|owner_call| owner_call.trigger.cwd.to_abs_path().ok())
|
||||
{
|
||||
cwd
|
||||
} else {
|
||||
turn_context
|
||||
.environments
|
||||
|
||||
@@ -4,6 +4,7 @@ use codex_network_proxy::BlockedRequestArgs;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::test_path_buf;
|
||||
use futures::poll;
|
||||
@@ -481,7 +482,7 @@ async fn register_call_with_default_shell_trigger(
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell_command".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
cwd: PathUri::from_abs_path(&test_path_buf("/tmp").abs()),
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: None,
|
||||
@@ -503,7 +504,7 @@ async fn active_call_preserves_triggering_command_context() {
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: "shell_command".to_string(),
|
||||
command: vec!["curl".to_string(), "https://example.com".to_string()],
|
||||
cwd: test_path_buf("/repo").abs(),
|
||||
cwd: PathUri::parse("file:///C:/repo").expect("valid Windows path URI"),
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: Some("fetch release metadata".to_string()),
|
||||
|
||||
@@ -179,7 +179,7 @@ impl ToolRuntime<ShellRequest, ExecToolCallOutput> for ShellRuntime {
|
||||
call_id: ctx.call_id.clone(),
|
||||
tool_name: flat_tool_name(&ctx.tool_name).into_owned(),
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.clone(),
|
||||
cwd: PathUri::from_abs_path(&req.cwd),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
additional_permissions: req.additional_permissions.clone(),
|
||||
justification: req.justification.clone(),
|
||||
|
||||
@@ -230,7 +230,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRunt
|
||||
call_id: ctx.call_id.clone(),
|
||||
tool_name: flat_tool_name(&ctx.tool_name).into_owned(),
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.to_abs_path().ok()?,
|
||||
cwd: req.cwd.clone(),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
additional_permissions: req.additional_permissions.clone(),
|
||||
justification: req.justification.clone(),
|
||||
|
||||
@@ -539,6 +539,7 @@ async fn serve_exec_with_pushed_events(
|
||||
#[test_case(PushedExecScenario::ReplayGap, false, false, false ; "truncated_event_replay")]
|
||||
#[test_case(PushedExecScenario::Complete, true, true, false ; "managed_network_uses_executor_proxy_launch")]
|
||||
#[test_case(PushedExecScenario::Complete, true, false, false ; "strict_managed_allowlist_omits_policy_callbacks")]
|
||||
#[cfg_attr(not(windows), test_case(PushedExecScenario::Complete, true, true, true ; "foreign_windows_managed_network_preserves_approval_registration"))]
|
||||
#[cfg_attr(not(windows), test_case(PushedExecScenario::Complete, false, false, true ; "foreign_windows_workspace_sandbox"))]
|
||||
#[test_case(PushedExecScenario::ElevatedPowerShell, false, false, true ; "windows_elevated_powershell_disables_profile")]
|
||||
#[cfg_attr(not(windows), test_case(PushedExecScenario::SandboxedInterceptedPatch, false, false, true ; "foreign_windows_intercepted_patch_is_sandboxed"))]
|
||||
|
||||
Reference in New Issue
Block a user