Prevent argument injection in git CLI calls

[pfleidi]

Entire-Checkpoint: ac699460bd01

• CheckoutBranch() and performGitResetHard() pass string arguments directly to git subprocesses. A ref starting with - would be interpreted as a git flag instead of a ref, causing unintended behavior (e.g., git checkout -b evil creates a
  branch, git reset --hard -q silently resets to HEAD).
• Add input validation rejecting refs/hashes that start with - before they reach git, with tests demonstrating both attack vectors.

Test plan

• TestCheckoutBranch/rejects_ref_starting_with_dash_to_prevent_argument_injection — verifies CheckoutBranch("-b evil") returns an error instead of creating a branch
• TestPerformGitResetHard_RejectsArgumentInjection — verifies performGitResetHard("-q") returns an error instead of silently resetting to HEAD
• Full CI suite passes (mise run fmt && mise run lint && mise run test:ci)

[Copilot]

Pull request overview

This PR addresses a security vulnerability by preventing argument injection in two git CLI wrapper functions. When refs or commit hashes starting with - were passed to git checkout or git reset --hard, they could be interpreted as git flags rather than arguments, leading to unintended behavior (e.g., creating branches, silently resetting to HEAD).

Changes:

• Added input validation to reject refs/hashes starting with - in CheckoutBranch() and performGitResetHard()
• Added comprehensive tests demonstrating both attack vectors and validating the fixes

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
cmd/entire/cli/git_operations.go	Added dash-prefix validation to CheckoutBranch() to prevent argument injection attacks
cmd/entire/cli/rewind.go	Added dash-prefix validation to performGitResetHard() to prevent argument injection attacks
cmd/entire/cli/resume_test.go	Added security tests validating that both functions reject dash-prefixed inputs, plus imported strings package

[evisdren]

This is probably okay in the short term.

But I wonder if we should actually split this out into SwitchBranch and CheckoutBranch functions. CheckoutBranch is a bit overloaded. It seems like the intention of the function is just to switch to a local branch or If you go with switch branch, then you can pass in -- <branch> which will treat everything after the -- as a branch name and fail if a - is passed as an evil flag.

The nuance here is that git switch has to handle both commit hashes and branch names. For commit hashes you have to pass in git switch --detach <commithash>, which is actually clearer in my opinion. The challenge is then determining the difference between a commit hash and a branch. You could shell out to a git sub process to do that (most reliable), but that seems wasteful.

The actual better long term strategy here would be to split up the swtich into SwitchBranchWithName() and SwitchBranchWithCommit() and pass in the right argument. That would fully remove the argument injection problem since the argument would always come after a -- and be treated as a literal and make it clearer in the codebase what argument we're passing.

[pfleidi]

Yeah, SwitchBranch would also address the case where legitimate branch names beginning with a - would work and not confuse git unnecessarily.

I like the idea but that might be a change for down the road. What do you think?

[evisdren]

pretty much same comment as above

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

cmd/entire/cli/resume_test.go:177

• The test case CheckoutBranch("-b evil") likely doesn’t reproduce the described argument-injection behavior because exec.CommandContext passes the entire string as a single argv element (it won’t split on spaces). Git option parsing typically requires -b and the branch name as separate args (or a single arg form like -B<name> / --orphan=<name>). As written, this test may have already failed even before the fix, so it may not actually prove the vulnerability. Consider changing the injected ref to a single-argument option form that Git will accept (e.g., a long option with = or a short option with an attached value) and assert it would have changed repo state without the new validation.

	t.Run("rejects ref starting with dash to prevent argument injection", func(t *testing.T) {
		// "git checkout -b evil" would create a new branch named "evil" instead
		// of failing, because git interprets "-b" as a flag.
		err := CheckoutBranch("-b evil")
		if err == nil {
			t.Fatal("CheckoutBranch() should reject refs starting with '-', got nil")
		}
		if !strings.Contains(err.Error(), "invalid ref") {
			t.Errorf("CheckoutBranch() error = %q, want error containing 'invalid ref'", err.Error())
		}