Only pull required branch names (#1965)

Co-authored-by: Azeem Shaikh <[email protected]>

checks/branch_protection_test.go

@@ -33,11 +33,14 @@ func getBranchName(branch *clients.BranchRef) string {
 	return *branch.Name
 }
 
-func getBranch(branches []*clients.BranchRef, name string) *clients.BranchRef {
+func getBranch(branches []*clients.BranchRef, name string, isNonAdmin bool) *clients.BranchRef {
 	for _, branch := range branches {
 		branchName := getBranchName(branch)
 		if branchName == name {
-			return branch
+			if !isNonAdmin {
+				return branch
+			}
+			return scrubBranch(branch)
 		}
 	}
 	return nil
@@ -49,14 +52,6 @@ func scrubBranch(branch *clients.BranchRef) *clients.BranchRef {
 	return ret
 }
 
-func scrubBranches(branches []*clients.BranchRef) []*clients.BranchRef {
-	ret := make([]*clients.BranchRef, len(branches))
-	for i, branch := range branches {
-		ret[i] = scrubBranch(branch)
-	}
-	return ret
-}
-
 func TestReleaseAndDevBranchProtected(t *testing.T) {
 	t.Parallel()
 
@@ -399,10 +394,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
 			mockRepoClient.EXPECT().GetDefaultBranch().
 				DoAndReturn(func() (*clients.BranchRef, error) {
-					defaultBranch := getBranch(tt.branches, tt.defaultBranch)
-					if defaultBranch != nil && tt.nonadmin {
-						return scrubBranch(defaultBranch), nil
-					}
+					defaultBranch := getBranch(tt.branches, tt.defaultBranch, tt.nonadmin)
 					return defaultBranch, nil
 				}).AnyTimes()
 			mockRepoClient.EXPECT().ListReleases().
@@ -415,12 +407,9 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					}
 					return ret, nil
 				}).AnyTimes()
-			mockRepoClient.EXPECT().ListBranches().
-				DoAndReturn(func() ([]*clients.BranchRef, error) {
-					if tt.nonadmin {
-						return scrubBranches(tt.branches), nil
-					}
-					return tt.branches, nil
+			mockRepoClient.EXPECT().GetBranch(gomock.Any()).
+				DoAndReturn(func(b string) (*clients.BranchRef, error) {
+					return getBranch(tt.branches, b, tt.nonadmin), nil
 				}).AnyTimes()
 			dl := scut.TestDetailLogger{}
 			req := checker.CheckRequest{

checks/evaluation/security_policy.go

@@ -40,7 +40,6 @@ func SecurityPolicy(name string, dl checker.DetailLogger, r *checker.SecurityPol
 		}
 		if msg.Type == checker.FileTypeURL {
 			msg.Text = "security policy detected in org repo"
-
 		} else {
 			msg.Text = "security policy detected in current repo"
 		}

checks/raw/branch_protection.go

@@ -15,37 +15,56 @@
 package raw
 
 import (
-	"errors"
 	"fmt"
 	"regexp"
 
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/clients"
-	sce "github.com/ossf/scorecard/v4/errors"
 )
 
 const master = "master"
 
-type branchMap map[string]*clients.BranchRef
+var commit = regexp.MustCompile("^[a-f0-9]{40}$")
+
+type branchSet struct {
+	exists map[string]bool
+	set    []clients.BranchRef
+}
+
+func (set *branchSet) add(branch *clients.BranchRef) bool {
+	if branch != nil &&
+		branch.Name != nil &&
+		*branch.Name != "" &&
+		!set.exists[*branch.Name] {
+		set.set = append(set.set, *branch)
+		set.exists[*branch.Name] = true
+		return true
+	}
+	return false
+}
+
+func (set branchSet) contains(branch string) bool {
+	_, contains := set.exists[branch]
+	return contains
+}
 
 // BranchProtection retrieves the raw data for the Branch-Protection check.
 func BranchProtection(c clients.RepoClient) (checker.BranchProtectionsData, error) {
-	// Checks branch protection on both release and development branch.
-	// Get all branches. This will include information on whether they are protected.
-	branches, err := c.ListBranches()
+	branches := branchSet{
+		exists: make(map[string]bool),
+	}
+	// Add default branch.
+	defaultBranch, err := c.GetDefaultBranch()
 	if err != nil {
 		return checker.BranchProtectionsData{}, fmt.Errorf("%w", err)
 	}
-	branchesMap := getBranchMapFrom(branches)
+	branches.add(defaultBranch)
 
 	// Get release branches.
 	releases, err := c.ListReleases()
 	if err != nil {
 		return checker.BranchProtectionsData{}, fmt.Errorf("%w", err)
 	}
-
-	commit := regexp.MustCompile("^[a-f0-9]{40}$")
-	checkBranches := make(map[string]bool)
 	for _, release := range releases {
 		if release.TargetCommitish == "" {
 			// Log with a named error if target_commitish is nil.
@@ -57,78 +76,47 @@ func BranchProtection(c clients.RepoClient) (checker.BranchProtectionsData, erro
 			continue
 		}
 
-		// Try to resolve the branch name.
-		b, err := branchesMap.getBranchByName(release.TargetCommitish)
-		if err != nil {
-			// If the commitish branch is still not found, fail.
-			return checker.BranchProtectionsData{}, err
+		if branches.contains(release.TargetCommitish) ||
+			branches.contains(branchRedirect(release.TargetCommitish)) {
+			continue
 		}
 
-		// Branch is valid, add to list of branches to check.
-		checkBranches[*b.Name] = true
-	}
-
-	// Add default branch.
-	defaultBranch, err := c.GetDefaultBranch()
-	if err != nil {
-		return checker.BranchProtectionsData{}, fmt.Errorf("%w", err)
-	}
-	defaultBranchName := getBranchName(defaultBranch)
-	if defaultBranchName != "" {
-		checkBranches[defaultBranchName] = true
-	}
-
-	rawData := checker.BranchProtectionsData{}
-	// Check protections on all the branches.
-	for b := range checkBranches {
-		branch, err := branchesMap.getBranchByName(b)
+		// Get the associated release branch.
+		branchRef, err := c.GetBranch(release.TargetCommitish)
 		if err != nil {
-			if errors.Is(err, errInternalBranchNotFound) {
-				continue
-			}
-			return checker.BranchProtectionsData{}, err
+			return checker.BranchProtectionsData{},
+				fmt.Errorf("error during GetBranch(%s): %w", release.TargetCommitish, err)
+		}
+		if branches.add(branchRef) {
+			continue
 		}
 
-		rawData.Branches = append(rawData.Branches, *branch)
+		// Couldn't find the branch check for redirects.
+		redirectBranch := branchRedirect(release.TargetCommitish)
+		if redirectBranch == "" {
+			continue
+		}
+		branchRef, err = c.GetBranch(redirectBranch)
+		if err != nil {
+			return checker.BranchProtectionsData{},
+				fmt.Errorf("error during GetBranch(%s) %w", redirectBranch, err)
+		}
+		branches.add(branchRef)
+		// Branch doesn't exist or was deleted. Continue.
 	}
 
 	// No error, return the data.
-	return rawData, nil
+	return checker.BranchProtectionsData{
+		Branches: branches.set,
+	}, nil
 }
 
-func (b branchMap) getBranchByName(name string) (*clients.BranchRef, error) {
-	val, exists := b[name]
-	if exists {
-		return val, nil
-	}
-
+func branchRedirect(name string) string {
 	// Ideally, we should check using repositories.GetBranch if there was a branch redirect.
 	// See https://github.com/google/go-github/issues/1895
 	// For now, handle the common master -> main redirect.
 	if name == master {
-		val, exists := b["main"]
-		if exists {
-			return val, nil
-		}
-	}
-	return nil, sce.WithMessage(sce.ErrScorecardInternal,
-		fmt.Sprintf("could not find branch name %s: %v", name, errInternalBranchNotFound))
-}
-
-func getBranchMapFrom(branches []*clients.BranchRef) branchMap {
-	ret := make(branchMap)
-	for _, branch := range branches {
-		branchName := getBranchName(branch)
-		if branchName != "" {
-			ret[branchName] = branch
-		}
-	}
-	return ret
-}
-
-func getBranchName(branch *clients.BranchRef) string {
-	if branch == nil || branch.Name == nil {
-		return ""
+		return "main"
 	}
-	return *branch.Name
+	return ""
 }