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(()) +}