Conversation
…Through the Owner Check (#1583) Closes #1561. Closes #1562. ## The defects `origin_owner()` resolves the owner a write may target by running `git` against the script's own directory, and `in_scope()` compares the target against it. Two holes, both reached through the invoking environment rather than through anything in the tree. **The probe answered for the wrong repository (#1561).** The call inherited the environment unchanged, and git honours `GIT_DIR` over the `-C` argument for repository discovery, so an inherited one made the probe answer for whatever repository it named. `GIT_COMMON_DIR` does the same, since the config is read from the common dir. Both directions are reachable, and the one worth fixing is the admitting one: `GIT_DIR` naming a repository under the target's owner while the script itself sits in a checkout under a different one, so `in_scope()` returns true on a comparison that never described the script's checkout. **One write path never reached the check (#1562).** `wait` requests a Copilot review before its first poll, and that request is a `requestReviews` mutation. `in_scope()` had exactly two call sites, in `comment_on_pr` and `reply_to_thread`, and the request path had none, so `wait` on a repository under another owner issued a state-changing call under the logged-in identity while the refusal that stops the same target on `reply` never fired. ## What this changes **The probe runs on a stripped environment.** `git remote get-url origin` now runs without the four discovery names, and without any name starting with `GIT_CONFIG`. The second category is a prefix test rather than a list, because `GIT_CONFIG_KEY_n` has no fixed upper bound, so no enumeration of literal names can be complete. **`wait` refuses a cross-owner target outright**, exit `64`, ahead of its first read rather than at the request, so a refused run neither writes nor reads. All three write paths now state one rule, which is what makes `scripts/README.md`'s "the owner check is also enforced in-process" true without qualification. ## What this deliberately does not do The strip closes the channels an inherited tooling environment sets. It does not close an inherited `HOME`, `XDG_CONFIG_HOME`, or `PATH`, and it cannot: those deliver the same `insteadOf` injection, and stripping them would either break `safe.directory` or make the probe ignore a global config a legitimate user really has. A caller controlling any of the three already controls the process that would perform the write, so there is nothing left for this check to defend. The comment above the strip says so rather than leaving a reader to infer completeness from a name list. ## A design that was tried and reverted An intermediate version probed with `git config --local --get remote.origin.url`, which does close the `insteadOf` vector. It was reverted, because a review pass proved three defects a reviewer is likely to rediscover: - **It inverted the boundary.** A `[remote "origin"]` may carry more than one `url`, which is ordinary documented config. `remote get-url` returns the first, git's own fetch identity; `config --get` returns the last. So the scope anchor moved to the second owner, and a write to that owner was admitted where it had been refused. - `--local` does not follow `include.path`, so a checkout configuring its origin through an include read as no owner and every write refused. - A URL-shorthand alias origin yielded a raw value with no owner to parse, refusing the same way. Two cases pin the first two shapes against a return to that command. ## Correcting the issue text #1561 motivates the fix by saying a git hook, `git bisect run`, and `git rebase --exec` each export `GIT_DIR`. Measured against this host's git, none of the three does: a `pre-commit` hook receives `GIT_AUTHOR_*`, `GIT_EDITOR`, `GIT_EXEC_PATH`, `GIT_INDEX_FILE` and `GIT_PREFIX` and no `GIT_DIR`, and `post-commit`, `post-checkout`, `bisect run` and `rebase --exec` all leave it unset. The fix stands on its own, since an inherited `GIT_DIR` genuinely overrides `-C` whatever sets it, but the provenance was deleted rather than narrowed so a reader who re-derives it does not conclude the guard is unnecessary. ## Verification Every behavioral claim was proved against a reverted fix rather than trusted. Reverting `env=` fails five of the eight environment cases; reverting the `wait` guard fails both refusal cases at the first GraphQL call. One case is documented as not being a regression guard for any reverted state here, because `remote get-url` ignores the injection it covers. Beyond the new cases, the probe was driven through the real `origin_owner()` under each of `GIT_DIR`, `GIT_COMMON_DIR`, `GIT_CONFIG_COUNT` (as an `insteadOf` rewrite and as a direct override), `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_SYSTEM`, `GIT_CONFIG_PARAMETERS` and `GIT_CONFIG`, answering the anchor owner every time, and through the multi-url and `include.path` shapes that broke the reverted design. A test-setup defect of the same class was fixed alongside: the helper building the throwaway repositories shelled out to `git init` and `git remote add` on the inherited environment, so under an ambient `GIT_DIR` the cases errored and the `remote add` wrote an `origin` into whatever repository that variable named. One further test pins the command table to the parser's own `cmd` choices. `wait` became a write by acquiring a mutation rather than by being added, so a table keyed to "a new subcommand appeared" would not have caught it. ## Known and not addressed here #1582, `in_scope()`'s no-owner refusal gives one remedy for several unrelated probe failures. `wait` refusing on the same terms is what makes it matter, since an absent git or an inherited `GIT_CEILING_DIRECTORIES` now ends a read-only poll with advice that does not fit the cause. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Write commands now reject targets belonging to a different repository owner before contacting GitHub. - The `wait` command returns exit code 64 when the target is out of scope. - **Documentation** - Updated command-line documentation to explain scope checks and refusal behavior for `wait`, `comment`, and `reply`. - Clarified that owner validation occurs before review requests, polling, or other GitHub operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The scope probe hardening and wait scope enforcement are consistent with the stated boundary policy and are backed by focused regression tests and updated documentation.
Pull request overview
This promotion brings the develop fix into main to harden scripts/pr_review.py's in-process owner boundary against inherited git environment variables, and to ensure wait enforces the same scope refusal as other write-capable subcommands before it performs any GitHub I/O.
Changes:
- Strip
GIT_DIR,GIT_COMMON_DIR, related discovery variables, and anyGIT_CONFIG*variables from the environment used byorigin_owner()so the scope probe cannot be redirected by inherited git environment configuration. - Add an early
in_scope()gate forwait, returning exit code64before any GraphQL call or auto-request mutation when the target owner is out of scope. - Add targeted unit tests for both the environment-stripping behavior and the command-level invariant that all write-capable commands refuse cross-owner targets before touching the network; update docs accordingly.
File summaries
| File | Description |
|---|---|
| scripts/pr_review.py | Strips git discovery/config env for the owner probe and enforces scope refusal for wait before any GitHub reads/writes. |
| scripts/tests/test_pr_review.py | Adds regression tests for inherited env redirection and for ensuring all write commands refuse cross-owner before transport use. |
| scripts/README.md | Updates wait documentation to reflect the expanded mutation set and the new 64 out-of-scope behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Promotes one commit from
developtomain.7e39cb8Keep the Scope Probe Out of an Inherited Environment, and Route wait Through the Owner Check (Keep the Scope Probe Out of an Inherited Environment, and Route wait Through the Owner Check #1583)origin_owner()resolves the owner a write may target, andin_scope()refuses a write to any other owner. Two holes, both reached through the invoking environment rather than through anything in the tree.The probe inherited its environment unchanged, and git honours
GIT_DIRover the-Cargument for repository discovery, so an inherited one made it answer for whatever repository that variable named.GIT_COMMON_DIRdoes the same, the config being read from the common dir. The admitting direction is the one worth fixing: either variable naming a repository under the target's owner while the script itself sits in a checkout under a different one, soin_scope()returns true on a comparison that never described the script's checkout. Separately,waitrequested a Copilot review before its first poll, and that request is arequestReviewsmutation that never passed through the owner check at all, so a cross-ownerwaitissued a state-changing call under the logged-in identity while the refusal that stops the same target onreplynever fired.The probe now runs without the four discovery names and without any name starting with
GIT_CONFIG, the second as a prefix test rather than a list, sinceGIT_CONFIG_KEY_nhas no fixed upper bound and no enumeration of literal names can be complete.waitrefuses a cross-owner target outright, exit64, ahead of its first read rather than at the request, so all three write paths state one rule.What the strip does not close is stated in the code rather than left to inference: an inherited
HOME,XDG_CONFIG_HOMEorPATHdelivers the same rewrite, and a caller controlling any of those already controls the process that would perform the write.An intermediate design probing with
git config --local --get remote.origin.urlwas reverted, because it inverted the boundary on an origin carrying more than oneurl, whereremote get-urltakes the first andconfig --getthe last, and because--localfollows noinclude.path. Two cases pin those shapes against a return to that command.Copilot reviewed the feature pull request at its merged head, full coverage, approval recommended, no findings and none suppressed. CodeRabbit reviewed the round before it and raised one finding, fixed and resolved on the feature pull request before it merged, then reported itself rate limited on the final head.
Closes #1561
Closes #1562