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
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.
|
Addressed all five findings from review
Added focused classifier and real Validation passed:
Local smoke was blocked by a stale executable from the previously checked-out branch: this branch expects @jatmn, please re-review current head |
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.
The core #703 fix is real — parseable git push, git -C repo push, and git --pager=less push all classify as network and Engine.Evaluate blocks with ReasonNetworkBlocked when shell permission is already granted. The items below are gaps in the same classification surfaces this PR touched. Several are regressions from replacing the old whole-string fallback regex with structured resolvers; a few are pre-existing holes in the new parseGitInvocation model that mirror the original -C repo push bug class.
Root cause themes
Three structural problems recur across these findings. Fixing them one token at a time will keep producing follow-up rounds.
-
Incomplete option grammars.
parseGitInvocationandgitTargetsRemoteArchivemodel only part of what git actually accepts. A value-taking global omitted fromgitGlobalOptionConsumesValuemakes the next token look like the subcommand (-C repo pushwas the prototype;--pager less pushis the same class). Scanning all tokens for--remotewithout respecting git's positional grammar misclassifies pathspecs after the tree-ish. Guidance: treat git parsing as a small shared grammar module — one place that knows value-taking globals, terminal globals, subcommand position, and archive operand layout — and drive bothgitUsesNetworkandmatchesUnparseableGitNetworkfrom it. Normalize casing inside that module so AST and fallback cannot diverge. -
Fallback resolver parity loss. Base
mainclassified many obfuscated commands via(?i)\b(curl|wget|…)\banywhere in the string. This PR's structured CMD/PowerShell resolvers are more precise but incomplete: they miss operand shapes the old regex caught (start MyTitle curl,cmd /c /d curl,powershell -EncodedCommand …). Guidance: when adding a resolver for a construct, add a parity table against the old regex for that construct's known spellings, and keep a conservative last-resort path (for example, after structured resolution fails, scan the remaining payload with the anchored network-token matcher you already use elsewhere) so precision does not trade away coverage. -
AST/fallback input normalization asymmetry. The AST path lowercases via
literalWordTextsbeforegitUsesNetwork; the fallback passes raw tokenizer output into the same functions. CMD caret stripping runs on the token path vianormalizeCMDTokenbut not on the string payload path insidefallbackCommandInterpreterPayload. Guidance: normalize once at the boundary each path enters shared logic (lowercase git tokens inparseGitInvocation; resetwordQuotedwhen a quote closes; runnormalizeCMDTokenon interpreter payloads before tokenizing).
Please add regressions in the existing parity suites (TestGitGlobalOptionsResolveIdenticallyOnBothPaths, TestClassifyUnparseableLocalGitArchiveStaysNonNetwork, engine_test.go CMD cases) rather than one-off probes.
Findings
-
[P2] Treat
--pageras a value-taking Git global option
internal/sandbox/analyzer.go:407,internal/agent/command_prefix.go:415What happens. Git accepts
--pager <pager>before the subcommand.git --pager less push origin mainis a normal spelling;lessis the pager program,pushis the subcommand.gitGlobalOptionConsumesValueomits--pager, soparseGitInvocationrecordslessas the subcommand,gitUsesNetworkreturns false, and the command never reaches the unparseable fallback because it parses cleanly.Verified on head
c457b9e.Classify→[shell]only (nonetwork).Engine.EvaluatewithNetworkDenyand shell permission already granted →ActionAllow,ReasonToolRequiresApproval— the same failure mode as the original #703 report.git --pager=less push …works because the joined form is skipped as one flag; only the separated spelling is broken.Regression status. Pre-existing on base
main(91b413c) — same miss there. Still in scope for this PR because you centralized git parsing to fix exactly this class of bug for-Cand documented the paired lists incommand_prefix.go.Root cause. The value-consuming global list was copied from the
-Cfix without including other git globals that take a separate operand (--pager, and likely--config/-cif not already covered by joined forms).Guidance. Add
--pagertogitGlobalOptionConsumesValueand mirror it ingitOptionConsumesValue. ExtendTestGitGlobalOptionsResolveIdenticallyOnBothPathswith{"--pager less push origin main", true}on both AST and& rem 'fallback paths. Consider auditing git's global options for other separated-value globals so this does not recur option by option. -
[P2] Do not treat post-tree pathspecs as
--remoteongit archive
internal/sandbox/analyzer.go:311What happens.
gitTargetsRemoteArchivescans every argument and flags any--remote/--remote=…token. Git only treats--remoteas a global option before the tree-ish; after the tree-ish, operands are pathspecs.git archive HEAD --remotearchives a tree entry named--remotelocally.git archive HEAD --remote=originis likewise a pathspec, notorigin.Verified on head
c457b9e.git archive HEAD --remoteandgit archive HEAD --remote=origin→network+RiskCriticalon both AST and fallback.git archive HEAD -- --remotecorrectly stays non-network (you fixed the--case).git archive --remote=origin HEADcorrectly stays network.Regression status. Introduced by this PR. Base
mainclassified everyarchiveas network (over-broad). This PR narrowed correctly but applied--remotedetection globally instead of only in pre-pathspec positions.Root cause. The comment at line 304 says "The whole argument list is scanned rather than only the part after the subcommand, so option order cannot hide it" — that rule is correct for global options but wrong once the tree-ish and pathspec operands begin. You already implemented the
--end-of-options rule; the missing piece is stopping the scan after the archive operand layout is consumed.Guidance. Walk the
archiveargument list positionally: skip globals, confirm subcommandarchive, consume the tree-ish (and any archive-specific options like-o), then treat remaining tokens as pathspecs unless a--separator appears. Only scan pre-pathspec positions for--remote. Add toTestClassifyUnparseableLocalGitArchiveStaysNonNetwork:git archive HEAD --remote & rem 'git archive HEAD --remote=origin & rem '
Expect nonetworkcategory on both paths. Keep existing remote-archive positives (git archive --remote=origin HEAD).
-
[P2] Preserve the network gate for CMD
STARTtitle forms the fallback still mishandles
internal/sandbox/risk.go:351,internal/sandbox/risk.go:370,internal/sandbox/risk.go:187What happens. Two
STARTshapes dropnetworkon the unparseable fallback path:-
Unquoted single-word title —
start MyTitle curl https://evil.test & rem '. CMD treatsMyTitleas the window title andcurlas the program.cmdStartPayloadonly skips a title when it contains whitespace (strings.ContainsAny(fields[0], " \t")) or, on the token path, whenfields[0].quotedis true. A one-word unquoted title is misread as the program. -
Empty quoted title + caret obfuscation —
start "" c^u^r^l https://evil.test & rem '. After"",fallbackCommandTokenInfoleaveswordQuoted=trueon the next token.cmdStartPayloadTokenInfoonly skips quoted titles, soc^u^r^lis treated as the title. The string-based CMD path (fallbackPayloadUsesNetwork→fallbackCommandTokenInfo) never runsnormalizeCMDToken, so carets are not stripped.
Verified on head
c457b9e. Both return[shell, unparseable_command]only;Engine.Evaluate→ActionAllowwhen shell permission is granted.start "x" curl …andstart "" curl …(without carets) work — tests inrisk_hardening_test.goandengine_test.gocover quoted titles but not these two shapes.Regression status. Introduced by this PR. Base
mainclassified both asnetworkvia the whole-string\bcurl\bfallback regex.Root cause.
STARTtitle detection was modeled on "quoted or contains whitespace" instead of CMD's actual rule: the first operand is always the title when a second operand exists (unless it's a/switch). The token and string resolver paths also diverge on caret normalization.Guidance.
- In
cmdStartPayload/cmdStartPayloadTokenInfo: when one or more operands remain after/switches, treat the first operand as the title if a second operand exists (conservative and matches CMD). Align with the existingfallbackPayloadUsesNetworkconservatism forstartthat already triesfields[1:]andfields[2:]. - Reset
wordQuotedto false when a closing quote produces an empty token (""). - Run
normalizeCMDTokenon tokens infallbackPayloadUsesNetwork/ the string CMD path, or normalize the interpreter payload beforefallbackCommandTokenInfo. - Add regressions beside the existing
start "x" curlcases inrisk_hardening_test.goandengine_test.go.
-
-
[P2] Normalize git token casing inside the shared parser
internal/sandbox/analyzer.go:370What happens. The AST path lowercases git arguments via
literalWordTextsbeforegitUsesNetwork. The unparseable fallback passes raw tokenizer output intomatchesUnparseableGitNetwork→parseGitInvocation→gitUsesNetwork. Mixed-case obfuscation therefore diverges across paths:git PUSH origin main(parseable) →networkon AST.git PUSH origin main & rem '(fallback) →unparseable_commandonly, nonetwork.git --Git-Dir repo push origin main & rem '→repomisread as subcommand becausegitGlobalOptionConsumesValueis exact-case on--Git-Dir.
Verified on head
c457b9e. Uppercase fallback cases above missnetworkandEngine.Evaluateallows. Parseable uppercasegit PUSHworks on head (improvement over needing exact case).Regression status. Fallback uppercase miss is pre-existing on base
main(samegit PUSH & rem 'behavior). The AST/fallback asymmetry is new because this PR added lowercasing on the AST path only.Root cause. Normalization happens in callers instead of inside
parseGitInvocation, so shared logic behaves differently depending on which path invoked it.Guidance. Lowercase (or otherwise normalize) each token at the top of
parseGitInvocationbefore terminal-global, value-consuming, and subcommand detection — including option names passed togitGlobalOptionConsumesValue. ExtendTestGitGlobalOptionsResolveIdenticallyOnBothPathswith uppercase/mixed-case verbs and globals on both paths. This is one function change that fixes both paths permanently. -
[P2] Honor CMD switches after
/c//kbefore extracting the payload
internal/sandbox/risk.go:473What happens.
fallbackCommandInterpreterPayloadreturnsstrings.Join(args[index+1:], " ")immediately after the first/cor/kwithout consuming intervening CMD switches. Forcmd /c /d curl https://evil.test & rem ', the payload becomes/d curl …. The CMD resolver treats/das the program and never seescurl.Verified on head
c457b9e.Classify→[shell, unparseable_command];Engine.Evaluate→ActionAllowwith shell permission granted.cmd /d /c curl …is covered and works;/c /dordering is not.Regression status. Introduced by this PR. Base
maincaughtcurlvia whole-string regex.Root cause. Payload extraction assumes the token after
/cis command text. CMD permits switches between/cand the command (cmd /c /d /s /q curl).Guidance. After locating
/cor/k, advance past known CMD switches (/d,/s,/q,/e:…,/u,/a, etc.) before returning the payload — mirror howcmdStartPayloadskips/switches. Alternatively, return the tail and run it throughfallbackPayloadUsesNetwork/cmdCommandBodyFieldsinstead of treating the first post-/ctoken as the executable. Addcmd /c /d curl … & rem 'torisk_hardening_test.goandengine_test.goalongside the existing/d /ccase. -
[P3] Recurse into PowerShell
-EncodedCommand/-Filepayloads on the fallback path
internal/sandbox/risk.go:473What happens.
fallbackCommandInterpreterPayloadhandlescmd /c|/kandpowershell|pwsh -c|-command, but not-EncodedCommand,-enc,-f, or-File.powershell -EncodedCommand curl & rem 'is POSIX-unparseable yet returns only[shell, unparseable_command];Engine.Evaluateallows when shell permission is granted.Verified on head
c457b9e. Nonetworkcategory. (Literalcurlin the probe is unrealistic for real base64 payloads; the gap is that no payload is extracted at all.)Regression status. Introduced by this PR for this spelling. Base
mainclassified it asnetworkvia regex.Root cause. Interpreter payload extraction was implemented for the common
-ccase only; PowerShell's other command-carrying flags were not wired through.Guidance. Extend
fallbackCommandInterpreterPayload(andfallbackCommandInterpreterArgsif needed) to recognize-EncodedCommand/-encand-File/-f, returning the following argument as payload. For-EncodedCommand, you cannot decode base64 here — either recurse the raw next token conservatively (if it contains known network program names after decoding is impractical) or classify the outerpowershell/pwshinvocation as network when only encoded/file flags are present and no safer inner parse exists. Add a fallback-path regression; decoding accuracy is not required for this gate, coverage is.
Amp-Thread-ID: https://ampcode.com/threads/T-019fd69c-90f8-77d7-9883-d1f2efa22c93 Co-authored-by: Amp <amp@ampcode.com>
|
Addressed the reproducible findings in e887d2b. PierrunoYT updated the fallback classification to:
Regression coverage was added at both Two findings were not implemented because direct verification with Git 2.55.0 disproved the stated command grammar:
Validation passed:
The advisory lint and vulnerability tools remain blocked by the repository/toolchain mismatch: the pinned tools build with Go 1.25 but this branch targets Go 1.26.5. |
|
CI follow-up: all three platform smoke/test jobs pass on e887d2b (Ubuntu, macOS, and Windows). The remaining failed PierrunoYT attempted to rerun the failed job, but GitHub requires repository admin permission for that action. A maintainer can rerun the failed job once the upstream vulnerability service is available. |
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.
Rechecked on head e887d2b. The core classification work is real and well-covered: parseable git push, git -C repo push, git PUSH, and the unparseable git push … & rem ' path all reach ReasonNetworkBlocked when shell permission is already granted. The problems below are gaps in the same surfaces this PR expanded — mostly regressions from replacing base main's whole-string \bcurl\b/\bwget\b fallback with structured resolvers that are not yet at parity.
The PR is mergeable but still blocked. Security & code health remains red; the author attributes that to a govulncheck HTTP 403 from vuln.go.dev. Please confirm whether that required check is still failing for the same infra reason.
Findings
Must fix before merge
-
[P1] Do not miss remote
git archivewhen-otakes--as its operand
internal/sandbox/analyzer.go:306(gitTargetsRemoteArchive)
git archive -o -- --remote=origin HEADis a real remote archive (verified: git runsupload-archiveagainstorigin). The scanner treats the-ovalue--as end-of-options and returns non-network, soEngine.EvaluatewithNetworkDeny+PermissionGrantedreturnsActionAllow. Walk archive argv with value-taking options (-o/--output/--prefix/--mtime/ etc.) and only stop at--when it is not an option operand. Add AST and fallback regressions for this remote-positive case. -
[P1] Restore fail-closed network detection on the unparseable fallback path
internal/sandbox/risk.go(fallbackBodyUsesNetwork,fallbackPowerShellPayload,cmdCommandBodyFields,fallbackEnvSplitPayload,isShellCommandFlag/fallbackDashCPayload)
On head, all of the following evaluate toActionAllowunderNetworkDeny+PermissionGranted. Basemain'sunparseableNetworkPatternmatched every one via\bcurl\b/\bwget\banywhere in the string.PowerShell (Windows-primary):
powershell /Command curl … & rem 'powershell curl … & rem 'andpowershell -NoProfile curl …powershell -Command:curl …/-EncodedCommand:…joined formspowershell -Command Invoke-WebRequest|iwr|irm …(payload is extracted, but fallback pattern omits cmdlets AST already lists innetworkPrograms)
Shell launchers:
bash -lc 'curl …'andbash -ce 'curl …'(parseable and unparseable;bash -x -c '…'works)env -S 'curl …'on the AST path (wrapper consumes the payload; unparseable spaced form prompts)env -S'curl …' && "unterminated"andenv --split-string='curl …' && "unterminated"(joined GNU forms; verified)
CMD (unparseable):
@@curl … & rem ',call @curl … & rem 'for /f %i in ('curl …') do echo %i & rem '(FOR /F IN ('cmd')executes the IN body)cmd /c"curl …" & rem '(glued/c"…")git pus^h … & rem ',git archive --rem^ote=origin … & rem '(caret normalization stops at program token)
Other prefixes base caught:
busybox wget … && "unterminated",strace curl … && "unterminated"
Guidance: treat this as one parity problem, not ten one-off token fixes. When structured resolution cannot prove the program is local, keep a conservative fail-closed path (or recurse into the same launcher/wrapper shapes base implicitly covered). Accept
-and/PowerShell flags, joinedenv -Sforms, clustered-lc/-ce, CMD@stripping afterCALL,FOR /FIN bodies, glued/c, and caret-normalized git tokens. AddEngine.Evaluateregressions for representative cases in each bucket.
Should fix in this PR or an immediate follow-up
-
[P2] Stop false-positive network prompts for local
git archiveoutput paths
internal/sandbox/analyzer.go:306(gitTargetsRemoteArchive)
git archive -o --remote HEADcreates a local tar file named--remote(verified) but classifies as network.git -C --remote archive HEADis similarly wrong. These are extra prompts, not silent allows, but they contradict this PR's local-archive contract. Model value-taking globals and archive options so--remotein an operand position is not read as the remote flag. Add negative regressions alongside the P1 remote-positive case. -
[P3] Correct the false
bash/sh--commandcontract in comments/tests
internal/sandbox/analyzer.go/internal/sandbox/risk.gocomments;analyzer_test/risk_hardening_test
bash --commandandsh --commandare rejected by the shells (invalid/illegal option). Tests and comments treat--commandas a-csynonym. This is not a typical production bypass, but the contract is wrong and should not be pinned as regression coverage.
Scope / claims (not implementation bugs in this diff)
- [P2] Do not claim
Fixes #703from classification changes alone
Issue #703 reports that after approving a network prompt onmacos-seatbelt, the sandbox stayed deny (context canceled). Basemainalready classifies plaingit pushas network viagitUsesNetwork; this PR's production diff is classification/fallback only and does not changeGrantRequestPermissions/ turn-grant wiring inloop.go. The renamed loop test usesBackendUnavailableand checks fake stdout, notNetworkAllowon the execution profile. Either land and test the grant-application path #703 describes on a wrapping backend, or scope the PR to classification and link #703 without closing it.
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.