security(delegate): restrict model-controlled acp_command to the supported ACP transport

[claudlos]

delegate_task exposes acp_command / acp_args as free-form, model-controllable
tool-call arguments. They propagate unvalidated through child construction into
subprocess.Popen([acp_command] + acp_args, ...) (agent/copilot_acp_client.py),
so a prompt-injected delegate_task call with acp_command="python" (or
powershell/bash + inline code in acp_args) is arbitrary host-process execution
that bypasses the terminal backend's sandboxing and command approvals — a
model-reachable RCE crossing the OS boundary.

Add a copilot-only allowlist (_validate_acp_command / _SUPPORTED_ACP_COMMANDS)
enforced on BOTH the top-level and per-task model-supplied overrides, before any
child agent is built. A bare copilot or an absolute path ending in /copilot
(custom install location) is accepted; everything else returns a clear tool
error. Operator-configured delegation.command (creds["command"]) is trusted
and is not routed through the check. Schema descriptions updated to state the
restriction.

Supersedes #39462 (stale / merge-conflicted), which first proposed the
copilot-only allowlist; this carries that approach forward, rebased on current
main, with top-level + per-task enforcement and regression tests. Credit also to
@nikshepsvn (#27426) for the earlier acp_command crash-hardening attempt. The
companion credential-inheritance half of this finding (the ACP subprocess
inheriting os.environ secrets) is addressed separately in the
"sanitize agent subprocess env" change.

Co-authored-by: ssiweifnag 120658181+ssiweifnag@users.noreply.github.com
Co-authored-by: nikshepsvn 23241247+nikshepsvn@users.noreply.github.com
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

[alt-glitch]

This was generated by AI during triage.

Related to #39462, which first proposed the copilot-only acp_command allowlist (now stale / merge-conflicted). This PR carries that approach forward on current main with both top-level and per-task enforcement plus regression tests. Credit also to #27426 for the earlier acp_command crash-hardening. Not a duplicate — it supersedes #39462 with adaptations.

[tonydwb]

Code Review Summary

Verdict: Approved

This security PR restricts the model-supplied acp_command override in delegate_task to a known allowlist, preventing arbitrary host binary execution (CWE-78). Previously, a model could set acp_command="python" or acp_command="/usr/bin/malicious" and it would reach subprocess.Popen.

Key observations

• _SUPPORTED_ACP_COMMANDS allowlist contains only {"copilot"}
• _validate_acp_command() rejects non-allowlisted values with a clear error
• Validation runs at both top-level and per-task levels
• Path components are stripped (e.g. /usr/local/bin/copilot matches copilot)
• Comprehensive test coverage: allowlist, rejection, field name propagation

Looks Good

• Clean defense-in-depth: validates before any child agent is built
• Test suite covers all three exit paths (success, transport failure, non-200)
• Existing ACP args test updated to use allowed command
• Allowlist is intentionally narrow (copilot only) — new ACP transports require explicit allowlist expansion

---

Reviewed by Hermes Agent (cron)

[egilewski]

requesting changes

The new validator blocks obvious arbitrary command names like python, but it also accepts any path whose basename is copilot. That means the model can still provide a value such as /tmp/copilot or another writable/location-controlled path ending in that basename, and delegate_task will pass it through as override_acp_command before agent/copilot_acp_client.py executes [self._acp_command] + self._acp_args with subprocess.Popen. The transport has not been restricted to the supported GitHub Copilot CLI in that case; it is still a model-selected host binary outside the terminal approval/sandbox path, only renamed to copilot.

The model-facing override should not accept arbitrary paths by basename. It should be constrained to the supported command identity (copilot / platform executable name), with custom installation paths kept in trusted operator config (delegation.command) or another non-model-controlled resolution path.

Security evidence:

Area	Source	Sink / boundary	Evidence	Result
Model-controlled top-level acp_command	delegate_task(..., acp_command=...)	Child ACP subprocess command	PR head validates this path before child construction and blocks python	Partially fixed
Model-controlled per-task tasks[i].acp_command	Task object / recovered JSON task array	Child ACP subprocess command	PR head validates each task override before child construction	Partially fixed
Arbitrary path named copilot	acp_command="/tmp/copilot"	subprocess.Popen([self._acp_command] + self._acp_args) in agent/copilot_acp_client.py	Focused validator probe on PR head returned null for /tmp/copilot, so it is accepted	Still vulnerable
Operator config delegation.command	User configuration	Child ACP subprocess command	The PR intentionally leaves creds["command"] outside model validation	Acceptable trusted boundary
acp_args	Model-controlled tool parameter	Args to allowlisted command	No shell expansion; residual risk depends on the command identity being genuinely fixed	Secondary to command-path bypass

Signed: GPT-5.5-xhigh in Codex

[egilewski]

This still accepts arbitrary model-supplied paths whose basename is copilot, for example /tmp/copilot. Since that exact string later becomes the ACP subprocess command, a model can still select a host binary outside the terminal approval/sandbox path as long as it names it copilot; custom install paths should come from trusted operator config, not the model-facing override.

[egilewski]

looks mergeable

Security evidence:

Area	Source	Sink / boundary	Evidence	Result
Trust boundary	Model-controlled delegate_task arguments	Child ACP subprocess command selection	The model-facing top-level acp_command and per-task tasks[i].acp_command are validated before any child agent is built.	Fixed
Current-main reproduction	delegate_task(goal=..., acp_command="/tmp/copilot") on refreshed current main	_build_child_agent(..., override_acp_command="/tmp/copilot"), which later feeds the ACP subprocess transport	A focused mocked probe on current main showed build_called: true and override: "/tmp/copilot".	Vulnerable on main
PR validation	Replayed PR patch onto current main fd2a35b1691138b79b606e7961d3c78f7019722b	_validate_acp_command() before child construction	The probe rejected python, /tmp/copilot, /usr/local/bin/copilot, C:\tools\copilot.exe, and ./copilot, while allowing the bare supported command names.	Fixed on replay
Positive / negative tests	tests/tools/test_delegate.py on replayed current main	Top-level and per-task validation, path rejection, supported command identity	pytest tests/tools/test_delegate.py -q -p no:cacheprovider passed 150 passed, 2 warnings; py_compile also passed for the touched source and test files.	Covered
Residual bypass search	Diff and direct validator probes	Path-shaped model input, arbitrary binaries, operator-configured command boundary	The validator rejects any slash or backslash in model-supplied values and only strips known executable suffixes from bare names; trusted delegation.command remains outside the model-facing override.	No blocker found
Reviewer tool	CodeRabbit against base 3e99ec0ff99d1ec81d47f1814ad31453291718b6 after the required rate-limit wait	Independent review gate	CodeRabbit completed with findings only in files outside this PR diff; the PR diff remains limited to tools/delegate_tool.py and tests/tools/test_delegate.py, so no CodeRabbit finding was used as a PR blocker.	No PR blocker

Signed: GPT-5.5-xhigh in Codex

[egilewski]

looks mergeable

Security evidence:

Area	Source	Sink / boundary	Evidence	Result
Trust boundary	Model-controlled delegate_task acp_command and tasks[i].acp_command values	Child ACP subprocess command selection before subprocess.Popen	The current head validates both the top-level and per-task model-facing overrides before any child agent is built.	Fixed
Current-main reproduction	delegate_task(goal=..., acp_command="/tmp/copilot") on current main dbbf102b8e1877924353b001a8724ef533c8f2af	_build_child_agent(..., override_acp_command="/tmp/copilot"), which feeds the ACP subprocess transport	A mocked probe showed build_called: true and override: "/tmp/copilot", reproducing the model-selected path bypass on main.	Vulnerable on main
PR validation	Same mocked probe on current head 4ebcb0775da5e8bbc5a36b6f991dc4b4a2aa3873	_validate_acp_command() before child construction	The probe rejected /tmp/copilot before child construction; direct validator checks rejected python and /tmp/copilot while allowing bare copilot.	Fixed on PR head
Positive / negative tests	tests/tools/test_delegate.py focused allowlist and delegate rejection cases	Top-level validation, per-task validation, path rejection, bare command identity	pytest for the changed subset passed 9 passed; py_compile passed for tools/delegate_tool.py and tests/tools/test_delegate.py.	Covered
Residual bypass search	Diff inspection and direct validator probes	Path-shaped input, arbitrary binary names, Windows executable suffixes, trusted operator config boundary	The validator rejects / and \ before suffix normalization, strips only known executable suffixes from bare names, and leaves custom install paths to trusted delegation.command.	No blocker found
Reviewer tool	CodeRabbit clean-pass review plus final main refresh	Independent review and current-main compatibility	CodeRabbit completed with findings: 0; after main advanced to dbbf102b8e1877924353b001a8724ef533c8f2af, the new main changes did not touch the reviewed files and merge/diff checks still passed.	No PR blocker

Signed: GPT-5.5-xhigh in Codex

[teknium1]

Thanks for carrying this security fix forward. I verified the premise against current main and did not find a blocker in the current diff.

Verification

• Current main still forwards model-controlled acp_command into _build_child_agent(..., override_acp_command=...) without an allowlist at tools/delegate_tool.py:2289.
• The resulting ACP client stores that command and executes it with subprocess.Popen([self._acp_command] + self._acp_args, ...) at agent/copilot_acp_client.py:350 and agent/copilot_acp_client.py:437.
• The PR diff at ebe89871d9034c09d0bf9fa45a9a6c8c6f80f4a3 adds top-level and per-task validation before child construction, rejects path-shaped model input, and keeps trusted operator delegation.command outside the model-facing check.

The design fits the repo rubric: it is a narrow boundary fix in the existing delegate tool, with no new core tool, env var, hook, or cache/alternation impact. Salvageability looks high.

Automated hermes-sweeper review.

[teknium1]

Thanks for this — the security finding is real (a model-supplied acp_command reaches subprocess.Popen as a host binary), and the analysis and tests are solid.

We're closing it because the fix direction isn't the one we want. Restricting the value to a copilot-only allowlist treats the symptom: it keeps acp_command as a model-facing tool argument and filters what the model may pass. But choosing which subprocess binary drives the transport isn't a decision the model should make at all — it's operator/infrastructure config (delegation.command), the same way toolset selection is.

We just applied this principle to the sibling case in #56386, which removed the model-facing toolsets argument from delegate_task entirely (subagents inherit the parent's toolsets; the model can't name them). acp_command / acp_args will get the same treatment — removed from the model schema, resolved only from trusted operator config + parent inheritance — so the "model introduces a host binary" bug class becomes impossible rather than allowlisted.

Closing in favor of that direction. Your reproduction and the credential-inheritance half of the finding remain useful references. Credit to you and to @ssiweifnag (#39462) and @nikshepsvn (#27426) for surfacing the acp_command surface.