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/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/agent/loop_test.go b/internal/agent/loop_test.go index 1dfec336f..d45abee18 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -2428,22 +2428,22 @@ 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" { if windowsTestUsesPowerShell() { - command = `$env:PATH = '.;' + $env:PATH; curl.cmd https://example.com` + command = `$env:PATH = '.;' + $env:PATH; git.cmd push gitlawb://example.com/repo.git main` } else { - command = "set PATH=.;%PATH% && curl https://example.com" + command = "set PATH=.;%PATH% && git push gitlawb://example.com/repo.git main" } - fakeCurl := filepath.Join(root, "curl.cmd") - if err := os.WriteFile(fakeCurl, []byte("@echo fake curl %*\r\n"), 0o755); err != nil { + 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) } } @@ -2465,7 +2465,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", @@ -2507,7 +2507,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 7973fa951..2062c687d 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -278,8 +278,68 @@ func packageManagerOffline(words []string) bool { } func gitUsesNetwork(words []string) bool { - switch firstSubcommand(words, nil) { - case "clone", "fetch", "pull", "push", "ls-remote", "archive": + switch gitSubcommand(words) { + 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 — +// `git -C repo push origin main` looked like the subcommand "repo" and so +// classified as no-network, dropping the proactive network prompt for the most +// common form of the command. internal/agent/command_prefix.go resolves the same +// option set for its own prefix matching. +func gitSubcommand(words []string) string { + for index := 0; index < len(words); index++ { + word := words[index] + if word == "" { + continue + } + if strings.HasPrefix(word, "-") { + // A joined value (--git-dir=/x, -C/x) is one token and needs no skip; + // a separated one puts its value in the next token. + if gitGlobalOptionConsumesValue(word) { + index++ + } + continue + } + if isNumericToken(word) { + continue + } + return word + } + return "" +} + +// gitGlobalOptionConsumesValue lists git's global options whose value is a +// SEPARATE token (mirrors gitOptionConsumesValue in internal/agent). +func gitGlobalOptionConsumesValue(option string) bool { + switch option { + 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 9f44eca25..c7a23455d 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -84,7 +84,29 @@ func TestAnalyzeCommand(t *testing.T) { {name: "next dev", script: "next dev", network: true}, {name: "git clone", script: "git clone https://example.com/repo.git", 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 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 + // generic "first non-dash token" scan reads the value as the subcommand and + // misses the network verb entirely (issue #703 review). + {name: "git -C push", script: "git -C repo push origin main", network: true}, + {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.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}, {name: "process pattern is not network", script: `pkill -f "python3 -m http.server 8000"`, network: false}, 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 a0e784f67..0371a6248 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -33,7 +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. - 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`) + // 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. @@ -66,8 +68,159 @@ 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 { - return unparseableNetworkPattern.MatchString(command) + 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 + } + program, args := executableTokenBase(body[0]), body[1:] + if program == "git" && matchesUnparseableGitNetwork(args) { + return true + } + 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 +} + +// 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) { + index++ + } + continue + } + switch word { + 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 +} + +// 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 { + token = token[slash+1:] + } + return trimExecutableSuffix(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 + } + // 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') { + 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 ce2f89994..5298426ce 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -273,6 +273,9 @@ func TestClassifyASTCatchesNetworkProgramsRegexMisses(t *testing.T) { "ftp ftp.example.com", "sftp user@host", "sudo telnet example.com 23", + "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") { @@ -281,6 +284,46 @@ func TestClassifyASTCatchesNetworkProgramsRegexMisses(t *testing.T) { } } +// TestClassifyParseableGitNetworkCommandsUseASTPath covers the git forms that +// PARSE cleanly, so the unparseable-command regex fallback is never consulted and +// the AST classifier is the only thing standing between these commands and +// unprompted egress. `git -C ` is the canonical way to operate on a +// repo without cd, and its value used to be read as the subcommand. +func TestClassifyParseableGitNetworkCommandsUseASTPath(t *testing.T) { + for _, command := range []string{ + "git -C repo push origin main", + "git -c http.sslVerify=false push origin main", + "git --git-dir /repo/.git fetch origin", + "git --work-tree /repo pull origin main", + "git --namespace ns push origin main", + "git.exe push origin main", + "git.exe -C repo push gitlawb://example.com/repo.git main", + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) reported TooComplex; this case must exercise the AST path", command) + } + risk := classifyCommand(command) + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) + } + + // Local git work through the same global options stays off the network path. + for _, command := range []string{ + `git -C repo commit -m "local change"`, + "git -C repo status", + `git.exe commit -m "local change"`, + } { + t.Run(command, func(t *testing.T) { + if risk := classifyCommand(command); HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = categories %v; want no network category", command, risk.Categories) + } + }) + } +} + func TestClassifyFlagsUnparseableCommand(t *testing.T) { // An unparseable (e.g. obfuscated) script can't be analyzed statically; the // AST analyzer reports TooComplex so the classifier elevates it to High. @@ -291,12 +334,155 @@ 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) + 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`, + `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 '`, + `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 '`, + `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. + `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) + 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) + } + }) } - 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) +} + +// 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 +// 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) { + 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 '`, + `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) + 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) + } + }) } } 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 } }