Reject oversized reviewed terminal input (#41159)

## Why

Reviewing a shortened `write_stdin` action while sending the complete input could allow unreviewed bytes to reach an escalated terminal.

## What changed

- Require the fully formatted action and approval reason to fit within 8,000 bytes.
- Reject truncated or oversized actions before requesting approval or writing any bytes to the terminal.
- Document the reviewed stdin size limit in the app-server protocol.

## Testing

Add an integration test that uses JSON escaping to push a formatted action over the limit and verifies that it is neither offered for approval nor executed, while a subsequent valid write still succeeds.

GitOrigin-RevId: f6c96a78a65a136366c8119a7d242dfc2ec05fe6
This commit is contained in:
jif
2026-08-27 14:54:13 +00:00
committed by copyberry
parent 453a9bcc69
commit bce96bcb43
6 changed files with 147 additions and 4 deletions
+1 -2
View File
@@ -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)]
+3 -1
View File
@@ -266,7 +266,9 @@ impl ApprovalAction {
}
}
fn into_guardian_request(self) -> std::io::Result<crate::guardian::GuardianApprovalRequest> {
pub(crate) fn into_guardian_request(
self,
) -> std::io::Result<crate::guardian::GuardianApprovalRequest> {
Ok(match self {
Self::ExecCommand {
id,
@@ -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,
};
+1
View File
@@ -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;
@@ -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(())
}