From 4a71ce3bde163059879afd67e99b9d46b5b693a9 Mon Sep 17 00:00:00 2001 From: beardthelion Date: Fri, 18 Sep 2026 16:49:04 +0000 Subject: [PATCH] fix(acp): evaluate V4A multi-file patch paths individually for auto-approve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- acp_adapter/edit_approval.py | 19 ++++-- tests/acp_adapter/test_edit_approval.py | 80 +++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/acp_adapter/edit_approval.py b/acp_adapter/edit_approval.py index ed05b7c671..8c091fc1d3 100644 --- a/acp_adapter/edit_approval.py +++ b/acp_adapter/edit_approval.py @@ -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 diff --git a/tests/acp_adapter/test_edit_approval.py b/tests/acp_adapter/test_edit_approval.py index 8cdbf82fd2..3dfb49b4a0 100644 --- a/tests/acp_adapter/test_edit_approval.py +++ b/tests/acp_adapter/test_edit_approval.py @@ -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))