Files
kandev/apps/backend/internal/scriptengine/providers_test.go
T
João Salavisa 8f23b8a19e 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

219 lines
8.0 KiB
Go

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)
vars := provider()
if got := vars["kandev.agents.install"]; got != "" {
t.Fatalf("expected empty, got %q", got)
}
})
t.Run("deduplicates scripts", func(t *testing.T) {
scripts := []string{
"npm install -g @anthropic-ai/claude-code@2.1.50",
"npm install -g @openai/codex@0.104.0",
"npm install -g @anthropic-ai/claude-code@2.1.50",
}
provider := AgentInstallProvider(scripts)
vars := provider()
want := "npm install -g @anthropic-ai/claude-code@2.1.50\nnpm install -g @openai/codex@0.104.0"
if got := vars["kandev.agents.install"]; got != want {
t.Fatalf("got %q, want %q", got, want)
}
})
t.Run("trims whitespace and skips empty", func(t *testing.T) {
scripts := []string{" npm install -g foo ", "", " ", "npm install -g bar"}
provider := AgentInstallProvider(scripts)
vars := provider()
want := "npm install -g foo\nnpm install -g bar"
if got := vars["kandev.agents.install"]; got != want {
t.Fatalf("got %q, want %q", got, want)
}
})
}
func TestRepositoryProvider_UsesRepositorySetupScriptKey(t *testing.T) {
provider := RepositoryProvider(map[string]any{
"repository_path": "/tmp/repo",
"base_branch": "main",
"repository_setup_script": "npm ci",
"setup_script": "should-not-be-used",
}, nil, nil, nil)
vars := provider()
if got := vars["repository.setup_script"]; got != "npm ci" {
t.Fatalf("repository.setup_script = %q, want %q", got, "npm ci")
}
}
// 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)
vars := provider()
setup := vars["github.auth_setup"]
if !strings.Contains(setup, "fallback") {
t.Errorf("expected fallback comment, got %q", setup)
}
if strings.Contains(setup, "credential") {
t.Error("expected no credential helper when no token")
}
})
t.Run("empty env returns fallback commands", func(t *testing.T) {
provider := GitHubAuthProvider(map[string]string{})
vars := provider()
setup := vars["github.auth_setup"]
if !strings.Contains(setup, "fallback") {
t.Errorf("expected fallback comment, got %q", setup)
}
})
t.Run("GH_TOKEN present configures credential helper", func(t *testing.T) {
provider := GitHubAuthProvider(map[string]string{"GH_TOKEN": "ghp_test123"})
vars := provider()
setup := vars["github.auth_setup"]
if !strings.Contains(setup, "credential.https://github.com.helper") {
t.Errorf("expected credential helper, got %q", setup)
}
// Must use /bin/sh, not /bin/bash
if strings.Contains(setup, "/bin/bash") {
t.Error("expected /bin/sh, not /bin/bash")
}
if !strings.Contains(setup, "/bin/sh") {
t.Error("expected /bin/sh in credential helper")
}
// Token must NOT be hardcoded in the output script
if strings.Contains(setup, "ghp_test123") {
t.Error("token value must not appear literally in the script")
}
// Must use env var fallback pattern
if !strings.Contains(setup, "${GH_TOKEN:-${GITHUB_TOKEN}}") {
t.Error("expected ${GH_TOKEN:-${GITHUB_TOKEN}} fallback pattern")
}
})
t.Run("GITHUB_TOKEN fallback configures credential helper", func(t *testing.T) {
provider := GitHubAuthProvider(map[string]string{"GITHUB_TOKEN": "ghp_fallback"})
vars := provider()
setup := vars["github.auth_setup"]
if !strings.Contains(setup, "credential.https://github.com.helper") {
t.Errorf("expected credential helper with GITHUB_TOKEN fallback, got %q", setup)
}
if strings.Contains(setup, "ghp_fallback") {
t.Error("token value must not appear literally in the script")
}
})
t.Run("includes gh CLI setup", func(t *testing.T) {
provider := GitHubAuthProvider(map[string]string{"GH_TOKEN": "ghp_test"})
vars := provider()
setup := vars["github.auth_setup"]
if !strings.Contains(setup, "gh config set git_protocol https") {
t.Error("expected gh CLI protocol config")
}
if !strings.Contains(setup, "gh auth setup-git") {
t.Error("expected gh auth setup-git backup")
}
})
}