Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .claude/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@
"Bash(gh api * -F *)",
"Bash(gh api --input*)",
"Bash(gh api * --input*)",
"Bash(gh api --field *)",
"Bash(gh api * --field *)",
"Bash(gh api --raw-field *)",
"Bash(gh api * --raw-field *)",
"Bash(gh issue close*)"
]
},
Expand Down
9 changes: 9 additions & 0 deletions backend/tests/test_agent_permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,15 @@ def test_gh_api_writes_ask_in_either_flag_position(rules: dict[str, list[str]])
f"gh api --method DELETE {endpoint}",
f"gh api {endpoint} -f body=hi",
f"gh api -f body=hi {endpoint}",
# Long forms. `gh api --help`: `-F, --field` and `-f, --raw-field`.
# Enumerating only the short spellings left these reaching `allow` via
# `Bash(gh api *)` -- a write that never prompts. This is the exact
# leak the enumeration keeps re-opening, so it is pinned by command
# string here rather than by asserting which rule happens to catch it.
f"gh api {endpoint} --field body=hi",
f"gh api --field body=hi {endpoint}",
f"gh api {endpoint} --raw-field body=hi",
f"gh api --raw-field body=hi {endpoint}",
):
assert decide(write, rules) == "ask", f"{write!r} must ask"

Expand Down
31 changes: 24 additions & 7 deletions docs/agents/local-agent-environment.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ not `.git`'s own recovery data:

| Category | Rules |
|---|---|
| Escapes to GitHub | **every `gh api`**, `gh pr merge`, `gh release create` / `edit` / `delete` / `delete-asset` / `upload`, `gh repo edit`, `gh secret`, `gh workflow run` |
| Escapes to GitHub | **every `gh api` WRITE** (see below), `gh pr merge`, `gh release create` / `edit` / `delete` / `delete-asset` / `upload`, `gh repo edit`, `gh secret`, `gh workflow run` |
| Destroys the recovery mechanism | `git gc`, `git prune`, `git repack`, `git maintenance`, `git reflog expire`, `git reflog delete`, `git update-ref`, `git tag -d` / `--delete` / `-f` |
| Leaves the user boundary | `sudo` |

Expand Down Expand Up @@ -117,18 +117,35 @@ because leaving `rm` and `reset --hard` unattended is only defensible while the
object database and reflog can recover them — a `gc --prune=now` that ran
unprompted would remove the ground that argument stands on.

**`gh api` is guarded bluntly, and that is deliberate.** It was enumerated by
shape first, and it leaked:
**`gh api` asks on WRITES only, and the write flags are enumerated.** Reads
run unattended — `gh api <path>` and `gh api <path> --jq ...` are how a session
reads inline review comments, which no `gh pr view` field returns.

It was blanket-guarded first (`Bash(gh api)` / `Bash(gh api *)` in `ask`),
because the marker that makes a call a write sits at an arbitrary argument
position and **prefix globbing cannot reach it**:

```
gh api repos/o/r/pulls/N/merge -X PUT # merges, bypassing `gh pr merge`
gh api <path> -f key=val # any -f/-F makes it a POST
```

The marker sits at an arbitrary argument position, and **prefix globbing cannot
reach it.** Narrowing it back to specific forms re-opens both lines, so
`quality-check.sh` requires the blanket spelling rather than merely "some `gh
api` rule exists".
The blanket cost a prompt on every read, so #657 replaced it with one rule per
write flag in each argument position. That trade is the standing one, and it
has a standing risk: **a write flag nobody enumerated resolves to `allow`.**
Not hypothetical — `-F, --field` and `-f, --raw-field` (`gh api --help`) had
only their short forms listed, so `gh api <path> --field k=v` was a GitHub
write that never prompted, until it was pinned.

So `quality-check.sh` no longer asserts the *spelling*. It used to require the
literal blanket rules, which made it fail on a clean `main` the moment #657
respelled them, while the property it cared about still held. It now asserts
the *property*, by command string, in `MUST_BE_GUARDED` — those pins survive a
respelling.

**When you add a `gh api` write flag, add its command string to
`MUST_BE_GUARDED` too**, in both argument positions. That list is the guard;
the rules in `settings.json` are just how it is currently satisfied.

### Pushing is guarded server-side, not by a prompt

Expand Down
34 changes: 27 additions & 7 deletions scripts/quality-check.sh
Original file line number Diff line number Diff line change
Expand Up @@ -210,12 +210,27 @@ if ! python3 - <<'PY'
import json, re, sys

# Patterns match the command AS WRITTEN -- prefix globbing, no normalisation.
# `gh api` is guarded by a BLANKET rule on purpose: the dangerous shapes put
# their marker at an arbitrary argument position (`gh api <path> -X PUT`,
# `gh api <path> -f k=v`), which a prefix glob cannot reach. Enumerating them
# left real holes twice. Narrowing it back to specific forms re-opens the
# holes, so the check requires the blanket spelling rather than merely "some
# rule exists".
#
# `gh api` USED to be pinned here by spelling: this list demanded the literal
# `Bash(gh api)` / `Bash(gh api *)` blanket rules, on the grounds that the
# dangerous shapes put their marker at an arbitrary argument position
# (`gh api <path> -X PUT`, `gh api <path> -f k=v`) which a prefix glob cannot
# reach. #657 replaced that blanket with an enumeration of the write flags, to
# stop `gh api` reads prompting on every call -- and this gate then failed on
# a clean `origin/main`, because the strings it demanded were gone while the
# PROPERTY it cared about still held.
#
# Pinning a spelling was the wrong assertion. What matters is that a `gh api`
# WRITE cannot reach `allow`, not which rule stops it -- and that is already
# asserted below, by command string, in MUST_BE_GUARDED. Those entries survive
# any future respelling; the literal ones did not.
#
# The enumeration's real risk is a write flag nobody listed. That is not
# hypothetical: `-F, --field` and `-f, --raw-field` (see `gh api --help`) had
# only their SHORT forms enumerated, so `gh api <path> --field k=v` resolved to
# `allow` -- a write that never prompted. Both long forms are pinned below now.
# When adding a `gh api` write flag, add its command string there too, or the
# next respelling re-opens the same hole silently.
#
# `git push` USED to be in that same sentence and no longer is. It was never
# the glob that made it safe -- the glob was a blunt instrument compensating
Expand Down Expand Up @@ -259,7 +274,6 @@ REQUIRED = {
"Bash(podman machine rm)", "Bash(podman system reset)",
],
"ask": [
"Bash(gh api)", "Bash(gh api *)",
"Bash(gh pr merge*)", "Bash(gh repo edit*)",
"Bash(gh release create*)", "Bash(gh release edit*)",
"Bash(gh release delete*)", "Bash(gh release delete-asset*)",
Expand Down Expand Up @@ -322,6 +336,12 @@ MUST_BE_GUARDED = [
# gh reaching GitHub, including the raw API path
"gh api repos/o/r/pulls/1/merge -X PUT",
"gh api repos/o/r/releases -f tag_name=v1",
# Long forms of the two field flags. Enumerating only `-f`/`-F` left these
# reaching `allow`; see the REQUIRED comment above.
"gh api repos/o/r/releases --field tag_name=v1",
"gh api repos/o/r/releases --raw-field tag_name=v1",
"gh api --field tag_name=v1 repos/o/r/releases",
"gh api --raw-field tag_name=v1 repos/o/r/releases",
"gh pr merge 588 --squash",
# Publishing a release escapes to GitHub irreversibly. The read-only verbs
# are exempt (see MUST_NOT_BE_GUARDED) -- the gate pins both directions so
Expand Down
Loading