From e8afaed5029fdc16cee53c2007930b1d34401b79 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 20 Feb 2026 10:39:55 -0800 Subject: [PATCH] Refactor network approvals to host/protocol/port scope (#12140) ## Summary Simplify network approvals by removing per-attempt proxy correlation and moving to session-level approval dedupe keyed by (host, protocol, port). Instead of encoding attempt IDs into proxy credentials/URLs, we now treat approvals as a destination policy decision. - Concurrent calls to the same destination share one approval prompt. - Different destinations (or same host on different ports) get separate prompts. - Allow once approves the current queued request group only. - Allow for session caches that (host, protocol, port) and auto-allows future matching requests. - Never policy continues to deny without prompting. Example: - 3 calls: - a.com (line 443) - b.com (line 443) - a.com (line 443) => 2 prompts total (a, b), second a waits on the first decision. - a.com:80 is treated separately from a.com line 443 ## Testing - `just fmt` (in `codex-rs`) - `cargo test -p codex-core tools::network_approval::tests` - `cargo test -p codex-core` (unit tests pass; existing integration-suite failures remain in this environment) --- codex-rs/Cargo.lock | 1 - ...CommandExecutionRequestApprovalParams.json | 35 + .../schema/json/ServerRequest.json | 35 + .../codex_app_server_protocol.schemas.json | 11 + .../CommandExecutionRequestApprovalParams.ts | 5 + .../typescript/v2/NetworkApprovalContext.ts | 6 + .../typescript/v2/NetworkApprovalProtocol.ts | 5 + .../schema/typescript/v2/index.ts | 2 + .../app-server-protocol/src/protocol/v2.rs | 32 + codex-rs/app-server-test-client/src/lib.rs | 4 + codex-rs/app-server/README.md | 2 +- .../app-server/src/bespoke_event_handling.rs | 62 +- .../app-server/src/codex_message_processor.rs | 1 - codex-rs/core/src/codex.rs | 2 - codex-rs/core/src/exec.rs | 24 +- codex-rs/core/src/network_policy_decision.rs | 2 - codex-rs/core/src/sandboxing/mod.rs | 2 - codex-rs/core/src/tasks/user_shell.rs | 1 - .../core/src/tools/handlers/apply_patch.rs | 2 - codex-rs/core/src/tools/handlers/shell.rs | 3 - codex-rs/core/src/tools/network_approval.rs | 761 ++++++++---------- codex-rs/core/src/tools/orchestrator.rs | 3 - codex-rs/core/src/tools/runtimes/shell.rs | 5 +- .../core/src/tools/runtimes/unified_exec.rs | 4 +- codex-rs/core/src/tools/sandboxing.rs | 1 - .../core/src/unified_exec/async_watcher.rs | 37 - codex-rs/core/src/unified_exec/mod.rs | 2 +- .../core/src/unified_exec/process_manager.rs | 64 +- codex-rs/core/tests/suite/exec.rs | 1 - .../exec-server/src/posix/escalate_server.rs | 1 - .../linux-sandbox/tests/suite/landlock.rs | 2 - codex-rs/network-proxy/Cargo.toml | 1 - codex-rs/network-proxy/src/http_proxy.rs | 50 -- codex-rs/network-proxy/src/lib.rs | 1 - codex-rs/network-proxy/src/metadata.rs | 50 -- codex-rs/network-proxy/src/network_policy.rs | 7 - codex-rs/network-proxy/src/proxy.rs | 36 +- codex-rs/network-proxy/src/runtime.rs | 14 +- codex-rs/network-proxy/src/socks5.rs | 8 - .../tui/src/bottom_pane/approval_overlay.rs | 24 +- 40 files changed, 570 insertions(+), 739 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalContext.ts create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalProtocol.ts delete mode 100644 codex-rs/network-proxy/src/metadata.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 87f0e4cd4..ccdfddb3a 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1986,7 +1986,6 @@ version = "0.0.0" dependencies = [ "anyhow", "async-trait", - "base64 0.22.1", "clap", "codex-utils-absolute-path", "codex-utils-rustls-provider", diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 193870195..b7ad3fe31 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -110,6 +110,30 @@ "type": "object" } ] + }, + "NetworkApprovalContext": { + "properties": { + "host": { + "type": "string" + }, + "protocol": { + "$ref": "#/definitions/NetworkApprovalProtocol" + } + }, + "required": [ + "host", + "protocol" + ], + "type": "object" + }, + "NetworkApprovalProtocol": { + "enum": [ + "http", + "https", + "socks5Tcp", + "socks5Udp" + ], + "type": "string" } }, "properties": { @@ -147,6 +171,17 @@ "itemId": { "type": "string" }, + "networkApprovalContext": { + "anyOf": [ + { + "$ref": "#/definitions/NetworkApprovalContext" + }, + { + "type": "null" + } + ], + "description": "Optional context for managed-network approval prompts." + }, "proposedExecpolicyAmendment": { "description": "Optional proposed execpolicy amendment to allow similar commands without prompting.", "items": { diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 6be3bebc5..865912058 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -213,6 +213,17 @@ "itemId": { "type": "string" }, + "networkApprovalContext": { + "anyOf": [ + { + "$ref": "#/definitions/NetworkApprovalContext" + }, + { + "type": "null" + } + ], + "description": "Optional context for managed-network approval prompts." + }, "proposedExecpolicyAmendment": { "description": "Optional proposed execpolicy amendment to allow similar commands without prompting.", "items": { @@ -419,6 +430,30 @@ ], "type": "object" }, + "NetworkApprovalContext": { + "properties": { + "host": { + "type": "string" + }, + "protocol": { + "$ref": "#/definitions/NetworkApprovalProtocol" + } + }, + "required": [ + "host", + "protocol" + ], + "type": "object" + }, + "NetworkApprovalProtocol": { + "enum": [ + "http", + "https", + "socks5Tcp", + "socks5Udp" + ], + "type": "string" + }, "ParsedCommand": { "oneOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index df73bf2c4..e1e0137b7 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -2192,6 +2192,17 @@ "itemId": { "type": "string" }, + "networkApprovalContext": { + "anyOf": [ + { + "$ref": "#/definitions/NetworkApprovalContext" + }, + { + "type": "null" + } + ], + "description": "Optional context for managed-network approval prompts." + }, "proposedExecpolicyAmendment": { "description": "Optional proposed execpolicy amendment to allow similar commands without prompting.", "items": { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts index 290ce5669..4021e50a6 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts @@ -3,6 +3,7 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { CommandAction } from "./CommandAction"; import type { ExecPolicyAmendment } from "./ExecPolicyAmendment"; +import type { NetworkApprovalContext } from "./NetworkApprovalContext"; export type CommandExecutionRequestApprovalParams = { threadId: string, turnId: string, itemId: string, /** @@ -19,6 +20,10 @@ approvalId?: string | null, * Optional explanatory reason (e.g. request for network access). */ reason?: string | null, +/** + * Optional context for managed-network approval prompts. + */ +networkApprovalContext?: NetworkApprovalContext | null, /** * The command to be executed. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalContext.ts b/codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalContext.ts new file mode 100644 index 000000000..b4b78e473 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalContext.ts @@ -0,0 +1,6 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { NetworkApprovalProtocol } from "./NetworkApprovalProtocol"; + +export type NetworkApprovalContext = { host: string, protocol: NetworkApprovalProtocol, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalProtocol.ts b/codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalProtocol.ts new file mode 100644 index 000000000..9dd4066fd --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/NetworkApprovalProtocol.ts @@ -0,0 +1,5 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +export type NetworkApprovalProtocol = "http" | "https" | "socks5Tcp" | "socks5Udp"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index e595d0ce0..db94051fa 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -100,6 +100,8 @@ export type { ModelListResponse } from "./ModelListResponse"; export type { ModelRerouteReason } from "./ModelRerouteReason"; export type { ModelReroutedNotification } from "./ModelReroutedNotification"; export type { NetworkAccess } from "./NetworkAccess"; +export type { NetworkApprovalContext } from "./NetworkApprovalContext"; +export type { NetworkApprovalProtocol } from "./NetworkApprovalProtocol"; export type { NetworkRequirements } from "./NetworkRequirements"; export type { OverriddenMetadata } from "./OverriddenMetadata"; export type { PatchApplyStatus } from "./PatchApplyStatus"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index fb2f86931..dfb185b40 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -5,6 +5,8 @@ use crate::protocol::common::AuthMode; use codex_experimental_api_macros::ExperimentalApi; use codex_protocol::account::PlanType; use codex_protocol::approvals::ExecPolicyAmendment as CoreExecPolicyAmendment; +use codex_protocol::approvals::NetworkApprovalContext as CoreNetworkApprovalContext; +use codex_protocol::approvals::NetworkApprovalProtocol as CoreNetworkApprovalProtocol; use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::CollaborationModeMask; use codex_protocol::config_types::ForcedLoginMethod; @@ -650,6 +652,32 @@ pub enum CommandExecutionApprovalDecision { Cancel, } +v2_enum_from_core! { + pub enum NetworkApprovalProtocol from CoreNetworkApprovalProtocol { + Http, + Https, + Socks5Tcp, + Socks5Udp, + } +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct NetworkApprovalContext { + pub host: String, + pub protocol: NetworkApprovalProtocol, +} + +impl From for NetworkApprovalContext { + fn from(value: CoreNetworkApprovalContext) -> Self { + Self { + host: value.host, + protocol: value.protocol.into(), + } + } +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -3331,6 +3359,10 @@ pub struct CommandExecutionRequestApprovalParams { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] pub reason: Option, + /// Optional context for managed-network approval prompts. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional = nullable)] + pub network_approval_context: Option, /// The command to be executed. #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index 7b846c80f..a1d10be33 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -1491,6 +1491,7 @@ impl CodexClient { item_id, approval_id, reason, + network_approval_context, command, cwd, command_actions, @@ -1506,6 +1507,9 @@ impl CodexClient { if let Some(reason) = reason.as_deref() { println!("< reason: {reason}"); } + if let Some(network_approval_context) = network_approval_context.as_ref() { + println!("< network approval context: {network_approval_context:?}"); + } if let Some(command) = command.as_deref() { println!("< command: {command}"); } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 3246bebd8..95ab4e43e 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -667,7 +667,7 @@ Certain actions (shell commands or modifying files) may require explicit user ap Order of messages: 1. `item/started` — shows the pending `commandExecution` item with `command`, `cwd`, and other fields so you can render the proposed action. -2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, optionally `approvalId` (for subcommand callbacks), `reason`, plus `command`, `cwd`, and `commandActions` for friendly display. +2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, optionally `approvalId` (for subcommand callbacks), and `reason`. For normal command approvals, it also includes `command`, `cwd`, and `commandActions` for friendly display. For network-only approvals, those command fields may be omitted and `networkApprovalContext` is provided instead. 3. Client response — `{ "decision": "accept", "acceptSettings": { "forSession": false } }` or `{ "decision": "decline" }`. 4. `item/completed` — final `commandExecution` item with `status: "completed" | "failed" | "declined"` and execution output. Render this as the authoritative result. diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index ea4fa691d..fca2bcc1c 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -44,6 +44,7 @@ use codex_app_server_protocol::McpToolCallError; use codex_app_server_protocol::McpToolCallResult; use codex_app_server_protocol::McpToolCallStatus; use codex_app_server_protocol::ModelReroutedNotification; +use codex_app_server_protocol::NetworkApprovalContext as V2NetworkApprovalContext; use codex_app_server_protocol::PatchApplyStatus; use codex_app_server_protocol::PlanDeltaNotification; use codex_app_server_protocol::RawResponseItemCompletedNotification; @@ -106,6 +107,17 @@ use tracing::error; type JsonValue = serde_json::Value; +enum CommandExecutionApprovalPresentation { + Network(V2NetworkApprovalContext), + Command(CommandExecutionCompletionItem), +} + +struct CommandExecutionCompletionItem { + command: String, + cwd: PathBuf, + command_actions: Vec, +} + #[allow(clippy::too_many_arguments)] pub(crate) async fn apply_bespoke_event_handling( event: Event, @@ -245,6 +257,7 @@ pub(crate) async fn apply_bespoke_event_handling( command, cwd, reason, + network_approval_context, proposed_execpolicy_amendment, parsed_cmd, .. @@ -280,7 +293,32 @@ pub(crate) async fn apply_bespoke_event_handling( .cloned() .map(V2ParsedCommand::from) .collect::>(); - let command_string = shlex_join(&command); + let presentation = if let Some(network_approval_context) = + network_approval_context.map(V2NetworkApprovalContext::from) + { + CommandExecutionApprovalPresentation::Network(network_approval_context) + } else { + let command_string = shlex_join(&command); + let completion_item = CommandExecutionCompletionItem { + command: command_string, + cwd: cwd.clone(), + command_actions: command_actions.clone(), + }; + CommandExecutionApprovalPresentation::Command(completion_item) + }; + let (network_approval_context, command, cwd, command_actions, completion_item) = + match presentation { + CommandExecutionApprovalPresentation::Network( + network_approval_context, + ) => (Some(network_approval_context), None, None, None, None), + CommandExecutionApprovalPresentation::Command(completion_item) => ( + None, + Some(completion_item.command.clone()), + Some(completion_item.cwd.clone()), + Some(completion_item.command_actions.clone()), + Some(completion_item), + ), + }; let proposed_execpolicy_amendment_v2 = proposed_execpolicy_amendment.map(V2ExecPolicyAmendment::from); @@ -290,9 +328,10 @@ pub(crate) async fn apply_bespoke_event_handling( item_id: call_id.clone(), approval_id: approval_id.clone(), reason, - command: Some(command_string.clone()), - cwd: Some(cwd.clone()), - command_actions: Some(command_actions.clone()), + network_approval_context, + command, + cwd, + command_actions, proposed_execpolicy_amendment: proposed_execpolicy_amendment_v2, }; let rx = outgoing @@ -306,9 +345,7 @@ pub(crate) async fn apply_bespoke_event_handling( conversation_id, approval_id, call_id, - command_string, - cwd, - command_actions, + completion_item, rx, conversation, outgoing, @@ -1790,9 +1827,7 @@ async fn on_command_execution_request_approval_response( conversation_id: ThreadId, approval_id: Option, item_id: String, - command: String, - cwd: PathBuf, - command_actions: Vec, + completion_item: Option, receiver: oneshot::Receiver, conversation: Arc, outgoing: ThreadScopedOutgoingMessageSender, @@ -1864,15 +1899,16 @@ async fn on_command_execution_request_approval_response( if let Some(status) = completion_status && !suppress_subcommand_completion_item + && let Some(completion_item) = completion_item { complete_command_execution_item( conversation_id, event_turn_id.clone(), item_id.clone(), - command.clone(), - cwd.clone(), + completion_item.command, + completion_item.cwd, None, - command_actions.clone(), + completion_item.command_actions, status, &outgoing, ) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 60c9c4638..24c89edaf 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1763,7 +1763,6 @@ impl CodexMessageProcessor { network: started_network_proxy .as_ref() .map(codex_core::config::StartedNetworkProxy::proxy), - network_attempt_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level, justification: None, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index a384ffc00..b5614cced 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -8347,7 +8347,6 @@ mod tests { expiration: timeout_ms.into(), env: HashMap::new(), network: None, - network_attempt_id: None, sandbox_permissions, windows_sandbox_level: turn_context.windows_sandbox_level, justification: Some("test".to_string()), @@ -8361,7 +8360,6 @@ mod tests { expiration: timeout_ms.into(), env: HashMap::new(), network: None, - network_attempt_id: None, windows_sandbox_level: turn_context.windows_sandbox_level, justification: params.justification.clone(), arg0: None, diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 4e1220b87..1e0e59d4e 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -15,7 +15,6 @@ use tokio::io::AsyncReadExt; use tokio::io::BufReader; use tokio::process::Child; use tokio_util::sync::CancellationToken; -use uuid::Uuid; use crate::error::CodexErr; use crate::error::Result; @@ -67,7 +66,6 @@ pub struct ExecParams { pub expiration: ExecExpiration, pub env: HashMap, pub network: Option, - pub network_attempt_id: Option, pub sandbox_permissions: SandboxPermissions, pub windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel, pub justification: Option, @@ -186,15 +184,13 @@ pub async fn process_exec_tool_call( mut env, expiration, network, - network_attempt_id, sandbox_permissions, windows_sandbox_level, justification, arg0: _, } = params; - let network_attempt_id = network_attempt_id.map(|attempt_id| attempt_id.to_string()); if let Some(network) = network.as_ref() { - network.apply_to_env_for_attempt(&mut env, network_attempt_id.as_deref()); + network.apply_to_env(&mut env); } let (program, args) = command.split_first().ok_or_else(|| { CodexErr::Io(io::Error::new( @@ -242,7 +238,6 @@ pub(crate) async fn execute_exec_env( cwd, env, network, - network_attempt_id, expiration, sandbox, windows_sandbox_level, @@ -251,18 +246,12 @@ pub(crate) async fn execute_exec_env( arg0, } = env; - let network_attempt_id = match network_attempt_id.as_deref() { - Some(attempt_id) => Uuid::parse_str(attempt_id).ok(), - None => network.as_ref().map(|_| Uuid::new_v4()), - }; - let params = ExecParams { command, cwd, expiration, env, network: network.clone(), - network_attempt_id, sandbox_permissions, windows_sandbox_level, justification, @@ -356,14 +345,12 @@ async fn exec_windows_sandbox( cwd, mut env, network, - network_attempt_id, expiration, windows_sandbox_level, .. } = params; - let network_attempt_id = network_attempt_id.map(|attempt_id| attempt_id.to_string()); if let Some(network) = network.as_ref() { - network.apply_to_env_for_attempt(&mut env, network_attempt_id.as_deref()); + network.apply_to_env(&mut env); } // TODO(iceweasel-oai): run_windows_sandbox_capture should support all @@ -717,16 +704,13 @@ async fn exec( cwd, mut env, network, - network_attempt_id, arg0, expiration, windows_sandbox_level: _, .. } = params; - let network_attempt_id = network_attempt_id.map(|attempt_id| attempt_id.to_string()); - if let Some(network) = network.as_ref() { - network.apply_to_env_for_attempt(&mut env, network_attempt_id.as_deref()); + network.apply_to_env(&mut env); } let (program, args) = command.split_first().ok_or_else(|| { @@ -1137,7 +1121,6 @@ mod tests { expiration: 500.into(), env, network: None, - network_attempt_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled, justification: None, @@ -1191,7 +1174,6 @@ mod tests { expiration: ExecExpiration::Cancellation(cancel_token), env, network: None, - network_attempt_id: None, sandbox_permissions: SandboxPermissions::UseDefault, windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled, justification: None, diff --git a/codex-rs/core/src/network_policy_decision.rs b/codex-rs/core/src/network_policy_decision.rs index 3da6d34d6..81ba7db82 100644 --- a/codex-rs/core/src/network_policy_decision.rs +++ b/codex-rs/core/src/network_policy_decision.rs @@ -220,7 +220,6 @@ mod tests { method: Some("GET".to_string()), mode: None, protocol: "http".to_string(), - attempt_id: Some("attempt-1".to_string()), decision: Some("ask".to_string()), source: Some("decider".to_string()), port: Some(80), @@ -238,7 +237,6 @@ mod tests { method: Some("GET".to_string()), mode: None, protocol: "http".to_string(), - attempt_id: Some("attempt-1".to_string()), decision: Some("deny".to_string()), source: Some("baseline_policy".to_string()), port: Some(80), diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 4bf8f6609..f9c3c171e 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -46,7 +46,6 @@ pub struct ExecRequest { pub cwd: PathBuf, pub env: HashMap, pub network: Option, - pub network_attempt_id: Option, pub expiration: ExecExpiration, pub sandbox: SandboxType, pub windows_sandbox_level: WindowsSandboxLevel, @@ -222,7 +221,6 @@ impl SandboxManager { cwd: spec.cwd, env, network: network.cloned(), - network_attempt_id: None, expiration: spec.expiration, sandbox, windows_sandbox_level, diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index 80112c61c..d356193f5 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -151,7 +151,6 @@ pub(crate) async fn execute_user_shell_command( Some(session.conversation_id), ), network: turn_context.network.clone(), - network_attempt_id: 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(), diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 49c679e52..8f14fcced 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -143,7 +143,6 @@ impl ToolHandler for ApplyPatchHandler { turn: turn.as_ref(), call_id: call_id.clone(), tool_name: tool_name.to_string(), - network_attempt_id: None, }; let out = orchestrator .run(&mut runtime, &req, &tool_ctx, &turn, turn.approval_policy) @@ -234,7 +233,6 @@ pub(crate) async fn intercept_apply_patch( turn, call_id: call_id.to_string(), tool_name: tool_name.to_string(), - network_attempt_id: None, }; let out = orchestrator .run(&mut runtime, &req, &tool_ctx, turn, turn.approval_policy) diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 31bf3f129..8ca6de0d8 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -54,7 +54,6 @@ impl ShellHandler { expiration: params.timeout_ms.into(), env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), network: turn_context.network.clone(), - network_attempt_id: None, sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), windows_sandbox_level: turn_context.windows_sandbox_level, justification: params.justification.clone(), @@ -84,7 +83,6 @@ impl ShellCommandHandler { expiration: params.timeout_ms.into(), env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), network: turn_context.network.clone(), - network_attempt_id: None, sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), windows_sandbox_level: turn_context.windows_sandbox_level, justification: params.justification.clone(), @@ -327,7 +325,6 @@ impl ShellHandler { turn: turn.as_ref(), call_id: call_id.clone(), tool_name, - network_attempt_id: None, }; let out = orchestrator .run(&mut runtime, &req, &tool_ctx, &turn, turn.approval_policy) diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index d5643e4fe..4a893f74e 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -10,12 +10,14 @@ use codex_network_proxy::NetworkProtocol; use codex_network_proxy::NetworkProxy; use codex_protocol::approvals::NetworkApprovalContext; use codex_protocol::approvals::NetworkApprovalProtocol; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; +use indexmap::IndexMap; use std::collections::HashMap; use std::collections::HashSet; -use std::path::PathBuf; use std::sync::Arc; use tokio::sync::Mutex; +use tokio::sync::Notify; use tokio::sync::RwLock; use uuid::Uuid; @@ -27,168 +29,207 @@ pub(crate) enum NetworkApprovalMode { #[derive(Clone, Debug)] pub(crate) struct NetworkApprovalSpec { - pub command: Vec, - pub cwd: PathBuf, pub network: Option, pub mode: NetworkApprovalMode, } #[derive(Clone, Debug)] pub(crate) struct DeferredNetworkApproval { - attempt_id: String, + registration_id: String, } impl DeferredNetworkApproval { - pub(crate) fn attempt_id(&self) -> &str { - &self.attempt_id + pub(crate) fn registration_id(&self) -> &str { + &self.registration_id } } #[derive(Debug)] pub(crate) struct ActiveNetworkApproval { - attempt_id: Option, + registration_id: Option, mode: NetworkApprovalMode, } impl ActiveNetworkApproval { - pub(crate) fn attempt_id(&self) -> Option<&str> { - self.attempt_id.as_deref() - } - pub(crate) fn mode(&self) -> NetworkApprovalMode { self.mode } pub(crate) fn into_deferred(self) -> Option { - match (self.mode, self.attempt_id) { - (NetworkApprovalMode::Deferred, Some(attempt_id)) => { - Some(DeferredNetworkApproval { attempt_id }) + match (self.mode, self.registration_id) { + (NetworkApprovalMode::Deferred, Some(registration_id)) => { + Some(DeferredNetworkApproval { registration_id }) } _ => None, } } } +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +struct HostApprovalKey { + host: String, + protocol: &'static str, + port: u16, +} + +impl HostApprovalKey { + fn from_request(request: &NetworkPolicyRequest, protocol: NetworkApprovalProtocol) -> Self { + Self { + host: request.host.to_ascii_lowercase(), + protocol: protocol_key_label(protocol), + port: request.port, + } + } +} + +fn protocol_key_label(protocol: NetworkApprovalProtocol) -> &'static str { + match protocol { + NetworkApprovalProtocol::Http => "http", + NetworkApprovalProtocol::Https => "https", + NetworkApprovalProtocol::Socks5Tcp => "socks5-tcp", + NetworkApprovalProtocol::Socks5Udp => "socks5-udp", + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum PendingApprovalDecision { + AllowOnce, + AllowForSession, + Deny, +} + #[derive(Clone, Debug, Eq, PartialEq)] -pub(crate) enum NetworkApprovalOutcome { +enum NetworkApprovalOutcome { DeniedByUser, DeniedByPolicy(String), } -struct NetworkApprovalAttempt { - turn_id: String, - call_id: String, - command: Vec, - cwd: PathBuf, - approved_hosts: Mutex>, - outcome: Mutex>, +fn allows_network_prompt(policy: AskForApproval) -> bool { + !matches!(policy, AskForApproval::Never) +} + +impl PendingApprovalDecision { + fn to_network_decision(self) -> NetworkDecision { + match self { + Self::AllowOnce | Self::AllowForSession => NetworkDecision::Allow, + Self::Deny => NetworkDecision::deny("not_allowed"), + } + } +} + +struct PendingHostApproval { + decision: Mutex>, + notify: Notify, +} + +impl PendingHostApproval { + fn new() -> Self { + Self { + decision: Mutex::new(None), + notify: Notify::new(), + } + } + + async fn wait_for_decision(&self) -> PendingApprovalDecision { + loop { + let notified = self.notify.notified(); + if let Some(decision) = *self.decision.lock().await { + return decision; + } + notified.await; + } + } + + async fn set_decision(&self, decision: PendingApprovalDecision) { + { + let mut current = self.decision.lock().await; + *current = Some(decision); + } + self.notify.notify_waiters(); + } +} + +struct ActiveNetworkApprovalCall { + registration_id: String, } pub(crate) struct NetworkApprovalService { - attempts: Mutex>>, - session_approved_hosts: Mutex>, + active_calls: Mutex>>, + call_outcomes: Mutex>, + pending_host_approvals: Mutex>>, + session_approved_hosts: Mutex>, } impl Default for NetworkApprovalService { fn default() -> Self { Self { - attempts: Mutex::new(HashMap::new()), + active_calls: Mutex::new(IndexMap::new()), + call_outcomes: Mutex::new(HashMap::new()), + pending_host_approvals: Mutex::new(HashMap::new()), session_approved_hosts: Mutex::new(HashSet::new()), } } } impl NetworkApprovalService { - pub(crate) async fn register_attempt( - &self, - attempt_id: String, - turn_id: String, - call_id: String, - command: Vec, - cwd: PathBuf, - ) { - let mut attempts = self.attempts.lock().await; - attempts.insert( - attempt_id, - Arc::new(NetworkApprovalAttempt { - turn_id, - call_id, - command, - cwd, - approved_hosts: Mutex::new(HashSet::new()), - outcome: Mutex::new(None), - }), - ); + async fn register_call(&self, registration_id: String) { + let mut active_calls = self.active_calls.lock().await; + let key = registration_id.clone(); + active_calls.insert(key, Arc::new(ActiveNetworkApprovalCall { registration_id })); } - pub(crate) async fn unregister_attempt(&self, attempt_id: &str) { - let mut attempts = self.attempts.lock().await; - attempts.remove(attempt_id); + pub(crate) async fn unregister_call(&self, registration_id: &str) { + let mut active_calls = self.active_calls.lock().await; + active_calls.shift_remove(registration_id); + let mut call_outcomes = self.call_outcomes.lock().await; + call_outcomes.remove(registration_id); } - pub(crate) async fn take_outcome(&self, attempt_id: &str) -> Option { - let attempt = { - let attempts = self.attempts.lock().await; - attempts.get(attempt_id).cloned() - }?; - let mut outcome = attempt.outcome.lock().await; - outcome.take() - } - - pub(crate) async fn take_user_denial_outcome(&self, attempt_id: &str) -> bool { - let attempt = { - let attempts = self.attempts.lock().await; - attempts.get(attempt_id).cloned() - }; - let Some(attempt) = attempt else { - return false; - }; - let mut outcome = attempt.outcome.lock().await; - if matches!(outcome.as_ref(), Some(NetworkApprovalOutcome::DeniedByUser)) { - outcome.take(); - return true; - } - false - } - - async fn resolve_attempt_for_request( - &self, - request: &NetworkPolicyRequest, - ) -> Option> { - let attempts = self.attempts.lock().await; - - if let Some(attempt_id) = request.attempt_id.as_deref() { - if let Some(attempt) = attempts.get(attempt_id).cloned() { - return Some(attempt); - } - return None; - } - - if attempts.len() == 1 { - return attempts.values().next().cloned(); + async fn resolve_single_active_call(&self) -> Option> { + let active_calls = self.active_calls.lock().await; + if active_calls.len() == 1 { + return active_calls.values().next().cloned(); } None } - async fn resolve_attempt_for_blocked_request( + async fn get_or_create_pending_approval( &self, - blocked: &BlockedRequest, - ) -> Option> { - let attempts = self.attempts.lock().await; - - if let Some(attempt_id) = blocked.attempt_id.as_deref() { - if let Some(attempt) = attempts.get(attempt_id).cloned() { - return Some(attempt); - } - return None; + key: HostApprovalKey, + ) -> (Arc, bool) { + let mut pending = self.pending_host_approvals.lock().await; + if let Some(existing) = pending.get(&key).cloned() { + return (existing, false); } - if attempts.len() == 1 { - return attempts.values().next().cloned(); - } + let created = Arc::new(PendingHostApproval::new()); + pending.insert(key, Arc::clone(&created)); + (created, true) + } - None + async fn record_outcome_for_single_active_call(&self, outcome: NetworkApprovalOutcome) { + let Some(owner_call) = self.resolve_single_active_call().await else { + return; + }; + self.record_call_outcome(&owner_call.registration_id, outcome) + .await; + } + + async fn take_call_outcome(&self, registration_id: &str) -> Option { + let mut call_outcomes = self.call_outcomes.lock().await; + call_outcomes.remove(registration_id) + } + + async fn record_call_outcome(&self, registration_id: &str, outcome: NetworkApprovalOutcome) { + let mut call_outcomes = self.call_outcomes.lock().await; + if matches!( + call_outcomes.get(registration_id), + Some(NetworkApprovalOutcome::DeniedByUser) + ) { + return; + } + call_outcomes.insert(registration_id.to_string(), outcome); } pub(crate) async fn record_blocked_request(&self, blocked: BlockedRequest) { @@ -196,15 +237,24 @@ impl NetworkApprovalService { return; }; - let Some(attempt) = self.resolve_attempt_for_blocked_request(&blocked).await else { - return; - }; + self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByPolicy(message)) + .await; + } - let mut outcome = attempt.outcome.lock().await; - if matches!(outcome.as_ref(), Some(NetworkApprovalOutcome::DeniedByUser)) { - return; - } - *outcome = Some(NetworkApprovalOutcome::DeniedByPolicy(message)); + async fn active_turn_context(session: &Session) -> Option> { + let active_turn = session.active_turn.lock().await; + active_turn + .as_ref() + .and_then(|turn| turn.tasks.first()) + .map(|(_, task)| Arc::clone(&task.turn_context)) + } + + fn format_network_target(protocol: &str, host: &str, port: u16) -> String { + format!("{protocol}://{host}:{port}") + } + + fn approval_id_for_key(key: &HostApprovalKey) -> String { + format!("network#{}#{}#{}", key.protocol, key.host, key.port) } pub(crate) async fn handle_inline_policy_request( @@ -214,46 +264,63 @@ impl NetworkApprovalService { ) -> NetworkDecision { const REASON_NOT_ALLOWED: &str = "not_allowed"; - { - let approved_hosts = self.session_approved_hosts.lock().await; - if approved_hosts.contains(request.host.as_str()) { - return NetworkDecision::Allow; - } - } - - let Some(attempt) = self.resolve_attempt_for_request(&request).await else { - return NetworkDecision::deny(REASON_NOT_ALLOWED); - }; - - { - let approved_hosts = attempt.approved_hosts.lock().await; - if approved_hosts.contains(request.host.as_str()) { - return NetworkDecision::Allow; - } - } - let protocol = match request.protocol { NetworkProtocol::Http => NetworkApprovalProtocol::Http, NetworkProtocol::HttpsConnect => NetworkApprovalProtocol::Https, NetworkProtocol::Socks5Tcp => NetworkApprovalProtocol::Socks5Tcp, NetworkProtocol::Socks5Udp => NetworkApprovalProtocol::Socks5Udp, }; + let key = HostApprovalKey::from_request(&request, protocol); - let Some(turn_context) = session.turn_context_for_sub_id(&attempt.turn_id).await else { + { + let approved_hosts = self.session_approved_hosts.lock().await; + if approved_hosts.contains(&key) { + return NetworkDecision::Allow; + } + } + + let (pending, is_owner) = self.get_or_create_pending_approval(key.clone()).await; + if !is_owner { + return pending.wait_for_decision().await.to_network_decision(); + } + + let target = Self::format_network_target(key.protocol, request.host.as_str(), key.port); + let policy_denial_message = + format!("Network access to \"{target}\" was blocked by policy."); + let prompt_reason = format!("{} is not in the allowed_domains", request.host); + + let Some(turn_context) = Self::active_turn_context(session).await else { + pending.set_decision(PendingApprovalDecision::Deny).await; + let mut pending_approvals = self.pending_host_approvals.lock().await; + pending_approvals.remove(&key); + self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByPolicy( + policy_denial_message, + )) + .await; return NetworkDecision::deny(REASON_NOT_ALLOWED); }; + if !allows_network_prompt(turn_context.approval_policy) { + pending.set_decision(PendingApprovalDecision::Deny).await; + let mut pending_approvals = self.pending_host_approvals.lock().await; + pending_approvals.remove(&key); + self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByPolicy( + policy_denial_message, + )) + .await; + return NetworkDecision::deny(REASON_NOT_ALLOWED); + } + + let approval_id = Self::approval_id_for_key(&key); + let prompt_command = vec!["network-access".to_string(), target.clone()]; let approval_decision = session .request_command_approval( turn_context.as_ref(), - attempt.call_id.clone(), + approval_id, None, - attempt.command.clone(), - attempt.cwd.clone(), - Some(format!( - "Network access to \"{}\" is blocked by policy.", - request.host - )), + prompt_command, + turn_context.cwd.clone(), + Some(prompt_reason), Some(NetworkApprovalContext { host: request.host.clone(), protocol, @@ -262,23 +329,28 @@ impl NetworkApprovalService { ) .await; - match approval_decision { + let resolved = match approval_decision { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { - let mut approved_hosts = attempt.approved_hosts.lock().await; - approved_hosts.insert(request.host); - NetworkDecision::Allow - } - ReviewDecision::ApprovedForSession => { - let mut approved_hosts = self.session_approved_hosts.lock().await; - approved_hosts.insert(request.host); - NetworkDecision::Allow + PendingApprovalDecision::AllowOnce } + ReviewDecision::ApprovedForSession => PendingApprovalDecision::AllowForSession, ReviewDecision::Denied | ReviewDecision::Abort => { - let mut outcome = attempt.outcome.lock().await; - *outcome = Some(NetworkApprovalOutcome::DeniedByUser); - NetworkDecision::deny(REASON_NOT_ALLOWED) + self.record_outcome_for_single_active_call(NetworkApprovalOutcome::DeniedByUser) + .await; + PendingApprovalDecision::Deny } + }; + + if matches!(resolved, PendingApprovalDecision::AllowForSession) { + let mut approved_hosts = self.session_approved_hosts.lock().await; + approved_hosts.insert(key.clone()); } + + pending.set_decision(resolved).await; + let mut pending_approvals = self.pending_host_approvals.lock().await; + pending_approvals.remove(&key); + + resolved.to_network_decision() } } @@ -313,8 +385,8 @@ pub(crate) fn build_network_policy_decider( pub(crate) async fn begin_network_approval( session: &Session, - turn_id: &str, - call_id: &str, + _turn_id: &str, + _call_id: &str, has_managed_network_requirements: bool, spec: Option, ) -> Option { @@ -323,21 +395,15 @@ pub(crate) async fn begin_network_approval( return None; } - let attempt_id = Uuid::new_v4().to_string(); + let registration_id = Uuid::new_v4().to_string(); session .services .network_approval - .register_attempt( - attempt_id.clone(), - turn_id.to_string(), - call_id.to_string(), - spec.command, - spec.cwd, - ) + .register_call(registration_id.clone()) .await; Some(ActiveNetworkApproval { - attempt_id: Some(attempt_id), + registration_id: Some(registration_id), mode: spec.mode, }) } @@ -346,20 +412,20 @@ pub(crate) async fn finish_immediate_network_approval( session: &Session, active: ActiveNetworkApproval, ) -> Result<(), ToolError> { - let Some(attempt_id) = active.attempt_id.as_deref() else { + let Some(registration_id) = active.registration_id.as_deref() else { return Ok(()); }; let approval_outcome = session .services .network_approval - .take_outcome(attempt_id) + .take_call_outcome(registration_id) .await; session .services .network_approval - .unregister_attempt(attempt_id) + .unregister_call(registration_id) .await; match approval_outcome { @@ -371,22 +437,6 @@ pub(crate) async fn finish_immediate_network_approval( } } -pub(crate) async fn deferred_rejection_message( - session: &Session, - deferred: &DeferredNetworkApproval, -) -> Option { - match session - .services - .network_approval - .take_outcome(deferred.attempt_id()) - .await - { - Some(NetworkApprovalOutcome::DeniedByUser) => Some("rejected by user".to_string()), - Some(NetworkApprovalOutcome::DeniedByPolicy(message)) => Some(message), - None => None, - } -} - pub(crate) async fn finish_deferred_network_approval( session: &Session, deferred: Option, @@ -397,7 +447,7 @@ pub(crate) async fn finish_deferred_network_approval( session .services .network_approval - .unregister_attempt(deferred.attempt_id()) + .unregister_call(deferred.registration_id()) .await; } @@ -405,272 +455,145 @@ pub(crate) async fn finish_deferred_network_approval( mod tests { use super::*; use codex_network_proxy::BlockedRequestArgs; - use codex_network_proxy::NetworkPolicyRequestArgs; + use codex_protocol::protocol::AskForApproval; use pretty_assertions::assert_eq; - fn http_request(host: &str, attempt_id: Option<&str>) -> NetworkPolicyRequest { - NetworkPolicyRequest::new(NetworkPolicyRequestArgs { - protocol: NetworkProtocol::Http, + #[tokio::test] + async fn pending_approvals_are_deduped_per_host_protocol_and_port() { + let service = NetworkApprovalService::default(); + let key = HostApprovalKey { + host: "example.com".to_string(), + protocol: "http", + port: 443, + }; + + let (first, first_is_owner) = service.get_or_create_pending_approval(key.clone()).await; + let (second, second_is_owner) = service.get_or_create_pending_approval(key).await; + + assert!(first_is_owner); + assert!(!second_is_owner); + assert!(Arc::ptr_eq(&first, &second)); + } + + #[tokio::test] + async fn pending_approvals_do_not_dedupe_across_ports() { + let service = NetworkApprovalService::default(); + let first_key = HostApprovalKey { + host: "example.com".to_string(), + protocol: "https", + port: 443, + }; + let second_key = HostApprovalKey { + host: "example.com".to_string(), + protocol: "https", + port: 8443, + }; + + let (first, first_is_owner) = service.get_or_create_pending_approval(first_key).await; + let (second, second_is_owner) = service.get_or_create_pending_approval(second_key).await; + + assert!(first_is_owner); + assert!(second_is_owner); + assert!(!Arc::ptr_eq(&first, &second)); + } + + #[tokio::test] + async fn pending_waiters_receive_owner_decision() { + let pending = Arc::new(PendingHostApproval::new()); + + let waiter = { + let pending = Arc::clone(&pending); + tokio::spawn(async move { pending.wait_for_decision().await }) + }; + + pending + .set_decision(PendingApprovalDecision::AllowOnce) + .await; + + let decision = waiter.await.expect("waiter should complete"); + assert_eq!(decision, PendingApprovalDecision::AllowOnce); + } + + #[test] + fn allow_once_and_allow_for_session_both_allow_network() { + assert_eq!( + PendingApprovalDecision::AllowOnce.to_network_decision(), + NetworkDecision::Allow + ); + assert_eq!( + PendingApprovalDecision::AllowForSession.to_network_decision(), + NetworkDecision::Allow + ); + } + + #[test] + fn never_policy_disables_network_prompts() { + assert!(!allows_network_prompt(AskForApproval::Never)); + assert!(allows_network_prompt(AskForApproval::OnRequest)); + assert!(allows_network_prompt(AskForApproval::OnFailure)); + assert!(allows_network_prompt(AskForApproval::UnlessTrusted)); + } + + fn denied_blocked_request(host: &str) -> BlockedRequest { + BlockedRequest::new(BlockedRequestArgs { host: host.to_string(), - port: 80, - client_addr: None, - method: Some("GET".to_string()), - command: None, - exec_policy_hint: None, - attempt_id: attempt_id.map(ToString::to_string), + reason: "not_allowed".to_string(), + client: None, + method: None, + mode: None, + protocol: "http".to_string(), + decision: Some("deny".to_string()), + source: Some("decider".to_string()), + port: Some(80), }) } #[tokio::test] - async fn resolve_attempt_for_request_falls_back_to_single_active_attempt() { + async fn record_blocked_request_sets_policy_outcome_for_owner_call() { let service = NetworkApprovalService::default(); + service.register_call("registration-1".to_string()).await; + service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) + .record_blocked_request(denied_blocked_request("example.com")) .await; - let resolved = service - .resolve_attempt_for_request(&http_request("example.com", None)) - .await - .expect("single active attempt should be used as fallback"); - assert_eq!(resolved.call_id, "call-1"); - } - - #[tokio::test] - async fn resolve_attempt_for_request_returns_exact_attempt_match() { - let service = NetworkApprovalService::default(); - service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) - .await; - service - .register_attempt( - "attempt-2".to_string(), - "turn-2".to_string(), - "call-2".to_string(), - vec!["curl".to_string(), "openai.com".to_string()], - std::env::temp_dir(), - ) - .await; - - let resolved = service - .resolve_attempt_for_request(&http_request("openai.com", Some("attempt-2"))) - .await - .expect("attempt-2 should resolve"); - assert_eq!(resolved.call_id, "call-2"); - } - - #[tokio::test] - async fn resolve_attempt_for_request_returns_none_for_unknown_attempt_id() { - let service = NetworkApprovalService::default(); - service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) - .await; - - let resolved = service - .resolve_attempt_for_request(&http_request("example.com", Some("attempt-unknown"))) - .await; - assert!(resolved.is_none()); - } - - #[tokio::test] - async fn resolve_attempt_for_request_returns_none_when_ambiguous() { - let service = NetworkApprovalService::default(); - service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) - .await; - service - .register_attempt( - "attempt-2".to_string(), - "turn-2".to_string(), - "call-2".to_string(), - vec!["curl".to_string(), "robinhood.com".to_string()], - std::env::temp_dir(), - ) - .await; - - let resolved = service - .resolve_attempt_for_request(&http_request("example.com", None)) - .await; - assert!(resolved.is_none()); - } - - #[tokio::test] - async fn take_outcome_clears_stored_value() { - let service = NetworkApprovalService::default(); - service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) - .await; - - let attempt = { - let attempts = service.attempts.lock().await; - attempts - .get("attempt-1") - .cloned() - .expect("attempt should exist") - }; - { - let mut outcome = attempt.outcome.lock().await; - *outcome = Some(NetworkApprovalOutcome::DeniedByUser); - } - assert_eq!( - service.take_outcome("attempt-1").await, - Some(NetworkApprovalOutcome::DeniedByUser) - ); - assert_eq!(service.take_outcome("attempt-1").await, None); - } - - #[tokio::test] - async fn take_user_denial_outcome_preserves_policy_denial() { - let service = NetworkApprovalService::default(); - service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) - .await; - - let attempt = { - let attempts = service.attempts.lock().await; - attempts - .get("attempt-1") - .cloned() - .expect("attempt should exist") - }; - { - let mut outcome = attempt.outcome.lock().await; - *outcome = Some(NetworkApprovalOutcome::DeniedByPolicy( - "policy denied".to_string(), - )); - } - - assert!(!service.take_user_denial_outcome("attempt-1").await); - assert_eq!( - service.take_outcome("attempt-1").await, + service.take_call_outcome("registration-1").await, Some(NetworkApprovalOutcome::DeniedByPolicy( - "policy denied".to_string(), + "Network access to \"example.com\" was blocked: domain is not on the allowlist for the current sandbox mode.".to_string() )) ); } #[tokio::test] - async fn record_blocked_request_stores_policy_denial_outcome() { + async fn blocked_request_policy_does_not_override_user_denial_outcome() { let service = NetworkApprovalService::default(); + service.register_call("registration-1".to_string()).await; + service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) + .record_call_outcome("registration-1", NetworkApprovalOutcome::DeniedByUser) .await; - service - .record_blocked_request(BlockedRequest::new(BlockedRequestArgs { - host: "example.com".to_string(), - reason: "denied".to_string(), - client: None, - method: Some("GET".to_string()), - mode: None, - protocol: "http".to_string(), - attempt_id: Some("attempt-1".to_string()), - decision: Some("deny".to_string()), - source: Some("baseline_policy".to_string()), - port: Some(80), - })) - .await; - - let outcome = service - .take_outcome("attempt-1") - .await - .expect("outcome should be recorded"); - match outcome { - NetworkApprovalOutcome::DeniedByPolicy(message) => { - assert_eq!( - message, - "Network access to \"example.com\" was blocked: domain is explicitly denied by policy and cannot be approved from this prompt.".to_string() - ); - } - NetworkApprovalOutcome::DeniedByUser => panic!("expected policy denial"), - } - } - - #[tokio::test] - async fn record_blocked_request_does_not_override_user_denial() { - let service = NetworkApprovalService::default(); - service - .register_attempt( - "attempt-1".to_string(), - "turn-1".to_string(), - "call-1".to_string(), - vec!["curl".to_string(), "example.com".to_string()], - std::env::temp_dir(), - ) - .await; - - let attempt = { - let attempts = service.attempts.lock().await; - attempts - .get("attempt-1") - .cloned() - .expect("attempt should exist") - }; - { - let mut outcome = attempt.outcome.lock().await; - *outcome = Some(NetworkApprovalOutcome::DeniedByUser); - } - - service - .record_blocked_request(BlockedRequest::new(BlockedRequestArgs { - host: "example.com".to_string(), - reason: "denied".to_string(), - client: None, - method: Some("GET".to_string()), - mode: None, - protocol: "http".to_string(), - attempt_id: Some("attempt-1".to_string()), - decision: Some("deny".to_string()), - source: Some("baseline_policy".to_string()), - port: Some(80), - })) + .record_blocked_request(denied_blocked_request("example.com")) .await; assert_eq!( - service.take_outcome("attempt-1").await, + service.take_call_outcome("registration-1").await, Some(NetworkApprovalOutcome::DeniedByUser) ); } + + #[tokio::test] + async fn record_blocked_request_ignores_ambiguous_unattributed_blocked_requests() { + let service = NetworkApprovalService::default(); + service.register_call("registration-1".to_string()).await; + service.register_call("registration-2".to_string()).await; + + service + .record_blocked_request(denied_blocked_request("example.com")) + .await; + + assert_eq!(service.take_call_outcome("registration-1").await, None); + assert_eq!(service.take_call_outcome("registration-2").await, None); + } } diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index cdbaac025..afb24d4b3 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -69,9 +69,6 @@ impl ToolOrchestrator { turn: tool_ctx.turn, call_id: tool_ctx.call_id.clone(), tool_name: tool_ctx.tool_name.clone(), - network_attempt_id: network_approval.as_ref().and_then(|network_approval| { - network_approval.attempt_id().map(ToString::to_string) - }), }; let run_result = tool.run(req, attempt, &attempt_tool_ctx).await; diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 6f1e4b44a..bcbeafadf 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -154,8 +154,6 @@ impl ToolRuntime for ShellRuntime { ) -> Option { req.network.as_ref()?; Some(NetworkApprovalSpec { - command: req.command.clone(), - cwd: req.cwd.clone(), network: req.network.clone(), mode: NetworkApprovalMode::Immediate, }) @@ -221,10 +219,9 @@ impl ToolRuntime for ShellRuntime { req.sandbox_permissions, req.justification.clone(), )?; - let mut env = attempt + let env = attempt .env_for(spec, req.network.as_ref()) .map_err(|err| ToolError::Codex(err.into()))?; - env.network_attempt_id = ctx.network_attempt_id.clone(); let out = execute_env(env, attempt.policy, Self::stdout_stream(ctx)) .await .map_err(ToolError::Codex)?; diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index be049d093..201de7ad8 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -157,8 +157,6 @@ impl<'a> ToolRuntime for UnifiedExecRunt ) -> Option { req.network.as_ref()?; Some(NetworkApprovalSpec { - command: req.command.clone(), - cwd: req.cwd.clone(), network: req.network.clone(), mode: NetworkApprovalMode::Deferred, }) @@ -188,7 +186,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt let mut env = req.env.clone(); if let Some(network) = req.network.as_ref() { - network.apply_to_env_for_attempt(&mut env, ctx.network_attempt_id.as_deref()); + network.apply_to_env(&mut env); } let spec = build_command_spec( &command, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 89b4bc01e..9721fed53 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -272,7 +272,6 @@ pub(crate) struct ToolCtx<'a> { pub turn: &'a TurnContext, pub call_id: String, pub tool_name: String, - pub network_attempt_id: Option, } #[derive(Debug)] diff --git a/codex-rs/core/src/unified_exec/async_watcher.rs b/codex-rs/core/src/unified_exec/async_watcher.rs index 772baab00..1fbb1e8f6 100644 --- a/codex-rs/core/src/unified_exec/async_watcher.rs +++ b/codex-rs/core/src/unified_exec/async_watcher.rs @@ -139,43 +139,6 @@ pub(crate) fn spawn_exit_watcher( }); } -pub(crate) fn spawn_network_denial_watcher( - process: Arc, - session: Arc, - process_id: String, - network_attempt_id: String, -) { - let exit_token = process.cancellation_token(); - tokio::spawn(async move { - let mut poll = tokio::time::interval(Duration::from_millis(100)); - poll.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); - - loop { - tokio::select! { - _ = exit_token.cancelled() => { - break; - } - _ = poll.tick() => { - if session - .services - .network_approval - .take_user_denial_outcome(&network_attempt_id) - .await - { - process.terminate(); - session - .services - .unified_exec_manager - .release_process_id(&process_id) - .await; - break; - } - } - } - } - }); -} - async fn process_chunk( pending: &mut Vec, transcript: &Arc>, diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 4339d25ac..a586572dd 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -155,7 +155,7 @@ struct ProcessEntry { process_id: String, command: Vec, tty: bool, - network_attempt_id: Option, + network_approval_id: Option, session: Weak, last_used: tokio::time::Instant, } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index f2468583d..694531646 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -20,7 +20,6 @@ use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::events::ToolEventStage; use crate::tools::network_approval::DeferredNetworkApproval; -use crate::tools::network_approval::deferred_rejection_message; use crate::tools::network_approval::finish_deferred_network_approval; use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::runtimes::unified_exec::UnifiedExecRequest as UnifiedExecToolRequest; @@ -44,7 +43,6 @@ use crate::unified_exec::WARNING_UNIFIED_EXEC_PROCESSES; use crate::unified_exec::WriteStdinRequest; use crate::unified_exec::async_watcher::emit_exec_end_for_unified_exec; use crate::unified_exec::async_watcher::spawn_exit_watcher; -use crate::unified_exec::async_watcher::spawn_network_denial_watcher; use crate::unified_exec::async_watcher::start_streaming_output; use crate::unified_exec::clamp_yield_time; use crate::unified_exec::generate_chunk_id; @@ -140,18 +138,18 @@ impl UnifiedExecProcessManager { store.remove(process_id) }; if let Some(entry) = removed { - Self::unregister_network_attempt_for_entry(&entry).await; + Self::unregister_network_approval_for_entry(&entry).await; } } - async fn unregister_network_attempt_for_entry(entry: &ProcessEntry) { - if let Some(attempt_id) = entry.network_attempt_id.as_deref() + async fn unregister_network_approval_for_entry(entry: &ProcessEntry) { + if let Some(network_approval_id) = entry.network_approval_id.as_deref() && let Some(session) = entry.session.upgrade() { session .services .network_approval - .unregister_attempt(attempt_id) + .unregister_call(network_approval_id) .await; } } @@ -248,17 +246,6 @@ impl UnifiedExecProcessManager { .await; self.release_process_id(&request.process_id).await; - if let Some(deferred) = deferred_network_approval.as_ref() - && let Some(message) = - deferred_rejection_message(context.session.as_ref(), deferred).await - { - finish_deferred_network_approval( - context.session.as_ref(), - deferred_network_approval.take(), - ) - .await; - return Err(UnifiedExecError::create_process(message)); - } finish_deferred_network_approval( context.session.as_ref(), deferred_network_approval.take(), @@ -266,27 +253,13 @@ impl UnifiedExecProcessManager { .await; process.check_for_sandbox_denial_with_text(&text).await?; } else { - if let Some(deferred) = deferred_network_approval.as_ref() - && let Some(message) = - deferred_rejection_message(context.session.as_ref(), deferred).await - { - process.terminate(); - finish_deferred_network_approval( - context.session.as_ref(), - deferred_network_approval.take(), - ) - .await; - self.release_process_id(&request.process_id).await; - return Err(UnifiedExecError::create_process(message)); - } - // Long‑lived command: persist the process so write_stdin can reuse // it, and register a background watcher that will emit // ExecCommandEnd when the PTY eventually exits (even if no further // tool calls are made). - let network_attempt_id = deferred_network_approval + let network_approval_id = deferred_network_approval .as_ref() - .map(|deferred| deferred.attempt_id().to_string()); + .map(|deferred| deferred.registration_id().to_string()); self.store_process( Arc::clone(&process), context, @@ -295,7 +268,7 @@ impl UnifiedExecProcessManager { start, process_id, request.tty, - network_attempt_id, + network_approval_id, Arc::clone(&transcript), ) .await; @@ -443,7 +416,7 @@ impl UnifiedExecProcessManager { } }; if let ProcessStatus::Exited { entry, .. } = &status { - Self::unregister_network_attempt_for_entry(entry).await; + Self::unregister_network_approval_for_entry(entry).await; } status } @@ -502,17 +475,16 @@ impl UnifiedExecProcessManager { started_at: Instant, process_id: String, tty: bool, - network_attempt_id: Option, + network_approval_id: Option, transcript: Arc>, ) { - let network_attempt_id_for_watcher = network_attempt_id.clone(); let entry = ProcessEntry { process: Arc::clone(&process), call_id: context.call_id.clone(), process_id: process_id.clone(), command: command.to_vec(), tty, - network_attempt_id, + network_approval_id, session: Arc::downgrade(&context.session), last_used: started_at, }; @@ -525,7 +497,7 @@ impl UnifiedExecProcessManager { // prune_processes_if_needed runs while holding process_store; do async // network-approval cleanup only after dropping that lock. if let Some(pruned_entry) = pruned_entry { - Self::unregister_network_attempt_for_entry(&pruned_entry).await; + Self::unregister_network_approval_for_entry(&pruned_entry).await; pruned_entry.process.terminate(); } @@ -550,17 +522,6 @@ impl UnifiedExecProcessManager { transcript, started_at, ); - - if context.turn.config.managed_network_requirements_enabled() - && let Some(network_attempt_id) = network_attempt_id_for_watcher - { - spawn_network_denial_watcher( - Arc::clone(&process), - Arc::clone(&context.session), - process_id, - network_attempt_id, - ); - } } pub(crate) async fn open_session_with_exec_env( @@ -637,7 +598,6 @@ impl UnifiedExecProcessManager { turn: context.turn.as_ref(), call_id: context.call_id.clone(), tool_name: "exec_command".to_string(), - network_attempt_id: None, }; orchestrator .run( @@ -792,7 +752,7 @@ impl UnifiedExecProcessManager { }; for entry in entries { - Self::unregister_network_attempt_for_entry(&entry).await; + Self::unregister_network_approval_for_entry(&entry).await; entry.process.terminate(); } } diff --git a/codex-rs/core/tests/suite/exec.rs b/codex-rs/core/tests/suite/exec.rs index c857affd2..c3cb15875 100644 --- a/codex-rs/core/tests/suite/exec.rs +++ b/codex-rs/core/tests/suite/exec.rs @@ -37,7 +37,6 @@ async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result(input: &T) -> Option { .map(|info| info.peer_addr().to_string()) } -fn request_network_attempt_id(req: &Request) -> Option { - // Some HTTP stacks normalize proxy credentials into `authorization`; accept both. - attempt_id_from_proxy_authorization(req.headers().get("proxy-authorization")) - .or_else(|| attempt_id_from_proxy_authorization(req.headers().get("authorization"))) -} - fn remove_hop_by_hop_request_headers(headers: &mut HeaderMap) { while let Some(raw_connection) = headers.get(header::CONNECTION).cloned() { headers.remove(header::CONNECTION); @@ -738,7 +721,6 @@ async fn proxy_disabled_response( method, mode: None, protocol: protocol.as_policy_protocol().to_string(), - attempt_id: None, decision: Some("deny".to_string()), source: Some("proxy_state".to_string()), port: Some(port), @@ -796,8 +778,6 @@ mod tests { use crate::config::NetworkMode; use crate::config::NetworkProxySettings; use crate::runtime::network_proxy_state_for_policy; - use base64::Engine; - use base64::engine::general_purpose::STANDARD; use pretty_assertions::assert_eq; use rama_http::Method; use rama_http::Request; @@ -873,36 +853,6 @@ mod tests { ); } - #[test] - fn request_network_attempt_id_reads_proxy_authorization_header() { - let encoded = STANDARD.encode("codex-net-attempt-attempt-1:"); - let req = Request::builder() - .method(Method::GET) - .uri("http://example.com") - .header("proxy-authorization", format!("Basic {encoded}")) - .body(Body::empty()) - .unwrap(); - assert_eq!( - request_network_attempt_id(&req), - Some("attempt-1".to_string()) - ); - } - - #[test] - fn request_network_attempt_id_reads_authorization_header_fallback() { - let encoded = STANDARD.encode("codex-net-attempt-attempt-2:"); - let req = Request::builder() - .method(Method::GET) - .uri("http://example.com") - .header("authorization", format!("Basic {encoded}")) - .body(Body::empty()) - .unwrap(); - assert_eq!( - request_network_attempt_id(&req), - Some("attempt-2".to_string()) - ); - } - #[test] fn remove_hop_by_hop_request_headers_keeps_forwarding_headers() { let mut headers = HeaderMap::new(); diff --git a/codex-rs/network-proxy/src/lib.rs b/codex-rs/network-proxy/src/lib.rs index 536dd4bf6..f0cb3ae47 100644 --- a/codex-rs/network-proxy/src/lib.rs +++ b/codex-rs/network-proxy/src/lib.rs @@ -3,7 +3,6 @@ mod admin; mod config; mod http_proxy; -mod metadata; mod network_policy; mod policy; mod proxy; diff --git a/codex-rs/network-proxy/src/metadata.rs b/codex-rs/network-proxy/src/metadata.rs deleted file mode 100644 index 2542125c3..000000000 --- a/codex-rs/network-proxy/src/metadata.rs +++ /dev/null @@ -1,50 +0,0 @@ -use base64::Engine; -use base64::engine::general_purpose::STANDARD; -use rama_http::HeaderValue; - -pub const NETWORK_ATTEMPT_USERNAME_PREFIX: &str = "codex-net-attempt-"; - -pub fn proxy_username_for_attempt_id(attempt_id: &str) -> String { - format!("{NETWORK_ATTEMPT_USERNAME_PREFIX}{attempt_id}") -} - -pub fn attempt_id_from_proxy_authorization(header: Option<&HeaderValue>) -> Option { - let header = header?; - let raw = header.to_str().ok()?; - let encoded = raw.strip_prefix("Basic ")?; - let decoded = STANDARD.decode(encoded.trim()).ok()?; - let decoded = String::from_utf8(decoded).ok()?; - let username = decoded - .split_once(':') - .map(|(user, _)| user) - .unwrap_or(decoded.as_str()); - let attempt_id = username.strip_prefix(NETWORK_ATTEMPT_USERNAME_PREFIX)?; - if attempt_id.is_empty() { - None - } else { - Some(attempt_id.to_string()) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use base64::engine::general_purpose::STANDARD; - - #[test] - fn parses_attempt_id_from_proxy_authorization_header() { - let encoded = STANDARD.encode(format!("{NETWORK_ATTEMPT_USERNAME_PREFIX}abc123:")); - let header = HeaderValue::from_str(&format!("Basic {encoded}")).unwrap(); - assert_eq!( - attempt_id_from_proxy_authorization(Some(&header)), - Some("abc123".to_string()) - ); - } - - #[test] - fn ignores_non_attempt_proxy_authorization_header() { - let encoded = STANDARD.encode("normal-user:password"); - let header = HeaderValue::from_str(&format!("Basic {encoded}")).unwrap(); - assert_eq!(attempt_id_from_proxy_authorization(Some(&header)), None); - } -} diff --git a/codex-rs/network-proxy/src/network_policy.rs b/codex-rs/network-proxy/src/network_policy.rs index fdbbe9281..6d8ec48d9 100644 --- a/codex-rs/network-proxy/src/network_policy.rs +++ b/codex-rs/network-proxy/src/network_policy.rs @@ -71,7 +71,6 @@ pub struct NetworkPolicyRequest { pub method: Option, pub command: Option, pub exec_policy_hint: Option, - pub attempt_id: Option, } pub struct NetworkPolicyRequestArgs { @@ -82,7 +81,6 @@ pub struct NetworkPolicyRequestArgs { pub method: Option, pub command: Option, pub exec_policy_hint: Option, - pub attempt_id: Option, } impl NetworkPolicyRequest { @@ -95,7 +93,6 @@ impl NetworkPolicyRequest { method, command, exec_policy_hint, - attempt_id, } = args; Self { protocol, @@ -105,7 +102,6 @@ impl NetworkPolicyRequest { method, command, exec_policy_hint, - attempt_id, } } } @@ -258,7 +254,6 @@ mod tests { method: Some("GET".to_string()), command: None, exec_policy_hint: None, - attempt_id: None, }); let decision = evaluate_host_policy(&state, Some(&decider), &request) @@ -292,7 +287,6 @@ mod tests { method: Some("GET".to_string()), command: None, exec_policy_hint: None, - attempt_id: None, }); let decision = evaluate_host_policy(&state, Some(&decider), &request) @@ -333,7 +327,6 @@ mod tests { method: Some("GET".to_string()), command: None, exec_policy_hint: None, - attempt_id: None, }); let decision = evaluate_host_policy(&state, Some(&decider), &request) diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index 2e93fb7a1..ba385dbba 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -1,7 +1,6 @@ use crate::admin; use crate::config; use crate::http_proxy; -use crate::metadata::proxy_username_for_attempt_id; use crate::network_policy::NetworkPolicyDecider; use crate::runtime::BlockedRequestObserver; use crate::runtime::unix_socket_permissions_supported; @@ -338,12 +337,8 @@ fn apply_proxy_env_overrides( socks_addr: SocketAddr, socks_enabled: bool, allow_local_binding: bool, - network_attempt_id: Option<&str>, ) { - let http_proxy_url = network_attempt_id - .map(proxy_username_for_attempt_id) - .map(|username| format!("http://{username}@{http_addr}")) - .unwrap_or_else(|| format!("http://{http_addr}")); + let http_proxy_url = format!("http://{http_addr}"); let socks_proxy_url = format!("socks5h://{socks_addr}"); env.insert( ALLOW_LOCAL_BINDING_ENV_KEY.to_string(), @@ -390,9 +385,7 @@ fn apply_proxy_env_overrides( // Keep HTTP_PROXY/HTTPS_PROXY as HTTP endpoints. A lot of clients break if // those vars contain SOCKS URLs. We only switch ALL_PROXY here. // - // For attempt-scoped runs, point ALL_PROXY at the HTTP proxy URL so the - // attempt metadata survives in proxy credentials for correlation. - if socks_enabled && network_attempt_id.is_none() { + if socks_enabled { set_env_keys(env, ALL_PROXY_ENV_KEYS, &socks_proxy_url); set_env_keys(env, FTP_PROXY_ENV_KEYS, &socks_proxy_url); } else { @@ -427,14 +420,6 @@ impl NetworkProxy { } pub fn apply_to_env(&self, env: &mut HashMap) { - self.apply_to_env_for_attempt(env, None); - } - - pub fn apply_to_env_for_attempt( - &self, - env: &mut HashMap, - network_attempt_id: Option<&str>, - ) { // Enforce proxying for child processes. We intentionally override existing values so // command-level environment cannot bypass the managed proxy endpoint. apply_proxy_env_overrides( @@ -443,7 +428,6 @@ impl NetworkProxy { self.socks_addr, self.socks_enabled, self.allow_local_binding, - network_attempt_id, ); } @@ -751,7 +735,6 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, - None, ); assert_eq!( @@ -802,7 +785,6 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), false, true, - None, ); assert_eq!( @@ -813,7 +795,7 @@ mod tests { } #[test] - fn apply_proxy_env_overrides_embeds_attempt_id_in_http_proxy_url() { + fn apply_proxy_env_overrides_uses_plain_http_proxy_url() { let mut env = HashMap::new(); apply_proxy_env_overrides( &mut env, @@ -821,28 +803,27 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, - Some("attempt-123"), ); assert_eq!( env.get("HTTP_PROXY"), - Some(&"http://codex-net-attempt-attempt-123@127.0.0.1:3128".to_string()) + Some(&"http://127.0.0.1:3128".to_string()) ); assert_eq!( env.get("HTTPS_PROXY"), - Some(&"http://codex-net-attempt-attempt-123@127.0.0.1:3128".to_string()) + Some(&"http://127.0.0.1:3128".to_string()) ); assert_eq!( env.get("WS_PROXY"), - Some(&"http://codex-net-attempt-attempt-123@127.0.0.1:3128".to_string()) + Some(&"http://127.0.0.1:3128".to_string()) ); assert_eq!( env.get("WSS_PROXY"), - Some(&"http://codex-net-attempt-attempt-123@127.0.0.1:3128".to_string()) + Some(&"http://127.0.0.1:3128".to_string()) ); assert_eq!( env.get("ALL_PROXY"), - Some(&"http://codex-net-attempt-attempt-123@127.0.0.1:3128".to_string()) + Some(&"socks5h://127.0.0.1:8081".to_string()) ); #[cfg(target_os = "macos")] assert_eq!( @@ -867,7 +848,6 @@ mod tests { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), true, false, - None, ); assert_eq!( diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index 7917747a8..238582a6a 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -75,8 +75,6 @@ pub struct BlockedRequest { pub mode: Option, pub protocol: String, #[serde(skip_serializing_if = "Option::is_none")] - pub attempt_id: Option, - #[serde(skip_serializing_if = "Option::is_none")] pub decision: Option, #[serde(skip_serializing_if = "Option::is_none")] pub source: Option, @@ -92,7 +90,6 @@ pub struct BlockedRequestArgs { pub method: Option, pub mode: Option, pub protocol: String, - pub attempt_id: Option, pub decision: Option, pub source: Option, pub port: Option, @@ -107,7 +104,6 @@ impl BlockedRequest { method, mode, protocol, - attempt_id, decision, source, port, @@ -119,7 +115,6 @@ impl BlockedRequest { method, mode, protocol, - attempt_id, decision, source, port, @@ -365,7 +360,6 @@ impl NetworkProxyState { let source = entry.source.clone(); let protocol = entry.protocol.clone(); let port = entry.port; - let attempt_id = entry.attempt_id.clone(); guard.blocked.push_back(entry); guard.blocked_total = guard.blocked_total.saturating_add(1); let total = guard.blocked_total; @@ -373,7 +367,7 @@ impl NetworkProxyState { guard.blocked.pop_front(); } debug!( - "recorded blocked request telemetry (total={}, host={}, reason={}, decision={:?}, source={:?}, protocol={}, port={:?}, attempt_id={:?}, buffered={})", + "recorded blocked request telemetry (total={}, host={}, reason={}, decision={:?}, source={:?}, protocol={}, port={:?}, buffered={})", total, host, reason, @@ -381,7 +375,6 @@ impl NetworkProxyState { source, protocol, port, - attempt_id, guard.blocked.len() ); debug!("{violation_line}"); @@ -700,7 +693,6 @@ mod tests { method: Some("GET".to_string()), mode: None, protocol: "http".to_string(), - attempt_id: None, decision: Some("ask".to_string()), source: Some("decider".to_string()), port: Some(80), @@ -741,7 +733,6 @@ mod tests { method: Some("GET".to_string()), mode: None, protocol: "http".to_string(), - attempt_id: None, decision: Some("ask".to_string()), source: Some("decider".to_string()), port: Some(80), @@ -764,7 +755,6 @@ mod tests { method: Some("GET".to_string()), mode: Some(NetworkMode::Full), protocol: "http".to_string(), - attempt_id: Some("attempt-1".to_string()), decision: Some("ask".to_string()), source: Some("decider".to_string()), port: Some(80), @@ -773,7 +763,7 @@ mod tests { assert_eq!( blocked_request_violation_log_line(&entry), - r#"CODEX_NETWORK_POLICY_VIOLATION {"host":"google.com","reason":"not_allowed","client":"127.0.0.1","method":"GET","mode":"full","protocol":"http","attempt_id":"attempt-1","decision":"ask","source":"decider","port":80,"timestamp":1735689600}"# + r#"CODEX_NETWORK_POLICY_VIOLATION {"host":"google.com","reason":"not_allowed","client":"127.0.0.1","method":"GET","mode":"full","protocol":"http","decision":"ask","source":"decider","port":80,"timestamp":1735689600}"# ); } diff --git a/codex-rs/network-proxy/src/socks5.rs b/codex-rs/network-proxy/src/socks5.rs index 176055a5d..409bd82e3 100644 --- a/codex-rs/network-proxy/src/socks5.rs +++ b/codex-rs/network-proxy/src/socks5.rs @@ -168,7 +168,6 @@ async fn handle_socks5_tcp( method: None, mode: None, protocol: "socks5".to_string(), - attempt_id: None, decision: Some(details.decision.as_str().to_string()), source: Some(details.source.as_str().to_string()), port: Some(port), @@ -202,7 +201,6 @@ async fn handle_socks5_tcp( method: None, mode: Some(NetworkMode::Limited), protocol: "socks5".to_string(), - attempt_id: None, decision: Some(details.decision.as_str().to_string()), source: Some(details.source.as_str().to_string()), port: Some(port), @@ -229,7 +227,6 @@ async fn handle_socks5_tcp( method: None, command: None, exec_policy_hint: None, - attempt_id: None, }); match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await { @@ -254,7 +251,6 @@ async fn handle_socks5_tcp( method: None, mode: None, protocol: "socks5".to_string(), - attempt_id: None, decision: Some(details.decision.as_str().to_string()), source: Some(details.source.as_str().to_string()), port: Some(port), @@ -318,7 +314,6 @@ async fn inspect_socks5_udp( method: None, mode: None, protocol: "socks5-udp".to_string(), - attempt_id: None, decision: Some(details.decision.as_str().to_string()), source: Some(details.source.as_str().to_string()), port: Some(port), @@ -352,7 +347,6 @@ async fn inspect_socks5_udp( method: None, mode: Some(NetworkMode::Limited), protocol: "socks5-udp".to_string(), - attempt_id: None, decision: Some(details.decision.as_str().to_string()), source: Some(details.source.as_str().to_string()), port: Some(port), @@ -375,7 +369,6 @@ async fn inspect_socks5_udp( method: None, command: None, exec_policy_hint: None, - attempt_id: None, }); match evaluate_host_policy(&state, policy_decider.as_ref(), &request).await { @@ -400,7 +393,6 @@ async fn inspect_socks5_udp( method: None, mode: None, protocol: "socks5-udp".to_string(), - attempt_id: None, decision: Some(details.decision.as_str().to_string()), source: Some(details.source.as_str().to_string()), port: Some(port), diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 567f18f33..169c8bdcc 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -122,7 +122,7 @@ impl ApprovalOverlay { || "Would you like to run the following command?".to_string(), |network_approval_context| { format!( - "Do you want to approve access to \"{}\"?", + "Do you want to approve network access to \"{}\"?", network_approval_context.host ) }, @@ -364,12 +364,14 @@ impl From for ApprovalRequestState { header.push(Line::from(vec!["Reason: ".into(), reason.italic()])); header.push(Line::from("")); } - let full_cmd = strip_bash_lc_and_escape(&command); - let mut full_cmd_lines = highlight_bash_to_lines(&full_cmd); - if let Some(first) = full_cmd_lines.first_mut() { - first.spans.insert(0, Span::from("$ ")); + if network_approval_context.is_none() { + let full_cmd = strip_bash_lc_and_escape(&command); + let mut full_cmd_lines = highlight_bash_to_lines(&full_cmd); + if let Some(first) = full_cmd_lines.first_mut() { + first.spans.insert(0, Span::from("$ ")); + } + header.extend(full_cmd_lines); } - header.extend(full_cmd_lines); Self { variant: ApprovalVariant::Exec { id, @@ -738,11 +740,15 @@ mod tests { .collect(); assert!( - rendered - .iter() - .any(|line| line.contains("Do you want to approve access to \"example.com\"?")), + rendered.iter().any(|line| { + line.contains("Do you want to approve network access to \"example.com\"?") + }), "expected network title to include host, got {rendered:?}" ); + assert!( + !rendered.iter().any(|line| line.contains("$ curl")), + "network prompt should not show command line, got {rendered:?}" + ); assert!( !rendered.iter().any(|line| line.contains("don't ask again")), "network prompt should not show execpolicy option, got {rendered:?}"