Skip to content

Commit d54e971

Browse files
PierrunoYTclaude
andcommitted
fix(sandbox): recognize --attr-source and widen the unparseable git fallback
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 #726. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 595c292 commit d54e971

5 files changed

Lines changed: 22 additions & 4 deletions

File tree

‎internal/agent/command_prefix.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -414,15 +414,16 @@ func gitSubcommand(command []string) (int, string, bool) {
414414

415415
func gitOptionConsumesValue(arg string) bool {
416416
switch arg {
417-
case "-C", "-c", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree":
417+
case "-C", "-c", "--attr-source", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree":
418418
return true
419419
default:
420420
return false
421421
}
422422
}
423423

424424
func gitOptionHasInlineValue(arg string) bool {
425-
return strings.HasPrefix(arg, "--config-env=") ||
425+
return strings.HasPrefix(arg, "--attr-source=") ||
426+
strings.HasPrefix(arg, "--config-env=") ||
426427
strings.HasPrefix(arg, "--exec-path=") ||
427428
strings.HasPrefix(arg, "--git-dir=") ||
428429
strings.HasPrefix(arg, "--namespace=") ||

‎internal/sandbox/analyzer.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ func gitSubcommand(words []string) string {
305305
// SEPARATE token (mirrors gitOptionConsumesValue in internal/agent).
306306
func gitGlobalOptionConsumesValue(option string) bool {
307307
switch option {
308-
case "-C", "-c", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree":
308+
case "-C", "-c", "--attr-source", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree":
309309
return true
310310
default:
311311
return false

‎internal/sandbox/analyzer_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ func TestAnalyzeCommand(t *testing.T) {
7474
{name: "git -c config push", script: "git -c http.sslVerify=false push origin main", network: true},
7575
{name: "git --git-dir fetch", script: "git --git-dir /repo/.git fetch origin", network: true},
7676
{name: "git --work-tree pull", script: "git --work-tree /repo pull origin main", network: true},
77+
{name: "git --attr-source push", script: "git --attr-source HEAD push origin main", network: true},
7778
{name: "git -C local commit", script: `git -C repo commit -m "local change"`, network: false},
7879
{name: "git.exe push", script: "git.exe push origin main", network: true},
7980
{name: "git.exe local commit", script: `git.exe commit -m "local change"`, network: false},

‎internal/sandbox/risk.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,15 @@ var (
3333
// unparseableNetworkPattern is used only after the shell parser fails. At
3434
// that point the command is already marked too complex, so this intentionally
3535
// favors catching obvious network programs over proving exact shell syntax.
36-
unparseableNetworkPattern = regexp.MustCompile(`(?i)\b(curl|wget|fetch|aria2c|ssh|scp|sftp|rsync|nc|ncat|netcat|telnet|ftp|npx|http-server|vite|next|nuxt|astro)\b|\b(npm|pnpm|yarn|bun|pip|pip2|pip3)\s+(install|add|publish|login|start|serve|dev|preview|run\s+(start|serve|dev|preview)|exec|x|dlx)\b|\bgo\s+get\b|\bgit(?:\s+[^\s;&|]+){0,8}\s+(clone|fetch|pull|push)\b|\bpython(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|\bgh\s+(api|repo\s+clone|release\s+download)\b`)
36+
// The git branch matches an optional .exe suffix (a parser-failing Windows
37+
// command like `git.exe push origin main & rem '` runs under cmd.exe and
38+
// must still classify as network) and scans an unbounded run of
39+
// non-separator tokens before the subcommand rather than capping at a
40+
// fixed count — Go's regexp is RE2-backed (linear time, no backtracking
41+
// blowup), so nothing is gained by capping, and a git invocation with more
42+
// than a handful of value-taking global options would otherwise silently
43+
// fall through uncapped.
44+
unparseableNetworkPattern = regexp.MustCompile(`(?i)\b(curl|wget|fetch|aria2c|ssh|scp|sftp|rsync|nc|ncat|netcat|telnet|ftp|npx|http-server|vite|next|nuxt|astro)\b|\b(npm|pnpm|yarn|bun|pip|pip2|pip3)\s+(install|add|publish|login|start|serve|dev|preview|run\s+(start|serve|dev|preview)|exec|x|dlx)\b|\bgo\s+get\b|\bgit(?:\.exe)?(?:\s+[^\s;&|]+)*\s+(clone|fetch|pull|push)\b|\bpython(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|\bgh\s+(api|repo\s+clone|release\s+download)\b`)
3745
// destructiveExtraPatterns hold high-severity patterns that the legacy
3846
// destructiveCommandPattern does not already cover. Folded in from the
3947
// blueprint safe_bash.go without duplicating existing matches.

‎internal/sandbox/risk_hardening_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,14 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) {
340340
`git pull origin main && "unterminated`,
341341
`git push gitlawb://example.com/repo.git main && "unterminated`,
342342
`git -C repo push gitlawb://example.com/repo.git main && "unterminated`,
343+
// git.exe runs under cmd.exe, which has no notion of a trailing single
344+
// quote — this parses fine there but fails the POSIX shell parser used
345+
// by AnalyzeCommand, so it must still be caught by the regex fallback.
346+
`git.exe push origin main & rem '`,
347+
// More value-taking global options than the fallback regex used to cap
348+
// its generic-token scan at (formerly {0,8}) — every option here still
349+
// precedes the actual subcommand.
350+
`git -c a=1 -c b=2 -c c=3 -c d=4 -c e=5 push gitlawb://example.com/repo.git main && "unterminated`,
343351
} {
344352
t.Run(command, func(t *testing.T) {
345353
risk := classifyCommand(command)

0 commit comments

Comments
 (0)