mirror of
https://github.com/l0ng-ai/openidea.git
synced 2026-11-15 08:08:18 +00:00
fix(agents): judge permission requests by their kind and command, not their title (#3006)
The Command Execution Policy and the permission card treated any request titled `Run …` as a shell command, but the title is free text the agent writes: a Codex request for wider sandbox access titled `Run git status` was approved by an allow entry `git status`. Only a tool call of kind `execute` now counts as a command, and only its structured `rawInput.command` is evaluated and shown; other requests ask with their own title. A command approval that also grants network access can still be blocked but is never auto-approved. Permission entries carry the command, so the card no longer parses the title; the native Codex driver puts the resolved command and any network context into rawInput. Co-authored-by: rohitdanda <10144914+rohitdanda@users.noreply.github.com>
This commit is contained in:
co-authored by
rohitdanda
parent
4dbb6920c8
commit
b43db63ff3
@@ -2266,15 +2266,19 @@ impl Task {
|
||||
.iter()
|
||||
.map(|question| question.answer_key.clone().unwrap_or_else(|| question.question.clone()))
|
||||
.collect();
|
||||
let command = security::extract_command(params);
|
||||
// What the card names as the command: not for a request that grants more than running it.
|
||||
let shown = command.as_deref().filter(|_| !security::widens_access(params)).map(shown_command);
|
||||
if let Some(hook) = self.shared.security.read().clone()
|
||||
&& let Some(command) = security::extract_command(params)
|
||||
&& let Some(command) = command.as_deref()
|
||||
{
|
||||
match security::evaluate_command(&command, &hook()) {
|
||||
match security::evaluate_command(command, &hook()) {
|
||||
security::PolicyDecision::Blocked(pattern) => {
|
||||
let reject = options.iter().find(|o| o.kind.starts_with("reject")).cloned();
|
||||
let _ = self.push_entry(AgentEntry::Permission {
|
||||
id,
|
||||
title,
|
||||
command: shown.clone(),
|
||||
options,
|
||||
questions: Vec::new(),
|
||||
chosen: Some(format!("Blocked by policy ({pattern})")),
|
||||
@@ -2284,7 +2288,7 @@ impl Task {
|
||||
None => json!({ "outcome": { "outcome": "cancelled" } }),
|
||||
};
|
||||
}
|
||||
security::PolicyDecision::Allowed => {
|
||||
security::PolicyDecision::Allowed if !security::widens_access(params) => {
|
||||
if let Some(allow) = options
|
||||
.iter()
|
||||
.find(|o| o.kind == "allow_once")
|
||||
@@ -2294,6 +2298,7 @@ impl Task {
|
||||
let _ = self.push_entry(AgentEntry::Permission {
|
||||
id,
|
||||
title,
|
||||
command: shown.clone(),
|
||||
options,
|
||||
questions: Vec::new(),
|
||||
chosen: Some(format!("{} (whitelisted)", allow.name)),
|
||||
@@ -2301,12 +2306,18 @@ impl Task {
|
||||
return json!({ "outcome": { "outcome": "selected", "optionId": allow.id } });
|
||||
}
|
||||
}
|
||||
security::PolicyDecision::AskUser => {}
|
||||
security::PolicyDecision::Allowed | security::PolicyDecision::AskUser => {}
|
||||
}
|
||||
}
|
||||
let (tx, rx) = std::sync::mpsc::channel();
|
||||
let entry =
|
||||
self.push_entry(AgentEntry::Permission { id, title, options: options.clone(), questions, chosen: None });
|
||||
let entry = self.push_entry(AgentEntry::Permission {
|
||||
id,
|
||||
title,
|
||||
command: shown,
|
||||
options: options.clone(),
|
||||
questions,
|
||||
chosen: None,
|
||||
});
|
||||
self.permissions.lock().insert(id, tx);
|
||||
self.set_status(AgentStatus::NeedsInput, None);
|
||||
self.approve_pending();
|
||||
@@ -2468,6 +2479,11 @@ fn permission_title(params: &Value) -> String {
|
||||
title.unwrap_or("The agent asks for permission").to_owned()
|
||||
}
|
||||
|
||||
/// A command as a permission card shows it: the script of a `<shell> -c <script>` line.
|
||||
fn shown_command(command: &str) -> String {
|
||||
shell_script(command).unwrap_or_else(|| command.to_owned())
|
||||
}
|
||||
|
||||
/// The script of a `<shell> -c <quoted script>` command line, when the whole line is exactly that.
|
||||
pub(crate) fn shell_script(command: &str) -> Option<String> {
|
||||
let (shell, rest) = command.split_once(' ')?;
|
||||
|
||||
@@ -384,6 +384,10 @@ pub enum AgentEntry {
|
||||
Permission {
|
||||
id: u32,
|
||||
title: String,
|
||||
/// The shell command the request asks to run, from its structured input; `None` for
|
||||
/// any other request, whatever its title says.
|
||||
#[serde(default)]
|
||||
command: Option<String>,
|
||||
options: Vec<AgentPermissionOption>,
|
||||
/// Set when the request asks `AskUserQuestion`'s questions; answered with a JSON object
|
||||
/// of question to answer instead of an option.
|
||||
|
||||
@@ -869,13 +869,14 @@ impl CodexShim {
|
||||
"item/commandExecution/requestApproval" => {
|
||||
let choices = translate::decisions(params, true);
|
||||
let title = translate::command_approval_title(params, item.as_ref());
|
||||
let call = tool_call_of(
|
||||
&item_id,
|
||||
&title,
|
||||
"execute",
|
||||
reason,
|
||||
Some(json!({ "command": params.get("command"), "cwd": params.get("cwd") })),
|
||||
);
|
||||
let mut input = json!({
|
||||
"command": translate::approval_command(params, item.as_ref()),
|
||||
"cwd": params.get("cwd"),
|
||||
});
|
||||
if let Some(network) = params.get("networkApprovalContext").filter(|c| !c.is_null()) {
|
||||
input["networkApprovalContext"] = network.clone();
|
||||
}
|
||||
let call = tool_call_of(&item_id, &title, "execute", reason, Some(input));
|
||||
Ok(self.ask(&thread, call, &choices, decline))
|
||||
}
|
||||
"item/fileChange/requestApproval" => {
|
||||
|
||||
@@ -757,19 +757,23 @@ pub(super) fn answer_of(choices: &[Choice], outcome: &Value, decline: Value) ->
|
||||
choices.iter().find(|c| selected && c.option["optionId"].as_str() == chosen).map_or(decline, |c| c.answer.clone())
|
||||
}
|
||||
|
||||
/// The command a command approval is about: the request's own, else its item's.
|
||||
pub(super) fn approval_command<'a>(params: &'a Value, item: Option<&'a Value>) -> Option<&'a str> {
|
||||
// Approval payloads can describe a different command from the item's earlier summary.
|
||||
let item = item.filter(|i| i.get("id") == params.get("itemId"));
|
||||
params
|
||||
.get("command")
|
||||
.and_then(Value::as_str)
|
||||
.filter(|c| !c.is_empty())
|
||||
.or_else(|| item.and_then(|i| i.get("command")).and_then(Value::as_str).filter(|c| !c.is_empty()))
|
||||
}
|
||||
|
||||
/// The ACP title of a command approval.
|
||||
pub(super) fn command_approval_title(params: &Value, item: Option<&Value>) -> String {
|
||||
if let Some(host) = params.pointer("/networkApprovalContext/host").and_then(Value::as_str) {
|
||||
return format!("Allow network access to {host}");
|
||||
}
|
||||
// Approval payloads can describe a different command from the item's earlier summary.
|
||||
let item = item.filter(|i| i.get("id") == params.get("itemId"));
|
||||
let command = params
|
||||
.get("command")
|
||||
.and_then(Value::as_str)
|
||||
.filter(|c| !c.is_empty())
|
||||
.or_else(|| item.and_then(|i| i.get("command")).and_then(Value::as_str).filter(|c| !c.is_empty()));
|
||||
match command {
|
||||
match approval_command(params, item) {
|
||||
Some(command) => format!("Run {}", crate::shell_script(command).unwrap_or_else(|| command.to_owned())),
|
||||
None => "Run a command".to_owned(),
|
||||
}
|
||||
|
||||
@@ -13,29 +13,23 @@ pub enum PolicyDecision {
|
||||
AskUser,
|
||||
}
|
||||
|
||||
/// Extracts the shell command string from an ACP `session/request_permission` payload
|
||||
/// when the request is for executing a command.
|
||||
/// The shell command an ACP `session/request_permission` asks to run: only a tool call of kind
|
||||
/// `execute`, and only its structured `rawInput` command. A title is free text the agent
|
||||
/// writes, so it never names the command.
|
||||
pub fn extract_command(params: &Value) -> Option<String> {
|
||||
let tool = params.get("toolCall").or_else(|| params.pointer("/subject/toolCall"))?;
|
||||
if tool.get("kind").and_then(Value::as_str) != Some("execute") {
|
||||
return None;
|
||||
}
|
||||
let raw_cmd = tool.pointer("/rawInput/command").or_else(|| tool.pointer("/rawInput/cmd"));
|
||||
if let Some(cmd) = raw_cmd.and_then(command_from_value) {
|
||||
return Some(cmd);
|
||||
}
|
||||
let kind = tool.get("kind").and_then(Value::as_str).unwrap_or_default();
|
||||
let name = tool.get("name").and_then(Value::as_str).unwrap_or_default();
|
||||
let title =
|
||||
tool.get("title").and_then(Value::as_str).or_else(|| params.get("title").and_then(Value::as_str))?.trim();
|
||||
if let Some(cmd) = title.strip_prefix("Run ").map(str::trim).filter(|s| !s.is_empty()) {
|
||||
return Some(cmd.to_owned());
|
||||
}
|
||||
if (kind == "execute" || matches!(name, "Bash" | "PowerShell" | "Execute" | "Run" | "terminal"))
|
||||
&& !title.is_empty()
|
||||
&& title != "Terminal"
|
||||
&& title != "Run a command"
|
||||
{
|
||||
return Some(title.to_owned());
|
||||
}
|
||||
None
|
||||
raw_cmd.and_then(command_from_value)
|
||||
}
|
||||
|
||||
/// Whether allowing the request grants more than running its command, such as network access
|
||||
/// for it: the command policy may block such a request but never approves it.
|
||||
pub fn widens_access(params: &Value) -> bool {
|
||||
let tool = params.get("toolCall").or_else(|| params.pointer("/subject/toolCall"));
|
||||
tool.and_then(|t| t.pointer("/rawInput/networkApprovalContext")).is_some_and(|c| !c.is_null())
|
||||
}
|
||||
|
||||
fn command_from_value(val: &Value) -> Option<String> {
|
||||
@@ -577,4 +571,30 @@ mod tests {
|
||||
});
|
||||
assert_eq!(extract_command(&write), None);
|
||||
}
|
||||
|
||||
/// Only a tool call of kind `execute` runs a command, and its command is in `rawInput`:
|
||||
/// a title says what the agent wants it to say.
|
||||
#[test]
|
||||
fn a_request_is_a_command_only_by_its_kind_and_input() {
|
||||
let escalation = json!({ "sessionId": "t", "toolCall": {
|
||||
"toolCallId": "i1", "title": "Run git status", "kind": "other", "status": "pending",
|
||||
"rawInput": { "fileSystem": { "write": ["C:\\"] }, "network": { "enabled": true } } },
|
||||
"options": [ { "optionId": "turn", "name": "Allow for this turn", "kind": "allow_once" },
|
||||
{ "optionId": "decline", "name": "Decline", "kind": "reject_once" } ] });
|
||||
assert_eq!(extract_command(&escalation), None);
|
||||
let titled_only = json!({ "toolCall": { "title": "Run git status", "kind": "execute" } });
|
||||
assert_eq!(extract_command(&titled_only), None);
|
||||
let named_only = json!({ "toolCall": { "title": "git status", "name": "Bash" } });
|
||||
assert_eq!(extract_command(&named_only), None);
|
||||
let mislabelled = json!({ "toolCall": { "title": "Run git status", "kind": "edit",
|
||||
"rawInput": { "command": "git status" } } });
|
||||
assert_eq!(extract_command(&mislabelled), None);
|
||||
let network = json!({ "toolCall": { "title": "Allow network access to example.com", "kind": "execute",
|
||||
"rawInput": { "command": "curl https://example.com", "networkApprovalContext": { "host": "example.com" } } } });
|
||||
assert_eq!(extract_command(&network).as_deref(), Some("curl https://example.com"));
|
||||
assert!(widens_access(&network));
|
||||
let plain = json!({ "toolCall": { "title": "Run git status", "kind": "execute",
|
||||
"rawInput": { "command": "git status" } } });
|
||||
assert!(!widens_access(&plain));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -264,6 +264,23 @@ impl Handler for FakeAgent {
|
||||
self.say(&session, &format!("late chose {chosen}"));
|
||||
return Ok(json!({"stopReason": "end_turn"}));
|
||||
}
|
||||
if text == "escalate" {
|
||||
// A request for wider access, titled with text the model wrote.
|
||||
let peer = self.latest();
|
||||
let answer: Value = tokio::runtime::Handle::current()
|
||||
.block_on(peer.request(
|
||||
"session/request_permission",
|
||||
json!({"sessionId": session, "toolCall": {"toolCallId": "e1", "title": "Run cargo test",
|
||||
"kind": "other", "rawInput": {"network": {"enabled": true}}},
|
||||
"options": [{"optionId": "yes", "name": "Allow for this turn", "kind": "allow_once"},
|
||||
{"optionId": "no", "name": "Decline", "kind": "reject_once"}]}),
|
||||
None,
|
||||
))
|
||||
.unwrap();
|
||||
let chosen = answer.pointer("/outcome/optionId").and_then(Value::as_str).unwrap_or("none");
|
||||
self.say(&session, &format!("escalation chose {chosen}"));
|
||||
return Ok(json!({"stopReason": "end_turn"}));
|
||||
}
|
||||
if text == "ask" {
|
||||
let peer = self.latest();
|
||||
let answer: Value = tokio::runtime::Handle::current()
|
||||
@@ -344,7 +361,8 @@ impl Handler for FakeAgent {
|
||||
let answer: Value = tokio::runtime::Handle::current()
|
||||
.block_on(peer.request(
|
||||
"session/request_permission",
|
||||
json!({"sessionId": session, "toolCall": {"toolCallId": "t1", "title": "Run cargo test"},
|
||||
json!({"sessionId": session, "toolCall": {"toolCallId": "t1", "title": "Run cargo test",
|
||||
"kind": "execute", "rawInput": {"command": "cargo test"}},
|
||||
"options": [{"optionId": "yes", "name": "Allow", "kind": "allow_once"},
|
||||
{"optionId": "no", "name": "Reject", "kind": "reject_once"}]}),
|
||||
None,
|
||||
@@ -3187,3 +3205,43 @@ async fn security_policy_auto_approves_whitelist_and_rejects_blacklist() {
|
||||
"{blocked_entries:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn security_policy_never_auto_approves_a_request_that_only_reads_like_a_command() {
|
||||
use ide_core::settings::AgentSecuritySettings;
|
||||
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let agent = Arc::new(FakeAgent::default());
|
||||
let mgr = manager(tmp.path(), agent);
|
||||
mgr.set_security_hook(Arc::new(|| AgentSecuritySettings {
|
||||
allowed_commands: vec!["cargo test".into()],
|
||||
blocked_commands: vec![],
|
||||
}));
|
||||
let task = mgr
|
||||
.create(NewAgentTask {
|
||||
project_root: None,
|
||||
prompt: text("escalate"),
|
||||
provider: "claude".into(),
|
||||
config: vec![],
|
||||
isolation: Isolation::Local,
|
||||
base: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
wait_for(&mgr, &task.id, AgentStatus::NeedsInput).await;
|
||||
let entries = mgr.entries(&task.id, None).unwrap();
|
||||
let pending = entries
|
||||
.iter()
|
||||
.find_map(|e| match e {
|
||||
AgentEntry::Permission { id, chosen: None, .. } => Some(*id),
|
||||
_ => None,
|
||||
})
|
||||
.unwrap_or_else(|| panic!("the request was answered without the user: {entries:?}"));
|
||||
mgr.respond(&task.id, pending, Some("no".into())).unwrap();
|
||||
wait_for(&mgr, &task.id, AgentStatus::Ready).await;
|
||||
let entries = mgr.entries(&task.id, None).unwrap();
|
||||
assert!(
|
||||
entries.iter().any(|e| matches!(e, AgentEntry::Agent { text } if text == "escalation chose no")),
|
||||
"{entries:?}"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -438,6 +438,7 @@ mod tests {
|
||||
t.push(AgentEntry::Permission {
|
||||
id: 1,
|
||||
title: "Run?".into(),
|
||||
command: None,
|
||||
options: vec![],
|
||||
questions: vec![],
|
||||
chosen: None,
|
||||
|
||||
@@ -49,6 +49,7 @@ const entries: AgentEntry[] = [
|
||||
type: "permission",
|
||||
id: 1,
|
||||
title: `Run ${command}`,
|
||||
command,
|
||||
options: [
|
||||
{ id: "once", kind: "allow_once", name: "Allow once" },
|
||||
{ id: "always", kind: "allow_always", name: `Always allow \`${command}\`` },
|
||||
|
||||
+6
-1
@@ -1897,7 +1897,12 @@ endedMs: number | null;
|
||||
/** Whether it can be stopped on its own. */
|
||||
cancellable: boolean } |
|
||||
/** A message between sessions in a subagent's conversation: its task (`incoming`) or its report. */
|
||||
{ type: "message"; incoming: boolean; text: string } | { type: "permission"; id: number; title: string; options: AgentPermissionOption[];
|
||||
{ type: "message"; incoming: boolean; text: string } | { type: "permission"; id: number; title: string;
|
||||
/**
|
||||
* The shell command the request asks to run, from its structured input; `None` for
|
||||
* any other request, whatever its title says.
|
||||
*/
|
||||
command?: string | null; options: AgentPermissionOption[];
|
||||
/**
|
||||
* Set when the request asks `AskUserQuestion`'s questions; answered with a JSON object
|
||||
* of question to answer instead of an option.
|
||||
|
||||
@@ -7,10 +7,12 @@ import {
|
||||
type PermissionEntry,
|
||||
} from "./permission";
|
||||
|
||||
const script = 'python3 -c \'from pathlib import Path\np = Path("note.txt")\np.write_text("one\\ntwo")\'';
|
||||
const entry: PermissionEntry = {
|
||||
type: "permission",
|
||||
id: 1,
|
||||
title: 'Run python3 -c \'from pathlib import Path\np = Path("note.txt")\np.write_text("one\\ntwo")\'',
|
||||
title: `Run ${script}`,
|
||||
command: script,
|
||||
options: [],
|
||||
chosen: null,
|
||||
};
|
||||
@@ -32,6 +34,24 @@ it("keeps a subagent command in the code area without its session label", () =>
|
||||
);
|
||||
});
|
||||
|
||||
it("shows a request as a command only when it carries one, whatever its title", () => {
|
||||
const escalation: PermissionEntry = {
|
||||
type: "permission",
|
||||
id: 2,
|
||||
title: "Run git status",
|
||||
options: [
|
||||
{ id: "turn", name: "Allow for this turn", kind: "allow_once" },
|
||||
{ id: "decline", name: "Decline", kind: "reject_once" },
|
||||
],
|
||||
chosen: null,
|
||||
};
|
||||
const details = permissionDetails(escalation);
|
||||
expect(details.command).toBeUndefined();
|
||||
expect(details.title).toBe("Allow tool call?");
|
||||
expect(details.detail).toBe("Run git status");
|
||||
expect(permissionDetails({ ...escalation, command: null }).command).toBeUndefined();
|
||||
});
|
||||
|
||||
describe("permission choices", () => {
|
||||
it.each([
|
||||
["allow_once", "yes", "Allow Once", "primary"],
|
||||
|
||||
@@ -4,7 +4,9 @@ import { tx } from "@/platform/i18n";
|
||||
export type PermissionEntry = Extract<AgentEntry, { type: "permission" }>;
|
||||
|
||||
export function permissionDetails(entry: PermissionEntry, provider = "Codex") {
|
||||
const command = entry.title.match(/^Run ([\s\S]+)/)?.[1] ?? entry.title.match(/^[^\n]+?: Run ([\s\S]+)/)?.[1];
|
||||
// The backend names the command from the request's structured input; the title is the
|
||||
// agent's own words and never makes a request a command.
|
||||
const command = entry.command ?? undefined;
|
||||
return {
|
||||
command,
|
||||
title: command ? tx`${provider} wants to run a command` : tx("Allow tool call?"),
|
||||
|
||||
@@ -245,7 +245,7 @@ describe("conversation rows", () => {
|
||||
tool("two"),
|
||||
{ type: "agent", text: "More detail" },
|
||||
tool("List .", { status: "running" }),
|
||||
{ type: "permission", id: 1, title: "Run python3", options: [], chosen },
|
||||
{ type: "permission", id: 1, title: "Run python3", command: "python3", options: [], chosen },
|
||||
{ type: "plan", items: [{ content: "Check files", status: "in_progress" }] },
|
||||
{ type: "thought", text: "Consider the result" },
|
||||
{ type: "agent", text: "Done" },
|
||||
@@ -955,7 +955,7 @@ describe("a pending permission request above the composer", () => {
|
||||
diffs: [],
|
||||
locations: [],
|
||||
},
|
||||
{ type: "permission", id: 12, title: `Run ${command}`, options, chosen: null },
|
||||
{ type: "permission", id: 12, title: `Run ${command}`, command, options, chosen: null },
|
||||
]);
|
||||
useAgents.setState({ entries: new Map(backend.entries) });
|
||||
render(<SessionEditor input={input("A")} />);
|
||||
@@ -978,7 +978,7 @@ describe("a pending permission request above the composer", () => {
|
||||
});
|
||||
|
||||
it.each(options)("sends the original $kind option id, then records it in the conversation", async (option) => {
|
||||
const pending: AgentEntry = { type: "permission", id: 12, title: `Run ${command}`, options, chosen: null };
|
||||
const pending: AgentEntry = { type: "permission", id: 12, title: `Run ${command}`, command, options, chosen: null };
|
||||
backend.answers = [];
|
||||
backend.entries.set("A", [pending]);
|
||||
useAgents.setState({ entries: new Map(backend.entries) });
|
||||
@@ -1001,7 +1001,7 @@ describe("a pending permission request above the composer", () => {
|
||||
});
|
||||
|
||||
it("is answered with a number key when the focus isn't in a text field", async () => {
|
||||
const pending: AgentEntry = { type: "permission", id: 12, title: `Run ${command}`, options, chosen: null };
|
||||
const pending: AgentEntry = { type: "permission", id: 12, title: `Run ${command}`, command, options, chosen: null };
|
||||
backend.answers = [];
|
||||
backend.entries.set("A", [pending]);
|
||||
useAgents.setState({ entries: new Map(backend.entries) });
|
||||
@@ -1024,6 +1024,7 @@ describe("a pending permission request above the composer", () => {
|
||||
type: "permission",
|
||||
id: 3,
|
||||
title: "Run ls d:/project",
|
||||
command: "ls d:/project",
|
||||
options: [
|
||||
{ id: "once", name: "Allow Once", kind: "allow_once" },
|
||||
{ id: "reject", name: "Reject", kind: "reject_once" },
|
||||
|
||||
Reference in New Issue
Block a user