From cb1978b913d2323cbf9fa6c6d69ad50971c1b567 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2026 15:46:56 -0700 Subject: [PATCH] core: enforce guardian review file permissions --- codex-rs/core/src/guardian/review_session.rs | 11 +- codex-rs/core/tests/suite/guardian_review.rs | 196 +++++++++++++++++++ 2 files changed, 206 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index b8c5c98075..07ea0407a2 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -921,9 +921,18 @@ pub(crate) fn build_guardian_review_session_config( guardian_config.notify = None; guardian_config.developer_instructions = None; guardian_config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); + let guardian_permission_profile = PermissionProfile::read_only(); + let (mut guardian_file_system_policy, guardian_network_policy) = + guardian_permission_profile.to_runtime_permissions(); + guardian_file_system_policy.preserve_deny_read_restrictions_from( + &parent_config.permissions.file_system_sandbox_policy(), + ); guardian_config .permissions - .set_permission_profile(PermissionProfile::read_only()) + .set_permission_profile(PermissionProfile::from_runtime_permissions( + &guardian_file_system_policy, + guardian_network_policy, + )) .map_err(|err| { anyhow::anyhow!("guardian review session could not set permission profile: {err}") })?; diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index df38fb4353..1583bedc2a 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -4,6 +4,12 @@ use anyhow::Result; use codex_core::config::Constrained; use codex_core::sandboxing::SandboxPermissions; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; @@ -20,6 +26,7 @@ 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::test_codex::test_codex; +use core_test_support::test_codex::turn_permission_fields; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; use serde_json::Value; @@ -168,3 +175,192 @@ printf '%s\n' "${@: -1}" >> "${payload_path}""#, Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_review_session_enforces_read_only_and_parent_deny_reads() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let mut builder = test_codex(); + let test = builder.build(&server).await?; + + let fixture_dir = test.cwd.path().join("guardian-review-deny-read"); + fs::create_dir_all(&fixture_dir)?; + let denied_path = fixture_dir.join("secret.env"); + let allowed_path = fixture_dir.join("notes.txt"); + let secret = "guardian review deny-read secret"; + let allowed = "guardian review allowed notes"; + fs::write(&denied_path, format!("{secret}\n"))?; + fs::write(&allowed_path, format!("{allowed}\n"))?; + + let parent_sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let mut file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd( + &parent_sandbox_policy, + test.cwd.path(), + ); + file_system_policy.entries.push(FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: format!("{}/**/*.env", test.cwd.path().display()), + }, + access: FileSystemAccessMode::Deny, + }); + let parent_permission_profile = PermissionProfile::from_runtime_permissions( + &file_system_policy, + NetworkSandboxPolicy::Restricted, + ); + let (sandbox_policy, permission_profile) = + turn_permission_fields(parent_permission_profile, test.cwd.path()); + + let parent_output_path = test.cwd.path().join("guardian-parent-approved.txt"); + let guardian_write_path = test.cwd.path().join("guardian-review-write.txt"); + let parent_command = format!( + "printf parent-approved > {}", + shlex::try_quote(parent_output_path.to_string_lossy().as_ref())? + ); + let guardian_write_command = format!( + "printf guardian-write > {}", + shlex::try_quote(guardian_write_path.to_string_lossy().as_ref())? + ); + let guardian_read_command = format!( + "read_status=0; cat {} || read_status=$?; cat {}; exit $read_status", + shlex::try_quote(denied_path.to_string_lossy().as_ref())?, + shlex::try_quote(allowed_path.to_string_lossy().as_ref())? + ); + + let parent_call_id = "parent-requires-guardian"; + let guardian_write_call_id = "guardian-write-probe"; + let guardian_read_call_id = "guardian-read-probe"; + let parent_tool_args = json!({ + "cmd": parent_command, + "yield_time_ms": 1_000_u64, + "sandbox_permissions": SandboxPermissions::RequireEscalated, + "justification": "Exercise Guardian approval routing.", + }); + let guardian_write_args = json!({ + "cmd": guardian_write_command, + "yield_time_ms": 1_000_u64, + }); + let guardian_read_args = json!({ + "cmd": guardian_read_command, + "yield_time_ms": 1_000_u64, + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-parent-tool"), + ev_function_call( + parent_call_id, + "exec_command", + &serde_json::to_string(&parent_tool_args)?, + ), + ev_completed("resp-parent-tool"), + ]), + sse(vec![ + ev_response_created("resp-guardian-probes"), + ev_function_call( + guardian_write_call_id, + "exec_command", + &serde_json::to_string(&guardian_write_args)?, + ), + ev_function_call( + guardian_read_call_id, + "exec_command", + &serde_json::to_string(&guardian_read_args)?, + ), + ev_completed("resp-guardian-probes"), + ]), + sse(vec![ + ev_response_created("resp-guardian-review"), + ev_assistant_message( + "msg-guardian-review", + &json!({ + "risk_level": "low", + "user_authorization": "high", + "outcome": "allow", + "rationale": "The parent command writes a marker file in the workspace.", + }) + .to_string(), + ), + ev_completed("resp-guardian-review"), + ]), + sse(vec![ + ev_response_created("resp-parent-done"), + ev_assistant_message("msg-parent-done", "done"), + ev_completed("resp-parent-done"), + ]), + ], + ) + .await; + + test.codex + .submit(Op::UserInput { + environments: None, + items: vec![UserInput::Text { + text: "run a command that requires Guardian review".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + cwd: Some(test.cwd.path().to_path_buf()), + approval_policy: Some(AskForApproval::OnRequest), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + sandbox_policy: Some(sandbox_policy), + permission_profile, + ..Default::default() + }, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + assert_eq!(fs::read_to_string(&parent_output_path)?, "parent-approved"); + assert!( + !guardian_write_path.exists(), + "guardian review should not be able to write files" + ); + + let guardian_write_output = responses + .function_call_output_text(guardian_write_call_id) + .expect("guardian write probe output"); + assert_denial_output( + &guardian_write_output, + "guardian write probe should be denied by the read-only profile", + ); + + let guardian_read_output = responses + .function_call_output_text(guardian_read_call_id) + .expect("guardian read probe output"); + assert!( + guardian_read_output.contains(allowed), + "guardian should still read allowed files: {guardian_read_output:?}" + ); + assert!( + !guardian_read_output.contains(secret), + "guardian deny-read secret leaked into tool output: {guardian_read_output:?}" + ); + assert_denial_output( + &guardian_read_output, + "guardian read probe should honor parent deny-read entries", + ); + + Ok(()) +} + +fn assert_denial_output(output: &str, context: &str) { + let output_lower = output.to_lowercase(); + let denied = output_lower.contains("permission denied") + || output_lower.contains("operation not permitted") + || output_lower.contains("read-only file system"); + assert!(denied, "{context}: {output:?}"); +}