fix: prevent command injection via untrusted git branch names (#1791)

* fix: prevent command injection via untrusted git branch names Untrusted git branch names (e.g. a fork PR head branch) were substituted unescaped into executor prepare scripts and run via `eval` in a shell, yielding command injection (CRITICAL RCE) in the Docker/Sprites/remote executors. git check-ref-format permits `;`, `|`, `$`, backticks and `()`, so a branch like `$(touch pwned)` or `main;touch pwned` executed. - scriptengine: shell-single-quote every data placeholder that lands in shell text (repository.path/name/branch/clone_url/ssh_url, workspace.path, worktree.base_path/path/branch/base_branch). Script fragments (setup_script, git.identity_setup, github.auth_setup, kandev.*) are left untouched. - default_scripts: wrap each data placeholder in single quotes in the postlude, clone lines, and the Sprites echo (double quotes still allow $(...) substitution). `'<escaped>'` round-trips to the literal. - orchestrator: defense-in-depth — reject a PR head branch that fails securityutil.IsValidBranchName before it reaches CheckoutBranch, while still creating the review task without a checkout. - tests: PoC-derived regression asserting the injected command no longer runs through the real sh -c/eval sink, provider escaping unit tests, and ingestion-layer validation coverage. * fix: harden branch-name escaping for legacy stored prepare scripts Automated review (Codex, CodeRabbit) flagged that shell-single-quoting the value while relying on the templates to add the surrounding quotes only protects the templates we ship. Profiles snapshot the default prepare script at creation time, so an OLD stored script referencing a placeholder bare (e.g. `git clone --depth=1 --branch {{repository.branch}} ...`) still resolved with an unquoted, injectable branch value. - scriptengine: add shellQuote, which emits a fully self-contained single-quoted shell token (surrounding quotes included). Every data placeholder (repository.path/name/branch/clone_url/ssh_url, workspace.path, worktree.base_path/path/branch/base_branch) now carries its own quoting, so a bare `{{placeholder}}` in any template — including legacy stored scripts — is safe. shellSingleQuote is kept for the git identity fragment, which supplies its own quotes. - default_scripts: reference data placeholders bare (the value self-quotes); the Sprites echo becomes printf with the values as separate args so a hostile URL/branch can't expand inside a double-quoted string. - tests: assert providers emit fully-quoted tokens that round-trip through a real shell; cover base_path/path/repository.path/name/ssh_url; add a regression proving a legacy stored script with bare placeholders no longer executes injected commands. * test: strengthen branch-injection regression assertions Address Greptile review feedback on the new tests. - assertNoUnquotedPayload now tracks real single-quote state char-by-char instead of "some quote before and after", so a payload in an unquoted context on a line that happens to contain other quotes is caught. - Drop the unused wantHasRepoRow table field; repo-row presence is already asserted via len(got) != 1. --------- Co-authored-by: Kandev Agent <agent@kandev.dev>
2026-07-19 10:33:29 +01:00
parent 410b1dd4d1
commit 8f23b8a19e
9 changed files with 505 additions and 39 deletions
@@ -0,0 +1,223 @@
+package lifecycle
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/kandev/kandev/internal/common/config"
+)
+
+// TestBranchNameCommandInjection_Regression guards the CRITICAL RCE fix where an
+// untrusted git branch name (e.g. a fork PR head branch, attacker controlled)
+// was interpolated UNESCAPED into the prepare script that the Docker/Sprites
+// executors run via `eval "$KANDEV_PREPARE_SCRIPT"` inside a shell.
+//
+// git check-ref-format permits `;`, `|`, `&`, `$`, backticks and `()` in ref
+// names, so `$(touch pwned)` and `main;touch pwned;#` are legal, nameable refs.
+//
+// Before the fix (double-quoted postlude + fully-unquoted clone line, and no
+// shell escaping in scriptengine), the snippets below executed the injected
+// command — the marker file appeared on disk. This test FAILS before the fix
+// and PASSES after: it asserts (1) the resolved script carries the payload only
+// inside safe single quotes and (2) running the snippet through the real
+// `sh -c`/`eval` sink does NOT create the marker.
+//
+// See the "PoC — before fix" write-up in the task for the original captured
+// exploit output.
+func TestBranchNameCommandInjection_Regression(t *testing.T) {
+	exec := NewDockerExecutor(config.DockerConfig{}, "", newTestDockerLogger())
+
+	// ---------------------------------------------------------------------
+	// Case A: postlude checkout (was double-quoted; $(...) would substitute).
+	// ---------------------------------------------------------------------
+	t.Run("postlude does not run $() from branch name", func(t *testing.T) {
+		tmp := t.TempDir()
+		marker := filepath.Join(tmp, "pwned_postlude")
+
+		workspace := filepath.Join(tmp, "workspace")
+		if err := os.MkdirAll(filepath.Join(workspace, ".git"), 0o755); err != nil {
+			t.Fatal(err)
+		}
+
+		maliciousBranch := "$(touch " + marker + ")" // legal, nameable ref
+
+		req := &ExecutorCreateRequest{
+			Metadata: map[string]interface{}{
+				"base_branch":          "main",
+				"repository_clone_url": "https://github.com/org/repo.git",
+				"repository_path":      "/tmp/repo",
+				"worktree_branch":      maliciousBranch,
+			},
+			Env: map[string]string{},
+		}
+
+		script := exec.resolvePrepareScript(req)
+
+		// The raw, unquoted `$(...)` must NOT appear naked. It may only appear
+		// inside single quotes, where the shell treats it as a literal.
+		assertNoUnquotedPayload(t, script, maliciousBranch)
+
+		postlude := extractPostlude(t, script)
+		postlude = strings.ReplaceAll(postlude, "/workspace", workspace)
+		runViaEval(t, tmp, postlude)
+
+		if _, err := os.Stat(marker); err == nil {
+			t.Fatalf("REGRESSION: injected `touch` ran via postlude; marker created: %s", marker)
+		}
+		t.Logf("safe: postlude did not execute injected command; marker absent")
+	})
+
+	// ---------------------------------------------------------------------
+	// Case B: unquoted clone line (was `--branch {{repository.branch}}`).
+	// ---------------------------------------------------------------------
+	t.Run("clone line does not run ; chained command from branch name", func(t *testing.T) {
+		tmp := t.TempDir()
+		marker := filepath.Join(tmp, "pwned_clone")
+
+		maliciousBranch := "main;touch " + marker + ";#"
+
+		req := &ExecutorCreateRequest{
+			Metadata: map[string]interface{}{
+				"base_branch":          maliciousBranch,
+				"repository_clone_url": "https://github.com/org/repo.git",
+				"repository_path":      "/tmp/repo",
+				"worktree_branch":      "feature/task-abc",
+			},
+			Env: map[string]string{},
+		}
+
+		script := exec.resolvePrepareScript(req)
+		assertNoUnquotedPayload(t, script, maliciousBranch)
+
+		cloneLine := extractLine(t, script, "git clone --depth=1 --branch")
+		runViaEval(t, tmp, cloneLine)
+
+		if _, err := os.Stat(marker); err == nil {
+			t.Fatalf("REGRESSION: injected `touch` ran via clone line; marker created: %s", marker)
+		}
+		t.Logf("safe: clone line did not execute injected command; marker absent")
+	})
+
+	// ---------------------------------------------------------------------
+	// Case C: LEGACY stored prepare_script with BARE / double-quoted
+	// placeholders. Profiles created before this fix snapshotted a default
+	// template that referenced {{repository.branch}} / {{worktree.branch}}
+	// with no single quotes. The providers now emit a self-contained quoted
+	// token, so those stored scripts are safe without editing every profile.
+	// This is the scenario the automated reviewers flagged.
+	// ---------------------------------------------------------------------
+	t.Run("legacy stored script with bare placeholders is safe", func(t *testing.T) {
+		tmp := t.TempDir()
+		markerClone := filepath.Join(tmp, "pwned_legacy_clone")
+		markerCheckout := filepath.Join(tmp, "pwned_legacy_checkout")
+
+		// Mimic an old stored prepare_script: both the clone branch and the
+		// checkout reference the placeholders BARE (no surrounding quotes),
+		// which is how pre-fix default templates were snapshotted.
+		legacyStored := "" +
+			"git clone --depth=1 --branch {{repository.branch}} {{repository.clone_url}} {{workspace.path}}\n" +
+			"git -C {{workspace.path}} checkout {{worktree.branch}}\n"
+
+		req := &ExecutorCreateRequest{
+			Metadata: map[string]interface{}{
+				MetadataKeySetupScript: legacyStored,
+				"base_branch":          "main;touch " + markerClone + ";#",
+				"repository_clone_url": "https://github.com/org/repo.git",
+				"repository_path":      "/tmp/repo",
+				"worktree_branch":      "$(touch " + markerCheckout + ")",
+			},
+			Env: map[string]string{},
+		}
+
+		script := exec.resolvePrepareScript(req)
+		assertNoUnquotedPayload(t, script, "main;touch "+markerClone+";#")
+		assertNoUnquotedPayload(t, script, "$(touch "+markerCheckout+")")
+
+		// Execute both legacy lines through the real eval sink.
+		runViaEval(t, tmp, extractLine(t, script, "git clone --depth=1 --branch"))
+		runViaEval(t, tmp, extractLine(t, script, "git -C"))
+
+		if _, err := os.Stat(markerClone); err == nil {
+			t.Fatalf("REGRESSION: legacy bare clone line executed injection: %s", markerClone)
+		}
+		if _, err := os.Stat(markerCheckout); err == nil {
+			t.Fatalf("REGRESSION: legacy checkout line executed injection: %s", markerCheckout)
+		}
+		t.Logf("safe: legacy stored script did not execute injected commands")
+	})
+}
+
+// assertNoUnquotedPayload fails if any character of the payload appears outside
+// an open single-quoted region — the only shell context where command
+// substitution / `;` is inert. It tracks real single-quote state char-by-char
+// (a `'` toggles the region) rather than the weaker "some quote before and
+// after" heuristic, so a line like `a='x' $(payload) b='y'` is correctly
+// rejected. Assumes the payload contains no single quote of its own (true for
+// these test payloads), so a quoted occurrence stays wholly inside one region.
+func assertNoUnquotedPayload(t *testing.T, script, payload string) {
+	t.Helper()
+	for _, line := range strings.Split(script, "\n") {
+		// inQuote[i] == true means byte i of the line sits inside an open
+		// single-quoted region.
+		inQuote := make([]bool, len(line))
+		open := false
+		for i := 0; i < len(line); i++ {
+			if line[i] == '\'' {
+				open = !open // the quote char itself is a delimiter
+				inQuote[i] = false
+				continue
+			}
+			inQuote[i] = open
+		}
+		for off := 0; ; {
+			idx := strings.Index(line[off:], payload)
+			if idx < 0 {
+				break
+			}
+			abs := off + idx
+			if !inQuote[abs] {
+				t.Fatalf("payload starts outside a single-quoted region; line injectable:\n%s", line)
+			}
+			off = abs + len(payload)
+		}
+	}
+}
+
+// runViaEval mimics container.go's bootstrap: it passes the snippet to a shell
+// as $KANDEV_PREPARE_SCRIPT and runs `eval "$KANDEV_PREPARE_SCRIPT"` inside
+// `sh -c`, exactly like the real execution sink.
+func runViaEval(t *testing.T, dir, snippet string) {
+	t.Helper()
+	cmd := exec.Command("sh", "-c", `eval "$KANDEV_PREPARE_SCRIPT"`)
+	cmd.Dir = dir
+	cmd.Env = append(os.Environ(), "KANDEV_PREPARE_SCRIPT="+snippet)
+	out, err := cmd.CombinedOutput()
+	// A non-zero exit is expected (git clone fails, checkout of a bogus branch
+	// fails); the injected side effect is what we assert against.
+	t.Logf("eval output (err=%v):\n%s", err, string(out))
+}
+
+// extractPostlude returns the kandev-managed postlude subshell block.
+func extractPostlude(t *testing.T, script string) string {
+	t.Helper()
+	idx := strings.Index(script, "# ---- kandev-managed:")
+	if idx < 0 {
+		t.Fatalf("could not locate postlude in script:\n%s", script)
+	}
+	return script[idx:]
+}
+
+// extractLine returns the first line containing prefix.
+func extractLine(t *testing.T, script, prefix string) string {
+	t.Helper()
+	for _, line := range strings.Split(script, "\n") {
+		if strings.Contains(line, prefix) {
+			return line
+		}
+	}
+	t.Fatalf("could not find line with prefix %q in:\n%s", prefix, script)
+	return ""
+}
@@ -43,17 +43,25 @@ func KandevBranchCheckoutPostlude() string {
 # branch off origin, and only as a last resort create the branch off HEAD.
 # The previous "git checkout -B feature origin/feature" form was destructive
 # for the resume case — overwriting local commits with the remote tip.
+#
+# SECURITY: the data placeholders below are referenced BARE (no surrounding
+# quotes). The scriptengine providers substitute a fully single-quoted,
+# self-contained shell token (see scriptengine.shellQuote), so a branch name
+# containing shell metacharacters (e.g. "$(...)", backticks, ";") resolves to a
+# quoted literal that cannot inject commands. Do NOT wrap these in double
+# quotes — double quotes would re-expose $(...) command substitution. Do NOT
+# assume they are unquoted data either; the value carries its own quoting.
 (
-  if [ -d "{{workspace.path}}/.git" ] \
-     && [ -n "{{worktree.branch}}" ] \
-     && [ "{{worktree.branch}}" != "{{repository.branch}}" ]; then
-    cd "{{workspace.path}}" || exit 0
-    if git rev-parse --verify "{{worktree.branch}}" >/dev/null 2>&1; then
-      git checkout "{{worktree.branch}}"
-    elif git fetch --depth=1 origin "{{worktree.branch}}" 2>/dev/null; then
-      git checkout -b "{{worktree.branch}}" "origin/{{worktree.branch}}"
+  if [ -d {{workspace.path}}/.git ] \
+     && [ -n {{worktree.branch}} ] \
+     && [ {{worktree.branch}} != {{repository.branch}} ]; then
+    cd {{workspace.path}} || exit 0
+    if git rev-parse --verify {{worktree.branch}} >/dev/null 2>&1; then
+      git checkout {{worktree.branch}}
+    elif git fetch --depth=1 origin {{worktree.branch}} 2>/dev/null; then
+      git checkout -b {{worktree.branch}} origin/{{worktree.branch}}
    else
-      git checkout -b "{{worktree.branch}}"
+      git checkout -b {{worktree.branch}}
    fi
  fi
 ) || true
@@ -104,6 +112,10 @@ git config --global url."https://github.com/".insteadOf "ssh://git@github.com/"
 # The kandev-managed feature-branch checkout is appended as an invariant
 # postlude (see KandevBranchCheckoutPostlude) — keep it out of the default
 # so old profiles snapshotting this script and the postlude never disagree.
+# SECURITY: the providers substitute fully single-quoted tokens (shellQuote) for
+# repository.branch / repository.clone_url / workspace.path, so a hostile branch
+# name or URL cannot break out of the git clone argument even though the
+# placeholders are referenced bare here. Do not add double quotes around them.
 git clone --depth=1 --branch {{repository.branch}} {{repository.clone_url}} {{workspace.path}}
 cd {{workspace.path}}

@@ -147,7 +159,11 @@ chmod +x /usr/local/bin/pnpm
 # The kandev-managed feature-branch checkout is appended as an invariant
 # postlude (see KandevBranchCheckoutPostlude) — keep it out of the default
 # so old profiles snapshotting this script and the postlude never disagree.
-echo "Cloning {{repository.clone_url}} (branch: {{repository.branch}})..."
+# SECURITY: the providers substitute fully single-quoted tokens (shellQuote) for
+# these placeholders. printf takes them as separate arguments, NOT inside a
+# double-quoted string (which would re-expand a hostile URL/branch). Do not
+# reintroduce an echo of these placeholders inside a double-quoted string.
+printf 'Cloning %s (branch: %s)...\n' {{repository.clone_url}} {{repository.branch}}
 git clone --depth=1 --quiet --branch {{repository.branch}} {{repository.clone_url}} {{workspace.path}}
 cd {{workspace.path}}

@@ -14,15 +14,19 @@ import (
 // part of the default) still get the checkout.
 func TestKandevBranchCheckoutPostlude_HasInvariantSteps(t *testing.T) {
 	postlude := KandevBranchCheckoutPostlude()
+	// Data placeholders are referenced BARE; the scriptengine providers
+	// substitute a self-contained single-quoted token (shellQuote), so a
+	// hostile branch name resolves to a quoted literal. The placeholders must
+	// NOT be double-quoted here (double quotes would re-expose $(...)).
 	want := []string{
-		`if [ -d "{{workspace.path}}/.git" ]`,
-		`[ -n "{{worktree.branch}}" ]`,
-		`[ "{{worktree.branch}}" != "{{repository.branch}}" ]`,
-		`cd "{{workspace.path}}"`,
-		`git rev-parse --verify "{{worktree.branch}}"`,
-		`git fetch --depth=1 origin "{{worktree.branch}}"`,
-		`git checkout -b "{{worktree.branch}}" "origin/{{worktree.branch}}"`,
-		`git checkout -b "{{worktree.branch}}"`,
+		`if [ -d {{workspace.path}}/.git ]`,
+		`[ -n {{worktree.branch}} ]`,
+		`[ {{worktree.branch}} != {{repository.branch}} ]`,
+		`cd {{workspace.path}}`,
+		`git rev-parse --verify {{worktree.branch}}`,
+		`git fetch --depth=1 origin {{worktree.branch}}`,
+		`git checkout -b {{worktree.branch}} origin/{{worktree.branch}}`,
+		`git checkout -b {{worktree.branch}}`,
 		`|| true`,
 	}
 	for _, w := range want {
@@ -30,10 +34,17 @@ func TestKandevBranchCheckoutPostlude_HasInvariantSteps(t *testing.T) {
 			t.Errorf("postlude missing %q", w)
 		}
 	}
-	// The destructive `-B "branch" "origin/branch"` form orphaned local
-	// commits on resume — verify it does NOT come back.
+	// Data placeholders must never be double-quoted: double quotes do not stop
+	// $(...) / backtick command substitution, which is the RCE we fixed.
+	if strings.Contains(postlude, `"{{worktree.branch}}"`) ||
+		strings.Contains(postlude, `"{{repository.branch}}"`) ||
+		strings.Contains(postlude, `"{{workspace.path}}`) {
+		t.Errorf("postlude must not double-quote data placeholders:\n%s", postlude)
+	}
+	// The destructive `-B branch origin/branch` form orphaned local commits on
+	// resume — verify it does NOT come back.
 	forbidden := []string{
-		`git checkout -B "{{worktree.branch}}" "origin/{{worktree.branch}}"`,
+		`git checkout -B {{worktree.branch}} origin/{{worktree.branch}}`,
 	}
 	for _, f := range forbidden {
 		if strings.Contains(postlude, f) {
@@ -183,8 +194,8 @@ func runIn(t *testing.T, dir string, cmd string, args ...string) []byte {
 func TestDefaultPrepareScripts_NoInlineFeatureBranchCheckout(t *testing.T) {
 	executors := []string{"local_docker", "remote_docker", "sprites"}
 	forbidden := []string{
-		`if [ -n "{{worktree.branch}}" ] && [ "{{worktree.branch}}" != "{{repository.branch}}" ]; then`,
-		`git checkout -B "{{worktree.branch}}" "origin/{{worktree.branch}}"`,
+		`if [ -n {{worktree.branch}} ] && [ {{worktree.branch}} != {{repository.branch}} ]; then`,
+		`git checkout -B {{worktree.branch}} origin/{{worktree.branch}}`,
 	}

 	for _, executorType := range executors {
@@ -702,7 +702,7 @@ func TestResolvePrepareScript(t *testing.T) {
 		if !strings.Contains(script, "git remote set-url") {
 			t.Error("expected token stripping after clone")
 		}
-		if !strings.Contains(script, "git checkout -b \"feature/task-abc\"") {
+		if !strings.Contains(script, "git checkout -b 'feature/task-abc'") {
 			t.Fatalf("expected Docker prepare script to create task branch, got:\n%s", script)
 		}
 	})
@@ -83,7 +83,10 @@ func TestResolvePreparerSetupScript_UsesExplicitScript(t *testing.T) {
 	}

 	got := resolvePreparerSetupScript(req, "/tmp/my-repo")
-	if strings.TrimSpace(got) != "echo /tmp/my-repo" {
+	// Data placeholders resolve to a self-contained single-quoted shell token
+	// (security: shellQuote) — functionally identical for echo, safe if the
+	// value ever carried shell metacharacters.
+	if strings.TrimSpace(got) != "echo '/tmp/my-repo'" {
 		t.Fatalf("expected explicit script to be used and resolved, got %q", got)
 	}
 }
@@ -106,12 +109,14 @@ func TestResolvePreparerSetupScript_WorktreePlaceholders(t *testing.T) {
 	}

 	got := resolvePreparerSetupScript(req, "/tmp/worktrees/wt-123")
+	// Data placeholders resolve to self-contained single-quoted tokens
+	// (shellQuote); worktree.id is a kandev UUID and stays unquoted.
 	expected := []string{
-		"echo /tmp/worktrees",
-		"echo /tmp/worktrees/wt-123",
+		"echo '/tmp/worktrees'",
+		"echo '/tmp/worktrees/wt-123'",
 		"echo wt-123",
-		"echo feature/test-abc",
-		"echo main",
+		"echo 'feature/test-abc'",
+		"echo 'main'",
 	}
 	for _, want := range expected {
 		if !strings.Contains(got, want) {
@@ -10,6 +10,7 @@ import (
 	"go.uber.org/zap"

 	promptcfg "github.com/kandev/kandev/config/prompts"
+	"github.com/kandev/kandev/internal/common/securityutil"
 	"github.com/kandev/kandev/internal/events"
 	"github.com/kandev/kandev/internal/events/bus"
 	"github.com/kandev/kandev/internal/github"
@@ -512,6 +513,19 @@ func (s *Service) resolveReviewRepository(ctx context.Context, workspaceID strin
 		zap.String("repo", repoSlug),
 		zap.String("repo_id", repoID),
 		zap.String("base_branch", baseBranch))
+	// SECURITY (defense-in-depth): the PR head branch is attacker-controlled for
+	// fork PRs and flows unescaped-at-source into executor prepare scripts via
+	// {{worktree.branch}} / {{repository.branch}}. Reject any head branch that
+	// isn't a plain, safe git ref before it ever reaches CheckoutBranch. We still
+	// create the review task (just without a checkout) so a malicious ref cannot
+	// suppress review entirely; the scriptengine now also shell-escapes values.
+	if !securityutil.IsValidBranchName(pr.HeadBranch) {
+		s.logger.Warn("PR head branch failed branch-name validation; creating review task without checkout branch",
+			zap.String("repo", repoSlug),
+			zap.String("pr_head_branch", pr.HeadBranch),
+			zap.Int("pr_number", pr.Number))
+		return []ReviewTaskRepository{{RepositoryID: repoID, BaseBranch: baseBranch, PRNumber: pr.Number}}
+	}
 	// BaseBranch = repo default branch (e.g. "main") for worktree creation.
 	// CheckoutBranch = PR head branch to fetch and checkout after worktree is created.
 	return []ReviewTaskRepository{{RepositoryID: repoID, BaseBranch: baseBranch, CheckoutBranch: pr.HeadBranch, PRNumber: pr.Number}}
@@ -0,0 +1,70 @@
+package orchestrator
+
+import (
+	"context"
+	"testing"
+
+	"github.com/kandev/kandev/internal/github"
+)
+
+// stubReviewResolver is a minimal RepositoryResolver that always resolves to a
+// fixed repo/base-branch, so resolveReviewRepository reaches the head-branch
+// validation branch under test.
+type stubReviewResolver struct {
+	repoID     string
+	baseBranch string
+}
+
+func (s stubReviewResolver) ResolveForReview(
+	_ context.Context, _, _, _, _, _ string,
+) (string, string, error) {
+	return s.repoID, s.baseBranch, nil
+}
+
+// TestResolveReviewRepository_RejectsUnsafeHeadBranch is the ingestion-layer
+// (defense-in-depth) guard for the branch-name command-injection RCE. A fork PR
+// head branch is attacker-controlled and flows into executor prepare scripts via
+// {{worktree.branch}} / {{repository.branch}}. resolveReviewRepository must not
+// propagate a head branch that fails securityutil.IsValidBranchName into
+// CheckoutBranch — but it must still create the review task (without a checkout)
+// so a hostile branch name cannot suppress review entirely.
+func TestResolveReviewRepository_RejectsUnsafeHeadBranch(t *testing.T) {
+	svc := &Service{
+		logger:             testLogger(),
+		repositoryResolver: stubReviewResolver{repoID: "repo1", baseBranch: "main"},
+	}
+
+	cases := []struct {
+		name         string
+		headBranch   string
+		wantCheckout string
+	}{
+		{name: "safe branch is checked out", headBranch: "feature/login", wantCheckout: "feature/login"},
+		{name: "command substitution rejected", headBranch: "$(touch pwned)", wantCheckout: ""},
+		{name: "semicolon chaining rejected", headBranch: "main;touch pwned", wantCheckout: ""},
+		{name: "backtick rejected", headBranch: "`id`", wantCheckout: ""},
+		{name: "space rejected", headBranch: "a b", wantCheckout: ""},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			pr := &github.PR{
+				Number:     7,
+				RepoOwner:  "myorg",
+				RepoName:   "myrepo",
+				BaseBranch: "main",
+				HeadBranch: tc.headBranch,
+			}
+			got := svc.resolveReviewRepository(context.Background(), "ws1", pr)
+			if len(got) != 1 {
+				t.Fatalf("expected 1 repo row, got %d", len(got))
+			}
+			if got[0].RepositoryID != "repo1" {
+				t.Fatalf("RepositoryID = %q, want repo1", got[0].RepositoryID)
+			}
+			if got[0].CheckoutBranch != tc.wantCheckout {
+				t.Fatalf("CheckoutBranch = %q, want %q (head=%q)", got[0].CheckoutBranch, tc.wantCheckout, tc.headBranch)
+			}
+		})
+	}
+}
@@ -20,18 +20,26 @@ func RepositoryProvider(
 	return func() map[string]string {
 		vars := make(map[string]string)

+		// SECURITY: repository.path/name/branch/clone_url/ssh_url are DATA that
+		// land in shell text (git clone args, etc.). shellQuote emits a fully
+		// single-quoted, self-contained shell token so a hostile value (e.g. a
+		// fork PR branch named "$(...)") is a literal string even when a
+		// template (including an OLD stored prepare_script) references the
+		// placeholder BARE, e.g. `--branch {{repository.branch}}`.
 		repoPath := getMetaString(metadata, "repository_path")
 		if repoPath != "" {
-			vars["repository.path"] = repoPath
-			vars["repository.name"] = repoNameFromPath(repoPath)
+			vars["repository.path"] = shellQuote(repoPath)
+			vars["repository.name"] = shellQuote(repoNameFromPath(repoPath))
 		}

 		branch := getMetaString(metadata, "base_branch")
 		if branch == "" {
 			branch = getMetaString(metadata, "repository_branch")
 		}
-		vars["repository.branch"] = branch
+		vars["repository.branch"] = shellQuote(branch)

+		// repository.setup_script is a script FRAGMENT (intentional multi-line
+		// shell), not data — do NOT quote it.
 		setupScript := getMetaString(metadata, "repository_setup_script")
 		vars["repository.setup_script"] = setupScript

@@ -39,12 +47,12 @@ func RepositoryProvider(
 		cloneURL := getMetaString(metadata, "repository_clone_url")
 		if cloneURL == "" && repoPath != "" && repoURLResolver != nil {
 			if remoteURL, err := repoURLResolver(repoPath); err == nil && remoteURL != "" {
-				vars["repository.ssh_url"] = remoteURL
+				vars["repository.ssh_url"] = shellQuote(remoteURL)
 				cloneURL = remoteURL
 			}
 		}
 		if cloneURL != "" {
-			vars["repository.clone_url"] = injectToken(cloneURL, env, tokenInjector)
+			vars["repository.clone_url"] = shellQuote(injectToken(cloneURL, env, tokenInjector))
 		}

 		return vars
@@ -67,23 +75,34 @@ func AgentctlProvider(agentctlPort int, workspacePath string) PlaceholderProvide
 }

 // WorkspaceProvider returns workspace path placeholder.
+//
+// SECURITY: workspace.path is DATA in shell text; shellQuote emits a
+// self-contained single-quoted token so it stays a literal even if it ever
+// carries metacharacters and even when referenced bare in a template.
 func WorkspaceProvider(workspacePath string) PlaceholderProvider {
 	return func() map[string]string {
 		return map[string]string{
-			"workspace.path": workspacePath,
+			"workspace.path": shellQuote(workspacePath),
 		}
 	}
 }

 // WorktreeProvider returns placeholders that describe the selected worktree context.
+//
+// SECURITY: worktree paths and the (untrusted, possibly fork-PR-controlled)
+// branch names are DATA that land in shell text. shellQuote emits a
+// self-contained single-quoted token so a hostile branch like "$(touch pwned)"
+// cannot inject commands even when a stored/legacy template references the
+// placeholder bare. worktree.id is a kandev-generated UUID (not shell data),
+// left as-is.
 func WorktreeProvider(basePath, path, id, branch, baseBranch string) PlaceholderProvider {
 	return func() map[string]string {
 		return map[string]string{
-			"worktree.base_path":   basePath,
-			"worktree.path":        path,
+			"worktree.base_path":   shellQuote(basePath),
+			"worktree.path":        shellQuote(path),
 			"worktree.id":          id,
-			"worktree.branch":      branch,
-			"worktree.base_branch": baseBranch,
+			"worktree.branch":      shellQuote(branch),
+			"worktree.base_branch": shellQuote(baseBranch),
 		}
 	}
 }
@@ -163,10 +182,25 @@ func AgentInstallProvider(installScripts []string) PlaceholderProvider {
 	}
 }

+// shellSingleQuote escapes embedded single quotes for use INSIDE an existing
+// pair of single quotes supplied by the caller/template (e.g. '%s'). It does
+// NOT add the surrounding quotes.
 func shellSingleQuote(value string) string {
 	return strings.ReplaceAll(value, "'", `'"'"'`)
 }

+// shellQuote returns value as a complete, self-contained single-quoted shell
+// token (surrounding quotes included, embedded quotes escaped). The result is
+// safe to drop into a script UNQUOTED — `--branch <shellQuote(v)>` cannot be
+// broken out of by shell metacharacters ($(), backticks, ;, |, whitespace).
+//
+// Prefer this over shellSingleQuote for any DATA placeholder, because stored
+// prepare_script templates (snapshotted before a template was hardened) may
+// reference the placeholder bare, so the value itself must carry its quoting.
+func shellQuote(value string) string {
+	return "'" + shellSingleQuote(value) + "'"
+}
+
 // GitHubAuthProvider returns placeholders for GitHub authentication setup.
 // It configures both the gh CLI and git credential helper to use the GitHub token.
 func GitHubAuthProvider(env map[string]string) PlaceholderProvider {
@@ -1,10 +1,26 @@
 package scriptengine

 import (
+	"os/exec"
 	"strings"
 	"testing"
 )

+// shellUnquote runs `printf %s <arg>` through /bin/sh so the test can prove that
+// a single-quoted, shell-escaped value parses back to the intended literal
+// without any command substitution firing.
+func shellUnquote(t *testing.T, arg string) string {
+	t.Helper()
+	if _, err := exec.LookPath("sh"); err != nil {
+		t.Skip("sh not available")
+	}
+	out, err := exec.Command("sh", "-c", "printf %s "+arg).CombinedOutput()
+	if err != nil {
+		t.Fatalf("sh failed for %q: %v\n%s", arg, err, out)
+	}
+	return string(out)
+}
+
 func TestAgentInstallProvider(t *testing.T) {
 	t.Run("empty scripts produce empty placeholder", func(t *testing.T) {
 		provider := AgentInstallProvider(nil)
@@ -53,6 +69,83 @@ func TestRepositoryProvider_UsesRepositorySetupScriptKey(t *testing.T) {
 	}
 }

+// TestProviders_ShellEscapeDataPlaceholders is the scriptengine-level
+// regression guard for the branch-name command-injection RCE. Every data
+// placeholder that lands in shell text (branch/url/path) must be emitted as a
+// fully self-contained single-quoted token (shellQuote), so a hostile value
+// like "$(touch pwned)" or "a;b" is a literal even when a template — including
+// a stored/legacy prepare_script — references the placeholder BARE.
+func TestProviders_ShellEscapeDataPlaceholders(t *testing.T) {
+	// A payload containing a single quote exercises the escape sequence: the
+	// embedded `'` becomes `'"'"'`, and shellQuote wraps the whole in `'...'`.
+	const evil = `x'$(touch pwned)`
+	const wantQuoted = `'x'"'"'$(touch pwned)'` // '  x  '"'"'  $(touch pwned)  '
+
+	// assertQuoted checks the provider emitted the fully-quoted token AND that
+	// the token parses back to the original literal through a real shell (no
+	// command substitution fires).
+	assertQuoted := func(t *testing.T, key, got string) {
+		t.Helper()
+		if got != wantQuoted {
+			t.Errorf("%s = %q, want %q", key, got, wantQuoted)
+		}
+		if out := shellUnquote(t, got); out != evil {
+			t.Errorf("%s token %q evaluated to %q, want literal %q", key, got, out, evil)
+		}
+	}
+
+	t.Run("WorktreeProvider quotes branch and paths", func(t *testing.T) {
+		vars := WorktreeProvider(evil, evil, "wt-id", evil, evil)()
+		for _, key := range []string{"worktree.base_path", "worktree.path", "worktree.branch", "worktree.base_branch"} {
+			assertQuoted(t, key, vars[key])
+		}
+		// worktree.id is a kandev UUID, intentionally not quoted.
+		if got := vars["worktree.id"]; got != "wt-id" {
+			t.Errorf("worktree.id = %q, want unmodified", got)
+		}
+	})
+
+	t.Run("WorkspaceProvider quotes path", func(t *testing.T) {
+		assertQuoted(t, "workspace.path", WorkspaceProvider(evil)()["workspace.path"])
+	})
+
+	t.Run("RepositoryProvider quotes branch, paths and clone url", func(t *testing.T) {
+		vars := RepositoryProvider(map[string]any{
+			"base_branch":          evil,
+			"repository_clone_url": evil,
+			"repository_path":      evil,
+		}, nil, nil, nil)()
+		assertQuoted(t, "repository.branch", vars["repository.branch"])
+		assertQuoted(t, "repository.clone_url", vars["repository.clone_url"])
+		assertQuoted(t, "repository.path", vars["repository.path"])
+		// repository.name is derived from the (hostile) path's last segment and
+		// must also be quoted, since it can land in shell text.
+		assertQuoted(t, "repository.name", vars["repository.name"])
+	})
+
+	t.Run("resolver-derived repository.ssh_url is quoted", func(t *testing.T) {
+		// When no explicit clone URL is set, the provider resolves the remote
+		// via repoURLResolver; that value is attacker-influenceable (a remote
+		// URL) and must be quoted too.
+		resolver := func(string) (string, error) { return evil, nil }
+		vars := RepositoryProvider(map[string]any{
+			"repository_path": "/tmp/repo",
+		}, nil, resolver, nil)()
+		assertQuoted(t, "repository.ssh_url", vars["repository.ssh_url"])
+		assertQuoted(t, "repository.clone_url", vars["repository.clone_url"])
+	})
+
+	t.Run("repository.setup_script is NOT quoted (script fragment)", func(t *testing.T) {
+		fragment := "npm ci\necho 'hi'"
+		vars := RepositoryProvider(map[string]any{
+			"repository_setup_script": fragment,
+		}, nil, nil, nil)()
+		if got := vars["repository.setup_script"]; got != fragment {
+			t.Errorf("repository.setup_script = %q, want unmodified %q", got, fragment)
+		}
+	})
+}
+
 func TestGitHubAuthProvider(t *testing.T) {
 	t.Run("nil env returns fallback commands", func(t *testing.T) {
 		provider := GitHubAuthProvider(nil)