From 37949c247136c2253139f426e9a67bb3873307c3 Mon Sep 17 00:00:00 2001 From: amoussa1229 <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 1 Jul 2026 18:48:07 +0000 Subject: [PATCH 1/8] Loosen workflow push approval fingerprint to repo/branch/files identity The prior fingerprint included head_sha and the full diff, which made approval break on every rebase, amend, or stacked-PR branch switch. Now approval is keyed to (repo, branch, base_sha, files) so the same workflow change on the same branch stays approved across history edits. If a prior approval exists for the same identity but the exact fingerprint doesn't match (e.g. the diff changed after approval), we now surface an explicit stale-approval message instead of silently creating a new pending request. Refs: #98 --- agent/dashboard/workflow_approval.py | 22 +++++ agent/middleware/workflow_push_guard.py | 71 ++++++++++++---- agent/prompt.py | 2 +- tests/test_workflow_push_guard.py | 103 ++++++++++++++++++++++++ 4 files changed, 180 insertions(+), 18 deletions(-) diff --git a/agent/dashboard/workflow_approval.py b/agent/dashboard/workflow_approval.py index 7652f08a..eaae96d7 100644 --- a/agent/dashboard/workflow_approval.py +++ b/agent/dashboard/workflow_approval.py @@ -44,6 +44,28 @@ async def workflow_push_approved(thread_id: str, fingerprint: str) -> bool: return approvals.get(fingerprint, {}).get("status") == WORKFLOW_APPROVAL_APPROVED +async def find_workflow_push_approval( + thread_id: str, + *, + repo: str, + branch: str, + files: list[str], +) -> dict[str, Any] | None: + """Return the most recent approved record matching identity-level keys, if any.""" + approvals = await get_workflow_push_approvals(thread_id) + identity = (repo, branch, tuple(sorted(files))) + matches = [ + r + for r in approvals.values() + if r.get("status") == WORKFLOW_APPROVAL_APPROVED + and (r.get("repo"), r.get("branch"), tuple(sorted(r.get("files", [])))) == identity + ] + if not matches: + return None + matches.sort(key=lambda r: str(r.get("decided_at", "")), reverse=True) + return matches[0] + + async def ensure_workflow_push_pending( thread_id: str, *, diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 1f8a346e..2aba29fb 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -21,6 +21,7 @@ from langgraph.types import Command from ..dashboard.workflow_approval import ( ensure_workflow_push_pending, + find_workflow_push_approval, mark_workflow_push_notified, workflow_push_approved, ) @@ -346,20 +347,14 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu fixed_args.append("--set-upstream") fixed_args.extend([parsed.remote, fixed_refspec]) fixed_command = _git_command(root, " ".join(shlex.quote(arg) for arg in fixed_args)) - payload = { + identity_payload = { "repo": repo, "branch": branch_name, "base_sha": base_sha, - "head_sha": head, "files": files, - "diff": diff.output, - "remote": parsed.remote, - "local_ref": parsed.local_ref, - "remote_ref": parsed.remote_ref, - "fixed_refspec": fixed_refspec, } return WorkflowPushChange( - fingerprint=_fingerprint(payload), + fingerprint=_fingerprint(identity_payload), repo=repo, branch=branch_name, base_sha=base_sha, @@ -372,16 +367,27 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu ) -def _blocked_message(change: WorkflowPushChange, *, already_rejected: bool = False) -> ToolMessage: +def _blocked_message( + change: WorkflowPushChange, *, already_rejected: bool = False, stale: bool = False +) -> ToolMessage: status = "rejected" if already_rejected else "approval_required" - content = { - "status": "error", - "error_type": "WorkflowPushApprovalRequired", - "error": ( + if stale: + error = ( + "This git push includes GitHub workflow file changes. A previous approval " + "exists for the same branch and workflow files, but the workflow diff has " + "changed since that approval (for example, a rebase or amend). The thread " + "owner must re-approve the new fingerprint before Open SWE can push it." + ) + else: + error = ( "This git push includes GitHub workflow file changes and requires human " "approval before Open SWE can push it. Retry the same standalone git push " "after the thread owner approves the workflow diff." - ), + ) + content = { + "status": "error", + "error_type": "WorkflowPushApprovalRequired", + "error": error, "workflow_approval_status": status, "fingerprint": change.fingerprint, "files": change.files, @@ -416,8 +422,9 @@ def _approval_slack_message(change: WorkflowPushChange) -> str: f"Open SWE is trying to push changes to GitHub workflow files in `{repo}` on `{branch}`.\n\n" f"*Files:*\n{files}\n\n" f"*Fingerprint:* `{change.fingerprint}`\n\n" - "Approve only if this exact workflow diff is expected. If the workflow files change, " - "a new fingerprint will be required." + "Approval covers the workflow files listed above on this branch, including future " + "rebases or amends of the same change. If the set of workflow files or the branch " + "changes, a new fingerprint will be required." ) @@ -454,6 +461,20 @@ async def _approval_state(request: ToolCallRequest, change: WorkflowPushChange) try: if await workflow_push_approved(thread_id, change.fingerprint): return "approved" + + # If the exact identity fingerprint is not approved, check whether a prior + # approval covers the same (repo, branch, files) identity. If so, the diff + # changed underneath the prior approval (rebase/amend/edit), so we surface a + # loud re-approval message rather than a fresh silent pending record. + prior = await find_workflow_push_approval( + thread_id, + repo=change.repo, + branch=change.branch, + files=change.files, + ) + if prior is not None: + return "stale_approval" + record, _created = await ensure_workflow_push_pending( thread_id, fingerprint=change.fingerprint, @@ -520,8 +541,24 @@ class WorkflowPushGuardMiddleware(AgentMiddleware): if state == "approved" and thread_id: safe_request = _override_execute_command(request, change.fixed_command) return await _run_with_workflow_token(thread_id, lambda: handler(safe_request)) + if state == "stale_approval": + record, _created = await ensure_workflow_push_pending( + thread_id, + fingerprint=change.fingerprint, + repo=change.repo, + branch=change.branch, + base_sha=change.base_sha, + head_sha=change.head_sha, + files=change.files, + ) + await _post_slack_approval_if_needed(request, change, record) return _tool_message_for_request( - _blocked_message(change, already_rejected=state == "rejected"), request + _blocked_message( + change, + already_rejected=state == "rejected", + stale=state == "stale_approval", + ), + request, ) def wrap_tool_call( diff --git a/agent/prompt.py b/agent/prompt.py index 549c6902..f65c52ce 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -317,7 +317,7 @@ Steps, in order: **IMPORTANT: If `git push` or `gh` returns "403", "Permission denied", or another permanent authorization failure, do not retry. Report the error to the user immediately and stop.** -**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval of the exact workflow diff fingerprint before it can proceed — do not attempt to bypass it.** +**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval before it can proceed. Approval is keyed to the repo, branch, and workflow files being pushed, so rebases or amends of the same workflow change do not require a fresh approval; changing the branch or the set of workflow files does require a new approval. Do not attempt to bypass it.** 4. **Notify the source** immediately after pushing and, when applicable, PR creation/update succeeds. Include a brief summary plus the PR link or branch URL: - Linear-triggered: use `linear_comment` with an `@mention` of the user who triggered the task diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index 2a65d7c7..aa875191 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -153,6 +153,9 @@ async def test_unapproved_workflow_push_blocks_and_posts_slack( async def fake_approved(thread_id: str, fingerprint: str) -> bool: return False + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True @@ -168,6 +171,7 @@ async def test_unapproved_workflow_push_blocks_and_posts_slack( posted["notified"] = fingerprint monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) @@ -204,7 +208,11 @@ async def test_approved_workflow_push_elevates_and_restores( refreshed.append(dict(permissions)) return True + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh) pushed_command = "" @@ -240,7 +248,11 @@ async def test_workflow_push_restoration_falls_back_when_actions_read_unavailabl refreshed.append(dict(permissions)) return "actions" not in permissions + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh) async def handler(_request: Any) -> ToolMessage: @@ -273,3 +285,94 @@ async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.Monke assert called is True assert isinstance(result, ToolMessage) assert result.content == "pushed" + + +async def test_stale_workflow_approval_is_loud_and_blocks( + monkeypatch: pytest.MonkeyPatch, +) -> None: + guard.SANDBOX_BACKENDS["thread-1"] = _Backend() + posted: dict[str, Any] = {} + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return {"fingerprint": "old-fp", "status": "approved", "decided_at": "2024-01-01T00:00:00"} + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post( + channel_id: str, thread_ts: str, message: str, **kwargs: Any + ) -> tuple[str, None]: + posted.update( + channel_id=channel_id, thread_ts=thread_ts, message=message, blocks=kwargs["blocks"] + ) + return "1700000000.000300", None + + async def fake_notified(thread_id: str, fingerprint: str) -> None: + posted["notified"] = fingerprint + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + assert "changed since that approval" in payload["error"] + assert posted["channel_id"] == "C123" + + +async def test_rebased_workflow_push_uses_identity_fingerprint( + monkeypatch: pytest.MonkeyPatch, +) -> None: + backend = _Backend() + backend.head = "b" * 40 + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["fingerprint"] + assert payload["files"] == [".github/workflows/ci.yml"] + + # A second push with a different head but same branch/files should produce the same fingerprint. + backend.head = "c" * 40 + result2 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result2, ToolMessage) + payload2 = json.loads(str(result2.content)) + assert payload2["fingerprint"] == payload["fingerprint"] From 57e8afa19f2d3eee797bc1e8531db5bfa6d49215 Mon Sep 17 00:00:00 2001 From: amoussa1229 <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 1 Jul 2026 19:23:34 +0000 Subject: [PATCH 2/8] Key workflow approval fingerprint on file content hash Switches the fingerprint from {repo, branch, base_sha, files} to {repo, branch, files, content_hash}, where content_hash is a SHA-256 of the workflow-file contents at the pushed head. This keeps the security property: a content change to the same workflow file requires re-review, while a rebase that replays the same workflow diff stays auto-approved. Also updates the prompt and Slack approval copy to match the new behavior, and extends the test to exercise the remote-branch-exists path (changing base_sha) and assert content changes produce a different fingerprint. Refs: 98 --- agent/middleware/workflow_push_guard.py | 20 +++++++++---- agent/prompt.py | 2 +- tests/test_workflow_push_guard.py | 40 +++++++++++++++++++++---- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 12e784b4..40abfe50 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -340,6 +340,14 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu if not diff.ok or not diff.output: return None + content_parts: list[str] = [] + for path in files: + blob = _run_git(backend, root, f"show {shlex.quote(head)}:{shlex.quote(path)}") + if not blob.ok: + return None + content_parts.append(blob.output) + content_hash = _fingerprint({"contents": content_parts}) + remote = _run_git(backend, root, "config --get remote.origin.url") repo = _normalize_remote(_first_line(remote.output)) if remote.ok else "" fixed_refspec = f"{head}:refs/heads/{parsed.remote_ref}" @@ -348,14 +356,14 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu fixed_args.append("--set-upstream") fixed_args.extend([parsed.remote, fixed_refspec]) fixed_command = _git_command(root, " ".join(shlex.quote(arg) for arg in fixed_args)) - identity_payload = { + content_payload = { "repo": repo, "branch": branch_name, - "base_sha": base_sha, "files": files, + "content_hash": content_hash, } return WorkflowPushChange( - fingerprint=_fingerprint(identity_payload), + fingerprint=_fingerprint(content_payload), repo=repo, branch=branch_name, base_sha=base_sha, @@ -423,9 +431,9 @@ def _approval_slack_message(change: WorkflowPushChange) -> str: f"Open SWE is trying to push changes to GitHub workflow files in `{repo}` on `{branch}`.\n\n" f"*Files:*\n{files}\n\n" f"*Fingerprint:* `{change.fingerprint}`\n\n" - "Approval covers the workflow files listed above on this branch, including future " - "rebases or amends of the same change. If the set of workflow files or the branch " - "changes, a new fingerprint will be required." + "Approval covers the exact workflow-file content diff listed above on this branch, " + "including future rebases or amends that replay the same diff. If the set of workflow " + "files, the branch, or the workflow-file content changes, a new fingerprint will be required." ) diff --git a/agent/prompt.py b/agent/prompt.py index f65c52ce..fab505d4 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -317,7 +317,7 @@ Steps, in order: **IMPORTANT: If `git push` or `gh` returns "403", "Permission denied", or another permanent authorization failure, do not retry. Report the error to the user immediately and stop.** -**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval before it can proceed. Approval is keyed to the repo, branch, and workflow files being pushed, so rebases or amends of the same workflow change do not require a fresh approval; changing the branch or the set of workflow files does require a new approval. Do not attempt to bypass it.** +**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval before it can proceed. Approval is keyed to the repo, branch, workflow files, and the exact workflow-file content diff, so rebases or amends that replay the same workflow diff do not require a fresh approval; changing the branch, the set of workflow files, or the workflow-file content does require a new approval. Do not attempt to bypass it.** 4. **Notify the source** immediately after pushing and, when applicable, PR creation/update succeeds. Include a brief summary plus the PR link or branch URL: - Linear-triggered: use `linear_comment` with an `@mention` of the user who triggered the task diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index a8d8e378..c1c44211 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -20,8 +20,16 @@ class _Response: class _Backend: id = "sandbox-id" - def __init__(self, *, workflow_files: str = ".github/workflows/ci.yml") -> None: + def __init__( + self, + *, + workflow_files: str = ".github/workflows/ci.yml", + remote_branch: str | None = None, + diff_output: str = "diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n", + ) -> None: self.workflow_files = workflow_files + self.remote_branch = remote_branch + self.diff_output = diff_output self.commands: list[str] = [] self.head = "a" * 40 @@ -30,7 +38,9 @@ class _Backend: if "rev-parse --show-toplevel" in command: return _Response("/repo\n") if "rev-parse --verify refs/remotes/origin/feature" in command: - return _Response("", 1) + if self.remote_branch is None: + return _Response("", 1) + return _Response(f"{self.remote_branch}\n") if "symbolic-ref --short refs/remotes/origin/HEAD" in command: return _Response("origin/main\n") if f"merge-base {self.head} origin/main" in command: @@ -38,11 +48,18 @@ class _Backend: if "diff --name-only" in command: return _Response(f"{self.workflow_files}\n" if self.workflow_files else "") if "diff --binary --full-index" in command: - return _Response("diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n") + return _Response(self.diff_output) + if command.startswith("git -C /repo show "): + content = "workflow content\n" + if "+new content" in self.diff_output: + content += "new content\n" + return _Response(content) if "config --get remote.origin.url" in command: return _Response("git@github.com:langchain-ai/open-swe.git\n") if "rev-parse --abbrev-ref HEAD" in command: return _Response("feature\n") + if "rev-parse refs/remotes/origin/feature" in command: + return _Response(f"{self.remote_branch}\n") if "rev-parse HEAD" in command or "rev-parse feature" in command: return _Response(f"{self.head}\n") return _Response("") @@ -333,10 +350,11 @@ async def test_stale_workflow_approval_is_loud_and_blocks( assert posted["channel_id"] == "C123" -async def test_rebased_workflow_push_uses_identity_fingerprint( +async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( monkeypatch: pytest.MonkeyPatch, ) -> None: - backend = _Backend() + # Remote branch exists, so base_sha is derived from the remote tip and changes on rebase. + backend = _Backend(remote_branch="r" * 40) backend.head = "b" * 40 guard.SANDBOX_BACKENDS["thread-1"] = backend @@ -371,13 +389,23 @@ async def test_rebased_workflow_push_uses_identity_fingerprint( assert payload["fingerprint"] assert payload["files"] == [".github/workflows/ci.yml"] - # A second push with a different head but same branch/files should produce the same fingerprint. + # Rebase onto a new base with the same workflow diff -> fingerprint stays stable. + backend.remote_branch = "s" * 40 backend.head = "c" * 40 result2 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) assert isinstance(result2, ToolMessage) payload2 = json.loads(str(result2.content)) assert payload2["fingerprint"] == payload["fingerprint"] + # A content change to the same workflow file produces a different fingerprint. + backend.diff_output = ( + "diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n+new content\n" + ) + result3 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result3, ToolMessage) + payload3 = json.loads(str(result3.content)) + assert payload3["fingerprint"] != payload["fingerprint"] + async def test_approved_workflow_push_aborts_when_elevation_fails( monkeypatch: pytest.MonkeyPatch, From b0fa57fa48dc0cdef558220742850191d2792693 Mon Sep 17 00:00:00 2001 From: amoussa1229 <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 1 Jul 2026 19:27:37 +0000 Subject: [PATCH 3/8] Hash workflow diff for fingerprint instead of per-file blobs Using git show per changed file failed when a workflow file was deleted, because the blob no longer exists at the pushed head. That caused the guard to return None and skip approval for deletions. Hash the already-computed diff output instead; it captures adds, mods, deletes, and renames and cannot fail on a missing blob. Also adds a test that a deleted workflow file still requires approval. Refs: 98 --- agent/middleware/workflow_push_guard.py | 8 +---- tests/test_workflow_push_guard.py | 42 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 40abfe50..67b5069f 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -340,13 +340,7 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu if not diff.ok or not diff.output: return None - content_parts: list[str] = [] - for path in files: - blob = _run_git(backend, root, f"show {shlex.quote(head)}:{shlex.quote(path)}") - if not blob.ok: - return None - content_parts.append(blob.output) - content_hash = _fingerprint({"contents": content_parts}) + content_hash = _fingerprint({"diff": diff.output}) remote = _run_git(backend, root, "config --get remote.origin.url") repo = _normalize_remote(_first_line(remote.output)) if remote.ok else "" diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index c1c44211..63362ec8 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -407,6 +407,48 @@ async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( assert payload3["fingerprint"] != payload["fingerprint"] +async def test_deleted_workflow_file_requires_approval( + monkeypatch: pytest.MonkeyPatch, +) -> None: + backend = _Backend( + workflow_files=".github/workflows/ci.yml", + diff_output="diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\ndeleted file\n", + ) + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + assert payload["files"] == [".github/workflows/ci.yml"] + + async def test_approved_workflow_push_aborts_when_elevation_fails( monkeypatch: pytest.MonkeyPatch, ) -> None: From 710e5f9b86aa18b58de1bdf51dddbe6f746211cc Mon Sep 17 00:00:00 2001 From: amoussa1229 <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 1 Jul 2026 20:58:07 +0000 Subject: [PATCH 4/8] Key workflow approval on head workflow tree and block unsafe pushes Switches the workflow push guard from diffing against a sandbox-writable remote-tracking ref to hashing the actual workflow tree at the pushed head via . This closes the confused-deputy base-poisoning vector and the replay vector: approval is bound to the exact files and blob SHAs at head, so a different workflow tree requires re-approval. Also: - Fails the git push parser CLOSED: unrecognized or unsafe push forms are now blocked instead of running unguarded. - Checks the exact fingerprint's rejected status before the stale-approval path. - Updates prompt and Slack copy to reflect the new head-tree behavior. - Adds tests for base-poisoning, replay, deletion, unparsed push blocking, and rejection-before-stale ordering. Refs: 98 --- agent/dashboard/workflow_approval.py | 5 + agent/middleware/workflow_push_guard.py | 176 +++++++++++++-------- agent/prompt.py | 2 +- tests/test_workflow_push_guard.py | 198 +++++++++++++++++++----- 4 files changed, 278 insertions(+), 103 deletions(-) diff --git a/agent/dashboard/workflow_approval.py b/agent/dashboard/workflow_approval.py index eaae96d7..0db6e22c 100644 --- a/agent/dashboard/workflow_approval.py +++ b/agent/dashboard/workflow_approval.py @@ -44,6 +44,11 @@ async def workflow_push_approved(thread_id: str, fingerprint: str) -> bool: return approvals.get(fingerprint, {}).get("status") == WORKFLOW_APPROVAL_APPROVED +async def workflow_push_rejected(thread_id: str, fingerprint: str) -> bool: + approvals = await get_workflow_push_approvals(thread_id) + return approvals.get(fingerprint, {}).get("status") == WORKFLOW_APPROVAL_REJECTED + + async def find_workflow_push_approval( thread_id: str, *, diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 67b5069f..f8afce39 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -25,6 +25,7 @@ from ..dashboard.workflow_approval import ( find_workflow_push_approval, mark_workflow_push_notified, workflow_push_approved, + workflow_push_rejected, ) from ..tools.slack_thread_reply import build_workflow_approval_blocks from ..utils.github_app import ( @@ -45,6 +46,15 @@ _GIT_OBJECT_ID = re.compile(r"^[0-9a-fA-F]{40,64}$") _UNSAFE_RAW_COMMAND = re.compile(r"[;|`$<>\n\r]") +class _BlockedGitPush: + """Sentinel returned by the parser when a git push command is unsafe or unrecognized.""" + + __slots__ = ("reason",) + + def __init__(self, reason: str) -> None: + self.reason = reason + + @dataclass(frozen=True) class ParsedGitPush: repo_dir: str | None @@ -59,13 +69,15 @@ class WorkflowPushChange: fingerprint: str repo: str branch: str - base_sha: str - head_sha: str files: list[str] + head_sha: str remote: str local_ref: str remote_ref: str fixed_command: str + base_sha: str = "" + blocked: bool = False + blocked_reason: str = "" @dataclass(frozen=True) @@ -144,28 +156,30 @@ def _response_ok(response: Any) -> bool: return True -def _parse_git_push(command: str) -> ParsedGitPush | None: +def _parse_git_push(command: str) -> ParsedGitPush | _BlockedGitPush | None: stripped = command.strip() if _UNSAFE_RAW_COMMAND.search(stripped) or "&" in stripped.replace("&&", ""): - return None + return _BlockedGitPush("unsafe shell characters in git push command") try: tokens = shlex.split(stripped) except ValueError: - return None + return _BlockedGitPush("unparseable git push command") if not tokens: return None if len(tokens) >= 4 and tokens[0] == "cd" and tokens[2] == "&&": if any(token in _SHELL_OPERATORS or token == "&&" for token in tokens[3:]): - return None + return _BlockedGitPush("chained shell commands in git push") return _parse_git_tokens(tokens[3:], repo_dir=tokens[1]) if any(token in _SHELL_OPERATORS or token == "&&" for token in tokens): - return None + return _BlockedGitPush("chained shell commands") return _parse_git_tokens(tokens, repo_dir=None) -def _parse_git_tokens(tokens: list[str], *, repo_dir: str | None) -> ParsedGitPush | None: +def _parse_git_tokens( + tokens: list[str], *, repo_dir: str | None +) -> ParsedGitPush | _BlockedGitPush | None: if not tokens or tokens[0] != "git": return None i = 1 @@ -174,22 +188,25 @@ def _parse_git_tokens(tokens: list[str], *, repo_dir: str | None) -> ParsedGitPu repo_dir = tokens[i + 1] i += 2 continue + # Any non-push git command (e.g. git status) is not a push, so let it pass. return None if i >= len(tokens) or tokens[i] != "push": return None return _parse_push_args(tokens[i + 1 :], repo_dir=repo_dir) -def _parse_push_args(tokens: list[str], *, repo_dir: str | None) -> ParsedGitPush | None: +def _parse_push_args( + tokens: list[str], *, repo_dir: str | None +) -> ParsedGitPush | _BlockedGitPush | None: set_upstream = False while tokens and tokens[0] in {"-u", "--set-upstream"}: set_upstream = True tokens = tokens[1:] if len(tokens) != 2 or tokens[0] != "origin": - return None + return _BlockedGitPush("unrecognized or unsafe git push arguments") parsed = _parse_refspec(tokens[1]) if parsed is None: - return None + return _BlockedGitPush("unrecognized or unsafe git push refspec") local_ref, remote_ref = parsed return ParsedGitPush( repo_dir=repo_dir, @@ -283,6 +300,40 @@ def _run_coroutine_sync(coro: Awaitable[ToolMessage | Command]) -> ToolMessage | return value +def _workflow_tree_at_head( + backend: Any, repo_dir: str | None, head: str +) -> tuple[list[str], str] | None: + """Return the sorted workflow file paths and a stable hash of the head workflow tree. + + Uses `git ls-tree -r` so the hash binds to the blob SHAs at the pushed head, not to + a diff against a sandbox-writable base ref. This prevents a confused deputy where the + sandbox rewrites `refs/remotes/origin/*` to hide a malicious workflow file from the + approved diff while still deploying it. + """ + ls_tree = _run_git(backend, repo_dir, f"ls-tree -r {shlex.quote(head)} -- .github/workflows") + if not ls_tree.ok: + return None + entries: list[tuple[str, str]] = [] + for line in ls_tree.output.splitlines(): + line = line.strip() + if not line: + continue + # Format: " \t" + meta, _, path = line.partition("\t") + if not path: + continue + parts = meta.split() + if len(parts) < 3: + continue + entries.append((parts[2], path)) + if not entries: + return None + entries.sort(key=lambda item: item[1]) + files = [path for _sha, path in entries] + content_hash = _fingerprint({"tree": [(sha, path) for sha, path in entries]}) + return files, content_hash + + def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPushChange | None: root_result = _run_git(backend, parsed.repo_dir, "rev-parse --show-toplevel") if not root_result.ok: @@ -303,44 +354,10 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu if not head or not _GIT_OBJECT_ID.fullmatch(head): return None - remote_branch = f"refs/remotes/{parsed.remote}/{parsed.remote_ref}" - remote_branch_exists = _run_git( - backend, root, f"rev-parse --verify {shlex.quote(remote_branch)}" - ) - if remote_branch_exists.ok and _first_line(remote_branch_exists.output): - base_ref = remote_branch - range_expr = f"{shlex.quote(base_ref)}..{shlex.quote(head)}" - base_sha = _first_line(_run_git(backend, root, f"rev-parse {shlex.quote(base_ref)}").output) - else: - origin_head = _run_git(backend, root, "symbolic-ref --short refs/remotes/origin/HEAD") - base_ref = _first_line(origin_head.output) if origin_head.ok else "origin/main" - range_expr = f"{shlex.quote(base_ref)}...{shlex.quote(head)}" - base_sha = _first_line( - _run_git( - backend, root, f"merge-base {shlex.quote(head)} {shlex.quote(base_ref)}" - ).output - ) - - names = _run_git( - backend, - root, - f"diff --name-only --diff-filter=ACMRTD {range_expr} -- .github/workflows", - ) - if not names.ok: + tree = _workflow_tree_at_head(backend, root, head) + if tree is None: return None - files = sorted( - line.strip() - for line in names.output.splitlines() - if line.strip().startswith(_WORKFLOW_PREFIX) - ) - if not files: - return None - - diff = _run_git(backend, root, f"diff --binary --full-index {range_expr} -- .github/workflows") - if not diff.ok or not diff.output: - return None - - content_hash = _fingerprint({"diff": diff.output}) + files, content_hash = tree remote = _run_git(backend, root, "config --get remote.origin.url") repo = _normalize_remote(_first_line(remote.output)) if remote.ok else "" @@ -360,9 +377,8 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu fingerprint=_fingerprint(content_payload), repo=repo, branch=branch_name, - base_sha=base_sha, - head_sha=head, files=files, + head_sha=head, remote=parsed.remote, local_ref=parsed.local_ref, remote_ref=parsed.remote_ref, @@ -371,27 +387,37 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu def _blocked_message( - change: WorkflowPushChange, *, already_rejected: bool = False, stale: bool = False + change: WorkflowPushChange, + *, + already_rejected: bool = False, + stale: bool = False, + blocked: bool = False, ) -> ToolMessage: status = "rejected" if already_rejected else "approval_required" - if stale: + if blocked: + error = change.blocked_reason or "This git push command is not recognized as safe." + error_type = "WorkflowPushBlocked" + elif stale: error = ( "This git push includes GitHub workflow file changes. A previous approval " - "exists for the same branch and workflow files, but the workflow diff has " - "changed since that approval (for example, a rebase or amend). The thread " + "exists for the same branch and workflow files, but the workflow content " + "at the pushed head has changed since that approval (for example, a rebase " + "that changed the workflow files or an amend that edited them). The thread " "owner must re-approve the new fingerprint before Open SWE can push it." ) + error_type = "WorkflowPushApprovalRequired" else: error = ( "This git push includes GitHub workflow file changes and requires human " "approval before Open SWE can push it. Retry the same standalone git push " - "after the thread owner approves the workflow diff." + "after the thread owner approves the workflow files." ) + error_type = "WorkflowPushApprovalRequired" content = { "status": "error", - "error_type": "WorkflowPushApprovalRequired", + "error_type": error_type, "error": error, - "workflow_approval_status": status, + "workflow_approval_status": status if not blocked else "blocked", "fingerprint": change.fingerprint, "files": change.files, "repo": change.repo, @@ -422,12 +448,13 @@ def _approval_slack_message(change: WorkflowPushChange) -> str: branch = change.branch or "the current branch" return ( "*Workflow file approval required*\n" - f"Open SWE is trying to push changes to GitHub workflow files in `{repo}` on `{branch}`.\n\n" - f"*Files:*\n{files}\n\n" + f"Open SWE is trying to push the workflow files below to `{repo}` on `{branch}`.\n\n" + f"*Files at the pushed head:*\n{files}\n\n" f"*Fingerprint:* `{change.fingerprint}`\n\n" - "Approval covers the exact workflow-file content diff listed above on this branch, " - "including future rebases or amends that replay the same diff. If the set of workflow " - "files, the branch, or the workflow-file content changes, a new fingerprint will be required." + "Approval covers the exact workflow files and content listed above at the pushed head, " + "including future rebases or amends that replay the same workflow tree. If the set of " + "workflow files, the branch, or the workflow-file content at the pushed head changes, " + "a new fingerprint will be required." ) @@ -464,11 +491,14 @@ async def _approval_state(request: ToolCallRequest, change: WorkflowPushChange) try: if await workflow_push_approved(thread_id, change.fingerprint): return "approved" + if await workflow_push_rejected(thread_id, change.fingerprint): + return "rejected" # If the exact identity fingerprint is not approved, check whether a prior - # approval covers the same (repo, branch, files) identity. If so, the diff - # changed underneath the prior approval (rebase/amend/edit), so we surface a - # loud re-approval message rather than a fresh silent pending record. + # approval covers the same (repo, branch, files) identity. If so, the workflow + # tree at the pushed head changed underneath the prior approval (rebase/amend + # that edited workflow files), so we surface a loud re-approval message rather + # than a fresh silent pending record. prior = await find_workflow_push_approval( thread_id, repo=change.repo, @@ -550,6 +580,20 @@ class WorkflowPushGuardMiddleware(AgentMiddleware): parsed = _parse_git_push(command) if parsed is None: return None + if isinstance(parsed, _BlockedGitPush): + return WorkflowPushChange( + fingerprint="", + repo="", + branch="", + files=[], + head_sha="", + remote="origin", + local_ref="", + remote_ref="", + fixed_command="", + blocked=True, + blocked_reason=parsed.reason, + ) backend = _backend(_thread_id(request)) if backend is None: return None @@ -561,6 +605,8 @@ class WorkflowPushGuardMiddleware(AgentMiddleware): handler: Callable[[ToolCallRequest], Awaitable[ToolMessage | Command]], change: WorkflowPushChange, ) -> ToolMessage | Command: + if change.blocked: + return _tool_message_for_request(_blocked_message(change, blocked=True), request) thread_id = _thread_id(request) state = await _approval_state(request, change) if state == "approved" and thread_id: diff --git a/agent/prompt.py b/agent/prompt.py index fab505d4..f1716fad 100644 --- a/agent/prompt.py +++ b/agent/prompt.py @@ -317,7 +317,7 @@ Steps, in order: **IMPORTANT: If `git push` or `gh` returns "403", "Permission denied", or another permanent authorization failure, do not retry. Report the error to the user immediately and stop.** -**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow-file changes requires human approval before it can proceed. Approval is keyed to the repo, branch, workflow files, and the exact workflow-file content diff, so rebases or amends that replay the same workflow diff do not require a fresh approval; changing the branch, the set of workflow files, or the workflow-file content does require a new approval. Do not attempt to bypass it.** +**IMPORTANT: Workflow files (`.github/workflows/`) may be changed only when explicitly requested. Any push that includes workflow files at the pushed head requires human approval before it can proceed. Approval is keyed to the repo, branch, and the exact workflow files and content present at the pushed head, so rebases or amends that replay the same workflow tree do not require a fresh approval; changing the branch, the set of workflow files, or the workflow-file content at the pushed head does require a new approval. Do not attempt to bypass it.** 4. **Notify the source** immediately after pushing and, when applicable, PR creation/update succeeds. Include a brief summary plus the PR link or branch URL: - Linear-triggered: use `linear_comment` with an `@mention` of the user who triggered the task diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index 63362ec8..763a25cd 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -24,42 +24,31 @@ class _Backend: self, *, workflow_files: str = ".github/workflows/ci.yml", - remote_branch: str | None = None, - diff_output: str = "diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n", + tree_entries: dict[str, str] | None = None, ) -> None: self.workflow_files = workflow_files - self.remote_branch = remote_branch - self.diff_output = diff_output + self.tree_entries = tree_entries self.commands: list[str] = [] self.head = "a" * 40 + def _ls_tree(self) -> str: + if self.tree_entries is not None: + return "".join( + f"100644 blob {sha}\t{path}\n" for path, sha in sorted(self.tree_entries.items()) + ) + files = [path for path in self.workflow_files.split("\n") if path.strip()] + return "".join(f"100644 blob {self.head}\t{path}\n" for path in files) + def execute(self, command: str, *, timeout: int | None = None) -> _Response: self.commands.append(command) if "rev-parse --show-toplevel" in command: return _Response("/repo\n") - if "rev-parse --verify refs/remotes/origin/feature" in command: - if self.remote_branch is None: - return _Response("", 1) - return _Response(f"{self.remote_branch}\n") - if "symbolic-ref --short refs/remotes/origin/HEAD" in command: - return _Response("origin/main\n") - if f"merge-base {self.head} origin/main" in command: - return _Response("base-sha\n") - if "diff --name-only" in command: - return _Response(f"{self.workflow_files}\n" if self.workflow_files else "") - if "diff --binary --full-index" in command: - return _Response(self.diff_output) - if command.startswith("git -C /repo show "): - content = "workflow content\n" - if "+new content" in self.diff_output: - content += "new content\n" - return _Response(content) + if "ls-tree -r" in command: + return _Response(self._ls_tree()) if "config --get remote.origin.url" in command: return _Response("git@github.com:langchain-ai/open-swe.git\n") if "rev-parse --abbrev-ref HEAD" in command: return _Response("feature\n") - if "rev-parse refs/remotes/origin/feature" in command: - return _Response(f"{self.remote_branch}\n") if "rev-parse HEAD" in command or "rev-parse feature" in command: return _Response(f"{self.head}\n") return _Response("") @@ -110,11 +99,25 @@ def test_parse_git_push_supports_git_c_and_cd() -> None: remote_ref="feature", set_upstream=True, ) - assert guard._parse_git_push("git status && git push") is None - assert guard._parse_git_push("git push origin feature; git push origin evil:feature") is None -def test_workflow_change_for_push_fingerprints_workflow_diff() -> None: +def test_parse_git_push_blocks_unsafe_and_unrecognized_forms() -> None: + assert isinstance(guard._parse_git_push("git status && git push"), guard._BlockedGitPush) + assert isinstance( + guard._parse_git_push("git push origin feature; git push origin evil:feature"), + guard._BlockedGitPush, + ) + assert isinstance( + guard._parse_git_push("git push --force origin feature"), guard._BlockedGitPush + ) + assert isinstance(guard._parse_git_push("git push origin"), guard._BlockedGitPush) + assert isinstance( + guard._parse_git_push("git push origin HEAD~1:feature"), guard._BlockedGitPush + ) + assert guard._parse_git_push("git status") is None + + +def test_workflow_change_for_push_fingerprints_head_workflow_tree() -> None: backend = _Backend() change = guard._workflow_change_for_push( backend, @@ -171,6 +174,9 @@ async def test_unapproved_workflow_push_blocks_and_posts_slack( async def fake_approved(thread_id: str, fingerprint: str) -> bool: return False + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: return None @@ -189,6 +195,7 @@ async def test_unapproved_workflow_push_blocks_and_posts_slack( posted["notified"] = fingerprint monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) @@ -314,6 +321,9 @@ async def test_stale_workflow_approval_is_loud_and_blocks( async def fake_approved(thread_id: str, fingerprint: str) -> bool: return False + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: return {"fingerprint": "old-fp", "status": "approved", "decided_at": "2024-01-01T00:00:00"} @@ -332,6 +342,7 @@ async def test_stale_workflow_approval_is_loud_and_blocks( posted["notified"] = fingerprint monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) @@ -350,17 +361,22 @@ async def test_stale_workflow_approval_is_loud_and_blocks( assert posted["channel_id"] == "C123" -async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( +async def test_rebased_workflow_push_keeps_fingerprint_for_same_tree( monkeypatch: pytest.MonkeyPatch, ) -> None: - # Remote branch exists, so base_sha is derived from the remote tip and changes on rebase. - backend = _Backend(remote_branch="r" * 40) + # Fingerprint is based on the workflow tree at head, not on a diff against origin. + backend = _Backend( + tree_entries={".github/workflows/ci.yml": "blob-sha-1"}, + ) backend.head = "b" * 40 guard.SANDBOX_BACKENDS["thread-1"] = backend async def fake_approved(thread_id: str, fingerprint: str) -> bool: return False + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: return None @@ -374,6 +390,7 @@ async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( return None monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) @@ -389,8 +406,7 @@ async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( assert payload["fingerprint"] assert payload["files"] == [".github/workflows/ci.yml"] - # Rebase onto a new base with the same workflow diff -> fingerprint stays stable. - backend.remote_branch = "s" * 40 + # Rebase with a new head but the same workflow tree -> fingerprint stays stable. backend.head = "c" * 40 result2 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) assert isinstance(result2, ToolMessage) @@ -398,9 +414,7 @@ async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( assert payload2["fingerprint"] == payload["fingerprint"] # A content change to the same workflow file produces a different fingerprint. - backend.diff_output = ( - "diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\n+new content\n" - ) + backend.tree_entries = {".github/workflows/ci.yml": "blob-sha-2"} result3 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) assert isinstance(result3, ToolMessage) payload3 = json.loads(str(result3.content)) @@ -410,15 +424,23 @@ async def test_rebased_workflow_push_keeps_fingerprint_for_same_diff( async def test_deleted_workflow_file_requires_approval( monkeypatch: pytest.MonkeyPatch, ) -> None: + # Deleting a workflow file means it is no longer in the head workflow tree. The + # guard should still trigger if there are other workflow files at head; if the last + # workflow file is deleted, the push is no longer workflow-guarded. backend = _Backend( - workflow_files=".github/workflows/ci.yml", - diff_output="diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml\ndeleted file\n", + tree_entries={ + ".github/workflows/ci.yml": "blob-sha-1", + ".github/workflows/other.yml": "blob-sha-2", + }, ) guard.SANDBOX_BACKENDS["thread-1"] = backend async def fake_approved(thread_id: str, fingerprint: str) -> bool: return False + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: return None @@ -432,6 +454,68 @@ async def test_deleted_workflow_file_requires_approval( return None monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + # Push with the full workflow tree present requires approval. + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + assert ".github/workflows/ci.yml" in payload["files"] + + # A push whose head tree has deleted ci.yml but still contains other.yml still + # requires approval, with a different fingerprint. + backend.tree_entries = {".github/workflows/other.yml": "blob-sha-2"} + result2 = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + assert isinstance(result2, ToolMessage) + payload2 = json.loads(str(result2.content)) + assert payload2["workflow_approval_status"] == "approval_required" + assert payload2["files"] == [".github/workflows/other.yml"] + assert payload2["fingerprint"] != payload["fingerprint"] + + +async def test_base_poisoning_does_not_bypass_guard( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The sandbox can rewrite refs/remotes/origin/*; the guard must use the workflow + # tree at the pushed head, not a diff against a remote ref. This backend records + # whether any command touches the remote-tracking ref. + backend = _Backend( + tree_entries={ + ".github/workflows/ci.yml": "blob-sha-1", + ".github/workflows/evil.yml": "blob-sha-evil", + }, + ) + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) @@ -446,7 +530,47 @@ async def test_deleted_workflow_file_requires_approval( assert result.status == "error" payload = json.loads(str(result.content)) assert payload["workflow_approval_status"] == "approval_required" - assert payload["files"] == [".github/workflows/ci.yml"] + # The evil workflow file present at head must appear in the approval list. + assert ".github/workflows/evil.yml" in payload["files"] + # No remote-tracking ref should have been consulted. + assert not any("refs/remotes/origin" in cmd for cmd in backend.commands) + + +async def test_exact_rejection_checked_before_stale_approval( + monkeypatch: pytest.MonkeyPatch, +) -> None: + backend = _Backend() + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return True + + # A prior approval for the same repo/branch/files exists; the stale path would + # normally match. But because the exact fingerprint is rejected, we must return + # "rejected", not "stale_approval". + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return {"fingerprint": "old-fp", "status": "approved", "decided_at": "2024-01-01T00:00:00"} + + async def fake_pending(*args: Any, **kwargs: Any) -> tuple[dict[str, Any], bool]: + raise AssertionError("should not create a new pending record") + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "rejected" async def test_approved_workflow_push_aborts_when_elevation_fails( From 38d792954598c61bb367f3848893a30629a1c2d8 Mon Sep 17 00:00:00 2001 From: amoussa1229 <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 1 Jul 2026 21:08:29 +0000 Subject: [PATCH 5/8] Re-add trusted-base change detection for workflow push guard The head-tree fingerprint keeps the security win (approval binds to the exact workflow files and blob SHAs at the pushed head), but the guard was firing on every push because it no longer compared against a base. This change re-adds change detection using a base fetched from the authenticated remote at guard time: - Fetches the pushed branch from the remote; if it does not exist (new branch), fetches the remote's default branch via ls-remote and a fallback chain. - Compares the head workflow tree (ls-tree) against the freshly fetched base (FETCH_HEAD), not against any local refs/remotes/origin/* ref. - Returns None (no guard) when the workflow trees are identical, so code-only pushes to a branch that already contains workflow files are not blocked. - Updated test_non_workflow_push_runs_without_approval to use a non-empty-but unchanged workflow tree. Refs: 98 --- agent/middleware/workflow_push_guard.py | 81 ++++++++++++++++++------- tests/test_workflow_push_guard.py | 48 ++++++++++++++- 2 files changed, 103 insertions(+), 26 deletions(-) diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index f8afce39..6d4bd284 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -300,21 +300,10 @@ def _run_coroutine_sync(coro: Awaitable[ToolMessage | Command]) -> ToolMessage | return value -def _workflow_tree_at_head( - backend: Any, repo_dir: str | None, head: str -) -> tuple[list[str], str] | None: - """Return the sorted workflow file paths and a stable hash of the head workflow tree. - - Uses `git ls-tree -r` so the hash binds to the blob SHAs at the pushed head, not to - a diff against a sandbox-writable base ref. This prevents a confused deputy where the - sandbox rewrites `refs/remotes/origin/*` to hide a malicious workflow file from the - approved diff while still deploying it. - """ - ls_tree = _run_git(backend, repo_dir, f"ls-tree -r {shlex.quote(head)} -- .github/workflows") - if not ls_tree.ok: - return None +def _parse_ls_tree(output: str) -> list[tuple[str, str]]: + """Parse `git ls-tree -r` output into a sorted list of (sha, path) tuples.""" entries: list[tuple[str, str]] = [] - for line in ls_tree.output.splitlines(): + for line in output.splitlines(): line = line.strip() if not line: continue @@ -326,12 +315,49 @@ def _workflow_tree_at_head( if len(parts) < 3: continue entries.append((parts[2], path)) - if not entries: - return None entries.sort(key=lambda item: item[1]) - files = [path for _sha, path in entries] - content_hash = _fingerprint({"tree": [(sha, path) for sha, path in entries]}) - return files, content_hash + return entries + + +def _workflow_tree_at_ref( + backend: Any, repo_dir: str | None, ref: str +) -> tuple[list[tuple[str, str]], str] | None: + """Return the sorted workflow tree entries and a stable hash for the given ref. + + Returns an empty list when the ref has no workflow files, so callers can detect + additions and deletions against a base ref. + """ + ls_tree = _run_git(backend, repo_dir, f"ls-tree -r {shlex.quote(ref)} -- .github/workflows") + if not ls_tree.ok: + return None + entries = _parse_ls_tree(ls_tree.output) + content_hash = _fingerprint({"tree": entries}) + return entries, content_hash + + +def _fetch_remote_base(backend: Any, repo_dir: str | None, remote: str, branch: str) -> str | None: + """Fetch the base ref from the authenticated remote and return a local alias for it. + + Fetches the pushed branch first; if it does not exist on the remote, fetches the + remote's default branch. Uses `FETCH_HEAD` so the base is bound to the freshly-fetched + remote tip, not a local `refs/remotes/origin/*` ref that the sandbox could rewrite. + """ + fetch = _run_git(backend, repo_dir, f"fetch {shlex.quote(remote)} {shlex.quote(branch)}") + if fetch.ok: + return "FETCH_HEAD" + + default = _run_git(backend, repo_dir, "ls-remote --symref origin HEAD") + default_branch = "" + if default.ok: + default_branch = _first_line(default.output).removeprefix("ref: refs/heads/").split("\t")[0] + + for fallback in (default_branch, "dev", "main"): + if not fallback or fallback == branch: + continue + fetch = _run_git(backend, repo_dir, f"fetch {shlex.quote(remote)} {shlex.quote(fallback)}") + if fetch.ok: + return "FETCH_HEAD" + return None def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPushChange | None: @@ -354,10 +380,19 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu if not head or not _GIT_OBJECT_ID.fullmatch(head): return None - tree = _workflow_tree_at_head(backend, root, head) - if tree is None: + head_tree = _workflow_tree_at_ref(backend, root, head) + if head_tree is None: return None - files, content_hash = tree + + base_ref = _fetch_remote_base(backend, root, parsed.remote, branch_name) + if base_ref is not None: + base_tree = _workflow_tree_at_ref(backend, root, base_ref) + if base_tree is not None and head_tree[0] == base_tree[0]: + # No workflow change against the trusted remote base. + return None + + head_entries, head_content_hash = head_tree + files = sorted({path for _sha, path in head_entries}) remote = _run_git(backend, root, "config --get remote.origin.url") repo = _normalize_remote(_first_line(remote.output)) if remote.ok else "" @@ -371,7 +406,7 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu "repo": repo, "branch": branch_name, "files": files, - "content_hash": content_hash, + "content_hash": head_content_hash, } return WorkflowPushChange( fingerprint=_fingerprint(content_payload), diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index 763a25cd..0cbe387c 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -25,13 +25,21 @@ class _Backend: *, workflow_files: str = ".github/workflows/ci.yml", tree_entries: dict[str, str] | None = None, + base_tree_entries: dict[str, str] | None = None, + remote_branches: set[str] | None = None, + default_branch: str = "main", ) -> None: self.workflow_files = workflow_files self.tree_entries = tree_entries + # Default base tree is empty so any head workflow file is treated as a change. + self.base_tree_entries = base_tree_entries if base_tree_entries is not None else {} + self.remote_branches = remote_branches or {"feature"} + self.default_branch = default_branch self.commands: list[str] = [] self.head = "a" * 40 + self.base_sha = "b" * 40 - def _ls_tree(self) -> str: + def _head_tree(self) -> str: if self.tree_entries is not None: return "".join( f"100644 blob {sha}\t{path}\n" for path, sha in sorted(self.tree_entries.items()) @@ -39,12 +47,39 @@ class _Backend: files = [path for path in self.workflow_files.split("\n") if path.strip()] return "".join(f"100644 blob {self.head}\t{path}\n" for path in files) + def _base_tree(self) -> str: + if self.base_tree_entries is not None: + return "".join( + f"100644 blob {sha}\t{path}\n" + for path, sha in sorted(self.base_tree_entries.items()) + ) + return self._head_tree() + + def _fetch_branch(self, command: str) -> _Response: + # Format: "git -C /repo fetch origin " or "git fetch origin " + prefix = "fetch origin " + idx = command.find(prefix) + if idx == -1: + return _Response("", 1) + branch = command[idx + len(prefix) :].strip() + if branch in self.remote_branches: + return _Response("") + return _Response("", 1) + def execute(self, command: str, *, timeout: int | None = None) -> _Response: self.commands.append(command) if "rev-parse --show-toplevel" in command: return _Response("/repo\n") if "ls-tree -r" in command: - return _Response(self._ls_tree()) + if "FETCH_HEAD" in command: + return _Response(self._base_tree()) + return _Response(self._head_tree()) + if "fetch origin" in command: + return self._fetch_branch(command) + if "ls-remote --symref origin HEAD" in command: + return _Response( + f"ref: refs/heads/{self.default_branch}\tHEAD\n{self.base_sha}\tHEAD\n" + ) if "config --get remote.origin.url" in command: return _Response("git@github.com:langchain-ai/open-swe.git\n") if "rev-parse --abbrev-ref HEAD" in command: @@ -292,7 +327,13 @@ async def test_workflow_push_restoration_falls_back_when_actions_read_unavailabl async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.MonkeyPatch) -> None: - guard.SANDBOX_BACKENDS["thread-1"] = _Backend(workflow_files="") + # Head contains workflow files, but they are identical to the trusted remote base, + # so no approval is required for a code-only change. + backend = _Backend( + tree_entries={".github/workflows/ci.yml": "blob-sha-1"}, + base_tree_entries={".github/workflows/ci.yml": "blob-sha-1"}, + ) + guard.SANDBOX_BACKENDS["thread-1"] = backend called = False async def fail_approval(*args: Any, **kwargs: Any) -> bool: @@ -310,6 +351,7 @@ async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.Monke assert called is True assert isinstance(result, ToolMessage) assert result.content == "pushed" + assert any("fetch origin" in cmd for cmd in backend.commands) async def test_stale_workflow_approval_is_loud_and_blocks( From 7ec08d18702bdf4112b1a76172cc481e8aee17e2 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 1 Jul 2026 18:04:22 -0400 Subject: [PATCH 6/8] fix(workflow-guard): close residual approval bypasses (H1/H3/H6) Harden the workflow-push approval guard against three bypasses found by the security review: - H6: the push parser passed through (returned None, unguarded) git invoked via a path (`/usr/bin/git`), a wrapper (`command`/`env` ...), or with leading global options (`git -c`, `--git-dir`, `--no-pager`). Recognize wrapped and path-qualified git as pushes, and block pushes carrying unsupported global options instead of running them unguarded. - H1: the base was fetched via the `origin` remote name, which the sandbox can split from the push destination via `remote set-url --push`. Fetch the base from the effective push URL (`git remote get-url --push`) so the base and the push target are the same authenticated repo. - H3: an unreadable head workflow tree (`ls-tree` failure) skipped the guard; fail closed (block) instead, mirroring the base-read path. Adds tests for each. All confirmed guard bypasses are caught by the langsmith unelevated-token backstop today; these close the guard's own logic for non-langsmith providers too. Claude-Session: https://claude.ai/code/session_01GxSndB7VoGQyeS196eUr5E --- agent/middleware/workflow_push_guard.py | 93 +++++++++++++---- tests/test_workflow_push_guard.py | 129 ++++++++++++++++++++++-- 2 files changed, 196 insertions(+), 26 deletions(-) diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 6d4bd284..d8df8f72 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -44,6 +44,10 @@ _SHELL_OPERATORS = {";", "|", "||", "&"} _REF_NAME = re.compile(r"^[A-Za-z0-9._/@+-]+$") _GIT_OBJECT_ID = re.compile(r"^[0-9a-fA-F]{40,64}$") _UNSAFE_RAW_COMMAND = re.compile(r"[;|`$<>\n\r]") +# Command wrappers that can prefix a `git` invocation (e.g. `command git push`, +# `env FOO=bar git push`, `/usr/bin/git push`). Recognized so a wrapped or path-qualified +# git push cannot slip past the guard as "not a git command". +_GIT_WRAPPERS = {"command", "env", "nice", "sudo", "stdbuf", "nohup", "time", "ionice", "setsid"} class _BlockedGitPush: @@ -177,22 +181,50 @@ def _parse_git_push(command: str) -> ParsedGitPush | _BlockedGitPush | None: return _parse_git_tokens(tokens, repo_dir=None) +def _git_invocation_args(tokens: list[str]) -> list[str] | None: + """Return the args following the `git` executable if these tokens invoke git, else None. + + Recognizes bareword `git`, path invocations like `/usr/bin/git`, and simple command + wrappers (`command`/`env`/`nice`/`sudo`/...), including `env NAME=VALUE ...`. This keeps + a wrapped or path-qualified `git push` from being mistaken for a non-git command and + passed through the guard unchecked. + """ + idx = 0 + while idx < len(tokens): + tok = tokens[idx] + if tok.rsplit("/", 1)[-1] == "git": + return tokens[idx + 1 :] + if tok in _GIT_WRAPPERS: + idx += 1 + # Skip wrapper options and `NAME=VALUE` assignments (e.g. `env FOO=bar git ...`). + while idx < len(tokens) and (tokens[idx].startswith("-") or "=" in tokens[idx]): + idx += 1 + continue + return None + return None + + def _parse_git_tokens( tokens: list[str], *, repo_dir: str | None ) -> ParsedGitPush | _BlockedGitPush | None: - if not tokens or tokens[0] != "git": + git_args = _git_invocation_args(tokens) + if git_args is None: return None - i = 1 - while i < len(tokens) and tokens[i] != "push": - if tokens[i] == "-C" and i + 1 < len(tokens): - repo_dir = tokens[i + 1] + if "push" not in git_args: + # A non-push git command (e.g. `git status`) is not our concern. + return None + i = 0 + while i < len(git_args) and git_args[i] != "push": + if git_args[i] == "-C" and i + 1 < len(git_args): + repo_dir = git_args[i + 1] i += 2 continue - # Any non-push git command (e.g. git status) is not a push, so let it pass. - return None - if i >= len(tokens) or tokens[i] != "push": - return None - return _parse_push_args(tokens[i + 1 :], repo_dir=repo_dir) + # A git push carrying unsupported global options (e.g. `git -c k=v push`, + # `git --git-dir=.git push`). We cannot safely normalize it, so fail closed. + return _BlockedGitPush( + "unsupported git options before `push`; use `git push origin `" + ) + return _parse_push_args(git_args[i + 1 :], repo_dir=repo_dir) def _parse_push_args( @@ -336,17 +368,25 @@ def _workflow_tree_at_ref( def _fetch_remote_base(backend: Any, repo_dir: str | None, remote: str, branch: str) -> str | None: - """Fetch the base ref from the authenticated remote and return a local alias for it. + """Fetch the base ref from the exact URL the push will target and return a local alias. - Fetches the pushed branch first; if it does not exist on the remote, fetches the - remote's default branch. Uses `FETCH_HEAD` so the base is bound to the freshly-fetched - remote tip, not a local `refs/remotes/origin/*` ref that the sandbox could rewrite. + The base is fetched from the remote's effective *push* URL (`git remote get-url --push`), + not the remote name, because git honors a separate `pushurl`: an untrusted sandbox can + point the fetch URL at attacker/local content while the push still lands on the real + repo. Fetching the base from the push URL keeps the base and the push destination the + same authenticated repo, so a split cannot hide a workflow change. Uses `FETCH_HEAD` + (freshly fetched), never a local `refs/remotes/origin/*` ref the sandbox could rewrite. """ - fetch = _run_git(backend, repo_dir, f"fetch {shlex.quote(remote)} {shlex.quote(branch)}") + push_url_res = _run_git(backend, repo_dir, f"remote get-url --push {shlex.quote(remote)}") + push_url = _first_line(push_url_res.output) if push_url_res.ok else "" + if not push_url: + return None + + fetch = _run_git(backend, repo_dir, f"fetch {shlex.quote(push_url)} {shlex.quote(branch)}") if fetch.ok: return "FETCH_HEAD" - default = _run_git(backend, repo_dir, "ls-remote --symref origin HEAD") + default = _run_git(backend, repo_dir, f"ls-remote --symref {shlex.quote(push_url)} HEAD") default_branch = "" if default.ok: default_branch = _first_line(default.output).removeprefix("ref: refs/heads/").split("\t")[0] @@ -354,7 +394,9 @@ def _fetch_remote_base(backend: Any, repo_dir: str | None, remote: str, branch: for fallback in (default_branch, "dev", "main"): if not fallback or fallback == branch: continue - fetch = _run_git(backend, repo_dir, f"fetch {shlex.quote(remote)} {shlex.quote(fallback)}") + fetch = _run_git( + backend, repo_dir, f"fetch {shlex.quote(push_url)} {shlex.quote(fallback)}" + ) if fetch.ok: return "FETCH_HEAD" return None @@ -382,7 +424,22 @@ def _workflow_change_for_push(backend: Any, parsed: ParsedGitPush) -> WorkflowPu head_tree = _workflow_tree_at_ref(backend, root, head) if head_tree is None: - return None + # The workflow tree at head could not be read (an empty tree returns ([], hash), so + # None means the `ls-tree` read genuinely failed). Fail closed rather than skipping + # the guard, mirroring the base-read path. + return WorkflowPushChange( + fingerprint="", + repo="", + branch=branch_name, + files=[], + head_sha=head, + remote=parsed.remote, + local_ref=parsed.local_ref, + remote_ref=parsed.remote_ref, + fixed_command="", + blocked=True, + blocked_reason="could not read the workflow tree at the pushed head; blocking to be safe", + ) base_ref = _fetch_remote_base(backend, root, parsed.remote, branch_name) if base_ref is not None: diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index 0cbe387c..6edc93f8 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -28,6 +28,8 @@ class _Backend: base_tree_entries: dict[str, str] | None = None, remote_branches: set[str] | None = None, default_branch: str = "main", + push_url: str = "https://github.com/langchain-ai/open-swe.git", + ls_tree_head_ok: bool = True, ) -> None: self.workflow_files = workflow_files self.tree_entries = tree_entries @@ -35,6 +37,8 @@ class _Backend: self.base_tree_entries = base_tree_entries if base_tree_entries is not None else {} self.remote_branches = remote_branches or {"feature"} self.default_branch = default_branch + self.push_url = push_url + self.ls_tree_head_ok = ls_tree_head_ok self.commands: list[str] = [] self.head = "a" * 40 self.base_sha = "b" * 40 @@ -56,12 +60,14 @@ class _Backend: return self._head_tree() def _fetch_branch(self, command: str) -> _Response: - # Format: "git -C /repo fetch origin " or "git fetch origin " - prefix = "fetch origin " - idx = command.find(prefix) + # Format: "git [-C /repo] fetch " + idx = command.find(" fetch ") if idx == -1: return _Response("", 1) - branch = command[idx + len(prefix) :].strip() + parts = command[idx + len(" fetch ") :].split() + if len(parts) < 2: + return _Response("", 1) + branch = parts[-1] if branch in self.remote_branches: return _Response("") return _Response("", 1) @@ -70,13 +76,15 @@ class _Backend: self.commands.append(command) if "rev-parse --show-toplevel" in command: return _Response("/repo\n") + if "remote get-url --push" in command: + return _Response("", 1) if not self.push_url else _Response(f"{self.push_url}\n") if "ls-tree -r" in command: if "FETCH_HEAD" in command: return _Response(self._base_tree()) - return _Response(self._head_tree()) - if "fetch origin" in command: + return _Response(self._head_tree()) if self.ls_tree_head_ok else _Response("", 1) + if " fetch " in command: return self._fetch_branch(command) - if "ls-remote --symref origin HEAD" in command: + if "ls-remote --symref" in command: return _Response( f"ref: refs/heads/{self.default_branch}\tHEAD\n{self.base_sha}\tHEAD\n" ) @@ -351,7 +359,9 @@ async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.Monke assert called is True assert isinstance(result, ToolMessage) assert result.content == "pushed" - assert any("fetch origin" in cmd for cmd in backend.commands) + # The base is fetched from the effective push URL, not the `origin` remote name. + assert any("remote get-url --push" in cmd for cmd in backend.commands) + assert any(f" fetch {backend.push_url}" in cmd for cmd in backend.commands) async def test_stale_workflow_approval_is_loud_and_blocks( @@ -687,3 +697,106 @@ async def test_approved_workflow_push_runs_on_non_langsmith_providers( assert isinstance(result, ToolMessage) assert result.content == "pushed" assert refresh_calls == [] + + +def test_parse_git_push_guards_wrapped_and_optioned_forms() -> None: + # Path-qualified and wrapped git invocations must still be recognized as pushes + # (so the guard engages), not passed through as "not a git command". + for command in ( + "/usr/bin/git push origin feature", + "command git push origin feature", + "env FOO=bar git push origin feature", + ): + parsed = guard._parse_git_push(command) + assert isinstance(parsed, guard.ParsedGitPush), command + assert parsed.remote_ref == "feature" + + # A push carrying unsupported global options cannot be normalized, so it is blocked + # (fail closed) rather than run unguarded. + for command in ( + "git -c protocol.version=2 push origin feature", + "git --git-dir=.git push origin feature", + "git --no-pager push origin feature", + ): + assert isinstance(guard._parse_git_push(command), guard._BlockedGitPush), command + + # A non-push git command is still ignored. + assert guard._parse_git_push("/usr/bin/git status") is None + + +async def test_base_fetched_from_push_url_not_origin_remote( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # A changed workflow file at head (vs the trusted base fetched from the push URL) must + # require approval, and the base must be fetched from the push URL, never the bare + # `origin` remote name (which the sandbox can repoint via `remote set-url --push`). + backend = _Backend( + tree_entries={".github/workflows/ci.yml": "head-sha"}, + base_tree_entries={".github/workflows/ci.yml": "base-sha"}, + ) + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + assert any("remote get-url --push" in cmd for cmd in backend.commands) + assert any(f" fetch {backend.push_url}" in cmd for cmd in backend.commands) + assert not any(" fetch origin " in cmd for cmd in backend.commands) + + +async def test_unreadable_head_tree_blocks(monkeypatch: pytest.MonkeyPatch) -> None: + # If `ls-tree` on the head cannot be read, the guard must fail closed (block), not + # skip the guard and run the original push. + backend = _Backend(ls_tree_head_ok=False) + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fail_approval(*args: Any, **kwargs: Any) -> bool: + raise AssertionError("approval should not be checked for a blocked push") + + monkeypatch.setattr(guard, "workflow_push_approved", fail_approval) + + called = False + + async def handler(_request: Any) -> ToolMessage: + nonlocal called + called = True + return ToolMessage(content="pushed", tool_call_id="call-1") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert called is False + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["error_type"] == "WorkflowPushBlocked" From abbb10dd00e916b9aa0dbaa9feb9c6622f8f3347 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 1 Jul 2026 18:59:59 -0400 Subject: [PATCH 7/8] fix(workflow-guard): fail closed on ambiguous wrappers and multi/rewritten push URLs Follow-up hardening from the security re-verification of the prior fix: - H6: the parser's wrapper skip-loop skipped a wrapper's option flag but not its space-separated value, so `nice -n 10 git push`, `sudo -u ci git push`, `timeout 5 git push` (and wrappers outside the set) returned None and ran unguarded. Rewrite `_parse_git_tokens` to locate the `git` executable and fail closed on any unrecognized leading token before a git push, instead of a silent pass-through. - H1: `git remote get-url --push` returns only the first of multiple pushurl entries while `git push` writes to ALL of them, and insteadOf/pushInsteadOf can rewrite the destination. Use `get-url --push --all` and fail closed unless there is exactly one push URL and no URL rewrite is configured. Adds tests for valued-wrapper-option pushes, multiple push URLs, and URL rewrites. Claude-Session: https://claude.ai/code/session_01GxSndB7VoGQyeS196eUr5E --- agent/middleware/workflow_push_guard.py | 72 ++++++++------ tests/test_workflow_push_guard.py | 122 +++++++++++++++++++++++- 2 files changed, 161 insertions(+), 33 deletions(-) diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index d8df8f72..624afa53 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -181,38 +181,33 @@ def _parse_git_push(command: str) -> ParsedGitPush | _BlockedGitPush | None: return _parse_git_tokens(tokens, repo_dir=None) -def _git_invocation_args(tokens: list[str]) -> list[str] | None: - """Return the args following the `git` executable if these tokens invoke git, else None. - - Recognizes bareword `git`, path invocations like `/usr/bin/git`, and simple command - wrappers (`command`/`env`/`nice`/`sudo`/...), including `env NAME=VALUE ...`. This keeps - a wrapped or path-qualified `git push` from being mistaken for a non-git command and - passed through the guard unchecked. - """ - idx = 0 - while idx < len(tokens): - tok = tokens[idx] - if tok.rsplit("/", 1)[-1] == "git": - return tokens[idx + 1 :] - if tok in _GIT_WRAPPERS: - idx += 1 - # Skip wrapper options and `NAME=VALUE` assignments (e.g. `env FOO=bar git ...`). - while idx < len(tokens) and (tokens[idx].startswith("-") or "=" in tokens[idx]): - idx += 1 - continue - return None - return None - - def _parse_git_tokens( tokens: list[str], *, repo_dir: str | None ) -> ParsedGitPush | _BlockedGitPush | None: - git_args = _git_invocation_args(tokens) - if git_args is None: + """Parse tokens into a supported git push, a block sentinel, or None. + + Recognizes bareword `git`, path invocations (`/usr/bin/git`), and wrapper-prefixed + forms (`command`/`env`/`nice`/... git). Anything git-push-shaped that cannot be reduced + to `git [-C ] push [-u] origin ` fails closed to a block rather than a + silent pass-through. Non-push git commands and non-git commands return None. + """ + git_idx = next((idx for idx, tok in enumerate(tokens) if tok.rsplit("/", 1)[-1] == "git"), None) + if git_idx is None: return None + git_args = tokens[git_idx + 1 :] if "push" not in git_args: - # A non-push git command (e.g. `git status`) is not our concern. + # A non-push git command (e.g. `git status`, `git pull`). return None + # Everything before the `git` executable must be a benign wrapper prefix (a known + # wrapper, an option flag, or a NAME=VALUE assignment). An unrecognized leading token + # (a value-taking wrapper option like the `10` in `nice -n 10`, an unknown wrapper, or + # `git` used as an argument to another program) is ambiguous, so fail closed. + if any( + not (tok in _GIT_WRAPPERS or tok.startswith("-") or "=" in tok) for tok in tokens[:git_idx] + ): + return _BlockedGitPush( + "unrecognized wrapper before `git push`; use `git push origin `" + ) i = 0 while i < len(git_args) and git_args[i] != "push": if git_args[i] == "-C" and i + 1 < len(git_args): @@ -376,12 +371,31 @@ def _fetch_remote_base(backend: Any, repo_dir: str | None, remote: str, branch: repo. Fetching the base from the push URL keeps the base and the push destination the same authenticated repo, so a split cannot hide a workflow change. Uses `FETCH_HEAD` (freshly fetched), never a local `refs/remotes/origin/*` ref the sandbox could rewrite. + + Returns None (which makes the caller require approval) if the push destination is + ambiguous or rewritten: multiple `pushurl` entries (git pushes to ALL of them, so a + single base cannot represent the destination) or any `insteadOf`/`pushInsteadOf` URL + rewrite the sandbox could use to make the inspected URL differ from the push target. """ - push_url_res = _run_git(backend, repo_dir, f"remote get-url --push {shlex.quote(remote)}") - push_url = _first_line(push_url_res.output) if push_url_res.ok else "" - if not push_url: + rewrites = _run_git( + backend, repo_dir, "config --get-regexp " + shlex.quote(r"url\..*\.(push)?insteadof") + ) + if rewrites.ok and rewrites.output.strip(): + # A URL rewrite means the inspected push URL may not be the real destination. return None + push_url_res = _run_git(backend, repo_dir, f"remote get-url --push --all {shlex.quote(remote)}") + push_urls = ( + [line.strip() for line in push_url_res.output.splitlines() if line.strip()] + if push_url_res.ok + else [] + ) + # `git push` sends to every configured push URL; if there is not exactly one we cannot + # represent the destination with a single base, so fail closed. + if len(push_urls) != 1: + return None + push_url = push_urls[0] + fetch = _run_git(backend, repo_dir, f"fetch {shlex.quote(push_url)} {shlex.quote(branch)}") if fetch.ok: return "FETCH_HEAD" diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index 6edc93f8..6db0fae2 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -29,6 +29,8 @@ class _Backend: remote_branches: set[str] | None = None, default_branch: str = "main", push_url: str = "https://github.com/langchain-ai/open-swe.git", + push_urls: list[str] | None = None, + url_rewrites: str = "", ls_tree_head_ok: bool = True, ) -> None: self.workflow_files = workflow_files @@ -38,6 +40,9 @@ class _Backend: self.remote_branches = remote_branches or {"feature"} self.default_branch = default_branch self.push_url = push_url + # Effective push URLs (`git remote get-url --push --all`); defaults to [push_url]. + self.push_urls = push_urls if push_urls is not None else ([push_url] if push_url else []) + self.url_rewrites = url_rewrites self.ls_tree_head_ok = ls_tree_head_ok self.commands: list[str] = [] self.head = "a" * 40 @@ -76,8 +81,12 @@ class _Backend: self.commands.append(command) if "rev-parse --show-toplevel" in command: return _Response("/repo\n") + if "config --get-regexp" in command: + return _Response(f"{self.url_rewrites}\n") if self.url_rewrites else _Response("", 1) if "remote get-url --push" in command: - return _Response("", 1) if not self.push_url else _Response(f"{self.push_url}\n") + if not self.push_urls: + return _Response("", 1) + return _Response("".join(f"{url}\n" for url in self.push_urls)) if "ls-tree -r" in command: if "FETCH_HEAD" in command: return _Response(self._base_tree()) @@ -711,14 +720,21 @@ def test_parse_git_push_guards_wrapped_and_optioned_forms() -> None: assert isinstance(parsed, guard.ParsedGitPush), command assert parsed.remote_ref == "feature" - # A push carrying unsupported global options cannot be normalized, so it is blocked - # (fail closed) rather than run unguarded. + # A push carrying unsupported global options, an unknown wrapper, or a value-taking + # wrapper option cannot be normalized, so it is blocked (fail closed) rather than run + # unguarded. None of these may return None. for command in ( "git -c protocol.version=2 push origin feature", "git --git-dir=.git push origin feature", "git --no-pager push origin feature", + "nice -n 10 git push origin feature", + "sudo -u ci git push origin feature", + "timeout 5 git push origin feature", + "ionice -c 2 git push origin feature", ): - assert isinstance(guard._parse_git_push(command), guard._BlockedGitPush), command + result = guard._parse_git_push(command) + assert result is not None, command + assert isinstance(result, guard._BlockedGitPush), command # A non-push git command is still ignored. assert guard._parse_git_push("/usr/bin/git status") is None @@ -775,6 +791,104 @@ async def test_base_fetched_from_push_url_not_origin_remote( assert not any(" fetch origin " in cmd for cmd in backend.commands) +async def test_multiple_push_urls_fail_closed(monkeypatch: pytest.MonkeyPatch) -> None: + # `git push` sends to ALL configured push URLs, but `get-url --push` shows only the + # first. If the head workflow tree matches the base fetched from the first (benign) + # URL, the guard must still require approval rather than skip, because a second push + # URL could land the change on the real repo. Head == base here, but multiple push + # URLs force fail-closed (approval required). + backend = _Backend( + tree_entries={".github/workflows/ci.yml": "same-sha"}, + base_tree_entries={".github/workflows/ci.yml": "same-sha"}, + push_urls=[ + "https://github.com/attacker/local.git", + "https://github.com/langchain-ai/open-swe.git", + ], + ) + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + async def handler(_request: Any) -> ToolMessage: + raise AssertionError("push must not run without approval when push URLs are ambiguous") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + + +async def test_url_rewrite_fails_closed(monkeypatch: pytest.MonkeyPatch) -> None: + # An insteadOf/pushInsteadOf rewrite means the inspected push URL may not be the real + # destination, so the guard must require approval (never auto-skip). + backend = _Backend( + tree_entries={".github/workflows/ci.yml": "same-sha"}, + base_tree_entries={".github/workflows/ci.yml": "same-sha"}, + url_rewrites="url.https://evil.example/.insteadof https://github.com/", + ) + guard.SANDBOX_BACKENDS["thread-1"] = backend + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_rejected(thread_id: str, fingerprint: str) -> bool: + return False + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + async def fake_pending(thread_id: str, **kwargs: Any) -> tuple[dict[str, Any], bool]: + return {"fingerprint": kwargs["fingerprint"], "status": "pending", "notified": False}, True + + async def fake_post(*args: Any, **kwargs: Any) -> tuple[str, None]: + return "1700000000.000300", None + + async def fake_notified(*args: Any, **kwargs: Any) -> None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "workflow_push_rejected", fake_rejected) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "ensure_workflow_push_pending", fake_pending) + monkeypatch.setattr(guard, "post_slack_thread_reply_with_ts", fake_post) + monkeypatch.setattr(guard, "mark_workflow_push_notified", fake_notified) + + async def handler(_request: Any) -> ToolMessage: + raise AssertionError("push must not run without approval when a URL rewrite exists") + + result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + payload = json.loads(str(result.content)) + assert payload["workflow_approval_status"] == "approval_required" + + async def test_unreadable_head_tree_blocks(monkeypatch: pytest.MonkeyPatch) -> None: # If `ls-tree` on the head cannot be read, the guard must fail closed (block), not # skip the guard and run the original push. From aedfbbd00c042a90c395f06c122d013c778bf18f Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 1 Jul 2026 19:34:40 -0400 Subject: [PATCH 8/8] fix(workflow-guard): only guard commands that actually push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser failed closed on any execute command containing shell metacharacters, even ones with no `git push` at all (a bundled clone+commit heredoc, `echo x && ls`). That blocked the E2E full-flow implement step before it could push, so the opened PR carried an empty file list and Playwright failed — and it would break most real agent shell usage (pipes, redirects, heredocs, env vars). Engage the guard only when the command genuinely invokes `git push`, detected precisely on the shlex-normalized token stream. Push the E2E branch as a standalone, inspectable `git push` rather than bundling it in the heredoc. Hardening (from an adversarial security-review pass): a `git push` the shell would run can hide from a literal token scan via fused separators (`true;git push`), subshell/grouping (`(git push …)`), value-option splicing (`git -c ; git push`), or expansion/quoting (`git${IFS}push`, `git $(printf push)`, `git $'push'`, `git $'\x70ush'`). Fail closed on those via a quote-aware command skeleton (ANSI-C `$'…'` decoded) while still allowing legitimate metacharacter commands — chained commits, pipes, redirects, `$VAR`, and `$'…\t…'` format strings. --- agent/middleware/workflow_push_guard.py | 149 +++++++++++++++++++++++- tests/e2e/fake_llm.py | 19 ++- tests/test_workflow_push_guard.py | 76 ++++++++++++ 3 files changed, 236 insertions(+), 8 deletions(-) diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 624afa53..e5994d9a 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import codecs import hashlib import json import logging @@ -44,10 +45,19 @@ _SHELL_OPERATORS = {";", "|", "||", "&"} _REF_NAME = re.compile(r"^[A-Za-z0-9._/@+-]+$") _GIT_OBJECT_ID = re.compile(r"^[0-9a-fA-F]{40,64}$") _UNSAFE_RAW_COMMAND = re.compile(r"[;|`$<>\n\r]") +# Unquoted shell expansion/substitution (`$VAR`, `${...}`, `$(...)`, backticks) can produce +# a `git push` whose text we cannot see; its presence alongside a push forces fail-closed. +_EXPANSION = re.compile(r"[$`]") +# Unquoted command separators and grouping. Splitting the unquoted command skeleton on these +# yields the individual simple-commands the shell would run, each checked for a `git push`. +_SEGMENT_SPLIT = re.compile(r"[;|&(){}\n]+") # Command wrappers that can prefix a `git` invocation (e.g. `command git push`, # `env FOO=bar git push`, `/usr/bin/git push`). Recognized so a wrapped or path-qualified # git push cannot slip past the guard as "not a git command". _GIT_WRAPPERS = {"command", "env", "nice", "sudo", "stdbuf", "nohup", "time", "ionice", "setsid"} +# Git global options that consume the following token as a separate value; skipped when +# locating the git subcommand so `git -c k=v push` is still recognized as a push. +_GIT_VALUE_OPTIONS = {"-C", "-c", "--namespace", "--git-dir", "--work-tree", "--exec-path"} class _BlockedGitPush: @@ -160,14 +170,145 @@ def _response_ok(response: Any) -> bool: return True +def _tokens_invoke_git_push(tokens: list[str]) -> bool: + """True if the token stream runs `git push` as a git subcommand (not merely mentions it). + + Scans each `git` invocation (bareword or path-qualified, after any wrapper prefix), + skips `-C ` and option flags, and checks whether the first positional argument is + `push`. This ignores the word "push" appearing inside a commit message or another + command, so `git commit -m "push it" && ls` is not treated as a push, while a genuine + `git ... push` anywhere in a chain is. + """ + i = 0 + while i < len(tokens): + if tokens[i].rsplit("/", 1)[-1] != "git": + i += 1 + continue + j = i + 1 + while j < len(tokens): + tok = tokens[j] + if tok in _SHELL_OPERATORS or tok == "&&": + break + if ( + tok in _GIT_VALUE_OPTIONS + and j + 1 < len(tokens) + and tokens[j + 1] not in _SHELL_OPERATORS + and tokens[j + 1] != "&&" + ): + j += 2 # option consumes the next token as its value + continue + if tok.startswith("-"): + j += 1 + continue + if tok == "push": + return True + break # first positional is a non-push subcommand + i = j + 1 + return False + + +def _decode_ansi_c(body: str) -> str: + """Best-effort ANSI-C (`$'...'`) escape decoding, so `$'\\x70ush'` / `$'\\160ush'` reveal + the literal `push` the shell would run. Falls back to the raw body if decoding fails.""" + if "\\" not in body: + return body + try: + return codecs.decode(body, "unicode_escape") + except Exception: + return body + + +def _strip_quoted(command: str) -> str: + """Return the command with quoted spans and escaped characters blanked to spaces. + + Leaves only the unquoted shell structure, so metacharacters that are literal (inside + quotes or backslash-escaped — e.g. a `;` or `&` inside a commit message) do not look + like command separators. Plain `'...'`/`"..."` spans wrap arguments and are blanked; + `$'...'`/`$"..."` (ANSI-C / locale quoting) form a literal *word* (e.g. `git $'push'` runs + `git push`), so their content is kept — otherwise the push word would vanish. Unbalanced + quotes blank the remainder (fail-closed shape). + """ + out: list[str] = [] + i, n = 0, len(command) + while i < n: + c = command[i] + if c in "'\"": + # `$'...'` / `$"..."` — the preceding `$` marks a literal word, so keep content + # and drop the `$` so adjacent pieces concatenate (`$'pus'$'h'` -> `push`). + word_quote = bool(out) and out[-1] == "$" + if word_quote: + out.pop() + if c == "'": + j = command.find("'", i + 1) + if j == -1: + return "".join(out) + " " + out.append(_decode_ansi_c(command[i + 1 : j]) if word_quote else " ") + i = j + 1 + else: + k = i + 1 + buf: list[str] = [] + while k < n and command[k] != '"': + if command[k] == "\\" and k + 1 < n: + buf.append(command[k + 1]) + k += 2 + else: + buf.append(command[k]) + k += 1 + if k >= n: + return "".join(out) + " " + out.append("".join(buf) if word_quote else " ") + i = k + 1 + elif c == "\\" and i + 1 < n: + out.append(" ") + i += 2 + else: + out.append(c) + i += 1 + return "".join(out) + + +def _maybe_obfuscated_git_push(command: str) -> bool: + """True if the shell would run a `git push` that `_tokens_invoke_git_push` could not see. + + Only reached when the precise token scan found no clean push. `shlex` splits on + whitespace and performs no expansion, so a push can hide behind a fused separator + (`true;git push`), subshell/grouping (`(git push ...)`), or expansion (`git${IFS}push`, + `git $(printf push)`). We work on the unquoted command skeleton so literal metacharacters + inside a commit message are ignored (no false positive on `git commit -m "push it" && ls` + or `git log | grep push`), then fail closed if any unquoted simple-command is a `git push` + or if unquoted expansion could produce one. + """ + skeleton = _strip_quoted(command) + if "push" not in skeleton: + # The only `push` text is inside quotes (a literal argument), so it cannot be a + # command word. (A push spelled purely by expansion with no literal `push` anywhere + # is left to the GitHub proxy token scope, which lacks workflows:write until approval.) + return False + if _EXPANSION.search(skeleton): + return True + return any( + _tokens_invoke_git_push(segment.split()) for segment in _SEGMENT_SPLIT.split(skeleton) + ) + + def _parse_git_push(command: str) -> ParsedGitPush | _BlockedGitPush | None: stripped = command.strip() + try: + probe = shlex.split(stripped) + except ValueError: + probe = None + if probe is None or not _tokens_invoke_git_push(probe): + # No push the token scan can normalize. Fail closed if one is hidden behind shell + # metacharacters/expansion; otherwise leave the command untouched. + if _maybe_obfuscated_git_push(stripped): + return _BlockedGitPush( + "possible git push obscured by shell metacharacters; issue it as a plain " + "`git push origin `" + ) + return None if _UNSAFE_RAW_COMMAND.search(stripped) or "&" in stripped.replace("&&", ""): return _BlockedGitPush("unsafe shell characters in git push command") - try: - tokens = shlex.split(stripped) - except ValueError: - return _BlockedGitPush("unparseable git push command") + tokens = probe if not tokens: return None diff --git a/tests/e2e/fake_llm.py b/tests/e2e/fake_llm.py index b970b0d6..08dcd665 100644 --- a/tests/e2e/fake_llm.py +++ b/tests/e2e/fake_llm.py @@ -28,8 +28,11 @@ from langchain_core.language_models import BaseChatModel from langchain_core.messages import AIMessage, BaseMessage, HumanMessage, ToolMessage from langchain_core.outputs import ChatGeneration, ChatResult -# One shell command that does the whole git workflow. Each execute() runs in a -# fresh shell rooted at the sandbox dir, so the clone+commit+push is bundled. +# One shell command that clones, implements, and commits. Each execute() runs in a +# fresh shell rooted at the sandbox dir, and the `repo` checkout persists on disk +# between calls. The push is a separate, standalone step: the workflow-push guard only +# inspects (and gates) pushes issued as a plain `git push origin `, so bundling +# the push inside this heredoc would trip its fail-closed check. _IMPLEMENT_SCRIPT = f""" set -e rm -rf repo @@ -44,10 +47,12 @@ def greet(name): EOF git add -A git commit -m "{PR_TITLE}" -git push origin {FEATURE_BRANCH} -echo PUSHED_OK +echo IMPLEMENTED_OK """.strip() +# Standalone push in the inspectable form the workflow-push guard expects. +_PUSH_SCRIPT = f"cd repo && git push origin {FEATURE_BRANCH}" + _PLAN_URL_RE = re.compile(r"https?://[^\s\"'<>)\]|]+/plan\b") _ATTRIBUTION_RE = re.compile(r"@([A-Za-z0-9-]+):") @@ -281,6 +286,12 @@ SCRIPT_LIBRARY: dict[str, tuple[StepSpec, ...]] = { {"command": _IMPLEMENT_SCRIPT}, "call-impl", ), + _tool_step( + "Pushing the branch.", + "execute", + {"command": _PUSH_SCRIPT}, + "call-push", + ), _tool_step( "Opening a pull request.", "open_pull_request", diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index 6db0fae2..ac38e1ea 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -169,6 +169,82 @@ def test_parse_git_push_blocks_unsafe_and_unrecognized_forms() -> None: assert guard._parse_git_push("git status") is None +def test_parse_git_push_ignores_commands_without_a_push() -> None: + # Chained or heredoc commands that never `git push` are not our concern — the + # fail-closed blocking is reserved for commands that actually push. + assert guard._parse_git_push("echo planning && ls") is None + assert guard._parse_git_push("git add -A && git commit -m 'x'") is None + assert guard._parse_git_push("cat > f <<'EOF'\nhi\nEOF") is None + # "push" inside a commit message is not a push subcommand. + assert guard._parse_git_push("git commit -m 'push it real good' && ls") is None + # A standalone `cd && git push origin ` stays inspectable, not blocked. + parsed = guard._parse_git_push("cd repo && git push origin feature") + assert isinstance(parsed, guard.ParsedGitPush) + assert parsed.repo_dir == "repo" + assert parsed.remote_ref == "feature" + + +def test_parse_git_push_still_catches_quote_obfuscated_push() -> None: + # Shell-quoting that the shell would run as a real `git push` must not slip past the + # guard just because the raw string lacks a literal "push" token: it is detected and + # inspected, never passed through as None. + parsed = guard._parse_git_push('git "pu""sh" origin feature') + assert isinstance(parsed, guard.ParsedGitPush) + assert parsed.remote_ref == "feature" + assert guard._tokens_invoke_git_push(["git", "status", "&&", "git", "push"]) is True + assert guard._tokens_invoke_git_push(["command", "git", "push", "origin", "x"]) is True + assert guard._tokens_invoke_git_push(["git", "-c", "protocol.version=2", "push"]) is True + assert guard._tokens_invoke_git_push(["git", "commit", "-m", "push"]) is False + # A value-option must not swallow a shell operator as its "value" and thereby hide the + # real `git push` that follows the operator. + assert guard._tokens_invoke_git_push(["git", "-c", ";", "git", "push"]) is True + + +def test_parse_git_push_fails_closed_on_metachar_obfuscated_push() -> None: + # `shlex` splits only on whitespace and performs no expansion, so a push can hide behind + # a fused separator or a variable/command expansion. Each of these is a real push the + # sandbox shell would run, and must fail closed rather than pass through unguarded. + for command in ( + "true;git push origin HEAD", + "echo hi|git push origin HEAD", + "git${IFS}push origin HEAD", + "$(echo git) push origin HEAD", + "git $(printf push) origin main", + "cmd=push; git ${cmd} origin main", + "git -c ; git push origin main", + "git -c | git push origin main", + "(git push origin main)", + "(git push -u origin HEAD)", + "{ git push origin main; }", + "git $'push' origin main", + "git $'pus'$'h' origin main", + r"git $'\x70ush' origin main", + r"git $'\160ush' origin main", + ): + assert isinstance(guard._parse_git_push(command), guard._BlockedGitPush), command + + +def test_parse_git_push_allows_legitimate_metachar_commands() -> None: + # Standalone operators / redirects around a non-push git command (or a push mentioned in + # a commit message) are common and must not be over-blocked. Metacharacters that are + # literal because they sit inside a quoted commit message must not read as separators. + assert guard._parse_git_push("git commit -m 'push it real good' && ls") is None + assert guard._parse_git_push('git commit -m "push & shove (v2)" && ls') is None + assert guard._parse_git_push('git commit -m "$MSG about push" && npm test') is None + assert guard._parse_git_push("git log | grep push") is None + assert guard._parse_git_push("git diff > push.txt") is None + # A non-push git command with an unquoted variable must not be blocked just for the `$`. + assert guard._parse_git_push("git checkout $BRANCH") is None + assert guard._parse_git_push("git log --grep=$PATTERN | head") is None + # Legitimate ANSI-C quoting (a tab/newline in a git format or message) is not a push. + assert guard._parse_git_push(r"git log --pretty=$'%h\t%s'") is None + assert guard._parse_git_push(r"git commit -m $'line1\nline2'") is None + # The standard `cd && git push` form is recognized, not blocked as obfuscation. + parsed = guard._parse_git_push("cd repo && git push origin feature") + assert isinstance(parsed, guard.ParsedGitPush) + assert parsed.remote_ref == "feature" + + def test_workflow_change_for_push_fingerprints_head_workflow_tree() -> None: backend = _Backend() change = guard._workflow_change_for_push(