mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix(core) RequestPermissions + ApplyPatch (#14055)
## Summary The apply_patch tool should also respect AdditionalPermissions ## Testing - [x] Added unit tests --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
66e71cce11
commit
c1defcc98c
@@ -18,6 +18,7 @@ use crate::tools::context::ToolOutput;
|
||||
use crate::tools::context::ToolPayload;
|
||||
use crate::tools::events::ToolEmitter;
|
||||
use crate::tools::events::ToolEventCtx;
|
||||
use crate::tools::handlers::apply_granted_turn_permissions;
|
||||
use crate::tools::handlers::parse_arguments;
|
||||
use crate::tools::orchestrator::ToolOrchestrator;
|
||||
use crate::tools::registry::ToolHandler;
|
||||
@@ -30,7 +31,10 @@ use crate::tools::spec::JsonSchema;
|
||||
use async_trait::async_trait;
|
||||
use codex_apply_patch::ApplyPatchAction;
|
||||
use codex_apply_patch::ApplyPatchFileChange;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::collections::BTreeSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
pub struct ApplyPatchHandler;
|
||||
@@ -61,6 +65,31 @@ fn to_abs_path(cwd: &Path, path: &Path) -> Option<AbsolutePathBuf> {
|
||||
AbsolutePathBuf::resolve_path_against_base(path, cwd).ok()
|
||||
}
|
||||
|
||||
fn write_permissions_for_paths(file_paths: &[AbsolutePathBuf]) -> Option<PermissionProfile> {
|
||||
let write_paths = file_paths
|
||||
.iter()
|
||||
.map(|path| {
|
||||
path.parent()
|
||||
.unwrap_or_else(|| path.clone())
|
||||
.into_path_buf()
|
||||
})
|
||||
.collect::<BTreeSet<_>>()
|
||||
.into_iter()
|
||||
.map(AbsolutePathBuf::from_absolute_path)
|
||||
.collect::<Result<Vec<_>, _>>()
|
||||
.ok()?;
|
||||
|
||||
let permissions = (!write_paths.is_empty()).then_some(PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(write_paths),
|
||||
}),
|
||||
..Default::default()
|
||||
})?;
|
||||
|
||||
crate::sandboxing::normalize_additional_permissions(permissions).ok()
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl ToolHandler for ApplyPatchHandler {
|
||||
fn kind(&self) -> ToolKind {
|
||||
@@ -119,6 +148,12 @@ impl ToolHandler for ApplyPatchHandler {
|
||||
InternalApplyPatchInvocation::DelegateToExec(apply) => {
|
||||
let changes = convert_apply_patch_to_protocol(&apply.action);
|
||||
let file_paths = file_paths_for_action(&apply.action);
|
||||
let effective_additional_permissions = apply_granted_turn_permissions(
|
||||
session.as_ref(),
|
||||
crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
write_permissions_for_paths(&file_paths),
|
||||
)
|
||||
.await;
|
||||
let emitter =
|
||||
ToolEmitter::apply_patch(changes.clone(), apply.auto_approved);
|
||||
let event_ctx = ToolEventCtx::new(
|
||||
@@ -134,6 +169,12 @@ impl ToolHandler for ApplyPatchHandler {
|
||||
file_paths,
|
||||
changes,
|
||||
exec_approval_requirement: apply.exec_approval_requirement,
|
||||
sandbox_permissions: effective_additional_permissions
|
||||
.sandbox_permissions,
|
||||
additional_permissions: effective_additional_permissions
|
||||
.additional_permissions,
|
||||
permissions_preapproved: effective_additional_permissions
|
||||
.permissions_preapproved,
|
||||
timeout_ms: None,
|
||||
codex_exe: turn.codex_linux_sandbox_exe.clone(),
|
||||
};
|
||||
@@ -222,6 +263,12 @@ pub(crate) async fn intercept_apply_patch(
|
||||
InternalApplyPatchInvocation::DelegateToExec(apply) => {
|
||||
let changes = convert_apply_patch_to_protocol(&apply.action);
|
||||
let approval_keys = file_paths_for_action(&apply.action);
|
||||
let effective_additional_permissions = apply_granted_turn_permissions(
|
||||
session.as_ref(),
|
||||
crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
write_permissions_for_paths(&approval_keys),
|
||||
)
|
||||
.await;
|
||||
let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved);
|
||||
let event_ctx = ToolEventCtx::new(
|
||||
session.as_ref(),
|
||||
@@ -236,6 +283,11 @@ pub(crate) async fn intercept_apply_patch(
|
||||
file_paths: approval_keys,
|
||||
changes,
|
||||
exec_approval_requirement: apply.exec_approval_requirement,
|
||||
sandbox_permissions: effective_additional_permissions.sandbox_permissions,
|
||||
additional_permissions: effective_additional_permissions
|
||||
.additional_permissions,
|
||||
permissions_preapproved: effective_additional_permissions
|
||||
.permissions_preapproved,
|
||||
timeout_ms,
|
||||
codex_exe: turn.codex_linux_sandbox_exe.clone(),
|
||||
};
|
||||
|
||||
@@ -23,6 +23,7 @@ use crate::tools::sandboxing::ToolRuntime;
|
||||
use crate::tools::sandboxing::with_cached_approval;
|
||||
use codex_apply_patch::ApplyPatchAction;
|
||||
use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::FileChange;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
@@ -37,6 +38,9 @@ pub struct ApplyPatchRequest {
|
||||
pub file_paths: Vec<AbsolutePathBuf>,
|
||||
pub changes: std::collections::HashMap<PathBuf, FileChange>,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub additional_permissions: Option<PermissionProfile>,
|
||||
pub permissions_preapproved: bool,
|
||||
pub timeout_ms: Option<u64>,
|
||||
pub codex_exe: Option<PathBuf>,
|
||||
}
|
||||
@@ -87,8 +91,8 @@ impl ApplyPatchRuntime {
|
||||
expiration: req.timeout_ms.into(),
|
||||
// Run apply_patch with a minimal environment for determinism and to avoid leaks.
|
||||
env: HashMap::new(),
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
additional_permissions: req.additional_permissions.clone(),
|
||||
justification: None,
|
||||
})
|
||||
}
|
||||
@@ -134,6 +138,9 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
let action = ApplyPatchRuntime::build_guardian_review_request(req);
|
||||
return review_approval_request(session, turn, action, retry_reason).await;
|
||||
}
|
||||
if req.permissions_preapproved && retry_reason.is_none() {
|
||||
return ReviewDecision::Approved;
|
||||
}
|
||||
if let Some(reason) = retry_reason {
|
||||
let rx_approve = session
|
||||
.request_patch_approval(turn, call_id, changes.clone(), Some(reason), None)
|
||||
@@ -244,6 +251,9 @@ mod tests {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
permissions_preapproved: false,
|
||||
timeout_ms: None,
|
||||
codex_exe: None,
|
||||
};
|
||||
|
||||
@@ -15,6 +15,7 @@ use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use core_test_support::responses::ev_apply_patch_function_call;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
@@ -60,6 +61,14 @@ fn exec_command_event(call_id: &str, command: &str) -> Result<Value> {
|
||||
Ok(ev_function_call(call_id, "exec_command", &args_str))
|
||||
}
|
||||
|
||||
fn build_add_file_patch(patch_path: &Path, content: &str) -> String {
|
||||
format!(
|
||||
"*** Begin Patch\n*** Add File: {}\n+{}\n*** End Patch\n",
|
||||
patch_path.display(),
|
||||
content
|
||||
)
|
||||
}
|
||||
|
||||
fn workspace_write_excluding_tmp() -> SandboxPolicy {
|
||||
SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
@@ -291,3 +300,121 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(target_os = "macos")]
|
||||
async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_without_prompt()
|
||||
-> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = workspace_write_excluding_tmp();
|
||||
let sandbox_policy_for_config = sandbox_policy.clone();
|
||||
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config);
|
||||
config
|
||||
.features
|
||||
.enable(Feature::RequestPermissions)
|
||||
.expect("test config should allow feature update");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::RequestPermissionsTool)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let requested_dir = tempfile::tempdir()?;
|
||||
let requested_file = requested_dir.path().join("allowed-patch.txt");
|
||||
let requested_permissions = requested_directory_write_permissions(requested_dir.path());
|
||||
let normalized_requested_permissions =
|
||||
normalized_directory_write_permissions(requested_dir.path())?;
|
||||
let patch = build_add_file_patch(&requested_file, "patched-via-request-permissions");
|
||||
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-patch-1"),
|
||||
request_permissions_tool_event(
|
||||
"permissions-call",
|
||||
"Allow patching outside the workspace",
|
||||
&requested_permissions,
|
||||
)?,
|
||||
ev_completed("resp-request-permissions-patch-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-patch-2"),
|
||||
ev_apply_patch_function_call("apply-patch-call", &patch),
|
||||
ev_completed("resp-request-permissions-patch-2"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-patch-3"),
|
||||
ev_assistant_message("msg-request-permissions-patch-1", "done"),
|
||||
ev_completed("resp-request-permissions-patch-3"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"patch outside the workspace",
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let granted_permissions = expect_request_permissions_event(&test, "permissions-call").await;
|
||||
assert_eq!(
|
||||
granted_permissions,
|
||||
normalized_requested_permissions.clone()
|
||||
);
|
||||
test.codex
|
||||
.submit(Op::RequestPermissionsResponse {
|
||||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
match event {
|
||||
EventMsg::TurnComplete(_) => {}
|
||||
EventMsg::ApplyPatchApprovalRequest(approval) => {
|
||||
panic!(
|
||||
"unexpected apply_patch approval request after granted permissions: {:?}",
|
||||
approval.call_id
|
||||
)
|
||||
}
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
|
||||
let patch_output = responses
|
||||
.function_call_output_text("apply-patch-call")
|
||||
.map(|output| json!({ "output": output }))
|
||||
.unwrap_or_else(|| panic!("expected apply-patch-call output"));
|
||||
let (exit_code, stdout) = parse_result(&patch_output);
|
||||
assert!(exit_code.is_none() || exit_code == Some(0));
|
||||
assert!(
|
||||
stdout.contains("Success."),
|
||||
"unexpected patch output: {stdout}"
|
||||
);
|
||||
assert_eq!(
|
||||
fs::read_to_string(&requested_file)?,
|
||||
"patched-via-request-permissions\n"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user