fix(sandbox): classify git push as network access#726
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughGit ChangesSandbox network classification
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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.
| {name: "direct vite", script: "vite --host 127.0.0.1", network: true}, | ||
| {name: "next dev", script: "next dev", network: true}, | ||
| {name: "git clone", script: "git clone https://example.com/repo.git", network: true}, | ||
| {name: "git push custom remote", script: "git push gitlawb main", network: true}, |
| "ftp ftp.example.com", | ||
| "sftp user@host", | ||
| "sudo telnet example.com 23", | ||
| "git push gitlawb main", |
| switch firstSubcommand(words, nil) { | ||
| case "clone", "push": | ||
| return true | ||
| default: | ||
| return false | ||
| } |
| switch firstSubcommand(words, nil) { | ||
| case "clone", "push": | ||
| return true | ||
| default: | ||
| return false | ||
| } |
| case "go": | ||
| return firstSubcommand(words, nil) == "get" | ||
| case "git": | ||
| return firstSubcommand(words, nil) == "clone" | ||
| switch firstSubcommand(words, nil) { | ||
| case "clone", "push": | ||
| return true | ||
| default: | ||
| return false | ||
| } |
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.
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 clone,fetch,pull, andpushas network operations.git pushflow.