From ea136829ee98b1e018d9b09e5461ac6b86f71044 Mon Sep 17 00:00:00 2001 From: Pete Tomasik Date: Thu, 16 Jul 2026 14:51:33 -0400 Subject: [PATCH 1/6] Fix commit verification on non-default branches checkCommitOnBranch ran `git merge-base --is-ancestor ` against the bare BUILDKITE_BRANCH name, but at verification time only the repository's default branch exists as a local ref. Git does not resolve a bare name to refs/remotes/origin/*, so merge-base exited 128 for any non-default branch and the check degraded to "unavailable" (warn-only, even under strict). Strict verification therefore only ever enforced on the default branch. Fetch the branch tip and pin it to a SHA, then check ancestry against that SHA. Only trust a "not an ancestor" result once the repo is non-shallow, and treat a merge-base that fails to run as unavailable rather than a definitive failure. Fast-break the checkout retry loop on a definitive failure so a provably-off-branch commit fails immediately instead of re-cloning through the full backoff. --- internal/job/checkout.go | 6 +++ internal/job/commit_verification.go | 67 ++++++++++++++++++----------- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 0fde09bb24..049b2de96a 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -299,6 +299,12 @@ func (e *Executor) checkout(ctx context.Context) error { var errGit *gitError switch { + case errors.Is(err, ErrCommitVerificationFailed): + // A commit that is provably not on its branch won't become valid by + // retrying, so fail fast instead of re-cloning through the whole backoff. + e.shell.Warningf("Checkout failed! %s", err) + r.Break() + case errors.Is(err, errCheckoutAttemptTimedOut): // The per-attempt timeout fired and git was signal-killed. // Treat this like a generic transient failure: warn, clean diff --git a/internal/job/commit_verification.go b/internal/job/commit_verification.go index 13a075e2c5..87e51dcf31 100644 --- a/internal/job/commit_verification.go +++ b/internal/job/commit_verification.go @@ -26,47 +26,62 @@ var ErrCommitVerificationUnavailable = errors.New("commit verification unavailab func (e *Executor) checkCommitOnBranch(ctx context.Context) error { e.shell.Commentf("Verifying commit %q is on branch %q", e.Commit, e.Branch) + // The build's branch usually isn't a local ref at this point: the checkout + // fetches the commit directly (detached HEAD) and only the repository's + // default branch gets a local ref from the clone. Fetch the branch tip so + // there's a resolvable ref to check the commit's ancestry against, and pin it + // to a SHA (the deepening fetches below overwrite FETCH_HEAD). Without this, + // merge-base against the bare branch name fails to resolve for any non-default + // branch and the check silently degrades to "unavailable". + if fetchErr := e.shell.Command("git", "fetch", "origin", e.Branch).Run(ctx); fetchErr != nil { + return fmt.Errorf("%w: unable to fetch branch %q: %w", ErrCommitVerificationUnavailable, e.Branch, fetchErr) + } + branchTip, err := e.shell.Command("git", "rev-parse", "FETCH_HEAD").RunAndCaptureStdout(ctx) + if err != nil { + return fmt.Errorf("%w: unable to resolve branch %q: %w", ErrCommitVerificationUnavailable, e.Branch, err) + } + branchTip = strings.TrimSpace(branchTip) + + // merge-base --is-ancestor walks back from the branch tip: exit 0 means the + // commit is reachable from it (definitive, even on a shallow clone), exit 1 + // means it is not. On a shallow clone a "not an ancestor" result can be a + // false negative when the connecting history lies beyond the shallow boundary, + // so a negative (or otherwise inconclusive) result is only trusted once the + // repository is no longer shallow; until then we deepen the branch and re-check. for _, fetchFlag := range []string{"", "--deepen=50", "--unshallow"} { if fetchFlag != "" { - // After the first iteration, try to unshallow the repo (a bit or a lot) e.shell.Commentf("Deepening checkout to verify commit (%s)...", fetchFlag) - fetchErr := e.shell.Command("git", "fetch", fetchFlag).Run(ctx) - if fetchErr != nil { + if fetchErr := e.shell.Command("git", "fetch", fetchFlag, "origin", e.Branch).Run(ctx); fetchErr != nil { return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, fetchErr) } } - // Try the ancestry check - err := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx) - - switch shell.ExitCode(err) { - 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: - // unclear — continue below - default: - return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, err) + mergeBaseErr := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, branchTip).Run(ctx) + if mergeBaseErr == nil { + return nil // commit is reachable from the branch tip: verified } - - // On the first iteration, check if the checkout is shallow. If it is - // not, the 128 exit code reflects some other error. - if fetchFlag != "" { - continue + // A non-exit error (e.g. git failed to spawn) tells us nothing about + // ancestry, so treat it as unavailable rather than a definitive failure. + if !shell.IsExitError(mergeBaseErr) { + return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, mergeBaseErr) } - output, shallowErr := e.shell.Command("git", "rev-parse", "--is-shallow-repository").RunAndCaptureStdout(ctx) + exitCode := shell.ExitCode(mergeBaseErr) + + shallow, shallowErr := e.shell.Command("git", "rev-parse", "--is-shallow-repository").RunAndCaptureStdout(ctx) if shallowErr != nil { return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, shallowErr) } - - 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) + if strings.TrimSpace(shallow) != "true" { + // Full history, so the result is now definitive. + if exitCode == 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", ErrCommitVerificationUnavailable, e.Commit, e.Branch) } + // Still shallow, so deepen on the next iteration and re-check. } - // All attempts exhausted — verification is unavailable + // All attempts exhausted, so verification is unavailable. return fmt.Errorf("%w: unable to verify commit %q on branch %q after exhausting fetch strategies", ErrCommitVerificationUnavailable, e.Commit, e.Branch) } From 0ef4e33fc5851f50e754ec6ec98f21055971b746 Mon Sep 17 00:00:00 2001 From: Pete Tomasik Date: Thu, 16 Jul 2026 14:51:43 -0400 Subject: [PATCH 2/6] Test commit verification on non-default and shallow branches Cover the paths the non-default-branch fix introduced: a shallow clone with a genuinely off-branch commit must deepen and then block, the retry loop must fast-break rather than re-verify on every attempt, and an unavailable check must stay warn-only even under strict. Strengthen the warn-mode test to prove it swallows a genuine failure rather than passing because the check silently degraded. Add a regression test for the original bug: a build on a non-default branch with an off-branch commit. --- internal/job/commit_verification_test.go | 242 ++++++++++++++++-- .../integration/checkout_integration_test.go | 99 +++++-- 2 files changed, 298 insertions(+), 43 deletions(-) diff --git a/internal/job/commit_verification_test.go b/internal/job/commit_verification_test.go index 3ae38b319f..e95e727ff8 100644 --- a/internal/job/commit_verification_test.go +++ b/internal/job/commit_verification_test.go @@ -1,8 +1,10 @@ package job import ( + "errors" "os" "path/filepath" + "strings" "testing" "github.com/buildkite/agent/v3/internal/job/githttptest" @@ -141,13 +143,18 @@ func TestVerifyCommit(t *testing.T) { ExecutorConfig: ExecutorConfig{ GitCommitVerification: "strict", Commit: commit, - Branch: "origin/feature", + // A plain branch name, as BUILDKITE_BRANCH always is, not a + // pre-resolved remote-tracking ref. checkCommitOnBranch fetches it. + Branch: "feature", }, } - err = e.verifyCommit(ctx) - if err != nil { - t.Errorf("verifyCommit() error = %v, want nil", err) + // checkCommitOnBranch returns nil only when the commit is genuinely + // verified on the branch; an unavailable check returns an error instead. + // Asserting nil here proves verification ran, rather than silently + // degrading to a warning (which verifyCommit would also report as nil). + if err := e.checkCommitOnBranch(ctx); err != nil { + t.Errorf("checkCommitOnBranch() error = %v, want nil (verified)", err) } }) @@ -239,8 +246,8 @@ func TestVerifyCommit(t *testing.T) { shell: sh, ExecutorConfig: ExecutorConfig{ GitCommitVerification: "strict", - Commit: commit, // commit from feature-a - Branch: "origin/feature-b", // but checking against feature-b + Commit: commit, // commit from feature-a + Branch: "feature-b", // but checking against feature-b }, } @@ -334,14 +341,19 @@ func TestVerifyCommit(t *testing.T) { shell: sh, ExecutorConfig: ExecutorConfig{ GitCommitVerification: "warn", - Commit: commit, // commit from feature-a - Branch: "origin/feature-b", // but checking against feature-b + Commit: commit, // commit from feature-a + Branch: "feature-b", // but checking against feature-b }, } - // In warn mode, verification failure should NOT return an error - err = e.verifyCommit(ctx) - if err != nil { + // The underlying check must genuinely detect the failure (not merely + // degrade to "unavailable"), or warn mode swallowing it below proves nothing. + if err := e.checkCommitOnBranch(ctx); !errors.Is(err, ErrCommitVerificationFailed) { + t.Fatalf("checkCommitOnBranch() error = %v, want ErrCommitVerificationFailed", err) + } + + // In warn mode, that failure must NOT be surfaced as an error. + if err := e.verifyCommit(ctx); err != nil { t.Errorf("verifyCommit() in warn mode error = %v, want nil", err) } }) @@ -367,15 +379,35 @@ func TestVerifyCommit(t *testing.T) { 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 { + // InitRepository puts the initial commit on main; PushBranch adds one more + // commit on feature. The initial commit is an ancestor of feature but sits + // beyond a depth=1 clone's boundary, so verifying it forces the deepen path. + if _, _, err := s.PushBranch("verify-shallow", "feature"); err != nil { t.Fatalf("PushBranch error = %v", err) } - // Get the initial commit (parent of the feature commit) by reading it from the server repo + // Read the initial commit SHA (the deep ancestor) from a throwaway full clone. + refDir, err := os.MkdirTemp("", "verify-commit-ref-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(refDir) }) //nolint:errcheck // Best-effort cleanup. + refSh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := refSh.Command("git", "clone", s.RepoURL("verify-shallow"), refDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := refSh.Chdir(refDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + deepCommit, err := refSh.Command("git", "rev-parse", "origin/main").RunAndCaptureStdout(ctx) + if err != nil { + t.Fatalf("rev-parse origin/main error = %v", err) + } + deepCommit = strings.TrimSpace(deepCommit) + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") if err != nil { t.Fatalf("MkdirTemp error = %v", err) @@ -387,7 +419,7 @@ func TestVerifyCommit(t *testing.T) { t.Fatalf("shell.New() error = %v", err) } - // Shallow clone with depth=1 — only gets the tip commit + // Shallow clone with depth=1: the deep ancestor is beyond the boundary. 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) } @@ -395,21 +427,181 @@ func TestVerifyCommit(t *testing.T) { 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. + // The commit is a genuine ancestor of feature, but not visible until the + // shallow clone is deepened. verifyCommit must deepen rather than trust the + // initial shallow "not an ancestor" result. e := &Executor{ shell: sh, ExecutorConfig: ExecutorConfig{ GitCommitVerification: "strict", - Commit: commit, - Branch: "origin/feature", + Commit: deepCommit, + Branch: "feature", }, } - err = e.verifyCommit(ctx) + // Assert on checkCommitOnBranch (nil only when truly verified) so the test + // fails if the deepen path regresses to reporting the commit unavailable. + if err := e.checkCommitOnBranch(ctx); err != nil { + t.Errorf("checkCommitOnBranch() error = %v, want nil (verified after deepening)", err) + } + }) + + t.Run("fails on a shallow clone when commit is not an ancestor", 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 := t.Context() + + s := githttptest.NewServer() + defer s.Close() + + if err := s.CreateRepository("verify-shallow-fail"); err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + if _, err := s.InitRepository("verify-shallow-fail"); err != nil { + t.Fatalf("InitRepository error = %v", err) + } + if _, _, err := s.PushBranch("verify-shallow-fail", "feature"); err != nil { + t.Fatalf("PushBranch error = %v", err) + } + + // Build a commit on "other" that diverges from feature, so it is genuinely + // not reachable from it. Deepening feature never brings this commit in, so + // the shallow clone below must fetch it directly (as the real checkout does + // for BUILDKITE_COMMIT) for the ancestry check to resolve to a definitive + // "not on branch" instead of "unavailable". + 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-shallow-fail"), 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", "other").Run(ctx); err != nil { + t.Fatalf("git checkout error = %v", err) + } + if err := os.WriteFile(filepath.Join(workDir, "unique-other.txt"), []byte("unique content on other"), 0o644); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := setupSh.Command("git", "add", "unique-other.txt").Run(ctx); err != nil { + t.Fatalf("git add error = %v", err) + } + if err := setupSh.Command("git", "commit", "-m", "unique commit on other").Run(ctx); err != nil { + t.Fatalf("git commit error = %v", err) + } + if err := setupSh.Command("git", "push", "origin", "other").Run(ctx); err != nil { + t.Fatalf("git push error = %v", err) + } + offBranchCommit, err := setupSh.Command("git", "rev-parse", "HEAD").RunAndCaptureStdout(ctx) + if err != nil { + t.Fatalf("rev-parse HEAD error = %v", err) + } + offBranchCommit = strings.TrimSpace(offBranchCommit) + + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") if err != nil { - t.Errorf("verifyCommit() error = %v, want nil", err) + 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 of feature, then fetch the off-branch commit at depth=1 so + // the repo stays shallow going into checkCommitOnBranch. + if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", s.RepoURL("verify-shallow-fail"), 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", "--depth=1", "origin", "other").Run(ctx); err != nil { + t.Fatalf("git fetch (off-branch commit) error = %v", err) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: offBranchCommit, + Branch: "feature", + }, + } + + // A shallow clone must not let a genuinely off-branch commit slip through: + // deepen/unshallow, then report a definitive failure, not "unavailable". + if err := e.checkCommitOnBranch(ctx); !errors.Is(err, ErrCommitVerificationFailed) { + t.Errorf("checkCommitOnBranch() error = %v, want ErrCommitVerificationFailed", err) + } + }) + + t.Run("unavailable check does not fail strict mode", 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 := t.Context() + + s := githttptest.NewServer() + defer s.Close() + + if err := s.CreateRepository("verify-unavailable"); err != nil { + t.Fatalf("CreateRepository error = %v", err) + } + if _, err := s.InitRepository("verify-unavailable"); err != nil { + t.Fatalf("InitRepository error = %v", err) + } + commit, _, err := s.PushBranch("verify-unavailable", "feature") + if err != nil { + t.Fatalf("PushBranch 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-unavailable"), 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) + } + + // A branch the remote doesn't have can't be fetched, so ancestry can't be + // checked. That is an infrastructure failure, not evidence of an attack. + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: commit, + Branch: "does-not-exist", + }, + } + + if err := e.checkCommitOnBranch(ctx); !errors.Is(err, ErrCommitVerificationUnavailable) { + t.Fatalf("checkCommitOnBranch() error = %v, want ErrCommitVerificationUnavailable", err) + } + + // Even in strict mode, an unavailable check must not fail the build. + if err := e.verifyCommit(ctx); err != nil { + t.Errorf("verifyCommit() in strict mode with unavailable check error = %v, want nil", err) } }) } diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index 565114e244..602e370e44 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -1771,14 +1771,19 @@ func TestCommitVerificationWithValidCommit(t *testing.T) { 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. + // Expect the normal checkout flow with commit verification inserted between + // fetch and checkout: fetch the branch tip, pin it via FETCH_HEAD, then + // merge-base --is-ancestor against that SHA. Here the branch (main) tip is the + // build commit itself, so both merge-base args are commitHash. Since this is a + // full clone (not shallow) and the commit verifies, merge-base succeeds + // immediately, so no rev-parse --is-shallow-repository is needed. git.ExpectAll([][]any{ {"clone", "-v", "--", tester.Repo.Path, "."}, {"clean", "-fdq"}, {"fetch", "-v", "--", "origin", commitHash}, - {"merge-base", "--is-ancestor", commitHash, "main"}, + {"fetch", "origin", "main"}, + {"rev-parse", "FETCH_HEAD"}, + {"merge-base", "--is-ancestor", commitHash, commitHash}, {"-c", "advice.detachedHead=false", "checkout", "-f", commitHash}, {"clean", "-fdq"}, {"--no-pager", "log", "-1", commitHash, "-s", "--no-color", gitShowFormatArg}, @@ -1815,26 +1820,84 @@ func TestCommitVerificationFailsWithInvalidCommit(t *testing.T) { fmt.Sprintf("BUILDKITE_COMMIT=%s", commitHash), } - git := tester. - MustMock(t, "git"). - PassthroughToLocalCommand() + // Use real git (no mock) and assert on the outcome rather than the exact + // command sequence: the build must fail because the commit is not on the branch. + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).Min(0).Max(bintest.InfiniteTimes).AndExitWith(1) + agent.IgnoreUnexpectedInvocations() - // 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"}, - }) + // 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") + } + if !strings.Contains(tester.Output, "is not on branch") { + t.Errorf("expected a commit-verification failure in the output, got:\n%s", tester.Output) + } + // The retry loop fast-breaks on a provably-off-branch commit, so verification + // runs once rather than re-cloning through the full backoff (default 6 + // attempts). checkCommitOnBranch logs "Verifying commit" once per attempt. + if got := strings.Count(tester.Output, "Verifying commit"); got != 1 { + t.Errorf("commit verification ran %d times, want 1 (fast-break should prevent retries)", got) + } +} + +// TestCommitVerificationFailsForCommitNotOnNonDefaultBranch is the regression +// test for the case the old bare-branch-name check missed: a build on a branch +// other than the repository default. The clone only materialises the default +// branch as a local ref, so checking a commit against the bare build-branch name +// used to resolve to nothing and silently degrade to "unavailable" (warn, never +// block), even for a commit that is genuinely not on that branch. +func TestCommitVerificationFailsForCommitNotOnNonDefaultBranch(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + // Add a commit on main (the default branch) that diverges from update-test-txt, + // so it is genuinely not reachable from that non-default branch. + if err := tester.Repo.CheckoutBranch("main"); err != nil { + t.Fatalf("CheckoutBranch(main) error = %v", err) + } + if err := os.WriteFile(filepath.Join(tester.Repo.Path, "main-only.txt"), []byte("main only"), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := tester.Repo.Add("main-only.txt"); err != nil { + t.Fatalf("Add error = %v", err) + } + if err := tester.Repo.Commit("Commit only on main"); err != nil { + t.Fatalf("Commit error = %v", err) + } + offBranchCommit, err := tester.Repo.RevParse("main") + if err != nil { + t.Fatalf("RevParse(main) error = %v", err) + } + offBranchCommit = strings.TrimSpace(offBranchCommit) + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=-v", + "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", + "BUILDKITE_GIT_FETCH_FLAGS=-v", + "BUILDKITE_GIT_COMMIT_VERIFICATION=strict", + "BUILDKITE_BRANCH=update-test-txt", + fmt.Sprintf("BUILDKITE_COMMIT=%s", offBranchCommit), + } + + // Use real git (no mock) and assert on the outcome rather than the exact + // command sequence: the build must fail because the commit is not on the branch. agent := tester.MockAgent(t) - agent.Expect("meta-data", "exists", job.CommitMetadataKey).AndExitWith(1) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).Min(0).Max(bintest.InfiniteTimes).AndExitWith(1) + agent.IgnoreUnexpectedInvocations() - // 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") + t.Fatalf("expected build to fail: commit %s is not on branch update-test-txt, but it passed. Output:\n%s", offBranchCommit, tester.Output) + } + if !strings.Contains(tester.Output, "is not on branch") { + t.Errorf("expected a commit-verification failure in the output, got:\n%s", tester.Output) } } From 8e9d831b05a37792bf872e87c254947ee91e91e1 Mon Sep 17 00:00:00 2001 From: Pete Tomasik Date: Thu, 16 Jul 2026 15:46:47 -0400 Subject: [PATCH 3/6] Verify commits on non-pull-request builds verifyCommit skipped verification whenever PullRequest was non-empty, but BUILDKITE_PULL_REQUEST is the string "false" on every ordinary non-PR build. The guard therefore fired on almost all builds and returned before the ancestry check ran, logging "Skipping commit verification: pull request build (#false)". Skip only for real PR builds, matching how the rest of the checkout treats the "false" sentinel. --- internal/job/commit_verification.go | 5 +++-- internal/job/integration/checkout_integration_test.go | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/job/commit_verification.go b/internal/job/commit_verification.go index 87e51dcf31..0304912e34 100644 --- a/internal/job/commit_verification.go +++ b/internal/job/commit_verification.go @@ -111,8 +111,9 @@ func (e *Executor) verifyCommit(ctx context.Context) error { return nil } - // Skip if this is a PR build — the commit may be on a merge ref, not the target branch - if e.PullRequest != "" { + // Skip if this is a PR build: the commit may be on a merge ref, not the target + // branch. BUILDKITE_PULL_REQUEST is the string "false" (not empty) on non-PR builds. + if e.PullRequest != "" && e.PullRequest != "false" { e.shell.Commentf("Skipping commit verification: pull request build (#%s)", e.PullRequest) return nil } diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index 602e370e44..c91b966ae3 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -1817,6 +1817,10 @@ func TestCommitVerificationFailsWithInvalidCommit(t *testing.T) { "BUILDKITE_GIT_CLEAN_FLAGS=-fdq", "BUILDKITE_GIT_FETCH_FLAGS=-v", "BUILDKITE_GIT_COMMIT_VERIFICATION=strict", + // The non-PR sentinel. The agent sets this on every ordinary build, so + // verification must still run (a naive PullRequest != "" skip would disable + // it for almost every build). + "BUILDKITE_PULL_REQUEST=false", fmt.Sprintf("BUILDKITE_COMMIT=%s", commitHash), } From 3889d3c3cd3fa8fe7ab9d6387eb32fa5d89412c4 Mon Sep 17 00:00:00 2001 From: Pete Tomasik Date: Thu, 16 Jul 2026 15:52:43 -0400 Subject: [PATCH 4/6] Avoid duplicated prefix in commit verification warnings The warn-mode branches logged "Commit verification failed/unavailable:" in front of an error already wrapped with the matching sentinel, so the message read "Commit verification failed: commit verification failed: ...". Log the error directly, since it already carries the prefix. --- internal/job/commit_verification.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/job/commit_verification.go b/internal/job/commit_verification.go index 0304912e34..5a72a7b4ff 100644 --- a/internal/job/commit_verification.go +++ b/internal/job/commit_verification.go @@ -138,13 +138,15 @@ func (e *Executor) verifyCommit(ctx context.Context) error { if e.GitCommitVerification == "strict" { return err } - e.shell.Warningf("Commit verification failed: %v", err) + // err already begins with "commit verification failed", so log it as-is. + e.shell.Warningf("%s", err) return nil } // Verification unavailable — infrastructure issue, not a security concern. // We always warn but never block, even in strict mode, to avoid users // disabling verification entirely due to infrastructure false positives. - e.shell.Warningf("Commit verification unavailable: %v", err) + // err already begins with "commit verification unavailable", so log it as-is. + e.shell.Warningf("%s", err) return nil } From 3acfa66880fe2aa90ffe7fef70c8f6a6e3005f3a Mon Sep 17 00:00:00 2001 From: Pete Tomasik Date: Thu, 16 Jul 2026 16:12:48 -0400 Subject: [PATCH 5/6] Use a file:// remote for shallow commit verification tests The two shallow-clone subtests fetched over githttptest, which serves via `git upload-pack --stateless-rpc`. Shallow deepening over that transport is unreliable across git versions: on CI, `git fetch --deepen` marked the repo non-shallow without fetching the deep ancestor, so "passes after deepening a shallow clone" saw the commit as unavailable and failed. The production code is unaffected (real remotes deepen correctly). Build the fixtures in a bare repo served over file://, where shallow deepening is deterministic, via a shared setupFileBackedRepo helper. The ancestry relationships are genuine, so the verdict is correct regardless of shallow-negotiation quirks. --- internal/job/commit_verification_test.go | 212 ++++++++++------------- 1 file changed, 94 insertions(+), 118 deletions(-) diff --git a/internal/job/commit_verification_test.go b/internal/job/commit_verification_test.go index e95e727ff8..b5f5ceb9aa 100644 --- a/internal/job/commit_verification_test.go +++ b/internal/job/commit_verification_test.go @@ -1,6 +1,7 @@ package job import ( + "context" "errors" "os" "path/filepath" @@ -11,6 +12,87 @@ import ( "github.com/buildkite/agent/v3/internal/shell" ) +// setupFileBackedRepo creates a bare git repository served over file:// with a +// "feature" branch (commits a <- b <- c) and a divergent "other" branch (a <- o). +// It returns the file:// URL, a deep ancestor of feature (a, beyond a depth=1 +// clone's boundary), and an off-branch commit (o, present on the remote but not on +// feature). A file:// remote is used rather than githttptest because shallow +// deepening over the test server's stateless-rpc HTTP transport is not reliable +// across git versions. +func setupFileBackedRepo(t *testing.T, ctx context.Context) (repoURL, deepAncestor, offBranchCommit string) { + t.Helper() + 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") + + bareDir, err := os.MkdirTemp("", "verify-bare-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(bareDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := sh.Command("git", "init", "--bare", bareDir).Run(ctx); err != nil { + t.Fatalf("git init --bare error = %v", err) + } + repoURL = "file://" + bareDir + + workDir, err := os.MkdirTemp("", "verify-work-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. + if err := sh.Command("git", "clone", repoURL, workDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(workDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + + commit := func(name string) string { + if err := os.WriteFile(filepath.Join(workDir, name), []byte(name), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := sh.Command("git", "add", name).Run(ctx); err != nil { + t.Fatalf("git add error = %v", err) + } + if err := sh.Command("git", "commit", "-m", "commit "+name).Run(ctx); err != nil { + t.Fatalf("git commit error = %v", err) + } + sha, err := sh.Command("git", "rev-parse", "HEAD").RunAndCaptureStdout(ctx) + if err != nil { + t.Fatalf("rev-parse HEAD error = %v", err) + } + return strings.TrimSpace(sha) + } + + // feature: a <- b <- c, where a is two commits below the tip. + deepAncestor = commit("a.txt") + commit("b.txt") + commit("c.txt") + if err := sh.Command("git", "branch", "-m", "feature").Run(ctx); err != nil { + t.Fatalf("git branch -m feature error = %v", err) + } + if err := sh.Command("git", "push", "origin", "feature").Run(ctx); err != nil { + t.Fatalf("git push feature error = %v", err) + } + + // other: a <- o, diverging from feature so o is not an ancestor of feature. + if err := sh.Command("git", "checkout", "-b", "other", deepAncestor).Run(ctx); err != nil { + t.Fatalf("git checkout -b other error = %v", err) + } + offBranchCommit = commit("other.txt") + if err := sh.Command("git", "push", "origin", "other").Run(ctx); err != nil { + t.Fatalf("git push other error = %v", err) + } + + return repoURL, deepAncestor, offBranchCommit +} + func TestVerifyCommit(t *testing.T) { // Table-driven tests for the skip conditions — these don't need a real git repo skipTests := []struct { @@ -359,154 +441,47 @@ func TestVerifyCommit(t *testing.T) { }) 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 := t.Context() - - 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) - } - - // InitRepository puts the initial commit on main; PushBranch adds one more - // commit on feature. The initial commit is an ancestor of feature but sits - // beyond a depth=1 clone's boundary, so verifying it forces the deepen path. - if _, _, err := s.PushBranch("verify-shallow", "feature"); err != nil { - t.Fatalf("PushBranch error = %v", err) - } - - // Read the initial commit SHA (the deep ancestor) from a throwaway full clone. - refDir, err := os.MkdirTemp("", "verify-commit-ref-") - if err != nil { - t.Fatalf("MkdirTemp error = %v", err) - } - t.Cleanup(func() { os.RemoveAll(refDir) }) //nolint:errcheck // Best-effort cleanup. - refSh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) - } - if err := refSh.Command("git", "clone", s.RepoURL("verify-shallow"), refDir).Run(ctx); err != nil { - t.Fatalf("git clone error = %v", err) - } - if err := refSh.Chdir(refDir); err != nil { - t.Fatalf("Chdir error = %v", err) - } - deepCommit, err := refSh.Command("git", "rev-parse", "origin/main").RunAndCaptureStdout(ctx) - if err != nil { - t.Fatalf("rev-parse origin/main error = %v", err) - } - deepCommit = strings.TrimSpace(deepCommit) + repoURL, deepAncestor, _ := setupFileBackedRepo(t, ctx) 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: the deep ancestor is beyond the boundary. - if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", s.RepoURL("verify-shallow"), cloneDir).Run(ctx); err != nil { + // Shallow clone with depth=1: the ancestor is beyond the boundary, so the + // check must deepen to find it. + if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", repoURL, 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 commit is a genuine ancestor of feature, but not visible until the - // shallow clone is deepened. verifyCommit must deepen rather than trust the - // initial shallow "not an ancestor" result. e := &Executor{ shell: sh, ExecutorConfig: ExecutorConfig{ GitCommitVerification: "strict", - Commit: deepCommit, + Commit: deepAncestor, Branch: "feature", }, } - // Assert on checkCommitOnBranch (nil only when truly verified) so the test - // fails if the deepen path regresses to reporting the commit unavailable. + // nil only when truly verified, so this fails if the deepen path regresses + // to reporting the commit unavailable. if err := e.checkCommitOnBranch(ctx); err != nil { t.Errorf("checkCommitOnBranch() error = %v, want nil (verified after deepening)", err) } }) t.Run("fails on a shallow clone when commit is not an ancestor", 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 := t.Context() - - s := githttptest.NewServer() - defer s.Close() - - if err := s.CreateRepository("verify-shallow-fail"); err != nil { - t.Fatalf("CreateRepository error = %v", err) - } - if _, err := s.InitRepository("verify-shallow-fail"); err != nil { - t.Fatalf("InitRepository error = %v", err) - } - if _, _, err := s.PushBranch("verify-shallow-fail", "feature"); err != nil { - t.Fatalf("PushBranch error = %v", err) - } - - // Build a commit on "other" that diverges from feature, so it is genuinely - // not reachable from it. Deepening feature never brings this commit in, so - // the shallow clone below must fetch it directly (as the real checkout does - // for BUILDKITE_COMMIT) for the ancestry check to resolve to a definitive - // "not on branch" instead of "unavailable". - 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-shallow-fail"), 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", "other").Run(ctx); err != nil { - t.Fatalf("git checkout error = %v", err) - } - if err := os.WriteFile(filepath.Join(workDir, "unique-other.txt"), []byte("unique content on other"), 0o644); err != nil { - t.Fatalf("WriteFile error = %v", err) - } - if err := setupSh.Command("git", "add", "unique-other.txt").Run(ctx); err != nil { - t.Fatalf("git add error = %v", err) - } - if err := setupSh.Command("git", "commit", "-m", "unique commit on other").Run(ctx); err != nil { - t.Fatalf("git commit error = %v", err) - } - if err := setupSh.Command("git", "push", "origin", "other").Run(ctx); err != nil { - t.Fatalf("git push error = %v", err) - } - offBranchCommit, err := setupSh.Command("git", "rev-parse", "HEAD").RunAndCaptureStdout(ctx) - if err != nil { - t.Fatalf("rev-parse HEAD error = %v", err) - } - offBranchCommit = strings.TrimSpace(offBranchCommit) + repoURL, _, offBranchCommit := setupFileBackedRepo(t, ctx) cloneDir, err := os.MkdirTemp("", "verify-commit-test-") if err != nil { @@ -518,9 +493,10 @@ func TestVerifyCommit(t *testing.T) { t.Fatalf("shell.New() error = %v", err) } - // Shallow clone of feature, then fetch the off-branch commit at depth=1 so - // the repo stays shallow going into checkCommitOnBranch. - if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", s.RepoURL("verify-shallow-fail"), cloneDir).Run(ctx); err != nil { + // Shallow clone of feature, then fetch the off-branch commit at depth=1 so it + // is present locally (as the real checkout fetches BUILDKITE_COMMIT) while the + // repo stays shallow going into checkCommitOnBranch. + if err := sh.Command("git", "clone", "--depth=1", "--branch", "feature", repoURL, cloneDir).Run(ctx); err != nil { t.Fatalf("git clone error = %v", err) } if err := sh.Chdir(cloneDir); err != nil { From ab0f3a5fecb215a115b9d2eae6c06fed63f9d931 Mon Sep 17 00:00:00 2001 From: Pete Tomasik Date: Fri, 17 Jul 2026 14:53:11 -0400 Subject: [PATCH 6/6] Qualify branch ref in commit verification fetch An unqualified 'git fetch origin ' resolves refs/tags/ before refs/heads/, so a tag sharing the build branch's name pins FETCH_HEAD to the tag tip and merge-base then verifies the commit against the tag. A commit reachable only from the tag would pass verification even when it is not on the branch. Fetch refs/heads/ via the gitFetch helper at both the initial and deepening sites so the branch ref always wins. --- internal/job/commit_verification.go | 19 ++- internal/job/commit_verification_test.go | 122 ++++++++++++++++++ .../integration/checkout_integration_test.go | 13 +- 3 files changed, 146 insertions(+), 8 deletions(-) diff --git a/internal/job/commit_verification.go b/internal/job/commit_verification.go index 5a72a7b4ff..784d936382 100644 --- a/internal/job/commit_verification.go +++ b/internal/job/commit_verification.go @@ -33,7 +33,17 @@ func (e *Executor) checkCommitOnBranch(ctx context.Context) error { // to a SHA (the deepening fetches below overwrite FETCH_HEAD). Without this, // merge-base against the bare branch name fails to resolve for any non-default // branch and the check silently degrades to "unavailable". - if fetchErr := e.shell.Command("git", "fetch", "origin", e.Branch).Run(ctx); fetchErr != nil { + // + // Qualify the ref as refs/heads/: git resolves a bare name against + // refs/tags/ before refs/heads/, so a tag sharing the branch's name would pin + // FETCH_HEAD to the tag tip and verify the commit against the tag instead of + // the branch, letting a commit reachable only from the tag pass. + branchRef := "refs/heads/" + e.Branch + if fetchErr := gitFetch(ctx, gitFetchArgs{ + Shell: e.shell, + Repository: "origin", + RefSpecs: []string{branchRef}, + }); fetchErr != nil { return fmt.Errorf("%w: unable to fetch branch %q: %w", ErrCommitVerificationUnavailable, e.Branch, fetchErr) } branchTip, err := e.shell.Command("git", "rev-parse", "FETCH_HEAD").RunAndCaptureStdout(ctx) @@ -51,7 +61,12 @@ func (e *Executor) checkCommitOnBranch(ctx context.Context) error { for _, fetchFlag := range []string{"", "--deepen=50", "--unshallow"} { if fetchFlag != "" { e.shell.Commentf("Deepening checkout to verify commit (%s)...", fetchFlag) - if fetchErr := e.shell.Command("git", "fetch", fetchFlag, "origin", e.Branch).Run(ctx); fetchErr != nil { + if fetchErr := gitFetch(ctx, gitFetchArgs{ + Shell: e.shell, + GitFetchFlags: fetchFlag, + Repository: "origin", + RefSpecs: []string{branchRef}, + }); fetchErr != nil { return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, fetchErr) } } diff --git a/internal/job/commit_verification_test.go b/internal/job/commit_verification_test.go index b5f5ceb9aa..057b01422f 100644 --- a/internal/job/commit_verification_test.go +++ b/internal/job/commit_verification_test.go @@ -93,6 +93,88 @@ func setupFileBackedRepo(t *testing.T, ctx context.Context) (repoURL, deepAncest return repoURL, deepAncestor, offBranchCommit } +// setupTagBranchCollisionRepo creates a bare repo served over file:// where a +// branch and a tag share the name "release". The branch is a <- b; the tag points +// at a divergent commit t (a <- t) that is not reachable from the branch. It +// returns the file:// URL and the off-branch commit the tag points at. This is +// the ambiguous case: git resolves a bare name against refs/tags/ before +// refs/heads/, so an unqualified fetch of "release" pins FETCH_HEAD to the tag. +func setupTagBranchCollisionRepo(t *testing.T, ctx context.Context) (repoURL, offBranchTagCommit string) { + t.Helper() + 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") + + bareDir, err := os.MkdirTemp("", "verify-collision-bare-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(bareDir) }) //nolint:errcheck // Best-effort cleanup. + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := sh.Command("git", "init", "--bare", bareDir).Run(ctx); err != nil { + t.Fatalf("git init --bare error = %v", err) + } + repoURL = "file://" + bareDir + + workDir, err := os.MkdirTemp("", "verify-collision-work-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(workDir) }) //nolint:errcheck // Best-effort cleanup. + if err := sh.Command("git", "clone", repoURL, workDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(workDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + + commit := func(name string) string { + if err := os.WriteFile(filepath.Join(workDir, name), []byte(name), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if err := sh.Command("git", "add", name).Run(ctx); err != nil { + t.Fatalf("git add error = %v", err) + } + if err := sh.Command("git", "commit", "-m", "commit "+name).Run(ctx); err != nil { + t.Fatalf("git commit error = %v", err) + } + sha, err := sh.Command("git", "rev-parse", "HEAD").RunAndCaptureStdout(ctx) + if err != nil { + t.Fatalf("rev-parse HEAD error = %v", err) + } + return strings.TrimSpace(sha) + } + + // release branch: a <- b. + base := commit("a.txt") + commit("b.txt") + if err := sh.Command("git", "branch", "-m", "release").Run(ctx); err != nil { + t.Fatalf("git branch -m release error = %v", err) + } + if err := sh.Command("git", "push", "origin", "release").Run(ctx); err != nil { + t.Fatalf("git push release error = %v", err) + } + + // tag "release" on a divergent commit t (a <- t), not reachable from the branch. + if err := sh.Command("git", "checkout", "-b", "tagline", base).Run(ctx); err != nil { + t.Fatalf("git checkout -b tagline error = %v", err) + } + offBranchTagCommit = commit("t.txt") + if err := sh.Command("git", "tag", "release").Run(ctx); err != nil { + t.Fatalf("git tag release error = %v", err) + } + if err := sh.Command("git", "push", "origin", "refs/tags/release").Run(ctx); err != nil { + t.Fatalf("git push tag release error = %v", err) + } + + return repoURL, offBranchTagCommit +} + func TestVerifyCommit(t *testing.T) { // Table-driven tests for the skip conditions — these don't need a real git repo skipTests := []struct { @@ -522,6 +604,46 @@ func TestVerifyCommit(t *testing.T) { } }) + t.Run("fails when a same-named tag carries an off-branch commit", func(t *testing.T) { + ctx := t.Context() + repoURL, offBranchTagCommit := setupTagBranchCollisionRepo(t, ctx) + + 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) + } + + // A full clone brings refs/heads/release plus the refs/tags/release object, + // so the off-branch commit exists locally going into the check. + if err := sh.Command("git", "clone", "--branch", "release", repoURL, 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) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: offBranchTagCommit, + Branch: "release", + }, + } + + // The fetch must resolve refs/heads/release, not the same-named tag. If it + // pinned FETCH_HEAD to the tag tip, this commit (the tag itself) would pass; + // qualifying the ref makes it a definitive failure. + if err := e.checkCommitOnBranch(ctx); !errors.Is(err, ErrCommitVerificationFailed) { + t.Errorf("checkCommitOnBranch() error = %v, want ErrCommitVerificationFailed", err) + } + }) + t.Run("unavailable check does not fail strict mode", func(t *testing.T) { t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index c91b966ae3..79c06054e7 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -1772,16 +1772,17 @@ func TestCommitVerificationWithValidCommit(t *testing.T) { PassthroughToLocalCommand() // Expect the normal checkout flow with commit verification inserted between - // fetch and checkout: fetch the branch tip, pin it via FETCH_HEAD, then - // merge-base --is-ancestor against that SHA. Here the branch (main) tip is the - // build commit itself, so both merge-base args are commitHash. Since this is a - // full clone (not shallow) and the commit verifies, merge-base succeeds - // immediately, so no rev-parse --is-shallow-repository is needed. + // fetch and checkout: fetch the branch tip (as refs/heads/main, so a same-named + // tag can't win the ref lookup), pin it via FETCH_HEAD, then merge-base + // --is-ancestor against that SHA. Here the branch (main) tip is the build commit + // itself, so both merge-base args are commitHash. Since this is a full clone + // (not shallow) and the commit verifies, merge-base succeeds immediately, so no + // rev-parse --is-shallow-repository is needed. git.ExpectAll([][]any{ {"clone", "-v", "--", tester.Repo.Path, "."}, {"clean", "-fdq"}, {"fetch", "-v", "--", "origin", commitHash}, - {"fetch", "origin", "main"}, + {"fetch", "--", "origin", "refs/heads/main"}, {"rev-parse", "FETCH_HEAD"}, {"merge-base", "--is-ancestor", commitHash, commitHash}, {"-c", "advice.detachedHead=false", "checkout", "-f", commitHash},