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..784d936382 100644 --- a/internal/job/commit_verification.go +++ b/internal/job/commit_verification.go @@ -26,47 +26,77 @@ 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". + // + // 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) + 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 := 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) } } - // 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) } @@ -96,8 +126,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 } @@ -122,13 +153,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 } diff --git a/internal/job/commit_verification_test.go b/internal/job/commit_verification_test.go index 3ae38b319f..057b01422f 100644 --- a/internal/job/commit_verification_test.go +++ b/internal/job/commit_verification_test.go @@ -1,14 +1,180 @@ package job import ( + "context" + "errors" "os" "path/filepath" + "strings" "testing" "github.com/buildkite/agent/v3/internal/job/githttptest" "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 +} + +// 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 { @@ -141,13 +307,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 +410,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,19 +505,146 @@ 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) } }) t.Run("passes after deepening a shallow clone", func(t *testing.T) { + ctx := t.Context() + 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 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) + } + + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: "strict", + Commit: deepAncestor, + Branch: "feature", + }, + } + + // 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) { + ctx := t.Context() + repoURL, _, offBranchCommit := 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 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 { + 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("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") t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") @@ -357,59 +655,51 @@ func TestVerifyCommit(t *testing.T) { s := githttptest.NewServer() defer s.Close() - err := s.CreateRepository("verify-shallow") - if err != nil { + if err := s.CreateRepository("verify-unavailable"); err != nil { t.Fatalf("CreateRepository error = %v", err) } - - _, err = s.InitRepository("verify-shallow") - if err != nil { + if _, err := s.InitRepository("verify-unavailable"); 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") + commit, _, err := s.PushBranch("verify-unavailable", "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 { + 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) } - // 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. + // 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: "origin/feature", + Branch: "does-not-exist", }, } - err = e.verifyCommit(ctx) - if err != nil { - t.Errorf("verifyCommit() error = %v, want nil", err) + 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..79c06054e7 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -1771,14 +1771,20 @@ 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 (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}, - {"merge-base", "--is-ancestor", commitHash, "main"}, + {"fetch", "--", "origin", "refs/heads/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}, @@ -1812,29 +1818,91 @@ 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), } - 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) } }