diff --git a/checks/branch_protection.go b/checks/branch_protection.go index bb48a863396..33f6d60984d 100644 --- a/checks/branch_protection.go +++ b/checks/branch_protection.go @@ -19,6 +19,8 @@ import ( "github.com/ossf/scorecard/v4/checks/evaluation" "github.com/ossf/scorecard/v4/checks/raw" sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/probes" + "github.com/ossf/scorecard/v4/probes/zrunner" ) // CheckBranchProtection is the exported name for Branch-Protected check. @@ -34,17 +36,23 @@ func init() { // BranchProtection runs the Branch-Protection check. func BranchProtection(c *checker.CheckRequest) checker.CheckResult { - rawData, err := raw.BranchProtection(c.RepoClient) + rawData, err := raw.BranchProtection(c) if err != nil { e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) return checker.CreateRuntimeErrorResult(CheckBranchProtection, e) } - // Return raw results. - if c.RawResults != nil { - c.RawResults.BranchProtectionResults = rawData + // Set the raw results. + pRawResults := getRawResults(c) + pRawResults.BranchProtectionResults = rawData + + // Evaluate the probes. + findings, err := zrunner.Run(pRawResults, probes.BranchProtection) + if err != nil { + e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, e) } // Return the score evaluation. - return evaluation.BranchProtection(CheckBranchProtection, c.Dlogger, &rawData) + return evaluation.BranchProtection(CheckBranchProtection, findings, c.Dlogger) } diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index d56a4cce2bd..d0c6f087fcd 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -174,7 +174,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Error: nil, Score: 4, NumberOfWarn: 9, - NumberOfInfo: 12, + NumberOfInfo: 11, NumberOfDebug: 0, }, defaultBranch: main, @@ -232,7 +232,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 4, - NumberOfInfo: 18, + NumberOfInfo: 16, NumberOfDebug: 0, }, defaultBranch: main, diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index f5848d521dc..4177d0061f0 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -16,10 +16,22 @@ package evaluation import ( "fmt" + "strconv" "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/clients" sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/blocksDeleteOnBranches" + "github.com/ossf/scorecard/v4/probes/blocksForcePushOnBranches" + "github.com/ossf/scorecard/v4/probes/branchProtectionAppliesToAdmins" + "github.com/ossf/scorecard/v4/probes/branchesAreProtected" + "github.com/ossf/scorecard/v4/probes/dismissesStaleReviews" + "github.com/ossf/scorecard/v4/probes/requiresApproversForPullRequests" + "github.com/ossf/scorecard/v4/probes/requiresCodeOwnersReview" + "github.com/ossf/scorecard/v4/probes/requiresLastPushApproval" + "github.com/ossf/scorecard/v4/probes/requiresPRsToChangeCode" + "github.com/ossf/scorecard/v4/probes/requiresUpToDateBranches" + "github.com/ossf/scorecard/v4/probes/runsStatusChecksBeforeMerging" ) const ( @@ -60,41 +72,143 @@ const ( ) // BranchProtection runs Branch-Protection check. -func BranchProtection(name string, dl checker.DetailLogger, - r *checker.BranchProtectionsData, +func BranchProtection(name string, + findings []finding.Finding, dl checker.DetailLogger, ) checker.CheckResult { - var scores []levelScore - - // Check protections on all the branches. - for i := range r.Branches { - var score levelScore - b := r.Branches[i] - - // Protected field only indicates that the branch matches - // one `Branch protection rules`. All settings may be disabled, - // so it does not provide any guarantees. - protected := !(b.Protected != nil && !*b.Protected) - if !protected { + expectedProbes := []string{ + blocksDeleteOnBranches.Probe, + blocksForcePushOnBranches.Probe, + branchesAreProtected.Probe, + branchProtectionAppliesToAdmins.Probe, + dismissesStaleReviews.Probe, + requiresApproversForPullRequests.Probe, + requiresCodeOwnersReview.Probe, + requiresLastPushApproval.Probe, + requiresUpToDateBranches.Probe, + runsStatusChecksBeforeMerging.Probe, + requiresPRsToChangeCode.Probe, + } + + if !finding.UniqueProbesEqual(findings, expectedProbes) { + e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results") + return checker.CreateRuntimeErrorResult(name, e) + } + + // Create a map branches and whether theyare protected + // Protected field only indates that the branch matches + // one `Branch protection rules`. All settings may be disabled, + // so it does not provide any guarantees. + protectedBranches := make(map[string]bool) + for i := range findings { + f := &findings[i] + if f.Outcome == finding.OutcomeNotApplicable { + return checker.CreateInconclusiveResult(name, + "unable to detect any development/release branches") + } + branchName, err := getBranchName(f) + if err != nil { + return checker.CreateRuntimeErrorResult(name, err) + } + // the order of this switch statement matters. + switch { + // Sanity check: + case f.Probe != branchesAreProtected.Probe: + continue + // Sanity check: + case branchName == "": + e := sce.WithMessage(sce.ErrScorecardInternal, "probe is missing branch name") + return checker.CreateRuntimeErrorResult(name, e) + // Now we can check whether the branch is protected: + case f.Outcome == finding.OutcomeNegative: + protectedBranches[branchName] = false dl.Warn(&checker.LogMessage{ - Text: fmt.Sprintf("branch protection not enabled for branch '%s'", *b.Name), + Text: fmt.Sprintf("branch protection not enabled for branch '%s'", branchName), }) + case f.Outcome == finding.OutcomePositive: + protectedBranches[branchName] = true + default: + continue } - score.scores.basic, score.maxes.basic = basicNonAdminProtection(&b, dl) - score.scores.review, score.maxes.review = nonAdminReviewProtection(&b) - score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(&b, dl) - score.scores.context, score.maxes.context = nonAdminContextProtection(&b, dl) - score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(&b, dl) - // Do we want this? - score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(&b, dl) - score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(&b, r.CodeownersFiles, dl) - - scores = append(scores, score) } - if len(scores) == 0 { + branchScores := make(map[string]*levelScore) + + for i := range findings { + f := &findings[i] + if f.Outcome == finding.OutcomeNotApplicable { + return checker.CreateInconclusiveResult(name, "unable to detect any development/release branches") + } + + branchName, err := getBranchName(f) + if err != nil { + return checker.CreateRuntimeErrorResult(name, err) + } + if branchName == "" { + e := sce.WithMessage(sce.ErrScorecardInternal, "probe is missing branch name") + return checker.CreateRuntimeErrorResult(name, e) + } + + if _, ok := branchScores[branchName]; !ok { + branchScores[branchName] = &levelScore{} + } + + var score, max int + + doLogging := protectedBranches[branchName] + switch f.Probe { + case blocksDeleteOnBranches.Probe, blocksForcePushOnBranches.Probe: + score, max = deleteAndForcePushProtection(f, doLogging, dl) + branchScores[branchName].scores.basic += score + branchScores[branchName].maxes.basic += max + + case dismissesStaleReviews.Probe, branchProtectionAppliesToAdmins.Probe: + score, max = adminThoroughReviewProtection(f, doLogging, dl) + branchScores[branchName].scores.adminThoroughReview += score + branchScores[branchName].maxes.adminThoroughReview += max + + case requiresApproversForPullRequests.Probe: + // 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) + 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.OutcomePositive && noOfRequiredReviewers > 0 { + branchScores[branchName].scores.review += reviewerWeight + } + branchScores[branchName].maxes.review += max + + case requiresCodeOwnersReview.Probe: + score, max = codeownerBranchProtection(f, doLogging, dl) + branchScores[branchName].scores.codeownerReview += score + branchScores[branchName].maxes.codeownerReview += max + + case requiresUpToDateBranches.Probe, requiresLastPushApproval.Probe, + requiresPRsToChangeCode.Probe: + score, max = adminReviewProtection(f, doLogging, dl) + branchScores[branchName].scores.adminReview += score + branchScores[branchName].maxes.adminReview += max + + case runsStatusChecksBeforeMerging.Probe: + score, max = nonAdminContextProtection(f, doLogging, dl) + branchScores[branchName].scores.context += score + branchScores[branchName].maxes.context += max + } + } + + if len(branchScores) == 0 { return checker.CreateInconclusiveResult(name, "unable to detect any development/release branches") } + scores := make([]levelScore, 0, len(branchScores)) + for _, v := range branchScores { + scores = append(scores, *v) + } + score, err := computeFinalScore(scores) if err != nil { return checker.CreateRuntimeErrorResult(name, err) @@ -113,6 +227,14 @@ func BranchProtection(name string, dl checker.DetailLogger, } } +func getBranchName(f *finding.Finding) (string, error) { + name, ok := f.Values["branchName"] + if !ok { + return "", sce.WithMessage(sce.ErrScorecardInternal, "no branch name found") + } + return name, nil +} + func sumUpScoreForTier(t tier, scoresData []levelScore) int { sum := 0 for i := range scoresData { @@ -133,6 +255,39 @@ func sumUpScoreForTier(t tier, scoresData []levelScore) int { return sum } +func logWithDebug(f *finding.Finding, doLogging bool, dl checker.DetailLogger) { + switch f.Outcome { + case finding.OutcomeNotAvailable: + debug(dl, doLogging, f.Message) + case finding.OutcomePositive: + info(dl, doLogging, f.Message) + case finding.OutcomeNegative: + warn(dl, doLogging, f.Message) + default: + // To satisfy linter + } +} + +func logWithoutDebug(f *finding.Finding, doLogging bool, dl checker.DetailLogger) { + switch f.Outcome { + case finding.OutcomePositive: + info(dl, doLogging, f.Message) + case finding.OutcomeNegative: + warn(dl, doLogging, f.Message) + default: + // To satisfy linter + } +} + +func logInfoOrWarn(f *finding.Finding, doLogging bool, dl checker.DetailLogger) { + switch f.Outcome { + case finding.OutcomePositive: + info(dl, doLogging, f.Message) + default: + warn(dl, doLogging, f.Message) + } +} + func normalizeScore(score, max, level int) float64 { if max == 0 { return float64(level) @@ -226,208 +381,84 @@ func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interfac }) } -func basicNonAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { - score := 0 - max := 0 - // Only log information if the branch is protected. - log := branch.Protected != nil && *branch.Protected - - max++ - if branch.BranchProtectionRule.AllowForcePushes != nil { - switch *branch.BranchProtectionRule.AllowForcePushes { - case true: - warn(dl, log, "'force pushes' enabled on branch '%s'", *branch.Name) - case false: - info(dl, log, "'force pushes' disabled on branch '%s'", *branch.Name) - score++ - } +func deleteAndForcePushProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) { + var score, max int + logWithoutDebug(f, doLogging, dl) + if f.Outcome == finding.OutcomePositive { + score++ } - max++ - if branch.BranchProtectionRule.AllowDeletions != nil { - switch *branch.BranchProtectionRule.AllowDeletions { - case true: - warn(dl, log, "'allow deletion' enabled on branch '%s'", *branch.Name) - case false: - info(dl, log, "'allow deletion' disabled on branch '%s'", *branch.Name) - score++ - } - } return score, max } -func nonAdminContextProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { - score := 0 - max := 0 - // Only log information if the branch is protected. - log := branch.Protected != nil && *branch.Protected - - // This means there are specific checks enabled. - // If only `Requires status check to pass before merging` is enabled - // but no specific checks are declared, it's equivalent - // to having no status check at all. - max++ - switch { - case len(branch.BranchProtectionRule.CheckRules.Contexts) > 0: - info(dl, log, "status check found to merge onto on branch '%s'", *branch.Name) +func nonAdminContextProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) { + var score, max int + logInfoOrWarn(f, doLogging, dl) + if f.Outcome == finding.OutcomePositive { score++ - default: - warn(dl, log, "no status checks found to merge onto branch '%s'", *branch.Name) } + max++ return score, max } -func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) { - score := 0 - max := 0 - - // Having at least 1 reviewer is twice as important as the other Tier 2 requirements. - const reviewerWeight = 2 - max += reviewerWeight - if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) > 0 { - // We do not display anything here, it's done in nonAdminThoroughReviewProtection() - score += reviewerWeight +func adminReviewProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) { + var score, max int + if f.Outcome == finding.OutcomePositive { + score++ } - return score, max -} - -func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { - score := 0 - max := 0 - - // Only log information if the branch is protected. - log := branch.Protected != nil && *branch.Protected - - // Process UpToDateBeforeMerge value. - if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge == nil { - debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name) - } else { - // Note: `This setting will not take effect unless at least one status check is enabled`. - max++ - if *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge { - info(dl, log, "status checks require up-to-date branches for '%s'", *branch.Name) - score++ - } else { - warn(dl, log, "status checks do not require up-to-date branches for '%s'", *branch.Name) + switch f.Probe { + case requiresLastPushApproval.Probe, + requiresUpToDateBranches.Probe: + logWithDebug(f, doLogging, dl) + if f.Outcome != finding.OutcomeNotAvailable { + max++ } - } - - // Process RequireLastPushApproval value. - if branch.BranchProtectionRule.RequireLastPushApproval == nil { - debug(dl, log, "unable to retrieve whether 'last push approval' is required to merge on branch '%s'", *branch.Name) - } else { + default: + logInfoOrWarn(f, doLogging, dl) max++ - if *branch.BranchProtectionRule.RequireLastPushApproval { - info(dl, log, "'last push approval' enabled on branch '%s'", *branch.Name) - score++ - } else { - warn(dl, log, "'last push approval' disabled on branch '%s'", *branch.Name) - } } - - max++ - 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) - } - return score, max } -func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { - score := 0 - max := 0 - // Only log information if the branch is protected. - log := branch.Protected != nil && *branch.Protected +func adminThoroughReviewProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) { + var score, max int - if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil { - // Note: we don't increase max possible score for non-admin viewers. + logWithDebug(f, doLogging, dl) + if f.Outcome == finding.OutcomePositive { + score++ + } + if f.Outcome != finding.OutcomeNotAvailable { max++ - switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews { - case true: - info(dl, log, "stale review dismissal enabled on branch '%s'", *branch.Name) - score++ - case false: - warn(dl, log, "stale review dismissal disabled on branch '%s'", *branch.Name) - } - } else { - debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name) } + return score, max +} - // nil typically means we do not have access to the value. - if branch.BranchProtectionRule.EnforceAdmins != nil { - // Note: we don't increase max possible score for non-admin viewers. - max++ - switch *branch.BranchProtectionRule.EnforceAdmins { - case true: - info(dl, log, "settings apply to administrators on branch '%s'", *branch.Name) +func nonAdminThoroughReviewProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) { + var score, max int + if f.Outcome == finding.OutcomePositive { + noOfRequiredReviews, _ := strconv.Atoi(f.Values["numberOfRequiredReviewers"]) //nolint:errcheck + if noOfRequiredReviews >= minReviews { + info(dl, doLogging, f.Message) score++ - case false: - warn(dl, log, "settings do not apply to administrators on branch '%s'", *branch.Name) + } else { + warn(dl, doLogging, f.Message) } - } else { - debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", *branch.Name) + } else if f.Outcome == finding.OutcomeNegative { + warn(dl, doLogging, f.Message) } - + max++ return score, max } -func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { - score := 0 - max := 0 - - // Only log information if the branch is protected. - log := branch.Protected != nil && *branch.Protected - - max++ - - reviewers := valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) - if reviewers >= minReviews { - info(dl, log, "number of required reviewers is %d on branch '%s'", reviewers, *branch.Name) +func codeownerBranchProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) { + var score, max int + if f.Outcome == finding.OutcomePositive { + info(dl, doLogging, f.Message) score++ } else { - warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d", - reviewers, *branch.Name, minReviews) + warn(dl, doLogging, f.Message) } - - return score, max -} - -func codeownerBranchProtection( - branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger, -) (int, int) { - score := 0 - max := 1 - - log := branch.Protected != nil && *branch.Protected - - 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) - if len(codeownersFiles) == 0 { - warn(dl, log, "codeowners branch protection is being ignored - but no codeowners file found in repo") - } else { - score++ - } - default: - warn(dl, log, "codeowner review is not required on branch '%s'", *branch.Name) - } - } - + max++ 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 index 0084e4e0a34..f19714145eb 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -18,562 +18,1349 @@ import ( "testing" "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/clients" + sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/finding" scut "github.com/ossf/scorecard/v4/utests" ) -func testScore(branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger) (int, error) { - var score levelScore - score.scores.basic, score.maxes.basic = basicNonAdminProtection(branch, dl) - score.scores.review, score.maxes.review = nonAdminReviewProtection(branch) - score.scores.adminReview, score.maxes.adminReview = adminReviewProtection(branch, dl) - score.scores.context, score.maxes.context = nonAdminContextProtection(branch, dl) - score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(branch, dl) - score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(branch, dl) - score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(branch, codeownersFiles, dl) - - return computeFinalScore([]levelScore{score}) -} - -// TODO: order of tests to have progressive scores. -func TestIsBranchProtected(t *testing.T) { +func TestBranchProtection(t *testing.T) { t.Parallel() - trueVal := true - falseVal := false - var zeroVal int32 - var oneVal int32 = 1 - branchVal := "branch-name" tests := []struct { - name string - branch *clients.BranchRef - codeownersFiles []string - expected scut.TestReturn + name string + findings []finding.Finding + result scut.TestReturn }{ { - name: "GitHub default settings", - expected: scut.TestReturn{ - Error: nil, - Score: 3, - NumberOfWarn: 6, - NumberOfInfo: 2, - NumberOfDebug: 1, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &falseVal, - }, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &trueVal, - Contexts: nil, - UpToDateBeforeMerge: &falseVal, + name: "Branch name is an empty string which is not allowed and will error", + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "", }, }, }, - }, - { - name: "Nothing is enabled and values are nil", - expected: scut.TestReturn{ - Error: nil, - Score: 0, - NumberOfWarn: 3, - NumberOfInfo: 0, - NumberOfDebug: 4, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, + result: scut.TestReturn{ + Error: sce.ErrScorecardInternal, + Score: checker.InconclusiveResultScore, }, }, { name: "Required status check enabled", - expected: scut.TestReturn{ - Error: nil, - Score: 4, - NumberOfWarn: 5, - NumberOfInfo: 5, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - }, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &trueVal, - UpToDateBeforeMerge: &trueVal, - Contexts: []string{"foo"}, - }, - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, }, }, + result: scut.TestReturn{ + Score: 4, + NumberOfInfo: 5, + NumberOfWarn: 5, + }, }, { name: "Required status check enabled without checking for status string", - expected: scut.TestReturn{ - Error: nil, - Score: 4, - NumberOfWarn: 6, - NumberOfInfo: 4, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, - }, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &trueVal, - UpToDateBeforeMerge: &trueVal, - Contexts: nil, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, }, + result: scut.TestReturn{ + Score: 4, + NumberOfInfo: 4, + NumberOfWarn: 6, + }, }, { name: "Admin run only preventing force pushes and deletions", - expected: scut.TestReturn{ - Error: nil, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ Score: 3, NumberOfWarn: 6, NumberOfInfo: 2, NumberOfDebug: 1, }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &falseVal, - Contexts: nil, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &falseVal, - }, - }, - }, }, { name: "Admin run with all tier 2 requirements except require PRs and reviewers", - expected: scut.TestReturn{ - Error: nil, - Score: 4, // Should be 4.2 if we allow decimal puctuation + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ + Score: 4, NumberOfWarn: 2, NumberOfInfo: 6, NumberOfDebug: 1, }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLastPushApproval: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &trueVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &falseVal, - }, - }, - }, }, { name: "Admin run on project requiring pull requests but without approver -- best a single maintainer can do", - expected: scut.TestReturn{ - Error: nil, - Score: 4, // Should be 4.8 if we allow decimal punctuation - NumberOfWarn: 2, - NumberOfInfo: 9, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLastPushApproval: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &trueVal, - UpToDateBeforeMerge: &trueVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &trueVal, - RequireCodeOwnerReviews: &trueVal, - RequiredApprovingReviewCount: &zeroVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, }, + result: scut.TestReturn{ + Score: 4, + NumberOfWarn: 1, + NumberOfInfo: 9, + }, }, { name: "Admin run on project with all tier 2 requirements", - expected: scut.TestReturn{ - Error: nil, - Score: 6, - NumberOfWarn: 4, - NumberOfInfo: 6, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLastPushApproval: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &trueVal, - Contexts: nil, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &oneVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "1", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ + Score: 6, + NumberOfWarn: 4, + NumberOfInfo: 6, }, }, { name: "Non-admin run on project that require zero reviewer (or don't require PRs at all, we can't differentiate it)", - expected: scut.TestReturn{ - Error: nil, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ Score: 3, NumberOfWarn: 3, NumberOfInfo: 2, NumberOfDebug: 4, }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: nil, - RequireLastPushApproval: nil, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: nil, - UpToDateBeforeMerge: nil, - Contexts: nil, - }, - }, - }, }, { name: "Non-admin run on project that require 1 reviewer", - expected: scut.TestReturn{ - Error: nil, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "1", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomeNotAvailable, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ Score: 6, NumberOfWarn: 3, NumberOfInfo: 3, NumberOfDebug: 4, }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: nil, - RequireLastPushApproval: nil, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: nil, - UpToDateBeforeMerge: nil, - Contexts: nil, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: nil, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &oneVal, - }, - }, - }, }, { name: "Required admin enforcement enabled", - expected: scut.TestReturn{ - Error: nil, - Score: 3, - NumberOfWarn: 5, - NumberOfInfo: 5, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &falseVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, }, + result: scut.TestReturn{ + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 5, + }, }, { name: "Required linear history enabled", - expected: scut.TestReturn{ - Error: nil, - Score: 3, - NumberOfWarn: 6, - NumberOfInfo: 4, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &falseVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, }, + result: scut.TestReturn{ + Score: 3, + NumberOfWarn: 6, + NumberOfInfo: 4, + }, }, { name: "Allow force push enabled", - expected: scut.TestReturn{ - Error: nil, - Score: 1, - NumberOfWarn: 7, - NumberOfInfo: 3, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &trueVal, - AllowDeletions: &falseVal, - - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &falseVal, - Contexts: []string{"foo"}, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, }, + result: scut.TestReturn{ + Score: 1, + NumberOfWarn: 7, + NumberOfInfo: 3, + }, }, { name: "Allow deletions enabled", - expected: scut.TestReturn{ - Error: nil, - Score: 1, - NumberOfWarn: 7, - NumberOfInfo: 3, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &trueVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &falseVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &falseVal, - RequireCodeOwnerReviews: &falseVal, - RequiredApprovingReviewCount: &zeroVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", }, }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "0", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomeNegative, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ + Score: 1, + NumberOfWarn: 7, + NumberOfInfo: 3, }, }, { name: "Branches are protected", - expected: scut.TestReturn{ - Error: nil, - Score: 8, - NumberOfWarn: 2, - NumberOfInfo: 9, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - RequireLastPushApproval: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &trueVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &trueVal, - RequireCodeOwnerReviews: &trueVal, - RequiredApprovingReviewCount: &oneVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "1", + }, + }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ + Score: 8, + NumberOfWarn: 1, + NumberOfInfo: 9, }, }, { name: "Branches are protected and require codeowner review", - expected: scut.TestReturn{ - Error: nil, - Score: 8, - NumberOfWarn: 1, - NumberOfInfo: 9, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - RequireLastPushApproval: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &trueVal, - UpToDateBeforeMerge: &trueVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &trueVal, - RequireCodeOwnerReviews: &trueVal, - RequiredApprovingReviewCount: &oneVal, + findings: []finding.Finding{ + { + Probe: "blocksDeleteOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", }, }, - }, - codeownersFiles: []string{".github/CODEOWNERS"}, - }, - { - name: "Branches are protected and require codeowner review, but file is not present", - expected: scut.TestReturn{ - Error: nil, - Score: 5, - NumberOfWarn: 3, - NumberOfInfo: 8, - NumberOfDebug: 0, - }, - branch: &clients.BranchRef{ - Name: &branchVal, - Protected: &trueVal, - BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLastPushApproval: &falseVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, - CheckRules: clients.StatusChecksRule{ - RequiresStatusChecks: &falseVal, - UpToDateBeforeMerge: &trueVal, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: clients.PullRequestReviewRule{ - Required: &trueVal, - DismissStaleReviews: &trueVal, - RequireCodeOwnerReviews: &trueVal, - RequiredApprovingReviewCount: &oneVal, + { + Probe: "blocksForcePushOnBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchesAreProtected", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "branchProtectionAppliesToAdmins", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "dismissesStaleReviews", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresApproversForPullRequests", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + "numberOfRequiredReviewers": "1", }, }, + { + Probe: "requiresCodeOwnersReview", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresLastPushApproval", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresUpToDateBranches", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "runsStatusChecksBeforeMerging", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + { + Probe: "requiresPRsToChangeCode", + Outcome: finding.OutcomePositive, + Values: map[string]string{ + "branchName": "main", + }, + }, + }, + result: scut.TestReturn{ + Score: 8, + NumberOfWarn: 1, + NumberOfInfo: 9, }, }, } for _, tt := range tests { - tt := tt // Re-initializing variable so it is not changed while executing the closure below + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() dl := scut.TestDetailLogger{} - score, err := testScore(tt.branch, tt.codeownersFiles, &dl) - actual := &checker.CheckResult{ - Score: score, - Error: err, - } - scut.ValidateTestReturn(t, tt.name, &tt.expected, actual, &dl) + got := BranchProtection(tt.name, tt.findings, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.result, &got, &dl) }) } } diff --git a/checks/raw/branch_protection.go b/checks/raw/branch_protection.go index 6e58bd2f2f8..505fb608da0 100644 --- a/checks/raw/branch_protection.go +++ b/checks/raw/branch_protection.go @@ -51,7 +51,8 @@ func (set branchSet) contains(branch string) bool { } // BranchProtection retrieves the raw data for the Branch-Protection check. -func BranchProtection(c clients.RepoClient) (checker.BranchProtectionsData, error) { +func BranchProtection(cr *checker.CheckRequest) (checker.BranchProtectionsData, error) { + c := cr.RepoClient branches := branchSet{ exists: make(map[string]bool), } diff --git a/checks/raw/branch_protection_test.go b/checks/raw/branch_protection_test.go index 4db6f385f65..7a5ce78031b 100644 --- a/checks/raw/branch_protection_test.go +++ b/checks/raw/branch_protection_test.go @@ -273,7 +273,11 @@ func TestBranchProtection(t *testing.T) { return tt.releases, tt.releasesErr }) mockRepoClient.EXPECT().ListFiles(gomock.Any()).AnyTimes().Return(tt.repoFiles, nil) - rawData, err := BranchProtection(mockRepoClient) + + c := &checker.CheckRequest{ + RepoClient: mockRepoClient, + } + rawData, err := BranchProtection(c) if !errors.Is(err, tt.wantErr) { t.Errorf("failed. expected: %v, got: %v", tt.wantErr, err) t.Fail() diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index 0696903de5c..38804dc27d2 100644 --- a/e2e/branch_protection_test.go +++ b/e2e/branch_protection_test.go @@ -48,7 +48,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { Error: nil, Score: 6, NumberOfWarn: 2, - NumberOfInfo: 5, + NumberOfInfo: 4, NumberOfDebug: 4, } result := checks.BranchProtection(&req) diff --git a/probes/blocksDeleteOnBranches/impl.go b/probes/blocksDeleteOnBranches/impl.go index bd295fa4475..a48bf1923fd 100644 --- a/probes/blocksDeleteOnBranches/impl.go +++ b/probes/blocksDeleteOnBranches/impl.go @@ -40,6 +40,15 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] diff --git a/probes/blocksForcePushOnBranches/impl.go b/probes/blocksForcePushOnBranches/impl.go index d87921c1dd1..41871d7eae4 100644 --- a/probes/blocksForcePushOnBranches/impl.go +++ b/probes/blocksForcePushOnBranches/impl.go @@ -40,8 +40,18 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] + var text string var outcome finding.Outcome switch { diff --git a/probes/branchProtectionAppliesToAdmins/impl.go b/probes/branchProtectionAppliesToAdmins/impl.go index 6170ff5cb80..14fcec6973f 100644 --- a/probes/branchProtectionAppliesToAdmins/impl.go +++ b/probes/branchProtectionAppliesToAdmins/impl.go @@ -41,6 +41,15 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] diff --git a/probes/branchesAreProtected/def.yml b/probes/branchesAreProtected/def.yml new file mode 100644 index 00000000000..ddbb00a89d3 --- /dev/null +++ b/probes/branchesAreProtected/def.yml @@ -0,0 +1,30 @@ +# Copyright 2023 OpenSSF Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +id: branchesAreProtected +short: Check that the projects branches are protected. +motivation: > + Branches that are not protected may allow excessive actions that could compromise the projects security. +implementation: > + Checks the protection rules of default and release branches. +outcome: + - The probe returns one OutcomePositive for each branch that is protected, and one OutcomeNegative for branches that are not protected. Scorecard only considers default and releases branches. +remediation: + effort: Low + text: + - For Gitlab-hosted project, follow the documentation on how to protect branches, https://docs.gitlab.com/ee/user/project/protected_branches.html + - For GitHub-hosted projects, follow [the documentation on protected branches, https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches + markdown: + - For Gitlab-hosted project, follow [the documentation on how to protect branches](https://docs.gitlab.com/ee/user/project/protected_branches.html) + - For GitHub-hosted projects, follow [the documentation on protected branches](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches) diff --git a/probes/branchesAreProtected/impl.go b/probes/branchesAreProtected/impl.go new file mode 100644 index 00000000000..e35b9dd6150 --- /dev/null +++ b/probes/branchesAreProtected/impl.go @@ -0,0 +1,73 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package branchesAreProtected + +import ( + "embed" + "fmt" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" +) + +//go:embed *.yml +var fs embed.FS + +const ( + Probe = "branchesAreProtected" + BranchNameKey = "branchName" +) + +func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } + + r := raw.BranchProtectionResults + var findings []finding.Finding + + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + + for i := range r.Branches { + branch := &r.Branches[i] + + protected := (branch.Protected != nil && *branch.Protected) + var text string + var outcome finding.Outcome + if protected { + text = fmt.Sprintf("branch '%s' is protected", *branch.Name) + outcome = finding.OutcomePositive + } else { + text = fmt.Sprintf("branch '%s' is not protected", *branch.Name) + outcome = finding.OutcomeNegative + } + f, err := finding.NewWith(fs, Probe, text, nil, outcome) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f = f.WithValue(BranchNameKey, *branch.Name) + findings = append(findings, *f) + } + return findings, Probe, nil +} diff --git a/probes/branchesAreProtected/impl_test.go b/probes/branchesAreProtected/impl_test.go new file mode 100644 index 00000000000..823f888da65 --- /dev/null +++ b/probes/branchesAreProtected/impl_test.go @@ -0,0 +1,169 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package branchesAreProtected + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/finding" +) + +func Test_Run(t *testing.T) { + t.Parallel() + trueVal := true + falseVal := false + branchVal1 := "branch-name1" + branchVal2 := "branch-name1" + + //nolint:govet + tests := []struct { + name string + raw *checker.RawResults + outcomes []finding.Outcome + err error + }{ + { + name: "One branch. Protection unknown", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + Protected: nil, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + }, + }, + { + name: "Two protected branches", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + Protected: &trueVal, + }, + { + Name: &branchVal2, + Protected: &trueVal, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, finding.OutcomePositive, + }, + }, + { + name: "Two branches. First is protected", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + Protected: &trueVal, + }, + { + Name: &branchVal2, + Protected: &falseVal, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, finding.OutcomeNegative, + }, + }, + { + name: "Two branches. Second is protected", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + Protected: &falseVal, + }, + { + Name: &branchVal2, + Protected: &trueVal, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, finding.OutcomePositive, + }, + }, + { + name: "Two branches. First one is not protected, second unknown", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + Protected: &falseVal, + }, + { + Name: &branchVal2, + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: nil, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, finding.OutcomeNegative, + }, + }, + } + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + findings, s, err := Run(tt.raw) + if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors())) + } + if err != nil { + return + } + if diff := cmp.Diff(Probe, s); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(len(tt.outcomes), len(findings)); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + for i := range tt.outcomes { + outcome := &tt.outcomes[i] + f := &findings[i] + if diff := cmp.Diff(*outcome, f.Outcome); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +} diff --git a/probes/dismissesStaleReviews/impl.go b/probes/dismissesStaleReviews/impl.go index fd63d7646da..814122bfe1c 100644 --- a/probes/dismissesStaleReviews/impl.go +++ b/probes/dismissesStaleReviews/impl.go @@ -41,6 +41,15 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] diff --git a/probes/entries.go b/probes/entries.go index 62c0dc2d3cb..99dadceeae2 100644 --- a/probes/entries.go +++ b/probes/entries.go @@ -19,9 +19,14 @@ import ( "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/blocksDeleteOnBranches" + "github.com/ossf/scorecard/v4/probes/blocksForcePushOnBranches" + "github.com/ossf/scorecard/v4/probes/branchProtectionAppliesToAdmins" + "github.com/ossf/scorecard/v4/probes/branchesAreProtected" "github.com/ossf/scorecard/v4/probes/codeApproved" "github.com/ossf/scorecard/v4/probes/codeReviewOneReviewers" "github.com/ossf/scorecard/v4/probes/contributorsFromOrgOrCompany" + "github.com/ossf/scorecard/v4/probes/dismissesStaleReviews" "github.com/ossf/scorecard/v4/probes/freeOfUnverifiedBinaryArtifacts" "github.com/ossf/scorecard/v4/probes/fuzzedWithCLibFuzzer" "github.com/ossf/scorecard/v4/probes/fuzzedWithClusterFuzzLite" @@ -50,6 +55,12 @@ import ( "github.com/ossf/scorecard/v4/probes/pinsDependencies" "github.com/ossf/scorecard/v4/probes/releasesAreSigned" "github.com/ossf/scorecard/v4/probes/releasesHaveProvenance" + "github.com/ossf/scorecard/v4/probes/requiresApproversForPullRequests" + "github.com/ossf/scorecard/v4/probes/requiresCodeOwnersReview" + "github.com/ossf/scorecard/v4/probes/requiresLastPushApproval" + "github.com/ossf/scorecard/v4/probes/requiresPRsToChangeCode" + "github.com/ossf/scorecard/v4/probes/requiresUpToDateBranches" + "github.com/ossf/scorecard/v4/probes/runsStatusChecksBeforeMerging" "github.com/ossf/scorecard/v4/probes/sastToolConfigured" "github.com/ossf/scorecard/v4/probes/sastToolRunsOnAllCommits" "github.com/ossf/scorecard/v4/probes/securityPolicyContainsLinks" @@ -147,6 +158,19 @@ var ( releasesAreSigned.Run, releasesHaveProvenance.Run, } + BranchProtection = []ProbeImpl{ + blocksDeleteOnBranches.Run, + blocksForcePushOnBranches.Run, + branchesAreProtected.Run, + branchProtectionAppliesToAdmins.Run, + dismissesStaleReviews.Run, + requiresApproversForPullRequests.Run, + requiresCodeOwnersReview.Run, + requiresLastPushApproval.Run, + requiresUpToDateBranches.Run, + runsStatusChecksBeforeMerging.Run, + requiresPRsToChangeCode.Run, + } PinnedDependencies = []ProbeImpl{ pinsDependencies.Run, } diff --git a/probes/requiresApproversForPullRequests/impl.go b/probes/requiresApproversForPullRequests/impl.go index 14f114f2f5e..05960a1189e 100644 --- a/probes/requiresApproversForPullRequests/impl.go +++ b/probes/requiresApproversForPullRequests/impl.go @@ -45,10 +45,19 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] + nilMsg := fmt.Sprintf("could not determine whether branch '%s' has required approving review count", *branch.Name) - trueMsg := fmt.Sprintf("required approving review count on branch '%s'", *branch.Name) falseMsg := fmt.Sprintf("branch '%s' does not require approvers", *branch.Name) p := branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount @@ -62,7 +71,8 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { case p == nil: f = f.WithMessage(nilMsg).WithOutcome(finding.OutcomeNotAvailable) case *p > 0: - f = f.WithMessage(trueMsg).WithOutcome(finding.OutcomePositive) + msg := fmt.Sprintf("required approving review count is %d on branch '%s'", *p, *branch.Name) + f = f.WithMessage(msg).WithOutcome(finding.OutcomePositive) f = f.WithValue(RequiredReviewersKey, strconv.Itoa(int(*p))) case *p == 0: f = f.WithMessage(falseMsg).WithOutcome(finding.OutcomeNegative) diff --git a/probes/requiresCodeOwnersReview/impl.go b/probes/requiresCodeOwnersReview/impl.go index b5ff4bc9143..b1463d68d82 100644 --- a/probes/requiresCodeOwnersReview/impl.go +++ b/probes/requiresCodeOwnersReview/impl.go @@ -40,8 +40,18 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] + reqOwnerReviews := branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews var text string var outcome finding.Outcome diff --git a/probes/requiresCodeOwnersReview/impl_test.go b/probes/requiresCodeOwnersReview/impl_test.go index b58a00c716a..7d0df964d01 100644 --- a/probes/requiresCodeOwnersReview/impl_test.go +++ b/probes/requiresCodeOwnersReview/impl_test.go @@ -104,7 +104,7 @@ func Test_Run(t *testing.T) { }, }, }, - CodeownersFiles: []string{"file"}, + CodeownersFiles: []string{"file1"}, }, }, outcomes: []finding.Outcome{ @@ -112,7 +112,7 @@ func Test_Run(t *testing.T) { }, }, { - name: "2 branches require code owner reviews with files = 2 negative outcomes", + name: "2 branches require code owner reviews with files = 2 positive outcomes", raw: &checker.RawResults{ BranchProtectionResults: checker.BranchProtectionsData{ Branches: []clients.BranchRef{ @@ -133,11 +133,11 @@ func Test_Run(t *testing.T) { }, }, }, - CodeownersFiles: []string{}, + CodeownersFiles: []string{"file1", "file2"}, }, }, outcomes: []finding.Outcome{ - finding.OutcomeNegative, finding.OutcomeNegative, + finding.OutcomePositive, finding.OutcomePositive, }, }, { @@ -170,7 +170,7 @@ func Test_Run(t *testing.T) { }, }, { - name: "Requires code owner reviews on 1/2 branches - without files = 2 negative outcomes", + name: "Requires code owner reviews on 1/2 branches - without files = 1 positive and 1 negative", raw: &checker.RawResults{ BranchProtectionResults: checker.BranchProtectionsData{ Branches: []clients.BranchRef{ @@ -191,11 +191,11 @@ func Test_Run(t *testing.T) { }, }, }, - CodeownersFiles: []string{}, + CodeownersFiles: []string{"file"}, }, }, outcomes: []finding.Outcome{ - finding.OutcomeNegative, finding.OutcomeNegative, + finding.OutcomePositive, finding.OutcomeNegative, }, }, { @@ -228,7 +228,7 @@ func Test_Run(t *testing.T) { }, }, { - name: "Requires code owner reviews on 1/2 branches - without files = 2 negative outcomes", + name: "Requires code owner reviews on 1/2 branches - without files = 2 negative", raw: &checker.RawResults{ BranchProtectionResults: checker.BranchProtectionsData{ Branches: []clients.BranchRef{ diff --git a/probes/requiresLastPushApproval/impl.go b/probes/requiresLastPushApproval/impl.go index 43ef8e53442..e4ff33cfd2b 100644 --- a/probes/requiresLastPushApproval/impl.go +++ b/probes/requiresLastPushApproval/impl.go @@ -41,6 +41,15 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] diff --git a/probes/requiresPRsToChangeCode/def.yml b/probes/requiresPRsToChangeCode/def.yml new file mode 100644 index 00000000000..01bdeb8a741 --- /dev/null +++ b/probes/requiresPRsToChangeCode/def.yml @@ -0,0 +1,32 @@ +# Copyright 2023 OpenSSF Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +id: requiresPRsToChangeCode +short: Check that the project requires pull requests to change code. +motivation: > + Changing code without pull requests does not leave a traceable trail and can allow malicious actors to sneak in vulnerable code. +implementation: > + The probe checks which branches require pull requests to change the branches' code. The probe only considers default and release branches. +outcome: + - The probe returns one OutcomePositive for each branch that requires pull requests to change code, and one OutcomeNegative for branches that don't. +remediation: + effort: Low + text: + - Configure the project such that contributors must make PRs to change code. + - For GitHub-hosted projects, see [the Pull Requests documentation](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests). + - For Gitlab-hosted projects, see [the Merge Requests documentation](https://docs.gitlab.com/ee/user/project/merge_requests/). + markdown: + - Configure the project such that contributors must make PRs to change code. + - For GitHub-hosted projects, see [the Pull Requests documentation](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests). + - For Gitlab-hosted projects, see [the Merge Requests documentation](https://docs.gitlab.com/ee/user/project/merge_requests/). \ No newline at end of file diff --git a/probes/requiresPRsToChangeCode/impl.go b/probes/requiresPRsToChangeCode/impl.go new file mode 100644 index 00000000000..b895d8320b8 --- /dev/null +++ b/probes/requiresPRsToChangeCode/impl.go @@ -0,0 +1,86 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package requiresPRsToChangeCode + +import ( + "embed" + "errors" + "fmt" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" +) + +//go:embed *.yml +var fs embed.FS + +const ( + Probe = "requiresPRsToChangeCode" + BranchNameKey = "branchName" +) + +var errWrongValue = errors.New("wrong value, should not happen") + +func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } + + r := raw.BranchProtectionResults + var findings []finding.Finding + + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + + for i := range r.Branches { + branch := &r.Branches[i] + + nilMsg := fmt.Sprintf("could not determine whether branch '%s' requires PRs to change code", *branch.Name) + trueMsg := fmt.Sprintf("PRs are required in order to make changes on branch '%s'", *branch.Name) + falseMsg := fmt.Sprintf("PRs are not required to make changes on branch '%s'; ", *branch.Name) + + "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" + + p := branch.BranchProtectionRule.RequiredPullRequestReviews.Required + + f, err := finding.NewWith(fs, Probe, "", nil, finding.OutcomeNotAvailable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + + switch { + case p == nil: + f = f.WithMessage(nilMsg).WithOutcome(finding.OutcomeNotAvailable) + case *p: + f = f.WithMessage(trueMsg).WithOutcome(finding.OutcomePositive) + case !*p: + f = f.WithMessage(falseMsg).WithOutcome(finding.OutcomeNegative) + default: + return nil, Probe, fmt.Errorf("create finding: %w", errWrongValue) + } + f = f.WithValue(BranchNameKey, *branch.Name) + findings = append(findings, *f) + } + return findings, Probe, nil +} diff --git a/probes/requiresPRsToChangeCode/impl_test.go b/probes/requiresPRsToChangeCode/impl_test.go new file mode 100644 index 00000000000..9bf181d3a7d --- /dev/null +++ b/probes/requiresPRsToChangeCode/impl_test.go @@ -0,0 +1,202 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package requiresPRsToChangeCode + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/finding" +) + +func Test_Run(t *testing.T) { + t.Parallel() + trueVal := true + falseVal := false + branchVal1 := "branch-name1" + branchVal2 := "branch-name1" + //nolint:govet + tests := []struct { + name string + raw *checker.RawResults + outcomes []finding.Outcome + err error + }{ + { + name: "1 branch requires PRs to change code", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, + }, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, + }, + }, + { + name: "2 branches require PRs to change code = 2 positive outcomes", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, + }, + }, + }, + { + Name: &branchVal2, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, + }, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, finding.OutcomePositive, + }, + }, + { + name: "1 branches require PRs to change code and 1 branch doesn't = 1 positive 1 negative", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, + }, + }, + }, + { + Name: &branchVal2, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, finding.OutcomeNegative, + }, + }, + { + name: "Requires PRs to change code on 1/2 branches = 1 negative and 1 positive", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, + }, + }, + { + Name: &branchVal2, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, + }, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, finding.OutcomePositive, + }, + }, + { + name: "1 branch does not require PRs to change code and 1 lacks data = 1 negative and 1 unavailable", + raw: &checker.RawResults{ + BranchProtectionResults: checker.BranchProtectionsData{ + Branches: []clients.BranchRef{ + { + Name: &branchVal1, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, + }, + }, + { + Name: &branchVal2, + BranchProtectionRule: clients.BranchProtectionRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: nil, + }, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, finding.OutcomeNotAvailable, + }, + }, + } + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + findings, s, err := Run(tt.raw) + if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors())) + } + if err != nil { + return + } + if diff := cmp.Diff(Probe, s); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(len(tt.outcomes), len(findings)); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + for i := range tt.outcomes { + outcome := &tt.outcomes[i] + f := &findings[i] + if diff := cmp.Diff(*outcome, f.Outcome); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +} diff --git a/probes/requiresUpToDateBranches/impl.go b/probes/requiresUpToDateBranches/impl.go index fc083cf5656..ed9331cb71b 100644 --- a/probes/requiresUpToDateBranches/impl.go +++ b/probes/requiresUpToDateBranches/impl.go @@ -41,6 +41,15 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] diff --git a/probes/runsStatusChecksBeforeMerging/impl.go b/probes/runsStatusChecksBeforeMerging/impl.go index ba9c6bc691c..6d5720f6bea 100644 --- a/probes/runsStatusChecksBeforeMerging/impl.go +++ b/probes/runsStatusChecksBeforeMerging/impl.go @@ -40,28 +40,38 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { r := raw.BranchProtectionResults var findings []finding.Finding + if len(r.Branches) == 0 { + f, err := finding.NewWith(fs, Probe, "no branches found", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + for i := range r.Branches { branch := &r.Branches[i] + var f *finding.Finding + var err error + switch { case len(branch.BranchProtectionRule.CheckRules.Contexts) > 0: - f, err := finding.NewWith(fs, Probe, + f, err = finding.NewWith(fs, Probe, fmt.Sprintf("status check found to merge onto on branch '%s'", *branch.Name), nil, finding.OutcomePositive) if err != nil { return nil, Probe, fmt.Errorf("create finding: %w", err) } - f = f.WithValue(BranchNameKey, *branch.Name) - findings = append(findings, *f) default: - f, err := finding.NewWith(fs, Probe, + f, err = finding.NewWith(fs, Probe, fmt.Sprintf("no status checks found to merge onto branch '%s'", *branch.Name), nil, finding.OutcomeNegative) if err != nil { return nil, Probe, fmt.Errorf("create finding: %w", err) } - f = f.WithValue(BranchNameKey, *branch.Name) - findings = append(findings, *f) } + f = f.WithValue(BranchNameKey, *branch.Name) + findings = append(findings, *f) } return findings, Probe, nil }