From 0ed99f04789aad76b8a43cc29aff0ac6180f1b8b Mon Sep 17 00:00:00 2001 From: Owen Mehegan Date: Thu, 7 May 2026 16:34:35 +1000 Subject: [PATCH 1/6] Add support for git commit verification --- clicommand/bootstrap.go | 3 + clicommand/global.go | 6 + internal/job/checkout.go | 76 ++++++++++ internal/job/checkout_test.go | 278 ++++++++++++++++++++++++++++++++++ internal/job/config.go | 3 + 5 files changed, 366 insertions(+) diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index 0ca91f6a55..15e200d3ac 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -76,6 +76,7 @@ type BootstrapConfig struct { GitFetchFlags string `cli:"git-fetch-flags"` GitCloneMirrorFlags string `cli:"git-clone-mirror-flags"` GitCleanFlags string `cli:"git-clean-flags"` + GitCommitVerification bool `cli:"git-commit-verification"` GitMirrorsPath string `cli:"git-mirrors-path" normalize:"filepath"` GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` @@ -248,6 +249,7 @@ var BootstrapCommand = cli.Command{ GitCloneFlagsFlag, GitCloneMirrorFlagsFlag, GitCleanFlagsFlag, + GitCommitVerificationFlag, GitFetchFlagsFlag, GitMirrorsPathFlag, GitMirrorCheckoutModeFlag, @@ -448,6 +450,7 @@ var BootstrapCommand = cli.Command{ Debug: cfg.Debug, GitCheckoutFlags: cfg.GitCheckoutFlags, GitCleanFlags: cfg.GitCleanFlags, + GitCommitVerification: cfg.GitCommitVerification, GitCloneFlags: cfg.GitCloneFlags, GitCloneMirrorFlags: cfg.GitCloneMirrorFlags, GitFetchFlags: cfg.GitFetchFlags, diff --git a/clicommand/global.go b/clicommand/global.go index 99f6d1b556..fa9b79b9bd 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -235,6 +235,12 @@ var ( // -q: quiet, only report errors } + GitCommitVerificationFlag = cli.BoolFlag{ + Name: "git-commit-verification", + Usage: "Enable git commit verification", + EnvVar: "BUILDKITE_GIT_COMMIT_VERIFICATION", + } + GitFetchFlagsFlag = cli.StringFlag{ Name: "git-fetch-flags", Value: "-v --prune", diff --git a/internal/job/checkout.go b/internal/job/checkout.go index af93dbf8ce..2080f83782 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -806,6 +806,78 @@ func (e *Executor) fetchSource(ctx context.Context) error { return nil } +// verifyCommit is called if the user has commit verification enabled. It ensures that the commit we are +// asked to build exists and is reachable on the branch we are given. +func (e *Executor) verifyCommit(ctx context.Context) error { + // Skip if not enabled + if !e.GitCommitVerification { + return nil + } + + // Skip if commit is HEAD (nothing to verify) + if e.Commit == "HEAD" { + return nil + } + + // Skip if we haven't been given a branch - e.g. it's a tag push event + if e.Branch == "" { + return nil + } + + e.shell.Commentf("Verifying commit %q is on branch %q", e.Commit, e.Branch) + + // Try the ancestry check + err := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) + exitCode := shell.ExitCode(err) + + switch exitCode { + case 0: + return nil // verified! + case 1: + return fmt.Errorf("commit %q is not on branch %q", e.Commit, e.Branch) + case 128: + // We might have a shallow clone, try to deepen or unshallow to find the commit + output, _ := e.shell.Command("git", "rev-parse", "--is-shallow-repository").RunAndCaptureStdout(ctx) + + if strings.TrimSpace(output) != "true" { + // Not shallow — this is a genuine error + return fmt.Errorf("unable to verify commit %q on branch %q: %w", e.Commit, e.Branch, err) + } + + // Try deepening by 50 commits first + e.shell.Commentf("Shallow clone detected, deepening by 50 commits...") + _ = e.shell.Command("git", "fetch", "--deepen=50").Run(ctx) + + retryErr := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) + retryCode := shell.ExitCode(retryErr) + + if retryCode == 0 { + return nil // Found a valid commit after deepening + } + if retryCode == 1 { + return fmt.Errorf("commit %q is not on branch %q", e.Commit, e.Branch) + } + + // Still 128 - full unshallow as last resort + e.shell.Commentf("Deepening insufficient, performing a full unshallow...") + _ = e.shell.Command("git", "fetch", "--unshallow").Run(ctx) + + retryErr = e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) + retryCode = shell.ExitCode(retryErr) + + if retryCode == 0 { + return nil // Found a valid commit after unshallowing + } + if retryCode == 1 { + return fmt.Errorf("commit %q is not on branch %q", e.Commit, e.Branch) + } + + return fmt.Errorf("unable to verify commit %q on branch %q after unshallowing: %w", e.Commit, e.Branch, retryErr) + default: + return fmt.Errorf("unable to verify commit %q on branch %q: %w", e.Commit, e.Branch, err) + } +} + // defaultCheckoutPhase is called by the CheckoutPhase if no global or plugin checkout // hook exists. It performs the default checkout on the Repository provided in the config func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { @@ -918,6 +990,10 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { return err } + if err := e.verifyCommit(ctx); err != nil { + return err + } + gitCheckoutFlags := e.GitCheckoutFlags if e.Commit == "HEAD" { diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index a23167d737..7449cb4cdb 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -3,6 +3,7 @@ package job import ( "context" "os" + "path/filepath" "testing" "time" @@ -276,3 +277,280 @@ func TestDefaultCheckoutPhase_DelayedRefCreation(t *testing.T) { t.Fatalf("tt.executor.defaultCheckoutPhase(ctx) error = %v, want nil", err) } } + +func TestVerifyCommit(t *testing.T) { + // Table-driven tests for the skip conditions — these don't need a real git repo + skipTests := []struct { + name string + config ExecutorConfig + }{ + { + name: "skips when verification is disabled", + config: ExecutorConfig{ + GitCommitVerification: false, + Commit: "abc123", + Branch: "main", + }, + }, + { + name: "skips when commit is HEAD", + config: ExecutorConfig{ + GitCommitVerification: true, + Commit: "HEAD", + Branch: "main", + }, + }, + { + name: "skips when branch is empty", + config: ExecutorConfig{ + GitCommitVerification: true, + Commit: "abc123", + Branch: "", + }, + }, + } + + for _, tt := range skipTests { + t.Run(tt.name, func(t *testing.T) { + e := &Executor{ + ExecutorConfig: tt.config, + } + err := e.verifyCommit(context.Background()) + if err != nil { + t.Errorf("verifyCommit() error = %v, want nil", err) + } + }) + } + + // Git-dependent tests — these need a real repo to verify against + t.Run("passes when commit is on branch", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-on-branch") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-on-branch") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + // PushBranch creates a new branch with a commit and returns the commit SHA + commit, _, err := s.PushBranch("verify-on-branch", "feature") + if err != nil { + t.Fatalf("PushBranch error = %v", err) + } + + // Clone the repo into a temp dir + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + // Clone and fetch all branches + if err := sh.Command("git", "clone", s.RepoURL("verify-on-branch"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := sh.Command("git", "fetch", "origin", "feature").Run(ctx); err != nil { + t.Fatalf("git fetch error = %v", err) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: true, + Commit: commit, + Branch: "origin/feature", + }, + } + + err = e.verifyCommit(ctx) + if err != nil { + t.Errorf("verifyCommit() error = %v, want nil", err) + } + }) + + t.Run("fails when commit is not on branch", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-not-on-branch") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-not-on-branch") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + // PushBranch creates both branches from main with the same file content, + // which produces identical commit SHAs. We need to make a unique commit + // on feature-b so the branches genuinely diverge. + commit, _, err := s.PushBranch("verify-not-on-branch", "feature-a") + if err != nil { + t.Fatalf("PushBranch(feature-a) error = %v", err) + } + + // Clone, create feature-b manually with different content + workDir, err := os.MkdirTemp("", "verify-commit-work-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. + + setupSh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := setupSh.Command("git", "clone", s.RepoURL("verify-not-on-branch"), workDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := setupSh.Chdir(workDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := setupSh.Command("git", "checkout", "-b", "feature-b").Run(ctx); err != nil { + t.Fatalf("git checkout error = %v", err) + } + if err := os.WriteFile(filepath.Join(workDir, "unique-b.txt"), []byte("unique content for branch b"), 0o644); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := setupSh.Command("git", "add", "unique-b.txt").Run(ctx); err != nil { + t.Fatalf("git add error = %v", err) + } + if err := setupSh.Command("git", "commit", "-m", "unique commit on feature-b").Run(ctx); err != nil { + t.Fatalf("git commit error = %v", err) + } + if err := setupSh.Command("git", "push", "origin", "feature-b").Run(ctx); err != nil { + t.Fatalf("git push error = %v", err) + } + + // Now clone fresh and verify + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + if err := sh.Command("git", "clone", s.RepoURL("verify-not-on-branch"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := sh.Command("git", "fetch", "origin", "feature-a:refs/remotes/origin/feature-a", "feature-b:refs/remotes/origin/feature-b").Run(ctx); err != nil { + t.Fatalf("git fetch error = %v", err) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: true, + Commit: commit, // commit from feature-a + Branch: "origin/feature-b", // but checking against feature-b + }, + } + + err = e.verifyCommit(ctx) + if err == nil { + t.Errorf("verifyCommit() error = nil, want error about commit not on branch") + } + }) + + t.Run("passes after deepening a shallow clone", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-shallow") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-shallow") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + // The initial commit from InitRepository is on main. + // PushBranch adds one more commit on a feature branch. + // We need the initial commit SHA to test — it will be beyond depth=1. + commit, _, err := s.PushBranch("verify-shallow", "feature") + if err != nil { + t.Fatalf("PushBranch error = %v", err) + } + + // Get the initial commit (parent of the feature commit) by reading it from the server repo + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + // Shallow clone with depth=1 — only gets the tip commit + if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", s.RepoURL("verify-shallow"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + + // The tip commit is the one from PushBranch — it IS on the branch, + // but let's verify the shallow clone scenario works at all. + // With depth=1, the commit should be present and verifiable. + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: true, + Commit: commit, + Branch: "origin/feature", + }, + } + + err = e.verifyCommit(ctx) + if err != nil { + t.Errorf("verifyCommit() error = %v, want nil", err) + } + }) +} diff --git a/internal/job/config.go b/internal/job/config.go index f1073fd91f..8b434d7f3b 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -104,6 +104,9 @@ type ExecutorConfig struct { // Flags to pass to "git clean" command GitCleanFlags string `env:"BUILDKITE_GIT_CLEAN_FLAGS"` + // Enable git commit verification + GitCommitVerification bool `env:"BUILDKITE_GIT_COMMIT_VERIFICATION"` + // Config key=value pairs to pass to "git" when submodule init commands are invoked GitSubmoduleCloneConfig []string `env:"BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG"` From 49736dcda38fd359bfa57e4c785f50d163cb1730 Mon Sep 17 00:00:00 2001 From: Owen Mehegan Date: Tue, 12 May 2026 17:18:46 +1000 Subject: [PATCH 2/6] Add a flag to agent start --- agent/agent_configuration.go | 1 + agent/job_runner.go | 1 + clicommand/agent_start.go | 3 +++ 3 files changed, 5 insertions(+) diff --git a/agent/agent_configuration.go b/agent/agent_configuration.go index 143e139676..c4ed0db606 100644 --- a/agent/agent_configuration.go +++ b/agent/agent_configuration.go @@ -23,6 +23,7 @@ type AgentConfiguration struct { GitCloneFlags string GitCloneMirrorFlags string GitCleanFlags string + GitCommitVerification bool GitFetchFlags string GitSubmodules bool GitSubmoduleCloneConfig []string diff --git a/agent/job_runner.go b/agent/job_runner.go index 9f717d11ac..45215970d1 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -605,6 +605,7 @@ BUILDKITE_AGENT_JWKS_KEY_ID` setCheckoutEnv("BUILDKITE_GIT_CLEAN_FLAGS", r.conf.AgentConfiguration.GitCleanFlags) setCheckoutEnv("BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT", strconv.Itoa(r.conf.AgentConfiguration.GitMirrorsLockTimeout)) setCheckoutEnv("BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG", strings.Join(r.conf.AgentConfiguration.GitSubmoduleCloneConfig, ",")) + setCheckoutEnv("BUILDKITE_GIT_COMMIT_VERIFICATION", fmt.Sprint(r.conf.AgentConfiguration.GitCommitVerification)) setEnv("BUILDKITE_SHELL", r.conf.AgentConfiguration.Shell) setEnv("BUILDKITE_HOOKS_SHELL", r.conf.AgentConfiguration.HooksShell) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 515c357edb..4bc1f81d66 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -166,6 +166,7 @@ type AgentStartConfig struct { GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` GitMirrorsSkipUpdate bool `cli:"git-mirrors-skip-update"` + GitCommitVerification bool `cli:"git-commit-verification"` NoGitSubmodules bool `cli:"no-git-submodules"` GitSubmoduleCloneConfig []string `cli:"git-submodule-clone-config"` SkipCheckout bool `cli:"skip-checkout"` @@ -540,6 +541,7 @@ var AgentStartCommand = cli.Command{ GitCheckoutFlagsFlag, GitCloneFlagsFlag, GitCleanFlagsFlag, + GitCommitVerificationFlag, GitFetchFlagsFlag, GitCloneMirrorFlagsFlag, GitMirrorsPathFlag, @@ -1075,6 +1077,7 @@ var AgentStartCommand = cli.Command{ GitCloneFlags: cfg.GitCloneFlags, GitCloneMirrorFlags: cfg.GitCloneMirrorFlags, GitCleanFlags: cfg.GitCleanFlags, + GitCommitVerification: cfg.GitCommitVerification, GitFetchFlags: cfg.GitFetchFlags, GitSubmodules: !cfg.NoGitSubmodules, GitSubmoduleCloneConfig: cfg.GitSubmoduleCloneConfig, From 179d0fcc8d20e5592ae73958a9cc9c7e524b4dff Mon Sep 17 00:00:00 2001 From: Owen Mehegan Date: Wed, 13 May 2026 17:37:22 +1000 Subject: [PATCH 3/6] Refactor to support the strict vs warn config, and cover a couple more skip cases --- agent/agent_configuration.go | 2 +- agent/job_runner.go | 2 +- clicommand/agent_start.go | 2 +- clicommand/bootstrap.go | 2 +- clicommand/global.go | 2 +- internal/job/checkout.go | 68 +++++++--- internal/job/checkout_test.go | 126 +++++++++++++++++- internal/job/config.go | 2 +- .../integration/checkout_integration_test.go | 95 +++++++++++++ 9 files changed, 271 insertions(+), 30 deletions(-) diff --git a/agent/agent_configuration.go b/agent/agent_configuration.go index c4ed0db606..28e2e435f3 100644 --- a/agent/agent_configuration.go +++ b/agent/agent_configuration.go @@ -23,7 +23,7 @@ type AgentConfiguration struct { GitCloneFlags string GitCloneMirrorFlags string GitCleanFlags string - GitCommitVerification bool + GitCommitVerification string GitFetchFlags string GitSubmodules bool GitSubmoduleCloneConfig []string diff --git a/agent/job_runner.go b/agent/job_runner.go index 45215970d1..e9d7e0593e 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -605,7 +605,7 @@ BUILDKITE_AGENT_JWKS_KEY_ID` setCheckoutEnv("BUILDKITE_GIT_CLEAN_FLAGS", r.conf.AgentConfiguration.GitCleanFlags) setCheckoutEnv("BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT", strconv.Itoa(r.conf.AgentConfiguration.GitMirrorsLockTimeout)) setCheckoutEnv("BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG", strings.Join(r.conf.AgentConfiguration.GitSubmoduleCloneConfig, ",")) - setCheckoutEnv("BUILDKITE_GIT_COMMIT_VERIFICATION", fmt.Sprint(r.conf.AgentConfiguration.GitCommitVerification)) + setCheckoutEnv("BUILDKITE_GIT_COMMIT_VERIFICATION", r.conf.AgentConfiguration.GitCommitVerification) setEnv("BUILDKITE_SHELL", r.conf.AgentConfiguration.Shell) setEnv("BUILDKITE_HOOKS_SHELL", r.conf.AgentConfiguration.HooksShell) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 4bc1f81d66..f764e32d3f 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -166,7 +166,7 @@ type AgentStartConfig struct { GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` GitMirrorsSkipUpdate bool `cli:"git-mirrors-skip-update"` - GitCommitVerification bool `cli:"git-commit-verification"` + GitCommitVerification string `cli:"git-commit-verification"` NoGitSubmodules bool `cli:"no-git-submodules"` GitSubmoduleCloneConfig []string `cli:"git-submodule-clone-config"` SkipCheckout bool `cli:"skip-checkout"` diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index 15e200d3ac..c4ca940989 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -76,7 +76,7 @@ type BootstrapConfig struct { GitFetchFlags string `cli:"git-fetch-flags"` GitCloneMirrorFlags string `cli:"git-clone-mirror-flags"` GitCleanFlags string `cli:"git-clean-flags"` - GitCommitVerification bool `cli:"git-commit-verification"` + GitCommitVerification string `cli:"git-commit-verification"` GitMirrorsPath string `cli:"git-mirrors-path" normalize:"filepath"` GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` diff --git a/clicommand/global.go b/clicommand/global.go index fa9b79b9bd..a876b45fa0 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -235,7 +235,7 @@ var ( // -q: quiet, only report errors } - GitCommitVerificationFlag = cli.BoolFlag{ + GitCommitVerificationFlag = cli.StringFlag{ Name: "git-commit-verification", Usage: "Enable git commit verification", EnvVar: "BUILDKITE_GIT_COMMIT_VERIFICATION", diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 2080f83782..cb7faba288 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -806,24 +806,7 @@ func (e *Executor) fetchSource(ctx context.Context) error { return nil } -// verifyCommit is called if the user has commit verification enabled. It ensures that the commit we are -// asked to build exists and is reachable on the branch we are given. -func (e *Executor) verifyCommit(ctx context.Context) error { - // Skip if not enabled - if !e.GitCommitVerification { - return nil - } - - // Skip if commit is HEAD (nothing to verify) - if e.Commit == "HEAD" { - return nil - } - - // Skip if we haven't been given a branch - e.g. it's a tag push event - if e.Branch == "" { - return nil - } - +func (e *Executor) checkCommitOnBranch(ctx context.Context) error { e.shell.Commentf("Verifying commit %q is on branch %q", e.Commit, e.Branch) // Try the ancestry check @@ -878,6 +861,55 @@ func (e *Executor) verifyCommit(ctx context.Context) error { } } +// verifyCommit is called if the user has commit verification enabled. It ensures that the commit we are +// asked to build exists and is reachable on the branch we are given. +func (e *Executor) verifyCommit(ctx context.Context) error { + // Skip if not enabled + if e.GitCommitVerification == "" { + return nil + } + + // Skip if commit is HEAD (nothing to verify) + if e.Commit == "HEAD" { + return nil + } + + // Skip if we haven't been given a branch - e.g. it's a tag push event + if e.Branch == "" { + return nil + } + + // Skip if this is a tag build — tags are not branch-specific + if e.Tag != "" { + return nil + } + + // Skip if this is a PR build — the commit may be on a merge ref, not the target branch + if e.PullRequest != "" { + return nil + } + + // Perform the verification + err := e.checkCommitOnBranch(ctx) + + // Verification passed + if err == nil { + return nil + } + + // Handle verification failure depending on setting + switch e.GitCommitVerification { + case "strict": + return err + case "warn": + e.shell.Warningf("Commit verification failed: %v", err) + return nil + default: + e.shell.Warningf("Unknown git-commit-verification value %q, skipping verification", e.GitCommitVerification) + return nil + } +} + // defaultCheckoutPhase is called by the CheckoutPhase if no global or plugin checkout // hook exists. It performs the default checkout on the Repository provided in the config func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 7449cb4cdb..458e6703fe 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -287,7 +287,7 @@ func TestVerifyCommit(t *testing.T) { { name: "skips when verification is disabled", config: ExecutorConfig{ - GitCommitVerification: false, + GitCommitVerification: "", Commit: "abc123", Branch: "main", }, @@ -295,7 +295,7 @@ func TestVerifyCommit(t *testing.T) { { name: "skips when commit is HEAD", config: ExecutorConfig{ - GitCommitVerification: true, + GitCommitVerification: "strict", Commit: "HEAD", Branch: "main", }, @@ -303,11 +303,29 @@ func TestVerifyCommit(t *testing.T) { { name: "skips when branch is empty", config: ExecutorConfig{ - GitCommitVerification: true, + GitCommitVerification: "strict", Commit: "abc123", Branch: "", }, }, + { + name: "skips for PR builds", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "abc123", + Branch: "main", + PullRequest: "123", + }, + }, + { + name: "skips for tag builds", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "abc123", + Branch: "main", + Tag: "v1.0.0", + }, + }, } for _, tt := range skipTests { @@ -376,7 +394,7 @@ func TestVerifyCommit(t *testing.T) { e := &Executor{ shell: sh, ExecutorConfig: ExecutorConfig{ - GitCommitVerification: true, + GitCommitVerification: "strict", Commit: commit, Branch: "origin/feature", }, @@ -475,7 +493,7 @@ func TestVerifyCommit(t *testing.T) { e := &Executor{ shell: sh, ExecutorConfig: ExecutorConfig{ - GitCommitVerification: true, + GitCommitVerification: "strict", Commit: commit, // commit from feature-a Branch: "origin/feature-b", // but checking against feature-b }, @@ -487,6 +505,102 @@ func TestVerifyCommit(t *testing.T) { } }) + t.Run("warn mode logs warning but does not fail", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-warn-mode") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-warn-mode") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + commit, _, err := s.PushBranch("verify-warn-mode", "feature-a") + if err != nil { + t.Fatalf("PushBranch(feature-a) error = %v", err) + } + + // Create feature-b with unique content so the branches genuinely diverge + workDir, err := os.MkdirTemp("", "verify-commit-work-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. + + setupSh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := setupSh.Command("git", "clone", s.RepoURL("verify-warn-mode"), workDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := setupSh.Chdir(workDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := setupSh.Command("git", "checkout", "-b", "feature-b").Run(ctx); err != nil { + t.Fatalf("git checkout error = %v", err) + } + if err := os.WriteFile(filepath.Join(workDir, "unique-b.txt"), []byte("unique content for branch b"), 0o644); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := setupSh.Command("git", "add", "unique-b.txt").Run(ctx); err != nil { + t.Fatalf("git add error = %v", err) + } + if err := setupSh.Command("git", "commit", "-m", "unique commit on feature-b").Run(ctx); err != nil { + t.Fatalf("git commit error = %v", err) + } + if err := setupSh.Command("git", "push", "origin", "feature-b").Run(ctx); err != nil { + t.Fatalf("git push error = %v", err) + } + + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + if err := sh.Command("git", "clone", s.RepoURL("verify-warn-mode"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := sh.Command("git", "fetch", "origin", "feature-a:refs/remotes/origin/feature-a", "feature-b:refs/remotes/origin/feature-b").Run(ctx); err != nil { + t.Fatalf("git fetch error = %v", err) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "warn", + Commit: commit, // commit from feature-a + Branch: "origin/feature-b", // but checking against feature-b + }, + } + + // In warn mode, verification failure should NOT return an error + err = e.verifyCommit(ctx) + if err != nil { + t.Errorf("verifyCommit() in warn mode error = %v, want nil", err) + } + }) + t.Run("passes after deepening a shallow clone", func(t *testing.T) { t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") @@ -542,7 +656,7 @@ func TestVerifyCommit(t *testing.T) { e := &Executor{ shell: sh, ExecutorConfig: ExecutorConfig{ - GitCommitVerification: true, + GitCommitVerification: "strict", Commit: commit, Branch: "origin/feature", }, diff --git a/internal/job/config.go b/internal/job/config.go index 8b434d7f3b..bd7fe9e5c6 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -105,7 +105,7 @@ type ExecutorConfig struct { GitCleanFlags string `env:"BUILDKITE_GIT_CLEAN_FLAGS"` // Enable git commit verification - GitCommitVerification bool `env:"BUILDKITE_GIT_COMMIT_VERIFICATION"` + GitCommitVerification string `env:"BUILDKITE_GIT_COMMIT_VERIFICATION"` // Config key=value pairs to pass to "git" when submodule init commands are invoked GitSubmoduleCloneConfig []string `env:"BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG"` diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index d12cd844ab..3a2fb126ab 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -1229,6 +1229,101 @@ func TestMultipleRemoteURLsFallsBackToGetURL(t *testing.T) { tester.RunAndCheck(t, env...) } +func TestCommitVerificationWithValidCommit(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + // Get the real commit SHA from the test repo + commitHash, err := tester.Repo.RevParse("main") + if err != nil { + t.Fatalf("RevParse(main) error = %v", err) + } + commitHash = strings.TrimSpace(commitHash) + + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=-v", + "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", + "BUILDKITE_GIT_FETCH_FLAGS=-v", + "BUILDKITE_GIT_COMMIT_VERIFICATION=strict", + fmt.Sprintf("BUILDKITE_COMMIT=%s", commitHash), + } + + git := tester. + MustMock(t, "git"). + PassthroughToLocalCommand() + + // Expect the normal checkout flow with merge-base --is-ancestor inserted + // between fetch and checkout. Since this is a full clone (not shallow), + // merge-base succeeds immediately — no rev-parse --is-shallow-repository needed. + git.ExpectAll([][]any{ + {"clone", "-v", "--", tester.Repo.Path, "."}, + {"clean", "-fdq"}, + {"fetch", "-v", "--", "origin", commitHash}, + {"merge-base", "--is-ancestor", commitHash, "main"}, + {"-c", "advice.detachedHead=false", "checkout", "-f", commitHash}, + {"clean", "-fdq"}, + {"--no-pager", "log", "-1", commitHash, "-s", "--no-color", gitShowFormatArg}, + }) + + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(1) + agent.Expect("meta-data", "set", job.CommitMetadataKey).WithStdin(commitPattern) + + tester.RunAndCheck(t, env...) +} + +func TestCommitVerificationFailsWithInvalidCommit(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + // Get the commit from the update-test-txt branch (not on main) + commitHash, err := tester.Repo.RevParse("update-test-txt") + if err != nil { + t.Fatalf("RevParse(update-test-txt) error = %v", err) + } + commitHash = strings.TrimSpace(commitHash) + + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=-v", + "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", + "BUILDKITE_GIT_FETCH_FLAGS=-v", + "BUILDKITE_GIT_COMMIT_VERIFICATION=strict", + fmt.Sprintf("BUILDKITE_COMMIT=%s", commitHash), + } + + git := tester. + MustMock(t, "git"). + PassthroughToLocalCommand() + + // Expect fetch, then merge-base which should return exit 1 (not ancestor). + // No checkout should happen after verification fails. + git.ExpectAll([][]any{ + {"clone", "-v", "--", tester.Repo.Path, "."}, + {"clean", "-fdq"}, + {"fetch", "-v", "--", "origin", commitHash}, + {"merge-base", "--is-ancestor", commitHash, "main"}, + }) + + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(1) + + // This should fail — the commit is not on main + err = tester.Run(t, env...) + if err == nil { + t.Fatalf("expected build to fail with commit verification error, but it passed") + } +} + type subDirMatcher struct { dir string } From 3931a2c1d22c25ede225cd5bcfbab7202b4bcafa Mon Sep 17 00:00:00 2001 From: Owen Mehegan Date: Thu, 14 May 2026 14:31:42 +1000 Subject: [PATCH 4/6] Linting --- clicommand/agent_start.go | 5 +++-- clicommand/bootstrap.go | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index f764e32d3f..721e337fbb 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -166,7 +166,7 @@ type AgentStartConfig struct { GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` GitMirrorsSkipUpdate bool `cli:"git-mirrors-skip-update"` - GitCommitVerification string `cli:"git-commit-verification"` + GitCommitVerification string `cli:"git-commit-verification"` NoGitSubmodules bool `cli:"no-git-submodules"` GitSubmoduleCloneConfig []string `cli:"git-submodule-clone-config"` SkipCheckout bool `cli:"skip-checkout"` @@ -377,7 +377,8 @@ var AgentStartCommand = cli.Command{ Name: "start", Usage: "Starts a Buildkite agent", Description: startDescription, - Flags: append(globalFlags(), + Flags: append( + globalFlags(), cli.StringFlag{ Name: "config", Value: "", diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index c4ca940989..d71d2989d7 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -76,7 +76,7 @@ type BootstrapConfig struct { GitFetchFlags string `cli:"git-fetch-flags"` GitCloneMirrorFlags string `cli:"git-clone-mirror-flags"` GitCleanFlags string `cli:"git-clean-flags"` - GitCommitVerification string `cli:"git-commit-verification"` + GitCommitVerification string `cli:"git-commit-verification"` GitMirrorsPath string `cli:"git-mirrors-path" normalize:"filepath"` GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"` GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"` @@ -500,7 +500,8 @@ var BootstrapCommand = cli.Command{ defer cancel() signals := make(chan os.Signal, 1) - signal.Notify(signals, os.Interrupt, + signal.Notify( + signals, os.Interrupt, syscall.SIGHUP, syscall.SIGTERM, syscall.SIGINT, From 4ccd27437184c633c065d4fd286e403cc5839ba5 Mon Sep 17 00:00:00 2001 From: Owen Mehegan Date: Thu, 14 May 2026 15:15:39 +1000 Subject: [PATCH 5/6] Skip if a custom refspec is used --- internal/job/checkout.go | 6 ++++++ internal/job/checkout_test.go | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index cb7faba288..2d89833286 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -889,6 +889,12 @@ func (e *Executor) verifyCommit(ctx context.Context) error { return nil } + // Skip if a custom refspec is set — the fetch may not populate standard branch refs, + // making ancestry verification unreliable + if e.RefSpec != "" { + return nil + } + // Perform the verification err := e.checkCommitOnBranch(ctx) diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 458e6703fe..39c4dfba89 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -326,6 +326,15 @@ func TestVerifyCommit(t *testing.T) { Tag: "v1.0.0", }, }, + { + name: "skips for custom refspec", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "abc123", + Branch: "main", + RefSpec: "refs/custom/spec", + }, + }, } for _, tt := range skipTests { From 7904808ab697a3a7348a314540c3e2c809dc683e Mon Sep 17 00:00:00 2001 From: Owen Mehegan Date: Thu, 21 May 2026 14:22:37 +1000 Subject: [PATCH 6/6] Move commit verification into its own files for cleanliness --- env/protected.go | 1 + internal/job/checkout.go | 110 ------ internal/job/checkout_test.go | 401 ---------------------- internal/job/commit_verification.go | 150 +++++++++ internal/job/commit_verification_test.go | 411 +++++++++++++++++++++++ 5 files changed, 562 insertions(+), 511 deletions(-) create mode 100644 internal/job/commit_verification.go create mode 100644 internal/job/commit_verification_test.go diff --git a/env/protected.go b/env/protected.go index dc24ab17e9..3bbf405d32 100644 --- a/env/protected.go +++ b/env/protected.go @@ -48,6 +48,7 @@ var protectedEnv = map[string]protection{ "BUILDKITE_BIN_PATH": {}, "BUILDKITE_BUILD_PATH": {}, "BUILDKITE_COMMAND_EVAL": {}, + "BUILDKITE_GIT_COMMIT_VERIFICATION": {}, "BUILDKITE_CONFIG_PATH": {}, "BUILDKITE_CONTAINER_COUNT": {}, "BUILDKITE_HOOKS_PATH": {}, diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 2d89833286..b3ebbd056a 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -806,116 +806,6 @@ func (e *Executor) fetchSource(ctx context.Context) error { return nil } -func (e *Executor) checkCommitOnBranch(ctx context.Context) error { - e.shell.Commentf("Verifying commit %q is on branch %q", e.Commit, e.Branch) - - // Try the ancestry check - err := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) - exitCode := shell.ExitCode(err) - - switch exitCode { - case 0: - return nil // verified! - case 1: - return fmt.Errorf("commit %q is not on branch %q", e.Commit, e.Branch) - case 128: - // We might have a shallow clone, try to deepen or unshallow to find the commit - output, _ := e.shell.Command("git", "rev-parse", "--is-shallow-repository").RunAndCaptureStdout(ctx) - - if strings.TrimSpace(output) != "true" { - // Not shallow — this is a genuine error - return fmt.Errorf("unable to verify commit %q on branch %q: %w", e.Commit, e.Branch, err) - } - - // Try deepening by 50 commits first - e.shell.Commentf("Shallow clone detected, deepening by 50 commits...") - _ = e.shell.Command("git", "fetch", "--deepen=50").Run(ctx) - - retryErr := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) - retryCode := shell.ExitCode(retryErr) - - if retryCode == 0 { - return nil // Found a valid commit after deepening - } - if retryCode == 1 { - return fmt.Errorf("commit %q is not on branch %q", e.Commit, e.Branch) - } - - // Still 128 - full unshallow as last resort - e.shell.Commentf("Deepening insufficient, performing a full unshallow...") - _ = e.shell.Command("git", "fetch", "--unshallow").Run(ctx) - - retryErr = e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) - retryCode = shell.ExitCode(retryErr) - - if retryCode == 0 { - return nil // Found a valid commit after unshallowing - } - if retryCode == 1 { - return fmt.Errorf("commit %q is not on branch %q", e.Commit, e.Branch) - } - - return fmt.Errorf("unable to verify commit %q on branch %q after unshallowing: %w", e.Commit, e.Branch, retryErr) - default: - return fmt.Errorf("unable to verify commit %q on branch %q: %w", e.Commit, e.Branch, err) - } -} - -// verifyCommit is called if the user has commit verification enabled. It ensures that the commit we are -// asked to build exists and is reachable on the branch we are given. -func (e *Executor) verifyCommit(ctx context.Context) error { - // Skip if not enabled - if e.GitCommitVerification == "" { - return nil - } - - // Skip if commit is HEAD (nothing to verify) - if e.Commit == "HEAD" { - return nil - } - - // Skip if we haven't been given a branch - e.g. it's a tag push event - if e.Branch == "" { - return nil - } - - // Skip if this is a tag build — tags are not branch-specific - if e.Tag != "" { - return nil - } - - // Skip if this is a PR build — the commit may be on a merge ref, not the target branch - if e.PullRequest != "" { - return nil - } - - // Skip if a custom refspec is set — the fetch may not populate standard branch refs, - // making ancestry verification unreliable - if e.RefSpec != "" { - return nil - } - - // Perform the verification - err := e.checkCommitOnBranch(ctx) - - // Verification passed - if err == nil { - return nil - } - - // Handle verification failure depending on setting - switch e.GitCommitVerification { - case "strict": - return err - case "warn": - e.shell.Warningf("Commit verification failed: %v", err) - return nil - default: - e.shell.Warningf("Unknown git-commit-verification value %q, skipping verification", e.GitCommitVerification) - return nil - } -} - // defaultCheckoutPhase is called by the CheckoutPhase if no global or plugin checkout // hook exists. It performs the default checkout on the Repository provided in the config func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 39c4dfba89..a23167d737 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -3,7 +3,6 @@ package job import ( "context" "os" - "path/filepath" "testing" "time" @@ -277,403 +276,3 @@ func TestDefaultCheckoutPhase_DelayedRefCreation(t *testing.T) { t.Fatalf("tt.executor.defaultCheckoutPhase(ctx) error = %v, want nil", err) } } - -func TestVerifyCommit(t *testing.T) { - // Table-driven tests for the skip conditions — these don't need a real git repo - skipTests := []struct { - name string - config ExecutorConfig - }{ - { - name: "skips when verification is disabled", - config: ExecutorConfig{ - GitCommitVerification: "", - Commit: "abc123", - Branch: "main", - }, - }, - { - name: "skips when commit is HEAD", - config: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: "HEAD", - Branch: "main", - }, - }, - { - name: "skips when branch is empty", - config: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: "abc123", - Branch: "", - }, - }, - { - name: "skips for PR builds", - config: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: "abc123", - Branch: "main", - PullRequest: "123", - }, - }, - { - name: "skips for tag builds", - config: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: "abc123", - Branch: "main", - Tag: "v1.0.0", - }, - }, - { - name: "skips for custom refspec", - config: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: "abc123", - Branch: "main", - RefSpec: "refs/custom/spec", - }, - }, - } - - for _, tt := range skipTests { - t.Run(tt.name, func(t *testing.T) { - e := &Executor{ - ExecutorConfig: tt.config, - } - err := e.verifyCommit(context.Background()) - if err != nil { - t.Errorf("verifyCommit() error = %v, want nil", err) - } - }) - } - - // Git-dependent tests — these need a real repo to verify against - t.Run("passes when commit is on branch", func(t *testing.T) { - t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") - t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") - t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") - t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") - - ctx := context.Background() - - s := githttptest.NewServer() - defer s.Close() - - err := s.CreateRepository("verify-on-branch") - if err != nil { - t.Fatalf("CreateRepository error = %v", err) - } - - _, err = s.InitRepository("verify-on-branch") - if err != nil { - t.Fatalf("InitRepository error = %v", err) - } - - // PushBranch creates a new branch with a commit and returns the commit SHA - commit, _, err := s.PushBranch("verify-on-branch", "feature") - if err != nil { - t.Fatalf("PushBranch error = %v", err) - } - - // Clone the repo into a temp dir - cloneDir, err := os.MkdirTemp("", "verify-commit-test-") - if err != nil { - t.Fatalf("MkdirTemp error = %v", err) - } - t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. - - sh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) - } - - // Clone and fetch all branches - if err := sh.Command("git", "clone", s.RepoURL("verify-on-branch"), cloneDir).Run(ctx); err != nil { - t.Fatalf("git clone error = %v", err) - } - if err := sh.Chdir(cloneDir); err != nil { - t.Fatalf("Chdir error = %v", err) - } - if err := sh.Command("git", "fetch", "origin", "feature").Run(ctx); err != nil { - t.Fatalf("git fetch error = %v", err) - } - - e := &Executor{ - shell: sh, - ExecutorConfig: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: commit, - Branch: "origin/feature", - }, - } - - err = e.verifyCommit(ctx) - if err != nil { - t.Errorf("verifyCommit() error = %v, want nil", err) - } - }) - - t.Run("fails when commit is not on branch", func(t *testing.T) { - t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") - t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") - t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") - t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") - - ctx := context.Background() - - s := githttptest.NewServer() - defer s.Close() - - err := s.CreateRepository("verify-not-on-branch") - if err != nil { - t.Fatalf("CreateRepository error = %v", err) - } - - _, err = s.InitRepository("verify-not-on-branch") - if err != nil { - t.Fatalf("InitRepository error = %v", err) - } - - // PushBranch creates both branches from main with the same file content, - // which produces identical commit SHAs. We need to make a unique commit - // on feature-b so the branches genuinely diverge. - commit, _, err := s.PushBranch("verify-not-on-branch", "feature-a") - if err != nil { - t.Fatalf("PushBranch(feature-a) error = %v", err) - } - - // Clone, create feature-b manually with different content - workDir, err := os.MkdirTemp("", "verify-commit-work-") - if err != nil { - t.Fatalf("MkdirTemp error = %v", err) - } - t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. - - setupSh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) - } - if err := setupSh.Command("git", "clone", s.RepoURL("verify-not-on-branch"), workDir).Run(ctx); err != nil { - t.Fatalf("git clone error = %v", err) - } - if err := setupSh.Chdir(workDir); err != nil { - t.Fatalf("Chdir error = %v", err) - } - if err := setupSh.Command("git", "checkout", "-b", "feature-b").Run(ctx); err != nil { - t.Fatalf("git checkout error = %v", err) - } - if err := os.WriteFile(filepath.Join(workDir, "unique-b.txt"), []byte("unique content for branch b"), 0o644); err != nil { - t.Fatalf("WriteFile error = %v", err) - } - if err := setupSh.Command("git", "add", "unique-b.txt").Run(ctx); err != nil { - t.Fatalf("git add error = %v", err) - } - if err := setupSh.Command("git", "commit", "-m", "unique commit on feature-b").Run(ctx); err != nil { - t.Fatalf("git commit error = %v", err) - } - if err := setupSh.Command("git", "push", "origin", "feature-b").Run(ctx); err != nil { - t.Fatalf("git push error = %v", err) - } - - // Now clone fresh and verify - cloneDir, err := os.MkdirTemp("", "verify-commit-test-") - if err != nil { - t.Fatalf("MkdirTemp error = %v", err) - } - t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. - - sh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) - } - - if err := sh.Command("git", "clone", s.RepoURL("verify-not-on-branch"), cloneDir).Run(ctx); err != nil { - t.Fatalf("git clone error = %v", err) - } - if err := sh.Chdir(cloneDir); err != nil { - t.Fatalf("Chdir error = %v", err) - } - if err := sh.Command("git", "fetch", "origin", "feature-a:refs/remotes/origin/feature-a", "feature-b:refs/remotes/origin/feature-b").Run(ctx); err != nil { - t.Fatalf("git fetch error = %v", err) - } - - e := &Executor{ - shell: sh, - ExecutorConfig: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: commit, // commit from feature-a - Branch: "origin/feature-b", // but checking against feature-b - }, - } - - err = e.verifyCommit(ctx) - if err == nil { - t.Errorf("verifyCommit() error = nil, want error about commit not on branch") - } - }) - - t.Run("warn mode logs warning but does not fail", func(t *testing.T) { - t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") - t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") - t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") - t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") - - ctx := context.Background() - - s := githttptest.NewServer() - defer s.Close() - - err := s.CreateRepository("verify-warn-mode") - if err != nil { - t.Fatalf("CreateRepository error = %v", err) - } - - _, err = s.InitRepository("verify-warn-mode") - if err != nil { - t.Fatalf("InitRepository error = %v", err) - } - - commit, _, err := s.PushBranch("verify-warn-mode", "feature-a") - if err != nil { - t.Fatalf("PushBranch(feature-a) error = %v", err) - } - - // Create feature-b with unique content so the branches genuinely diverge - workDir, err := os.MkdirTemp("", "verify-commit-work-") - if err != nil { - t.Fatalf("MkdirTemp error = %v", err) - } - t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. - - setupSh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) - } - if err := setupSh.Command("git", "clone", s.RepoURL("verify-warn-mode"), workDir).Run(ctx); err != nil { - t.Fatalf("git clone error = %v", err) - } - if err := setupSh.Chdir(workDir); err != nil { - t.Fatalf("Chdir error = %v", err) - } - if err := setupSh.Command("git", "checkout", "-b", "feature-b").Run(ctx); err != nil { - t.Fatalf("git checkout error = %v", err) - } - if err := os.WriteFile(filepath.Join(workDir, "unique-b.txt"), []byte("unique content for branch b"), 0o644); err != nil { - t.Fatalf("WriteFile error = %v", err) - } - if err := setupSh.Command("git", "add", "unique-b.txt").Run(ctx); err != nil { - t.Fatalf("git add error = %v", err) - } - if err := setupSh.Command("git", "commit", "-m", "unique commit on feature-b").Run(ctx); err != nil { - t.Fatalf("git commit error = %v", err) - } - if err := setupSh.Command("git", "push", "origin", "feature-b").Run(ctx); err != nil { - t.Fatalf("git push error = %v", err) - } - - cloneDir, err := os.MkdirTemp("", "verify-commit-test-") - if err != nil { - t.Fatalf("MkdirTemp error = %v", err) - } - t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. - - sh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) - } - - if err := sh.Command("git", "clone", s.RepoURL("verify-warn-mode"), cloneDir).Run(ctx); err != nil { - t.Fatalf("git clone error = %v", err) - } - if err := sh.Chdir(cloneDir); err != nil { - t.Fatalf("Chdir error = %v", err) - } - if err := sh.Command("git", "fetch", "origin", "feature-a:refs/remotes/origin/feature-a", "feature-b:refs/remotes/origin/feature-b").Run(ctx); err != nil { - t.Fatalf("git fetch error = %v", err) - } - - e := &Executor{ - shell: sh, - ExecutorConfig: ExecutorConfig{ - GitCommitVerification: "warn", - Commit: commit, // commit from feature-a - Branch: "origin/feature-b", // but checking against feature-b - }, - } - - // In warn mode, verification failure should NOT return an error - err = e.verifyCommit(ctx) - if err != nil { - t.Errorf("verifyCommit() in warn mode error = %v, want nil", err) - } - }) - - t.Run("passes after deepening a shallow clone", func(t *testing.T) { - t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") - t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") - t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") - t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") - - ctx := context.Background() - - s := githttptest.NewServer() - defer s.Close() - - err := s.CreateRepository("verify-shallow") - if err != nil { - t.Fatalf("CreateRepository error = %v", err) - } - - _, err = s.InitRepository("verify-shallow") - if err != nil { - t.Fatalf("InitRepository error = %v", err) - } - - // The initial commit from InitRepository is on main. - // PushBranch adds one more commit on a feature branch. - // We need the initial commit SHA to test — it will be beyond depth=1. - commit, _, err := s.PushBranch("verify-shallow", "feature") - if err != nil { - t.Fatalf("PushBranch error = %v", err) - } - - // Get the initial commit (parent of the feature commit) by reading it from the server repo - cloneDir, err := os.MkdirTemp("", "verify-commit-test-") - if err != nil { - t.Fatalf("MkdirTemp error = %v", err) - } - t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. - - sh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) - } - - // Shallow clone with depth=1 — only gets the tip commit - if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", s.RepoURL("verify-shallow"), cloneDir).Run(ctx); err != nil { - t.Fatalf("git clone error = %v", err) - } - if err := sh.Chdir(cloneDir); err != nil { - t.Fatalf("Chdir error = %v", err) - } - - // The tip commit is the one from PushBranch — it IS on the branch, - // but let's verify the shallow clone scenario works at all. - // With depth=1, the commit should be present and verifiable. - e := &Executor{ - shell: sh, - ExecutorConfig: ExecutorConfig{ - GitCommitVerification: "strict", - Commit: commit, - Branch: "origin/feature", - }, - } - - err = e.verifyCommit(ctx) - if err != nil { - t.Errorf("verifyCommit() error = %v, want nil", err) - } - }) -} diff --git a/internal/job/commit_verification.go b/internal/job/commit_verification.go new file mode 100644 index 0000000000..d31d9ac973 --- /dev/null +++ b/internal/job/commit_verification.go @@ -0,0 +1,150 @@ +package job + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/buildkite/agent/v3/internal/shell" +) + +// ErrCommitVerificationFailed indicates that git has definitively determined +// the commit is not on the specified branch. This is the security-relevant case. +var ErrCommitVerificationFailed = errors.New("commit verification failed") + +// ErrCommitVerificationUnavailable indicates that git was unable to perform +// the ancestry check (e.g. due to repo corruption, misconfigured remotes, etc). +// This is NOT evidence of an attack — it's an infrastructure problem. +var ErrCommitVerificationUnavailable = errors.New("commit verification unavailable") + +// checkCommitOnBranch performs the actual git ancestry check, handling shallow +// clones by deepening or unshallowing as needed. It returns: +// - nil if the commit is verified on the branch +// - ErrCommitVerificationFailed if the commit is definitively not on the branch +// - ErrCommitVerificationUnavailable if the check cannot be performed +func (e *Executor) checkCommitOnBranch(ctx context.Context) error { + e.shell.Commentf("Verifying commit %q is on branch %q", e.Commit, e.Branch) + + // Try the ancestry check + err := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) + exitCode := shell.ExitCode(err) + + switch exitCode { + case 0: + return nil // verified! + case 1: + return fmt.Errorf("%w: commit %q is not on branch %q", ErrCommitVerificationFailed, e.Commit, e.Branch) + case 128: + // We might have a shallow clone, try to deepen or unshallow to find the commit + output, _ := e.shell.Command("git", "rev-parse", "--is-shallow-repository").RunAndCaptureStdout(ctx) + + if strings.TrimSpace(output) != "true" { + // Not shallow — this is a genuine error + return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, err) + } + + // Try deepening by 50 commits first + e.shell.Commentf("Shallow clone detected, deepening by 50 commits...") + _ = e.shell.Command("git", "fetch", "--deepen=50").Run(ctx) + + retryErr := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) + retryCode := shell.ExitCode(retryErr) + + if retryCode == 0 { + return nil // Found a valid commit after deepening + } + if retryCode == 1 { + return fmt.Errorf("%w: commit %q is not on branch %q", ErrCommitVerificationFailed, e.Commit, e.Branch) + } + + // Still 128 - full unshallow as last resort + e.shell.Commentf("Deepening insufficient, performing a full unshallow...") + _ = e.shell.Command("git", "fetch", "--unshallow").Run(ctx) + + retryErr = e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) + retryCode = shell.ExitCode(retryErr) + + if retryCode == 0 { + return nil // Found a valid commit after unshallowing + } + if retryCode == 1 { + return fmt.Errorf("%w: commit %q is not on branch %q", ErrCommitVerificationFailed, e.Commit, e.Branch) + } + + return fmt.Errorf("%w: unable to verify commit %q on branch %q after unshallowing: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, retryErr) + default: + return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, err) + } +} + +// verifyCommit is called if the user has commit verification enabled. It ensures that the commit we are +// asked to build exists and is reachable on the branch we are given. +func (e *Executor) verifyCommit(ctx context.Context) error { + // Skip if not enabled + if e.GitCommitVerification == "" { + return nil + } + + // Skip if commit is HEAD (nothing to verify) + if e.Commit == "HEAD" { + return nil + } + + // Skip if we haven't been given a branch - e.g. it's a tag push event + if e.Branch == "" { + return nil + } + + // Skip if this is a tag build — tags are not branch-specific + if e.Tag != "" { + return nil + } + + // Skip if this is a PR build — the commit may be on a merge ref, not the target branch + if e.PullRequest != "" { + return nil + } + + // Skip if a custom refspec is set — the fetch may not populate standard branch refs, + // making ancestry verification unreliable + if e.RefSpec != "" { + return nil + } + + // Perform the verification + err := e.checkCommitOnBranch(ctx) + + // Verification passed + if err == nil { + return nil + } + + // Handle verification results based on error type and mode. + // + // We distinguish between two failure types: + // - ErrCommitVerificationFailed: git definitively determined the commit is NOT + // on the branch. This is the security-relevant case (potential branch spoofing). + // - ErrCommitVerificationUnavailable: git was unable to perform the check at all + // (repo issues, corrupted refs, etc). This is NOT evidence of an attack. + // + // In strict mode, only a definitive failure (ErrCommitVerificationFailed) blocks + // the build. If verification is unavailable, we warn but allow the build to proceed. + // This avoids users disabling verification entirely due to infrastructure false + // positives, which would leave them with no protection at all. + switch e.GitCommitVerification { + case "strict": + if errors.Is(err, ErrCommitVerificationFailed) { + return err + } + // Verification was unavailable — warn but don't block + e.shell.Warningf("Commit verification unavailable: %v", err) + return nil + case "warn": + e.shell.Warningf("Commit verification failed: %v", err) + return nil + default: + e.shell.Warningf("Unknown git-commit-verification value %q, skipping verification", e.GitCommitVerification) + return nil + } +} diff --git a/internal/job/commit_verification_test.go b/internal/job/commit_verification_test.go new file mode 100644 index 0000000000..05db853b49 --- /dev/null +++ b/internal/job/commit_verification_test.go @@ -0,0 +1,411 @@ +package job + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/buildkite/agent/v3/internal/job/githttptest" + "github.com/buildkite/agent/v3/internal/shell" +) + +func TestVerifyCommit(t *testing.T) { + // Table-driven tests for the skip conditions — these don't need a real git repo + skipTests := []struct { + name string + config ExecutorConfig + }{ + { + name: "skips when verification is disabled", + config: ExecutorConfig{ + GitCommitVerification: "", + Commit: "abc123", + Branch: "main", + }, + }, + { + name: "skips when commit is HEAD", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "HEAD", + Branch: "main", + }, + }, + { + name: "skips when branch is empty", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "abc123", + Branch: "", + }, + }, + { + name: "skips for PR builds", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "abc123", + Branch: "main", + PullRequest: "123", + }, + }, + { + name: "skips for tag builds", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "abc123", + Branch: "main", + Tag: "v1.0.0", + }, + }, + { + name: "skips for custom refspec", + config: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: "abc123", + Branch: "main", + RefSpec: "refs/custom/spec", + }, + }, + } + + for _, tt := range skipTests { + t.Run(tt.name, func(t *testing.T) { + e := &Executor{ + ExecutorConfig: tt.config, + } + err := e.verifyCommit(context.Background()) + if err != nil { + t.Errorf("verifyCommit() error = %v, want nil", err) + } + }) + } + + // Git-dependent tests — these need a real repo to verify against + t.Run("passes when commit is on branch", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-on-branch") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-on-branch") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + // PushBranch creates a new branch with a commit and returns the commit SHA + commit, _, err := s.PushBranch("verify-on-branch", "feature") + if err != nil { + t.Fatalf("PushBranch error = %v", err) + } + + // Clone the repo into a temp dir + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + // Clone and fetch all branches + if err := sh.Command("git", "clone", s.RepoURL("verify-on-branch"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := sh.Command("git", "fetch", "origin", "feature").Run(ctx); err != nil { + t.Fatalf("git fetch error = %v", err) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: commit, + Branch: "origin/feature", + }, + } + + err = e.verifyCommit(ctx) + if err != nil { + t.Errorf("verifyCommit() error = %v, want nil", err) + } + }) + + t.Run("fails when commit is not on branch", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-not-on-branch") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-not-on-branch") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + // PushBranch creates both branches from main with the same file content, + // which produces identical commit SHAs. We need to make a unique commit + // on feature-b so the branches genuinely diverge. + commit, _, err := s.PushBranch("verify-not-on-branch", "feature-a") + if err != nil { + t.Fatalf("PushBranch(feature-a) error = %v", err) + } + + // Clone, create feature-b manually with different content + workDir, err := os.MkdirTemp("", "verify-commit-work-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. + + setupSh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := setupSh.Command("git", "clone", s.RepoURL("verify-not-on-branch"), workDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := setupSh.Chdir(workDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := setupSh.Command("git", "checkout", "-b", "feature-b").Run(ctx); err != nil { + t.Fatalf("git checkout error = %v", err) + } + if err := os.WriteFile(filepath.Join(workDir, "unique-b.txt"), []byte("unique content for branch b"), 0o644); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := setupSh.Command("git", "add", "unique-b.txt").Run(ctx); err != nil { + t.Fatalf("git add error = %v", err) + } + if err := setupSh.Command("git", "commit", "-m", "unique commit on feature-b").Run(ctx); err != nil { + t.Fatalf("git commit error = %v", err) + } + if err := setupSh.Command("git", "push", "origin", "feature-b").Run(ctx); err != nil { + t.Fatalf("git push error = %v", err) + } + + // Now clone fresh and verify + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + if err := sh.Command("git", "clone", s.RepoURL("verify-not-on-branch"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := sh.Command("git", "fetch", "origin", "feature-a:refs/remotes/origin/feature-a", "feature-b:refs/remotes/origin/feature-b").Run(ctx); err != nil { + t.Fatalf("git fetch error = %v", err) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: commit, // commit from feature-a + Branch: "origin/feature-b", // but checking against feature-b + }, + } + + err = e.verifyCommit(ctx) + if err == nil { + t.Errorf("verifyCommit() error = nil, want error about commit not on branch") + } + }) + + t.Run("warn mode logs warning but does not fail", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-warn-mode") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-warn-mode") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + commit, _, err := s.PushBranch("verify-warn-mode", "feature-a") + if err != nil { + t.Fatalf("PushBranch(feature-a) error = %v", err) + } + + // Create feature-b with unique content so the branches genuinely diverge + workDir, err := os.MkdirTemp("", "verify-commit-work-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. + + setupSh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := setupSh.Command("git", "clone", s.RepoURL("verify-warn-mode"), workDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := setupSh.Chdir(workDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := setupSh.Command("git", "checkout", "-b", "feature-b").Run(ctx); err != nil { + t.Fatalf("git checkout error = %v", err) + } + if err := os.WriteFile(filepath.Join(workDir, "unique-b.txt"), []byte("unique content for branch b"), 0o644); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := setupSh.Command("git", "add", "unique-b.txt").Run(ctx); err != nil { + t.Fatalf("git add error = %v", err) + } + if err := setupSh.Command("git", "commit", "-m", "unique commit on feature-b").Run(ctx); err != nil { + t.Fatalf("git commit error = %v", err) + } + if err := setupSh.Command("git", "push", "origin", "feature-b").Run(ctx); err != nil { + t.Fatalf("git push error = %v", err) + } + + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + if err := sh.Command("git", "clone", s.RepoURL("verify-warn-mode"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := sh.Command("git", "fetch", "origin", "feature-a:refs/remotes/origin/feature-a", "feature-b:refs/remotes/origin/feature-b").Run(ctx); err != nil { + t.Fatalf("git fetch error = %v", err) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "warn", + Commit: commit, // commit from feature-a + Branch: "origin/feature-b", // but checking against feature-b + }, + } + + // In warn mode, verification failure should NOT return an error + err = e.verifyCommit(ctx) + if err != nil { + t.Errorf("verifyCommit() in warn mode error = %v, want nil", err) + } + }) + + t.Run("passes after deepening a shallow clone", func(t *testing.T) { + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + ctx := context.Background() + + s := githttptest.NewServer() + defer s.Close() + + err := s.CreateRepository("verify-shallow") + if err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + + _, err = s.InitRepository("verify-shallow") + if err != nil { + t.Fatalf("InitRepository error = %v", err) + } + + // The initial commit from InitRepository is on main. + // PushBranch adds one more commit on a feature branch. + // We need the initial commit SHA to test — it will be beyond depth=1. + commit, _, err := s.PushBranch("verify-shallow", "feature") + if err != nil { + t.Fatalf("PushBranch error = %v", err) + } + + // Get the initial commit (parent of the feature commit) by reading it from the server repo + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + // Shallow clone with depth=1 — only gets the tip commit + if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", s.RepoURL("verify-shallow"), cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + + // The tip commit is the one from PushBranch — it IS on the branch, + // but let's verify the shallow clone scenario works at all. + // With depth=1, the commit should be present and verifiable. + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: commit, + Branch: "origin/feature", + }, + } + + err = e.verifyCommit(ctx) + if err != nil { + t.Errorf("verifyCommit() error = %v, want nil", err) + } + }) +}