diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index 7b93fdf70a..b5554016a2 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index a222d3d598..63b6ed7775 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -30,6 +30,7 @@ use codex_core::path_utils; use codex_core::path_utils::SymlinkWritePaths; use codex_core::path_utils::resolve_symlink_write_paths; use codex_core::path_utils::write_atomically; +use codex_protocol::protocol::AskForApproval; use codex_utils_absolute_path::AbsolutePathBuf; use serde_json::Value as JsonValue; use std::borrow::Cow; @@ -734,8 +735,13 @@ fn toml_value_to_value(value: &TomlValue) -> anyhow::Result { } } -fn validate_config(value: &TomlValue) -> Result<(), toml::de::Error> { - let _: ConfigToml = value.clone().try_into()?; +fn validate_config(value: &TomlValue) -> anyhow::Result<()> { + let config: ConfigToml = value.clone().try_into()?; + if config.approval_policy == Some(AskForApproval::UnlessTrusted) { + anyhow::bail!( + "approval_policy = \"untrusted\" is no longer supported; remove this setting" + ); + } Ok(()) } diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 26a8859c18..b0f5062389 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -8,6 +8,7 @@ use codex_code_mode::WebSocketCodeModeSessionProvider; use codex_config::LoaderOverrides; use codex_config::NoopThreadConfigLoader; use codex_core::config::Config; +use codex_core::config::UnsupportedUntrustedApprovalPolicyError; use codex_core::resolve_installation_id; use codex_login::AuthManager; #[cfg(debug_assertions)] @@ -504,11 +505,19 @@ pub async fn run_main_with_transport_options( config.http_client_factory(), ); } + Err(err) + if err.get_ref().is_some_and( + ::is::< + UnsupportedUntrustedApprovalPolicyError, + >, + ) => + { + return Err(err); + } Err(err) => { warn!(error = %err, "Failed to preload config for cloud config bundle"); - // TODO: Decide whether bootstrap config preload failures should block startup. // If this fails, we cannot install cloud/thread config loaders, so non-strict - // startup may continue without managed cloud config. + // startup continues without managed cloud config. } }; let mut config_warnings = Vec::new(); diff --git a/codex-rs/app-server/tests/common/lib.rs b/codex-rs/app-server/tests/common/lib.rs index ad6d99ae5c..538274ee54 100644 --- a/codex-rs/app-server/tests/common/lib.rs +++ b/codex-rs/app-server/tests/common/lib.rs @@ -37,6 +37,7 @@ pub use mock_model_server::create_mock_responses_server_sequence_unchecked; pub use models_cache::write_models_cache; pub use models_cache::write_models_cache_with_models; pub use responses::create_apply_patch_sse_response; +pub use responses::create_escalated_shell_command_sse_response; pub use responses::create_exec_command_sse_response; pub use responses::create_final_assistant_message_sse_response; pub use responses::create_request_permissions_sse_response; diff --git a/codex-rs/app-server/tests/common/responses.rs b/codex-rs/app-server/tests/common/responses.rs index 586d1446c0..a862259b06 100644 --- a/codex-rs/app-server/tests/common/responses.rs +++ b/codex-rs/app-server/tests/common/responses.rs @@ -22,6 +22,27 @@ pub fn create_shell_command_sse_response( ])) } +pub fn create_escalated_shell_command_sse_response( + command: Vec, + workdir: Option<&Path>, + timeout_ms: Option, + call_id: &str, +) -> anyhow::Result { + let command_str = shlex::try_join(command.iter().map(String::as_str))?; + let tool_call_arguments = serde_json::to_string(&json!({ + "command": command_str, + "workdir": workdir.map(|w| w.to_string_lossy()), + "timeout_ms": timeout_ms, + "sandbox_permissions": "require_escalated", + "justification": "Test approval request." + }))?; + Ok(responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call(call_id, "shell_command", &tool_call_arguments), + responses::ev_completed("resp-1"), + ])) +} + pub fn create_final_assistant_message_sse_response(message: &str) -> anyhow::Result { Ok(responses::sse(vec![ responses::ev_response_created("resp-1"), diff --git a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs index fcbe5ed9ef..2ccc6ddbee 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs @@ -720,7 +720,7 @@ async fn start_resource_test_app_server_with_extra_config( ) -> Result<(TempDir, TestAppServer)> { let codex_home = TempDir::new()?; MockResponsesConfig::new(responses_server_uri) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .with_root_config(&format!( "chatgpt_base_url = \"{apps_server_url}\"\nmcp_oauth_credentials_store = \"file\"" )) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs index 8dfefccc11..51821100fb 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs @@ -1354,7 +1354,7 @@ fn write_config_toml( format!( r#" model = "mock-model" -approval_policy = "untrusted" +approval_policy = "on-request" sandbox_mode = "read-only" model_provider = "mock_provider" diff --git a/codex-rs/app-server/tests/suite/v2/request_permissions.rs b/codex-rs/app-server/tests/suite/v2/request_permissions.rs index 787989b6d8..93cb18cae7 100644 --- a/codex-rs/app-server/tests/suite/v2/request_permissions.rs +++ b/codex-rs/app-server/tests/suite/v2/request_permissions.rs @@ -36,7 +36,7 @@ async fn request_permissions_round_trip() -> Result<()> { ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .enable_feature(Feature::RequestPermissionsTool) .write(codex_home.path())?; diff --git a/codex-rs/app-server/tests/suite/v2/request_user_input.rs b/codex-rs/app-server/tests/suite/v2/request_user_input.rs index 8daa098a33..bd52ef2fcb 100644 --- a/codex-rs/app-server/tests/suite/v2/request_user_input.rs +++ b/codex-rs/app-server/tests/suite/v2/request_user_input.rs @@ -77,7 +77,7 @@ async fn request_user_input_round_trip_for_mode( ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .write(codex_home.path())?; let mut mcp = TestAppServer::builder() diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 48af99641a..070e441563 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -212,7 +212,7 @@ async fn review_start_exec_approval_item_id_matches_command_execution_item() -> let codex_home = TempDir::new()?; MockResponsesConfig::new(&server.uri()) .with_provider_name("Mock provider") - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .disable_feature(Feature::ShellSnapshot) .write(codex_home.path())?; diff --git a/codex-rs/app-server/tests/suite/v2/thread_queue.rs b/codex-rs/app-server/tests/suite/v2/thread_queue.rs index 97adbf789b..67291b0255 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_queue.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_queue.rs @@ -4,10 +4,10 @@ use anyhow::Context; use anyhow::Result; use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; +use app_test_support::create_escalated_shell_command_sse_response; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_mock_responses_server_sequence_unchecked; -use app_test_support::create_shell_command_sse_response; use codex_app_server_protocol::ClientInfo; use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::CommandExecutionApprovalDecision; @@ -863,7 +863,7 @@ async fn queue_app(responses: Vec) -> Result<(TestAppServer, TempDir, Mo async fn queue_app_with_server(server: MockServer) -> Result<(TestAppServer, TempDir, MockServer)> { let codex_home = TempDir::new()?; let config = MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .with_root_config(r#"approvals_reviewer = "user""#); config.write(codex_home.path())?; let app = TestAppServer::builder() @@ -888,7 +888,7 @@ fn blocked_turn_response() -> Result { "import time; time.sleep(10)".to_string(), ]; - create_shell_command_sse_response( + create_escalated_shell_command_sse_response( shell_command, /*workdir*/ None, /*timeout_ms*/ Some(10_000), diff --git a/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs b/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs index 5d9ef748a7..891201d644 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs @@ -1,9 +1,9 @@ use anyhow::Result; use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; +use app_test_support::create_escalated_shell_command_sse_response; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_sequence; -use app_test_support::create_shell_command_sse_response; use app_test_support::format_with_current_shell_display; use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::CommandExecutionApprovalDecision; @@ -202,7 +202,7 @@ async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { std::fs::create_dir(&workspace)?; let responses = vec![ - create_shell_command_sse_response( + create_escalated_shell_command_sse_response( vec![ "python3".to_string(), "-c".to_string(), @@ -216,7 +216,7 @@ async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .write(&codex_home)?; let mut mcp = TestAppServer::builder() diff --git a/codex-rs/app-server/tests/suite/v2/thread_status.rs b/codex-rs/app-server/tests/suite/v2/thread_status.rs index bf2db5c0f7..9c51958f97 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_status.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_status.rs @@ -27,7 +27,7 @@ async fn thread_status_changed_emits_runtime_updates() -> Result<()> { let responses = vec![create_final_assistant_message_sse_response("done")?]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .enable_feature(Feature::CollaborationModes) .write(codex_home.path())?; @@ -131,7 +131,7 @@ async fn thread_status_changed_can_be_opted_out() -> Result<()> { let responses = vec![create_final_assistant_message_sse_response("done")?]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .enable_feature(Feature::CollaborationModes) .write(codex_home.path())?; diff --git a/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs b/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs index 6caf01d203..4e3225e7a4 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs @@ -226,7 +226,7 @@ async fn turn_interrupt_resolves_pending_command_approval_request() -> Result<() )?]) .await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .with_root_config(r#"approvals_reviewer = "user""#) .write(&codex_home)?; diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index cd107bf668..52a05a5925 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -3,6 +3,7 @@ use anyhow::Result; use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; use app_test_support::create_apply_patch_sse_response; +use app_test_support::create_escalated_shell_command_sse_response; use app_test_support::create_exec_command_sse_response; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_repeating_assistant; @@ -2253,7 +2254,7 @@ async fn turn_start_exec_approval_toggle_v2() -> Result<()> { // Mock server: first turn requests a shell call (elicitation), then completes. // Second turn same, but we'll set approval_policy=never to avoid elicitation. let responses = vec![ - create_shell_command_sse_response( + create_escalated_shell_command_sse_response( first_shell_command, /*workdir*/ None, Some(5000), @@ -2273,9 +2274,9 @@ async fn turn_start_exec_approval_toggle_v2() -> Result<()> { create_final_assistant_message_sse_response("done 2")?, ]; let server = create_mock_responses_server_sequence(responses).await; - // Default approval is untrusted to force elicitation on first turn. + // The first tool call explicitly requests escalation to force elicitation. MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .write(codex_home.as_path())?; let mut mcp = TestAppServer::builder() @@ -2467,7 +2468,7 @@ async fn run_turn_start_exec_approval_rejection_v2( expected_approval_command.replace(bearer_token, "[REDACTED_SECRET]"); let responses = vec![ - create_shell_command_sse_response( + create_escalated_shell_command_sse_response( shell_command, /*workdir*/ None, Some(5000), @@ -2477,7 +2478,7 @@ async fn run_turn_start_exec_approval_rejection_v2( ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .write(codex_home.as_path())?; let mut mcp = TestAppServer::builder() @@ -2638,7 +2639,7 @@ async fn turn_start_explicit_local_environment_updates_legacy_cwd_between_turns( ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .write(&codex_home)?; let mut mcp = TestAppServer::builder() @@ -3077,7 +3078,8 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { ]; let server = create_mock_responses_server_sequence_unchecked(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") + .with_sandbox_mode("read-only") // Snapshot startup is unrelated to the file-approval behavior under test. .disable_feature(Feature::ShellSnapshot) .write(&codex_home)?; @@ -4048,7 +4050,8 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") + .with_sandbox_mode("read-only") .write(&codex_home)?; let mut mcp = TestAppServer::builder() @@ -4276,7 +4279,8 @@ async fn run_turn_start_file_change_approval_rejection_v2( ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") + .with_sandbox_mode("read-only") .write(&codex_home)?; let mut mcp = TestAppServer::builder() @@ -4568,7 +4572,7 @@ async fn command_execution_notifications_include_trusted_plugin_id() -> Result<( ]; let server = create_mock_responses_server_sequence(responses).await; MockResponsesConfig::new(&server.uri()) - .with_approval_policy("untrusted") + .with_approval_policy("on-request") .with_sandbox_mode("danger-full-access") .enable_feature(Feature::Plugins) .disable_feature(Feature::RemotePlugin) @@ -4630,25 +4634,9 @@ async fn command_execution_notifications_include_trusted_plugin_id() -> Result<( }) .await??; if method == "item/started" { - let server_req = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_request_message(), - ) - .await??; - let ServerRequest::CommandExecutionRequestApproval { request_id, params } = server_req - else { - panic!("expected CommandExecutionRequestApproval request"); - }; - assert_eq!(params.item_id, "plugin-command"); - mcp.send_response( - request_id, - serde_json::to_value(CommandExecutionRequestApprovalResponse { - decision: CommandExecutionApprovalDecision::Decline, - })?, - ) - .await?; + assert_eq!(status, CommandExecutionStatus::InProgress); } else { - assert_eq!(status, CommandExecutionStatus::Declined); + assert_eq!(status, CommandExecutionStatus::Completed); } } diff --git a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs index 99c5951a23..f3f0aa2eb2 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs @@ -9,6 +9,7 @@ use anyhow::Result; use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; +use app_test_support::create_escalated_shell_command_sse_response; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_mock_responses_server_sequence_unchecked; @@ -191,7 +192,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { eprintln!("using zsh path for zsh-fork test: {}", zsh_path.display()); let responses = vec![ - create_shell_command_sse_response( + create_escalated_shell_command_sse_response( vec![ "python3".to_string(), "-c".to_string(), @@ -207,7 +208,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { create_config_toml( &codex_home, &server.uri(), - "untrusted", + "on-request", &BTreeMap::from([ (Feature::ShellZshFork, true), (Feature::UnifiedExec, false), @@ -322,7 +323,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_cancel_v2() -> Result<()> { }; eprintln!("using zsh path for zsh-fork test: {}", zsh_path.display()); - let responses = vec![create_shell_command_sse_response( + let responses = vec![create_escalated_shell_command_sse_response( vec![ "python3".to_string(), "-c".to_string(), @@ -336,7 +337,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_cancel_v2() -> Result<()> { create_config_toml( &codex_home, &server.uri(), - "untrusted", + "on-request", &BTreeMap::from([ (Feature::ShellZshFork, true), (Feature::UnifiedExec, false), @@ -491,7 +492,7 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() create_config_toml( &codex_home, &server.uri(), - "untrusted", + "on-request", &BTreeMap::from([ (Feature::ShellZshFork, true), (Feature::UnifiedExec, false), diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index d7b7074489..0c9024841b 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -3214,22 +3214,16 @@ allowed_approvals_reviewers = ["user"] #[test] fn deserialize_allowed_approval_policies() -> Result<()> { let toml_str = r#" - allowed_approval_policies = ["untrusted", "on-request"] + allowed_approval_policies = ["on-request", "never"] "#; let config: ConfigRequirementsToml = from_str(toml_str)?; let requirements: ConfigRequirements = with_unknown_source(config).try_into()?; assert_eq!( requirements.approval_policy.value(), - AskForApproval::UnlessTrusted, + AskForApproval::OnRequest, "currently, there is no way to specify the default value for approval policy in the toml, so it picks the first allowed value" ); - assert!( - requirements - .approval_policy - .can_set(&AskForApproval::UnlessTrusted) - .is_ok() - ); assert!( requirements .approval_policy @@ -3237,11 +3231,13 @@ allowed_approvals_reviewers = ["user"] .is_ok() ); assert_eq!( - requirements.approval_policy.can_set(&AskForApproval::Never), + requirements + .approval_policy + .can_set(&AskForApproval::UnlessTrusted), Err(ConstraintError::InvalidValue { field_name: "approval_policy", - candidate: "Never".into(), - allowed: "[UnlessTrusted, OnRequest]".into(), + candidate: "UnlessTrusted".into(), + allowed: "[OnRequest, Never]".into(), requirement_source: RequirementSource::Unknown, }) ); diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index 08931f5474..ceff237fe8 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -170,6 +170,7 @@ pub struct ConfigToml { pub model_auto_compact_token_limit_scope: Option, /// Default approval policy for executing commands. + #[schemars(with = "Option")] pub approval_policy: Option, /// Configures who approval requests are routed to for review once they have diff --git a/codex-rs/config/src/profile_toml.rs b/codex-rs/config/src/profile_toml.rs index 7d13c02a41..076e19c364 100644 --- a/codex-rs/config/src/profile_toml.rs +++ b/codex-rs/config/src/profile_toml.rs @@ -29,6 +29,7 @@ pub struct ConfigProfile { /// The key in the `model_providers` map identifying the /// [`ModelProviderInfo`] to use. pub model_provider: Option, + #[schemars(with = "Option")] pub approval_policy: Option, pub approvals_reviewer: Option, pub sandbox_mode: Option, diff --git a/codex-rs/config/src/schema.rs b/codex-rs/config/src/schema.rs index 3ef954b47b..bcebcbb759 100644 --- a/codex-rs/config/src/schema.rs +++ b/codex-rs/config/src/schema.rs @@ -2,6 +2,8 @@ use crate::config_toml::ConfigToml; use crate::types::RawMcpServerConfig; use codex_features::FEATURES; use codex_features::legacy_feature_keys; +use codex_protocol::protocol::GranularApprovalConfig; +use schemars::JsonSchema; use schemars::r#gen::SchemaGenerator; use schemars::r#gen::SchemaSettings; use schemars::schema::InstanceType; @@ -14,6 +16,28 @@ use serde_json::Map; use serde_json::Value; use std::path::Path; +/// Determines the conditions under which the user is consulted to approve +/// running the command proposed by Codex. +#[allow(dead_code)] +#[derive(JsonSchema)] +#[schemars(rename = "AskForApproval")] +#[serde(rename_all = "kebab-case")] +pub(crate) enum ConfigAskForApproval { + /// The model decides when to ask the user for approval. + OnRequest, + + /// Fine-grained controls for individual approval flows. + /// + /// When a field is `true`, commands in that category are allowed. When it + /// is `false`, those requests are automatically rejected instead of shown + /// to the user. + Granular(GranularApprovalConfig), + + /// Never ask the user to approve commands. Failures are immediately returned + /// to the model, and never escalated to the user for approval. + Never, +} + /// Schema for the `[features]` map with known + legacy keys only. pub fn features_schema(schema_gen: &mut SchemaGenerator) -> Schema { let mut object = SchemaObject { diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index c806f53bcd..273f94e811 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -255,13 +255,6 @@ "AskForApproval": { "description": "Determines the conditions under which the user is consulted to approve running the command proposed by Codex.", "oneOf": [ - { - "description": "Under this policy, only \"known safe\" commands—as determined by `is_safe_command()`—that **only read files** are auto‑approved. Everything else will ask the user to approve.", - "enum": [ - "untrusted" - ], - "type": "string" - }, { "description": "The model decides when to ask the user for approval.", "enum": [ diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 48d2ea64b0..493b9ee48d 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -9319,7 +9319,7 @@ async fn model_catalog_json_rejects_empty_catalog() -> std::io::Result<()> { fn create_test_fixture() -> std::io::Result { let toml = r#" model = "o3" -approval_policy = "untrusted" +approval_policy = "on-request" [analytics] enabled = true @@ -10405,7 +10405,7 @@ async fn test_untrusted_project_gets_unless_trusted_approval_policy() -> anyhow: ) .await?; - // Verify that untrusted projects get UnlessTrusted approval policy + // Verify that untrusted projects get the internal always-prompt policy. assert_eq!( config.permissions.approval_policy.value(), AskForApproval::UnlessTrusted, @@ -10805,8 +10805,7 @@ trust_level = "untrusted" } #[tokio::test] -async fn explicit_approval_policy_falls_back_when_disallowed_by_requirements() -> std::io::Result<()> -{ +async fn explicit_untrusted_approval_policy_is_rejected() -> std::io::Result<()> { let codex_home = TempDir::new()?; std::fs::write( codex_home.path().join(CONFIG_TOML_FILE), @@ -10814,19 +10813,17 @@ async fn explicit_approval_policy_falls_back_when_disallowed_by_requirements() - "#, )?; - let config = ConfigBuilder::without_managed_config_for_tests() + let error = ConfigBuilder::without_managed_config_for_tests() .codex_home(codex_home.path().to_path_buf()) .fallback_cwd(Some(codex_home.path().to_path_buf())) - .cloud_config_bundle( - CloudConfigBundleFixture::loader_with_enterprise_requirement( - r#"allowed_approval_policies = ["on-request"]"#, - ), - ) .build() - .await?; - assert_eq!( - config.permissions.approval_policy.value(), - AskForApproval::OnRequest + .await + .expect_err("untrusted approval policy should be rejected"); + assert!( + error + .to_string() + .contains("approval_policy = \"untrusted\" is no longer supported"), + "unexpected error: {error}" ); Ok(()) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 7cfa392daf..fecce3bf4a 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -186,6 +186,11 @@ pub(crate) use resolved_permission_profile::PermissionProfileState; const DEFAULT_IGNORE_LARGE_UNTRACKED_DIRS: i64 = 200; const DEFAULT_IGNORE_LARGE_UNTRACKED_FILES: i64 = 10 * 1024 * 1024; +/// Signals that a public config selected the retired `untrusted` approval policy. +#[derive(Debug, thiserror::Error)] +#[error("approval_policy = \"untrusted\" is no longer supported; remove this setting")] +pub struct UnsupportedUntrustedApprovalPolicyError; + /// Compatibility-only config retained so legacy `ghost_snapshot` settings /// continue to load even though snapshots are no longer produced. #[derive(Debug, Clone, PartialEq, Eq)] @@ -3554,6 +3559,12 @@ impl Config { } configured_network_proxy_config.enabled = true; } + if cfg.approval_policy == Some(AskForApproval::UnlessTrusted) { + return Err(std::io::Error::new( + ErrorKind::InvalidData, + UnsupportedUntrustedApprovalPolicyError, + )); + } let approval_policy_was_explicit = approval_policy_override.is_some() || cfg.approval_policy.is_some(); let mut approval_policy = approval_policy_override diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 9e327353fe..f24b062469 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -26,7 +26,6 @@ use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::protocol::AskForApproval; use codex_shell_command::is_dangerous_command::DangerousCommandMatch; use codex_shell_command::is_dangerous_command::dangerous_command_match; -use codex_shell_command::is_safe_command::is_known_safe_command; use thiserror::Error; use tokio::fs; use tokio::sync::Semaphore; @@ -150,7 +149,7 @@ pub(crate) static BANNED_PREFIX_SUGGESTIONS: &[&[&str]] = &[ /// /// The command tokens may be the original argv or a shell-specific lowering of /// a wrapper such as `bash -lc ...` or `powershell.exe -Command ...`. We only -/// need to distinguish the PowerShell case because its safelist and dangerous +/// need to distinguish the PowerShell case because its dangerous-command /// heuristics operate on PowerShell-flavored inner command words rather than /// the generic command classifier. #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -740,17 +739,9 @@ pub(crate) fn render_decision_for_unmatched_command( permission_profile, windows_sandbox_level, sandbox_permissions, - command_origin, + command_origin: _, } = context; let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); - let is_known_safe = match command_origin { - ExecPolicyCommandOrigin::Generic => is_known_safe_command(command), - #[cfg(windows)] - ExecPolicyCommandOrigin::PowerShell => { - codex_shell_command::is_safe_command::is_safe_powershell_words(command) - } - }; - // When the Windows sandbox backend is disabled, managed filesystem // restrictions are only a policy shape; there is no platform sandbox to // enforce the boundary. Keep that legacy case conservative while still @@ -759,13 +750,6 @@ pub(crate) fn render_decision_for_unmatched_command( && windows_sandbox_level == WindowsSandboxLevel::Disabled && profile_has_managed_filesystem_restrictions(permission_profile); - if is_known_safe - && (approval_policy == AskForApproval::UnlessTrusted - || windows_managed_fs_restrictions_without_sandbox_backend) - { - return Decision::Allow; - } - // If the command is flagged as dangerous or we have no sandbox protection, // we should never allow it to run without approval. // @@ -789,8 +773,8 @@ pub(crate) fn render_decision_for_unmatched_command( Decision::Allow } AskForApproval::UnlessTrusted => { - // We already checked the unmatched-command safelist and it - // returned false, so we must prompt. + // Projects marked untrusted require approval for every command + // that is not explicitly allowed by an exec policy rule. Decision::Prompt } AskForApproval::OnRequest => { diff --git a/codex-rs/core/src/exec_policy_windows_tests.rs b/codex-rs/core/src/exec_policy_windows_tests.rs index 57820f1344..b07932eda1 100644 --- a/codex-rs/core/src/exec_policy_windows_tests.rs +++ b/codex-rs/core/src/exec_policy_windows_tests.rs @@ -67,11 +67,11 @@ fn commands_for_exec_policy_parses_powershell_shell_wrapper() { } #[test] -fn unmatched_safe_powershell_words_are_allowed() { +fn unmatched_powershell_read_requires_approval_without_sandbox() { let command = vec!["Get-Content".to_string(), "Cargo.toml".to_string()]; assert_eq!( - Decision::Allow, + Decision::Prompt, render_decision_for_unmatched_command( &command, UnmatchedCommandContext { diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 0138cda4ca..4233e5f94d 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -96,7 +96,6 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::ExecCommandHandler; use crate::tools::handlers::RequestPermissionsHandler; -use crate::tools::handlers::ShellCommandHandler; use crate::tools::registry::ToolExecutor; use crate::tools::router::ToolCallSource; use crate::turn_diff_tracker::TurnDiffTracker; @@ -11301,111 +11300,6 @@ async fn sample_rollout( ) } -#[tokio::test] -async fn rejects_escalated_permissions_when_policy_not_on_request() { - use crate::exec_policy::ExecApprovalRequest; - use crate::sandboxing::SandboxPermissions; - use crate::tools::sandboxing::ExecApprovalRequirement; - use crate::turn_diff_tracker::TurnDiffTracker; - use codex_protocol::protocol::AskForApproval; - use codex_tools::ShellCommandBackendConfig; - - let (session, mut turn_context_raw) = make_session_and_context().await; - // Ensure policy is NOT OnRequest so the early rejection path triggers - Arc::make_mut(&mut turn_context_raw.config) - .permissions - .approval_policy - .set(AskForApproval::Never) - .expect("test setup should allow updating approval policy"); - let session = Arc::new(session); - let mut turn_context = Arc::new(turn_context_raw); - let step_context = StepContext::for_test(Arc::clone(&turn_context)); - - let command_script = "echo hi"; - let timeout_ms = 1000; - let sandbox_permissions = SandboxPermissions::RequireEscalated; - - let turn_diff_tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); - - let tool_name = "shell_command"; - let call_id = "test-call".to_string(); - - let handler = ShellCommandHandler::from(ShellCommandBackendConfig::Classic); - #[allow(deprecated)] - let workdir = Some(turn_context.cwd.to_string_lossy().to_string()); - let resp = handler - .handle(ToolInvocation { - session: Arc::clone(&session), - turn: Arc::clone(&turn_context), - step_context, - cancellation_token: CancellationToken::new(), - tracker: Arc::clone(&turn_diff_tracker), - call_id, - tool_name: codex_tools::ToolName::plain(tool_name), - source: crate::tools::context::ToolCallSource::Direct, - payload: ToolPayload::Function { - arguments: serde_json::json!({ - "command": command_script, - "workdir": workdir, - "timeout_ms": timeout_ms, - "sandbox_permissions": sandbox_permissions, - "justification": Some("test"), - }) - .to_string(), - }, - }) - .await; - - let Err(FunctionCallError::RespondToModel(output)) = resp else { - panic!("expected error result"); - }; - - let expected = format!( - "approval policy is {policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {policy:?}", - policy = turn_context.approval_policy() - ); - - pretty_assertions::assert_eq!(output, expected); - pretty_assertions::assert_eq!( - session - .granted_turn_permissions(codex_exec_server::LOCAL_ENVIRONMENT_ID) - .await, - None - ); - - // The rejection should not poison the non-escalated path for the same - // command. Force DangerFullAccess so this check stays focused on approval - // policy rather than platform-specific sandbox behavior. - let turn_context_mut = Arc::get_mut(&mut turn_context).expect("unique thread settings Arc"); - Arc::make_mut(&mut turn_context_mut.config) - .permissions - .set_permission_profile(PermissionProfile::Disabled) - .expect("test setup should allow updating permission profile"); - - let command = session.user_shell().derive_exec_args( - command_script, - turn_context.config.permissions.allow_login_shell, - ); - let exec_approval_requirement = session - .services - .exec_policy - .create_exec_approval_requirement_for_command(ExecApprovalRequest { - command: &command, - approval_policy: turn_context.approval_policy(), - permission_profile: turn_context.permission_profile(), - environment_policy: None, - windows_sandbox_level: turn_context.windows_sandbox_level, - sandbox_permissions: SandboxPermissions::UseDefault, - prefix_rule: None, - allow_prefix_rules: turn_context.allow_prefix_rules(), - }) - .await; - assert!(matches!( - exec_approval_requirement, - ExecApprovalRequirement::Skip { .. } - )); -} - #[cfg(unix)] #[tokio::test] async fn shell_tool_cancellation_waits_for_runtime_cleanup() -> anyhow::Result<()> { diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 7b7b238b23..4cf246a8b9 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -32,56 +32,10 @@ use crate::turn_diff_tracker::TurnDiffTracker; use codex_protocol::protocol::EnvironmentConfig; use codex_protocol::protocol::EnvironmentConfigState; use codex_protocol::protocol::TurnEnvironmentSelection; -use codex_shell_command::is_safe_command::is_known_safe_command; -use codex_shell_command::powershell::try_find_powershell_executable_blocking; -use codex_shell_command::powershell::try_find_pwsh_executable_blocking; use codex_utils_path_uri::PathUri; use serde_json::json; use tokio::sync::Mutex; -/// The logic for is_known_safe_command() has heuristics for known shells, -/// so we must ensure the commands generated by [ShellCommandHandler] can be -/// recognized as safe if the `command` is safe. -#[test] -fn commands_generated_by_shell_command_handler_can_be_matched_by_is_known_safe_command() { - let bash_shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - }; - assert_safe(&bash_shell, "ls -la"); - - let zsh_shell = Shell { - shell_type: ShellType::Zsh, - shell_path: PathBuf::from("/bin/zsh"), - }; - assert_safe(&zsh_shell, "ls -la"); - - if let Some(path) = try_find_powershell_executable_blocking() { - let powershell = Shell { - shell_type: ShellType::PowerShell, - shell_path: path.to_path_buf(), - }; - assert_safe(&powershell, "ls -Name"); - } - - if let Some(path) = try_find_pwsh_executable_blocking() { - let pwsh = Shell { - shell_type: ShellType::PowerShell, - shell_path: path.to_path_buf(), - }; - assert_safe(&pwsh, "ls -Name"); - } -} - -fn assert_safe(shell: &Shell, command: &str) { - assert!(is_known_safe_command(&shell.derive_exec_args( - command, /* use_login_shell */ /*use_login_shell*/ true - ))); - assert!(is_known_safe_command(&shell.derive_exec_args( - command, /* use_login_shell */ /*use_login_shell*/ false - ))); -} - #[tokio::test] async fn shell_command_handler_to_exec_params_uses_selected_environment() { let (session, mut turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index df5f1b4980..206020d53c 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -385,7 +385,7 @@ struct CoreShellActionProvider { #[allow(clippy::large_enum_variant)] enum DecisionSource { PrefixRule, - /// Often, this is `is_safe_command()`. + /// Derived from the unmatched-command sandbox fallback. UnmatchedCommandFallback, } diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 868fed1f5c..ec2b62375c 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -569,24 +569,24 @@ impl Expectation { assert_eq!( result.exit_code, Some(0), - "expected successful trusted command exit: {}", + "expected successful command exit: {}", result.stdout ); assert!( result.stdout.contains(stdout_contains), - "trusted command stdout missing {stdout_contains:?}: {}", + "command stdout missing {stdout_contains:?}: {}", result.stdout ); } Expectation::CommandSuccessNoExitCode { stdout_contains } => { assert!( result.exit_code.is_none() || result.exit_code == Some(0), - "expected no exit code for trusted command: {}", + "expected no exit code for command: {}", result.stdout ); assert!( result.stdout.contains(stdout_contains), - "trusted command stdout missing {stdout_contains:?}: {}", + "command stdout missing {stdout_contains:?}: {}", result.stdout ); } @@ -980,7 +980,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "trusted_command_unless_trusted_runs_without_prompt", + name: "simple_command_unless_trusted_requires_approval", approval_policy: UnlessTrusted, sandbox_policy: SandboxPolicy::DangerFullAccess, action: ActionKind::RunCommand { @@ -989,13 +989,16 @@ fn scenarios() -> Vec { sandbox_permissions: SandboxPermissions::UseDefault, features: vec![], model_override: Some("gpt-5.2"), - outcome: Outcome::Auto, - expectation: Expectation::CommandSuccess { - stdout_contains: "trusted-unless", + outcome: Outcome::ExecApproval { + decision: ReviewDecision::denied("blocked in untrusted project"), + expected_reason: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "blocked in untrusted project", }, }, ScenarioSpec { - name: "trusted_command_unless_trusted_runs_without_prompt_gpt_5_1_no_exit", + name: "simple_command_unless_trusted_requires_approval_gpt_5_1_no_exit", approval_policy: UnlessTrusted, sandbox_policy: SandboxPolicy::DangerFullAccess, action: ActionKind::RunCommand { @@ -1004,9 +1007,12 @@ fn scenarios() -> Vec { sandbox_permissions: SandboxPermissions::UseDefault, features: vec![], model_override: Some("gpt-5.4"), - outcome: Outcome::Auto, - expectation: Expectation::CommandSuccessNoExitCode { - stdout_contains: "trusted-unless", + outcome: Outcome::ExecApproval { + decision: ReviewDecision::denied("blocked in untrusted project"), + expected_reason: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "blocked in untrusted project", }, }, ScenarioSpec { @@ -1046,7 +1052,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "known_safe_escalation_on_request_requires_approval", + name: "simple_command_escalation_on_request_requires_approval", approval_policy: OnRequest, sandbox_policy: workspace_write(false), action: ActionKind::RunCommand { @@ -1068,7 +1074,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "known_safe_escalation_granular_sandbox_disabled_rejects", + name: "simple_command_escalation_granular_sandbox_disabled_rejects", approval_policy: Granular(GranularApprovalConfig { sandbox_approval: false, rules: true, @@ -1287,7 +1293,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "trusted_command_on_request_read_only_runs_without_prompt", + name: "simple_command_on_request_read_only_runs_without_prompt", approval_policy: OnRequest, sandbox_policy: SandboxPolicy::new_read_only_policy(), action: ActionKind::RunCommand { @@ -1302,7 +1308,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "trusted_command_on_request_read_only_runs_without_prompt_gpt_5_1_no_exit", + name: "simple_command_on_request_read_only_runs_without_prompt_gpt_5_1_no_exit", approval_policy: OnRequest, sandbox_policy: SandboxPolicy::new_read_only_policy(), action: ActionKind::RunCommand { @@ -1606,7 +1612,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "trusted_command_never_runs_without_prompt", + name: "simple_command_never_runs_without_prompt", approval_policy: Never, sandbox_policy: SandboxPolicy::new_read_only_policy(), action: ActionKind::RunCommand { @@ -1732,7 +1738,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "unified exec on request no approval for safe command", + name: "unified exec on request no approval for simple command", approval_policy: OnRequest, sandbox_policy: SandboxPolicy::DangerFullAccess, action: ActionKind::RunUnifiedExecCommand { @@ -1788,7 +1794,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "safe command with heredoc and redirect still requires approval", + name: "heredoc with redirect still requires approval", approval_policy: AskForApproval::OnRequest, sandbox_policy: workspace_write(false), action: ActionKind::RunUnifiedExecCommand { @@ -1975,7 +1981,7 @@ fn scenarios() -> Vec { }, }, ScenarioSpec { - name: "quoted_shell_metacharacters_unless_trusted_run_without_prompt", + name: "quoted_shell_metacharacters_unless_trusted_require_approval", approval_policy: UnlessTrusted, sandbox_policy: workspace_write(false), action: ActionKind::RunCommand { @@ -1984,9 +1990,12 @@ fn scenarios() -> Vec { sandbox_permissions: SandboxPermissions::UseDefault, features: vec![], model_override: Some("gpt-5.2"), - outcome: Outcome::Auto, - expectation: Expectation::CommandSuccess { - stdout_contains: "-g*.py -{delete,print}", + outcome: Outcome::ExecApproval { + decision: ReviewDecision::denied("blocked in untrusted project"), + expected_reason: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "blocked in untrusted project", }, }, ] diff --git a/codex-rs/mcp-server/src/codex_tool_config.rs b/codex-rs/mcp-server/src/codex_tool_config.rs index 531c8f8038..b9c8aa8505 100644 --- a/codex-rs/mcp-server/src/codex_tool_config.rs +++ b/codex-rs/mcp-server/src/codex_tool_config.rs @@ -36,7 +36,7 @@ pub struct CodexToolCallParam { pub cwd: Option, /// Approval policy for shell commands generated by the model: - /// `untrusted`, `on-request`, `never`. + /// `on-request`, `never`. #[serde(default, skip_serializing_if = "Option::is_none")] pub approval_policy: Option, @@ -67,7 +67,6 @@ pub struct CodexToolCallParam { #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(rename_all = "kebab-case")] pub enum CodexToolCallApprovalPolicy { - Untrusted, OnRequest, Never, } @@ -75,7 +74,6 @@ pub enum CodexToolCallApprovalPolicy { impl From for AskForApproval { fn from(value: CodexToolCallApprovalPolicy) -> Self { match value { - CodexToolCallApprovalPolicy::Untrusted => AskForApproval::UnlessTrusted, CodexToolCallApprovalPolicy::OnRequest => AskForApproval::OnRequest, CodexToolCallApprovalPolicy::Never => AskForApproval::Never, } @@ -299,9 +297,8 @@ mod tests { "additionalProperties": false, "properties": { "approval-policy": { - "description": "Approval policy for shell commands generated by the model: `untrusted`, `on-request`, `never`.", + "description": "Approval policy for shell commands generated by the model: `on-request`, `never`.", "enum": [ - "untrusted", "on-request", "never" ], diff --git a/codex-rs/mcp-server/tests/common/responses.rs b/codex-rs/mcp-server/tests/common/responses.rs index 48a575a4c6..2b4d78ce18 100644 --- a/codex-rs/mcp-server/tests/common/responses.rs +++ b/codex-rs/mcp-server/tests/common/responses.rs @@ -14,6 +14,8 @@ pub fn create_shell_command_sse_response( "command": command_str, "workdir": workdir.map(|w| w.to_string_lossy()), "timeout_ms": timeout_ms, + "sandbox_permissions": "require_escalated", + "justification": "Test approval request.", }))?; let response_id = format!("resp-{call_id}"); Ok(responses::sse(vec![ diff --git a/codex-rs/mcp-server/tests/suite/codex_tool.rs b/codex-rs/mcp-server/tests/suite/codex_tool.rs index 0b430f3eb5..dcc6bf8b0c 100644 --- a/codex-rs/mcp-server/tests/suite/codex_tool.rs +++ b/codex-rs/mcp-server/tests/suite/codex_tool.rs @@ -221,7 +221,7 @@ fn create_expected_elicitation_request_params( } /// Test that patch approval triggers an elicitation request to the MCP and that -/// sending the approval applies the patch, as expected. +/// sending a denial leaves the patch unapplied, as expected. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_patch_approval_triggers_elicitation() { if env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { @@ -244,11 +244,14 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { } let cwd = TempDir::new()?; - let test_file = cwd.path().join("destination_file.txt"); - std::fs::write(&test_file, "original content\n")?; + let test_file = Path::new("/").join( + cwd.path() + .file_name() + .ok_or_else(|| anyhow::anyhow!("temporary directory must have a file name"))?, + ); let patch_content = format!( - "*** Begin Patch\n*** Update File: {}\n-original content\n+modified content\n*** End Patch", + "*** Begin Patch\n*** Add File: {}\n+new content\n*** End Patch", test_file.as_path().to_string_lossy() ); @@ -258,7 +261,7 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { dir: _dir, } = create_mcp_process(vec![ create_apply_patch_sse_response(&patch_content, "call1234")?, - create_final_assistant_message_sse_response("Patch has been applied successfully!")?, + create_final_assistant_message_sse_response("Patch was not applied.")?, ]) .await?; @@ -267,11 +270,12 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { .send_codex_tool_call(CodexToolCallParam { cwd: Some(cwd.path().to_string_lossy().to_string()), prompt: "please modify the test file".to_string(), - // This test exercises patch approval elicitation, not local sandbox setup. - config: Some(HashMap::from([( - "sandbox_mode".to_string(), - json!("danger-full-access"), - )])), + // Use the user reviewer so this test exercises MCP elicitation even when auto-review + // is enabled by the surrounding environment. + config: Some(HashMap::from([ + ("approvals_reviewer".to_string(), json!("user")), + ("features.guardian_approval".to_string(), json!(false)), + ])), ..Default::default() }) .await?; @@ -296,9 +300,8 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { let mut expected_changes = HashMap::new(); expected_changes.insert( test_file.as_path().to_path_buf(), - FileChange::Update { - unified_diff: "@@ -1 +1 @@\n-original content\n+modified content\n".to_string(), - move_path: None, + FileChange::Add { + content: "new content\n".to_string(), }, ); @@ -314,12 +317,12 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { )?) ); - // Accept the patch approval request by responding to the elicitation + // Deny the patch approval request so this test does not require the Linux sandbox helper. mcp_process .send_response( elicitation_request_id, serde_json::to_value(PatchApprovalResponse { - decision: ReviewDecision::Approved, + decision: ReviewDecision::denied("rejected by test"), })?, ) .await?; @@ -337,21 +340,20 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { result: json!({ "content": [ { - "text": "Patch has been applied successfully!", + "text": "Patch was not applied.", "type": "text" } ], "structuredContent": { "threadId": params.thread_id, - "content": "Patch has been applied successfully!" + "content": "Patch was not applied." } }), }, codex_response ); - let file_contents = std::fs::read_to_string(test_file.as_path())?; - assert_eq!(file_contents, "modified content\n"); + assert!(!test_file.exists()); Ok(()) } @@ -644,8 +646,8 @@ async fn create_mcp_process(responses: Vec) -> anyhow::Result } /// Create a Codex config that uses the mock server as the model provider. -/// It also uses `approval_policy = "untrusted"` so that we exercise the -/// elicitation code path for shell commands. +/// The shell command explicitly requests escalation so that we exercise the +/// elicitation code path. fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { let config_toml = codex_home.join("config.toml"); std::fs::write( @@ -653,7 +655,7 @@ fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<() format!( r#" model = "mock-model" -approval_policy = "untrusted" +approval_policy = "on-request" sandbox_policy = "workspace-write" model_provider = "mock_provider" diff --git a/codex-rs/memories/read/src/usage.rs b/codex-rs/memories/read/src/usage.rs index 8691531483..cb9503efca 100644 --- a/codex-rs/memories/read/src/usage.rs +++ b/codex-rs/memories/read/src/usage.rs @@ -1,6 +1,4 @@ use codex_protocol::parse_command::ParsedCommand; -use codex_shell_command::bash::parse_shell_script_into_commands; -use codex_shell_command::is_safe_command::is_known_safe_command; use codex_shell_command::parse_command::parse_shell_script; pub use crate::metrics::MEMORIES_USAGE_METRIC; @@ -27,17 +25,15 @@ impl MemoriesUsageKind { } pub fn memories_usage_kinds_from_command(command: &str) -> Vec { - let Some(commands) = parse_shell_script_into_commands(command) else { - return Vec::new(); - }; - if !commands + let commands = parse_shell_script(command); + if commands .iter() - .all(|command| is_known_safe_command(command)) + .any(|command| matches!(command, ParsedCommand::Unknown { .. })) { return Vec::new(); } - parse_shell_script(command) + commands .into_iter() .filter_map(|command| match command { ParsedCommand::Read { path, .. } => get_memory_kind(path.display().to_string()), diff --git a/codex-rs/prompts/templates/permissions/approval_policy/unless_trusted.md b/codex-rs/prompts/templates/permissions/approval_policy/unless_trusted.md index 039f702656..e997ef2839 100644 --- a/codex-rs/prompts/templates/permissions/approval_policy/unless_trusted.md +++ b/codex-rs/prompts/templates/permissions/approval_policy/unless_trusted.md @@ -1 +1 @@ - Approvals are your mechanism to get user consent to run shell commands without the sandbox. `approval_policy` is `unless-trusted`: The harness will escalate most commands for user approval, apart from a limited allowlist of safe "read" commands. + `approval_policy` is `unless-trusted`: The harness will require user approval before running commands unless an explicit exec policy rule allows them. diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 2cf3e39cd9..b2230b0aea 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -914,9 +914,8 @@ impl Op { #[serde(rename_all = "kebab-case")] #[strum(serialize_all = "kebab-case")] pub enum AskForApproval { - /// Under this policy, only "known safe" commands—as determined by - /// `is_safe_command()`—that **only read files** are auto‑approved. - /// Everything else will ask the user to approve. + /// Internal policy for projects marked untrusted. Commands require + /// approval unless an explicit exec policy rule allows them. #[serde(rename = "untrusted")] #[strum(serialize = "untrusted")] UnlessTrusted, diff --git a/codex-rs/shell-command/src/bash.rs b/codex-rs/shell-command/src/bash.rs index 5157a61f73..ddd5807bfc 100644 --- a/codex-rs/shell-command/src/bash.rs +++ b/codex-rs/shell-command/src/bash.rs @@ -315,7 +315,6 @@ fn parse_raw_string(node: Node, src: &str) -> Option { #[cfg(test)] mod tests { use super::*; - use crate::is_safe_command::is_known_safe_command; use pretty_assertions::assert_eq; fn parse_seq(src: &str) -> Option>> { @@ -444,7 +443,6 @@ mod tests { for flag in ["-c", "-lc"] { let command = [shell, flag, script].map(str::to_owned); assert_eq!(parse_shell_lc_plain_commands(&command), None, "{command:?}"); - assert!(!is_known_safe_command(&command), "{command:?}"); } } } @@ -485,7 +483,7 @@ mod tests { assert_eq!(parse_seq(script), None, "{script:?}"); for shell in ["bash", "zsh"] { let command = [shell, "-lc", script].map(str::to_owned); - assert!(!is_known_safe_command(&command), "{command:?}"); + assert_eq!(parse_shell_lc_plain_commands(&command), None, "{command:?}"); } } } diff --git a/codex-rs/shell-command/src/command_safety/is_safe_command.rs b/codex-rs/shell-command/src/command_safety/is_safe_command.rs deleted file mode 100644 index 1f24a6d21d..0000000000 --- a/codex-rs/shell-command/src/command_safety/is_safe_command.rs +++ /dev/null @@ -1,536 +0,0 @@ -use crate::bash::parse_shell_lc_plain_commands; -use crate::command_safety::is_dangerous_command::executable_name_lookup_key; -#[cfg(windows)] -use crate::command_safety::windows_safe_commands::is_safe_command_windows; -#[cfg(windows)] -use crate::command_safety::windows_safe_commands::is_safe_powershell_words as is_safe_powershell_words_windows; - -pub fn is_known_safe_command(command: &[String]) -> bool { - let command: Vec = command - .iter() - .map(|s| { - if s == "zsh" { - "bash".to_string() - } else { - s.clone() - } - }) - .collect(); - - #[cfg(windows)] - { - if is_safe_command_windows(&command) { - return true; - } - } - - if is_safe_to_call_with_exec(&command) { - return true; - } - - // Support `bash -lc "..."` where the script consists solely of one or - // more "plain" commands (only bare words / quoted strings) combined with - // a conservative allow‑list of shell operators that themselves do not - // introduce side effects ( "&&", "||", ";", and "|" ). If every - // individual command in the script is itself a known‑safe command, then - // the composite expression is considered safe. - if let Some(all_commands) = parse_shell_lc_plain_commands(&command) - && !all_commands.is_empty() - && all_commands - .iter() - .all(|cmd| is_safe_to_call_with_exec(cmd)) - { - return true; - } - false -} - -/// Returns whether already-tokenized PowerShell words are read-only enough to -/// be auto-approved by the Windows safelist. -pub fn is_safe_powershell_words(command: &[String]) -> bool { - #[cfg(windows)] - { - is_safe_powershell_words_windows(command) - } - - #[cfg(not(windows))] - { - let _ = command; - false - } -} - -fn is_safe_to_call_with_exec(command: &[String]) -> bool { - let Some(cmd0) = command.first().map(String::as_str) else { - return false; - }; - - match executable_name_lookup_key(cmd0).as_deref() { - Some(cmd) if cfg!(target_os = "linux") && matches!(cmd, "numfmt" | "tac") => true, - - #[rustfmt::skip] - Some( - "cat" | - "cd" | - "cut" | - "echo" | - "expr" | - "false" | - "grep" | - "head" | - "id" | - "ls" | - "nl" | - "paste" | - "pwd" | - "rev" | - "seq" | - "stat" | - "tail" | - "tr" | - "true" | - "uname" | - "uniq" | - "wc" | - "which" | - "whoami") => { - true - }, - - Some("base64") => { - const UNSAFE_BASE64_OPTIONS: &[&str] = &["-o", "--output"]; - - !command.iter().skip(1).any(|arg| { - UNSAFE_BASE64_OPTIONS.contains(&arg.as_str()) - || arg.starts_with("--output=") - || (arg.starts_with("-o") && arg != "-o") - }) - } - - Some("find") => { - // Certain options to `find` can delete files, write to files, or - // execute arbitrary commands, so we cannot auto-approve the - // invocation of `find` in such cases. - #[rustfmt::skip] - const UNSAFE_FIND_OPTIONS: &[&str] = &[ - // Options that can execute arbitrary commands. - "-exec", "-execdir", "-ok", "-okdir", - // Option that deletes matching files. - "-delete", - // Options that write pathnames to a file. - "-fls", "-fprint", "-fprint0", "-fprintf", - ]; - - !command - .iter() - .any(|arg| UNSAFE_FIND_OPTIONS.contains(&arg.as_str())) - } - - // Ripgrep - Some("rg") => { - const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &[ - // Takes an arbitrary command that is executed for each match. - "--pre", - // Takes a command that can be used to obtain the local hostname. - "--hostname-bin", - ]; - const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &[ - // Calls out to other decompression tools, so do not auto-approve - // out of an abundance of caution. - "--search-zip", - "-z", - ]; - - !command.iter().any(|arg| { - UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg.as_str()) - || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS - .iter() - .any(|&opt| arg == opt || arg.starts_with(&format!("{opt}="))) - }) - } - - // Repository configuration can make even read-only Git commands execute helpers. - Some("git") => false, - - // Special-case `sed -n {N|M,N}p` - Some("sed") - if { - command.len() <= 4 - && command.get(1).map(String::as_str) == Some("-n") - && is_valid_sed_n_arg(command.get(2).map(String::as_str)) - } => - { - true - } - - // ── anything else ───────────────────────────────────────────────── - _ => false, - } -} - -// (bash parsing helpers implemented in crate::bash) - -/* ---------------------------------------------------------- -Example ----------------------------------------------------------- */ - -/// Returns true if `arg` matches /^(\d+,)?\d+p$/ -fn is_valid_sed_n_arg(arg: Option<&str>) -> bool { - // unwrap or bail - let s = match arg { - Some(s) => s, - None => return false, - }; - - // must end with 'p', strip it - let core = match s.strip_suffix('p') { - Some(rest) => rest, - None => return false, - }; - - // split on ',' and ensure 1 or 2 numeric parts - let parts: Vec<&str> = core.split(',').collect(); - match parts.as_slice() { - // single number, e.g. "10" - [num] => !num.is_empty() && num.chars().all(|c| c.is_ascii_digit()), - - // two numbers, e.g. "1,5" - [a, b] => { - !a.is_empty() - && !b.is_empty() - && a.chars().all(|c| c.is_ascii_digit()) - && b.chars().all(|c| c.is_ascii_digit()) - } - - // anything else (more than one comma) is invalid - _ => false, - } -} - -#[cfg(test)] -mod tests { - use super::*; - use std::string::ToString; - - fn vec_str(args: &[&str]) -> Vec { - args.iter().map(ToString::to_string).collect() - } - - #[test] - fn known_safe_examples() { - assert!(is_safe_to_call_with_exec(&vec_str(&["ls"]))); - assert!(is_safe_to_call_with_exec(&vec_str(&["base64"]))); - assert!(is_safe_to_call_with_exec(&vec_str(&[ - "sed", "-n", "1,5p", "file.txt" - ]))); - assert!(is_safe_to_call_with_exec(&vec_str(&[ - "nl", - "-nrz", - "Cargo.toml" - ]))); - - // Safe `find` command (no unsafe options). - assert!(is_safe_to_call_with_exec(&vec_str(&[ - "find", ".", "-name", "file.txt" - ]))); - - if cfg!(target_os = "linux") { - assert!(is_safe_to_call_with_exec(&vec_str(&["numfmt", "1000"]))); - assert!(is_safe_to_call_with_exec(&vec_str(&["tac", "Cargo.toml"]))); - } else { - assert!(!is_safe_to_call_with_exec(&vec_str(&["numfmt", "1000"]))); - assert!(!is_safe_to_call_with_exec(&vec_str(&["tac", "Cargo.toml"]))); - } - } - - #[test] - fn git_commands_are_not_known_safe() { - for args in [ - vec_str(&["git", "status", "--short"]), - vec_str(&["git", "log", "-p", "-1"]), - vec_str(&["git", "diff"]), - vec_str(&["git", "show", "HEAD"]), - vec_str(&["git", "branch"]), - vec_str(&["git", "branch", "--show-current"]), - vec_str(&["git", "--version"]), - vec_str(&["/usr/bin/git", "status"]), - vec_str(&["bash", "-lc", "git status"]), - vec_str(&["zsh", "-lc", "cd nested && git status"]), - vec_str(&["bash", "-lc", "git diff | head -20"]), - ] { - assert!( - !is_known_safe_command(&args), - "Git must not be trusted from its arguments alone: {args:?}", - ); - } - } - - #[test] - fn cargo_check_is_not_safe() { - assert!(!is_known_safe_command(&vec_str(&["cargo", "check"]))); - } - - #[test] - fn zsh_lc_safe_command_sequence() { - assert!(is_known_safe_command(&vec_str(&["zsh", "-lc", "ls"]))); - } - - #[test] - fn unknown_or_partial() { - assert!(!is_safe_to_call_with_exec(&vec_str(&["foo"]))); - assert!(!is_safe_to_call_with_exec(&vec_str(&["git", "fetch"]))); - assert!(!is_safe_to_call_with_exec(&vec_str(&[ - "sed", "-n", "xp", "file.txt" - ]))); - - // Unsafe `find` commands. - for args in [ - vec_str(&["find", ".", "-name", "file.txt", "-exec", "rm", "{}", ";"]), - vec_str(&[ - "find", ".", "-name", "*.py", "-execdir", "python3", "{}", ";", - ]), - vec_str(&["find", ".", "-name", "file.txt", "-ok", "rm", "{}", ";"]), - vec_str(&["find", ".", "-name", "*.py", "-okdir", "python3", "{}", ";"]), - vec_str(&["find", ".", "-delete", "-name", "file.txt"]), - vec_str(&["find", ".", "-fls", "/etc/passwd"]), - vec_str(&["find", ".", "-fprint", "/etc/passwd"]), - vec_str(&["find", ".", "-fprint0", "/etc/passwd"]), - vec_str(&["find", ".", "-fprintf", "/root/suid.txt", "%#m %u %p\n"]), - ] { - assert!( - !is_safe_to_call_with_exec(&args), - "expected {args:?} to be unsafe" - ); - } - } - - #[test] - fn base64_output_options_are_unsafe() { - for args in [ - vec_str(&["base64", "-o", "out.bin"]), - vec_str(&["base64", "--output", "out.bin"]), - vec_str(&["base64", "--output=out.bin"]), - vec_str(&["base64", "-ob64.txt"]), - ] { - assert!( - !is_safe_to_call_with_exec(&args), - "expected {args:?} to be considered unsafe due to output option" - ); - } - } - - #[test] - fn ripgrep_rules() { - // Safe ripgrep invocations – none of the unsafe flags are present. - assert!(is_safe_to_call_with_exec(&vec_str(&[ - "rg", - "Cargo.toml", - "-n" - ]))); - - // Unsafe flags that do not take an argument (present verbatim). - for args in [ - vec_str(&["rg", "--search-zip", "files"]), - vec_str(&["rg", "-z", "files"]), - ] { - assert!( - !is_safe_to_call_with_exec(&args), - "expected {args:?} to be considered unsafe due to zip-search flag", - ); - } - - // Unsafe flags that expect a value, provided in both split and = forms. - for args in [ - vec_str(&["rg", "--pre", "pwned", "files"]), - vec_str(&["rg", "--pre=pwned", "files"]), - vec_str(&["rg", "--hostname-bin", "pwned", "files"]), - vec_str(&["rg", "--hostname-bin=pwned", "files"]), - ] { - assert!( - !is_safe_to_call_with_exec(&args), - "expected {args:?} to be considered unsafe due to external-command flag", - ); - } - } - - #[test] - fn windows_powershell_full_path_is_safe() { - if !cfg!(windows) { - // Windows only because on Linux path splitting doesn't handle `/` separators properly - return; - } - - let Some(powershell) = crate::powershell::try_find_pwsh_executable_blocking() - .or_else(crate::powershell::try_find_powershell_executable_blocking) - else { - return; - }; - let powershell = powershell.as_path().to_str().unwrap(); - - assert!(is_known_safe_command(&vec_str(&[ - powershell, - "-Command", - "Get-Location", - ]))); - } - - #[test] - fn windows_git_full_path_is_not_safe() { - if !cfg!(windows) { - return; - } - - assert!(!is_known_safe_command(&vec_str(&[ - r"C:\Program Files\Git\cmd\git.exe", - "status", - ]))); - } - - #[test] - fn bash_lc_safe_examples() { - assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls"]))); - assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls -1"]))); - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "grep -R \"Cargo.toml\" -n" - ]))); - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "sed -n 1,5p file.txt" - ]))); - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "sed -n '1,5p' file.txt" - ]))); - - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "find . -name file.txt" - ]))); - } - - #[test] - fn bash_lc_safe_examples_with_operators() { - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "grep -R \"Cargo.toml\" -n || true" - ]))); - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "ls && pwd" - ]))); - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "echo 'hi' ; ls" - ]))); - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "ls | wc -l" - ]))); - } - - #[test] - fn bash_lc_unsafe_examples() { - assert!( - !is_known_safe_command(&vec_str(&["bash", "-lc", "git", "status"])), - "Four arg version is not known to be safe." - ); - assert!( - !is_known_safe_command(&vec_str(&["bash", "-lc", "'git status'"])), - "The extra quoting around 'git status' makes it a program named 'git status' and is therefore unsafe." - ); - - assert!( - !is_known_safe_command(&vec_str(&["bash", "-lc", "find . -name file.txt -delete"])), - "Unsafe find option should not be auto-approved." - ); - - // Disallowed because of unsafe command in sequence. - assert!( - !is_known_safe_command(&vec_str(&["bash", "-lc", "ls && rm -rf /"])), - "Sequence containing unsafe command must be rejected" - ); - - // Disallowed because of parentheses / subshell. - assert!( - !is_known_safe_command(&vec_str(&["bash", "-lc", "(ls)"])), - "Parentheses (subshell) are not provably safe with the current parser" - ); - assert!( - !is_known_safe_command(&vec_str(&["bash", "-lc", "ls || (pwd && echo hi)"])), - "Nested parentheses are not provably safe with the current parser" - ); - - // Disallowed redirection. - assert!( - !is_known_safe_command(&vec_str(&["bash", "-lc", "ls > out.txt"])), - "> redirection should be rejected" - ); - } - - #[test] - fn direct_powershell_words_use_windows_safelist() { - let command = vec_str(&["Get-Content", "Cargo.toml"]); - - if cfg!(windows) { - assert!(is_safe_powershell_words(&command)); - } else { - assert!(!is_safe_powershell_words(&command)); - } - } - - #[cfg(unix)] - #[test] - fn non_windows_safe_classification_does_not_spawn_repo_powershell_path() { - use std::fs; - use std::io::Write; - use std::os::unix::fs::PermissionsExt; - use std::time::SystemTime; - use std::time::UNIX_EPOCH; - - let unique = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("system clock should be after unix epoch") - .as_nanos(); - let temp_dir = std::env::temp_dir().join(format!( - "codex-safe-command-pwsh-test-{}-{unique}", - std::process::id() - )); - fs::create_dir(&temp_dir).expect("create temp dir for fake pwsh"); - let fake_pwsh = temp_dir.join("pwsh"); - let marker = temp_dir.join("marker"); - let quoted_marker = marker.to_string_lossy().replace('\'', "'\\''"); - - let mut script = fs::File::create(&fake_pwsh).expect("create fake pwsh"); - writeln!( - script, - "#!/bin/sh\nprintf spawned > '{quoted_marker}'\nexit 0" - ) - .expect("write fake pwsh"); - let mut permissions = fs::metadata(&fake_pwsh) - .expect("stat fake pwsh") - .permissions(); - permissions.set_mode(0o755); - fs::set_permissions(&fake_pwsh, permissions).expect("make fake pwsh executable"); - - assert!(!is_known_safe_command(&[ - fake_pwsh.to_string_lossy().into_owned(), - "-Command".to_string(), - "Get-ChildItem".to_string(), - ])); - assert!( - !marker.exists(), - "non-Windows safety classification must not spawn a PowerShell-looking path" - ); - - fs::remove_dir_all(temp_dir).expect("remove temp dir"); - } -} diff --git a/codex-rs/shell-command/src/command_safety/mod.rs b/codex-rs/shell-command/src/command_safety/mod.rs index 3225841e08..44d0103a9d 100644 --- a/codex-rs/shell-command/src/command_safety/mod.rs +++ b/codex-rs/shell-command/src/command_safety/mod.rs @@ -6,7 +6,4 @@ mod powershell_parser; mod powershell_tree_sitter; pub mod is_dangerous_command; -pub mod is_safe_command; -#[cfg(windows)] -pub(crate) mod windows_safe_commands; pub(crate) use powershell_tree_sitter::try_parse_powershell_commands; diff --git a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs deleted file mode 100644 index 3395108d23..0000000000 --- a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs +++ /dev/null @@ -1,503 +0,0 @@ -use crate::command_safety::try_parse_powershell_commands; -use std::path::Path; - -/// On Windows, we conservatively allow only clearly read-only PowerShell invocations -/// that match a small safelist. Anything else (including direct CMD commands) is unsafe. -pub fn is_safe_command_windows(command: &[String]) -> bool { - if let Some(commands) = try_parse_powershell_command_sequence(command) { - commands - .iter() - .all(|cmd| is_safe_powershell_words(cmd.as_slice())) - } else { - // Only PowerShell invocations are allowed on Windows for now; anything else is unsafe. - false - } -} - -/// Returns each command sequence if the invocation starts with a PowerShell binary. -/// For example, the tokens from `pwsh Get-ChildItem | Measure-Object` become two sequences. -fn try_parse_powershell_command_sequence(command: &[String]) -> Option>> { - let (exe, rest) = command.split_first()?; - if is_powershell_executable(exe) { - parse_powershell_invocation(exe, rest) - } else { - None - } -} - -/// Parses a PowerShell invocation into discrete command vectors, rejecting unsafe patterns. -fn parse_powershell_invocation(executable: &str, args: &[String]) -> Option>> { - if args.is_empty() { - // Examples rejected here: "pwsh" and "powershell.exe" with no additional arguments. - return None; - } - - let mut idx = 0; - while idx < args.len() { - let arg = &args[idx]; - let lower = arg.to_ascii_lowercase(); - match lower.as_str() { - "-command" | "/command" | "-c" => { - let script = args.get(idx + 1)?; - if idx + 2 != args.len() { - // Reject if there is more than one token representing the actual command. - // Examples rejected here: "pwsh -Command foo bar" and "powershell -c ls extra". - return None; - } - return parse_powershell_script(executable, script); - } - _ if lower.starts_with("-command:") || lower.starts_with("/command:") => { - if idx + 1 != args.len() { - // Reject if there are more tokens after the command itself. - // Examples rejected here: "pwsh -Command:dir C:\\" and "powershell /Command:dir C:\\" with trailing args. - return None; - } - let script = arg.split_once(':')?.1; - return parse_powershell_script(executable, script); - } - - // Benign, no-arg flags we tolerate. - "-nologo" | "-noprofile" | "-noninteractive" | "-mta" | "-sta" => { - idx += 1; - continue; - } - - // Explicitly forbidden/opaque or unnecessary for read-only operations. - "-encodedcommand" | "-ec" | "-file" | "/file" | "-windowstyle" | "-executionpolicy" - | "-workingdirectory" => { - // Examples rejected here: "pwsh -EncodedCommand ..." and "powershell -File script.ps1". - return None; - } - - // Unknown switch → bail conservatively. - _ if lower.starts_with('-') => { - // Examples rejected here: "pwsh -UnknownFlag" and "powershell -foo bar". - return None; - } - - // If we hit non-flag tokens, treat the remainder as a command sequence. - // This happens if powershell is invoked without -Command, e.g. - // ["pwsh", "-NoLogo", "git", "-c", "core.pager=cat", "status"] - _ => { - let script = join_arguments_as_script(&args[idx..]); - return parse_powershell_script(executable, &script); - } - } - } - - // Examples rejected here: "pwsh" and "powershell.exe -NoLogo" without a script. - None -} - -/// Tokenizes an inline PowerShell script and delegates to the command splitter. -/// Examples of when this is called: pwsh.exe -Command '