diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index f5848d521dc8..d90b773893b0 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -113,119 +113,6 @@ func BranchProtection(name string, dl checker.DetailLogger, } } -func sumUpScoreForTier(t tier, scoresData []levelScore) int { - sum := 0 - for i := range scoresData { - score := scoresData[i] - switch t { - case Tier1: - sum += score.scores.basic - case Tier2: - sum += score.scores.review + score.scores.adminReview - case Tier3: - sum += score.scores.context - case Tier4: - sum += score.scores.thoroughReview + score.scores.codeownerReview - case Tier5: - sum += score.scores.adminThoroughReview - } - } - return sum -} - -func normalizeScore(score, max, level int) float64 { - if max == 0 { - return float64(level) - } - return float64(score*level) / float64(max) -} - -func computeFinalScore(scores []levelScore) (int, error) { - if len(scores) == 0 { - return 0, sce.WithMessage(sce.ErrScorecardInternal, "scores are empty") - } - - score := float64(0) - maxScore := scores[0].maxes - - // First, check if they all pass the basic (admin and non-admin) checks. - maxBasicScore := maxScore.basic * len(scores) - basicScore := sumUpScoreForTier(Tier1, scores) - score += normalizeScore(basicScore, maxBasicScore, basicLevel) - if basicScore < maxBasicScore { - return int(score), nil - } - - // Second, check the (admin and non-admin) reviews. - maxReviewScore := maxScore.review * len(scores) - maxAdminReviewScore := maxScore.adminReview * len(scores) - adminNonAdminReviewScore := sumUpScoreForTier(Tier2, scores) - score += normalizeScore(adminNonAdminReviewScore, maxReviewScore+maxAdminReviewScore, adminNonAdminReviewLevel) - if adminNonAdminReviewScore < maxReviewScore+maxAdminReviewScore { - return int(score), nil - } - - // Third, check the use of non-admin context. - maxContextScore := maxScore.context * len(scores) - contextScore := sumUpScoreForTier(Tier3, scores) - score += normalizeScore(contextScore, maxContextScore, nonAdminContextLevel) - if contextScore < maxContextScore { - return int(score), nil - } - - // Fourth, check the thorough non-admin reviews. - // Also check whether this repo requires codeowner review - maxThoroughReviewScore := maxScore.thoroughReview * len(scores) - maxCodeownerReviewScore := maxScore.codeownerReview * len(scores) - tier4Score := sumUpScoreForTier(Tier4, scores) - score += normalizeScore(tier4Score, maxThoroughReviewScore+maxCodeownerReviewScore, nonAdminThoroughReviewLevel) - if tier4Score < maxThoroughReviewScore+maxCodeownerReviewScore { - return int(score), nil - } - - // Lastly, check the thorough admin review config. - // This one is controversial and has usability issues - // https://github.com/ossf/scorecard/issues/1027, so we may remove it. - maxAdminThoroughReviewScore := maxScore.adminThoroughReview * len(scores) - adminThoroughReviewScore := sumUpScoreForTier(Tier5, scores) - score += normalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel) - if adminThoroughReviewScore != maxAdminThoroughReviewScore { - return int(score), nil - } - - return int(score), nil -} - -func info(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { - if !doLogging { - return - } - - dl.Info(&checker.LogMessage{ - Text: fmt.Sprintf(desc, args...), - }) -} - -func debug(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { - if !doLogging { - return - } - - dl.Debug(&checker.LogMessage{ - Text: fmt.Sprintf(desc, args...), - }) -} - -func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { - if !doLogging { - return - } - - dl.Warn(&checker.LogMessage{ - Text: fmt.Sprintf(desc, args...), - }) -} - func basicNonAdminProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { score := 0 max := 0 @@ -423,6 +310,89 @@ func codeownerBranchProtection( return score, max } +func computeFinalScore(scores []levelScore) (int, error) { + if len(scores) == 0 { + return 0, sce.WithMessage(sce.ErrScorecardInternal, "scores are empty") + } + + score := float64(0) + maxScore := scores[0].maxes + + // First, check if they all pass the basic (admin and non-admin) checks. + maxBasicScore := maxScore.basic * len(scores) + basicScore := sumUpScoreForTier(Tier1, scores) + score += normalizeScore(basicScore, maxBasicScore, basicLevel) + if basicScore < maxBasicScore { + return int(score), nil + } + + // Second, check the (admin and non-admin) reviews. + maxReviewScore := maxScore.review * len(scores) + maxAdminReviewScore := maxScore.adminReview * len(scores) + adminNonAdminReviewScore := sumUpScoreForTier(Tier2, scores) + score += normalizeScore(adminNonAdminReviewScore, maxReviewScore+maxAdminReviewScore, adminNonAdminReviewLevel) + if adminNonAdminReviewScore < maxReviewScore+maxAdminReviewScore { + return int(score), nil + } + + // Third, check the use of non-admin context. + maxContextScore := maxScore.context * len(scores) + contextScore := sumUpScoreForTier(Tier3, scores) + score += normalizeScore(contextScore, maxContextScore, nonAdminContextLevel) + if contextScore < maxContextScore { + return int(score), nil + } + + // Fourth, check the thorough non-admin reviews. + // Also check whether this repo requires codeowner review + maxThoroughReviewScore := maxScore.thoroughReview * len(scores) + maxCodeownerReviewScore := maxScore.codeownerReview * len(scores) + tier4Score := sumUpScoreForTier(Tier4, scores) + score += normalizeScore(tier4Score, maxThoroughReviewScore+maxCodeownerReviewScore, nonAdminThoroughReviewLevel) + if tier4Score < maxThoroughReviewScore+maxCodeownerReviewScore { + return int(score), nil + } + + // Lastly, check the thorough admin review config. + // This one is controversial and has usability issues + // https://github.com/ossf/scorecard/issues/1027, so we may remove it. + maxAdminThoroughReviewScore := maxScore.adminThoroughReview * len(scores) + adminThoroughReviewScore := sumUpScoreForTier(Tier5, scores) + score += normalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel) + if adminThoroughReviewScore != maxAdminThoroughReviewScore { + return int(score), nil + } + + return int(score), nil +} + +func sumUpScoreForTier(t tier, scoresData []levelScore) int { + sum := 0 + for i := range scoresData { + score := scoresData[i] + switch t { + case Tier1: + sum += score.scores.basic + case Tier2: + sum += score.scores.review + score.scores.adminReview + case Tier3: + sum += score.scores.context + case Tier4: + sum += score.scores.thoroughReview + score.scores.codeownerReview + case Tier5: + sum += score.scores.adminThoroughReview + } + } + return sum +} + +func normalizeScore(score, max, level int) float64 { + if max == 0 { + return float64(level) + } + return float64(score*level) / float64(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 { @@ -431,3 +401,33 @@ func valueOrZero[T any](ptr *T) T { } return *ptr } + +func info(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { + if !doLogging { + return + } + + dl.Info(&checker.LogMessage{ + Text: fmt.Sprintf(desc, args...), + }) +} + +func debug(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { + if !doLogging { + return + } + + dl.Debug(&checker.LogMessage{ + Text: fmt.Sprintf(desc, args...), + }) +} + +func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { + if !doLogging { + return + } + + dl.Warn(&checker.LogMessage{ + Text: fmt.Sprintf(desc, args...), + }) +} diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 4c42c07e1fc5..035bbdc662d3 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -284,6 +284,10 @@ func (handler *branchesHandler) setup() error { return handler.errSetup } +func isPermissionsError(err error) bool { + return strings.Contains(err.Error(), "Resource not accessible") +} + func (handler *branchesHandler) query(branchName string) (*clients.BranchRef, error) { if !strings.EqualFold(handler.repourl.commitSHA, clients.HeadSHA) { return nil, fmt.Errorf("%w: branches only supported for HEAD queries", clients.ErrUnsupportedFeature) @@ -323,71 +327,6 @@ func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, er return branchRef, nil } -// TODO: Move these two functions to below the GetBranchRefFrom functions, the single place they're used. -func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionRule) { - copyBoolPtr(src.IsAdminEnforced, &dst.EnforceAdmins) - copyBoolPtr(src.RequireLastPushApproval, &dst.RequireLastPushApproval) - copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) - if src.RequiresStatusChecks != nil { - copyBoolPtr(src.RequiresStatusChecks, &dst.CheckRules.RequiresStatusChecks) - // TODO(#3255): Update when GitHub GraphQL bug is fixed - // Workaround for GitHub GraphQL bug https://github.com/orgs/community/discussions/59471 - // The setting RequiresStrictStatusChecks should tell if the branch is required - // to be up to date before merge, but it only returns the correct value if - // RequiresStatusChecks is true. If RequiresStatusChecks is false, RequiresStrictStatusChecks - // is wrongly retrieved as true. - if src.RequiresStrictStatusChecks != nil { - upToDateBeforeMerge := *src.RequiresStatusChecks && *src.RequiresStrictStatusChecks - copyBoolPtr(&upToDateBeforeMerge, &dst.CheckRules.UpToDateBeforeMerge) - } - } - // we always have the data to know if PRs are required - if dst.RequiredPullRequestReviews.Required == nil { - dst.RequiredPullRequestReviews.Required = asPtr(false) - } - // these values report as &false when PRs aren't required, so if they're true then PRs are required - if valueOrZero(src.RequireLastPushApproval) || valueOrZero(src.DismissesStaleReviews) { - dst.RequiredPullRequestReviews.Required = asPtr(true) - } -} - -func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { - // TODO: requiresConversationResolution, requiresSignatures, viewerAllowedToDismissReviews, viewerCanPush - switch v := src.(type) { - case *branchProtectionRule: - copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) - copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) - copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) - copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) - copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) - copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) - - // we always have the data to know if PRs are required - if dst.RequiredPullRequestReviews.Required == nil { - dst.RequiredPullRequestReviews.Required = asPtr(false) - } - // GitHub returns nil for RequiredApprovingReviewCount when PRs aren't required and non-nil when they are - // RequiresCodeOwnerReviews is &false even if PRs aren't required, so we need it to be true - if v.RequiredApprovingReviewCount != nil || valueOrZero(v.RequiresCodeOwnerReviews) { - dst.RequiredPullRequestReviews.Required = asPtr(true) - } - - case *refUpdateRule: - copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) - copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) - copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) - copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) - copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) - copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) - - // Evaluate if we have data to infer that the project requires PRs to make changes. If we don't have data, we let - // Required stay nil - if valueOrZero(v.RequiredApprovingReviewCount) > 0 || valueOrZero(v.RequiresCodeOwnerReviews) { - dst.RequiredPullRequestReviews.Required = asPtr(true) - } - } -} - func getDefaultBranchNameFrom(data *ruleSetData) string { if data == nil || data.Repository.DefaultBranchRef.Name == nil { return "" @@ -406,6 +345,49 @@ func getActiveRuleSetsFrom(data *ruleSetData) []*repoRuleSet { return ret } +const ( + ruleConditionDefaultBranch = "~DEFAULT_BRANCH" + ruleConditionAllBranches = "~ALL" + ruleDeletion = "DELETION" + ruleForcePush = "NON_FAST_FORWARD" + ruleLinear = "REQUIRED_LINEAR_HISTORY" + rulePullRequest = "PULL_REQUEST" + ruleStatusCheck = "REQUIRED_STATUS_CHECKS" +) + +func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) ([]*repoRuleSet, error) { + refName := refPrefix + name + ret := make([]*repoRuleSet, 0) +nextRule: + for _, rule := range rules { + for _, cond := range rule.Conditions.RefName.Exclude { + if match, err := fnmatch.Match(cond, refName); err != nil { + return nil, fmt.Errorf("exclude match error: %w", err) + } else if match { + continue nextRule + } + } + + for _, cond := range rule.Conditions.RefName.Include { + if cond == ruleConditionAllBranches { + ret = append(ret, rule) + break + } + if cond == ruleConditionDefaultBranch && defaultRef { + ret = append(ret, rule) + break + } + + if match, err := fnmatch.Match(cond, refName); err != nil { + return nil, fmt.Errorf("include match error: %w", err) + } else if match { + ret = append(ret, rule) + } + } + } + return ret, nil +} + func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { if data == nil { return nil @@ -454,51 +436,68 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { return branchRef } -func isPermissionsError(err error) bool { - return strings.Contains(err.Error(), "Resource not accessible") +func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionRule) { + copyBoolPtr(src.IsAdminEnforced, &dst.EnforceAdmins) + copyBoolPtr(src.RequireLastPushApproval, &dst.RequireLastPushApproval) + copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) + if src.RequiresStatusChecks != nil { + copyBoolPtr(src.RequiresStatusChecks, &dst.CheckRules.RequiresStatusChecks) + // TODO(#3255): Update when GitHub GraphQL bug is fixed + // Workaround for GitHub GraphQL bug https://github.com/orgs/community/discussions/59471 + // The setting RequiresStrictStatusChecks should tell if the branch is required + // to be up to date before merge, but it only returns the correct value if + // RequiresStatusChecks is true. If RequiresStatusChecks is false, RequiresStrictStatusChecks + // is wrongly retrieved as true. + if src.RequiresStrictStatusChecks != nil { + upToDateBeforeMerge := *src.RequiresStatusChecks && *src.RequiresStrictStatusChecks + copyBoolPtr(&upToDateBeforeMerge, &dst.CheckRules.UpToDateBeforeMerge) + } + } + // we always have the data to know if PRs are required + if dst.RequiredPullRequestReviews.Required == nil { + dst.RequiredPullRequestReviews.Required = asPtr(false) + } + // these values report as &false when PRs aren't required, so if they're true then PRs are required + if valueOrZero(src.RequireLastPushApproval) || valueOrZero(src.DismissesStaleReviews) { + dst.RequiredPullRequestReviews.Required = asPtr(true) + } } -const ( - ruleConditionDefaultBranch = "~DEFAULT_BRANCH" - ruleConditionAllBranches = "~ALL" - ruleDeletion = "DELETION" - ruleForcePush = "NON_FAST_FORWARD" - ruleLinear = "REQUIRED_LINEAR_HISTORY" - rulePullRequest = "PULL_REQUEST" - ruleStatusCheck = "REQUIRED_STATUS_CHECKS" -) +func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { + // TODO: requiresConversationResolution, requiresSignatures, viewerAllowedToDismissReviews, viewerCanPush + switch v := src.(type) { + case *branchProtectionRule: + copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) + copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) + copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) -func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) ([]*repoRuleSet, error) { - refName := refPrefix + name - ret := make([]*repoRuleSet, 0) -nextRule: - for _, rule := range rules { - for _, cond := range rule.Conditions.RefName.Exclude { - if match, err := fnmatch.Match(cond, refName); err != nil { - return nil, fmt.Errorf("exclude match error: %w", err) - } else if match { - continue nextRule - } + // we always have the data to know if PRs are required + if dst.RequiredPullRequestReviews.Required == nil { + dst.RequiredPullRequestReviews.Required = asPtr(false) + } + // GitHub returns nil for RequiredApprovingReviewCount when PRs aren't required and non-nil when they are + // RequiresCodeOwnerReviews is &false even if PRs aren't required, so we need it to be true + if v.RequiredApprovingReviewCount != nil || valueOrZero(v.RequiresCodeOwnerReviews) { + dst.RequiredPullRequestReviews.Required = asPtr(true) } - for _, cond := range rule.Conditions.RefName.Include { - if cond == ruleConditionAllBranches { - ret = append(ret, rule) - break - } - if cond == ruleConditionDefaultBranch && defaultRef { - ret = append(ret, rule) - break - } + case *refUpdateRule: + copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) + copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) + copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) - if match, err := fnmatch.Match(cond, refName); err != nil { - return nil, fmt.Errorf("include match error: %w", err) - } else if match { - ret = append(ret, rule) - } + // Evaluate if we have data to infer that the project requires PRs to make changes. If we don't have data, we let + // Required stay nil + if valueOrZero(v.RequiredApprovingReviewCount) > 0 || valueOrZero(v.RequiresCodeOwnerReviews) { + dst.RequiredPullRequestReviews.Required = asPtr(true) } } - return ret, nil } func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) {