diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 8558cabb7f..35e1b08b61 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1842,6 +1842,8 @@ Order of messages: When stdin approvals are enabled, a `write_stdin` approval sets `kind: "writeStdin"`, references the original terminal command's `itemId`, and has its own `approvalId`. The request belongs to the current turn, which may differ from the turn that opened the terminal. With `approvalsReviewer: "auto_review"`, the `item/autoApprovalReview/*` notifications likewise target the original command item and carry an action of type `writeStdin` with `approvalId`, `processId`, `stdin`, and `cwd`. For stdin approvals, `cwd` is the terminal’s launch directory, not its current working directory. Approving or denying a stdin write does not start, complete, or change the status of the parent command-execution item. +For reviewed stdin, the complete formatted action and approval reason must fit within 8,000 bytes. Oversized or truncated actions are rejected before any bytes reach the terminal, rather than reviewing a shortened input and executing the full input. + ### File change approvals Order of messages: diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 6c63588a0b..9f09b4c0e2 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -244,8 +244,7 @@ impl GuardianRejectionCircuitBreaker { } } -#[cfg(test)] -use approval_request::format_guardian_action_pretty; +pub(crate) use approval_request::format_guardian_action_pretty; #[cfg(test)] use approval_request::guardian_assessment_action; #[cfg(test)] diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 81a7cfced3..90b16c78d7 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -266,7 +266,9 @@ impl ApprovalAction { } } - fn into_guardian_request(self) -> std::io::Result { + pub(crate) fn into_guardian_request( + self, + ) -> std::io::Result { Ok(match self { Self::ExecCommand { id, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 2558833b01..1424239d50 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -798,6 +798,22 @@ impl UnifiedExecProcessManager { }; let _interaction_guard = locked_process.interaction_lock().lock_owned().await; if let Some(approval) = approval { + let approval_reason = "Send input to an existing escalated terminal. The cwd is its launch directory; the terminal's current directory and state may have changed.".to_string(); + let reviewed = crate::guardian::format_guardian_action_pretty( + &approval.clone().into_guardian_request().map_err(|err| { + UnifiedExecError::StdinApproval(ToolError::Rejected(err.to_string())) + })?, + ) + .map_err(|err| UnifiedExecError::StdinApproval(ToolError::Rejected(err.to_string())))?; + // Bound the entire serialized action plus its reason, including JSON + // escaping. Reject, never execute an unreviewed tail. + if reviewed.truncated + || reviewed.text.len().saturating_add(approval_reason.len()) > 8_000 + { + return Err(UnifiedExecError::StdinApproval(ToolError::Rejected( + "terminal input and permission details are too large to review safely; use a smaller input or start a new terminal with fewer grants".to_string(), + ))); + } let strict_auto_review = context .session .active_turn_context_and_strict_auto_review() @@ -809,7 +825,7 @@ impl UnifiedExecProcessManager { call_id: context.call_id.clone(), tool_name: ToolName::plain("write_stdin"), strict_auto_review, - approval_reason: Some("Send input to an existing escalated terminal. The cwd is its launch directory; the terminal's current directory and state may have changed.".to_string()), + approval_reason: Some(approval_reason), retry_reason: None, network_approval_context: None, }; diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index a34c08a902..5aba823765 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -170,6 +170,7 @@ mod turn_input_submission; mod turn_state; mod unified_exec; mod unified_exec_process_events; +mod unified_exec_stdin_review_size; #[cfg(unix)] mod unified_exec_zsh_fork_approvals; mod unstable_features_warning; diff --git a/codex-rs/core/tests/suite/unified_exec_stdin_review_size.rs b/codex-rs/core/tests/suite/unified_exec_stdin_review_size.rs new file mode 100644 index 0000000000..d4c8bb78ab --- /dev/null +++ b/codex-rs/core/tests/suite/unified_exec_stdin_review_size.rs @@ -0,0 +1,123 @@ +//! Stdin approval must cover the complete input before any bytes reach a terminal. + +use anyhow::Result; +use codex_core::TurnInputRequest; +use codex_features::Feature; +use codex_protocol::approvals::ExecApprovalKind; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; +use codex_protocol::protocol::SandboxPolicy; +use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::user_input::UserInput; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; +use core_test_support::responses::mount_sse_sequence; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::skip_if_sandbox; +use core_test_support::skip_if_target_windows; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn oversized_stdin_is_rejected_before_approval_or_execution() -> Result<()> { + skip_if_target_windows!(Ok(()), "uses a POSIX interactive shell"); + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + let server = start_mock_server().await; + let mut builder = test_codex().with_config(|config| { + config + .features + .enable(Feature::WriteStdinApproval) + .expect("enable stdin approvals"); + }); + let test = builder.build_with_auto_env(&server).await?; + // The raw input fits in 8KB, but its JSON escapes do not. None of the + // rejected input, including the trailing assignment, may reach the shell. + let oversized = format!("#{}\nREJECTED=1\n", "\"".repeat(4_100)); + let mut sequence = Vec::new(); + for (id, tool, args) in [ + ( + "open", + "exec_command", + json!({"cmd":"/bin/bash --noprofile --norc", "tty":true, "yield_time_ms":200, "sandbox_permissions":"require_escalated"}), + ), + ( + "oversized", + "write_stdin", + json!({"session_id":1000, "chars":oversized, "yield_time_ms":1000}), + ), + ( + "allowed", + "write_stdin", + json!({"session_id":1000, "chars":"test -z \"${REJECTED-}\" && printf 'STDIN_%s_OK\\n' REVIEW; exit\n", "yield_time_ms":1000}), + ), + ] { + sequence.push(sse(vec![ + ev_function_call(id, tool, &args.to_string()), + ev_completed(id), + ])); + } + sequence.push(sse(vec![ev_completed("done")])); + let responses = mount_sse_sequence(&server, sequence).await; + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "send terminal input".to_string(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::OnRequest), + sandbox_policy: Some(SandboxPolicy::ReadOnly { + network_access: false, + }), + ..Default::default() + }), + ) + .await?; + let mut approvals = Vec::new(); + loop { + match wait_for_event(&test.codex, |_| true).await { + EventMsg::ExecApprovalRequest(request) => { + let id = request.effective_approval_id(); + approvals.push((request.kind, id.clone())); + test.codex + .submit(Op::ExecApproval { + id, + turn_id: Some(request.turn_id), + decision: ReviewDecision::Approved, + }) + .await?; + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + assert_eq!( + approvals, + vec![ + (ExecApprovalKind::Command, "open".to_string()), + (ExecApprovalKind::WriteStdin, "allowed".to_string()), + ] + ); + let requests = responses.requests(); + let last = requests.last().expect("final model request"); + let rejected = last.function_call_output("oversized"); + assert!( + rejected.to_string().contains("too large to review safely"), + "{rejected}" + ); + let allowed = last.function_call_output("allowed"); + assert!(allowed.to_string().contains("STDIN_REVIEW_OK"), "{allowed}"); + test.codex.submit(Op::Shutdown).await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::ShutdownComplete) + }) + .await; + Ok(()) +}