Harden gh-write-guard's Write Gate and Flag Parsing

host-setup/agent-safety/gh-write-guard.py

@@ -44,9 +44,10 @@
 
 # --- What counts as a GitHub write -------------------------------------------------------------------
 # The gh subcommands that mutate.
-# The `gh api` command is handled separately, since it needs field and method inspection.
+# A path-qualified or `.exe`-suffixed `gh` still starts a shell word this matches, the same recognition `_is_gh_exe` gives it for argv-position parsing.
+# The `gh api` command is handled separately, in `_is_gh_write`, since it needs argv-aware method and GraphQL-query inspection rather than a fixed subcommand list.
 _GH_WRITE_SUB = re.compile(
-    r"""\bgh\s+(?:
+    r"""\bgh(?:\.exe)?\s+(?:
         pr\s+(?:create|comment|close|merge|edit|review|reopen|ready|lock|unlock)
       | issue\s+(?:create|comment|close|edit|reopen|delete|lock|unlock|pin|unpin|transfer)
       | release\s+(?:create|edit|delete|upload)
@@ -56,17 +57,7 @@
     )\b""",
     re.VERBOSE,
 )
-_GH_API = re.compile(r"\bgh\s+api\b")
-# `-X`/`--method` accept a separate value (`-X POST`), an attached one (`-XPOST`), and an equals-attached one (`-X=POST`, `--method=POST`).
-# This must match every spelling, matching how `_gh_effective_method` reads it.
-_EXPLICIT_WRITE_METHOD = re.compile(
-    r"(?:--method[= ]|-X[= ]?)\s*(?:POST|PUT|PATCH|DELETE)\b", re.IGNORECASE
-)
-# A gh api call with a field flag defaults to POST even without -X, so it is a write.
-# `-f`/`-F` also accept an attached value (`-fbody=x`), so no trailing `\b` is required after them.
-# It is required after the long-form spellings, where one legitimately separates the flag from the next word.
-_API_FIELD_FLAG = re.compile(r"(?:^|\s)(?:-f|-F)|(?:^|\s)(?:--field|--raw-field|--input)\b")
-_GRAPHQL = re.compile(r"\bgh\s+api\b.*\bgraphql\b", re.DOTALL)
+_GRAPHQL = re.compile(r"\bgh(?:\.exe)?\s+api\b.*\bgraphql\b", re.DOTALL)
 _MUTATION = re.compile(r"\bmutation\b")
 # Loose pre-filter only: matches `git` before `push` even with global options between them
 # (git -C <dir> push). _push_arg_lists is the accurate arbiter that confirms an executable push.
@@ -104,11 +95,12 @@
 # Every spelling gh accepts for the target flag, being `--repo x`, `--repo=x`, `-R x`, `-R=x`, and the attached short form `-Rx`.
 # A form left out is not a near-miss, it is a silent bypass of the whole repository scope, so each is read by argv position below (`_gh_write_targets`) rather than assumed to be a space-separated pair.
 _REPO_FLAG_BARE = {"--repo", "-R"}
-_REPOS_PATH_TOKEN = re.compile(r"^repos/(?P<owner>[A-Za-z0-9_.\-]+)/(?P<repo>[A-Za-z0-9_.\-]+)")
-# Flags whose own value is opaque text (a PR/issue title or body, a GraphQL field, a jq/template expression, a header), and so is skipped whole rather than pattern-matched for a repo target.
+# `gh api` accepts a leading slash on the path (`gh api /repos/o/r/...`), so it is optional here too.
+_REPOS_PATH_TOKEN = re.compile(r"^/?repos/(?P<owner>[A-Za-z0-9_.\-]+)/(?P<repo>[A-Za-z0-9_.\-]+)")
+# Flags whose own value is opaque text (a PR/issue title, body, or notes), and so is skipped whole rather than pattern-matched for a repo target.
 # Without this, a --body describing a `--repo <owner>/<repo>` doc line, or a commit message quoting the same convention, reads as a real flag.
-# The incident this closes denied an ordinary `git commit` whose message body merely quoted the fleet's own `--repo owner/repo` example text.
-_GH_TEXT_VALUE_FLAGS = {
+# Shared across every create/comment/edit-style subcommand (pr, issue, release, gist).
+_GH_CREATE_TEXT_VALUE_FLAGS = {
     "--title",
     "-t",
     "--body",
@@ -119,6 +111,10 @@
     "--message",
     "-m",
     "--desc",
+}
+# `gh api`'s own value-taking flags, meaningful only inside an `api` invocation.
+# `-f`/`-F` are the boolean `--fill` on `gh pr create`, so they must not be treated as value-consuming outside of `api`, or the flag right after them (a real `--repo <owner>/<repo>`) is silently skipped.
+_GH_API_VALUE_FLAGS = _GH_CREATE_TEXT_VALUE_FLAGS | {
     "-f",
     "-F",
     "--field",
@@ -139,15 +135,36 @@
 
 
 def _is_gh_write(cmd):
+    """True when `cmd` is a GitHub write: a known-mutating `gh` subcommand, a `git push`, or a `gh api`
+    call whose effective method is not GET.
+
+    The `gh api` half is argv-aware (via `_all_gh_arg_lists`/`_gh_api_path`/`_gh_effective_method`/
+    `_gh_graphql_query`), reading a flag or a GraphQL query only from where it actually sits in one
+    invocation's own argv rather than a substring search over the whole command. A raw substring search
+    reads a write-method spelling out of an opaque flag value too, such as a `--jq` expression that
+    merely contains the text `-XPOST` as data, misclassifying a harmless read as a write.
+
+    A GraphQL call whose body comes from `--input` is treated as a write whenever its query text cannot
+    be read at all (`_gh_graphql_query` returns None), since a `resolveReviewThread` mutation supplied
+    that way is equally invisible; `_check_reply_resolve_helper` denies that case explicitly.
+    """
     if _GH_WRITE_SUB.search(cmd) or _push_arg_lists(cmd):
         return True
-    if _GH_API.search(cmd):
-        if _EXPLICIT_WRITE_METHOD.search(cmd):
-            return True
-        if _GRAPHQL.search(cmd) and _MUTATION.search(cmd):
+    for args in _all_gh_arg_lists(cmd):
+        if not args or args[0] != "api":
+            continue
+        path = _gh_api_path(args)
+        if path == "graphql":
+            q = _gh_graphql_query(args)
+            if q:
+                if _MUTATION.search(q):
+                    return True
+                continue  # a genuine read-only query, not a mutation
+            if _gh_has_input(args):
+                return True  # uninspectable body; treat cautiously so rules 1-5 can look closer
+            continue
+        if _gh_effective_method(args) != "GET":
             return True
-        if _API_FIELD_FLAG.search(cmd) and not _GRAPHQL.search(cmd):
-            return True  # gh api <path> -f k=v  => POST
     return False
 
 
@@ -490,11 +507,13 @@ def _gh_write_targets(cmd):
     """
     targets = []
     for args in _all_gh_arg_lists(cmd):
+        # `-f`/`-F` are value-taking only inside `api`, on `pr create` etc. they are the boolean `--fill`, so treating them as value-consuming there would swallow a real following `--repo` flag whole.
+        flags = _GH_API_VALUE_FLAGS if args and args[0] == "api" else _GH_CREATE_TEXT_VALUE_FLAGS
         n = len(args)
         i = 0
         while i < n:
             t = args[i]
-            if t in _GH_TEXT_VALUE_FLAGS and "=" not in t:
+            if t in flags and "=" not in t:
                 i += 2  # this flag's own value is opaque text, never a repo target
                 continue
             if t in _REPO_FLAG_BARE:
@@ -530,7 +549,7 @@ def _gh_api_path(args):
     i = 1
     while i < n:
         t = args[i]
-        if t in _GH_TEXT_VALUE_FLAGS and "=" not in t:
+        if t in _GH_API_VALUE_FLAGS and "=" not in t:
             i += 2
             continue
         if t.startswith("-"):
@@ -581,15 +600,22 @@ def _gh_graphql_query(args):
     return None
 
 
+def _gh_has_input(args):
+    """True when this `gh api` invocation's own argv carries `--input` (bare or equals-attached), gh's
+    flag for supplying the request body from a file or stdin.
+    """
+    return any(t == "--input" or t.startswith("--input=") for t in args)
+
+
 def _gh_effective_method(args):
     """The effective HTTP method of a `gh api` invocation's own argv: an explicit `-X`/`--method` value
-    when present, in every spelling `gh` accepts, else POST when a field flag is present (`gh`'s own
-    default for a write-shaped call), else GET.
+    when present, in every spelling `gh` accepts, else POST when a field flag or `--input` is present
+    (`gh`'s own default for a write-shaped call), else GET.
     """
     n = len(args)
     i = 0
     method = None
-    has_field = False
+    has_field = _gh_has_input(args)
     while i < n:
         t = args[i]
         if t in ("-X", "--method"):
@@ -791,6 +817,10 @@ def _check_reply_resolve_helper(cmd, environ):
     target in its own text (the thread id is opaque), so the same fallback is permitted there whenever
     any grant is active this session, a coarser signal than a REST reply gets, and the residual gap the
     module docstring's "precision over recall" already accepts for this class of rule.
+
+    A GraphQL body supplied via `--input` is denied outright when it has no `-f`/`-F query=...` field to
+    read instead (`_gh_graphql_query` returns None), since a `resolveReviewThread` mutation there is
+    equally invisible to this parser and there is nothing to distinguish it from the inline case above.
     """
     granted = _granted_targets(environ)
     helper = (
@@ -811,6 +841,14 @@ def _check_reply_resolve_helper(cmd, environ):
                     "helper that captures the reply and the resolve in one call, so a reply can be left "
                     "unresolved across a push and a re-request. " + helper
                 )
+            if not q and _gh_has_input(args):
+                if granted:
+                    continue
+                return "deny", (
+                    "This gh api graphql call supplies its body via --input, which cannot be inspected "
+                    "for a resolveReviewThread mutation, so it is denied by the same rule as an inline "
+                    "one. " + helper
+                )
         if path and _REPLY_ENDPOINT_PATH.search(path) and _gh_effective_method(args) == "POST":
             m = _REPOS_PATH_TOKEN.match(path)
             if m:
@@ -1231,6 +1269,65 @@ def classify(cmd, cwd=None, origin=None, current_branch=None, rules_lookup=None,
         "deny",
         "the equals-attached -X=POST form still enters the write gate (CodeRabbit)",
     ),
+    (
+        "gh api repos/ptr727/PlexCleaner/pulls/5/comments/9/replies --input body.json",
+        {},
+        "deny",
+        "--input on the replies endpoint is still read as a write (promotion review)",
+    ),
+    (
+        "gh api graphql --method POST --input resolve.json",
+        {},
+        "deny",
+        "a GraphQL body from --input is denied as uninspectable (promotion review)",
+    ),
+    (
+        "gh api graphql --method POST --input resolve.json",
+        {_ALLOW_ENV: "esphome/esphome"},
+        "allow",
+        "an uninspectable --input GraphQL body is permitted under a cross-owner grant, like the inline case",
+    ),
+]
+
+_SCOPE_CASES_MORE: list[tuple[str, dict[str, str], str, str]] = [
+    # More Rule-3 scope cases, covering the promotion-review round's findings, kept as their own literal rather than growing the one above further.
+    # (command, environ, expected_decision, label)
+    (
+        "gh api repos/esphome/esphome/issues --jq '.[] | \"-XPOST\"'",
+        {},
+        "allow",
+        "a --jq expression only containing the text -XPOST is a read, not misread as a write (promotion review)",
+    ),
+    (
+        "gh.exe api repos/esphome/esphome/issues -f body=x",
+        {},
+        "deny",
+        "gh.exe is still recognized for an api write (CodeRabbit)",
+    ),
+    (
+        "gh.exe pr create --repo esphome/esphome --title x",
+        {},
+        "deny",
+        "gh.exe is still recognized for a pr-create write (companion to CodeRabbit's gh.exe finding)",
+    ),
+    (
+        "gh api /repos/esphome/esphome/issues -f title=x",
+        {},
+        "deny",
+        "a leading slash on the REST path does not hide the cross-owner target (CodeRabbit)",
+    ),
+    (
+        "gh pr create -f --repo esphome/esphome --title x",
+        {},
+        "deny",
+        "-f as pr create's boolean --fill does not swallow the following --repo (CodeRabbit)",
+    ),
+    (
+        "gh pr create -f --title x --body y",
+        {},
+        "allow",
+        "-f as pr create's boolean --fill does not swallow --title either, with no foreign target present",
+    ),
 ]
 
 # Rule-4 cases, covering branch-rule bypass.
@@ -1631,7 +1728,7 @@ def _selftest():
         if got != want:
             ok = False
         print(f"  {mark} [{got:5}] want={want:5} {label}")
-    for cmd, env, want, label in _SCOPE_CASES:
+    for cmd, env, want, label in _SCOPE_CASES + _SCOPE_CASES_MORE:
         got, _ = classify(
             cmd,
             origin=origin,