diff --git a/checker/raw_result.go b/checker/raw_result.go index c2789927051..49627be8fed 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -252,7 +252,8 @@ type WebhooksData struct { // BranchProtectionsData contains the raw results // for the Branch-Protection check. type BranchProtectionsData struct { - Branches []clients.BranchRef + Branches []clients.BranchRef + CodeownersFiles []string } // Tool represents a tool. diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index 699a097a663..34bf77d6283 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -73,6 +73,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { defaultBranch string releases []string nonadmin bool + repoFiles []string }{ { name: "Nil release and main branch names", @@ -172,7 +173,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 7, + NumberOfWarn: 8, NumberOfInfo: 9, NumberOfDebug: 0, }, @@ -226,7 +227,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 8, - NumberOfWarn: 2, + NumberOfWarn: 4, NumberOfInfo: 14, NumberOfDebug: 0, }, @@ -412,6 +413,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { DoAndReturn(func(b string) (*clients.BranchRef, error) { return getBranch(tt.branches, b, tt.nonadmin), nil }).AnyTimes() + mockRepoClient.EXPECT().ListFiles(gomock.Any()).AnyTimes().Return(tt.repoFiles, nil) dl := scut.TestDetailLogger{} req := checker.CheckRequest{ Dlogger: &dl, diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 939426b0725..fd50d4c4663 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -78,7 +78,7 @@ func BranchProtection(name string, dl checker.DetailLogger, 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 = codeownersBranchProtection(&b, dl) + score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(&b, r.CodeownersFiles, dl) scores = append(scores, score) } @@ -436,7 +436,9 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta return score, max } -func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) { +func codeownerBranchProtection( + branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger, +) (int, int) { score := 0 max := 1 @@ -446,7 +448,11 @@ func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogg switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews { case true: info(dl, log, "codeowner review is required on branch '%s'", *branch.Name) - score++ + 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) } diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index 4bf1ca64ba9..4f7b3b18f86 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -22,7 +22,7 @@ import ( scut "github.com/ossf/scorecard/v4/utests" ) -func testScore(branch *clients.BranchRef, dl checker.DetailLogger) (int, error) { +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.adminBasic, score.maxes.adminBasic = basicAdminProtection(branch, dl) @@ -31,7 +31,7 @@ func testScore(branch *clients.BranchRef, dl checker.DetailLogger) (int, error) 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 = codeownersBranchProtection(branch, dl) + score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(branch, codeownersFiles, dl) return computeScore([]levelScore{score}) } @@ -44,9 +44,10 @@ func TestIsBranchProtected(t *testing.T) { var oneVal int32 = 1 branchVal := "branch-name" tests := []struct { - name string - branch *clients.BranchRef - expected scut.TestReturn + name string + branch *clients.BranchRef + codeownersFiles []string + expected scut.TestReturn }{ { name: "Nothing is enabled", @@ -308,7 +309,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 8, - NumberOfWarn: 1, + NumberOfWarn: 2, NumberOfInfo: 7, NumberOfDebug: 0, }, @@ -335,11 +336,42 @@ func TestIsBranchProtected(t *testing.T) { }, { name: "Branches are protected and require codeowner review", + expected: scut.TestReturn{ + Error: nil, + Score: 8, + NumberOfWarn: 1, + NumberOfInfo: 7, + NumberOfDebug: 0, + }, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, + CheckRules: clients.StatusChecksRule{ + RequiresStatusChecks: &trueVal, + UpToDateBeforeMerge: &trueVal, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &trueVal, + RequireCodeOwnerReviews: &trueVal, + RequiredApprovingReviewCount: &oneVal, + }, + }, + }, + codeownersFiles: []string{".github/CODEOWNERS"}, + }, + { + name: "Branches are protected and require codeowner review, but file is not present", expected: scut.TestReturn{ Error: nil, Score: 8, NumberOfWarn: 2, - NumberOfInfo: 6, + NumberOfInfo: 7, NumberOfDebug: 0, }, branch: &clients.BranchRef{ @@ -357,7 +389,7 @@ func TestIsBranchProtected(t *testing.T) { }, RequiredPullRequestReviews: clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, - RequireCodeOwnerReviews: &falseVal, + RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, }, @@ -369,7 +401,7 @@ func TestIsBranchProtected(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() dl := scut.TestDetailLogger{} - score, err := testScore(tt.branch, &dl) + score, err := testScore(tt.branch, tt.codeownersFiles, &dl) actual := &checker.CheckResult{ Score: score, Error: err, diff --git a/checks/raw/branch_protection.go b/checks/raw/branch_protection.go index f777e498d28..c66d9e96225 100644 --- a/checks/raw/branch_protection.go +++ b/checks/raw/branch_protection.go @@ -16,9 +16,11 @@ package raw import ( "fmt" + "reflect" "regexp" "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/checks/fileparser" "github.com/ossf/scorecard/v4/clients" ) @@ -105,12 +107,54 @@ func BranchProtection(c clients.RepoClient) (checker.BranchProtectionsData, erro // Branch doesn't exist or was deleted. Continue. } + codeownersFiles := []string{} + if err := collectCodeownersFiles(c, &codeownersFiles); err != nil { + return checker.BranchProtectionsData{}, err + } + // No error, return the data. return checker.BranchProtectionsData{ - Branches: branches.set, + Branches: branches.set, + CodeownersFiles: codeownersFiles, }, nil } +func collectCodeownersFiles(c clients.RepoClient, codeownersFiles *[]string) error { + return fileparser.OnMatchingFileContentDo(c, fileparser.PathMatcher{ + Pattern: "CODEOWNERS", + CaseSensitive: true, + }, addCodeownersFile, codeownersFiles) +} + +var addCodeownersFile fileparser.DoWhileTrueOnFileContent = func( + path string, + content []byte, + args ...interface{}, +) (bool, error) { + fmt.Printf("got codeowners file at %s\n", path) + + if len(args) != 1 { + return false, fmt.Errorf( + "addCodeownersFile requires exactly 1 arguments: got %v: %w", + len(args), errInvalidArgLength) + } + + codeownersFiles := dataAsStringSlicePtr(args[0]) + + *codeownersFiles = append(*codeownersFiles, path) + + return true, nil +} + +func dataAsStringSlicePtr(data interface{}) *[]string { + pdata, ok := data.(*[]string) + if !ok { + // panic if it is not correct type + panic(fmt.Sprintf("expected type *[]string, got %v", reflect.TypeOf(data))) + } + return pdata +} + func branchRedirect(name string) string { // Ideally, we should check using repositories.GetBranch if there was a branch redirect. // See https://github.com/google/go-github/issues/1895 diff --git a/checks/raw/branch_protection_test.go b/checks/raw/branch_protection_test.go index e477b079678..1ae134d4155 100644 --- a/checks/raw/branch_protection_test.go +++ b/checks/raw/branch_protection_test.go @@ -67,6 +67,7 @@ func TestBranchProtection(t *testing.T) { tests := []struct { name string branches branchesArg + repoFiles []string releases []clients.Release releasesErr error want checker.BranchProtectionsData @@ -80,6 +81,9 @@ func TestBranchProtection(t *testing.T) { err: errBPTest, }, }, + want: checker.BranchProtectionsData{ + CodeownersFiles: []string{}, + }, }, { name: "null-default-branch-only", @@ -90,6 +94,9 @@ func TestBranchProtection(t *testing.T) { branchRef: nil, }, }, + want: checker.BranchProtectionsData{ + CodeownersFiles: []string{}, + }, }, { name: "default-branch-only", @@ -108,6 +115,7 @@ func TestBranchProtection(t *testing.T) { Name: &defaultBranchName, }, }, + CodeownersFiles: []string{}, }, }, { @@ -117,6 +125,9 @@ func TestBranchProtection(t *testing.T) { }, { name: "no-releases", + want: checker.BranchProtectionsData{ + CodeownersFiles: []string{}, + }, }, { name: "empty-targetcommitish", @@ -155,6 +166,9 @@ func TestBranchProtection(t *testing.T) { branchRef: nil, }, }, + want: checker.BranchProtectionsData{ + CodeownersFiles: []string{}, + }, }, { name: "add-release-branch", @@ -177,6 +191,7 @@ func TestBranchProtection(t *testing.T) { Name: &releaseBranchName, }, }, + CodeownersFiles: []string{}, }, }, { @@ -200,6 +215,7 @@ func TestBranchProtection(t *testing.T) { Name: &mainBranchName, }, }, + CodeownersFiles: []string{}, }, }, { @@ -233,6 +249,7 @@ func TestBranchProtection(t *testing.T) { Name: &releaseBranchName, }, }, + CodeownersFiles: []string{}, }, }, // TODO: Add tests for commitSHA regex matching. @@ -255,7 +272,7 @@ func TestBranchProtection(t *testing.T) { DoAndReturn(func() ([]clients.Release, error) { return tt.releases, tt.releasesErr }) - + mockRepoClient.EXPECT().ListFiles(gomock.Any()).AnyTimes().Return(tt.repoFiles, nil) rawData, err := BranchProtection(mockRepoClient) if !errors.Is(err, tt.wantErr) { t.Errorf("failed. expected: %v, got: %v", tt.wantErr, err) diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index 0fcaa5a6191..b1ef2020dfb 100644 --- a/e2e/branch_protection_test.go +++ b/e2e/branch_protection_test.go @@ -47,7 +47,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { expected := scut.TestReturn{ Error: nil, Score: 6, - NumberOfWarn: 1, + NumberOfWarn: 2, NumberOfInfo: 4, NumberOfDebug: 3, } diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index a9b2cb48e3d..43141f01564 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -73,6 +73,11 @@ type jsonBranchProtection struct { Name string `json:"name"` } +type jsonBranchProtectionMetadata struct { + Branches []jsonBranchProtection `json:"branches"` + CodeownersFiles []string `json:"codeownersFiles"` +} + type jsonReview struct { State string `json:"state"` Reviewer jsonUser `json:"reviewer"` @@ -265,7 +270,7 @@ type jsonRawResults struct { // Note: we return one at most. DependencyUpdateTools []jsonTool `json:"dependencyUpdateTools"` // Branch protection settings for development and release branches. - BranchProtections []jsonBranchProtection `json:"branchProtections"` + BranchProtections jsonBranchProtectionMetadata `json:"branchProtections"` // Contributors. Note: we could use the list of commits instead to store this data. // However, it's harder to get statistics using commit list, so we have a dedicated // structure for it. @@ -711,7 +716,7 @@ func (r *jsonScorecardRawResult) addDependencyUpdateToolRawResults(dut *checker. //nolint:unparam func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.BranchProtectionsData) error { - r.Results.BranchProtections = []jsonBranchProtection{} + branches := []jsonBranchProtection{} for _, v := range bp.Branches { var bp *jsonBranchProtectionSettings if v.Protected != nil && *v.Protected { @@ -728,11 +733,15 @@ func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.Branc StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts, } } - r.Results.BranchProtections = append(r.Results.BranchProtections, jsonBranchProtection{ + branches = append(branches, jsonBranchProtection{ Name: *v.Name, Protection: bp, }) } + r.Results.BranchProtections.Branches = branches + + r.Results.BranchProtections.CodeownersFiles = bp.CodeownersFiles + return nil }