Skip to content

fix(acp): reject unattended permission requests - #4609

Open
jmecom wants to merge 3 commits into
mainfrom
codex/security-acp-shell-auto-approval
Open

fix(acp): reject unattended permission requests#4609
jmecom wants to merge 3 commits into
mainfrom
codex/security-acp-shell-auto-approval

Conversation

@jmecom

@jmecom jmecom commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

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

jmecom added 2 commits August 3, 2026 13:44
Default managed sessions to dontAsk and reject or cancel permission prompts instead of selecting allow_once. Explicit owner-selected non-interactive modes remain available.

Co-authored-by: Jordan Mecom <jm@squareup.com>
Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Jordan Mecom <jm@squareup.com>
Signed-off-by: Jordan Mecom <jm@squareup.com>
@jmecom
jmecom marked this pull request as ready for review August 3, 2026 21:00
@jmecom
jmecom requested a review from a team as a code owner August 3, 2026 21:00
The permission tests re-implemented the `reject_once` lookup in the test
body rather than calling the code under test, so they would all still
pass if the harness went back to selecting `allow_once`. They could not
call it directly: `handle_permission_request` is a method on `AcpClient`,
which owns a live `Child` and its stdio pipes.

Extract the choice into `permission_denial_response` and point the tests
at it. No behaviour change. This covers the cancelled fallback, which had
no test despite being the fail-closed backstop for adapters that offer no
`reject_once`, plus the empty-option-list and missing-`optionId` edges.

Also drops `find_allow_once_returns_none_when_absent`, which asserted a
property of a search no production path performs any more.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Eli Foster <efoster@squareup.com>

@elifoster-block elifoster-block left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - expanded test coverage and removed an unused function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants