fix(sandbox): classify git push as network access

internal/agent/command_prefix.go

@@ -373,73 +373,69 @@ func validSedPrintArg(arg string) bool {
 	return true
 }
 
+// gitApprovableReadOnlySubcommands are the only git subcommands this prefix
+// matcher will ever auto-approve. Every other resolvable subcommand (push,
+// commit, ...) — and the case where no subcommand resolves at all — is
+// rejected by the zero value of the map lookup below, so this list is the
+// single place that grants approval; nothing else needs to enumerate it.
+var gitApprovableReadOnlySubcommands = map[string]bool{
+	"status": true, "log": true, "diff": true, "show": true, "branch": true,
+}
+
+// safeGitCommand approves only exact lowercase spellings of
+// status/log/diff/show/branch with no global option this matcher has not
+// specifically vetted as safe.
+//
+// Subcommand resolution goes through sandbox.GitSubcommand so option parsing
+// stays shared with the classifier, but authorization compares Original, never
+// Normalized. Git aliases are case-sensitive: STATUS can be an arbitrary alias
+// even though the classifier conservatively recognizes its normalized spelling
+// as status.
 func safeGitCommand(command []string) bool {
-	subIndex, subcommand, ok := gitSubcommand(command)
-	if !ok {
+	if len(command) < 2 {
 		return false
 	}
+	selection, ok := sandbox.GitSubcommand(command[1:])
+	if !ok || !gitApprovableReadOnlySubcommands[selection.Original] {
+		return false
+	}
+	// GitSubcommand indexes into command[1:] (it does not expect the "git"
+	// argv[0] itself); shift its answer back into command's own indexing so the
+	// slices below retain their command-relative meaning.
+	subIndex := selection.Index + 1
 	if gitHasUnsafeGlobalOption(command[1:subIndex]) {
 		return false
 	}
 	args := command[subIndex+1:]
-	switch subcommand {
-	case "status", "log", "diff", "show":
-		return gitArgsReadOnly(args)
-	case "branch":
+	if selection.Original == "branch" {
 		return gitArgsReadOnly(args) && gitBranchReadOnly(args)
-	default:
-		return false
-	}
-}
-
-func gitSubcommand(command []string) (int, string, bool) {
-	for index := 1; index < len(command); index++ {
-		arg := command[index]
-		if gitOptionConsumesValue(arg) {
-			index++
-			continue
-		}
-		if gitOptionHasInlineValue(arg) || arg == "--" || strings.HasPrefix(arg, "-") {
-			continue
-		}
-		switch arg {
-		case "status", "log", "diff", "show", "branch":
-			return index, arg, true
-		default:
-			return 0, "", false
-		}
 	}
-	return 0, "", false
-}
-
-func gitOptionConsumesValue(arg string) bool {
-	switch arg {
-	case "-C", "-c", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree":
-		return true
-	default:
-		return false
-	}
-}
-
-func gitOptionHasInlineValue(arg string) bool {
-	return strings.HasPrefix(arg, "--config-env=") ||
-		strings.HasPrefix(arg, "--exec-path=") ||
-		strings.HasPrefix(arg, "--git-dir=") ||
-		strings.HasPrefix(arg, "--namespace=") ||
-		strings.HasPrefix(arg, "--super-prefix=") ||
-		strings.HasPrefix(arg, "--work-tree=") ||
-		((strings.HasPrefix(arg, "-C") || strings.HasPrefix(arg, "-c")) && len(arg) > 2)
+	return gitArgsReadOnly(args)
 }
 
+// gitHasUnsafeGlobalOption rejects global options this matcher has not
+// specifically vetted as compatible with the read-only subcommands above.
+// -C changes which repository status/log/diff/show/branch inspects — a prefix
+// approved against the workspace repo must not silently extend to `git -C
+// /elsewhere status`, which reports on a DIFFERENT repository the approval
+// was never about. --upload-pack lets a fetch/clone run an arbitrary program
+// on the remote side; it is listed here defensively even though none of the
+// five approvable subcommands accept it, so a future addition to that list
+// does not silently inherit the gap.
+//
+// This still reads git's shared skip-list (GitGlobalOptionConsumesValue) to
+// step over an option's separate-token value correctly; only the "-C is
+// unsafe regardless" judgment is specific to this matcher, so it stays local
+// rather than joining the classifier's shared grammar.
 func gitHasUnsafeGlobalOption(args []string) bool {
 	for index := 0; index < len(args); index++ {
 		arg := args[index]
 		switch {
 		case strings.HasPrefix(arg, "--upload-pack"):
 			return true
-		case arg == "-C" || strings.HasPrefix(arg, "-C"):
+		case strings.HasPrefix(arg, "-C"):
 			return true
-		case gitOptionConsumesValue(arg):
+		case sandbox.GitGlobalOptionConsumesValue(arg):
 			index++
 		}
 	}

internal/agent/command_prefix_test.go

@@ -35,6 +35,36 @@ 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 TestSafeGitCommandRejectsCaseDistinctAliasSubcommand(t *testing.T) {
+	for _, command := range [][]string{
+		{"git", "-c", "alias.STATUS=!curl", "STATUS", "https://example.invalid"},
+		{"git", "-c", "alias.Status=!curl", "Status", "https://example.invalid"},
+	} {
+		if safeGitCommand(command) {
+			t.Errorf("safeGitCommand(%q) = true; case-distinct aliases must not receive reusable prefixes", command)
+		}
+	}
+	if prefix := proposedCommandPrefix("bash", map[string]any{
+		"command": `make test && git -c alias.STATUS=!curl STATUS https://example.invalid`,
+	}); prefix != nil {
+		t.Fatalf("case-distinct alias was accepted as a safe tail and exposed prefix %#v", prefix)
+	}
+	if !safeGitCommand([]string{"git", "--attr-source", "HEAD", "status"}) {
+		t.Fatal("ordinary lowercase status with a vetted global option must remain approvable")
+	}
+}
+
 func TestProposedCommandPrefixSupportsSegmentedCommands(t *testing.T) {
 	got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"})
 	if runtime.GOOS == "windows" {
@@ -231,3 +261,82 @@ func TestProposedCommandPrefixRejectsRequestedUnsafeLauncherPrefix(t *testing.T)
 		t.Fatalf("unsafe requested launcher prefix should be rejected, got %#v", got)
 	}
 }
+
+// A terminal global makes git print and exit, so nothing after it is a
+// subcommand. The sandbox classifier already stopped there; this parser walked
+// past it and resolved `git --help status` to the read-only prefix
+// `git status`, auto-approving a command the user never ran. The two scans read
+// one option grammar (sandbox.GitTerminalGlobalOption) so they cannot drift.
+func TestSafeGitCommandStopsAtTerminalGlobalOptions(t *testing.T) {
+	for _, command := range [][]string{
+		{"git", "--help", "status"},
+		{"git", "-h", "status"},
+		{"git", "--version", "log"},
+		{"git", "--exec-path", "status"},
+		{"git", "-C", "repo", "--help", "diff"},
+	} {
+		if safeGitCommand(command) {
+			t.Errorf("safeGitCommand(%q) = true; a terminal global ends the subcommand scan", command)
+		}
+	}
+	// An inline --exec-path=<path> is a value-carrying global, not terminal, so
+	// the subcommand after it is still real.
+	for _, command := range [][]string{
+		{"git", "--exec-path=/usr/libexec/git-core", "status"},
+		{"git", "--namespace", "ns", "status"},
+	} {
+		if !safeGitCommand(command) {
+			t.Errorf("safeGitCommand(%q) = false; want true", command)
+		}
+	}
+}
+
+// TestSafeGitCommandRejectsDashCEvenWithAnApprovableSubcommand isolates the
+// -C rejection from the terminal-global short circuit above (`-C repo --help
+// diff` never reaches gitHasUnsafeGlobalOption's -C case at all, because
+// --help stops the scan first). -C changes which repository the read-only
+// subcommand inspects, so an approval for the workspace repo must not extend
+// to a different one named this way — even for status/log/diff/show/branch,
+// which are otherwise auto-approved.
+func TestSafeGitCommandRejectsDashCEvenWithAnApprovableSubcommand(t *testing.T) {
+	for _, command := range [][]string{
+		{"git", "-C", "repo", "status"},
+		{"git", "-Crepo", "status"},
+		{"git", "-C", "repo", "branch"},
+	} {
+		if safeGitCommand(command) {
+			t.Errorf("safeGitCommand(%q) = true; -C must never be auto-approved", command)
+		}
+	}
+}
+
+// TestSafeGitCommandRejectsSubcommandsOutsideTheApprovedList proves the
+// refactor onto sandbox.GitSubcommand (a shared reader that resolves ANY
+// subcommand, not just the five this matcher approves) did not widen what
+// gets auto-approved: a resolvable-but-unapproved subcommand, and no
+// subcommand at all, both stay rejected.
+func TestSafeGitCommandRejectsSubcommandsOutsideTheApprovedList(t *testing.T) {
+	for _, command := range [][]string{
+		{"git", "push", "origin", "main"},
+		{"git", "commit", "-m", "msg"},
+		{"git", "fetch", "origin"},
+		{"git"},
+		{"git", "-C", "repo"},
+	} {
+		if safeGitCommand(command) {
+			t.Errorf("safeGitCommand(%q) = true; want rejected", command)
+		}
+	}
+	// The approved subcommands still resolve correctly through the same shared
+	// reader, with a global option in front — end-to-end proof the subIndex
+	// arithmetic across the sandbox.GitSubcommand boundary is still right.
+	for _, command := range [][]string{
+		{"git", "status"},
+		{"git", "--git-dir", "/repo/.git", "log"},
+		{"git", "branch"},
+	} {
+		if !safeGitCommand(command) {
+			t.Errorf("safeGitCommand(%q) = false; want approved", command)
+		}
+	}
+}