Merge branch 'main' into no-commits-bug

kgangerlm · Dec 19, 2023 · 8164524 · 8164524
2 parents 28288ae + 2ef20f1
commit 8164524
Show file tree

Hide file tree

Showing 39 changed files with 1,832 additions and 345 deletions.
diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
@@ -40,7 +40,7 @@ jobs:
         fetch-depth: 2 # needed to diff changed files
     - id: files
       name: Get changed files
-      uses: tj-actions/changed-files@1c938490c880156b746568a518594309cfb3f66b #v40.2.1
+      uses: tj-actions/changed-files@94549999469dbfa032becf298d95c87a14c34394 #v40.2.2
       with:
         files_ignore: '**.md'
     - id: docs_only_check

diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml
@@ -31,7 +31,7 @@ jobs:
       with:
         egress-policy: audit # TODO: change to 'egress-policy: block' after couple of runs
 
-    - uses: actions/stale@1160a2240286f5da8ec72b1c0816ce2481aabf84 # v3.0.18
+    - uses: actions/stale@28ca1036281a5e5922ead5184a1bbf96e5fc984e # v3.0.18
       with:
         repo-token: ${{ secrets.GITHUB_TOKEN }}
         stale-issue-message: 'This issue is stale because it has been open for 60 days with no activity.'

diff --git a/checker/raw_result.go b/checker/raw_result.go
@@ -257,6 +257,8 @@ const (
 	CodeQLWorkflow SASTWorkflowType = "CodeQL"
 	// SonarWorkflow represents a workflow that runs Sonar.
 	SonarWorkflow SASTWorkflowType = "Sonar"
+	// SnykWorkflow represents a workflow that runs Snyk.
+	SnykWorkflow SASTWorkflowType = "Snyk"
 )
 
 // SASTWorkflow represents a SAST workflow.

diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -93,7 +93,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -112,7 +113,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -151,7 +153,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -186,7 +189,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -207,7 +211,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -242,7 +247,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -263,7 +269,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -299,7 +306,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -337,7 +345,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+							Required:                     &trueVal,
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -285,9 +285,7 @@ func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) {
 	// Having at least 1 reviewer is twice as important as the other Tier 2 requirements.
 	const reviewerWeight = 2
 	max += reviewerWeight
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil &&
-		*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 {
+	if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) > 0 {
 		// We do not display anything here, it's done in nonAdminThoroughReviewProtection()
 		score += reviewerWeight
 	}
@@ -329,15 +327,13 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
 	}
 
 	max++
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil {
+	if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.Required) {
 		score++
 		info(dl, log, "PRs are required in order to make changes on branch '%s'", *branch.Name)
 	} else {
 		warn(dl, log, "PRs are not required to make changes on branch '%s'; or we don't have data to detect it."+
 			"If you think it might be the latter, make sure to run Scorecard with a PAT or use Repo "+
 			"Rules (that are always public) instead of Branch Protection settings", *branch.Name)
-		// Warning it here because since `RequiredPullRequestReviews` is nil, we won't check reviewers count afterwards
-		warn(dl, log, "No reviewers required to merge changes on branch '%s'", *branch.Name)
 	}
 
 	return score, max
@@ -349,8 +345,7 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL
 	// Only log information if the branch is protected.
 	log := branch.Protected != nil && *branch.Protected
 
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
+	if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
 		// Note: we don't increase max possible score for non-admin viewers.
 		max++
 		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
@@ -391,18 +386,13 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
 
 	max++
 
-	// On this first check we exclude the case where PRs are not required to make changes,
-	// because it's already covered on adminReviewProtection function.
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
-		if *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews {
-			info(dl, log, "number of required reviewers is %d on branch '%s'",
-				*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name)
-			score++
-		} else {
-			warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d",
-				*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name, minReviews)
-		}
+	reviewers := valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount)
+	if reviewers >= minReviews {
+		info(dl, log, "number of required reviewers is %d on branch '%s'", reviewers, *branch.Name)
+		score++
+	} else {
+		warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d",
+			reviewers, *branch.Name, minReviews)
 	}
 
 	return score, max
@@ -416,8 +406,7 @@ func codeownerBranchProtection(
 
 	log := branch.Protected != nil && *branch.Protected
 
-	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
-		branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
+	if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
 		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
 		case true:
 			info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)
@@ -433,3 +422,12 @@ func codeownerBranchProtection(
 
 	return score, max
 }
+
+// returns the pointer's value if it exists, the type's zero-value otherwise.
+func valueOrZero[T any](ptr *T) T {
+	if ptr == nil {
+		var zero T
+		return zero
+	}
+	return *ptr
+}
diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -50,7 +50,7 @@ func TestIsBranchProtected(t *testing.T) {
 		expected        scut.TestReturn
 	}{
 		{
-			name: "Configs as they are right after creating new Branch Protection setting",
+			name: "GitHub default settings",
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         3,
@@ -62,12 +62,14 @@ func TestIsBranchProtected(t *testing.T) {
 				Name:      &branchVal,
 				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
-					AllowDeletions:             &falseVal,
-					AllowForcePushes:           &falseVal,
-					RequireLinearHistory:       &falseVal,
-					EnforceAdmins:              &falseVal,
-					RequireLastPushApproval:    &falseVal,
-					RequiredPullRequestReviews: nil,
+					AllowDeletions:          &falseVal,
+					AllowForcePushes:        &falseVal,
+					RequireLinearHistory:    &falseVal,
+					EnforceAdmins:           &falseVal,
+					RequireLastPushApproval: &falseVal,
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required: &falseVal,
+					},
 					CheckRules: clients.StatusChecksRule{
 						RequiresStatusChecks: &trueVal,
 						Contexts:             nil,
@@ -103,7 +105,8 @@ func TestIsBranchProtected(t *testing.T) {
 				Name:      &branchVal,
 				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -139,7 +142,8 @@ func TestIsBranchProtected(t *testing.T) {
 					RequireLinearHistory:    &falseVal,
 					AllowForcePushes:        &falseVal,
 					AllowDeletions:          &falseVal,
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -175,7 +179,9 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: nil,
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required: &falseVal,
+					},
 				},
 			},
 		},
@@ -202,7 +208,9 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: nil,
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required: &falseVal,
+					},
 				},
 			},
 		},
@@ -229,7 +237,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -260,7 +269,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -291,7 +301,6 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  nil,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: nil,
 				},
 			},
 		},
@@ -318,7 +327,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  nil,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          nil,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -349,7 +359,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -380,7 +391,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -412,7 +424,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -443,7 +456,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -474,7 +488,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -505,7 +520,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -537,7 +553,8 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						Required:                     &trueVal,
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,