From d292f721242ef355ce5a38da90e0e33edc4790e6 Mon Sep 17 00:00:00 2001 From: Oliver Tepman Date: Wed, 14 Oct 2026 22:51:53 +0000 Subject: [PATCH] Allow stdin review under active grants with sandbox prompts disabled (#54119) ## Why With granular sandbox approvals disabled, `write_stdin` rejected input to terminals retaining additional permissions even when those permissions were still granted. ## What changed Let input reach approval review when the terminal's retained permissions match active grants for its environment. Require identical filesystem grants, including entry order, and an active network grant when retained permissions enable network access. Each input still requires review. Keep rejecting input when retained authority is no longer covered or the approval policy changes to `Never`. ## Testing Add regression coverage for user and Guardian review, turn grant expiry, session grants, and switching to `Never`. Cover filesystem restrictions, protected metadata, symlinks, and ordered denials, and verify that rejected input never reaches the terminal. GitOrigin-RevId: 9abf7da8d03b1526d26ca2a718e36349a255330b --- codex-rs/core/src/tools/approvals.rs | 13 +- .../core/src/unified_exec/stdin_approval.rs | 35 ++ .../suite/unified_exec_stdin_approval.rs | 353 ++++++++++++++++++ 3 files changed, 397 insertions(+), 4 deletions(-) diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 2052f5948c..7840013fe3 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -86,6 +86,8 @@ pub(crate) enum ApprovalAction { tty: bool, sandbox_permissions: SandboxPermissions, additional_permissions: Option, + #[serde(skip)] + additional_permissions_preapproved: bool, }, ApplyPatch { id: String, @@ -167,6 +169,7 @@ impl ApprovalAction { tty, sandbox_permissions, additional_permissions, + additional_permissions_preapproved: _, } => PermissionRequestPayload { tool_name: HookToolName::new("write_stdin"), tool_input: serde_json::json!({ @@ -299,6 +302,7 @@ impl ApprovalAction { tty, sandbox_permissions, additional_permissions, + additional_permissions_preapproved: _, } => crate::guardian::GuardianApprovalRequest::WriteStdin { id, approval_id, @@ -449,11 +453,12 @@ impl Session { action: ApprovalAction, ctx: ApprovalContext, ) -> ApprovalResult { - // Stdin that exceeds current permissions needs a fresh sandbox approval. - // Strict review of ordinary input follows the same routing as ordinary exec. + // Retained authority outside current grants needs a new sandbox approval. + // Current grants permit review of the input; they never approve the input itself. let policy = ctx.review_context.turn().approval_policy(); - if matches!(&action, ApprovalAction::WriteStdin { sandbox_permissions, .. } - if sandbox_permissions.requests_sandbox_override()) + if matches!(&action, ApprovalAction::WriteStdin { sandbox_permissions, additional_permissions_preapproved, .. } + if sandbox_permissions.requests_sandbox_override() + && (!additional_permissions_preapproved || matches!(policy, AskForApproval::Never))) && !(ctx.strict_auto_review && matches!(policy, AskForApproval::Never)) && let Some(reason) = prompt_is_rejected_by_policy(policy, /*prompt_is_rule*/ false) diff --git a/codex-rs/core/src/unified_exec/stdin_approval.rs b/codex-rs/core/src/unified_exec/stdin_approval.rs index 646ea8ec9b..35d3e363d2 100644 --- a/codex-rs/core/src/unified_exec/stdin_approval.rs +++ b/codex-rs/core/src/unified_exec/stdin_approval.rs @@ -20,6 +20,7 @@ use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::PermissionProfile; use codex_protocol::models::SandboxPermissions; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::AskForApproval; use codex_sandboxing::policy_transforms::effective_permission_profile; use codex_sandboxing::policy_transforms::merge_permission_profiles; @@ -237,6 +238,39 @@ impl ProcessEntry { let reason = permissions .approval_reason(sandbox_permissions) .map_err(approval_error)?; + // A scoped grant can authorize retained authority without permitting new + // sandbox escalations. Still review every input, and only recognize grants + // that remain active for this terminal's environment with the same policy. + let additional_permissions_preapproved = matches!( + context.step_context.turn.approval_policy(), + AskForApproval::Granular(policy) if !policy.sandbox_approval + ) && sandbox_permissions + == SandboxPermissions::WithAdditionalPermissions + && permissions + .additional_permissions + .as_ref() + .zip( + context + .step_context + .turn + .granted_permissions(&self.environment_id), + ) + .is_some_and(|(retained, current_grants)| { + // Root containment does not prove filesystem coverage: child + // restrictions, protected metadata, and symlinks can differ. + // Require the original filesystem grants, including their order. + retained.file_system == current_grants.file_system + && (retained + .network + .as_ref() + .and_then(|network| network.enabled) + != Some(true) + || current_grants + .network + .as_ref() + .and_then(|network| network.enabled) + == Some(true)) + }); let action = ApprovalAction::WriteStdin { id: self.call_id.clone(), approval_id: context.call_id.clone(), @@ -247,6 +281,7 @@ impl ProcessEntry { tty: self.tty, sandbox_permissions, additional_permissions: permissions.additional_permissions.clone(), + additional_permissions_preapproved, }; Ok(Some((action, reason))) } diff --git a/codex-rs/core/tests/suite/unified_exec_stdin_approval.rs b/codex-rs/core/tests/suite/unified_exec_stdin_approval.rs index 705eccf35e..ad945521e2 100644 --- a/codex-rs/core/tests/suite/unified_exec_stdin_approval.rs +++ b/codex-rs/core/tests/suite/unified_exec_stdin_approval.rs @@ -8,6 +8,7 @@ use codex_extension_api::ExtensionRegistryBuilder; use codex_features::Feature; use codex_protocol::approvals::ExecApprovalKind; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; @@ -22,6 +23,7 @@ use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::request_permissions::PermissionGrantScope; use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::user_input::UserInput; +use core_test_support::create_directory_symlink; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; @@ -30,6 +32,7 @@ use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_remote; use core_test_support::skip_if_sandbox; use core_test_support::skip_if_target_windows; use core_test_support::test_codex::TestCodexBuilder; @@ -428,3 +431,353 @@ async fn strict_stdin_review_reaches_guardian_with_sandbox_prompts_disabled() -> assert!(output.to_string().contains("STDIN_REVIEW_OK"), "{output}"); Ok(()) } + +#[derive(Clone, Copy)] +enum StdinReviewer { + User, + Guardian, +} + +#[derive(Clone, Copy)] +enum GrantChange { + Unchanged, + ReadOnlyChildExpires, + MetadataWriteExpires, + SymlinkWriteExpires, + UnchangedOrderedDenies, +} + +#[derive(Clone, Copy)] +enum LaterApprovalPolicy { + Inherit, + Never, +} + +/// A grant active at launch permits later input to reach review without enabling +/// sandbox escalation, including grants with interleaved denials. Expired authority +/// beneath parent grants and Never policy must block input without delivery. +#[test_case::test_case(PermissionGrantScope::Turn, StdinReviewer::User, GrantChange::Unchanged, LaterApprovalPolicy::Inherit; "turn_grant_user_review")] +#[test_case::test_case(PermissionGrantScope::Turn, StdinReviewer::Guardian, GrantChange::Unchanged, LaterApprovalPolicy::Inherit; "turn_grant_strict_review")] +#[test_case::test_case(PermissionGrantScope::Session, StdinReviewer::User, GrantChange::Unchanged, LaterApprovalPolicy::Never; "session_grant_never_policy")] +#[test_case::test_case(PermissionGrantScope::Session, StdinReviewer::User, GrantChange::ReadOnlyChildExpires, LaterApprovalPolicy::Inherit; "read_only_child_grant_expiry")] +#[test_case::test_case(PermissionGrantScope::Session, StdinReviewer::User, GrantChange::MetadataWriteExpires, LaterApprovalPolicy::Inherit; "explicit_metadata_grant_expiry")] +#[test_case::test_case(PermissionGrantScope::Session, StdinReviewer::User, GrantChange::SymlinkWriteExpires, LaterApprovalPolicy::Inherit; "symlink_grant_expiry")] +#[test_case::test_case(PermissionGrantScope::Session, StdinReviewer::User, GrantChange::UnchangedOrderedDenies, LaterApprovalPolicy::Inherit; "unchanged_interleaved_denials")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn stdin_reviews_current_grants_with_sandbox_prompts_disabled( + scope: PermissionGrantScope, + reviewer: StdinReviewer, + grant_change: GrantChange, + later_policy: LaterApprovalPolicy, +) -> Result<()> { + skip_if_target_windows!(Ok(()), "uses a POSIX interactive shell"); + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + if matches!(grant_change, GrantChange::SymlinkWriteExpires) { + skip_if_remote!(Ok(()), "creates a symlink on the local executor filesystem"); + if !cfg!(target_os = "linux") { + eprintln!("Skipping Linux symlink grant regression on another host OS"); + return Ok(()); + } + } + let harness = harness(test_codex()).await?; + let test = harness.test(); + harness.create_dir_all("allowed").await?; + let allowed = test.workspace_path_uri("allowed")?.to_abs_path()?; + let mut permission_profile = PermissionProfile::read_only(); + let mut grants = vec![(json!({"network":{"enabled":true}}), scope)]; + match grant_change { + GrantChange::ReadOnlyChildExpires + | GrantChange::MetadataWriteExpires + | GrantChange::SymlinkWriteExpires => { + let child = if matches!(grant_change, GrantChange::MetadataWriteExpires) { + "allowed/.git" + } else if matches!(grant_change, GrantChange::SymlinkWriteExpires) { + "allowed/link" + } else { + "allowed/child" + }; + if matches!(grant_change, GrantChange::SymlinkWriteExpires) { + harness.create_dir_all("outside").await?; + create_directory_symlink(&harness.path("outside"), &harness.path(child)); + } else { + harness.create_dir_all(child).await?; + } + let child = test.workspace_path_uri(child)?.to_abs_path()?; + if matches!(grant_change, GrantChange::ReadOnlyChildExpires) { + let mut file_system = permission_profile.file_system_sandbox_policy(); + file_system.entries.push(FileSystemSandboxEntry::new( + child.clone().into(), + FileSystemAccessMode::Read, + )); + permission_profile = PermissionProfile::from_runtime_permissions( + &file_system, + NetworkSandboxPolicy::Restricted, + ); + } + grants = vec![ + (json!({"file_system":{"write":[allowed]}}), scope), + ( + json!({"file_system":{"write":[child]}}), + PermissionGrantScope::Turn, + ), + ]; + } + GrantChange::UnchangedOrderedDenies => { + harness + .write_file("allowed/a/private/secret", "secret") + .await?; + harness.create_dir_all("allowed/b").await?; + let write_a = + FileSystemSandboxEntry::new(allowed.join("a").into(), FileSystemAccessMode::Write); + let deny_a = FileSystemSandboxEntry::new( + allowed.join("a/private").into(), + FileSystemAccessMode::Deny, + ); + let write_b = + FileSystemSandboxEntry::new(allowed.join("b").into(), FileSystemAccessMode::Write); + let retained = FileSystemPermissions { + entries: vec![write_a, deny_a], + glob_scan_max_depth: None, + }; + let extra = FileSystemPermissions { + entries: vec![write_b], + glob_scan_max_depth: None, + }; + grants = vec![ + (json!({"file_system":retained}), scope), + (json!({"file_system":extra}), scope), + ]; + } + GrantChange::Unchanged => {} + } + let turn_grant_expires = matches!( + grant_change, + GrantChange::ReadOnlyChildExpires + | GrantChange::MetadataWriteExpires + | GrantChange::SymlinkWriteExpires + ); + let decision = |outcome: &str| { + sse(vec![ + ev_assistant_message(outcome, &json!({"risk_level":"low", "user_authorization":"high", "outcome":outcome, "rationale":"stdin grant test"}).to_string()), + ev_completed(outcome), + ]) + }; + let mut responses = Vec::new(); + for (id, (permissions, _)) in ["grant", "extra_grant"].into_iter().zip(&grants) { + responses.push(tool_response( + id, + "request_permissions", + json!({"reason":"allow terminal access", "permissions":permissions}), + )); + } + responses.push(tool_response( + "open", + "exec_command", + json!({"cmd":"/bin/bash --noprofile --norc", "tty":true, "yield_time_ms":200}), + )); + if matches!(reviewer, StdinReviewer::Guardian) { + responses.push(decision("allow")); + } + let grant_probe = match grant_change { + GrantChange::ReadOnlyChildExpires => "printf launch > allowed/child/launched && ", + GrantChange::MetadataWriteExpires => "printf launch > allowed/.git/launched && ", + GrantChange::SymlinkWriteExpires => "printf launch > allowed/link/launched && ", + GrantChange::UnchangedOrderedDenies => { + "printf launch > allowed/a/launched && printf launch > allowed/b/launched && ! cat allowed/a/private/secret && " + } + GrantChange::Unchanged => "", + }; + responses.push(tool_response("allowed", "write_stdin", json!({"session_id":1000, "chars":format!("{grant_probe}printf 'CURRENT_%s_OK\\n' GRANT\n")}))); + if matches!(reviewer, StdinReviewer::Guardian) { + responses.push(decision("allow")); + } + responses.push(sse(vec![ev_completed("done")])); + let writes = mount_sse_sequence(harness.server(), responses).await; + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "grant permission, open a terminal, and send reviewed input".into(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + permission_profile: Some(permission_profile), + approval_policy: Some(AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: false, + rules: false, + skill_approval: true, + request_permissions: true, + mcp_elicitations: false, + })), + ..Default::default() + }), + ) + .await?; + let mut grant_scopes = grants.into_iter().map(|(_, scope)| scope); + let mut approvals = 0; + loop { + match wait_for_event(&test.codex, |_| true).await { + EventMsg::RequestPermissions(request) => { + test.codex + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: RequestPermissionsResponse { + permissions: request.permissions, + scope: grant_scopes.next().expect("requested grant"), + strict_auto_review: matches!(reviewer, StdinReviewer::Guardian), + }, + }) + .await?; + } + EventMsg::ExecApprovalRequest(request) => { + assert!(matches!(reviewer, StdinReviewer::User)); + assert_eq!( + (request.kind, request.effective_approval_id()), + (ExecApprovalKind::WriteStdin, "allowed".to_string()) + ); + approvals += 1; + test.codex + .submit(Op::ExecApproval { + id: request.effective_approval_id(), + turn_id: Some(request.turn_id), + decision: ReviewDecision::Approved, + }) + .await?; + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + assert_eq!(grant_scopes.next(), None); + assert_eq!( + approvals, + usize::from(matches!(reviewer, StdinReviewer::User)) + ); + let output = writes + .function_call_output_text("allowed") + .expect("stdin output"); + assert!(output.contains("CURRENT_GRANT_OK"), "{output}"); + if matches!(reviewer, StdinReviewer::Guardian) { + assert_eq!( + writes + .requests() + .iter() + .filter(|request| request.body_contains_text("Planned action JSON:")) + .count(), + 2 + ); + return Ok(()); + } + + let later = mount_function_call_agent_response( + harness.server(), + "later", + &json!({"session_id":1000, "chars":"LATER=1; printf 'LATER_%s_OK\\n' GRANT\n"}).to_string(), + "write_stdin", + ) + .await; + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "send input in a later turn".into(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approval_policy: match later_policy { + LaterApprovalPolicy::Inherit => None, + LaterApprovalPolicy::Never => Some(AskForApproval::Never), + }, + ..Default::default() + }), + ) + .await?; + let later_input_allowed = matches!(scope, PermissionGrantScope::Session) + && !turn_grant_expires + && matches!(later_policy, LaterApprovalPolicy::Inherit); + let mut later_approvals = 0; + loop { + match wait_for_event(&test.codex, |_| true).await { + EventMsg::ExecApprovalRequest(request) => { + assert!(later_input_allowed, "unexpected stdin review: {request:?}"); + assert_eq!( + (request.kind, request.effective_approval_id()), + (ExecApprovalKind::WriteStdin, "later".to_string()) + ); + later_approvals += 1; + test.codex + .submit(Op::ExecApproval { + id: request.effective_approval_id(), + turn_id: Some(request.turn_id), + decision: ReviewDecision::Approved, + }) + .await?; + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + assert_eq!(later_approvals, usize::from(later_input_allowed)); + let output = later + .completion + .function_call_output_text("later") + .expect("later stdin output"); + if matches!(later_policy, LaterApprovalPolicy::Never) { + assert!( + output.contains("AskForApproval is set to Never"), + "{output}" + ); + } else if later_input_allowed { + assert!(output.contains("LATER_GRANT_OK"), "{output}"); + } else { + assert!( + output.contains("AskForApproval::Granular.sandbox_approval is false"), + "{output}" + ); + } + if !later_input_allowed { + let probe = mount_function_call_agent_response( + harness.server(), + "probe", + &json!({"session_id":1000, "chars":"test -z \"${LATER-}\" && printf 'REJECTED_%s_OK\\n' INPUT; exit\n"}).to_string(), + "write_stdin", + ) + .await; + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "verify rejected input was not delivered".into(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::OnRequest), + ..Default::default() + }), + ) + .await?; + loop { + match wait_for_event(&test.codex, |_| true).await { + EventMsg::ExecApprovalRequest(request) => { + assert_eq!( + (request.kind, request.effective_approval_id()), + (ExecApprovalKind::WriteStdin, "probe".to_string()) + ); + test.codex + .submit(Op::ExecApproval { + id: request.effective_approval_id(), + turn_id: Some(request.turn_id), + decision: ReviewDecision::Approved, + }) + .await?; + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + let output = probe + .completion + .function_call_output_text("probe") + .expect("probe output"); + assert!(output.contains("REJECTED_INPUT_OK"), "{output}"); + } + Ok(()) +}