diff --git a/checker/raw_result.go b/checker/raw_result.go index c2789927051c..49627be8fed3 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 699a097a6638..929c20768c87 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", @@ -134,7 +135,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -158,10 +159,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLinearHistory: &falseVal, + RequireLastPushApproval: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -172,8 +174,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 7, - NumberOfInfo: 9, + NumberOfWarn: 9, + NumberOfInfo: 10, NumberOfDebug: 0, }, defaultBranch: main, @@ -192,10 +194,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, { @@ -212,10 +215,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -226,8 +230,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 8, - NumberOfWarn: 2, - NumberOfInfo: 14, + NumberOfWarn: 4, + NumberOfInfo: 16, NumberOfDebug: 0, }, defaultBranch: main, @@ -246,10 +250,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, { @@ -266,10 +271,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -280,7 +286,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -301,10 +307,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, { Name: &rel1, @@ -353,7 +360,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Score: 0, NumberOfWarn: 4, NumberOfInfo: 0, - NumberOfDebug: 6, + NumberOfDebug: 8, }, nonadmin: true, defaultBranch: main, @@ -412,6 +419,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 939426b0725b..7703cab920c5 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) } @@ -373,18 +373,31 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) ( // Only log information if the branch is protected. log := branch.Protected != nil && *branch.Protected - if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge != nil { + // Process UpToDateBeforeMerge value. + if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge == nil { + debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name) + } else { // Note: `This setting will not take effect unless at least one status check is enabled`. max++ - switch *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge { - case true: + if *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge { info(dl, log, "status checks require up-to-date branches for '%s'", *branch.Name) score++ - default: + } else { warn(dl, log, "status checks do not require up-to-date branches for '%s'", *branch.Name) } + } + + // 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 { - debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name) + 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) + } } return score, max @@ -401,10 +414,10 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL max++ switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews { case true: - info(dl, log, "Stale review dismissal enabled on branch '%s'", *branch.Name) + 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) + 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) @@ -436,7 +449,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 +461,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 4bf1ca64ba93..5fd5e541edbe 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,16 +44,17 @@ 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", expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -61,10 +62,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &falseVal, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, RequiredPullRequestReviews: clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, @@ -85,7 +87,7 @@ func TestIsBranchProtected(t *testing.T) { Score: 0, NumberOfWarn: 2, NumberOfInfo: 0, - NumberOfDebug: 3, + NumberOfDebug: 4, }, branch: &clients.BranchRef{ Name: &branchVal, @@ -97,7 +99,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 4, + NumberOfWarn: 5, NumberOfInfo: 4, NumberOfDebug: 0, }, @@ -115,10 +117,11 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -127,7 +130,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 5, + NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, }, @@ -135,10 +138,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, RequiredPullRequestReviews: clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, @@ -157,7 +161,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 5, + NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, }, @@ -165,10 +169,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &trueVal, UpToDateBeforeMerge: &falseVal, @@ -187,7 +192,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 3, - NumberOfWarn: 4, + NumberOfWarn: 5, NumberOfInfo: 4, NumberOfDebug: 0, }, @@ -195,10 +200,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &falseVal, @@ -217,7 +223,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 5, + NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, }, @@ -225,10 +231,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &falseVal, @@ -247,7 +254,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 1, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -255,10 +262,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &trueVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &trueVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, @@ -278,7 +286,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 1, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -286,10 +294,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &trueVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &trueVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &falseVal, @@ -308,18 +317,19 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 8, - NumberOfWarn: 1, - NumberOfInfo: 7, + NumberOfWarn: 2, + NumberOfInfo: 8, NumberOfDebug: 0, }, branch: &clients.BranchRef{ Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + RequireLastPushApproval: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &trueVal, @@ -338,18 +348,51 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 8, - NumberOfWarn: 2, - NumberOfInfo: 6, + NumberOfWarn: 1, + NumberOfInfo: 8, NumberOfDebug: 0, }, branch: &clients.BranchRef{ Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + RequireLastPushApproval: &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: 5, + NumberOfWarn: 3, + NumberOfInfo: 7, + NumberOfDebug: 0, + }, + branch: &clients.BranchRef{ + Name: &branchVal, + Protected: &trueVal, + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &trueVal, @@ -357,7 +400,7 @@ func TestIsBranchProtected(t *testing.T) { }, RequiredPullRequestReviews: clients.PullRequestReviewRule{ DismissStaleReviews: &trueVal, - RequireCodeOwnerReviews: &falseVal, + RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, }, @@ -369,7 +412,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 f777e498d286..c66d9e96225e 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 e477b0796787..1ae134d4155e 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/checks/raw/shell_download_validate.go b/checks/raw/shell_download_validate.go index 2dfc020a1f67..39b3293be26b 100644 --- a/checks/raw/shell_download_validate.go +++ b/checks/raw/shell_download_validate.go @@ -425,7 +425,6 @@ func isGoUnpinnedDownload(cmd []string) bool { if !isBinaryName("go", cmd[0]) { return false } - // `Go install` will automatically look up the // go.mod and go.sum, so we don't flag it. if len(cmd) <= 2 { @@ -456,6 +455,10 @@ func isGoUnpinnedDownload(cmd []string) bool { i++ } + if i+1 >= len(cmd) { + // this is case go get -d -v + return false + } // TODO check more than one package pkg := cmd[i+1] // Consider strings that are not URLs as local folders diff --git a/checks/raw/shell_download_validate_test.go b/checks/raw/shell_download_validate_test.go index b40912f46b7c..a38115aad349 100644 --- a/checks/raw/shell_download_validate_test.go +++ b/checks/raw/shell_download_validate_test.go @@ -106,3 +106,36 @@ func TestValidateShellFile(t *testing.T) { t.Errorf("failed to detect shell parsing error: %v", err) } } + +func Test_isGoUnpinnedDownload(t *testing.T) { + type args struct { + cmd []string + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "go get", + args: args{ + cmd: []string{"go", "get", "github.com/ossf/scorecard"}, + }, + want: true, + }, + { + name: "go get with -d -v", + args: args{ + cmd: []string{"go", "get", "-d", "-v"}, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isGoUnpinnedDownload(tt.args.cmd); got != tt.want { + t.Errorf("isGoUnpinnedDownload() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/clients/branch.go b/clients/branch.go index 0481a893fdda..ff11e4a683e0 100644 --- a/clients/branch.go +++ b/clients/branch.go @@ -28,6 +28,7 @@ type BranchProtectionRule struct { AllowForcePushes *bool RequireLinearHistory *bool EnforceAdmins *bool + RequireLastPushApproval *bool CheckRules StatusChecksRule } diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 8250929ecab5..027ca56be197 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -36,19 +36,20 @@ const ( query { repository(owner: "laurentsimon", name: "test3") { branchProtectionRules(first: 100) { - allowsDeletions - allowsForcePushes - dismissesStaleReviews - isAdminEnforced - ... - pattern - matchingRefs(first: 100) { - nodes { - name - - } - } - } + edges{ + node{ + allowsDeletions + allowsForcePushes + dismissesStaleReviews + isAdminEnforced + ... + pattern + matchingRefs(first: 100) { + nodes { + name + } + } + } } refs(first: 100, refPrefix: "refs/heads/") { nodes { @@ -86,6 +87,7 @@ type branchProtectionRule struct { RequiredApprovingReviewCount *int32 RequiresCodeOwnerReviews *bool RequiresLinearHistory *bool + RequireLastPushApproval *bool RequiredStatusCheckContexts []string // TODO: verify there is no conflicts. // BranchProtectionRuleConflicts interface{} @@ -184,6 +186,7 @@ func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, er 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) diff --git a/docs/checks.md b/docs/checks.md index 1056304005b4..3d7d9a0f4893 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -105,7 +105,8 @@ Tier 1 Requirements (3/10 points): - For administrators: Include administrator for review Tier 2 Requirements (6/10 points): - - Required reviewers >=1 ​ + - Required reviewers >=1 + - For administrators: Last push review - For administrators: Strict status checks (require branches to be up-to-date before merging) Tier 3 Requirements (8/10 points): diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 3e28e405c44f..fe7477c2abc3 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -194,7 +194,8 @@ checks: - For administrators: Include administrator for review Tier 2 Requirements (6/10 points): - - Required reviewers >=1 ​ + - Required reviewers >=1 + - For administrators: Last push review - For administrators: Strict status checks (require branches to be up-to-date before merging) Tier 3 Requirements (8/10 points): diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index 0fcaa5a61917..7a3862733469 100644 --- a/e2e/branch_protection_test.go +++ b/e2e/branch_protection_test.go @@ -47,9 +47,9 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { expected := scut.TestReturn{ Error: nil, Score: 6, - NumberOfWarn: 1, + NumberOfWarn: 2, NumberOfInfo: 4, - NumberOfDebug: 3, + NumberOfDebug: 4, } result := checks.BranchProtection(&req) // UPGRADEv2: to remove. @@ -84,10 +84,10 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { result := checks.BranchProtection(&req) // New version. - Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue()) + Expect(scut.ValidateTestReturn(nil, "branch protection accessible none", &expected, &result, &dl)).Should(BeTrue()) Expect(repoClient.Close()).Should(BeNil()) }) - It("Should fail to return branch protection on other repositories", func() { + It("Should fail to return branch protection on other repositories patch", func() { skipIfTokenIsNot(patTokenType, "PAT only") dl := scut.TestDetailLogger{} @@ -107,12 +107,12 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { Score: 1, NumberOfWarn: 3, NumberOfInfo: 3, - NumberOfDebug: 3, + NumberOfDebug: 4, } result := checks.BranchProtection(&req) // New version. - Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue()) + Expect(scut.ValidateTestReturn(nil, "branch protection accessible patch", &expected, &result, &dl)).Should(BeTrue()) Expect(repoClient.Close()).Should(BeNil()) }) }) diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index af4ebaacb083..fcbe461c2aa0 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"` @@ -266,7 +271,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. @@ -700,7 +705,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 { @@ -717,11 +722,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 }