fix(acp): evaluate V4A multi-file patch paths individually for auto-approve
Multi-file V4A proposals set EditProposal.path to a comma-joined display string, and should_auto_approve_edit evaluated it as one path: the sensitive-name check saw only the last segment and the workspace check resolved the joined string under the session cwd. A patch touching .env plus a normal file auto-approved under 'session', and one carrying an absolute path outside the workspace auto-approved under 'workspace_session' — both without the interactive prompt. EditProposal now carries a paths tuple with every real target; the sensitive check runs any() and the workspace check all() across them, falling back to (path,) for single-file proposals. The joined string remains for display only. Fixes #115213
This commit is contained in:
@@ -30,6 +30,10 @@ class EditProposal:
|
||||
old_text: str | None
|
||||
new_text: str
|
||||
arguments: dict[str, Any]
|
||||
# Every file the edit will actually touch. ``path`` may be a comma-joined
|
||||
# display string for multi-file V4A patches; ``paths`` is the authoritative
|
||||
# set for auto-approve checks. Empty means "``path`` alone".
|
||||
paths: tuple[str, ...] = ()
|
||||
|
||||
|
||||
EditApprovalRequester = Callable[[EditProposal], bool]
|
||||
@@ -118,6 +122,7 @@ def _proposal_for_patch_v4a(arguments: dict[str, Any]) -> EditProposal:
|
||||
return EditProposal(
|
||||
"patch", paths[0] if single else ", ".join(paths),
|
||||
_read_text_if_exists(paths[0]) if single else None, patch_body, dict(arguments),
|
||||
tuple(paths),
|
||||
)
|
||||
|
||||
|
||||
@@ -145,16 +150,22 @@ def should_auto_approve_edit(proposal: EditProposal, policy: str, cwd: str | Non
|
||||
|
||||
Session-scoped and conservative: sensitive paths still ask under autonomous policies."""
|
||||
policy = str(policy or AUTO_APPROVE_ASK).strip()
|
||||
if policy == AUTO_APPROVE_ASK or _is_sensitive_auto_approve_path(proposal.path):
|
||||
# Multi-file V4A proposals join paths into one display string; the checks
|
||||
# must run per real target or a sensitive/escaped file hides in the join.
|
||||
paths = proposal.paths or (proposal.path,)
|
||||
if policy == AUTO_APPROVE_ASK or any(_is_sensitive_auto_approve_path(p) for p in paths):
|
||||
return False
|
||||
path = Path(proposal.path).expanduser().resolve(strict=False)
|
||||
resolved = [Path(p).expanduser().resolve(strict=False) for p in paths]
|
||||
if policy == AUTO_APPROVE_SESSION:
|
||||
return True
|
||||
if policy == AUTO_APPROVE_WORKSPACE:
|
||||
# tempfile.gettempdir() is the real temp root on every platform
|
||||
# (``/private/tmp`` on macOS since resolve() follows the symlink).
|
||||
return path.is_relative_to(Path(tempfile.gettempdir()).resolve(strict=False)) or (
|
||||
bool(cwd) and path.is_relative_to(Path(cwd).expanduser().resolve(strict=False)))
|
||||
tmp = Path(tempfile.gettempdir()).resolve(strict=False)
|
||||
ws = Path(cwd).expanduser().resolve(strict=False) if cwd else None
|
||||
return all(
|
||||
path.is_relative_to(tmp) or (ws is not None and path.is_relative_to(ws))
|
||||
for path in resolved)
|
||||
return False
|
||||
|
||||
|
||||
|
||||
@@ -9,6 +9,7 @@ from pathlib import Path
|
||||
from acp_adapter.edit_approval import (
|
||||
EditProposal,
|
||||
build_acp_edit_tool_call,
|
||||
build_edit_proposal,
|
||||
set_edit_approval_requester,
|
||||
should_auto_approve_edit,
|
||||
)
|
||||
@@ -121,3 +122,82 @@ def test_workspace_auto_approval_allows_workspace_and_tmp_but_not_sensitive(tmp_
|
||||
"session",
|
||||
str(tmp_path),
|
||||
)
|
||||
|
||||
|
||||
def test_multifile_v4a_patch_does_not_bypass_sensitive_or_workspace_checks(tmp_path):
|
||||
"""Each V4A header path must pass the checks, not the comma-joined display string."""
|
||||
patch = (
|
||||
"*** Update File: .env\n@@\n+SECRET=x\n"
|
||||
"*** Update File: src/ok.py\n@@\n+ok\n"
|
||||
)
|
||||
proposal = build_edit_proposal("patch", {"mode": "patch", "patch": patch})
|
||||
assert proposal is not None
|
||||
|
||||
# A sensitive file anywhere in the patch still prompts under both policies.
|
||||
assert not should_auto_approve_edit(proposal, "session", str(tmp_path))
|
||||
assert not should_auto_approve_edit(proposal, "workspace_session", str(tmp_path))
|
||||
|
||||
|
||||
def test_multifile_v4a_patch_outside_workspace_path_prompts(tmp_path):
|
||||
# Escape target must be outside BOTH allowed roots (tempdir + session cwd).
|
||||
escape = str(Path.home() / ".hermes-acp-escape-test.txt")
|
||||
patch = (
|
||||
f"*** Update File: {tmp_path}/src/ok.py\n@@\n+ok\n"
|
||||
f"*** Update File: {escape}\n@@\n+evil\n"
|
||||
)
|
||||
proposal = build_edit_proposal("patch", {"mode": "patch", "patch": patch})
|
||||
assert proposal is not None
|
||||
|
||||
assert not should_auto_approve_edit(proposal, "workspace_session", str(tmp_path))
|
||||
# Session policy still approves non-sensitive absolute paths by design.
|
||||
assert should_auto_approve_edit(proposal, "session", str(tmp_path))
|
||||
|
||||
|
||||
def test_multifile_v4a_patch_all_safe_paths_auto_approves(tmp_path):
|
||||
patch = (
|
||||
f"*** Update File: {tmp_path}/a.py\n@@\n+a\n"
|
||||
f"*** Update File: {tmp_path}/b.py\n@@\n+b\n"
|
||||
)
|
||||
proposal = build_edit_proposal("patch", {"mode": "patch", "patch": patch})
|
||||
assert proposal is not None
|
||||
|
||||
assert should_auto_approve_edit(proposal, "workspace_session", str(tmp_path))
|
||||
assert should_auto_approve_edit(proposal, "session", str(tmp_path))
|
||||
|
||||
|
||||
def test_multifile_v4a_env_write_reaches_permission_prompt_e2e(tmp_path):
|
||||
"""Pre-fix, ``session`` policy auto-approved the ``.env`` hidden in the join."""
|
||||
env_target = tmp_path / ".env"
|
||||
ok_target = tmp_path / "ok.py"
|
||||
patch = (
|
||||
f"*** Update File: {env_target}\n@@\n+SECRET=x\n"
|
||||
f"*** Update File: {ok_target}\n@@\n+ok\n"
|
||||
)
|
||||
|
||||
prompted = []
|
||||
|
||||
def requester(proposal):
|
||||
if should_auto_approve_edit(proposal, "session", str(tmp_path)):
|
||||
return True
|
||||
prompted.append(proposal.path)
|
||||
return False
|
||||
|
||||
set_edit_approval_requester(requester)
|
||||
result = json.loads(
|
||||
handle_function_call("patch", {"mode": "patch", "patch": patch}, task_id="acp-v4a-e2e")
|
||||
)
|
||||
|
||||
assert prompted, "multi-file .env patch must reach the prompt, not auto-approve"
|
||||
assert "denied" in result["error"].lower()
|
||||
assert not env_target.exists()
|
||||
assert not ok_target.exists()
|
||||
|
||||
|
||||
def test_v4a_move_file_checks_both_endpoints(tmp_path):
|
||||
patch = (
|
||||
"*** Move File: src/a.py -> /tmp/../outside-dir/moved.py\n"
|
||||
"@@\n@@\n"
|
||||
)
|
||||
proposal = build_edit_proposal("patch", {"mode": "patch", "patch": patch})
|
||||
if proposal is not None:
|
||||
assert not should_auto_approve_edit(proposal, "workspace_session", str(tmp_path))
|
||||
|
||||
Reference in New Issue
Block a user