fix(guardrails): allow read-only git inside PowerShell grouping - #5548
Conversation
…Shell read-only test ps::git_command_is_readonly listed remote and stash as mutating stems, so a read-only loop running git remote -v or git stash list was refused. Cut the read-only forms (bare remote, remote -v, remote show, remote get-url, stash list, stash show) before the stem tests; every other remote or stash form, and any mutating git form elsewhere in the command, still blocks. Refs #4235 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Shell grouping
A `{}`/`()` group sends a PowerShell command to the fail-closed sink, so
`foreach ($d in ...) { git -C $p status; git -C $p log }` was refused. Add the
`interrogation-ok` sink scope to ps::classify_git_command: it relieves only the
special-construct trigger, and only when every git invocation is a built-in
interrogator on the new ps::git_command_is_interrogation_only allowlist.
The allowlist reads the argv, so an obscured or computed subcommand, an alias,
`-c`, `--exec-path`, `fetch`, `grep` and any mutating verb keep the command
blocked; dynamic invocation, launcher and here-string triggers are unchanged.
The blocklist behind `readonly-ok` (block-no-verify) is not used here because
its residuals would open `git ('re'+'set') --hard` inside a group.
Refs #4235
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…or a commit and names the PowerShell rewrite first Refs #4235 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…git-grouping Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uping fix Refs #4235 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d72cd409a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude finished @kyle-sexton's task in 9m 50s —— View job Security review complete
No security issues found. This PR touches The core change ( This is a loosening of a hook's block condition guarding irreversible git actions ( Other angles checked:
|
|
PR body contract — issue linkage This PR body conforms to the issue-linkage contract. Nothing to do. |
…only git group check
The interrogation-only test refused `$env:GIT_*` only, so `${env:GIT_EXTERNAL_DIFF}=...; & { git diff --ext-diff }` was allowed. It now refuses `env:GIT_`, `env:\GIT_` and `${env:GIT_` spellings and `SetEnvironmentVariable`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 45s —— View job Security review complete
1 CRITICAL finding, posted inline on The Set-Item -Path ('Env:' + 'GIT_PAGER') -Value 'C:\evil.exe'; & { git log }
Everything else checked out:
|
|
Claude finished @kyle-sexton's task in 4m 58s —— View job Code review (correctness & maintainability)
No correctness or maintainability findings. This lane is scoped to correctness/maintainability; a security lane already ran a What I checked for correctness:
Author-claimed verification (changelog parity checks, |
…git-grouping Renumber guardrails to 0.42.2 above main's 0.42.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 7s —— View job Security review complete
1 CRITICAL finding (unresolved carry-forward) — posted inline on
Set-Item -Path ('Env:' + 'GIT_PAGER') -Value 'C:\evil.exe'; & { git log }No contiguous This is the identical gap a prior review on this PR already flagged as CRITICAL at commit Instruction-surface lens: this PR loosens Other angles checked, no new issues:
|
|
Claude finished @kyle-sexton's task in 2m 5s —— View job Code review (correctness & maintainability) —
|
… check
The GIT_* guard was a contiguous-text match, so `Set-Item -Path ('Env:' + 'GIT_PAGER') ...; & { git log }` passed. The interrogation-only test now refuses any `git_` text, an `env:` that is not a `$env:NAME` read, an assignment to `$env:NAME`, `Environment::` and `SetEnvironmentVariable`. Merges main and renumbers to 0.42.3.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…git-grouping Renumber guardrails to 0.42.4 above main's 0.42.3. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 24s —— View job Code review (correctness & maintainability) —
|
|
Claude finished @kyle-sexton's task in 4m 5s —— View job Security review complete
1 CRITICAL finding, posted inline on Commit $m = [Environment].GetMethod(('Set'+'EnvironmentVariable'), [Type[]]@([string],[string]))
$m.Invoke($null, [object[]]@(('GI'+'T_PAGER'), 'C:\evil.exe'))
& { git log }None of A substring/regex approach can't close this class by construction — each literal spelling added closes only that spelling. The fix needs the same argv-aware treatment the git-token walk in this same function already required for the word Other angles checked, no new issues:
|
…git group check Extends the environment-write refusal to a `[Environment]` type literal and to `.Invoke(` and `GetMethod` reflection, and lists the environment-write refusal in the README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…git-grouping Renumber guardrails to 0.42.6 above main's 0.42.5. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…git-grouping Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…by syntax The owner chose to parse an Env: write and fail closed when its target is not a literal, instead of matching text. ps::has_unprovable_env_write replaces the seven substring checks in ps::git_command_is_interrogation_only. It refuses a provider cmdlet (Set-/New-/Add-/Copy-/Move-/Rename- Item, Content, ItemProperty) whose operands are not plain literals, an Env:/Function:/Alias: drive path, a $env:NAME assignment in any target-list form, a static or method call, a call through a computed target, a word spliced from quoted parts, ForEach-Object -MemberName, and a function, filter or alias definition. Regression rows cover a split Env: token, a path held in a variable, splat and pipeline paths, InvokeMember, and the variants found while probing. The fallback trigger line now leads with the PowerShell rewrite. Refs #4235 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…git-grouping Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 8s —— View job Code review (correctness & maintainability) —
|
|
Claude finished @kyle-sexton's task in 5m 36s —— View job Security review complete
1 CRITICAL finding (new bypass in the rewritten guard), posted inline on
But the $d = 'Env' + ':'
Set-Location -Path $d
Set-Item GIT_PAGER 'C:\evil.exe'
& { git log }evades every check in I was not able to execute the shell test suite in this sandbox (command execution requires approval this session doesn't grant, consistent with every prior review pass on this branch), so this is a static trace of the tokenizer logic, not a confirmed dynamic run — the full trace supporting each step is in the inline comment. Other angles checked, no new issues:
|
A relative write resolves against the current location, so Set-Location -Path $d followed by Set-Item GIT_PAGER x reached the Env: drive with no literal drive name anywhere. The location check looked only at the token after the command word, where a named parameter sits. Location changes (Set-Location, cd, Push-Location and the aliases) now go through the same operand walk as a provider write, and a path piped in counts as not plain. mkdir, a wrapper over New-Item, joins the write cmdlets. Refs #4235 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Refs #4235 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 4s —— View job Security review complete
No security issues found — the prior open CRITICAL finding is fixed. What changed since the last reviewed HEAD ( The fix. The previously-flagged bypass was that I traced this against the exact PoC from the prior round ( Additional adversarial checks I ran on the rewritten helper, no new gap found:
Not independently executed: Other angles re-checked, no new issues:
This closes the open CRITICAL thread from the earlier rounds on this branch — I have no further findings blocking this PR on security grounds. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 557b7e6b09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ty-exempt The shell-portability gate read the `\\S` in a Windows module-qualified command string as a GNU word-boundary construct. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…git-grouping Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 17s —— View job Security review complete
1 CRITICAL finding (unresolved carry-forward, new technique), posted inline on
& { env GIT_EXTERNAL_DIFF=C:\tools\mutator.exe git diff --ext-diff }I traced this line-by-line (full walkthrough in the inline comment): This was independently flagged by Codex as a P2 on commit Other angles re-checked, no new issues:
Not independently executed: command execution in this sandbox requires approval this session doesn't grant (consistent with every prior review round on this branch) — this is a static trace of the tokenizer logic against the exact PoC text, not a confirmed dynamic run through real |
|
Claude finished @kyle-sexton's task in 1m 22s —— View job Code review (correctness & maintainability) —
|
…roup `env GIT_EXTERNAL_DIFF=... git diff` sets the variable for the command it runs, so it is an environment write spelled as program arguments. The write check now refuses an `env` command word, by name or by path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude encountered an error after 5m 16s —— View job Security review in progress
|
|
Claude finished @kyle-sexton's task in 2m 35s —— View job Code review (correctness & maintainability) —
|
…eader Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 42s —— View job Code review (correctness & maintainability) —
|
|
Claude finished @kyle-sexton's task in 2m 12s —— View job Security review complete
No security issues found. What changed since the last reviewed HEAD ( The fix. This closes the CRITICAL gap the prior review round flagged and Codex independently flagged as P2 on I traced the fix against the exact PoC from that finding and against bypass attempts of my own:
I did not find a bypass of this fix, and no other file changed in this range that would reopen it. Other angles re-checked, no new issues:
Not independently executed: command execution in this sandbox requires approval this session doesn't grant (consistent with every prior review round on this branch) — this is a static trace of the tokenizer logic against the exact test/PoC text, not a confirmed dynamic run through real This closes the last open CRITICAL thread on this branch — I have no further findings blocking this PR on security grounds. |

Refs: #4235
Summary
A PowerShell command with
{}/()grouping was refused byblock-dangerous-giteven when every git call in it was read-only, soforeach ($d in ...) { git -C $d status; git -C $d log }failed. Guardrails goes to 0.44.1.The owner decided on #4235 (2026-09-30) how the environment-write guard in that relief must work, Option A: "Parse, do not substring-match: block any
Env:write (Set-Item/New-Item/${env:...}/[Environment]::SetEnvironmentVariable) whose target name is not a literal string, fail closed. Add regression cases for a concatenated and a computedEnv:path." This PR implements that decision. The Claude security lane's CRITICAL thread on the earlier substring guard (PRRT_kwDOTCGFQM6nlUph) is still open, and a newer one on the location-change arm (PRRT_kwDOTCGFQM6nqcu1) is addressed by2b63cb907; both stay for the lane to re-judge. The PR is a draft until then.Issue #4235 has two acceptance criteria beyond the relief:
block-no-verifynarrow-token criterion is met on main by fix(guardrails): PowerShell fail-closed sink allow token clears mutating block (#4252) #4833 (block-no-verify.shhonorsps-unparsable-special-constructfromblock_dangerous_git_allow).block-hook-bypassps-computed-positionalcriterion is not implemented. The owner excluded it in the PR fix(guardrails): PowerShell read-only git grouping + ps-computed-positional (#4235) #4817 close comment, so the PR usesRefs, notCloses. The owner decides whether to strike that criterion and close the issue.Fix
interrogation-oksink scope inps::classify_git_command. It relieves only the special-construct trigger, and only when every git invocation is on theps::git_command_is_interrogation_onlyallowlist (status,log,show,diff,rev-parse,ls-files,remote -v,stash listand similar), optionally behind-C <path>.-c,--exec-path, computed or obscured subcommands, aliases,fetch,grepand mutating verbs still block. Dynamic-invocation, launcher and here-string triggers are unchanged.ps::has_unprovable_env_writereplaces the seven substring checks for the environment-write guard. It reads syntax and refuses whatever it cannot prove has a plain literal target: a provider cmdlet (Set-/New-/Add-/Copy-/Move-/Rename-Item,Content,ItemProperty, their aliases,mkdir) whose operands are not plain words or that has no operand; anEnv:/Function:/Alias:drive path; a$env:NAMEassignment, alone, in a target list or as a foreach variable; a static::call; any method call;ForEach-Object -MemberName; a computed call target; a word spliced from quoted parts (Set'-'Item,E'nv':X); a function, filter or alias definition;Add-Type,New-PSDrive,Invoke-Command; a$( )inside an expandable string; and a write beside aSet-Location/cdwhose target is computed or piped in.ps::git_command_is_readonlyjudgesremoteandstashby arguments instead of listing them as mutating stems.Trade-off the owner may want to weigh: the scan refuses ordinary idioms that contain a method call, a static call or
"$($x.Name)"inside a group with git, because it cannot prove them harmless. An operator unrolls the loop into flat statements, computes the value into a variable first, or setsps-unparsable-special-construct. It does not read code in a file, or a script block held in a variable and run by a cmdlet that accepts one. The broader alternative (relieve only commands whose every command word is on an allowlist) stays available and is the owner's call.Verification
$n = "GI"+"T_PAGER"; New-Item -Path ("E"+"nv:") -Name $n ...; & { git log },$p = "E"+"nv:\GI"+"T_PAGER"; Set-Item $p ...; & { git log },Set-Content -Path ("E"+"nv:" + $e) ...; & { git log }, and theType.InvokeMemberform.& { git log }. They include splat, pipeline and variable-held paths,ForEach-Object -MemberName, reflection,Set-Alias, script blocks, andSet-Location -Path $computedfollowed by a relative write.block-dangerous-git.test.sh: 815 cases pass, including new rows for a splitEnv:token, a path held in a variable,InvokeMember, and the variants above, plus predicate pins that testps::has_unprovable_env_writeon its own. All 25 guardrails suites pass;shellcheckis clean on the changed.shfiles.scripts/check-changelog-parity.sh --check,--check-order,--check-preserved origin/main,--check-bump origin/mainandscripts/validate-plugins.shpass at this head.bash scripts/affected-tests.sh --run --jobs 8 origin/main: 430 shell suites run, and the only failures are three suites that need tools this host lacks (check-html-assets.test.shandcheck-script-contract.test.shneedhtmlhint;hook-census.test.shneedsstrace).scripts/check-guardrails-ps-differential.sh origin/mainover 837 commands: the only cells that move are 19 unique read-only git commands (for example& { git status --porcelain },(git diff) > out.txt) that main refused and the branch allows. Theblock-no-verify, convention, noncanonical, hook-bypass and delete rows show 0 moved cells, and no command is refused by the branch that main allows. The script reports the loosened cells by design.Related
ps-command.sh; main was merged into this branch twice since it opened.🤖 Generated with Claude Code