fix(sandbox): classify git push as network access - #726
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGit network detection now handles value-taking global options, remote archives, wrappers, and platform-specific Git executables. Parsed and unparseable network commands receive network classification, with regression coverage for approved ChangesSandbox network classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant SandboxClassifier
participant GitAnalyzer
participant GitProcess
Agent->>SandboxClassifier: request git push
SandboxClassifier->>GitAnalyzer: classify Git subcommand
GitAnalyzer-->>SandboxClassifier: critical network risk
SandboxClassifier-->>Agent: request network approval
Agent->>GitProcess: execute approved git push
GitProcess-->>Agent: return command output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the sandbox command analyzer/risk classifier to treat git push as network-sensitive, with added tests to ensure the AST-based analyzer flags it even when the command doesn’t contain an obvious URL.
Changes:
- Extend
commandUsesNetworkto classifygit pushas network access. - Add analyzer coverage for
git push(and a non-networkgit commit) inAnalyzeCommandtests. - Add a risk-classifier hardening test asserting
git pushis flagged as critical+network when regex-based detection would miss it.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/sandbox/analyzer.go | Expands git subcommand network detection to include push. |
| internal/sandbox/analyzer_test.go | Adds AnalyzeCommand test cases for git push and a local-only git commit. |
| internal/sandbox/risk_hardening_test.go | Adds a hardening test to ensure AST-based classification flags git push as network. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/risk_hardening_test.go (1)
297-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
t.Errorfovert.Fatalfin loops.Using
t.Fatalfinside a loop will immediately abort the test on the first failure, which prevents the remaining test cases from executing. Replacing it witht.Errorfallows all cases to be evaluated even if one fails.♻️ Proposed refactor
for _, command := range []string{ `curl https://example.com && "unterminated`, `git fetch origin && "unterminated`, `git pull origin main && "unterminated`, `git push gitlawb://example.com/repo.git main && "unterminated`, } { risk := classifyCommand(command) if !HasRiskCategory(risk, "unparseable_command") { - t.Fatalf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) } if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { - t.Fatalf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/risk_hardening_test.go` around lines 297 - 309, In the table-driven loop testing classifyCommand, replace both t.Fatalf calls with t.Errorf so each command case is evaluated even when an earlier assertion fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/sandbox/risk_hardening_test.go`:
- Around line 297-309: In the table-driven loop testing classifyCommand, replace
both t.Fatalf calls with t.Errorf so each command case is evaluated even when an
earlier assertion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45f38035-3958-4d77-b84f-f163855e8c49
📒 Files selected for processing (5)
internal/agent/loop_test.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for this, and the direction is right (git push should be network-gated). But the AST classifier still misses the most common git form, so I would like a fix before it lands.
The git branch of commandUsesNetwork calls firstSubcommand, which skips only dash-prefixed and numeric tokens. git's value-consuming global options put their value in the NEXT token, so firstSubcommand returns that value as the "subcommand." I ran the classifier against the current head:
git push origin main Network=true Risk=critical network <- correct
git -C repo push origin main Network=false Risk=high (none) <- missed
git -c http.sslVerify=false push Network=false Risk=high (none) <- missed
git --git-dir /x/.git push Network=false Risk=high (none) <- missed
git.exe push origin main Network=false Risk=high (none) <- missed
These all parse cleanly, so TooComplex stays false and the unparseable-pattern fallback never runs. So git -C <dir> push (the canonical form for operating on a repo without cd) classifies as plain shell, not network, and its risk drops from Critical to High.
To be fair on severity: this is not an always-open egress hole. When the sandbox backend is provisioned, the runtime deny-by-default still blocks the socket and raises the network prompt via ReasonNetworkBlocked, so the classifier is defense-in-depth there. But it becomes a real unprompted-egress path when the backend is unavailable or degraded, and the Critical-to-High mis-level can flip auto-allow in the more permissive autonomy modes regardless. Since the whole point of the PR is to classify these, I would rather close the gap than ship a gate that misses the most common invocation.
The fix looks small:
- In the git case, skip the values of git's space-separated value-consuming globals (-C, -c, --git-dir, --work-tree, --namespace, --exec-path, --super-prefix) before taking the subcommand. The joined --git-dir=/x form is already fine since it is one dash-prefixed token.
- Normalize a .exe program token so git.exe is treated as git.
- Add a PARSEABLE regression test: classifyCommand("git -C repo push origin main") should be RiskCritical with the network category. Right now the only -C test is the one with the trailing
&& "unterminated, which forces the unparseable path and masks this AST gap.
Otherwise the wiring is fine, and build/vet/gofmt are clean locally. Happy to re-review quickly once the AST path handles the option forms.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — the core fix is correct and well-covered: git clone|fetch|pull|push now classify as network access on the primary AST path, verified end-to-end (git push origin main → Network=true, git commit -m x stays Network=false), and the hardened regex fallback fails closed on unparseable variants. The one remaining gap is a minor consistency issue, not merge-blocking.
Nice work: the AST change (analyzer.go:153) and the fallback hardening (risk.go:36, now git(?:\s+[^\s;&|]+){0,8}\s+(clone|fetch|pull|push)) are both landed, and the new tests are real — TestAnalyzeCommand (git fetch/pull/push-custom-transport → network), TestClassifyASTCatchesNetworkProgramsRegexMisses, and TestClassifyUnparseableNetworkCommandFailsClosed including the git -C repo push … && "unterminated fail-closed case. All PR-relevant classification tests pass locally.
[Minor] AST path doesn't skip git global options, so git -C <dir> push isn't classified as network — inconsistent with the fallback you just hardened
internal/sandbox/analyzer.go:153 (root cause: firstSubcommand, analyzer.go:243) — pre-existing blind spot, PR-introduced inconsistency
The three reported findings all collapse to this single root cause. firstSubcommand skips dash-prefixed tokens but treats the next bare token as the subcommand, so for git -C /repo push origin main (words [-C, /repo, push, …]) it returns /repo, not push. The git case then returns Network=false. Runtime probe on the PR HEAD:
git push origin main Network=true TooComplex=false
git -C /repo push origin main Network=false TooComplex=false <- gap
git -c http.proxy=x push origin main Network=false TooComplex=false <- gap
git -C /repo fetch origin Network=false TooComplex=false <- gap
git -C /repo pull origin main Network=false TooComplex=false <- gap
git --git-dir=/repo/.git push origin main Network=true TooComplex=false (caught: --foo starts with '-')
The gap is specifically the space-separated value-taking global options (-C <path>, -c <name=value>, --git-dir <path>, --work-tree <path>, --namespace <ns>, --exec-path <path>). Because these commands parse cleanly (TooComplex=false), the hardened unparseableNetworkPattern at risk.go:36 is never consulted — that branch is gated on analysis.TooComplex at risk.go:132. So the fallback now tolerates git -C repo push but the primary AST path does not: the two paths disagree, and the network category the PR exists to add is silently omitted for the very common git -C <dir> push/fetch/pull form.
Impact is bounded — this is not a network-exfiltration bypass. Network enforcement mode is derived from policy.Network via NormalizeNetworkMode at profile.go:109, decoupled from the analyzer, and the auto-allow branch at engine.go:410 is gated on shellSandboxActive → NativeIsolation. So a misclassified git -C . push still runs wrapped by the platform sandbox with NetworkDeny enforced at the syscall level (the connect() is blocked and the agent reactively prompts), and where no native sandbox is active it prompts anyway via the general path rather than auto-allowing. The only real-world effect is degrading a proactive ReasonNetworkBlocked prompt (engine.go:355/357) into a reactive/generic one, plus the AST↔regex inconsistency.
Provenance: the underlying firstSubcommand blind spot is pre-existing — base analyzer.go:153 was firstSubcommand(words, nil) == "clone" and had the same hole for git -C <dir> clone. What this PR introduces is the inconsistency: it extended classification to push/fetch/pull and explicitly closed the -C gap in the regex fallback (and tests it), but left the primary AST path unfixed.
Suggested fix: give the git case a dedicated subcommand resolver that consumes git's global value-taking options before reading the subcommand (-C <path>, -c <name=value>, --git-dir, --work-tree, --namespace, --exec-path in their separate-token form), mirroring the tolerance already in unparseableNetworkPattern, then test the resolved token against {clone,fetch,pull,push}. Add git -C repo push origin main (plus -c / fetch / pull variants) to TestClassifyASTCatchesNetworkProgramsRegexMisses and TestAnalyzeCommand — those assertions fail today and would pin the fix.
Tests: go build ./..., go vet on the touched packages, and gofmt -l are clean; all PR-relevant classification/agent tests pass. The failing tests in internal/sandbox and internal/agent (path/symlink/out-of-workspace under /private/tmp) are pre-existing environmental failures that reproduce identically on base — not PR-attributable.
Merge is kevin's call per the program gate.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
Resolve the merge conflict with
main
GitHub currently reports this PR asCONFLICTING/DIRTY, so it cannot be merged or tested in its target-branch composition. Rebase or merge the current target branch and resolve the conflict before requesting another review. -
[P2] Classify Git commands after consuming global-option values
internal/sandbox/analyzer.go:153
The genericfirstSubcommandskips-C/-c/--git-dirthemselves but treats their following values as the subcommand. Consequently ordinary, parseable commands such asgit -C repo push origin main,git -c http.proxy=x fetch origin, andgit --git-dir /repo/.git pullproduce nonetworkrisk. Because they parse successfully, the new regex fallback is not consulted, and the engine skips its required proactive network-approval path. Use a Git-aware resolver that consumes value-taking global options (asinternal/agent/command_prefix.goalready does) and add parseable regression coverage. -
[P2] Recognize the Windows
git.execommand spelling
internal/sandbox/analyzer.go:152
effectiveProgramnormalizesgit.exetogit.exe, notgit, sogit.exe push origin mainnever reaches this new Git network classifier. It is parseable, so the fallback cannot repair the miss and the command does not receive the intended proactive network prompt. Normalize executable suffixes (or explicitly handlegit.exe) and cover that spelling in the analyzer and risk tests.
…ush-network main's #7xx already classifies git clone/fetch/pull/push/ls-remote/archive as network in gitUsesNetwork, so the conflict resolves to main's version and this branch keeps only what is still missing: - gitSubcommand resolves the subcommand past git's value-taking global options (-C, -c, --config-env, --exec-path, --git-dir, --namespace, --super-prefix, --work-tree). firstSubcommand treated their VALUE as the subcommand, so `git -C repo push origin main` — the canonical form for operating on a repo without cd — classified as plain shell with no network category. Those commands parse cleanly, so the unparseable-command regex fallback never ran. - effectiveProgram trims a trailing .exe, so `git.exe push` (and curl.exe, wget.exe, ...) reach the same classification as the bare name. Only .exe is trimmed; a .bat/.cmd of the same stem is a different script. - Parseable regression coverage for both, in the analyzer table and a new risk test that asserts TooComplex is false so the fallback cannot mask a future AST gap, plus the local-work cases that must stay off the network path. The hardened fallback regex, the t.Errorf-in-subtests conversion, and the end-to-end approved-git-push network-grant test come from this branch unchanged.
|
Merged current Resolve the merge conflict with [P2] Classify Git commands after consuming global-option values (@jatmn, @gnanam1990, @Vasanthdev2004 — all three findings share this root cause) — fixed with a git-aware
[P2] Recognize the Windows Parseable regression coverage — the key point from @Vasanthdev2004's review was that the only @copilot: "the approved-prompt behavior may need an execution-path change, not just classification" — checked, and no execution change is needed. The turn network grant already applies on approval; the missing piece was purely classification, so the command never reached that path. @copilot: use a Validation (Windows host, Go 1.26.5): @jatmn @Vasanthdev2004 @gnanam1990 — ready for another look; I can't use the reviewer-request button on this repo, hence the mention. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/risk_hardening_test.go (1)
337-342: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCover
git.exein the unparseable network fallback.
unparseableNetworkPatternmatchesgitonly, so an unparseablegit.exe push ...misses the criticalnetworkcategory even though parseablegit.exeis classified correctly. Accept an optional.exesuffix and add that regression case.Proposed fix
-|\bgit(?:\s+[^\s;&|]+){0,8}\s+(clone|fetch|pull|push)\b +|\bgit(?:\.exe)?(?:\s+[^\s;&|]+){0,8}\s+(clone|fetch|pull|push)\b+ `git.exe push gitlawb://example.com/repo.git main && "unterminated`,As per coding guidelines,
**/*_test.gorequires regression tests for behavior changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/risk_hardening_test.go` around lines 337 - 342, Update the unparseableNetworkPattern to match both git and git.exe command names while preserving the existing network-command requirements. In the risk-hardening regression table in the relevant test, add an unparseable git.exe push case so it is classified under the network category.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/sandbox/risk_hardening_test.go`:
- Around line 337-342: Update the unparseableNetworkPattern to match both git
and git.exe command names while preserving the existing network-command
requirements. In the risk-hardening regression table in the relevant test, add
an unparseable git.exe push case so it is classified under the network category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b9a21d6-40d7-4443-869c-0714562d2150
📒 Files selected for processing (5)
internal/agent/loop_test.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/risk.go
- internal/agent/loop_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Recognize
--attr-sourceas a value-taking Git global option
internal/sandbox/analyzer.go:308
Git accepts--attr-source <tree-ish>before the subcommand (for example,git --attr-source HEAD push origin main), but it is absent from this new skip list.gitSubcommandtherefore returnsheadinstead ofpush; because this command parses successfully, the fallback is not consulted and the shell call never receives the criticalnetworkclassification or its network-enabled approval profile. Add this global option and a regression case; update the paired command-prefix parser too if the documented mirroring is intentional. -
[P2] Make the unparseable Git fallback cover the supported Windows form and option count
internal/sandbox/risk.go:36
The new AST path normalizesgit.exe, but parser-failing Windows commands never reach that code. For example,git.exe push origin main & rem 'runs undercmd.exebut is rejected by the POSIX parser; this pattern requires whitespace immediately aftergit, so classification adds onlyunparseable_command, notnetwork, and skips the network approval/turn-grant path. The{0,8}cap has the same failure once a Git invocation has more than four value-taking global options. Match an optional.exesuffix and scan Git tokens up to a command separator without the arbitrary cap, with regressions for both forms.
…allback gitSubcommand (and its mirror in internal/agent/command_prefix.go) was missing --attr-source from git's value-taking global options, so `git --attr-source HEAD push origin main` resolved to the wrong subcommand and never got classified as network access. The unparseable-command regex fallback used when the shell parser fails now also matches an optional .exe suffix, so a Windows form like `git.exe push origin main & rem '` — valid under cmd.exe but rejected by the POSIX parser AnalyzeCommand uses — still classifies as network. The generic-token scan before the subcommand no longer caps at 8 tokens; Go's regexp package is RE2-backed (linear time, no backtracking blowup), so the cap only served to silently drop coverage once a git invocation had more than a handful of value-taking global options. Addresses review feedback from jatmn on PR Gitlawb#726. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/risk.go`:
- Around line 36-44: The unparseable Git fallback in unparseableNetworkPattern
must not treat arbitrary tokens before push, fetch, or pull as global options.
Restrict matching to recognized Git global options and their values, or reuse
the shared token-aware resolver from analyzer.go, while preserving support for
git.exe and complex valid invocations. Add a regression test covering a local
command such as git status push so it is not classified as network.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59e3c8be-fb6a-46bd-8c23-15d475e08ebd
📒 Files selected for processing (5)
internal/agent/command_prefix.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/risk_hardening_test.go
- internal/sandbox/analyzer.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Restore network classification for wrapper-prefixed unparseable commands
internal/sandbox/risk.go:71
This is a confirmed regression from8a7ffe6, not pre-existing drift. The refactor ofmatchesUnparseableNetworktokenizes each;/&/|segment and only elevates tonetworkwhentokens[0]is a git executable or when the rebuilt segment matches the new^-anchoredunparseableNetworkPattern. That drops basemain's whole-string\b(curl|wget|…)\b/\bgit\s+clone\bmatching, so obfuscated commands where the network binary is not argv[0] lose thenetworkcategory. Verified on base vs head:classifyCommand(sudo curl https://example.com && "unterminated)goes from Critical +networkto High +unparseable_commandonly; the same miss hitsenv curl …,env git fetch …,sudo git push …,sudo npm install …,timeout 5 curl …,PATH=.:$PATH git push …, andsh -c 'curl …' && "unterminated. Engine impact is also verified: withNetworkDenyandPermissionGranted: true, base returnsActionPrompt/ReasonNetworkBlocked, while head returnsActionAllowwith"tool requires approval before execution". The parseable AST fixes forgit -C repo push,git.exe, and quoted Windows forms are working; the gap is only on the fail-closed fallback when the executable is behind a wrapper or env assignment. Please keep the git-specific token-aware matcher, but also peelwrapperPrograms/ env assignments (aseffectiveProgramalready does on the parseable path) or otherwise restore fail-closed detection on this path, and add regressions besideTestClassifyUnparseableNetworkCommandFailsClosed. -
[P3] Strip Windows executable suffixes in the unparseable fallback
internal/sandbox/risk.go:118
Companion regression from the samematchesUnparseableNetworkrefactor. Parseable classification already normalizescurl.exethroughnormalizeProgramToken, butexecutableTokenBaseonly lowercases the basename and leaves.exeattached. Verified: unparseablecurl.exe https://example.com && "unterminatedlosesnetworkon head (network=false) while basemainstill matched it via\bcurl\b(network=true). Please reuse the same suffix normalization here (or callnormalizeProgramToken) and add an unparseablecurl.exe/wget.exeregression. -
[P3] Split fallback segments on newlines
internal/sandbox/risk.go:129
Companion regression from the same refactor.fallbackCommandTokensflushes commands only on;,&, and|, not on\n. Verified:true\ncurl https://example.com && "unterminatedstays one segment whose first token istrue, so the embeddedcurlis invisible to the new matcher (network=falseon head,network=trueon base). Please treat newline as a command separator (or otherwise scan later tokens in the segment) and add a regression. -
[P3] Gate local
git archiveon--remotewhen touching this verb list
internal/sandbox/analyzer.go:281
This one is partly drift, not a merge blocker for the coregit pushfix. Treating everyarchivesubcommand as network is already true on basemainforgit archive HEAD; this PR'sgitSubcommandfix extends that pre-existing false positive togit -C repo archive HEAD, which base misclassified as offline because it readrepoas the subcommand.git archiveonly needs network with--remote. If you are already editing the verb list in this PR, please gate both the AST and fallback matchers on a remote flag and addgit archive HEAD/git -C repo archive HEADnegative cases withnetwork: false. If not, this can be a follow-up; it should not block the wrapper-regression fix above.
Reworking the fallback to be token-aware made it read the program from tokens[0], which is only the program when nothing precedes it. A wrapper prefix, an environment assignment, or a shell launcher put it elsewhere, so `sudo curl …`, `env git fetch …`, `timeout 5 curl …`, `PATH=.:$PATH git push …`, and `sh -c 'curl …'` all lost the network category that the previous whole-string match caught. Under NetworkDeny with permission already granted that turned a network prompt into a plain allow — egress granted to a command too obfuscated to parse. The fallback now resolves each segment's program exactly as the parseable path does, through commandBodyFields (extracted from commandBody), and recurses into a shell launcher's -c payload the way analyzeInto does. Resolving the program rather than matching a network name anywhere in the string is what still keeps `git status push` and `echo https://example.com/repo.git push` out. Two companion gaps from the same refactor: executableTokenBase now strips Windows executable suffixes through the same helper normalizeProgramToken uses, so `curl.exe` matches here as it does there, and the fallback tokenizer treats a newline as a command separator rather than as whitespace, so a program on a later line is resolved instead of being read as arguments of the first. Separately, `git archive` is now gated on --remote on both paths. A local archive streams the object store and never leaves the machine; treating every archive as network cost a proactive prompt on an offline command, and this PR's gitSubcommand fix had extended that false positive to `git -C repo archive`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed all four findings in [P2] Wrapper-prefixed unparseable commands are fail-closed again. You called it: reading the program from Resolving the program instead of reverting to whole-string matching is what still keeps [P3] [P3] Newlines. [P3] Regressions (each verified to fail with the source changes stashed and the tests kept):
Validation
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/analyzer.go`:
- Around line 300-303: Update the argument scan in the analyzer logic to stop
processing words once the standalone `--` separator is encountered, so
subsequent pathspecs such as `--remote` are not classified as network options.
Preserve detection of `--remote` and `--remote=...` before the separator, and
add a regression case covering `git archive HEAD -- --remote`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0ce84d3-2a63-4901-b4ef-1978ea27905e
📒 Files selected for processing (6)
internal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/engine_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.gointernal/sandbox/safe_command.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sandbox/risk.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Respect
--when scanninggit archivefor--remote
internal/sandbox/analyzer.go:299
gitTargetsRemoteArchivewalks every argument token and treats any--remote/--remote=…as a network archive, with no stop at the standalone--end-of-options marker. After--, operands are pathspecs, not options, sogit archive HEAD -- --remote(and the same shape with-C/-o) is a local archive of a tree entry named--remote, not a remote fetch. Both the parseable AST path (gitUsesNetwork) and the unparseable fallback (matchesUnparseableGitNetwork) call this helper, so the false positive hits proactive network prompts and turn network grants for a purely local command. This gap was introduced when this PR gatedarchiveon--remote; it is not pre-existing drift. The same package already handles--correctly forrminhasRecursiveForce; mirror that pattern here and add a regression forgit archive HEAD -- --remoteexpectingnetwork: false. CodeRabbit'sCHANGES_REQUESTEDreview on head6225826is still open on this point. -
[P3] Align unparseable shell recursion depth with the AST path
internal/sandbox/risk.go:74
maxUnparseableShellDepthis3whilemaxAnalyzerDepthinanalyzer.gois4, despite the comment that the fallback "mirrors" the parseable limit. On a fourth nestedsh -c/bash -clayer that is also unparseable, the AST path can still reach an innercurl/git push, butmatchesUnparseableNetworkAtstops recursing one level earlier and drops thenetworkcategory. UnderNetworkDenywith shell permission already granted, that is the same fail-open shape the wrapper-prefix regression fixed in6225826, but it requires heavy obfuscation and is unlikely in normal use. This mismatch is new in this PR's fallback code, not drift from basemain. Either bump the fallback cap to4or document and test the intentional difference.
Follow-up (optional for #703; pre-existing or edge-case parity)
These are real classification gaps, but they are mostly outside the core git push fix or require obscure spellings. I would not block merge on them alone once the git archive -- issue above is fixed.
-
[P3] Recurse into
--commandpayloads on launcher paths
internal/sandbox/risk.go:152andinternal/sandbox/analyzer.go:488
fallbackDashCPayloadanddashCPayloadonly recognize-c, whileshellDashCPayloadinsafe_command.goalready accepts both-cand--command. An unparseablebash --command "git push origin main" && "unterminatednever recurses into the payload, so classification can stay atunparseable_commandwithoutnetworkafter a prior shell approval. The-c-only limitation on the parseable AST path predates this PR; this branch copied it into the new fallback instead of sharingshellDashCPayload. Worth fixing if you touch launcher recursion, but it is parity drift rather than a new regression from the6225826wrapper fix. -
[P3] Normalize drive-relative Windows executables on the fallback path
internal/sandbox/risk.go:166
executableTokenBasestrips basename and suffixes but does not callwindowsExecutablePathBasename, so drive-relative spellings such asC:git.exenormalize toc:gitinstead ofgit. The parseable path already classifies'C:curl.exe' https://example.comas network vianormalizeProgramToken. An unparseable Windows form likeC:git.exe push origin main & rem 'can miss thenetworkcategory on the fail-closed path while the AST path would catch it. This is an obscure Windows spelling gap on the fallback path introduced when this PR addedexecutableTokenBaseparity claims; the commongit.exe/ quoted-path forms you already test are fine.
Rechecked — prior review items look addressed on current head
The wrapper-prefix, curl.exe, newline-segment, and local-git archive regressions from the 8a7ffe6 review appear fixed in 6225826, with matching coverage in TestClassifyUnparseableNetworkBehindWrapperFailsClosed, TestEvaluatePromptsForUnparseableNetworkBehindWrapper, and TestClassifyUnparseableLocalGitArchiveStaysNonNetwork. Parseable -C / -c / --git-dir / git.exe / --attr-source forms and the git status push negatives also still look covered. Windows Smoke failed on internal/tools TestExecCommandForegroundServerReturnsSessionAndServesHTTP; this PR does not touch internal/tools, and internal/sandbox and internal/agent passed on that CI run.
Gating `git archive` on --remote scanned every token, with no stop at the standalone `--`. After that separator the operands are pathspecs, so `git archive HEAD -- --remote` archives a tree entry NAMED --remote out of the local object store — and was classified as network, costing a proactive prompt and a network grant on a purely local command. Both the AST path and the unparseable fallback call the same helper, so both were wrong; the end-of-options rule is the one hasRecursiveForce already applies to `rm -- -rf`. Three parity gaps in the same code, each one a claim my own comments made: The fallback's launcher recursion cap said it mirrored the parseable path while being one level shallower, so a fourth nested layer that the analyzer still reaches would drop the network category. It now IS maxAnalyzerDepth rather than a copy of its value. Both payload scanners knew only `-c`, though bash and zsh accept `--command` and shellDashCPayload already handled both — so `bash --command 'git push …'` was never recursed into on either path. executableTokenBase claimed parity with normalizeProgramToken but skipped windowsExecutablePathBasename, leaving a drive-relative `C:git.exe` as "c:git" and unmatched on the fail-closed path while the AST path classified it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All four addressed in [P2] [P3] Recursion depth. Bumped, but by making it On testing it: I could not honestly drive a four-layer [P3, optional] [P3, optional] Drive-relative Windows executables. Taken. Tests: 11 new cases across Validation: Agreed the Windows Smoke failure is unrelated — |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The git half of this is right and I could not break it. Requesting changes for one thing: the fallback rewrite is a net loss on the path it was supposed to harden.
The git classification works
I drove AnalyzeCommand / Classify / engine.Evaluate directly rather than reading the code. Every form the issue was about now classifies critical/network on the AST path: git push, git -C repo push origin main, git -c http.sslVerify=false push, git --git-dir /x push, git --git-dir=/x push, git --attr-source HEAD push, git --work-tree /w -C /r push, git.exe push, git.cmd push, C:git.exe push, /usr/bin/git push, sudo git push, env git push, GIT_SSH=x git push, bash -c "git -C repo push", bash --command "git -C repo push". git status, git -C repo commit -m ..., git archive HEAD, git -C repo archive HEAD and git archive HEAD -- --remote stay off it. The archive --remote gate and the -- pathspec rule are both correct.
The tests pin it, too. I mutated eight things — --attr-source out of gitGlobalOptionConsumesValue, the -- stop in gitTargetsRemoteArchive, newline splitting in fallbackCommandTokens, --command out of isShellCommandFlag, the .exe strip in executableTokenBase, the drive-relative basename branch, maxUnparseableShellDepth = 0, and commandBodyFields replaced with raw tokens — and every one turned a test red. Nothing here is a test that only looks like it covers its name.
The blocker: the fallback got narrower than main
matchesUnparseableNetworkAt (risk.go:94) now resolves one program per segment and matches the ^-anchored unparseableNetworkPattern (risk.go:38) against it. But fallbackCommandTokens only splits on ; & | and newlines (risk.go:227), and commandBodyFields only skips assignments and wrapper programs. So whenever the real program isn't the first body token of such a segment, the resolved program comes out as echo, $, (curl, {, then, do, x), >out, 2>err, eval or ! — nothing matches, and the anchor means the old whole-string \bcurl\b can't rescue it either.
I ran these through the real gate — NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}).Evaluate(...) with SideEffectShell and PermissionGranted: true, after asserting that sudo curl https://evil.test && "unterminated returns ActionPrompt / ReasonNetworkBlocked on both revisions so I knew the harness was live. On origin/main all fifteen return prompt / network access requires approval. On this branch all fifteen return allow:
echo $(curl https://evil.test) && "unterminated
echo `curl https://evil.test` && "unterminated
x=$(curl https://evil.test) && "unterminated
(curl https://evil.test) && "unterminated
( curl https://evil.test ) && "unterminated
{ curl https://evil.test ; } && "unterminated
cat <(curl https://evil.test) && "unterminated
if true; then curl https://evil.test; fi && "unterminated
for i in 1 2; do curl https://evil.test; done && "unterminated
while :; do wget https://evil.test; done && "unterminated
case x in x) curl https://evil.test;; esac && "unterminated
>out curl https://evil.test && "unterminated
2>err curl https://evil.test && "unterminated
eval "curl https://evil.test" && "unterminated
! curl https://evil.test && "unterminated
The new git path has the same hole, so it doesn't fully deliver on the title either — if true; then git push; fi, (git -C repo push), echo $(git push) and eval "git push" (all && "unterminated) classify as non-network.
This is the fail-closed path for commands too obfuscated to parse. A wrapper prefix used to be the cheapest bypass and you closed that; a $(...) or a then is now cheaper.
I'm not asking you to restore the old regex — the echo ssh://git@example.com/repo.git push case in your own test is exactly why you anchored it. It's the segmenter that needs to know more. I prototyped this in a worktree and it holds:
- also treat
()`{}as unquoted command boundaries infallbackCommandTokens; - skip leading shell keywords (
if while until then do else elif in !) and leading redirect tokens (>x,<x,2>x) before resolving the program; - treat
evallike a launcher and recurse on the rest of the segment.
That restores all fifteen, newly catches the four git construct forms, and go test ./internal/sandbox/ stays fully green — including TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork, so the ssh:// and git status push false positives don't come back. Please add regression cases for the construct forms; the current unparseable tests all exercise the wrapper/prefix shape, so none of them would have caught this.
Smaller things, not blocking
gitSubcommand(analyzer.go:325) doesn't stop at--help/--version, butmatchesUnparseableGitNetwork(risk.go:127) does. Sogit push --helpandgit --help pushare network on the AST path and not on the fallback. It only costs an extra prompt, but the two paths should agree.- The option list is duplicated between
gitGlobalOptionConsumesValue(analyzer.go:349) andgitOptionConsumesValue(internal/agent/command_prefix.go:415). They match today; the next git release that adds a global will update one of them. One exported source would be better. - Not yours to fix here, but the same
firstSubcommandtrap is wide open for everything else, and I confirmed each of these isNetwork == false(after asserting the plain forms do classify):npm --registry https://evil.test install left-pad,npm --prefix /tmp/x install,npm -w pkgs/a install,npm --loglevel silly install,yarn --registry https://evil.test add,pnpm --dir /tmp add,pip --index-url https://evil.test install requests,pip --cache-dir /tmp/c install,pip3 --proxy http://evil.test install,go -C sub get example.com/x,gh --repo o/r api /user. Worth a follow-up issue. - The body says the PR ensures approving the prompt enables the temporary network profile immediately, but there's no production change behind that —
loop.gois identical to main; what changed isTestRunApprovedNetworkBashPromptAppliesTurnNetworkGrantbeing re-pointed from curl to git push. Good coverage, just not a fix.
go build ./..., go vet, gofmt -l, and go test ./internal/sandbox/ ./internal/agent/ are all clean on the head as it stands.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbdf9-75a6-7669-9880-0bd2f35c08ca Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
5854a94
|
Addressed the latest fallback-classifier review in 5854a94. What changed:
Validation completed:
@Vasanthdev2004 ready for another look. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/risk_hardening_test.go (1)
480-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a short rationale comment for the non-network cases.
TestClassifyUnparseableShellSyntaxTextStaysNonNetworkhas no explanatory comment, unlikeTestClassifyUnparseableNetworkInShellConstructFailsClosedright above it. Each case here relies on a subtle bash distinction:${curl}and$((curl))reference a variable, not a command;arr=(curl)is an array literal; the escaped-backtick case prints literal text instead of running command substitution; andcommand if curl .../env then git push ...place the network-looking token behind a non-executing prefix. A short comment stating this contract (why each form must NOT be flagged) helps a future maintainer avoid "fixing" one of these into a false positive.📝 Suggested comment
+// TestClassifyUnparseableShellSyntaxTextStaysNonNetwork covers shell syntax +// where "curl"/"git push" appear as inert text rather than an invoked +// command: parameter/arithmetic expansion, array literals, backslash-escaped +// backticks (no command substitution), and network-looking tokens placed as +// plain arguments behind a non-executing prefix (command/env). These must +// stay non-network so the fallback tokenizer does not over-flag benign text. func TestClassifyUnparseableShellSyntaxTextStaysNonNetwork(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/risk_hardening_test.go` around lines 480 - 499, Add a concise rationale comment immediately above the table in TestClassifyUnparseableShellSyntaxTextStaysNonNetwork explaining that these malformed forms contain non-executing variable, arithmetic, array, escaped-backtick, or prefix contexts, so network-looking tokens must not receive the network category.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/sandbox/risk_hardening_test.go`:
- Around line 480-499: Add a concise rationale comment immediately above the
table in TestClassifyUnparseableShellSyntaxTextStaysNonNetwork explaining that
these malformed forms contain non-executing variable, arithmetic, array,
escaped-backtick, or prefix contexts, so network-looking tokens must not receive
the network category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a57647d8-c69f-4be8-b3f8-df8f84d4276a
📒 Files selected for processing (3)
internal/sandbox/engine_test.gointernal/sandbox/risk.gointernal/sandbox/risk_hardening_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/engine_test.go
- internal/sandbox/risk.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep the backtick quote stack intact when scanning parentheses
internal/sandbox/risk.go:325
outerQuotesis currently one untyped stack for two different delimiters: backtick substitutions push their surrounding quote at lines 286–295, while every unquoted)pops the same stack at lines 325–329. Inecho \curl)` && "unterminated, the inner)consumes the frame that belongs to the backtick; the closing backtick then evaluatesouterQuotes[len(outerQuotes)-1]after the slice is empty and panics. The unmatched double quote makes the AST analyzer returnTooComplex`, so this is a request-triggerable crash in the fallback classifier, not merely an invalid command rejected by the shell.Do not use stack length alone to decide that
)closes an opened grouping/substitution. Represent the opener kind (for example, a frame withbacktickversusparen), and only pop a parenthesis frame on). The fallback should also be total over arbitrary input: add regression coverage for mismatched delimiters and ensure classification returnsunparseable_commandrather than panicking. A small fuzz/property test aroundfallbackCommandTokensthat asserts no panic for arbitrary strings would prevent the same class of parser-state bug from recurring. -
[P2] Recurse into CMD command payloads on the fallback path
internal/sandbox/risk.go:94
The new fallback resolves a single direct program, applies an anchored pattern to that program and its arguments, then only recursively inspectsevalandshellPrograms(the POSIXsh/bashfamily).cmd/cmd.exeis neither a recognized launcher nor a wrapper, socmd.exe /c curl https://evil.test & rem 'resolves tocmdand the payload is never visited. That text is valid CMD:rem 'is a comment, while the unmatched apostrophe intentionally makes the POSIX AST parser returnTooComplex. The old whole-command fallback matched the embeddedcurl; the new classifier returns onlyunparseable_command.This bypasses the intended decision order.
Engine.Evaluateprompts forNetworkDenyonly when the risk includesnetwork; otherwise an existing shell permission (or an active shell sandbox) reachesActionAllow. On the Windows unelevated backend, where WFP network filtering is unavailable and the in-process approval gate is the network boundary, the command can therefore perform egress without the explicit network prompt.Treat command interpreters as launchers with syntax-specific payload extraction, rather than relying on the POSIX launcher list for all fallback input. At minimum, recognize normalized
cmd/cmd.exeand recursively classify the remainder after/cor/k(including the common/d /cprefix); do the equivalent for the PowerShell command flags if that interpreter remains supported as shell input. Keep the recursion depth limit, and add classifier and engine tests using CMD-valid/POSIX-unparseable commands to assert both thenetworkcategory andReasonNetworkBlocked. -
[P2] Preserve CMD conditional command groups in the fallback tokenizer
internal/sandbox/risk.go:317
fallbackCommandTokensidentifies a grouping(only when it is the first token of a segment (len(tokens) == 0). That assumption is not true for CMD conditionals:if 1==1 (curl https://evil.test) & rem 'is valid CMD, but is intentionally unparseable by the POSIX AST. The fallback first stripsif, thencommandBodyFieldsskips1==1as an assignment-like token, and the remaining(curltoken is treated as the executable. The anchored network matcher never examinescurl, so the command loses thenetworkcategory and bypasses the sameNetworkDenydecision described above. Base matchedcurlanywhere in this fallback path.Avoid inferring executable boundaries from only a flat token list once a command has entered a non-POSIX fallback. Either give CMD input a dedicated tokenizer that understands
ifconditions and parenthesized command blocks, or make the fallback conservative when it cannot establish a reliable program boundary—particularly after CMD control keywords. Add a Windows regression throughEngine.Evaluate, not justClassify, for conditional groups and nested groups; it should prove that removing the network classification changes the test fromReasonNetworkBlockedto an incorrect allow. -
[P3] Stop Git subcommand scanning at help/version requests
internal/sandbox/analyzer.go:325
gitSubcommandcorrectly consumes the value of-C, but it treats every other dash-prefixed token as skippable and continues searching for a verb. Thus parseablegit -C repo --help pushreturnspush, despite Git exiting after printing help; a local check of that command exits successfully without invoking a remote. The previous generic scan stopped atrepo, so this PR newly turns the option-prefixed form from non-network into aNetworkDenyprompt. The unparseable Git path already has the correct opposite rule:matchesUnparseableGitNetworkstops at--helpand--version, leaving the AST and fallback paths inconsistent.Make the Git option parser express terminal global options explicitly and share that parser (or its result) between the AST and fallback classifiers. On encountering
--helpor--version, stop subcommand discovery rather than skipping it; include variants with preceding value-taking globals and make parseable/fallback tests assert the same result. This also prevents future additions to one duplicated Git-option list from silently reintroducing path disagreement.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-b20f-736c-aae7-19d5e79bf3e5 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Addressed the outstanding fallback-classifier review on current head The prior head commit
Validation completed on Go 1.26.5:
@Vasanthdev2004, please re-review the current head and supersede the stale |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve the network gate for CMD invocation syntax
internal/sandbox/risk.go:94
This matters on the supported Windowscmd.exefallback when PowerShell is unavailable. A valid CMD command such as@curl https://evil.test & rem 'is rejected by the POSIX parser solely because CMD treats the trailing apostrophe as part of itsremcomment;AnalyzeCommandtherefore setsTooComplexand invokes this fallback.fallbackCommandTokensyields@curl, butexecutableTokenBasedoes not remove CMD's echo-suppression prefix, so the anchored matcher misses curl. The same loss occurs when a CMD construct invokes a later command:cmd.exe /c call curl https://evil.test & rem 'resolvescallas the program, andif not 1==2 curl ...stops atnot;start "" curl ...has the same shape. These commands receive nonetworkcategory, so the engine skips itsNetworkDenyprompt and never applies the network-enabled turn profile; the command is then launched under the deny profile and fails instead of receiving the approval flow this PR is intended to restore. The former whole-command fallback recognized these examples.Please address the root cause rather than adding individual spellings to
executableTokenBase: the fallback is using a small POSIX-oriented tokenizer to infer execution positions in valid CMD source. Make fallback parsing runtime-aware and give CMD a shared resolver for echo prefixes,IFconditions, and command-launching builtins such asCALL/START(or conservatively retain fail-closed network detection for CMD payloads until that resolver exists). Add classifier andEngine.Evaluateregressions for the direct CMD forms above, assertingTooComplexandReasonNetworkBlocked, so this cannot regress into a generic denied run again. -
[P3] Treat all terminal Git global options as terminal
internal/sandbox/analyzer.go:331
The new Git-aware scanner stops at--helpand--version, but it skips other terminal globals and continues to a later word.git -h pushrenders the localgit-pushhelp,git -v pushprints Git's version, andgit --html-path pushprints the local documentation directory; none contacts a remote, but all now reachpushand are classified as critical network.matchesUnparseableGitNetworkduplicates the same incomplete terminal-option rule, so parser-failing forms disagree with neither path. This is a regression from introducing the option scanner: before it, a value or path earlier in these forms stopped subcommand discovery.Please make Git global-option parsing one shared, explicit result rather than maintaining parallel skip lists in the AST and unparseable classifiers. In particular, distinguish a resolved network subcommand from a terminal global action and from an unknown/ordinary command; include the short help/version forms and the path-query globals (
--html-path,--man-path,--info-path). Cover the complete result table on both paths, including a value-taking prefix such asgit -C repo -h push, so a future option addition cannot reintroduce AST/fallback drift.
The unparseable fallback used POSIX rules to find the executable position in
text that is frequently valid CMD source — that is precisely how it reaches
this path, since cmd.exe's `rem` comment swallows an apostrophe the POSIX
parser then cannot close. `@curl ...`, `call curl ...`, `if not 1==2 curl ...`
and `start "" curl ...` each resolved to a token that is not the program
("@curl", "call", "not", "start"), so the network category was dropped and
Engine.Evaluate skipped its network prompt — the boundary itself on the Windows
unelevated backend, where WFP filtering is unavailable.
Each fallback segment is now resolved under both command languages. The CMD
resolver understands echo suppression, the CALL/START launchers (including
START's options and window title), IF conditions in every spelling CMD accepts,
and that only a FOR loop's DO body executes. A second resolution can only add
the network category, which is the direction this fail-closed path may err in.
Git global options are now read once, by parseGitInvocation, which reports a
resolved subcommand, a terminal global, or neither. Both classification paths
call it, so they can no longer disagree: while each kept its own terminal-option
list, `git -h push`, `git -v push` and `git --html-path push` printed locally
but were classified as critical network on the AST path.
Adds classifier and Engine.Evaluate regressions for the CMD forms, a shared
result table asserted on both git paths, and a fuzz target over the resolvers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed [P2] Preserve the network gate for CMD invocation syntax Fixed at the root, not by adding spellings to
All four of your examples now classify as The negatives are pinned too: [P3] Treat all terminal Git global options as terminal There is now one parser.
Also added Verification: |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve the network gate for CMD-escaped executables
internal/sandbox/risk.go:383
The unparseable fallback resolves the executable without applying CMD escaping or recognizing%ComSpec%. For example,cu^rl https://evil.test & rem 'is valid CMD: CMD removes the caret and runscurl. The trailing quote is consumed by CMD'sremcomment but makes the POSIX analyzer returnTooComplex, somatchesUnparseableNetworkis the only network classifier. It passescu^rltoexecutableTokenBase, which preserves^; the anchored pattern consequently sees neithercurlnor a network category, andEngine.Evaluateskips theNetworkDenyprompt.%ComSpec% /c curl ... & rem 'similarly does not recurse into its/cpayload because%ComSpec%is not resolved as the CMD interpreter. This matters on the unelevated Windows backend, where network filtering is unavailable and the approval gate is the egress boundary. Address the root cause by giving the fallback an explicit runtime-aware CMD normalization/resolution stage (caret escapes, environment-backed command interpreters, launchers, and payload extraction) before its executable matcher; do not keep adding individual spellings toexecutableTokenBase. Add classifier andEngine.Evaluateregressions for direct andcmd /ccaret escapes plus%ComSpec%. -
[P1] Do not lose a one-word CMD START title
internal/sandbox/risk.go:253
CMD treats every first quoted argument toSTARTas the window title, butfallbackCommandTokensdrops the quotes andcmdStartPayloaddiscards a title only if its resulting text has whitespace. Thusstart "x" curl https://evil.test & rem 'is valid CMD and launchescurl, while the fallback receives[start, x, curl, ...], decidesxis the program, and emits no network category or prompt. The current tests cover an empty title and a title containing a space, which masks the lost quote provenance. Fix the root cause in the CMD parser rather than special-casing one-word titles: preserve whether each token was quoted, or conservatively evaluate both valid START interpretations when that information has been discarded. Add classifier and engine-gate coverage for one-word quoted titles, with and withoutcmd /cand START options. -
[P1] Parse CMD's CMDEXTVERSION condition before resolving its command
internal/sandbox/risk.go:276
cmdConditionPayloaddoes not recognize CMD'sIF [NOT] CMDEXTVERSION <number> <command>form. Inif cmdextversion 1 curl https://evil.test & rem ', the CMDremcomment again makes the otherwise valid CMD source unparseable to the POSIX analyzer. The fallback consumes onlycmdextversion, returns[1, curl, ...], and therefore treats the version number as the program; the network-capable guarded command disappears before matching and the engine skips the network-approval gate. The underlying problem is a partial CMD grammar being used to make a security decision. Model each supported IF condition's operand shape before selecting the command, and fail closed for conditions the fallback cannot parse reliably rather than assuming one token is enough. IncludeCMDEXTVERSION,NOT, and engine-levelReasonNetworkBlockedregressions. -
[P1] Recurse into wrapper arguments that contain the executed command
internal/sandbox/safe_command.go:322
The new fallback delegates wrapper handling tocommandBodyFields, which correctly skips inert option values but does not distinguish options whose values themselves determine what runs.env -S 'curl https://evil.test' && "unterminatedcauses GNUenvto split and execute its next argument, yet the fallback consumes that complete payload as the-Svalue and finds no program.exec -a harmless curl https://evil.test && "unterminatedchanges onlyargv[0], but becauseexechas no value-consuming-option rule the resolver treatsharmlessas the executable and missescurl. Both commands take the unparseable fallback due to the unmatched quote and now lose the network category that the base broad matcher supplied. Fix this at the wrapper-semantics layer: represent whether an option's value is inert, a nested command, or an argv decoration, and recursively classify nested command text while skipping only the correct decoration operands. Add focused fallback and engine tests forenv -S/--split-stringandexec -a. -
[P2] Treat
--list-cmdsas a terminal Git global option
internal/sandbox/analyzer.go:351
git --list-cmds=main pushprints the locally installed command list and exits successfully; it never runspush.parseGitInvocationrecognizes only a hand-maintained subset of terminal globals, skips--list-cmds=mainas an ordinary option, then readspushas a network subcommand. Both the AST and fallback paths consequently request network access and apply a temporary network grant for a local operation. The broader root cause is that the parser models only selected Git global-option behaviors while claiming to resolve the invocation. Make terminal, value-taking, and ordinary global options one explicit shared result table sourced from Git's documented global options; do not extend separate skip lists. Add--list-cmds=<groups>and a value-taking-prefix case to the AST/fallback parity test.
Summary
git pushas network-sensitive before sandbox executiongitlawb://in analyzer and risk-classifier testsFixes #703.
Validation
go test ./...go vet ./...GOTOOLCHAIN=go1.26.5 go run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...The repository-wide lint command still reports 35 pre-existing findings unrelated to this change.
Summary by CodeRabbit
git archiveoperations.