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(