Skip to content

fix: keep gh api auto-approval read-only in Claude Code settings - #4558

Open
dennishavermans wants to merge 3 commits into
linuxfoundation:mainfrom
dennishavermans:fix/guard-gh-api-read-only
Open

fix: keep gh api auto-approval read-only in Claude Code settings#4558
dennishavermans wants to merge 3 commits into
linuxfoundation:mainfrom
dennishavermans:fix/guard-gh-api-read-only

Conversation

@dennishavermans

Copy link
Copy Markdown

Summary

Bash(gh api repos*) in .claude/settings.json was added so the review-pr skill can fetch PR comments, reviews, and commits, and it sits in a block of otherwise read-only gh rules. A prefix rule cannot see the HTTP method, so the same rule also auto-approves writes. This PR adds a PreToolUse guard, in the same shape as the existing guard-protected-files.sh, that lets gh api reads through untouched and blocks anything that changes the request method.

Everything below was measured on Claude Code 2.1.238 before opening this PR (each rule set in a scratch settings.json passed via --settings, raw stream-json transcripts, probes against a repo name that does not exist so the API returns 404 and nothing changes):

Command Rule as on main Rule + this guard
gh api repos/linuxfoundation/crowd.dev/pulls/4548/comments --paginate auto-approved (returns the PR's 21 inline comments) auto-approved, unchanged
gh api repos/<owner>/<repo> -X DELETE auto-approved blocked by the guard
gh api repos/<owner>/<repo> -f description=x auto-approved blocked by the guard
gh api repos/<owner>/<repo>/git/refs/heads/main -X DELETE auto-approved blocked by the guard

Two details worth knowing:

  • The POST case needs no -X: gh's own help says "adding request parameters will automatically switch the request method to POST", so any -f/-F ride-along turns an approved read into a write.
  • With a default gh auth login token (repo scope, no delete_repo), repository deletion itself would fail at the API. What the repo scope does cover through this rule: deleting branches via git/refs, editing repository settings via PATCH, and deleting comments and releases.

The deny list already blocks git reset --hard* and rm -rf*, so this brings remote state in line with the protection local state already has.

Changes

  • .claude/hooks/guard-gh-api.sh: blocks -X/--method and the parameter flags that imply POST (-f, -F, --field, --raw-field, --input) on gh api commands; exits 2 with an explanation. The flag scan runs on the raw hook input rather than an extracted command string, so a quote inside the command cannot hide a flag from the guard (unit-tested against a --jq '"' -X DELETE smuggle).
  • .claude/settings.json: wires the guard into PreToolUse with a Bash matcher, next to the existing Edit|Write|MultiEdit guard.

The review-pr skill's three documented gh api calls (comments, reviews, commits) were re-run under the guarded config and still auto-approve.

Type of change

  • Bug fix

JIRA ticket

n/a (external contribution; happy to reference one if a maintainer files it)

Notes for the reviewer

One deliberate trade-off: the guard blocks gh api writes outright rather than downgrading them to an approval prompt, matching the hard-deny style of your deny list. If you would rather keep human-approvable writes in interactive sessions, the exit-2 block can be swapped for a hook JSON permissionDecision: "ask"; I kept the measured, simpler contract. An alternative to all of this is deleting the Bash(gh api repos*) line and letting the three skill calls prompt each review session; the hook keeps the skill friction-free, which seemed closer to the intent of #4122.

Found while testing a static analysis tool for agent configuration files; every claim above was then verified by hand against Claude Code before this PR was opened.

The allow-list approves Bash(gh api repos*) so the review-pr skill can
fetch PR comments, reviews, and commits. A prefix rule cannot see the
HTTP method: measured on Claude Code 2.1.238, the rule as written
auto-approves `gh api repos/<owner>/<repo> -X DELETE`,
`... -f description=x` (gh switches GET to POST the moment a parameter
is added), and `... /git/refs/heads/main -X DELETE`. The deny list
already blocks `git reset --hard` and `rm -rf`, so remote state was
clearly not meant to be less protected than local state.

Adds a PreToolUse guard in the same shape as guard-protected-files.sh:
reads pass through untouched (the skill's three documented calls still
auto-approve; verified against a real PR), while -X/--method and the
parameter flags that imply POST exit 2 with an explanation. The flag
scan runs on the raw hook input rather than an extracted string, so a
quote inside the command cannot hide a flag from the guard.

Signed-off-by: Dennis Havermans <dennishvrmans@gmail.com>
@cursor

cursor Bot commented Sep 3, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Changes only Claude Code agent hooks and settings; no application runtime, auth, or data paths are affected.

Overview
The Bash(gh api repos*) permission auto-approves all gh api calls under that prefix, including DELETE/POST, because the allow list cannot distinguish HTTP method. This PR adds a PreToolUse guard on Bash commands so read-only gh api (e.g. for /review-pr) still passes while state-changing invocations are hard-blocked.

guard-gh-api.sh inspects the raw hook JSON for method/parameter flags (--method, --field, -X, -f/-F, etc.) that turn default GET into writes or POST, with exit code 2 and a clear stderr message. .claude/settings.json registers this hook next to the existing protected-files guard on the Bash matcher.

Reviewed by Cursor Bugbot for commit 4032ef2. Bugbot is set up for automated code reviews on this repo. Configure here.

@CLAassistant

CLAassistant commented Sep 3, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread .claude/hooks/guard-gh-api.sh
Bugbot is right: gh accepts -iX DELETE and a quoted "-X" DELETE as
readily as -X DELETE — measured, --verbose shows the DELETE request
line for each — and the space-anchored patterns saw neither.

Long flags are now matched as substrings, since the shell strips the
quotes but the raw hook input keeps them, and --method is --method
either way. Short flags are matched as clusters: any short group
containing X, f, or F blocks, anchored so a path segment like r-Xtra
stays innocent. Re-ran the battery: twelve write spellings block, the
review-pr reads pass, including one carrying eight harmless flags.

Signed-off-by: Dennis Havermans <dennishvrmans@gmail.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit f9f4653. Configure here.

Comment thread .claude/hooks/guard-gh-api.sh Outdated
pflag, the option parser gh is built on, documents three value
spellings for a short flag: separate (-f key=value), equals
(-f=key=value), and attached (-fkey=value), each also valid at the end
of a boolean cluster (-ifkey=value). The guard required a non-letter
after f/F, so the attached spellings passed as reads while gh made a
POST: measured with --verbose, -fkey=value, -Fkey=value, and
-ifkey=value each show the POST request line. gh even reads -friendly
as -f riendly and answers "invalid key: riendly".

The f/F pattern now matches any short cluster containing f or F with no
trailing constraint, the same shape as the -X pattern beside it. The
battery grows to 36 cases: 30 write spellings block, covering every
pflag-documented form of -X, -f, and -F, the long flags with and
without equals, quoted spellings, and clusters; 6 reads pass, including
the three commands the review-pr skill documents. Verified end to end
through Claude Code: the read executes, the attached-form write is
blocked by the hook.

Signed-off-by: Dennis Havermans <dennishvrmans@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants