🌱 Ensure Token-Permission and Branch-Protection probes use exported v…

…alue keys (#3977) * use exported value keys for token permissions Signed-off-by: Spencer Schrock <sschrock@google.com> * convert required reviewer count to use exported value key Signed-off-by: Spencer Schrock <sschrock@google.com> --------- Signed-off-by: Spencer Schrock <sschrock@google.com>
ossf · Apr 10, 2024 · 99a6dc4 · 99a6dc4
1 parent 6b071ed
commit 99a6dc4
Show file tree

Hide file tree

Showing 4 changed files with 76 additions and 23 deletions.
diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -167,16 +167,20 @@ func BranchProtection(name string,
 			branchScores[branchName].maxes.adminThoroughReview += max
 
 		case requiresApproversForPullRequests.Probe:
+			noOfRequiredReviewers, err := getReviewerCount(f)
+			if err != nil {
+				e := sce.WithMessage(sce.ErrScorecardInternal, "unable to get reviewer count")
+				return checker.CreateRuntimeErrorResult(name, e)
+			}
 			// Scorecard evaluation scores twice with this probe:
 			// Once if the count is above 0
 			// Once if the count is above 2
-			score, max = nonAdminThoroughReviewProtection(f, doLogging, dl)
+			score, max = logReviewerCount(f, doLogging, dl, noOfRequiredReviewers)
 			branchScores[branchName].scores.thoroughReview += score
 			branchScores[branchName].maxes.thoroughReview += max
 
 			reviewerWeight := 2
 			max = reviewerWeight
-			noOfRequiredReviewers, _ := strconv.Atoi(f.Values["numberOfRequiredReviewers"]) //nolint:errcheck
 			if f.Outcome == finding.OutcomeTrue && noOfRequiredReviewers > 0 {
 				branchScores[branchName].scores.review += reviewerWeight
 			}
@@ -235,6 +239,22 @@ func getBranchName(f *finding.Finding) (string, error) {
 	return name, nil
 }
 
+func getReviewerCount(f *finding.Finding) (int, error) {
+	// assume no review required if data not available
+	if f.Outcome == finding.OutcomeNotAvailable {
+		return 0, nil
+	}
+	countString, ok := f.Values[requiresApproversForPullRequests.RequiredReviewersKey]
+	if !ok {
+		return 0, sce.WithMessage(sce.ErrScorecardInternal, "no required reviewer count found")
+	}
+	count, err := strconv.Atoi(countString)
+	if err != nil {
+		return 0, sce.WithMessage(sce.ErrScorecardInternal, "unable to parse required reviewer count")
+	}
+	return count, nil
+}
+
 func sumUpScoreForTier(t tier, scoresData []levelScore) int {
 	sum := 0
 	for i := range scoresData {
@@ -427,10 +447,9 @@ func adminThoroughReviewProtection(f *finding.Finding, doLogging bool, dl checke
 	return score, max
 }
 
-func nonAdminThoroughReviewProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) {
+func logReviewerCount(f *finding.Finding, doLogging bool, dl checker.DetailLogger, noOfRequiredReviews int) (int, int) {
 	var score, max int
 	if f.Outcome == finding.OutcomeTrue {
-		noOfRequiredReviews, _ := strconv.Atoi(f.Values["numberOfRequiredReviewers"]) //nolint:errcheck
 		if noOfRequiredReviews >= minReviews {
 			info(dl, doLogging, f.Message)
 			score++

diff --git a/checks/evaluation/permissions.go b/checks/evaluation/permissions.go
@@ -25,10 +25,6 @@ import (
 	"github.com/ossf/scorecard/v4/probes/topLevelPermissions"
 )
 
-func isWriteAll(f *finding.Finding) bool {
-	return (f.Values["tokenName"] == "all" || f.Values["tokenName"] == "write-all")
-}
-
 // TokenPermissions applies the score policy for the Token-Permissions check.
 //
 //nolint:gocognit
@@ -75,15 +71,15 @@ func TokenPermissions(name string,
 		f := &findings[i]
 
 		// Log workflows with "none" permissions
-		if f.Values["permissionLevel"] == string(checker.PermissionLevelNone) {
+		if permissionLevel(f) == checker.PermissionLevelNone {
 			dl.Info(&checker.LogMessage{
 				Finding: f,
 			})
 			continue
 		}
 
 		// Log workflows with "read" permissions
-		if f.Values["permissionLevel"] == string(checker.PermissionLevelRead) {
+		if permissionLevel(f) == checker.PermissionLevelRead {
 			dl.Info(&checker.LogMessage{
 				Finding: f,
 			})
@@ -108,7 +104,7 @@ func TokenPermissions(name string,
 
 		addProbeToMaps(fPath, undeclaredPermissions, hasWritePermissions)
 
-		if f.Values["permissionLevel"] == string(checker.PermissionLevelUndeclared) {
+		if permissionLevel(f) == checker.PermissionLevelUndeclared {
 			score = updateScoreAndMapFromUndeclared(undeclaredPermissions,
 				hasWritePermissions, f, score, dl)
 			continue
@@ -120,7 +116,7 @@ func TokenPermissions(name string,
 				Finding: f,
 			})
 		case topLevelPermissions.Probe:
-			if f.Values["permissionLevel"] != string(checker.PermissionLevelWrite) {
+			if permissionLevel(f) != checker.PermissionLevelWrite {
 				continue
 			}
 			hasWritePermissions["topLevel"][fPath] = true
@@ -141,7 +137,7 @@ func TokenPermissions(name string,
 			}
 			score -= 0.5
 		case jobLevelPermissions.Probe:
-			if f.Values["permissionLevel"] != string(checker.PermissionLevelWrite) {
+			if permissionLevel(f) != checker.PermissionLevelWrite {
 				continue
 			}
 
@@ -221,7 +217,7 @@ func updateScoreFromUndeclaredTop(undeclaredPermissions map[string]map[string]bo
 }
 
 func isBothUndeclaredAndNotAvailableOrNotApplicable(f *finding.Finding, dl checker.DetailLogger) bool {
-	if f.Values["permissionLevel"] == string(checker.PermissionLevelUndeclared) {
+	if permissionLevel(f) == checker.PermissionLevelUndeclared {
 		if f.Outcome == finding.OutcomeNotAvailable {
 			return true
 		} else if f.Outcome == finding.OutcomeNotApplicable {
@@ -278,11 +274,10 @@ func addProbeToMaps(fPath string, hasWritePermissions, undeclaredPermissions map
 }
 
 func reduceBy(f *finding.Finding, dl checker.DetailLogger) float32 {
-	if f.Values["permissionLevel"] != string(checker.PermissionLevelWrite) {
+	if permissionLevel(f) != checker.PermissionLevelWrite {
 		return 0
 	}
-	tokenName := f.Values["tokenName"]
-	switch tokenName {
+	switch tokenName(f) {
 	case "checks", "statuses":
 		dl.Warn(&checker.LogMessage{
 			Finding: f,
@@ -301,3 +296,34 @@ func reduceBy(f *finding.Finding, dl checker.DetailLogger) float32 {
 	}
 	return 0
 }
+
+func isWriteAll(f *finding.Finding) bool {
+	token := tokenName(f)
+	return (token == "all" || token == "write-all")
+}
+
+func permissionLevel(f *finding.Finding) checker.PermissionLevel {
+	var key string
+	// these values should be the same, but better safe than sorry
+	switch f.Probe {
+	case jobLevelPermissions.Probe:
+		key = jobLevelPermissions.PermissionLevelKey
+	case topLevelPermissions.Probe:
+		key = topLevelPermissions.PermissionLevelKey
+	default:
+	}
+	return checker.PermissionLevel(f.Values[key])
+}
+
+func tokenName(f *finding.Finding) string {
+	var key string
+	// these values should be the same, but better safe than sorry
+	switch f.Probe {
+	case jobLevelPermissions.Probe:
+		key = jobLevelPermissions.TokenNameKey
+	case topLevelPermissions.Probe:
+		key = topLevelPermissions.TokenNameKey
+	default:
+	}
+	return f.Values[key]
+}
diff --git a/probes/jobLevelPermissions/impl.go b/probes/jobLevelPermissions/impl.go
@@ -28,7 +28,11 @@ import (
 //go:embed *.yml
 var fs embed.FS
 
-const Probe = "jobLevelPermissions"
+const (
+	Probe              = "jobLevelPermissions"
+	PermissionLevelKey = "permissionLevel"
+	TokenNameKey       = "tokenName"
+)
 
 func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	if raw == nil {
@@ -91,8 +95,8 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 		if err != nil {
 			return nil, Probe, fmt.Errorf("create finding: %w", err)
 		}
-		f = f.WithValue("permissionLevel", string(r.Type))
-		f = f.WithValue("tokenName", *r.Name)
+		f = f.WithValue(PermissionLevelKey, string(r.Type))
+		f = f.WithValue(TokenNameKey, *r.Name)
 		findings = append(findings, *f)
 	}
 

diff --git a/probes/topLevelPermissions/impl.go b/probes/topLevelPermissions/impl.go
@@ -28,7 +28,11 @@ import (
 //go:embed *.yml
 var fs embed.FS
 
-const Probe = "topLevelPermissions"
+const (
+	Probe              = "topLevelPermissions"
+	PermissionLevelKey = "permissionLevel"
+	TokenNameKey       = "tokenName"
+)
 
 func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	if raw == nil {
@@ -100,8 +104,8 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 		if err != nil {
 			return nil, Probe, fmt.Errorf("create finding: %w", err)
 		}
-		f = f.WithValue("permissionLevel", string(r.Type))
-		f = f.WithValue("tokenName", tokenName)
+		f = f.WithValue(PermissionLevelKey, string(r.Type))
+		f = f.WithValue(TokenNameKey, tokenName)
 		findings = append(findings, *f)
 	}