Merge cb1978b913 into sapling-pr-archive-bolinfest

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}")
         })?;

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:?}");
+}