mirror of
https://github.com/ossf/scorecard.git
synced 2024-09-11 08:55:27 +03:00
🌱 Add branch protection probe evaluation (#3759)
* 🌱 Add branch protection evaluation
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* make helper for getting the branchName
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* move check for branch name
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* define size of slice
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* add probe for protected branches.
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* change 'basicNonAdminProtection' to 'deleteAndForcePushProtection'
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* fix markdown in text field in def.yml
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* remove duplicate conditional
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* remove redundant 'protected' value from 'requiresCodeOwnersReview' probe
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* remove protected values from probes
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* Bring back negative outcome in case of 0 codeowners files
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* log based on whether branches are protected
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* remove unnecessary test
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* debug failing tests
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* Fix failing tests
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* rename test
Signed-off-by: Adam Korczynski <adam@adalogics.com>
* update to with latest upstream changes
Signed-off-by: AdamKorcz <adam@adalogics.com>
* fix linting issues
Signed-off-by: AdamKorcz <adam@adalogics.com>
* remove tests that represent impossible scenarios
Signed-off-by: AdamKorcz <adam@adalogics.com>
* remove protected finding value
This was discussed previously, but accidentally reverted
Signed-off-by: Spencer Schrock <sschrock@google.com>
* Revert "debug failing tests"
This reverts commit 00acf66ea6
.
Signed-off-by: Spencer Schrock <sschrock@google.com>
* use branchName key for branch name
Signed-off-by: Spencer Schrock <sschrock@google.com>
* include number of reviews in INFO
this was previously included by the old evaluation code
Signed-off-by: Spencer Schrock <sschrock@google.com>
* reduce info count by 1
requiring codeowners without a corresponding file used to give 1 INFO and 1 WARN
now it only gives 1 WARN
Signed-off-by: Spencer Schrock <sschrock@google.com>
---------
Signed-off-by: Adam Korczynski <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Co-authored-by: Spencer Schrock <sschrock@google.com>
This commit is contained in:
parent
4ae4ba246c
commit
4daefb64ae
@ -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)
|
||||
}
|
||||
|
@ -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,
|
||||
|
@ -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 {
|
||||
dl.Warn(&checker.LogMessage{
|
||||
Text: fmt.Sprintf("branch protection not enabled for branch '%s'", *b.Name),
|
||||
})
|
||||
}
|
||||
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)
|
||||
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 len(scores) == 0 {
|
||||
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'", branchName),
|
||||
})
|
||||
case f.Outcome == finding.OutcomePositive:
|
||||
protectedBranches[branchName] = true
|
||||
default:
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
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++
|
||||
}
|
||||
max++
|
||||
return score, max
|
||||
}
|
||||
|
||||
func adminReviewProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) {
|
||||
var score, max int
|
||||
if f.Outcome == finding.OutcomePositive {
|
||||
score++
|
||||
}
|
||||
switch f.Probe {
|
||||
case requiresLastPushApproval.Probe,
|
||||
requiresUpToDateBranches.Probe:
|
||||
logWithDebug(f, doLogging, dl)
|
||||
if f.Outcome != finding.OutcomeNotAvailable {
|
||||
max++
|
||||
}
|
||||
default:
|
||||
warn(dl, log, "no status checks found to merge onto branch '%s'", *branch.Name)
|
||||
}
|
||||
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
|
||||
}
|
||||
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`.
|
||||
logInfoOrWarn(f, doLogging, dl)
|
||||
max++
|
||||
if *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge {
|
||||
info(dl, log, "status checks require up-to-date branches for '%s'", *branch.Name)
|
||||
}
|
||||
return score, max
|
||||
}
|
||||
|
||||
func adminThoroughReviewProtection(f *finding.Finding, doLogging bool, dl checker.DetailLogger) (int, int) {
|
||||
var score, max int
|
||||
|
||||
logWithDebug(f, doLogging, dl)
|
||||
if f.Outcome == finding.OutcomePositive {
|
||||
score++
|
||||
}
|
||||
if f.Outcome != finding.OutcomeNotAvailable {
|
||||
max++
|
||||
}
|
||||
return score, max
|
||||
}
|
||||
|
||||
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++
|
||||
} else {
|
||||
warn(dl, log, "status checks do not require up-to-date branches for '%s'", *branch.Name)
|
||||
warn(dl, doLogging, f.Message)
|
||||
}
|
||||
} else if f.Outcome == finding.OutcomeNegative {
|
||||
warn(dl, doLogging, f.Message)
|
||||
}
|
||||
|
||||
// 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 {
|
||||
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
|
||||
|
||||
if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
|
||||
// Note: we don't increase max possible score for non-admin viewers.
|
||||
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)
|
||||
}
|
||||
|
||||
// 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)
|
||||
score++
|
||||
case false:
|
||||
warn(dl, log, "settings do not apply to administrators on branch '%s'", *branch.Name)
|
||||
}
|
||||
} else {
|
||||
debug(dl, log, "unable to retrieve whether or not settings apply to administrators on branch '%s'", *branch.Name)
|
||||
}
|
||||
|
||||
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)
|
||||
}
|
||||
|
||||
max++
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
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
|
||||
}
|
||||
|
File diff suppressed because it is too large
Load Diff
@ -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),
|
||||
}
|
||||
|
@ -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()
|
||||
|
@ -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)
|
||||
|
@ -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]
|
||||
|
||||
|
@ -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 {
|
||||
|
@ -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]
|
||||
|
||||
|
30
probes/branchesAreProtected/def.yml
Normal file
30
probes/branchesAreProtected/def.yml
Normal file
@ -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)
|
73
probes/branchesAreProtected/impl.go
Normal file
73
probes/branchesAreProtected/impl.go
Normal file
@ -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
|
||||
}
|
169
probes/branchesAreProtected/impl_test.go
Normal file
169
probes/branchesAreProtected/impl_test.go
Normal file
@ -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)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
@ -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]
|
||||
|
||||
|
@ -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,
|
||||
}
|
||||
|
@ -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)
|
||||
|
@ -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
|
||||
|
@ -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{
|
||||
|
@ -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]
|
||||
|
||||
|
32
probes/requiresPRsToChangeCode/def.yml
Normal file
32
probes/requiresPRsToChangeCode/def.yml
Normal file
@ -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/).
|
86
probes/requiresPRsToChangeCode/impl.go
Normal file
86
probes/requiresPRsToChangeCode/impl.go
Normal file
@ -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
|
||||
}
|
202
probes/requiresPRsToChangeCode/impl_test.go
Normal file
202
probes/requiresPRsToChangeCode/impl_test.go
Normal file
@ -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)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
@ -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]
|
||||
|
||||
|
@ -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
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user