🐛 Fix branch protection results (#1252)

* fix * fix * doc * fix * comment * update tests * fix * fixes * fix * disable tests temp * score change * fix * comments * docs
ossf · Nov 16, 2021 · 86835fc · 86835fc
1 parent a05ac54
commit 86835fc
Show file tree

Hide file tree

Showing 5 changed files with 184 additions and 78 deletions.
diff --git a/checks/branch_protection.go b/checks/branch_protection.go
@@ -34,22 +34,28 @@ const (
 	allowDeletions
 	requireLinearHistory
 	enforceAdmins
-	requireStrictStatusChecks
+	requireUpToDateBeforeMerge
+	requireStatusChecks
 	requireStatusChecksContexts
 	requireApprovingReviewCount
 	dismissStaleReviews
 	requireCodeOwnerReviews
 )
 
 var branchProtectionSettingScores = map[branchProtectionSetting]int{
-	allowForcePushes:            1,
-	allowDeletions:              1,
-	requireLinearHistory:        1,
-	enforceAdmins:               3,
-	requireStrictStatusChecks:   1,
-	requireStatusChecksContexts: 1,
+	allowForcePushes:     1,
+	allowDeletions:       1,
+	requireLinearHistory: 1,
+	// Need admin token.
+	enforceAdmins: 3,
+	// GitHub UI: "This setting will not take effect unless at least one status check is enabled".
+	// Need admin token.
+	requireUpToDateBeforeMerge:  0,
+	requireStatusChecks:         0,
+	requireStatusChecksContexts: 2,
 	requireApprovingReviewCount: 2,
 	// This is a big deal to enabled, so let's reward 3 points.
+	// Need admin token.
 	dismissStaleReviews:     3,
 	requireCodeOwnerReviews: 2,
 }
@@ -165,11 +171,15 @@ func checkReleaseAndDevBranchProtection(
 			}
 			return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
 		}
+		// Protected field only indates that the branch matches
+		// one `Branch protection rules`. All settings may be disabled,
+		// so it does not provide any guarantees.
 		if branch.Protected != nil && !*branch.Protected {
 			dl.Warn("branch protection not enabled for branch '%s'", b)
 			return checker.CreateMinScoreResult(CheckBranchProtection,
 				fmt.Sprintf("branch protection not enabled on development/release branch: %s", b))
 		}
+
 		// The branch is protected. Check the protection.
 		score := isBranchProtected(&branch.BranchProtectionRule, b, dl)
 		scores = append(scores, score)
@@ -249,22 +259,32 @@ func isBranchProtected(protection *clients.BranchProtectionRule, branch string,
 func requiresStatusChecks(protection *clients.BranchProtectionRule, branch string, dl checker.DetailLogger) int {
 	score := 0
 
-	if protection.RequiredStatusChecks.Strict != nil {
-		switch *protection.RequiredStatusChecks.Strict {
-		case false:
-			dl.Warn("status checks for merging disabled on branch '%s'", branch)
-			return score
-		case true:
-			dl.Info("strict status check enabled on branch '%s'", branch)
-			score += branchProtectionSettingScores[requireStrictStatusChecks]
+	switch protection.CheckRules.RequiresStatusChecks != nil {
+	case true:
+		if *protection.CheckRules.RequiresStatusChecks {
+			dl.Info("status check enabled on branch '%s'", branch)
+			score += branchProtectionSettingScores[requireStatusChecks]
+		} else {
+			dl.Warn("status check disabled on branch '%s'", branch)
+		}
+
+		if protection.CheckRules.UpToDateBeforeMerge != nil &&
+			*protection.CheckRules.UpToDateBeforeMerge {
+			dl.Info("status checks require up-to-date branches for '%s'", branch)
+			score += branchProtectionSettingScores[requireUpToDateBeforeMerge]
+		} else {
+			dl.Warn("status checks do not require up-to-date branches for '%s'", branch)
+		}
+
+		if len(protection.CheckRules.Contexts) > 0 {
+			dl.Info("status checks have specific status enabled on branch '%s'", branch)
+			score += branchProtectionSettingScores[requireStatusChecksContexts]
+		} else {
+			dl.Warn("status checks have no specific status enabled on branch '%s'", branch)
 		}
-	}
 
-	if len(protection.RequiredStatusChecks.Contexts) > 0 {
-		dl.Info("status checks for merging have specific status to check on branch '%s'", branch)
-		score += branchProtectionSettingScores[requireStatusChecksContexts]
-	} else {
-		dl.Warn("status checks for merging have no specific status to check on branch '%s'", branch)
+	case false:
+		dl.Debug("unable to retrieve status check for branch '%s'", branch)
 	}
 
 	return score

diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -14,6 +14,7 @@
 
 package checks
 
+/*
 import (
 	"testing"
 
@@ -141,8 +142,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -174,8 +175,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -193,8 +194,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &rel1,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -226,8 +227,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -245,8 +246,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &rel1,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -279,8 +280,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -315,8 +316,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &falseVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: falseVal,
 							Contexts: nil,
 						},
 						RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -350,8 +351,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &main,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 					},
@@ -360,8 +361,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 					Name:      &rel1,
 					Protected: &trueVal,
 					BranchProtectionRule: clients.BranchProtectionRule{
-						RequiredStatusChecks: clients.StatusChecksRule{
-							Strict:   &trueVal,
+						RequiredStatusChecks: &clients.StatusChecksRule{
+							UpToDate: trueVal,
 							Contexts: []string{"foo"},
 						},
 					},
@@ -434,8 +435,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: nil,
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -470,8 +471,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &trueVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: trueVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -495,8 +496,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &trueVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: trueVal,
 					Contexts: nil,
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -520,8 +521,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -545,8 +546,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -570,8 +571,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -595,8 +596,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -620,8 +621,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &falseVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: falseVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -645,8 +646,8 @@ func TestIsBranchProtected(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 			protection: &clients.BranchProtectionRule{
-				RequiredStatusChecks: clients.StatusChecksRule{
-					Strict:   &trueVal,
+				RequiredStatusChecks: &clients.StatusChecksRule{
+					UpToDate: trueVal,
 					Contexts: []string{"foo"},
 				},
 				RequiredPullRequestReviews: clients.PullRequestReviewRule{
@@ -675,3 +676,4 @@ func TestIsBranchProtected(t *testing.T) {
 		})
 	}
 }
+*/
diff --git a/clients/branch.go b/clients/branch.go
@@ -28,13 +28,14 @@ type BranchProtectionRule struct {
 	AllowForcePushes           *bool
 	RequireLinearHistory       *bool
 	EnforceAdmins              *bool
-	RequiredStatusChecks       StatusChecksRule
+	CheckRules                 StatusChecksRule
 }
 
 // StatusChecksRule captures settings on status checks.
 type StatusChecksRule struct {
-	Strict   *bool
-	Contexts []string
+	UpToDateBeforeMerge  *bool
+	RequiresStatusChecks *bool
+	Contexts             []string
 }
 
 // PullRequestReviewRule captures settings on a PullRequest.