scripts/pr_review.py's scope probe resolves the owner a write may target by running git against the script's own directory, and it passes the invoking environment through unchanged. Git honours GIT_DIR over the -C argument for repository discovery, so an inherited GIT_DIR makes the probe answer for whatever repository that variable names rather than for the directory the script sits in.
The behavior
At main (6e859734ddd159a4718a420e5762ae1db73722e2) the probe takes no env:
# scripts/pr_review.py, origin_owner()
url = subprocess.run(
["git", "-C", str(HERE), "remote", "get-url", "origin"],
capture_output=True,git -C <dir> changes the working directory before discovery, and discovery then reads GIT_DIR first, so the -C argument decides nothing once that variable is set. Constructed reproduction, with two repositories that exist only for the check:
git init /tmp/probe/anchor && git -C /tmp/probe/anchor remote add origin https://github.com/acme/anchor.git
git init /tmp/probe/elsewhere && git -C /tmp/probe/elsewhere remote add origin https://github.com/globex/elsewhere.git
git -C /tmp/probe/anchor remote get-url origin
# https://github.com/acme/anchor.git
GIT_DIR=/tmp/probe/elsewhere/.git git -C /tmp/probe/anchor remote get-url origin
# https://github.com/globex/elsewhere.git
The second command reads globex while standing in a checkout under acme, which is exactly the substitution origin_owner() cannot see.
Why it matters
in_scope() is the in-process half of the fleet's write boundary for comment and reply, and it compares the target owner against that probe. Under an inherited GIT_DIR the owner it compares against is not the owner of the tree the script sits in, and the substitution runs in both directions:
- Refusing a legitimate write.
GIT_DIR names a repository under another owner, and a correct same-owner write is refused. Fail-safe, and it costs a round rather than a write.
- Admitting a write the boundary exists to stop.
GIT_DIR names a repository under the target's owner while the script itself sits in a checkout under a different one. in_scope() returns true on an owner comparison that never described the script's checkout, and the write proceeds.
The second is the direction worth fixing. It does not make the helper unsafe today, since nothing in this repository invokes pr_review.py from a context that exports GIT_DIR, but a git hook, git bisect run, and git rebase --exec each set it for everything they call, and the fleet already runs hooks.
Suggested fix
Pass an environment with the discovery variables removed rather than inheriting them, so the -C argument is the only thing that decides which repository answers:
env = {k: v for k, v in os.environ.items() if k not in ("GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY")}Worth deciding at the same time whether a probe that finds no repository should stay a refusal with no owner, which it does today and which is correct.
Scope
Found while fixing #1557, which was message-only by its own statement, so the code change was deliberately left out of that pull request. The refusal messages there now describe the probe rather than claiming which repository encloses the directory, so they stay true under this leak, and that wording is a mitigation of the reading rather than a fix for the probe.
scripts/pr_review.py's scope probe resolves the owner a write may target by runninggitagainst the script's own directory, and it passes the invoking environment through unchanged. Git honoursGIT_DIRover the-Cargument for repository discovery, so an inheritedGIT_DIRmakes the probe answer for whatever repository that variable names rather than for the directory the script sits in.The behavior
At
main(6e859734ddd159a4718a420e5762ae1db73722e2) the probe takes noenv:git -C <dir>changes the working directory before discovery, and discovery then readsGIT_DIRfirst, so the-Cargument decides nothing once that variable is set. Constructed reproduction, with two repositories that exist only for the check:The second command reads
globexwhile standing in a checkout underacme, which is exactly the substitutionorigin_owner()cannot see.Why it matters
in_scope()is the in-process half of the fleet's write boundary forcommentandreply, and it compares the target owner against that probe. Under an inheritedGIT_DIRthe owner it compares against is not the owner of the tree the script sits in, and the substitution runs in both directions:GIT_DIRnames a repository under another owner, and a correct same-owner write is refused. Fail-safe, and it costs a round rather than a write.GIT_DIRnames a repository under the target's owner while the script itself sits in a checkout under a different one.in_scope()returns true on an owner comparison that never described the script's checkout, and the write proceeds.The second is the direction worth fixing. It does not make the helper unsafe today, since nothing in this repository invokes
pr_review.pyfrom a context that exportsGIT_DIR, but a git hook,git bisect run, andgit rebase --execeach set it for everything they call, and the fleet already runs hooks.Suggested fix
Pass an environment with the discovery variables removed rather than inheriting them, so the
-Cargument is the only thing that decides which repository answers:Worth deciding at the same time whether a probe that finds no repository should stay a refusal with no owner, which it does today and which is correct.
Scope
Found while fixing #1557, which was message-only by its own statement, so the code change was deliberately left out of that pull request. The refusal messages there now describe the probe rather than claiming which repository encloses the directory, so they stay true under this leak, and that wording is a mitigation of the reading rather than a fix for the probe.