From 39507eea5360b994d5f41f25e456d7f86733dc70 Mon Sep 17 00:00:00 2001 From: jif Date: Fri, 28 Aug 2026 15:26:25 +0000 Subject: [PATCH] Reject NUL bytes in reviewed terminal input (#41354) ## Why Manual terminal-input approvals shell-quote the proposed input, which cannot preserve NUL bytes for an accurate review. ## What changed Reject terminal input containing a NUL byte before creating an approval request or sending any of the input to the terminal. ## Testing Extend the unreviewable-input integration test to cover both oversized input and NUL-containing input, and verify that rejected input is not executed. GitOrigin-RevId: e738c59ba5570012663bf6168ad052be237ca226 --- .../core/src/unified_exec/stdin_approval.rs | 6 +++++ .../suite/unified_exec_stdin_review_size.rs | 27 +++++++++++-------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/unified_exec/stdin_approval.rs b/codex-rs/core/src/unified_exec/stdin_approval.rs index d07acb4813..5e4cba91b9 100644 --- a/codex-rs/core/src/unified_exec/stdin_approval.rs +++ b/codex-rs/core/src/unified_exec/stdin_approval.rs @@ -217,6 +217,12 @@ impl ProcessEntry { if sandbox_permissions == SandboxPermissions::UseDefault && !strict_auto_review { return Ok(None); } + // Manual approvals shell-quote the input, which cannot preserve NUL bytes. + if input.contains('\0') { + return Err(approval_error( + "terminal input contains a NUL byte and cannot be reviewed safely", + )); + } let reason = permissions .approval_reason(sandbox_permissions) .map_err(approval_error)?; 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 index d4c8bb78ab..b729d88606 100644 --- a/codex-rs/core/tests/suite/unified_exec_stdin_review_size.rs +++ b/codex-rs/core/tests/suite/unified_exec_stdin_review_size.rs @@ -24,8 +24,19 @@ use core_test_support::wait_for_event; use pretty_assertions::assert_eq; use serde_json::json; +// The oversized input fits in 8KB before JSON escaping. Neither rejected input +// may reach the shell, including the trailing assignment. +#[test_case::test_case( + format!("#{}\nREJECTED=1\n", "\"".repeat(4_100)), + "too large to review safely"; + "oversized" +)] +#[test_case::test_case("\0REJECTED=1\n".to_string(), "NUL byte"; "nul")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn oversized_stdin_is_rejected_before_approval_or_execution() -> Result<()> { +async fn unreviewable_stdin_is_rejected_before_approval_or_execution( + rejected_input: String, + expected_error: &str, +) -> Result<()> { skip_if_target_windows!(Ok(()), "uses a POSIX interactive shell"); skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); @@ -37,9 +48,6 @@ async fn oversized_stdin_is_rejected_before_approval_or_execution() -> Result<() .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 [ ( @@ -48,9 +56,9 @@ async fn oversized_stdin_is_rejected_before_approval_or_execution() -> Result<() json!({"cmd":"/bin/bash --noprofile --norc", "tty":true, "yield_time_ms":200, "sandbox_permissions":"require_escalated"}), ), ( - "oversized", + "rejected", "write_stdin", - json!({"session_id":1000, "chars":oversized, "yield_time_ms":1000}), + json!({"session_id":1000, "chars":rejected_input, "yield_time_ms":1000}), ), ( "allowed", @@ -107,11 +115,8 @@ async fn oversized_stdin_is_rejected_before_approval_or_execution() -> Result<() ); 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 rejected = last.function_call_output("rejected"); + assert!(rejected.to_string().contains(expected_error), "{rejected}"); let allowed = last.function_call_output("allowed"); assert!(allowed.to_string().contains("STDIN_REVIEW_OK"), "{allowed}"); test.codex.submit(Op::Shutdown).await?;