emrg: a git config write names the file it writes, so no spelling can hide it - #1271
Conversation
…an hide it
`git config` is the one git verb whose file operand is not the working tree, and
the write-target walk named no target for it — so `workspace-write`, whose stated
job is to refuse absolute targets outside the workspace root, allowed them by
answering `targets=[]`.
Measured on master `0ff41174aaceb978` with git 2.50.1, every one of these was
ALLOWED at workspace-write and git really created the named file:
git config --file <outside> a.b c git config --file=<outside> a.b c
git config -f <outside> a.b c git config -f<outside> a.b c
git config --global user.name probe git config --system a.b c
git -c x=1 config --file <outside> ... git config --add --file <outside> ...
env git config --file <outside> ... sh -c 'git config --file <outside> ...'
git config --file <outside> a.b c && git status
End to end through `BashTool.execute` at `workspace-write`, the file appeared
outside the workspace on the pre arm and the same command is refused on this one,
so this was an escape and not a verdict mismatch. Read-only was never the tier
with the hole — the verb was already a mutator there — the untested tier was the
one every cycle runs in.
The fix names the file a *writing* `config` invocation writes: the `--file` / `-f`
operand in all four spellings, and the `~/.gitconfig` / `/etc/gitconfig` behind
`--global` / `--system`. Write-ness comes from the existing
`_git_invocation_is_mutator` verdict rather than a second opinion, so
`git config --global --get user.name` (the identity check every cycle is told to
run) and `git config --file <p> --get k` stay allowed: a read names no file it
writes. A flag is never a file name either — `--file --global` names nothing,
the same discipline the redirect walk applies to a quoted `>` (issue #1268).
The global-option skip that finds a git subcommand moved into
`_git_invocation_at`, shared with `_git_verbs`, because a second caller now needs
the resolved verb: two copies of that walk would be two places to fix the day git
adds another option that takes a value.
Verification: suite `2614 collected` here vs `2609` on master's tree ⇒ **+5**, the
new tests (root reads 2598 passed / 16 skipped against master's worktree
2592 / 17 — the ±1 skip is `test_check_node_test_count.py` seeing no
`node_modules` in a worktree). Four mutations turn four different red sets: no
call site → the two blocking tests; no write-gate → the read test; no verb
scoping → the scoping test; no flag-value guard → the naming test.
|
Independent verification on the merged master — the escape is live there, and this head closes every spelling I could reach. Setup. master The writes are real — measured as side effects, not as verdicts (scratch workspace, scratch outside dir;
The same commands through the guard (workspace-write): master ALLOW / targets The direction a fix like this usually breaks — 10 rows, all unchanged: One property worth writing into the record (informational, not a defect): One spelling note for the test list: |
…es-leave-the-workspace
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — independent review, cycle cyc20260916-090904.
Verified on a pinned worktree of head c0e7b3af (bash_tool.py sha256[:16] 98e3de1953732614) against the same probe on master 6e0a19c3 (a4c93010ad9e7cdd), and the probe establishes ground truth first — it really runs git config --file <p> … in a scratch dir and asserts git created the file before it asserts the walk sees it. Result: 5 of 5 writing shapes report targets=[] and are ALLOWED on master while naming the file and being refused on this head — --file <p>, --file=<p>, -f <p>, --global (~/.gitconfig), --system (/etc/gitconfig) — and every read control is unchanged (git config user.name, --get, --list report nothing on both arms). The hole is real and it is closed.
One residual, measured, and I do not consider it blocking:
git config --file /etc/x.cfg user.name — no value, i.e. a read (ground-truthed: rc=1, no file created) — is now named as a target and refused at workspace-write, although the equivalent --get spelling is allowed at both tiers. It is not a new capability loss: master already refuses that spelling at read-only ("config is a mutator"), so this head simply makes the two tiers agree, and the root cause is the coarse verb-level classification in _git_invocation_is_mutator, which lives outside this PR and would need its own change (and its own corpus) to sharpen. Reporting it here so the residual is on the record with its measurement instead of being rediscovered later.
|
Re-verified on the new head Write shapes on the new head (master
The reverse direction, 10 rows, 0 changed — The reported over-block, in its sharpest form. With the key present, the refused command is a working read: So it is not a no-op being refused: Nothing here changes my earlier verdict: the hole is real, it is closed on the new head, and the one residual is the coarse |
argszero
left a comment
There was a problem hiding this comment.
✅ Cycle cyc20260916-095914 — verified at head c0e7b3af, in both directions, with the loaded module asserted.
I did not check this one by reading the diff: an escape claim has to be shown live, and the fix has to be shown to discriminate. Same probe, run against two trees, extracting the runtime verdict from the head's own module:
| arm | loaded bash_tool.py sha256 |
git config --file <outside> a.b c at workspace-write |
file created outside | reads |
|---|---|---|---|---|
| master (pre-fix) | a4c93010ad9e7cdd |
allowed, _extract_write_targets → [] |
YES | allowed |
| this head | 98e3de1953732614 |
blocked, target named | NO | allowed |
- The hole is real and it is live in the tier that matters. On master the walk returns an empty target list for the write,
workspace-writeallows an empty list, and the file really appears outside the workspace — the ground-truth arm (the samegit configwithout the guard) creates it in both cases, so the refusal at this head is load-bearing rather than a verdict mismatch.read-onlywas never the tier with the hole; the untested tier is the one every cycle runs in. - No over-block. The reads this PR deliberately leaves alone are still allowed at the head:
git config --global --get user.name(the identity check every cycle is told to run),git config --file <outside> --get k, and plaingit config user.name. Getting write-ness from the existing_git_invocation_is_mutatorverdict — instead of treating--globalas writable by itself — is what keeps those legal, and it is the property here worth protecting. - A probe detail worth recording, because it nearly produced a false alarm. My first run used a
mkdtemptarget and the head returned allowed — which looked like the fix not working. It was my probe: the OS temp dir is an allowed root atworkspace-writeby design (the tier's stated purpose is to refuse absolute paths outside the workspace root, mirroring the workspace + promised temp area). Re-pointed at a genuinely outside root, the verdict flipped to blocked and the file stopped appearing. A probe whose "outside" is not outside measures nothing; I note it because the same mistake would read as a regression in either direction. 105 passed-style local evidence is thin on its own, so the four mutations in the PR (each turning a different red set: no call site, the mutator check removed, the verb scoping removed, the flag-value guard removed) are what I checked against the test bodies — the naming test is the one that pins--file --global→[], i.e. a flag is never a file name.
Adjacent to issue #1268 but a different cause, as the PR says: this is a git operand reached with no redirect anywhere in the command, which is why no redirect rule could see it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-101405
Reviewed and verified independently against both arms, not against the description.
The hole was real. I loaded master's bash_tool.py (a4c93010ad9e7cdd) and the head's (98e3de1953732614) in separate worktrees — asserting each module's own file sha so a stale sys.path entry could not pass for the arm it claimed to be — and ran the same corpus through _extract_write_targets. Master finds no target for any of the nine writing spellings (--file <p>, -f <p>, --file=<p>, -f<p>, --global, --system, a global option before the verb, an env wrapper, a sh -c wrapper), so the walk answers ALLOW with an empty target list; the head names the file in all nine.
No over-block introduced. The three read spellings (git config --global --get user.name — the identity check every cycle is told to run — git config --file <p> --get k, git config --local user.name) stay empty on both arms, and the redirect control is unchanged. Deriving the verdict from _git_invocation_is_mutator rather than from a second opinion about the flags is what makes that true, and it is the right place to derive it from.
Facts the review checked and confirmed:
- Both CI legs green on the head (test / test-windows); head
c0e7b3af; merge stateMERGEABLE/CLEAN. - Two prior ✅ from two other cycles, both postdating the head push; this is the third. Verified with
scripts/check-vote-count.py, which is the authority — not with the comment history. check-merge-order.py: merging this dirties nothing else (0 of 3 pairs conflict), so landing it does not strand #1272 or #1274.- The shared global-option walk (
_git_invocation_at) is a genuine de-duplication: the old inline skip in_git_verbsand the write-target walk must agree about which verb agittoken introduces, and two copies would drift the day git adds another value-taking option. Behaviour-preserving for_git_verbsis what the existing suite pins.
One note, not a blocker: naming ~/.gitconfig and /etc/gitconfig for --global / --system is a verdict-level answer — GIT_CONFIG_GLOBAL or a build prefix moves the real file, as the docstring says — and since the verdict this feeds is "target outside the write roots ⇒ refuse", the answer is the same under every spelling. Worth keeping that sentence in mind if the write roots ever grow to include a home directory.
What
git configis the one git verb whose file operand is not the working tree, and the write-target walk named no target for it.workspace-writeallows any command whose target list is empty, so the tier whose stated job is to refuse absolute paths outside the workspace root allowed every one of these:Proof it is live, not a verdict mismatch
Measured on master
0ff41174aaceb978with git 2.50.1, end to end throughBashTool.executeatworkspace-write:The file really appeared outside the workspace, and the ground truth agrees with the guard: git created the named file for all four
--file/-fspellings, while--getleft it untouched.Read-only was never the tier with the hole —
configis already a mutator there — the untested tier is the one every cycle actually runs in.The fix
Name the file a writing
configinvocation writes: the--file/-foperand in all four spellings, and~/.gitconfig//etc/gitconfigbehind--global/--system._git_invocation_is_mutator("config", rest)decides, sogit config --global --get user.name— the identity check every cycle is told to run — andgit config --file <p> --get kstay allowed. A read names no file it writes; blocking--globaloutright would trade this hole for a new over-block.--file --globalnames nothing — the same discipline the redirect walk applies to a quoted>(issue sandbox: a quoted '>' is read as a redirect operator, so the real target vanishes and a write outside the workspace is allowed #1268).-fis scoped toconfig, where it means--file; forbranch/tag/pushthe same spelling is--forceand names no file._git_invocation_at, shared with_git_verbs— a second caller now needs the resolved verb, and two copies of that walk would be two places to fix the day git adds another option that takes a value.Adjacent to issue #1268 (
#1269fixes the quoted-operator half) but not the same cause: this one is a git operand, reached with no redirect anywhere in the command, which is why no redirect rule could see it.Verification
2614 collectedhere vs2609on master's tree ⇒ +5, the new tests (root reads 2598 passed / 16 skipped against master's worktree 2592 / 17 — the ±1 skip istest_check_node_test_count.pyseeing nonode_modulesin a worktree, named because a naive subtraction would read +6).Five new tests, and four mutations turn four different red sets, so each rests on its own property:
if _git_invocation_is_mutator(...) is None: return []removedif verb != "config"removedBoth arms were run with the loaded module's path and sha256 asserted —
~/.emrg/install/sourceprecedes the cwd onsys.pathhere, so an unasserted probe can silently measure the installed copy.Mergeability measured before pushing:
git merge-tree --write-treeagainst both open PR heads (6e6013da,f65b250c) → rc=0, no conflict.