8f23b8a19e
* 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>
130 lines
3.8 KiB
Go
130 lines
3.8 KiB
Go
package lifecycle
|
|
|
|
import (
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/kandev/kandev/internal/agent/executor"
|
|
)
|
|
|
|
func TestResolvePreparerSetupScript_LocalFallbackCommentOnly(t *testing.T) {
|
|
req := &EnvPrepareRequest{
|
|
ExecutorType: executor.NameStandalone,
|
|
RepositoryPath: "/tmp/my-repo",
|
|
}
|
|
|
|
got := resolvePreparerSetupScript(req, "/tmp/my-repo")
|
|
if got != "" {
|
|
t.Fatalf("expected comment-only default script to be treated as empty, got %q", got)
|
|
}
|
|
}
|
|
|
|
func TestIsScriptEffectivelyEmpty(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
input string
|
|
want bool
|
|
}{
|
|
{"empty", "", true},
|
|
{"shebang only", "#!/bin/bash\n", true},
|
|
{"shebang and comments", "#!/bin/bash\n# comment\n# another\n", true},
|
|
{"blank lines and comments", "\n# comment\n\n# more\n\n", true},
|
|
{"has command", "#!/bin/bash\necho hello\n", false},
|
|
{"command after comments", "#!/bin/bash\n# setup\napt-get install git\n", false},
|
|
}
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
got := isScriptEffectivelyEmpty(tt.input)
|
|
if got != tt.want {
|
|
t.Fatalf("isScriptEffectivelyEmpty(%q) = %v, want %v", tt.input, got, tt.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestResolvePreparerSetupScript_LocalWithRepoSetupScript(t *testing.T) {
|
|
req := &EnvPrepareRequest{
|
|
ExecutorType: executor.NameStandalone,
|
|
RepositoryPath: "/tmp/my-repo",
|
|
RepoSetupScript: "make install",
|
|
}
|
|
|
|
got := resolvePreparerSetupScript(req, "/tmp/my-repo")
|
|
if got == "" {
|
|
t.Fatal("expected non-empty script when repo setup script is set")
|
|
}
|
|
if !strings.Contains(got, "make install") {
|
|
t.Fatalf("expected repo setup script in resolved output, got %q", got)
|
|
}
|
|
}
|
|
|
|
func TestResolvePreparerSetupScript_WorktreeWithRepoSetupScript(t *testing.T) {
|
|
req := &EnvPrepareRequest{
|
|
ExecutorType: executor.NameStandalone,
|
|
UseWorktree: true,
|
|
RepositoryPath: "/tmp/my-repo",
|
|
RepoSetupScript: "npm ci",
|
|
}
|
|
|
|
got := resolvePreparerSetupScript(req, "/tmp/worktrees/wt-1")
|
|
if got == "" {
|
|
t.Fatal("expected non-empty script when repo setup script is set")
|
|
}
|
|
if !strings.Contains(got, "npm ci") {
|
|
t.Fatalf("expected repo setup script in resolved output, got %q", got)
|
|
}
|
|
}
|
|
|
|
func TestResolvePreparerSetupScript_UsesExplicitScript(t *testing.T) {
|
|
req := &EnvPrepareRequest{
|
|
ExecutorType: executor.NameStandalone,
|
|
RepositoryPath: "/tmp/my-repo",
|
|
SetupScript: "echo {{repository.path}}",
|
|
}
|
|
|
|
got := resolvePreparerSetupScript(req, "/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)
|
|
}
|
|
}
|
|
|
|
func TestResolvePreparerSetupScript_WorktreePlaceholders(t *testing.T) {
|
|
req := &EnvPrepareRequest{
|
|
ExecutorType: executor.NameStandalone,
|
|
UseWorktree: true,
|
|
RepositoryPath: "/tmp/main-repo",
|
|
BaseBranch: "main",
|
|
WorktreeID: "wt-123",
|
|
WorktreeBranch: "feature/test-abc",
|
|
SetupScript: strings.Join([]string{
|
|
"echo {{worktree.base_path}}",
|
|
"echo {{worktree.path}}",
|
|
"echo {{worktree.id}}",
|
|
"echo {{worktree.branch}}",
|
|
"echo {{worktree.base_branch}}",
|
|
}, "\n"),
|
|
}
|
|
|
|
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 wt-123",
|
|
"echo 'feature/test-abc'",
|
|
"echo 'main'",
|
|
}
|
|
for _, want := range expected {
|
|
if !strings.Contains(got, want) {
|
|
t.Fatalf("expected %q in resolved script, got %q", want, got)
|
|
}
|
|
}
|
|
if strings.Contains(got, "{{worktree.path}}") {
|
|
t.Fatalf("expected worktree placeholders to be resolved, got %q", got)
|
|
}
|
|
}
|