diff --git a/apps/backend/internal/agent/runtime/lifecycle/branch_injection_poc_test.go b/apps/backend/internal/agent/runtime/lifecycle/branch_injection_poc_test.go new file mode 100644 index 0000000000..b86f00d2b9 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/branch_injection_poc_test.go @@ -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 "" +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go b/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go index 641ef3a0d9..ce01c66a32 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go +++ b/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go @@ -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}} diff --git a/apps/backend/internal/agent/runtime/lifecycle/default_scripts_test.go b/apps/backend/internal/agent/runtime/lifecycle/default_scripts_test.go index 7fdb91f60a..e46f3fd11b 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/default_scripts_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/default_scripts_test.go @@ -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 { diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go index 95897f264c..fa9eb20c4b 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go @@ -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) } }) diff --git a/apps/backend/internal/agent/runtime/lifecycle/preparer_script_test.go b/apps/backend/internal/agent/runtime/lifecycle/preparer_script_test.go index 13a0dc69cc..db05700431 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/preparer_script_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/preparer_script_test.go @@ -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) { diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index 6191b25594..79404e1fae 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -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}} diff --git a/apps/backend/internal/orchestrator/event_handlers_github_branch_injection_test.go b/apps/backend/internal/orchestrator/event_handlers_github_branch_injection_test.go new file mode 100644 index 0000000000..ada28f7598 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_github_branch_injection_test.go @@ -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) + } + }) + } +} diff --git a/apps/backend/internal/scriptengine/providers.go b/apps/backend/internal/scriptengine/providers.go index d371e8cf15..eb1a9c9194 100644 --- a/apps/backend/internal/scriptengine/providers.go +++ b/apps/backend/internal/scriptengine/providers.go @@ -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 ` 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 { diff --git a/apps/backend/internal/scriptengine/providers_test.go b/apps/backend/internal/scriptengine/providers_test.go index c6156fe59f..843ae33b29 100644 --- a/apps/backend/internal/scriptengine/providers_test.go +++ b/apps/backend/internal/scriptengine/providers_test.go @@ -1,10 +1,26 @@ package scriptengine import ( + "os/exec" "strings" "testing" ) +// shellUnquote runs `printf %s ` 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)