Skip to content

Commit ad538bf

Browse files
jmecomelifoster-blockclaude
authored
fix(acp): reject unattended permission requests (#4609)
This change removes the ACP permission-bypass mode, defaults managed sessions to `dontAsk`, and answers permission requests with `reject_once` or cancellation in both ACP read loops. Unattended operations that require interactive approval now fail closed instead of being silently authorized. Explicit non-interactive modes that do not bypass a permission request remain available. Both layers have to change together: `apply_permission_mode` treats an unsupported mode and a failed `set_config_option` as non-fatal by design, so a request can still reach the harness even in a non-interactive mode. Removing `bypassPermissions` from the enum rather than only changing the default means the mode cannot be restored by configuration alone. The scope of the guarantee is that `buzz-acp` never grants approval. An agent that pre-authorizes tools in its own configuration (for example Claude Code's `settings.json`) still runs them without asking, which is outside this harness. ## Testing - `env -u BUZZ_ACP_LAZY_POOL bin/cargo test -p buzz-acp` at `16fff4d`: 671 library tests and 9 integration tests passed - `cargo clippy -p buzz-acp --all-targets -- -D warnings` and `cargo fmt -p buzz-acp -- --check`: clean - `git diff --check origin/main...codex/security-acp-shell-auto-approval` The permission tests previously re-implemented the `reject_once` lookup in the test body instead of calling the code under test, so they would have passed unchanged if the harness went back to selecting `allow_once`. They could not call it directly, because `handle_permission_request` is a method on `AcpClient`, which owns a live `Child` and its stdio pipes. The choice is now a free function, `permission_denial_response`, and the tests exercise it: `reject_once` preferred over offered allow options, the cancelled fallback when no `reject_once` exists, an empty option list, and a `reject_once` missing its `optionId`. The cancelled fallback had no coverage before despite being the fail-closed backstop. ## Operator notes - `BUZZ_ACP_PERMISSION_MODE=bypassPermissions` no longer parses, so a process configured with it fails to start rather than silently downgrading. - Desktop managed agents do not set a permission mode, so they inherit `dontAsk`. The desktop has no permission prompt, so operations needing approval now fail with no in-app way to approve them. Originating Buzz thread: `buzz://message?channel=3928fe05-df61-4b5d-b9c7-d623b9b10ea1&id=3c6c02312f763fbe0d2bfc33a6c1a362f91d0354f3d18b039cf7a0558c1439d1` --------- Signed-off-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Eli Foster <efoster@squareup.com> Co-authored-by: Eli Foster <efoster@squareup.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 885bed3 commit ad538bf

4 files changed

Lines changed: 148 additions & 116 deletions

File tree

crates/buzz-acp/src/acp.rs

Lines changed: 115 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ pub struct AcpClient {
155155
/// a `cancelled` outcome before the agent returns from `session/prompt`.
156156
pending_permission_id: Option<serde_json::Value>,
157157
/// Whether we have already sent a response to the pending permission request.
158-
/// Guards against double-response if a timeout fires after the allow_once
158+
/// Guards against double-response if a timeout fires after the rejection
159159
/// response was written but before `pending_permission_id` was cleared.
160160
permission_responded: bool,
161161
/// The JSON-RPC id of the most recently sent `session/prompt` request.
@@ -1162,7 +1162,8 @@ impl AcpClient {
11621162
///
11631163
/// While waiting, handles:
11641164
/// - `session/update` notifications → logged via tracing
1165-
/// - `session/request_permission` requests → auto-approved with `allow_once`
1165+
/// - `session/request_permission` requests → rejected unless an owner has
1166+
/// already selected a non-interactive permission mode at session setup
11661167
/// - Any other messages → debug-logged and ignored; if they carry an `id`
11671168
/// (i.e. they are requests, not notifications), a JSON-RPC -32601 error is sent.
11681169
///
@@ -1870,12 +1871,12 @@ impl AcpClient {
18701871
}
18711872
}
18721873

1873-
/// Auto-approve a `session/request_permission` request from the agent.
1874+
/// Reject a `session/request_permission` request from the agent.
18741875
///
1875-
/// Finds the option with `kind == "allow_once"` and responds with its `optionId`.
1876-
/// If no `allow_once` option exists, falls back to `reject_once`.
1877-
///
1878-
/// **Critical:** Never hardcode `optionId` — always find it dynamically by `kind`.
1876+
/// Buzz has no human permission prompt in this harness, so selecting
1877+
/// `allow_once` would turn any admitted prompt into an implicit approval.
1878+
/// Find `reject_once` by kind when the adapter offers it; otherwise use the
1879+
/// protocol's cancelled outcome, which is also fail-closed.
18791880
///
18801881
/// The request `id` is stored as `serde_json::Value` to support both numeric
18811882
/// and string IDs per JSON-RPC 2.0.
@@ -1901,40 +1902,7 @@ impl AcpClient {
19011902
options.len()
19021903
);
19031904

1904-
// Find allow_once by kind — NEVER hardcode optionId.
1905-
let allow_once = options
1906-
.iter()
1907-
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once"));
1908-
1909-
let response = if let Some(opt) = allow_once {
1910-
let option_id = opt["optionId"]
1911-
.as_str()
1912-
.ok_or_else(|| AcpError::Protocol("allow_once option missing optionId".into()))?;
1913-
tracing::info!(
1914-
target: "acp::permission",
1915-
"auto-approving permission id={id} with allow_once optionId={option_id:?}"
1916-
);
1917-
permission_response_selected(&id, option_id)
1918-
} else {
1919-
// No allow_once — fall back to reject_once.
1920-
tracing::warn!(
1921-
target: "acp::permission",
1922-
"no allow_once option found in permission request id={id}, falling back to reject_once"
1923-
);
1924-
let reject = options
1925-
.iter()
1926-
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once"));
1927-
1928-
if let Some(opt) = reject {
1929-
let option_id = opt["optionId"].as_str().unwrap_or("reject");
1930-
permission_response_selected(&id, option_id)
1931-
} else {
1932-
return Err(AcpError::Protocol(
1933-
"no suitable permission option found (neither allow_once nor reject_once)"
1934-
.into(),
1935-
));
1936-
}
1937-
};
1905+
let response = permission_denial_response(&id, options)?;
19381906

19391907
// Write the response first, then mark as responded.
19401908
//
@@ -2046,6 +2014,42 @@ fn permission_response_cancelled(id: &serde_json::Value) -> serde_json::Value {
20462014
})
20472015
}
20482016

2017+
/// Choose the fail-closed response to a `session/request_permission` request.
2018+
///
2019+
/// Buzz has no human permission prompt in this harness, so selecting
2020+
/// `allow_once` would turn any admitted prompt into an implicit approval.
2021+
/// Prefer the adapter's `reject_once` option — matched by `kind`, never by a
2022+
/// hardcoded `optionId` — and fall back to the protocol's cancelled outcome for
2023+
/// adapters that do not offer one. Both answers deny.
2024+
///
2025+
/// Kept free of the client so the decision is testable without an agent
2026+
/// subprocess: `AcpClient` owns a real `Child` and its stdio pipes.
2027+
fn permission_denial_response(
2028+
id: &serde_json::Value,
2029+
options: &[serde_json::Value],
2030+
) -> Result<serde_json::Value, AcpError> {
2031+
let reject_once = options
2032+
.iter()
2033+
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once"));
2034+
2035+
let Some(opt) = reject_once else {
2036+
tracing::warn!(
2037+
target: "acp::permission",
2038+
"no reject_once option found in permission request id={id}, cancelling"
2039+
);
2040+
return Ok(permission_response_cancelled(id));
2041+
};
2042+
2043+
let option_id = opt["optionId"]
2044+
.as_str()
2045+
.ok_or_else(|| AcpError::Protocol("reject_once option missing optionId".into()))?;
2046+
tracing::info!(
2047+
target: "acp::permission",
2048+
"rejecting permission id={id} with reject_once optionId={option_id:?}"
2049+
);
2050+
Ok(permission_response_selected(id, option_id))
2051+
}
2052+
20492053
/// Full `session/new` response — session ID plus the raw JSON result.
20502054
///
20512055
/// Callers use the extractor helpers to pull model info from `raw`.
@@ -2300,63 +2304,96 @@ mod tests {
23002304
assert_eq!(StopReason::from_str("Refusal"), Some(StopReason::Refusal));
23012305
}
23022306

2307+
fn options(json: &str) -> Vec<serde_json::Value> {
2308+
serde_json::from_str(json).expect("option list")
2309+
}
2310+
2311+
fn outcome(response: &serde_json::Value) -> Option<&str> {
2312+
response["result"]["outcome"]["outcome"].as_str()
2313+
}
2314+
2315+
/// The offered `allow_once` and `allow_always` options must be ignored:
2316+
/// there is no human to click them, so choosing either would make every
2317+
/// admitted prompt an implicit approval. `optionId`s are deliberately
2318+
/// non-obvious to prove they are matched by `kind`, never hardcoded.
23032319
#[test]
2304-
fn find_allow_once_by_kind_not_by_option_id() {
2305-
// optionId values are intentionally non-obvious to prove we don't hardcode them.
2306-
let options: Vec<serde_json::Value> = serde_json::from_str(
2320+
fn permission_requests_select_reject_once_not_allow_once() {
2321+
let options = options(
23072322
r#"[
23082323
{"optionId": "opt-reject-42", "name": "Reject", "kind": "reject_once"},
23092324
{"optionId": "opt-allow-99", "name": "Allow once", "kind": "allow_once"},
23102325
{"optionId": "opt-always-7", "name": "Always allow", "kind": "allow_always"}
23112326
]"#,
2312-
)
2313-
.unwrap();
2327+
);
23142328

2315-
let allow_once = options
2316-
.iter()
2317-
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once"));
2329+
let response =
2330+
permission_denial_response(&serde_json::json!(7), &options).expect("denial response");
23182331

2319-
assert!(allow_once.is_some(), "should find allow_once option");
2320-
let opt = allow_once.unwrap();
2321-
// Found by kind, not by hardcoded optionId
2322-
assert_eq!(opt["kind"].as_str(), Some("allow_once"));
2323-
assert_eq!(opt["optionId"].as_str(), Some("opt-allow-99"));
2332+
assert_eq!(outcome(&response), Some("selected"));
2333+
assert_eq!(
2334+
response["result"]["outcome"]["optionId"].as_str(),
2335+
Some("opt-reject-42"),
2336+
"must select reject_once even when allow options are offered"
2337+
);
23242338
}
23252339

2340+
/// Fail-closed backstop: an adapter that offers no `reject_once` must still
2341+
/// be denied, via the protocol's cancelled outcome rather than an error or
2342+
/// an approval.
23262343
#[test]
2327-
fn find_allow_once_returns_none_when_absent() {
2328-
let options: Vec<serde_json::Value> = serde_json::from_str(
2344+
fn permission_request_without_reject_once_is_cancelled() {
2345+
let options = options(
23292346
r#"[
2330-
{"optionId": "reject-1", "name": "Reject", "kind": "reject_once"},
2331-
{"optionId": "reject-always", "name": "Always reject", "kind": "reject_always"}
2347+
{"optionId": "opt-allow-99", "name": "Allow once", "kind": "allow_once"},
2348+
{"optionId": "opt-always-7", "name": "Always allow", "kind": "allow_always"}
23322349
]"#,
2333-
)
2334-
.unwrap();
2350+
);
23352351

2336-
let allow_once = options
2337-
.iter()
2338-
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once"));
2352+
let response = permission_denial_response(&serde_json::json!("req-1"), &options)
2353+
.expect("cancelled response");
23392354

2340-
assert!(allow_once.is_none());
2355+
assert_eq!(outcome(&response), Some("cancelled"));
2356+
assert_eq!(
2357+
response["id"].as_str(),
2358+
Some("req-1"),
2359+
"string ids must round-trip per JSON-RPC 2.0"
2360+
);
23412361
}
23422362

2363+
/// An empty option list is the degenerate form of the same backstop.
23432364
#[test]
2344-
fn find_reject_once_fallback_when_no_allow_once() {
2345-
let options: Vec<serde_json::Value> = serde_json::from_str(
2346-
r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#,
2347-
)
2348-
.unwrap();
2365+
fn permission_request_with_no_options_is_cancelled() {
2366+
let response =
2367+
permission_denial_response(&serde_json::json!(1), &[]).expect("cancelled response");
23492368

2350-
let allow_once = options
2351-
.iter()
2352-
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once"));
2353-
assert!(allow_once.is_none());
2369+
assert_eq!(outcome(&response), Some("cancelled"));
2370+
}
23542371

2355-
let reject_once = options
2356-
.iter()
2357-
.find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once"));
2358-
assert!(reject_once.is_some());
2359-
assert_eq!(reject_once.unwrap()["optionId"].as_str(), Some("rej-x"));
2372+
/// A `reject_once` option missing its `optionId` is a protocol violation.
2373+
/// Erroring propagates to the caller, which tears the turn down — still no
2374+
/// approval is ever sent.
2375+
#[test]
2376+
fn reject_once_without_option_id_is_a_protocol_error() {
2377+
let options = options(r#"[{"name": "Reject", "kind": "reject_once"}]"#);
2378+
2379+
let err = permission_denial_response(&serde_json::json!(1), &options)
2380+
.expect_err("missing optionId must error");
2381+
2382+
assert!(matches!(err, AcpError::Protocol(_)), "got {err:?}");
2383+
}
2384+
2385+
#[test]
2386+
fn find_reject_once_by_kind() {
2387+
let options =
2388+
options(r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#);
2389+
2390+
let response =
2391+
permission_denial_response(&serde_json::json!(1), &options).expect("denial response");
2392+
2393+
assert_eq!(
2394+
response["result"]["outcome"]["optionId"].as_str(),
2395+
Some("rej-x")
2396+
);
23602397
}
23612398

23622399
#[test]

0 commit comments

Comments
 (0)