Repository navigation
fix(gh-wrapper): close GH_REPO, PR URL and GraphQL draft bypasses - #443
Merged
Merged
Conversation
added 2 commits
October 19, 2026 18:28
Two of the three holes in #391. Both let an agent publish a non-draft PR on an off-org repo through the wrapper. Item 1: _gh_wrapper_resolve_owner ignored GH_REPO and a PR URL argument, so `GH_REPO=o/r gh pr ready 5`, `gh pr ready https://github.com/o/r/pull/5` and `GH_REPO=o/r gh pr create` all missed the off-org checks. The resolver now follows gh's own order: a PR or issue URL argument, then -R, then GH_REPO, then the checkout remote. Measured with read-only `gh pr view` calls on gh 2.102.0. URLs are read only for selector-taking subcommands; for create, new, list and status a URL is a flag value and must not beat -R. A new _gh_wrapper_spec_owner also reads HOST/OWNER/REPO, which gh accepts in -R and GH_REPO; before, `-R github.com/o/r` resolved to "github.com". Item 2: GraphQL introspection shows three mutations that open or promote a PR: createPullRequest and revertPullRequest (both open a PR, draft optional) and markPullRequestReadyForReview. updatePullRequest has no draft field. All three name their target by node ID, so the wrapper cannot prove the owner is in-org. They are refused on every `gh api` call to a GraphQL endpoint, from inline fields, -F @file, or an --input file. A query the wrapper cannot read (stdin, a missing file) or an --input file with \u escapes is refused too. No fleet script uses these mutations. Item 3 (claude-config hook chains) is not touched. Advances #391
Both commit reviewers flagged the URL scan in _gh_wrapper_resolve_owner: it took any URL argument, so `gh pr comment 5 -R o/r --body <URL>` resolved to the URL's owner and could pick the wrong identity, and it lowercased the owner although the function documents raw case. A URL that follows a value flag is now that flag's text and is skipped. Self-contained flags (--x=v, -Rvalue) and a short list of boolean flags do not hide a URL. pr ready has no value flag, so there every URL still counts, which keeps the draft refusal fail-closed. The owner is cut from the original argument, so its case is kept. Advances #391
This was referenced Oct 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Handles items 1 and 2 of #391. Item 3 lives in claude-config and is not touched here.
Item 1: GH_REPO and PR URLs
_gh_wrapper_resolve_ownerignoredGH_REPOand a PR URL argument, so these all missed the off-org checks:GH_REPO=o/r gh pr ready 5gh pr ready https://github.com/o/r/pull/5GH_REPO=o/r gh pr create(not forced to draft)The resolver now follows gh's own order: a PR or issue URL argument, then
-R, thenGH_REPO, then the checkout remote. I measured that order with read-onlygh pr viewandgh apicalls on gh 2.102.0 (-RbeatGH_REPO, a URL beat both, andGH_REPOfilled{owner}/{repo}forgh api).pr readyandpr createwere not run, since both write. They share gh's PR selector and repo override, andgh pr ready --helplists[<number> | <url> | <branch>].Details:
-R. A URL right after a value flag (--body URL) is skipped too, except onpr ready, which has no value flags._gh_wrapper_spec_ownerreadsHOST/OWNER/REPO, which gh accepts in-RandGH_REPO. Before,-R github.com/o/rresolved togithub.com.Item 2: GraphQL
Schema introspection (
__type(name:"Mutation")plus each input type) shows three mutations that open or promote a PR:createPullRequest: opens a PR;draftis optional.revertPullRequest: opens a revert PR;draftis optional.markPullRequestReadyForReview: promotes a draft.updatePullRequesthas no draft field.convertPullRequestToDraftgoes the safe way and is left alone.All three name their target by node ID, so the wrapper cannot prove that the owner is in-org. It refuses them on any
gh apicall to a GraphQL endpoint (graphql,/graphql, or a full URL), whether the query comes inline, from-F query=@file, or from an--inputfile. The guard is fail-closed:--inputfile containing\uescapes is refused, because an escape can hide a mutation name.A grep of the fleet found no script that uses these mutations.
Behavior change:
gh api graphql --input -and-F query=@-are now refused through the wrapper even for read-only queries.Tests
bash/tests/test-gh-wrapper-off-org-guards.shgains GH_REPO, URL, precedence, GraphQL and resolver cases. Against the origin/main wrapper, 54 cases fail and 63 pass. With this change, all 117 pass. Alltest-gh-wrapper-*.shfiles pass, andshellcheck -S infois clean.Advances #391