From f25a2bee62085dda8ad79e268ee7c8d1d3e712b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Mon, 10 Aug 2026 21:03:33 +0200 Subject: [PATCH] fix(agentctl): allow git pull --rebase past the flag allowlist (#2501) Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 5 (1M context) --- .../server/api/git_pull_rebase_test.go | 272 ++++++++++++++++++ .../internal/common/securityutil/git.go | 17 +- .../internal/common/securityutil/git_test.go | 32 +++ 3 files changed, 319 insertions(+), 2 deletions(-) create mode 100644 apps/backend/internal/agentctl/server/api/git_pull_rebase_test.go diff --git a/apps/backend/internal/agentctl/server/api/git_pull_rebase_test.go b/apps/backend/internal/agentctl/server/api/git_pull_rebase_test.go new file mode 100644 index 0000000000..3913b95dde --- /dev/null +++ b/apps/backend/internal/agentctl/server/api/git_pull_rebase_test.go @@ -0,0 +1,272 @@ +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/internal/agentctl/server/process" + "github.com/kandev/kandev/internal/common/logger" +) + +// postGitJSON drives a git endpoint through the real router and decodes the +// handler's result. Anything other than 200 is fatal: these handlers answer +// operational failures with 200 + Success:false, so a non-200 means the request +// never reached the GitOperator at all. +func postGitJSON(t *testing.T, srv *Server, path, body string) process.GitOperationResult { + t.Helper() + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + srv.Router().ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("POST %s status = %d, want 200: %s", path, rec.Code, rec.Body.String()) + } + var result process.GitOperationResult + if err := json.Unmarshal(rec.Body.Bytes(), &result); err != nil { + t.Fatalf("decode %s response: %v (body %s)", path, err, rec.Body.String()) + } + return result +} + +func postGitPull(t *testing.T, srv *Server, body string) process.GitOperationResult { + t.Helper() + return postGitJSON(t, srv, "/api/v1/git/pull", body) +} + +// newPullAPIServer wires the api Server over an existing working copy. +func newPullAPIServer(t *testing.T, repoDir string) *Server { + t.Helper() + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error"}) + cfg := &config.InstanceConfig{WorkDir: repoDir} + mgr := process.NewManager(cfg, log) + t.Cleanup(func() { _ = mgr.StopForTeardown(context.Background()) }) + return NewServer(cfg, mgr, nil, nil, log) +} + +// advanceOriginMain pushes one commit to the bare origin behind repoDir without +// touching repoDir's working copy, so the next pull has something to integrate. +// Returns the SHA of the new origin/main tip. +func advanceOriginMain(t *testing.T, repoDir, file, content, message string) string { + t.Helper() + remoteURL := strings.TrimSpace(runGitAPI(t, repoDir, "remote", "get-url", "origin")) + otherDir := t.TempDir() + runGitAPI(t, otherDir, "clone", remoteURL, ".") + runGitAPI(t, otherDir, "config", "user.email", "other@test.com") + runGitAPI(t, otherDir, "config", "user.name", "Other User") + runGitAPI(t, otherDir, "config", "core.hooksPath", "/dev/null") + writeFileAPI(t, otherDir, file, content) + runGitAPI(t, otherDir, "add", ".") + runGitAPI(t, otherDir, "commit", "-m", message) + runGitAPI(t, otherDir, "push", "origin", "main") + return strings.TrimSpace(runGitAPI(t, otherDir, "rev-parse", "HEAD")) +} + +// commitLocally adds one commit on the current branch of repoDir. +func commitLocally(t *testing.T, repoDir, file, content, message string) { + t.Helper() + writeFileAPI(t, repoDir, file, content) + runGitAPI(t, repoDir, "add", ".") + runGitAPI(t, repoDir, "commit", "-m", message) +} + +// TestHandleGitPull_RebaseReplaysLocalCommitOntoOrigin drives the documented +// {"rebase": true} option end to end against a real bare origin. Local main and +// origin/main have each moved on by one commit, so a successful pull must +// produce a linear history with the local commit replayed on top of the remote +// tip — a merge commit would mean the rebase flag was ignored. +// +// The flag allowlist in securityutil.IsKnownSafeGitFlag used to omit --rebase, +// so this path returned 200 with {"success":false,"error":"potentially unsafe +// flag: --rebase"} for every caller. +func TestHandleGitPull_RebaseReplaysLocalCommitOntoOrigin(t *testing.T) { + repoDir, cleanup := setupAPITestRepo(t) + t.Cleanup(cleanup) + + remoteTip := advanceOriginMain(t, repoDir, "remote.txt", "remote work\n", "feat: remote work") + commitLocally(t, repoDir, "local.txt", "local work\n", "feat: local work") + localMessage := strings.TrimSpace(runGitAPI(t, repoDir, "log", "-1", "--format=%s")) + + srv := newPullAPIServer(t, repoDir) + result := postGitPull(t, srv, `{"rebase": true}`) + + if !result.Success { + t.Fatalf("pull --rebase failed: error=%q output=%q conflicts=%v", + result.Error, result.Output, result.ConflictFiles) + } + if result.Operation != "pull" { + t.Errorf("result.Operation = %q, want %q", result.Operation, "pull") + } + + // Rebase, not merge: the local commit is still the tip, its parent is the + // remote tip, and the branch carries no merge commit. + if got := strings.TrimSpace(runGitAPI(t, repoDir, "log", "-1", "--format=%s")); got != localMessage { + t.Errorf("HEAD subject = %q, want the replayed local commit %q", got, localMessage) + } + if got := strings.TrimSpace(runGitAPI(t, repoDir, "rev-parse", "HEAD^")); got != remoteTip { + t.Errorf("HEAD^ = %q, want the origin/main tip %q — the local commit was not replayed on top", got, remoteTip) + } + if merges := strings.TrimSpace(runGitAPI(t, repoDir, "rev-list", "--merges", "HEAD")); merges != "" { + t.Errorf("branch contains merge commits %q — the pull merged instead of rebasing", merges) + } + // Both files present means neither side's work was dropped. + for _, name := range []string{"remote.txt", "local.txt"} { + if _, err := os.Stat(filepath.Join(repoDir, name)); err != nil { + t.Errorf("expected %s in the working tree after the rebase: %v", name, err) + } + } +} + +// TestHandleGitPull_RebaseConflictAutoAborts covers Pull's conflict-recovery +// path, which runs "rebase --abort" so the caller is not left in a detached +// mid-rebase state. --abort was missing from the same allowlist, so the abort +// itself failed and the repository stayed stuck. +func TestHandleGitPull_RebaseConflictAutoAborts(t *testing.T) { + repoDir, cleanup := setupAPITestRepo(t) + t.Cleanup(cleanup) + + // Both sides edit the same file so the replay conflicts. + advanceOriginMain(t, repoDir, "shared.txt", "remote version\n", "feat: remote edit") + commitLocally(t, repoDir, "shared.txt", "local version\n", "feat: local edit") + localTip := strings.TrimSpace(runGitAPI(t, repoDir, "rev-parse", "HEAD")) + + srv := newPullAPIServer(t, repoDir) + result := postGitPull(t, srv, `{"rebase": true}`) + + if result.Success { + t.Fatalf("pull --rebase reported success despite a conflicting change: %+v", result) + } + if len(result.ConflictFiles) == 0 { + t.Fatalf("expected the conflicting file to be reported, got none (error=%q output=%q)", + result.Error, result.Output) + } + if result.ConflictFiles[0] != "shared.txt" { + t.Errorf("ConflictFiles = %v, want [shared.txt]", result.ConflictFiles) + } + + // The abort must have run: no in-progress rebase state, and HEAD is back + // on the local commit with a clean tree. + for _, dir := range []string{"rebase-merge", "rebase-apply"} { + if _, err := os.Stat(filepath.Join(repoDir, ".git", dir)); err == nil { + t.Errorf(".git/%s still exists — the conflicting rebase was not aborted", dir) + } + } + if got := strings.TrimSpace(runGitAPI(t, repoDir, "rev-parse", "HEAD")); got != localTip { + t.Errorf("HEAD = %q after the aborted rebase, want the pre-pull local tip %q", got, localTip) + } + if status := strings.TrimSpace(runGitAPI(t, repoDir, "status", "--porcelain")); status != "" { + t.Errorf("working tree is dirty after the abort:\n%s", status) + } +} + +// runGitAPIExpectFailure runs git and requires a non-zero exit. Leaving a +// repository mid-rebase or mid-merge means running a command that conflicts, +// so the failure is the point — runGitAPI would call t.Fatalf on it. +func runGitAPIExpectFailure(t *testing.T, dir string, args ...string) { + t.Helper() + full := append([]string{ + "-C", dir, + "-c", "commit.gpgsign=false", + "-c", "tag.gpgsign=false", + }, args...) + cmd := exec.Command("git", full...) + env := make([]string, 0, len(os.Environ())) + for _, e := range os.Environ() { + if strings.HasPrefix(e, "GIT_") { + continue + } + env = append(env, e) + } + cmd.Env = env + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatalf("git %v was expected to conflict but succeeded:\n%s", args, out) + } +} + +// setupConflictingBranches leaves repoDir checked out on feature/x, whose only +// commit adds shared.txt with content that conflicts with main's. Returns the +// feature tip, which an abort must restore. +func setupConflictingBranches(t *testing.T, repoDir string) string { + t.Helper() + runGitAPI(t, repoDir, "checkout", "-b", "feature/x") + commitLocally(t, repoDir, "shared.txt", "feature version\n", "feat: feature edit") + featureTip := strings.TrimSpace(runGitAPI(t, repoDir, "rev-parse", "HEAD")) + + runGitAPI(t, repoDir, "checkout", "main") + commitLocally(t, repoDir, "shared.txt", "main version\n", "feat: main edit") + + runGitAPI(t, repoDir, "checkout", "feature/x") + return featureTip +} + +// assertAbortRestoredRepo pins the post-abort state: no in-progress operation +// left behind, HEAD back where it started, and a clean working tree. +func assertAbortRestoredRepo(t *testing.T, repoDir, wantHead string) { + t.Helper() + for _, name := range []string{"rebase-merge", "rebase-apply", "MERGE_HEAD"} { + if _, err := os.Stat(filepath.Join(repoDir, ".git", name)); err == nil { + t.Errorf(".git/%s still exists — the operation was not aborted", name) + } + } + if got := strings.TrimSpace(runGitAPI(t, repoDir, "rev-parse", "HEAD")); got != wantHead { + t.Errorf("HEAD = %q after the abort, want %q", got, wantHead) + } + if status := strings.TrimSpace(runGitAPI(t, repoDir, "status", "--porcelain")); status != "" { + t.Errorf("working tree is dirty after the abort:\n%s", status) + } +} + +// TestHandleGitAbort_ClearsInProgressRebase drives POST /api/v1/git/abort with +// {"operation": "rebase"} against a repository genuinely stuck mid-rebase. +// GitOperator.Abort issues "rebase --abort", which the flag allowlist rejected, +// so this endpoint failed for every caller regardless of repository state. +func TestHandleGitAbort_ClearsInProgressRebase(t *testing.T) { + repoDir, cleanup := setupAPITestRepo(t) + t.Cleanup(cleanup) + + featureTip := setupConflictingBranches(t, repoDir) + runGitAPIExpectFailure(t, repoDir, "rebase", "main") + if _, err := os.Stat(filepath.Join(repoDir, ".git", "rebase-merge")); err != nil { + t.Fatalf("fixture did not leave a rebase in progress: %v", err) + } + + srv := newPullAPIServer(t, repoDir) + result := postGitJSON(t, srv, "/api/v1/git/abort", `{"operation": "rebase"}`) + + if !result.Success { + t.Fatalf("abort rebase failed: error=%q output=%q", result.Error, result.Output) + } + if result.Operation != "abort" { + t.Errorf("result.Operation = %q, want %q", result.Operation, "abort") + } + assertAbortRestoredRepo(t, repoDir, featureTip) +} + +// TestHandleGitAbort_ClearsInProgressMerge is the "merge" half of the same +// endpoint; it issues "merge --abort" through the same allowlist. +func TestHandleGitAbort_ClearsInProgressMerge(t *testing.T) { + repoDir, cleanup := setupAPITestRepo(t) + t.Cleanup(cleanup) + + featureTip := setupConflictingBranches(t, repoDir) + runGitAPIExpectFailure(t, repoDir, "merge", "main") + if _, err := os.Stat(filepath.Join(repoDir, ".git", "MERGE_HEAD")); err != nil { + t.Fatalf("fixture did not leave a merge in progress: %v", err) + } + + srv := newPullAPIServer(t, repoDir) + result := postGitJSON(t, srv, "/api/v1/git/abort", `{"operation": "merge"}`) + + if !result.Success { + t.Fatalf("abort merge failed: error=%q output=%q", result.Error, result.Output) + } + assertAbortRestoredRepo(t, repoDir, featureTip) +} diff --git a/apps/backend/internal/common/securityutil/git.go b/apps/backend/internal/common/securityutil/git.go index a4f63b7782..7f8e59bc75 100644 --- a/apps/backend/internal/common/securityutil/git.go +++ b/apps/backend/internal/common/securityutil/git.go @@ -55,6 +55,20 @@ func IsValidBaseBranchRef(ref string) bool { // This prevents argument injection where user input could introduce malicious flags. // Only flags actually used by the Kandev codebase are whitelisted. func IsKnownSafeGitFlag(arg string) bool { + // Flags allowed only as an exact argument, because the prefix match below + // would admit variants this codebase never issues: "--rebase" would let + // through "--rebase-merges" and "--rebase=interactive" (which spawns an + // editor), and "--abort" would let through "--abort-*". + exactFlags := []string{ + "--", // Path separator - everything after this is treated as paths, not flags + "--rebase", + "--abort", + } + for _, safe := range exactFlags { + if arg == safe { + return true + } + } // Whitelist of git flags actually used by our codebase safeFlags := []string{ "-m", "-M", "-n", "--set-upstream", "--all", "--porcelain", "--short", @@ -63,10 +77,9 @@ func IsKnownSafeGitFlag(arg string) bool { "--amend", "--allow-empty", "--soft", "--mixed", "--hard", "--cached", "--force", "--source=HEAD", "--staged", "--worktree", "--dry-run", "--get-all", "--first-parent", "--is-ancestor", - "--", // Path separator - everything after this is treated as paths, not flags } for _, safe := range safeFlags { - if arg == safe || (safe != "--" && strings.HasPrefix(arg, safe)) { + if strings.HasPrefix(arg, safe) { return true } } diff --git a/apps/backend/internal/common/securityutil/git_test.go b/apps/backend/internal/common/securityutil/git_test.go index 7f25915dc8..8d3cb62a9e 100644 --- a/apps/backend/internal/common/securityutil/git_test.go +++ b/apps/backend/internal/common/securityutil/git_test.go @@ -42,3 +42,35 @@ func TestIsKnownSafeGitFlagRejectsPushOptionPrefixes(t *testing.T) { } } } + +// TestIsKnownSafeGitFlagAllowsRebaseAndAbort pins the two flags the git-pull +// rebase path needs. GitOperator.Pull builds "pull --rebase " +// and, when the rebase conflicts, recovers with "rebase --abort"; both were +// rejected here, so POST /api/v1/git/pull {"rebase":true} could never succeed +// and POST /api/v1/git/abort never worked at all. +func TestIsKnownSafeGitFlagAllowsRebaseAndAbort(t *testing.T) { + for _, flag := range []string{"--rebase", "--abort"} { + if !IsKnownSafeGitFlag(flag) { + t.Fatalf("IsKnownSafeGitFlag(%q) = false, want true — the git pull --rebase path needs it", flag) + } + } +} + +// TestIsKnownSafeGitFlagRejectsRebaseAndAbortVariants pins that --rebase and +// --abort are allowed as exact arguments only. The bulk of the allowlist +// matches by prefix, which for these two would also admit "--rebase-merges", +// "--rebase=interactive" (spawns an editor), and "--abort-on-*", none of which +// this codebase issues. +func TestIsKnownSafeGitFlagRejectsRebaseAndAbortVariants(t *testing.T) { + for _, flag := range []string{ + "--rebase-merges", + "--rebase=interactive", + "--rebase=i", + "--abort-on-error", + "--aborted", + } { + if IsKnownSafeGitFlag(flag) { + t.Fatalf("IsKnownSafeGitFlag(%q) = true, want false — only the exact flags are allowed", flag) + } + } +}