From 7a2294582939ecba4097a0660986d7d916703f40 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 18 Jul 2026 15:52:31 +0200 Subject: [PATCH 01/10] fix(sandbox): classify git push as network access --- internal/sandbox/analyzer.go | 7 ++++++- internal/sandbox/analyzer_test.go | 2 ++ internal/sandbox/risk_hardening_test.go | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 9509c5a81..400d0607f 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -150,7 +150,12 @@ func commandUsesNetwork(prog string, args []*syntax.Word) bool { 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 + } case "gh": return ghUsesNetwork(words) default: diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index 1154922b4..070cadc3e 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -56,6 +56,8 @@ func TestAnalyzeCommand(t *testing.T) { {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}, + {name: "git local commit", script: `git commit -m "local change"`, network: false}, {name: "gh release download", script: "gh release download v1.0.0", network: true}, {name: "no network", script: "ls -la && echo done", network: false}, {name: "process pattern is not network", script: `pkill -f "python3 -m http.server 8000"`, network: false}, diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index ce2f89994..b105d4b98 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -273,6 +273,7 @@ func TestClassifyASTCatchesNetworkProgramsRegexMisses(t *testing.T) { "ftp ftp.example.com", "sftp user@host", "sudo telnet example.com 23", + "git push gitlawb main", } { risk := classifyCommand(command) if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { From d72a19229d21ed94336f59c90880c14d05278195 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 18 Jul 2026 22:14:33 +0200 Subject: [PATCH 02/10] fix(sandbox): harden git network classification --- internal/agent/loop_test.go | 18 +++++++++--------- internal/sandbox/analyzer.go | 2 +- internal/sandbox/analyzer_test.go | 4 +++- internal/sandbox/risk.go | 2 +- internal/sandbox/risk_hardening_test.go | 23 ++++++++++++++++------- 5 files changed, 30 insertions(+), 19 deletions(-) diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index 485918f56..5031158d3 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -2308,18 +2308,18 @@ func TestRunPersistentCommandPrefixStillPromptsForNetwork(t *testing.T) { } } -func TestRunApprovedNetworkBashPromptAppliesTurnNetworkGrant(t *testing.T) { +func TestRunApprovedGitPushPromptAppliesTurnNetworkGrant(t *testing.T) { root := t.TempDir() - command := "PATH=.:$PATH curl https://example.com" + command := "PATH=.:$PATH git push gitlawb://example.com/repo.git main" if runtime.GOOS == "windows" { - command = "set PATH=.;%PATH% && curl https://example.com" - fakeCurl := filepath.Join(root, "curl.cmd") - if err := os.WriteFile(fakeCurl, []byte("@echo fake curl %*\r\n"), 0o755); err != nil { + command = "set PATH=.;%PATH% && git push gitlawb://example.com/repo.git main" + fakeGit := filepath.Join(root, "git.cmd") + if err := os.WriteFile(fakeGit, []byte("@echo fake git %*\r\n"), 0o755); err != nil { t.Fatal(err) } } else { - fakeCurl := filepath.Join(root, "curl") - if err := os.WriteFile(fakeCurl, []byte("#!/bin/sh\necho fake curl \"$@\"\n"), 0o755); err != nil { + fakeGit := filepath.Join(root, "git") + if err := os.WriteFile(fakeGit, []byte("#!/bin/sh\necho fake git \"$@\"\n"), 0o755); err != nil { t.Fatal(err) } } @@ -2341,7 +2341,7 @@ func TestRunApprovedNetworkBashPromptAppliesTurnNetworkGrant(t *testing.T) { } var requests []PermissionRequest var events []PermissionEvent - result, err := Run(context.Background(), "curl", provider, Options{ + result, err := Run(context.Background(), "push changes", provider, Options{ Registry: registry, PermissionMode: PermissionModeAsk, Autonomy: "medium", @@ -2383,7 +2383,7 @@ func TestRunApprovedNetworkBashPromptAppliesTurnNetworkGrant(t *testing.T) { t.Fatalf("expected tool result to be sent back to provider, got %d requests", len(provider.requests)) } lastMessage := provider.requests[1].Messages[len(provider.requests[1].Messages)-1] - if !strings.Contains(lastMessage.Content, "fake curl https://example.com") { + if !strings.Contains(lastMessage.Content, "fake git push gitlawb://example.com/repo.git main") { t.Fatalf("expected approved network command output after degraded execution, got %q", lastMessage.Content) } } diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 400d0607f..aeb93f058 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -151,7 +151,7 @@ func commandUsesNetwork(prog string, args []*syntax.Word) bool { return firstSubcommand(words, nil) == "get" case "git": switch firstSubcommand(words, nil) { - case "clone", "push": + case "clone", "fetch", "pull", "push": return true default: return false diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index 070cadc3e..2ce0f6adc 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -56,7 +56,9 @@ func TestAnalyzeCommand(t *testing.T) { {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}, + {name: "git fetch", script: "git fetch origin", network: true}, + {name: "git pull", script: "git pull origin main", network: true}, + {name: "git push custom transport", script: "git push gitlawb://example.com/repo.git main", network: true}, {name: "git local commit", script: `git commit -m "local change"`, network: false}, {name: "gh release download", script: "gh release download v1.0.0", network: true}, {name: "no network", script: "ls -la && echo done", network: false}, diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index a0e784f67..a383d3760 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -33,7 +33,7 @@ var ( // unparseableNetworkPattern is used only after the shell parser fails. At // that point the command is already marked too complex, so this intentionally // favors catching obvious network programs over proving exact shell syntax. - 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+clone\b|\bpython(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|\bgh\s+(api|repo\s+clone|release\s+download)\b`) + 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+(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`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index b105d4b98..c407773ee 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -273,7 +273,9 @@ func TestClassifyASTCatchesNetworkProgramsRegexMisses(t *testing.T) { "ftp ftp.example.com", "sftp user@host", "sudo telnet example.com 23", - "git push gitlawb main", + "git fetch origin", + "git pull origin main", + "git push gitlawb://example.com/repo.git main", } { risk := classifyCommand(command) if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { @@ -292,12 +294,19 @@ func TestClassifyFlagsUnparseableCommand(t *testing.T) { } func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { - risk := classifyCommand(`curl https://example.com && "unterminated`) - if !HasRiskCategory(risk, "unparseable_command") { - t.Fatalf("Classify of unparseable network command = categories %v; want unparseable_command", risk.Categories) - } - if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { - t.Fatalf("Classify of unparseable network command = level %s, categories %v; want critical network", risk.Level, risk.Categories) + 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) + } + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Fatalf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } } } From 5a7c67265e7262a1829a92fb396b950b01fe936f Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 19 Jul 2026 16:12:33 +0200 Subject: [PATCH 03/10] fix(sandbox): handle git options in network fallback --- internal/sandbox/risk.go | 2 +- internal/sandbox/risk_hardening_test.go | 17 ++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index a383d3760..256171706 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -33,7 +33,7 @@ var ( // unparseableNetworkPattern is used only after the shell parser fails. At // that point the command is already marked too complex, so this intentionally // favors catching obvious network programs over proving exact shell syntax. - 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+(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`) + 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`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index c407773ee..916785579 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -299,14 +299,17 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { `git fetch origin && "unterminated`, `git pull origin main && "unterminated`, `git push gitlawb://example.com/repo.git main && "unterminated`, + `git -C repo 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) - } - if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { - t.Fatalf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) - } + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) } } From d54e97130374d2d3cf5fce108ffbc7bd57c701ab Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 23:31:22 +0200 Subject: [PATCH 04/10] fix(sandbox): recognize --attr-source and widen the unparseable git fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/agent/command_prefix.go | 5 +++-- internal/sandbox/analyzer.go | 2 +- internal/sandbox/analyzer_test.go | 1 + internal/sandbox/risk.go | 10 +++++++++- internal/sandbox/risk_hardening_test.go | 8 ++++++++ 5 files changed, 22 insertions(+), 4 deletions(-) diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 2d0f5a606..dffc472a7 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -414,7 +414,7 @@ func gitSubcommand(command []string) (int, string, bool) { func gitOptionConsumesValue(arg string) bool { switch arg { - case "-C", "-c", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree": + case "-C", "-c", "--attr-source", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree": return true default: return false @@ -422,7 +422,8 @@ func gitOptionConsumesValue(arg string) bool { } func gitOptionHasInlineValue(arg string) bool { - return strings.HasPrefix(arg, "--config-env=") || + return strings.HasPrefix(arg, "--attr-source=") || + strings.HasPrefix(arg, "--config-env=") || strings.HasPrefix(arg, "--exec-path=") || strings.HasPrefix(arg, "--git-dir=") || strings.HasPrefix(arg, "--namespace=") || diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 4f24b3fd1..cb328d134 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -305,7 +305,7 @@ func gitSubcommand(words []string) string { // SEPARATE token (mirrors gitOptionConsumesValue in internal/agent). func gitGlobalOptionConsumesValue(option string) bool { switch option { - case "-C", "-c", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree": + case "-C", "-c", "--attr-source", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree": return true default: return false diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index 78d13ed8a..49bf4242b 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -74,6 +74,7 @@ func TestAnalyzeCommand(t *testing.T) { {name: "git -c config push", script: "git -c http.sslVerify=false push origin main", network: true}, {name: "git --git-dir fetch", script: "git --git-dir /repo/.git fetch origin", network: true}, {name: "git --work-tree pull", script: "git --work-tree /repo pull origin main", network: true}, + {name: "git --attr-source push", script: "git --attr-source HEAD push origin main", network: true}, {name: "git -C local commit", script: `git -C repo commit -m "local change"`, network: false}, {name: "git.exe push", script: "git.exe push origin main", network: true}, {name: "git.exe local commit", script: `git.exe commit -m "local change"`, network: false}, diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 256171706..7e684ec29 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -33,7 +33,15 @@ var ( // unparseableNetworkPattern is used only after the shell parser fails. At // that point the command is already marked too complex, so this intentionally // favors catching obvious network programs over proving exact shell syntax. - 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`) + // The git branch matches an optional .exe suffix (a parser-failing Windows + // command like `git.exe push origin main & rem '` runs under cmd.exe and + // must still classify as network) and scans an unbounded run of + // non-separator tokens before the subcommand rather than capping at a + // fixed count — Go's regexp is RE2-backed (linear time, no backtracking + // blowup), so nothing is gained by capping, and a git invocation with more + // than a handful of value-taking global options would otherwise silently + // fall through uncapped. + 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`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 0eff6eb18..340cec7fd 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -340,6 +340,14 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { `git pull origin main && "unterminated`, `git push gitlawb://example.com/repo.git main && "unterminated`, `git -C repo push gitlawb://example.com/repo.git main && "unterminated`, + // git.exe runs under cmd.exe, which has no notion of a trailing single + // quote — this parses fine there but fails the POSIX shell parser used + // by AnalyzeCommand, so it must still be caught by the regex fallback. + `git.exe push origin main & rem '`, + // More value-taking global options than the fallback regex used to cap + // its generic-token scan at (formerly {0,8}) — every option here still + // precedes the actual subcommand. + `git -c a=1 -c b=2 -c c=3 -c d=4 -c e=5 push gitlawb://example.com/repo.git main && "unterminated`, } { t.Run(command, func(t *testing.T) { risk := classifyCommand(command) From 708c2c241921e2814f41c367b335b9979d481dd9 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 16:13:10 +0200 Subject: [PATCH 05/10] fix(sandbox): restrict git fallback regex to recognized global options The unparseable-command regex fallback skipped any run of non-separator tokens before the git subcommand verb, so an arbitrary bare token (e.g. "status" in `git status push`, where push is only a pathspec) was mistaken for a git global option. This misclassified local-only commands as critical network on the unparseable fallback path. Restrict the skipped span to git's actual global options, mirroring gitGlobalOptionConsumesValue in analyzer.go: recognized value-taking options (-C, -c, --attr-source, --config-env, --exec-path, --git-dir, --namespace, --super-prefix, --work-tree) consume one following token, and any other dash-prefixed token (including joined --opt=value forms) is skipped on its own. A bare non-option token still halts the scan. Co-Authored-By: Claude Sonnet 5 --- internal/sandbox/risk.go | 20 +++++++++++++------- internal/sandbox/risk_hardening_test.go | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 7e684ec29..e0ddf734a 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -35,13 +35,19 @@ var ( // favors catching obvious network programs over proving exact shell syntax. // The git branch matches an optional .exe suffix (a parser-failing Windows // command like `git.exe push origin main & rem '` runs under cmd.exe and - // must still classify as network) and scans an unbounded run of - // non-separator tokens before the subcommand rather than capping at a - // fixed count — Go's regexp is RE2-backed (linear time, no backtracking - // blowup), so nothing is gained by capping, and a git invocation with more - // than a handful of value-taking global options would otherwise silently - // fall through uncapped. - 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`) + // must still classify as network) and, before the subcommand, skips only + // git's own global options — mirroring gitGlobalOptionConsumesValue in + // analyzer.go: a recognized value-taking option (-C, -c, --attr-source, + // --config-env, --exec-path, --git-dir, --namespace, --super-prefix, + // --work-tree) consumes one following token as its value, and any other + // dash-prefixed token (including a joined `--git-dir=/x` form) is skipped + // on its own. This span is unbounded — Go's regexp is RE2-backed (linear + // time, no backtracking blowup) — so a git invocation with any number of + // global options still reaches its subcommand. Critically, a bare + // non-option token (e.g. the "status" in `git status push`, where "push" + // is only a pathspec) does NOT match either alternative, so the scan stops + // and the command is correctly left unclassified as network. + 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+(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)\s+[^\s;&|]+|\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`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 340cec7fd..69c84cf6f 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -361,6 +361,28 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { } } +// TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork guards against the +// fallback regex treating an arbitrary bare token before a network verb as if +// it were a git global option. `status` in `git status push` is a pathspec +// argument to `git status`, not a value-taking global option, so `push` here +// is not the git subcommand and must not be classified as network — even +// though the trailing unmatched quote (a cmd.exe REM comment, invalid under +// the POSIX parser AnalyzeCommand uses) still forces the unparseable-command +// fallback path. +func TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork(t *testing.T) { + command := `git status push & rem '` + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Fatalf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if HasRiskCategory(risk, "network") { + t.Fatalf("Classify(%q) = level %s, categories %v; want no network category", command, risk.Level, risk.Categories) + } + if risk.Level != RiskHigh { + t.Fatalf("Classify(%q) = level %s; want high (unparseable_command only)", command, risk.Level) + } +} + func TestClassifyASTDoesNotFlagQuotedProgramName(t *testing.T) { // A program name inside a quoted argument is not a command, so the AST // analyzer must not flag it (documenting a destructive command in an echo). From f5ad5dee37a2ee6276e8f444dc09e9674d099325 Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 20:42:00 +0000 Subject: [PATCH 06/10] fix(sandbox): preserve quoted git fallback tokens Co-authored-by: Pierre Bruno --- internal/sandbox/risk.go | 20 +++++++++++--------- internal/sandbox/risk_hardening_test.go | 4 ++++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index e0ddf734a..28ac9ff54 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -39,15 +39,17 @@ var ( // git's own global options — mirroring gitGlobalOptionConsumesValue in // analyzer.go: a recognized value-taking option (-C, -c, --attr-source, // --config-env, --exec-path, --git-dir, --namespace, --super-prefix, - // --work-tree) consumes one following token as its value, and any other - // dash-prefixed token (including a joined `--git-dir=/x` form) is skipped - // on its own. This span is unbounded — Go's regexp is RE2-backed (linear - // time, no backtracking blowup) — so a git invocation with any number of - // global options still reaches its subcommand. Critically, a bare - // non-option token (e.g. the "status" in `git status push`, where "push" - // is only a pathspec) does NOT match either alternative, so the scan stops - // and the command is correctly left unclassified as network. - 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+(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)\s+[^\s;&|]+|\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`) + // --work-tree) consumes one following token as its value, including a + // single- or double-quoted value, and any other dash-prefixed token + // (including a joined `--git-dir=/x` form) is skipped on its own. The + // subcommand may also be quoted, as cmd.exe permits `git "push" ...`. + // This span is unbounded — Go's regexp is RE2-backed (linear time, no + // backtracking blowup) — so a git invocation with any number of global + // options still reaches its subcommand. Critically, a bare non-option token + // (e.g. the "status" in `git status push`, where "push" is only a pathspec) + // does NOT match either alternative, so the scan stops and the command is + // correctly left unclassified as network. + 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+(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)\s+(?:"[^"]*"|'[^']*'|[^\s;&|]+)|\s+-[^\s;&|]+)*\s+(?:"(?:clone|fetch|pull|push)"|'(?:clone|fetch|pull|push)'|(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`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 69c84cf6f..f62fbe0f5 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -344,6 +344,10 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { // quote — this parses fine there but fails the POSIX shell parser used // by AnalyzeCommand, so it must still be caught by the regex fallback. `git.exe push origin main & rem '`, + // cmd.exe accepts quoted option values and verbs. Preserve those token + // boundaries when the trailing REM quote forces the fallback path. + `git.exe -C "C:\Program Files\repo" push origin main & rem '`, + `git.exe -C "C:\Program Files\repo" "push" origin main & rem '`, // More value-taking global options than the fallback regex used to cap // its generic-token scan at (formerly {0,8}) — every option here still // precedes the actual subcommand. From 89c8e38cbbafa82969f323993e05792c7a439fb5 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 27 Jul 2026 21:22:50 +0000 Subject: [PATCH 07/10] fix(sandbox): complete Windows git fallback coverage Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-70fb-71ad-816d-7bf849ceb6c4 Co-authored-by: Pierre Bruno --- internal/agent/command_prefix_test.go | 11 +++++++ internal/sandbox/analyzer.go | 14 +-------- internal/sandbox/analyzer_test.go | 3 ++ internal/sandbox/risk.go | 18 ++++++----- internal/sandbox/risk_hardening_test.go | 42 ++++++++++++++++++------- 5 files changed, 55 insertions(+), 33 deletions(-) diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index 69508a67c..6a4ca30f7 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -35,6 +35,17 @@ func TestProposedCommandPrefixHonorsValidatedRequestedPrefix(t *testing.T) { } } +func TestSafeGitCommandConsumesAttrSourceOptionValue(t *testing.T) { + for _, command := range [][]string{ + {"git", "--attr-source", "HEAD", "status"}, + {"git", "--attr-source=HEAD", "status"}, + } { + if !safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = false; want true", command) + } + } +} + func TestProposedCommandPrefixSupportsSegmentedCommands(t *testing.T) { got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"}) if runtime.GOOS == "windows" { diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 082b79d76..6368eb1a3 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -453,7 +453,7 @@ func effectiveProgram(args []*syntax.Word) (string, []*syntax.Word) { if isNumericToken(text) { continue } - token := trimWindowsExecutableExtension(normalizeProgramToken(text)) + token := normalizeProgramToken(text) if wrapperPrograms[token] { wrapper = token continue @@ -463,18 +463,6 @@ func effectiveProgram(args []*syntax.Word) (string, []*syntax.Word) { return "", nil } -// trimWindowsExecutableExtension maps git.exe to git, curl.exe to curl, and so -// on, so a Windows spelling reaches the same classification as the bare name. -// Only ".exe" is trimmed: a .bat/.cmd of the same stem is a separate script, not -// the program it is named after, so classifying it as that program would be a -// guess. normalizeProgramToken has already lowercased the token. -func trimWindowsExecutableExtension(token string) string { - if trimmed := strings.TrimSuffix(token, ".exe"); trimmed != "" { - return trimmed - } - return token -} - // dashCPayload returns the literal text of the word following `-c` in an AST arg // list (the command a shell launcher will run), or "" when there is none. func dashCPayload(args []*syntax.Word) string { diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index c6185e613..daa9e11ad 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -86,6 +86,8 @@ func TestAnalyzeCommand(t *testing.T) { {name: "git fetch", script: "git fetch origin", network: true}, {name: "git pull", script: "git pull origin main", network: true}, {name: "git push custom transport", script: "git push gitlawb://example.com/repo.git main", network: true}, + {name: "git ls-remote", script: "git ls-remote origin", network: true}, + {name: "git remote archive", script: "git archive --remote=origin HEAD", network: true}, {name: "git status is offline", script: "git status", network: false}, {name: "git local commit", script: `git commit -m "local change"`, network: false}, // git's value-taking global options put their value in the NEXT token, so a @@ -98,6 +100,7 @@ func TestAnalyzeCommand(t *testing.T) { {name: "git --attr-source push", script: "git --attr-source HEAD push origin main", network: true}, {name: "git -C local commit", script: `git -C repo commit -m "local change"`, network: false}, {name: "git.exe push", script: "git.exe push origin main", network: true}, + {name: "git.cmd push", script: "git.cmd push origin main", network: true}, {name: "git.exe local commit", script: `git.exe commit -m "local change"`, network: false}, {name: "gh release download", script: "gh release download v1.0.0", network: true}, {name: "no network", script: "ls -la && echo done", network: false}, diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 28ac9ff54..aa879bcf4 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -33,23 +33,25 @@ var ( // unparseableNetworkPattern is used only after the shell parser fails. At // that point the command is already marked too complex, so this intentionally // favors catching obvious network programs over proving exact shell syntax. - // The git branch matches an optional .exe suffix (a parser-failing Windows - // command like `git.exe push origin main & rem '` runs under cmd.exe and - // must still classify as network) and, before the subcommand, skips only + // The git branch accepts Windows executable suffixes and a closing quote + // from a fully qualified path (a parser-failing command like + // `"C:\Program Files\Git\cmd\git.exe" push ... & rem '` runs under cmd.exe + // and must still classify as network). Before the subcommand, it skips only // git's own global options — mirroring gitGlobalOptionConsumesValue in // analyzer.go: a recognized value-taking option (-C, -c, --attr-source, // --config-env, --exec-path, --git-dir, --namespace, --super-prefix, - // --work-tree) consumes one following token as its value, including a - // single- or double-quoted value, and any other dash-prefixed token - // (including a joined `--git-dir=/x` form) is skipped on its own. The - // subcommand may also be quoted, as cmd.exe permits `git "push" ...`. + // --work-tree) consumes one following token as its value, and any other + // dash-prefixed token is skipped on its own. Both branches preserve quoted + // fragments, including spaces in separate or joined option values such as + // `-C "C:\Program Files\repo"` and `--git-dir="C:\Program Files\repo\.git"`. + // The subcommand may also be quoted, as cmd.exe permits `git "push" ...`. // This span is unbounded — Go's regexp is RE2-backed (linear time, no // backtracking blowup) — so a git invocation with any number of global // options still reaches its subcommand. Critically, a bare non-option token // (e.g. the "status" in `git status push`, where "push" is only a pathspec) // does NOT match either alternative, so the scan stops and the command is // correctly left unclassified as network. - 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+(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)\s+(?:"[^"]*"|'[^']*'|[^\s;&|]+)|\s+-[^\s;&|]+)*\s+(?:"(?:clone|fetch|pull|push)"|'(?:clone|fetch|pull|push)'|(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`) + 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|cmd|bat|com))?["']?(?:\s+(?:(?:(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)|"(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)"|'(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)')\s+(?:[^\s;&|"']|"[^"]*"|'[^']*')+|(?:-(?:[^\s;&|"']|"[^"]*"|'[^']*')+|"-[^"]*"|'-[^']*')))*\s+(?:"(?:clone|fetch|pull|push|ls-remote|archive)"|'(?:clone|fetch|pull|push|ls-remote|archive)'|(clone|fetch|pull|push|ls-remote|archive)\b)|\bpython(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|\bgh\s+(api|repo\s+clone|release\s+download)\b`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index f62fbe0f5..bb2829d4a 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -339,15 +339,23 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { `git fetch origin && "unterminated`, `git pull origin main && "unterminated`, `git push gitlawb://example.com/repo.git main && "unterminated`, + `git ls-remote gitlawb://example.com/repo.git & rem '`, + `git archive --remote=gitlawb://example.com/repo.git HEAD & rem '`, `git -C repo push gitlawb://example.com/repo.git main && "unterminated`, // git.exe runs under cmd.exe, which has no notion of a trailing single // quote — this parses fine there but fails the POSIX shell parser used // by AnalyzeCommand, so it must still be caught by the regex fallback. `git.exe push origin main & rem '`, - // cmd.exe accepts quoted option values and verbs. Preserve those token - // boundaries when the trailing REM quote forces the fallback path. + `git.cmd push origin main & rem '`, + // cmd.exe accepts quoted executable paths, option values, and verbs. + // Preserve those token boundaries when the trailing REM quote forces the + // fallback path, including joined short and long option-value forms. + `"C:\Program Files\Git\cmd\git.exe" "push" origin main & rem '`, `git.exe -C "C:\Program Files\repo" push origin main & rem '`, `git.exe -C "C:\Program Files\repo" "push" origin main & rem '`, + `git.exe -C"C:\Program Files\repo" "push" origin main & rem '`, + `git.exe --git-dir="C:\Program Files\repo\.git" push origin main & rem '`, + `git.exe "--git-dir=C:\Program Files\repo\.git" push origin main & rem '`, // More value-taking global options than the fallback regex used to cap // its generic-token scan at (formerly {0,8}) — every option here still // precedes the actual subcommand. @@ -374,16 +382,26 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { // the POSIX parser AnalyzeCommand uses) still forces the unparseable-command // fallback path. func TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork(t *testing.T) { - command := `git status push & rem '` - risk := classifyCommand(command) - if !HasRiskCategory(risk, "unparseable_command") { - t.Fatalf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) - } - if HasRiskCategory(risk, "network") { - t.Fatalf("Classify(%q) = level %s, categories %v; want no network category", command, risk.Level, risk.Categories) - } - if risk.Level != RiskHigh { - t.Fatalf("Classify(%q) = level %s; want high (unparseable_command only)", command, risk.Level) + for _, command := range []string{ + `git status push & rem '`, + `git "status" push & rem '`, + `git 'status' push & rem "`, + `git.exe -C "C:\Program Files\push" status & rem '`, + `git.exe --git-dir="C:\Program Files\push\.git" status & rem '`, + `git.exe "--git-dir=C:\Program Files\push\.git" status & rem '`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want no network category", command, risk.Level, risk.Categories) + } + if risk.Level != RiskHigh { + t.Errorf("Classify(%q) = level %s; want high (unparseable_command only)", command, risk.Level) + } + }) } } From 8a7ffe69cb1d7b7202e648eed14f6d2692072b76 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 28 Jul 2026 10:40:38 +0000 Subject: [PATCH 08/10] fix(sandbox): parse unclosed git commands safely Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno --- internal/sandbox/risk.go | 123 ++++++++++++++++++++---- internal/sandbox/risk_hardening_test.go | 13 +++ 2 files changed, 116 insertions(+), 20 deletions(-) diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index aa879bcf4..24225cbde 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -33,25 +33,9 @@ var ( // unparseableNetworkPattern is used only after the shell parser fails. At // that point the command is already marked too complex, so this intentionally // favors catching obvious network programs over proving exact shell syntax. - // The git branch accepts Windows executable suffixes and a closing quote - // from a fully qualified path (a parser-failing command like - // `"C:\Program Files\Git\cmd\git.exe" push ... & rem '` runs under cmd.exe - // and must still classify as network). Before the subcommand, it skips only - // git's own global options — mirroring gitGlobalOptionConsumesValue in - // analyzer.go: a recognized value-taking option (-C, -c, --attr-source, - // --config-env, --exec-path, --git-dir, --namespace, --super-prefix, - // --work-tree) consumes one following token as its value, and any other - // dash-prefixed token is skipped on its own. Both branches preserve quoted - // fragments, including spaces in separate or joined option values such as - // `-C "C:\Program Files\repo"` and `--git-dir="C:\Program Files\repo\.git"`. - // The subcommand may also be quoted, as cmd.exe permits `git "push" ...`. - // This span is unbounded — Go's regexp is RE2-backed (linear time, no - // backtracking blowup) — so a git invocation with any number of global - // options still reaches its subcommand. Critically, a bare non-option token - // (e.g. the "status" in `git status push`, where "push" is only a pathspec) - // does NOT match either alternative, so the scan stops and the command is - // correctly left unclassified as network. - 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|cmd|bat|com))?["']?(?:\s+(?:(?:(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)|"(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)"|'(?:-C|-c|--attr-source|--config-env|--exec-path|--git-dir|--namespace|--super-prefix|--work-tree)')\s+(?:[^\s;&|"']|"[^"]*"|'[^']*')+|(?:-(?:[^\s;&|"']|"[^"]*"|'[^']*')+|"-[^"]*"|'-[^']*')))*\s+(?:"(?:clone|fetch|pull|push|ls-remote|archive)"|'(?:clone|fetch|pull|push|ls-remote|archive)'|(clone|fetch|pull|push|ls-remote|archive)\b)|\bpython(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|\bgh\s+(api|repo\s+clone|release\s+download)\b`) + // Git needs token-aware handling below: a regex cannot reliably distinguish + // option values and executable path components from subcommands. + unparseableNetworkPattern = regexp.MustCompile(`(?i)^(?:(curl|wget|fetch|aria2c|ssh|scp|sftp|rsync|nc|ncat|netcat|telnet|ftp|npx|http-server|vite|next|nuxt|astro)(\s|$)|(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|go\s+get\b|python(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|gh\s+(api|repo\s+clone|release\s+download)\b)`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. @@ -85,7 +69,106 @@ func matchesDestructive(command string) bool { } func matchesUnparseableNetwork(command string) bool { - return unparseableNetworkPattern.MatchString(command) + commands := fallbackCommandTokens(command) + for _, tokens := range commands { + if len(tokens) == 0 { + continue + } + if isGitExecutableToken(tokens[0]) && matchesUnparseableGitNetwork(tokens) { + return true + } + executable := executableTokenBase(tokens[0]) + if unparseableNetworkPattern.MatchString(strings.Join(append([]string{executable}, tokens[1:]...), " ")) { + return true + } + } + return false +} + +func matchesUnparseableGitNetwork(tokens []string) bool { + for j := 1; j < len(tokens); j++ { + word := strings.ToLower(tokens[j]) + if word == "--help" || word == "--version" { + break + } + if strings.HasPrefix(word, "-") { + if gitGlobalOptionConsumesValue(word) { + j++ + } + continue + } + switch word { + case "clone", "fetch", "pull", "push", "ls-remote", "archive": + return true + } + break + } + return false +} + +func isGitExecutableToken(token string) bool { + switch executableTokenBase(token) { + case "git", "git.exe", "git.cmd", "git.bat", "git.com": + return true + default: + return false + } +} + +func executableTokenBase(token string) string { + token = strings.Trim(token, `\"'`) + if slash := strings.LastIndexAny(token, `/\\`); slash >= 0 { + token = token[slash+1:] + } + return strings.ToLower(token) +} + +// fallbackCommandTokens performs deliberately small shell/cmd tokenization. +// It preserves quoted spaces even when the command's trailing quote is +// unmatched (the condition that sends classification down this fallback). +func fallbackCommandTokens(command string) [][]string { + // Command strings commonly preserve cmd.exe's escaped quote spelling. + command = strings.ReplaceAll(command, `\"`, `"`) + var commands [][]string + var tokens []string + var word strings.Builder + var quote rune + flush := func() { + if word.Len() > 0 { + tokens = append(tokens, word.String()) + word.Reset() + } + } + flushCommand := func() { + flush() + if len(tokens) > 0 { + commands = append(commands, tokens) + tokens = nil + } + } + for _, r := range command { + if r == '\'' || r == '"' { + if quote == 0 { + quote = r + } else if quote == r { + quote = 0 + } else { + word.WriteRune(r) + } + continue + } + if quote == 0 && (r == ';' || r == '&' || r == '|') { + flushCommand() + continue + } + if quote == 0 && (r == ' ' || r == '\t' || r == '\r' || r == '\n') { + flush() + continue + } + word.WriteRune(r) + } + flushCommand() + return commands } func Classify(request Request) Risk { diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index bb2829d4a..2a6ac2db1 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -356,6 +356,9 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { `git.exe -C"C:\Program Files\repo" "push" origin main & rem '`, `git.exe --git-dir="C:\Program Files\repo\.git" push origin main & rem '`, `git.exe "--git-dir=C:\Program Files\repo\.git" push origin main & rem '`, + `git -C repo push origin main & rem '`, + `git -c user.name=test fetch origin & rem '`, + `git -C "C:\Program Files\repo" push origin main & rem '`, // More value-taking global options than the fallback regex used to cap // its generic-token scan at (formerly {0,8}) — every option here still // precedes the actual subcommand. @@ -389,6 +392,16 @@ func TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork(t *testing.T) { `git.exe -C "C:\Program Files\push" status & rem '`, `git.exe --git-dir="C:\Program Files\push\.git" status & rem '`, `git.exe "--git-dir=C:\Program Files\push\.git" status & rem '`, + `git -C push status & rem '`, + `git -c push status & rem '`, + `git -C "push" status & rem '`, + `git -c "push" status & rem '`, + `git --help push & rem '`, + `git --version push & rem '`, + `echo https://example.com/repo.git push & rem '`, + `echo ssh://git@example.com/repo.git push & rem '`, + `echo C:\repos\repo.git push & rem '`, + `echo git.example.com push & rem '`, } { t.Run(command, func(t *testing.T) { risk := classifyCommand(command) From 6225826fe478e76ed4e3d0a4eea30693ef1a460b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 28 Jul 2026 22:18:41 +0200 Subject: [PATCH 09/10] fix(sandbox): resolve the real program in the unparseable fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/sandbox/analyzer.go | 22 +++++- internal/sandbox/analyzer_test.go | 5 ++ internal/sandbox/engine_test.go | 26 +++++++ internal/sandbox/risk.go | 94 +++++++++++++++++++------ internal/sandbox/risk_hardening_test.go | 68 ++++++++++++++++++ internal/sandbox/safe_command.go | 36 ++++++++-- 6 files changed, 222 insertions(+), 29 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 6368eb1a3..2062c687d 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -279,13 +279,33 @@ func packageManagerOffline(words []string) bool { func gitUsesNetwork(words []string) bool { switch gitSubcommand(words) { - case "clone", "fetch", "pull", "push", "ls-remote", "archive": + case "clone", "fetch", "pull", "push", "ls-remote": return true + case "archive": + // `git archive HEAD` streams a tree out of the local object store and needs + // no egress at all; only `--remote=` sends the request to another + // host. Classifying every archive as network cost a proactive network + // prompt on a purely local command. + return gitTargetsRemoteArchive(words) default: return false } } +// gitTargetsRemoteArchive reports whether a git archive invocation carries the +// --remote option, in either its joined (--remote=) or separated +// (--remote ) spelling. The whole argument list is scanned rather than +// only the part after the subcommand, so option order cannot hide it. +func gitTargetsRemoteArchive(words []string) bool { + for _, word := range words { + lowered := strings.ToLower(word) + if lowered == "--remote" || strings.HasPrefix(lowered, "--remote=") { + return true + } + } + return false +} + // gitSubcommand resolves the subcommand past git's GLOBAL options. The generic // firstSubcommand cannot: git's value-taking globals put their value in the next // token, so scanning for the first non-dash token returns that value instead — diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index daa9e11ad..c7a23455d 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -88,6 +88,11 @@ func TestAnalyzeCommand(t *testing.T) { {name: "git push custom transport", script: "git push gitlawb://example.com/repo.git main", network: true}, {name: "git ls-remote", script: "git ls-remote origin", network: true}, {name: "git remote archive", script: "git archive --remote=origin HEAD", network: true}, + {name: "git remote archive separated value", script: "git archive --remote origin HEAD", network: true}, + // Only --remote leaves the machine; a local archive streams the object + // store and must not cost a network prompt (issue #703 review). + {name: "git local archive", script: "git archive HEAD", network: false}, + {name: "git -C local archive", script: "git -C repo archive HEAD -o out.tar", network: false}, {name: "git status is offline", script: "git status", network: false}, {name: "git local commit", script: `git commit -m "local change"`, network: false}, // git's value-taking global options put their value in the NEXT token, so a diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index e91486e9d..d023ca6b0 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -45,6 +45,32 @@ func TestEvaluatePromptsForNetworkToolsButExemptsThemFromShellNetworkPolicy(t *t } } +// TestEvaluatePromptsForUnparseableNetworkBehindWrapper is the engine-level +// regression for jatmn's #726 P2 finding. Classification is what decides egress +// here: when the fallback stopped recognizing a network program hidden behind a +// wrapper, an already-permission-granted shell command went from ActionPrompt / +// ReasonNetworkBlocked to a plain ActionAllow — silently granting network to a +// command too obfuscated to parse. +func TestEvaluatePromptsForUnparseableNetworkBehindWrapper(t *testing.T) { + engine := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}) + for _, command := range []string{ + `sudo curl https://evil.test && "unterminated`, + `env git push origin main && "unterminated`, + `curl.exe https://evil.test && "unterminated`, + "true\ncurl https://evil.test && \"unterminated", + } { + t.Run(command, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) = action %q reason %q, want a network prompt", command, decision.Action, decision.Reason) + } + }) + } +} + func TestEngineBashAllowGrantDoesNotBypassNetworkPrompt(t *testing.T) { store, err := NewGrantStore(StoreOptions{ FilePath: filepath.Join(t.TempDir(), "sandbox-grants.json"), diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 24225cbde..0371a6248 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -68,59 +68,107 @@ func matchesDestructive(command string) bool { return false } +// maxUnparseableShellDepth bounds `sh -c ` recursion in the fallback, +// mirroring maxAnalyzerDepth on the parseable path so a nested launcher chain +// cannot make classification unbounded. +const maxUnparseableShellDepth = 3 + func matchesUnparseableNetwork(command string) bool { - commands := fallbackCommandTokens(command) - for _, tokens := range commands { - if len(tokens) == 0 { + return matchesUnparseableNetworkAt(command, 0) +} + +// matchesUnparseableNetworkAt scans each fallback segment for a network program. +// It resolves the segment's real program the same way the parseable path does — +// past environment assignments and wrapper prefixes (sudo, env, timeout, nice, +// xargs, ...) and the option values those wrappers consume — because a fallback +// that only looked at the first token would let `sudo curl …`, `env git fetch …`, +// or `PATH=.:$PATH git push …` through. The point of this path is to fail closed +// on a command too obfuscated to parse; a wrapper prefix is the cheapest possible +// obfuscation. +// +// Resolving the program (rather than matching the network name anywhere in the +// string, as an earlier revision did) is what keeps `git status push` and +// `echo https://example.com/repo.git push` out: a network verb only counts when +// it belongs to a program actually being invoked. +func matchesUnparseableNetworkAt(command string, depth int) bool { + for _, tokens := range fallbackCommandTokens(command) { + body := commandBodyFields(tokens) + if len(body) == 0 { continue } - if isGitExecutableToken(tokens[0]) && matchesUnparseableGitNetwork(tokens) { + program, args := executableTokenBase(body[0]), body[1:] + if program == "git" && matchesUnparseableGitNetwork(args) { return true } - executable := executableTokenBase(tokens[0]) - if unparseableNetworkPattern.MatchString(strings.Join(append([]string{executable}, tokens[1:]...), " ")) { + if unparseableNetworkPattern.MatchString(strings.Join(append([]string{program}, args...), " ")) { return true } + // `sh -c ` runs the payload as a fresh command. The fallback + // tokenizer keeps a quoted payload as ONE token, so the network program + // inside it is not a token of this segment at all — recurse the way + // analyzeInto does on the parseable path. + if depth < maxUnparseableShellDepth && shellPrograms[program] { + if payload := fallbackDashCPayload(args); payload != "" { + if matchesUnparseableNetworkAt(payload, depth+1) { + return true + } + } + } } return false } -func matchesUnparseableGitNetwork(tokens []string) bool { - for j := 1; j < len(tokens); j++ { - word := strings.ToLower(tokens[j]) +// matchesUnparseableGitNetwork reports whether git's arguments (everything after +// the executable) name a subcommand that talks to a remote. +func matchesUnparseableGitNetwork(args []string) bool { + for index := 0; index < len(args); index++ { + word := strings.ToLower(args[index]) if word == "--help" || word == "--version" { break } if strings.HasPrefix(word, "-") { if gitGlobalOptionConsumesValue(word) { - j++ + index++ } continue } switch word { - case "clone", "fetch", "pull", "push", "ls-remote", "archive": + case "clone", "fetch", "pull", "push", "ls-remote": return true + case "archive": + // Only a --remote archive leaves the machine; `git archive HEAD` reads + // the local object store. See gitUsesNetwork for the same gate on the + // parseable path. + return gitTargetsRemoteArchive(args) } break } return false } -func isGitExecutableToken(token string) bool { - switch executableTokenBase(token) { - case "git", "git.exe", "git.cmd", "git.bat", "git.com": - return true - default: - return false +// fallbackDashCPayload returns the argument following `-c` (the command a shell +// launcher will run), or "" when there is none. It mirrors dashCPayload on the +// parseable path. +func fallbackDashCPayload(args []string) string { + for index := 0; index < len(args); index++ { + if args[index] == "-c" && index+1 < len(args) { + return args[index+1] + } } + return "" } +// executableTokenBase reduces a raw fallback token to a comparable program name. +// It strips quoting, any directory prefix, and a Windows executable suffix, so +// this path recognizes curl.exe and git.cmd exactly as normalizeProgramToken does +// on the parseable path — a token that normalized differently here used to be how +// `curl.exe https://… && "unterminated` lost its network classification. func executableTokenBase(token string) string { token = strings.Trim(token, `\"'`) - if slash := strings.LastIndexAny(token, `/\\`); slash >= 0 { + if slash := strings.LastIndexAny(token, `/\`); slash >= 0 { token = token[slash+1:] } - return strings.ToLower(token) + return trimExecutableSuffix(strings.ToLower(token)) } // fallbackCommandTokens performs deliberately small shell/cmd tokenization. @@ -157,11 +205,15 @@ func fallbackCommandTokens(command string) [][]string { } continue } - if quote == 0 && (r == ';' || r == '&' || r == '|') { + // A newline separates commands exactly as ;/&/| do. Treating it as mere + // whitespace kept a multi-line script as one segment, so anything after the + // first line was scanned as arguments of the first line's program and the + // program on line two was never resolved. + if quote == 0 && (r == ';' || r == '&' || r == '|' || r == '\n' || r == '\r') { flushCommand() continue } - if quote == 0 && (r == ' ' || r == '\t' || r == '\r' || r == '\n') { + if quote == 0 && (r == ' ' || r == '\t') { flush() continue } diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 2a6ac2db1..5298426ce 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -376,6 +376,74 @@ func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { } } +// TestClassifyUnparseableNetworkBehindWrapperFailsClosed is the regression test +// for jatmn's #726 P2/P3 findings: resolving the fallback's program from +// tokens[0] alone dropped the network category whenever the real program sat +// behind a wrapper (sudo/env/timeout/xargs), an environment assignment, a shell +// launcher's -c payload, a Windows executable suffix, or a newline — each of +// which base main still caught with its whole-string match. An unparseable +// command is already too obfuscated to analyze, so a wrapper prefix must not be +// enough to buy egress without a prompt. +func TestClassifyUnparseableNetworkBehindWrapperFailsClosed(t *testing.T) { + for _, command := range []string{ + // Wrapper programs, including ones whose options consume a value. + `sudo curl https://example.com && "unterminated`, + `sudo -u root curl https://example.com && "unterminated`, + `env curl https://example.com && "unterminated`, + `env git fetch origin && "unterminated`, + `sudo git push origin main && "unterminated`, + `sudo npm install && "unterminated`, + `timeout 5 curl https://example.com && "unterminated`, + `xargs curl https://example.com && "unterminated`, + // Environment-assignment prefixes. + `PATH=.:$PATH git push origin main && "unterminated`, + `GIT_SSH_COMMAND=ssh git push origin main && "unterminated`, + // A shell launcher's payload is a single token to the fallback tokenizer, + // so the program inside it is only visible by recursing into it. + `sh -c 'curl https://example.com' && "unterminated`, + `bash -c "git push origin main" && "unterminated`, + // Windows executable suffixes normalize on the parseable path already. + `curl.exe https://example.com && "unterminated`, + `wget.exe https://example.com && "unterminated`, + `sudo curl.exe https://example.com && "unterminated`, + // A newline separates commands; the network program is on its own line. + "true\ncurl https://example.com && \"unterminated", + "echo start\r\ngit push origin main && \"unterminated", + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) + } +} + +// TestClassifyUnparseableLocalGitArchiveStaysNonNetwork pins the other half of +// the archive gate: the fallback must agree with the AST path that only a +// --remote archive talks to another host. +func TestClassifyUnparseableLocalGitArchiveStaysNonNetwork(t *testing.T) { + for _, command := range []string{ + `git archive HEAD & rem '`, + `git archive -o out.tar HEAD & rem '`, + `git -C repo archive HEAD & rem '`, + `git.exe archive HEAD & rem '`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = categories %v; want no network category for a local archive", command, risk.Categories) + } + }) + } +} + // TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork guards against the // fallback regex treating an arbitrary bare token before a network verb as if // it were a git global option. `status` in `git status push` is a pathspec diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index 807004710..2e1c1e411 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -361,6 +361,16 @@ func firstProgram(fields []string) string { // command boundary (e.g. `sudo git rebase -i` -> "git rebase -i") instead of // matching the segment text anywhere as a raw substring. func commandBody(fields []string) string { + return strings.Join(commandBodyFields(fields), " ") +} + +// commandBodyFields is commandBody's scan, returning the fields from the first +// real command token onward (nil when the segment is nothing but assignments and +// wrappers). Callers that need to reason about the program and its arguments +// separately — the unparseable-command fallback in risk.go — use this rather +// than re-splitting commandBody's joined string, which would lose the token +// boundaries the fallback tokenizer worked to preserve. +func commandBodyFields(fields []string) []string { wrapper := "" for index := 0; index < len(fields); index++ { field := fields[index] @@ -381,9 +391,9 @@ func commandBody(fields []string) string { continue } // First real command token: the body starts here. - return strings.Join(fields[index:], " ") + return fields[index:] } - return "" + return nil } // isNumericToken reports whether a token is purely digits (e.g. the duration @@ -454,8 +464,20 @@ func normalizeProgramToken(field string) string { token = token[i+1:] } } - token = strings.ToLower(token) - for _, suffix := range []string{".exe", ".cmd", ".bat", ".com"} { + return trimExecutableSuffix(strings.ToLower(token)) +} + +// executableSuffixes are the Windows executable extensions stripped from a +// program token so curl.exe, git.cmd, and curl all normalize to one name. Both +// the parseable path (normalizeProgramToken) and the unparseable fallback +// (executableTokenBase) strip the same set; a token that normalizes differently +// on the two paths is exactly how a command slips past the fallback. +var executableSuffixes = []string{".exe", ".cmd", ".bat", ".com"} + +// trimExecutableSuffix removes one trailing Windows executable extension from an +// already-lowercased token. +func trimExecutableSuffix(token string) string { + for _, suffix := range executableSuffixes { if strings.HasSuffix(token, suffix) { return strings.TrimSuffix(token, suffix) } @@ -487,9 +509,9 @@ func windowsExecutablePathBasename(token string) (string, bool) { } func hasWindowsExecutableSuffix(token string) bool { - token = strings.ToLower(token) - for _, suffix := range []string{".exe", ".cmd", ".bat", ".com"} { - if strings.HasSuffix(token, suffix) { + lowered := strings.ToLower(token) + for _, suffix := range executableSuffixes { + if strings.HasSuffix(lowered, suffix) { return true } } From 0b56cd619c6f56f17149e2211034e6120f365e07 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 29 Jul 2026 14:41:13 +0200 Subject: [PATCH 10/10] fix(sandbox): stop reading a pathspec named --remote as a remote archive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/sandbox/analyzer.go | 18 +++++++++++--- internal/sandbox/analyzer_test.go | 8 ++++++ internal/sandbox/risk.go | 33 ++++++++++++++++++------- internal/sandbox/risk_hardening_test.go | 31 +++++++++++++++++++++++ 4 files changed, 78 insertions(+), 12 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 2062c687d..a4fb21e17 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -296,8 +296,17 @@ func gitUsesNetwork(words []string) bool { // --remote option, in either its joined (--remote=) or separated // (--remote ) spelling. The whole argument list is scanned rather than // only the part after the subcommand, so option order cannot hide it. +// +// Scanning stops at a standalone `--`: everything after it is a pathspec, so +// `git archive HEAD -- --remote` archives a tree entry NAMED --remote from the +// local object store and never touches the network. Treating that as remote +// would cost a network prompt — and a network grant — on a purely local +// command. Same end-of-options rule hasRecursiveForce applies to `rm -- -rf`. func gitTargetsRemoteArchive(words []string) bool { for _, word := range words { + if word == "--" { + return false + } lowered := strings.ToLower(word) if lowered == "--remote" || strings.HasPrefix(lowered, "--remote=") { return true @@ -483,11 +492,14 @@ func effectiveProgram(args []*syntax.Word) (string, []*syntax.Word) { return "", nil } -// dashCPayload returns the literal text of the word following `-c` in an AST arg -// list (the command a shell launcher will run), or "" when there is none. +// dashCPayload returns the literal text of the word following `-c`/`--command` +// in an AST arg list (the command a shell launcher will run), or "" when there +// is none. Both spellings are recognized, matching shellDashCPayload and the +// unparseable fallback: bash and zsh accept either, so knowing only `-c` would +// leave `bash --command ''` unclassified on both paths. func dashCPayload(args []*syntax.Word) string { for index := 0; index < len(args); index++ { - if wordText(args[index]) == "-c" && index+1 < len(args) { + if isShellCommandFlag(wordText(args[index])) && index+1 < len(args) { return wordText(args[index+1]) } } diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index c7a23455d..44edb6e5b 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -93,6 +93,14 @@ func TestAnalyzeCommand(t *testing.T) { // store and must not cost a network prompt (issue #703 review). {name: "git local archive", script: "git archive HEAD", network: false}, {name: "git -C local archive", script: "git -C repo archive HEAD -o out.tar", network: false}, + // After `--` every token is a pathspec, so this archives a tree entry + // named "--remote" from the local object store (issue #703 review). + {name: "git archive pathspec named --remote", script: "git archive HEAD -- --remote", network: false}, + {name: "git -C archive pathspec named --remote", script: "git -C repo archive HEAD -- --remote=origin", network: false}, + // A real --remote before the separator still is one. + {name: "git remote archive with pathspec", script: "git archive --remote=origin HEAD -- src", network: true}, + // Both spellings of the shell launcher's command flag are payloads. + {name: "bash --command wraps curl", script: `bash --command 'curl https://x.test'`, network: true}, {name: "git status is offline", script: "git status", network: false}, {name: "git local commit", script: `git commit -m "local change"`, network: false}, // git's value-taking global options put their value in the NEXT token, so a diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 0371a6248..55201f611 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -68,10 +68,11 @@ func matchesDestructive(command string) bool { return false } -// maxUnparseableShellDepth bounds `sh -c ` recursion in the fallback, -// mirroring maxAnalyzerDepth on the parseable path so a nested launcher chain -// cannot make classification unbounded. -const maxUnparseableShellDepth = 3 +// maxUnparseableShellDepth bounds `sh -c ` recursion in the fallback. +// It IS maxAnalyzerDepth rather than a copy of its value: a fallback that gave +// up a level earlier than the parseable path would drop the network category on +// exactly the deeply-nested launcher chains this path exists to fail closed on. +const maxUnparseableShellDepth = maxAnalyzerDepth func matchesUnparseableNetwork(command string) bool { return matchesUnparseableNetworkAt(command, 0) @@ -146,26 +147,40 @@ func matchesUnparseableGitNetwork(args []string) bool { return false } -// fallbackDashCPayload returns the argument following `-c` (the command a shell -// launcher will run), or "" when there is none. It mirrors dashCPayload on the -// parseable path. +// fallbackDashCPayload returns the argument following `-c` or `--command` (the +// command a shell launcher will run), or "" when there is none. Both spellings +// are accepted because bash and zsh accept both, and shellDashCPayload in +// safe_command.go already does — a fallback that only knew `-c` would let +// `bash --command "git push …"` past the very check this path exists for. func fallbackDashCPayload(args []string) string { for index := 0; index < len(args); index++ { - if args[index] == "-c" && index+1 < len(args) { + if isShellCommandFlag(args[index]) && index+1 < len(args) { return args[index+1] } } return "" } +// isShellCommandFlag reports whether a token is the flag whose value a shell +// launcher executes. +func isShellCommandFlag(token string) bool { + return token == "-c" || token == "--command" +} + // executableTokenBase reduces a raw fallback token to a comparable program name. // It strips quoting, any directory prefix, and a Windows executable suffix, so // this path recognizes curl.exe and git.cmd exactly as normalizeProgramToken does // on the parseable path — a token that normalized differently here used to be how // `curl.exe https://… && "unterminated` lost its network classification. +// +// Drive-relative spellings go through windowsExecutablePathBasename for the same +// reason: `C:git.exe` has no separator to cut on, so a plain basename scan leaves +// `c:git` and the deny never matches a program the parseable path classifies. func executableTokenBase(token string) string { token = strings.Trim(token, `\"'`) - if slash := strings.LastIndexAny(token, `/\`); slash >= 0 { + if basename, ok := windowsExecutablePathBasename(token); ok { + token = basename + } else if slash := strings.LastIndexAny(token, `/\`); slash >= 0 { token = token[slash+1:] } return trimExecutableSuffix(strings.ToLower(token)) diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 5298426ce..5405dd781 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -409,6 +409,16 @@ func TestClassifyUnparseableNetworkBehindWrapperFailsClosed(t *testing.T) { // A newline separates commands; the network program is on its own line. "true\ncurl https://example.com && \"unterminated", "echo start\r\ngit push origin main && \"unterminated", + // bash and zsh accept --command as well as -c, so the payload behind it + // has to be scanned too (issue #703 review). + `bash --command 'curl https://example.com' && "unterminated`, + `sh --command "git push origin main" && "unterminated`, + // A drive-relative Windows spelling has no separator to cut on, so the + // basename scan alone left "c:git" and never matched (same review). + `C:git.exe push origin main & rem '`, + `C:curl.exe https://example.com & rem '`, + // Recursion goes through more than one launcher layer. + `sh -c "sh -c 'curl https://example.com'" && "unterminated`, } { t.Run(command, func(t *testing.T) { risk := classifyCommand(command) @@ -422,6 +432,23 @@ func TestClassifyUnparseableNetworkBehindWrapperFailsClosed(t *testing.T) { } } +// TestUnparseableShellDepthMatchesAnalyzerDepth pins the two launcher-recursion +// caps together, which is the property jatmn's #703 review asked for: a fallback +// that gave up a level earlier than the AST path would drop the network category +// on exactly the deeply-nested chains it exists to fail closed on. +// +// Asserted as constants rather than by driving a four-deep command: the fallback +// tokenizer is deliberately small and does not model nested escaped quotes, so +// a literal four-layer `sh -c` string would be testing the tokenizer's escaping +// rather than the depth limit. The behavior that recursion happens at all, and +// through more than one layer, is covered above. +func TestUnparseableShellDepthMatchesAnalyzerDepth(t *testing.T) { + if maxUnparseableShellDepth != maxAnalyzerDepth { + t.Fatalf("maxUnparseableShellDepth = %d, maxAnalyzerDepth = %d; the fallback must not give up before the parseable path", + maxUnparseableShellDepth, maxAnalyzerDepth) + } +} + // TestClassifyUnparseableLocalGitArchiveStaysNonNetwork pins the other half of // the archive gate: the fallback must agree with the AST path that only a // --remote archive talks to another host. @@ -431,6 +458,10 @@ func TestClassifyUnparseableLocalGitArchiveStaysNonNetwork(t *testing.T) { `git archive -o out.tar HEAD & rem '`, `git -C repo archive HEAD & rem '`, `git.exe archive HEAD & rem '`, + // A pathspec named --remote after the end-of-options separator is a + // local tree entry, not a remote (issue #703 review). + `git archive HEAD -- --remote & rem '`, + `git archive HEAD -- --remote=origin & rem '`, } { t.Run(command, func(t *testing.T) { risk := classifyCommand(command)