--since-commit X without --branch means X..HEAD
This commit is contained in:
KevinPham-TruffleSecurity
@@ -3,7 +3,6 @@ package gitparse
|
||||
import (
|
||||
"bufio"
|
||||
"bytes"
|
||||
"cmp"
|
||||
"fmt"
|
||||
"io"
|
||||
"os"
|
||||
@@ -268,9 +267,8 @@ type gitArgs struct {
|
||||
// base), which is the diff-scan contract behind `--since-commit`. The range is
|
||||
// computed by git itself so that it is independent of commit dates and merge
|
||||
// topology. An empty base means a full-history scan of head (or of --all when
|
||||
// head is also empty). Callers supplying a base must also supply a head;
|
||||
// git.ScanRepo fills in HEAD for a missing one so that ^base is never paired
|
||||
// with --all, which would walk every ref not reachable from base.
|
||||
// head is also empty). An empty head with a non-empty base means base..HEAD,
|
||||
// the checked-out commit; ^base is never paired with --all.
|
||||
func (c *Parser) RepoPath(
|
||||
ctx context.Context,
|
||||
source string,
|
||||
@@ -457,9 +455,20 @@ func (c *Parser) prepGitArgs(source string, head string, base string, excludedGl
|
||||
args.show = append(args.show, "--diff-filter=AM")
|
||||
}
|
||||
|
||||
// Keep head or all last, before the --, not required but sensible
|
||||
// The positive end of the walk, kept last before the -- (not required but
|
||||
// sensible). A base with no head is a diff scan up to the checked-out
|
||||
// commit: the pre-commit hook and `--since-commit X` without `--branch`.
|
||||
// Defaulting to --all there would walk every ref not reachable from base,
|
||||
// which is not a diff of the change being made.
|
||||
// https://git-scm.com/docs/git-log#Documentation/git-log.txt---all
|
||||
args.log = append(args.log, cmp.Or(head, "--all"))
|
||||
switch {
|
||||
case head != "":
|
||||
args.log = append(args.log, head)
|
||||
case base != "":
|
||||
args.log = append(args.log, "HEAD")
|
||||
default:
|
||||
args.log = append(args.log, "--all")
|
||||
}
|
||||
|
||||
// `head ^base` is base..head. Keeping the two revisions as separate args
|
||||
// means head is always the positive end of the range and the base is
|
||||
|
||||
@@ -3,6 +3,7 @@ package gitparse
|
||||
import (
|
||||
"bytes"
|
||||
"path/filepath"
|
||||
"slices"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -65,8 +66,22 @@ func TestPrepGitArgs(t *testing.T) {
|
||||
// Revision args follow the flags; the exclusion belongs with the revisions.
|
||||
assert.Greater(t, indexOf(args.log, "^basesha"), indexOf(args.log, "headsha"))
|
||||
|
||||
// A base with no head is not a range this layer defines; git.ScanRepo
|
||||
// resolves the head to HEAD before calling RepoPath (see normalizeConfig).
|
||||
// A range with excludes: the pathspec separator must land after both
|
||||
// revisions or git reads the exclusion as a path.
|
||||
args = p.prepGitArgs(repopath, "headsha", "basesha", []string{"bloated.dat"}, false)
|
||||
full := slices.Concat(args.log, args.paths)
|
||||
assert.Greater(t, indexOf(full, "--"), indexOf(full, "^basesha"))
|
||||
|
||||
// A base with no head is base..HEAD: the checked-out commit is the positive
|
||||
// end, never --all. This is the pre-commit hook shape and `--since-commit X`
|
||||
// without `--branch`, and it must not go through normalizeConfig's merge
|
||||
// base, which cannot walk a shallow clone.
|
||||
args = p.prepGitArgs(repopath, "", "basesha", nil, false)
|
||||
assert.Contains(t, args.log, "HEAD")
|
||||
assert.Contains(t, args.log, "^basesha")
|
||||
assert.NotContains(t, args.log, "--all")
|
||||
assert.NotContains(t, args.log, "--diff-filter=AM")
|
||||
assert.Greater(t, indexOf(args.log, "^basesha"), indexOf(args.log, "HEAD"))
|
||||
|
||||
// test env passthrough used for pre-receive
|
||||
t.Setenv("GIT_OBJECT_DIRECTORY", "foo")
|
||||
|
||||
@@ -1034,6 +1034,10 @@ func (s *Git) ScanCommits(ctx context.Context, repo *git.Repository, path string
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
if scanOptions.BaseHash != "" && depth == 0 {
|
||||
logger.V(1).Info("no commits in range", logValues...)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -1366,12 +1370,6 @@ func (s *Git) ScanRepo(ctx context.Context, repo *git.Repository, repoPath strin
|
||||
// If either commit cannot be resolved, it returns early.
|
||||
// If both are resolved, it finds and sets the merge base in scanOptions.
|
||||
func normalizeConfig(scanOptions *ScanOptions, repo *git.Repository) error {
|
||||
// A base with no head is a diff scan up to the checked-out commit, so the head
|
||||
// is filled in as HEAD before anything is resolved.
|
||||
if scanOptions.BaseHash != "" && scanOptions.HeadHash == "" {
|
||||
scanOptions.HeadHash = "HEAD"
|
||||
}
|
||||
|
||||
baseCommit, err := resolveAndSetCommit(repo, &scanOptions.BaseHash)
|
||||
if err != nil {
|
||||
return err
|
||||
|
||||
+126
-43
@@ -1828,14 +1828,15 @@ func TestScanRepo_BaseMergedIntoHead(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestNormalizeConfig_BaseWithoutHead pins the contract that a diff scan always
|
||||
// has both ends of its range by the time it reaches the parser: a base with no
|
||||
// head is a scan up to the checked-out commit. Without this, the parser would
|
||||
// pair ^base with --all and walk every ref not reachable from base, which is
|
||||
// not what `--since-commit X` without `--branch` (the pre-commit shape) means.
|
||||
// TestNormalizeConfig_BaseWithoutHead pins what normalizeConfig does and does
|
||||
// not do with a base and no head: it resolves the base to a hash and leaves the
|
||||
// head empty. The parser supplies HEAD as the positive end of the range (see
|
||||
// gitparse.Parser.RepoPath). normalizeConfig must not resolve HEAD itself,
|
||||
// because that would route the base-only shape through MergeBase, which
|
||||
// go-git cannot compute across the graft of a shallow clone, including the
|
||||
// --shallow-since clone prepareRepoSinceCommit makes for exactly this shape.
|
||||
func TestNormalizeConfig_BaseWithoutHead(t *testing.T) {
|
||||
// main: A; feature: A -> B, checked out. A base of main against an
|
||||
// implicit head must resolve to HEAD (B) with the merge base A.
|
||||
// main: A; feature: A -> B, checked out.
|
||||
path := setupTestRepo(t, "base-without-head")
|
||||
addTestFileAndCommit(t, path, "a.txt", "a\n")
|
||||
runGit(t, path, "branch", "-M", "main")
|
||||
@@ -1854,14 +1855,16 @@ func TestNormalizeConfig_BaseWithoutHead(t *testing.T) {
|
||||
wantHead string
|
||||
}{
|
||||
{
|
||||
name: "base ref and no head resolves head to HEAD",
|
||||
base: "main", wantBase: shaA, wantHead: shaB,
|
||||
// The base is resolved to a hash; the head stays empty and no merge
|
||||
// base is computed. The parser turns this into main..HEAD.
|
||||
name: "base ref and no head leaves head empty",
|
||||
base: "main", wantBase: shaA, wantHead: "",
|
||||
},
|
||||
{
|
||||
// The pre-commit invocation: base and head are the same commit, so
|
||||
// the range is empty and only staged changes are left to scan.
|
||||
name: "base HEAD and no head is an empty range",
|
||||
base: "HEAD", wantBase: shaB, wantHead: shaB,
|
||||
// The pre-commit invocation: base HEAD, no head. The parser produces
|
||||
// HEAD..HEAD, an empty range, and only staged changes remain to scan.
|
||||
name: "base HEAD and no head leaves head empty",
|
||||
base: "HEAD", wantBase: shaB, wantHead: "",
|
||||
},
|
||||
{
|
||||
// Full-history scans set neither end and must stay that way; an
|
||||
@@ -1944,10 +1947,53 @@ func TestScanRepo_BaseWithoutHead(t *testing.T) {
|
||||
assert.Contains(t, data, stagedSecret, "staged changes must still be scanned")
|
||||
assert.NotContains(t, data, decoySecret, "a commit on an unrelated branch leaked into the scan")
|
||||
})
|
||||
|
||||
// Remote URLs are cloned with --mirror, so production base-only scans
|
||||
// run against a bare repository where the parser's HEAD has to
|
||||
// resolve through GIT_DIR rather than a checkout. A mirror's HEAD
|
||||
// follows the origin's checked-out branch, here feature at F.
|
||||
t.Run("bare mirror clone scans base..HEAD", func(t *testing.T) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||
defer cancel()
|
||||
|
||||
f := build(t)
|
||||
mirror := filepath.Join(t.TempDir(), "mirror.git")
|
||||
runGit(t, "", "clone", "-q", "--mirror", "file://"+f.path, mirror)
|
||||
assert.True(t, isRepoBare(mirror), "fixture is not bare")
|
||||
assert.Equal(t, f.sha["F"], gitRevParse(t, mirror, "HEAD"), "mirror HEAD should be the origin's checked-out commit")
|
||||
|
||||
got, data, err := scanRepoRange(ctx, t, mirror, f.sha["C"], "")
|
||||
assert.NoError(t, err)
|
||||
|
||||
for _, letter := range []string{"F", "M", "E", "D"} {
|
||||
assert.True(t, got[f.sha[letter]], "commit %s should have been scanned; got %v", letter, got)
|
||||
}
|
||||
for _, letter := range []string{"A", "B", "C", "X"} {
|
||||
assert.False(t, got[f.sha[letter]], "commit %s is outside C..HEAD and must not be scanned", letter)
|
||||
}
|
||||
assert.NotContains(t, data, decoySecret, "a commit on an unrelated branch leaked into the scan")
|
||||
})
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestScanRepo_SwappedRangeEnds pins the behavior when the base is a descendant
|
||||
// of the head, e.g. a CI job that passes its arguments in the wrong order. The
|
||||
// range base..head is empty, so nothing is scanned and the scan succeeds; it
|
||||
// must not widen to anything else. ScanCommits logs the empty range at V(1) so
|
||||
// the case is diagnosable without failing the pre-commit hook, whose
|
||||
// HEAD..HEAD range is empty by design.
|
||||
func TestScanRepo_SwappedRangeEnds(t *testing.T) {
|
||||
f := buildMergedBaseFixture(t, false, false)
|
||||
|
||||
// base F is a descendant of head C: git log C ^F is empty.
|
||||
got, data := scanFixtureCommits(t, f, f.sha["F"], f.sha["C"])
|
||||
|
||||
assert.Empty(t, got, "an empty range must not scan any commit; got %v", got)
|
||||
assert.NotContains(t, data, fixtureAWSKey)
|
||||
assert.NotContains(t, data, fixtureGitHubToken)
|
||||
}
|
||||
|
||||
// TestScanRepo_BaseNotUsable pins the behavior when a diff scan is given a base
|
||||
// it cannot turn into a commit in the repository. Every case must fail the scan:
|
||||
// the tempting alternative, degrading to a full-history scan, reports success
|
||||
@@ -2042,9 +2088,13 @@ func TestScanRepo_BaseNotUsable(t *testing.T) {
|
||||
// the commit as the range boundary (bfsCommitIterator.Next), so a base sitting
|
||||
// on the graft has no resolvable merge base.
|
||||
//
|
||||
// Both behaviors below predate the base..head range change; normalizeConfig is
|
||||
// not part of it. See https://github.com/trufflesecurity/trufflehog/issues/4895
|
||||
// for the same failure surfacing on GitLab.
|
||||
// That walk only runs when both ends are supplied. With a base alone the
|
||||
// parser defaults the head to HEAD and git computes the range, so the same
|
||||
// clone scans cleanly; this is the shape `--since-commit X` without `--branch`
|
||||
// takes against a GitHub URL, where prepareRepoSinceCommit clones with
|
||||
// --shallow-since and puts X on the graft. See
|
||||
// https://github.com/trufflesecurity/trufflehog/issues/4895 for the both-ends
|
||||
// failure surfacing on GitLab.
|
||||
func TestScanRepo_ShallowClone(t *testing.T) {
|
||||
// A fixture deep enough that a clone can keep the base and still cut the
|
||||
// history off somewhere above it.
|
||||
@@ -2061,40 +2111,73 @@ func TestScanRepo_ShallowClone(t *testing.T) {
|
||||
runGit(t, "", "clone", "-q", "--depth", depth, "file://"+origin, path)
|
||||
return path
|
||||
}
|
||||
|
||||
t.Run("base is the graft boundary", func(t *testing.T) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||
defer cancel()
|
||||
|
||||
// Depth 2 keeps the tip and the base; the base's parent is cut off,
|
||||
// which is the shape --shallow-since produces for its own base commit.
|
||||
shallow := clone(t, newOrigin(t), "2")
|
||||
head, base := gitRevParse(t, shallow, "HEAD"), gitRevParse(t, shallow, "HEAD~1")
|
||||
// Depth 2 keeps the tip and the base; the base's parent is cut off, which
|
||||
// is the shape --shallow-since produces for its own base commit.
|
||||
graftAtBase := func(t *testing.T) (shallow, head, base string) {
|
||||
shallow = clone(t, newOrigin(t), "2")
|
||||
head, base = gitRevParse(t, shallow, "HEAD"), gitRevParse(t, shallow, "HEAD~1")
|
||||
assert.False(t, gitHasObject(t, shallow, base+"^"), "fixture is wrong: the base's parent is present")
|
||||
return shallow, head, base
|
||||
}
|
||||
|
||||
got, _, err := scanRepoRange(ctx, t, shallow, base, head)
|
||||
// Both parser strategies build their `git log` from the same args, so both
|
||||
// must agree.
|
||||
for _, lowMemory := range []bool{false, true} {
|
||||
mode := "default"
|
||||
if lowMemory {
|
||||
mode = "low-memory"
|
||||
}
|
||||
t.Run(mode, func(t *testing.T) {
|
||||
feature.UseGitLowMemoryScan.Store(lowMemory)
|
||||
t.Cleanup(func() { feature.UseGitLowMemoryScan.Store(false) })
|
||||
|
||||
assert.ErrorContains(t, err, "unable to resolve merge base")
|
||||
assert.Empty(t, got, "an unresolvable merge base must fail the scan, not scan an arbitrary range")
|
||||
})
|
||||
t.Run("base is the graft boundary", func(t *testing.T) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||
defer cancel()
|
||||
|
||||
t.Run("base is above the graft boundary", func(t *testing.T) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||
defer cancel()
|
||||
shallow, head, base := graftAtBase(t)
|
||||
|
||||
// One more commit of depth is all it takes: the base's parent is present,
|
||||
// so the merge base resolves and git scans the range over a history it
|
||||
// cannot fully walk.
|
||||
shallow := clone(t, newOrigin(t), "3")
|
||||
head, base := gitRevParse(t, shallow, "HEAD"), gitRevParse(t, shallow, "HEAD~1")
|
||||
assert.True(t, gitHasObject(t, shallow, base+"^"), "fixture is wrong: the base's parent is missing")
|
||||
got, _, err := scanRepoRange(ctx, t, shallow, base, head)
|
||||
|
||||
got, _, err := scanRepoRange(ctx, t, shallow, base, head)
|
||||
assert.ErrorContains(t, err, "unable to resolve merge base")
|
||||
assert.Empty(t, got, "an unresolvable merge base must fail the scan, not scan an arbitrary range")
|
||||
})
|
||||
|
||||
assert.NoError(t, err)
|
||||
assert.True(t, got[head], "the one commit in base..head should have been scanned; got %v", got)
|
||||
assert.False(t, got[base], "the base is excluded from its own range")
|
||||
})
|
||||
t.Run("base is the graft boundary with implicit head", func(t *testing.T) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||
defer cancel()
|
||||
|
||||
// Same clone, no head: normalizeConfig must not resolve HEAD and
|
||||
// run MergeBase, or the --shallow-since path would fail exactly
|
||||
// where it used to work.
|
||||
shallow, head, base := graftAtBase(t)
|
||||
|
||||
got, _, err := scanRepoRange(ctx, t, shallow, base, "")
|
||||
|
||||
assert.NoError(t, err)
|
||||
assert.True(t, got[head], "the one commit in base..HEAD should have been scanned; got %v", got)
|
||||
assert.False(t, got[base], "the base is excluded from its own range")
|
||||
})
|
||||
|
||||
t.Run("base is above the graft boundary", func(t *testing.T) {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||
defer cancel()
|
||||
|
||||
// One more commit of depth is all it takes: the base's parent is
|
||||
// present, so the merge base resolves and git scans the range over
|
||||
// a history it cannot fully walk.
|
||||
shallow := clone(t, newOrigin(t), "3")
|
||||
head, base := gitRevParse(t, shallow, "HEAD"), gitRevParse(t, shallow, "HEAD~1")
|
||||
assert.True(t, gitHasObject(t, shallow, base+"^"), "fixture is wrong: the base's parent is missing")
|
||||
|
||||
got, _, err := scanRepoRange(ctx, t, shallow, base, head)
|
||||
|
||||
assert.NoError(t, err)
|
||||
assert.True(t, got[head], "the one commit in base..head should have been scanned; got %v", got)
|
||||
assert.False(t, got[base], "the base is excluded from its own range")
|
||||
})
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// runGit runs a git command, failing the test on error. An empty repoPath runs
|
||||
|
||||
Reference in New Issue
Block a user