From 62031e55233bbcded70399886cafbbd6b5736f60 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sun, 6 Aug 2023 08:44:19 -0400 Subject: [PATCH 01/22] repo rulesets via v4 api Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 164 +++++++++++++++++++++++++++++---- 1 file changed, 144 insertions(+), 20 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 5779d72c110..d09e2f43661 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -33,23 +33,23 @@ const ( // See https://github.community/t/graphql-api-protected-branch/14380 /* Example of query: - query { +{ repository(owner: "laurentsimon", name: "test3") { branchProtectionRules(first: 100) { - edges{ - node{ - 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 { @@ -57,7 +57,36 @@ const ( refUpdateRule { requiredApprovingReviewCount allowsForcePushes - ... + } + } + } + rulesets(first: 100) { + edges { + node { + name + enforcement + target + conditions { + refName { + exclude + include + } + } + pullRequestRules: rules(first: 100, type: PULL_REQUEST) { + edges { + node { + parameters { + ... on PullRequestParameters { + dismissStaleReviewsOnPush + requireCodeOwnerReview + requireLastPushApproval + requiredApprovingReviewCount + requiredReviewThreadResolution + } + } + } + } + } } } } @@ -107,6 +136,48 @@ type defaultBranchData struct { } } +type pullRequestRuleParameters struct { + DismissStaleReviewsOnPush *bool + RequireCodeOwnerReview *bool + RequireLastPushApproval *bool + RequiredApprovingReviewCount *int32 + RequiredReviewThreadResolution *bool +} +type pullRequestRules struct { + Node struct { + Parameters struct { + PullRequestParameters pullRequestRuleParameters `graphql:"... on PullRequestParameters"` + } + } +} +type ruleSetCondition struct { + RefName struct { + Include []string + Exclude []string + } +} + +type repoRuleSet struct { + Name *string + Enforcement *string + Target *string + Conditions *ruleSetCondition + PullRequestRules struct { + Edges []*pullRequestRules + } `graphql:"rules(first: 100, type: PULL_REQUEST)"` +} + +type ruleSetData struct { + Repository struct { + Rulesets struct { + Nodes []*repoRuleSet + } `graphql:"rulesets(first: 100)"` + } `graphql:"repository(owner: $owner, name: $name)"` + RateLimit struct { + Cost *int + } +} + type branchData struct { Repository struct { Ref *branch `graphql:"ref(qualifiedName: $branchRefName)"` @@ -122,6 +193,7 @@ type branchesHandler struct { errSetup error repourl *repoURL defaultBranchRef *clients.BranchRef + ruleSets []*repoRuleSet } func (handler *branchesHandler) init(ctx context.Context, repourl *repoURL) { @@ -143,12 +215,23 @@ func (handler *branchesHandler) setup() error { "owner": githubv4.String(handler.repourl.owner), "name": githubv4.String(handler.repourl.repo), } + + rulesData := new(ruleSetData) + if err := handler.graphClient.Query(handler.ctx, rulesData, vars); err != nil { + handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) + return + } + handler.ruleSets = rulesData.Repository.Rulesets.Nodes + handler.data = new(defaultBranchData) - if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil { + // If the repository is using RuleSets, ignore permissions errors on branch protection as that requires admin. + if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil && len(handler.ruleSets) > 0 && isPermissionsError(err) { handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) return } - handler.defaultBranchRef = getBranchRefFrom(handler.data.Repository.DefaultBranchRef) + + rules := rulesMatchingBranch(handler.ruleSets, *handler.data.Repository.DefaultBranchRef.Name, true) + handler.defaultBranchRef = getBranchRefFrom(handler.data.Repository.DefaultBranchRef, rules) }) return handler.errSetup } @@ -166,7 +249,8 @@ func (handler *branchesHandler) query(branchName string) (*clients.BranchRef, er if err := handler.graphClient.Query(handler.ctx, queryData, vars); err != nil { return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) } - return getBranchRefFrom(queryData.Repository.Ref), nil + rules := rulesMatchingBranch(handler.ruleSets, branchName, branchName == *handler.data.Repository.DefaultBranchRef.Name) + return getBranchRefFrom(queryData.Repository.Ref, rules), nil } func (handler *branchesHandler) getDefaultBranch() (*clients.BranchRef, error) { @@ -177,6 +261,7 @@ func (handler *branchesHandler) getDefaultBranch() (*clients.BranchRef, error) { } func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, error) { + // TODO: has setup() been called yet? current implementation assumes yes. branchRef, err := handler.query(branch) if err != nil { return nil, fmt.Errorf("error during branchesHandler.query: %w", err) @@ -224,7 +309,7 @@ func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { } } -func getBranchRefFrom(data *branch) *clients.BranchRef { +func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { if data == nil { return nil } @@ -239,6 +324,7 @@ func getBranchRefFrom(data *branch) *clients.BranchRef { branchRef.Protected = new(bool) if data.RefUpdateRule == nil && data.BranchProtectionRule == nil { + // TODO: consider rules that match the branch here. *branchRef.Protected = false return branchRef } @@ -268,3 +354,41 @@ func getBranchRefFrom(data *branch) *clients.BranchRef { return branchRef } + +func isPermissionsError(err error) bool { + return strings.Contains(err.Error(), "Resource not accessible by personal access token") +} + +func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) []*repoRuleSet { + ret := make([]*repoRuleSet, 0) +nextRule: + for _, rule := range rules { + if rule.Enforcement == nil || *rule.Enforcement != "ACTIVE" { + continue + } + + if rule.Conditions == nil { + // TODO: is this even possible? + ret = append(ret, rule) + continue + } + + // TODO: confirm this behaviour: + for _, cond := range rule.Conditions.RefName.Exclude { + if cond == name { + continue nextRule + } + if cond == "~DEFAULT_BRANCH" && defaultRef { + continue nextRule + } + } + + for _, cond := range rule.Conditions.RefName.Include { + if cond == name { + ret = append(ret, rule) + } + + } + } + return ret +} From 3af4e0359ab8d6795ea00e4519b391b9df302e99 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 3 Aug 2023 18:08:42 -0700 Subject: [PATCH 02/22] good enough fnmatch implementation. Signed-off-by: Spencer Schrock --- .../githubrepo/internal/fnmatch/fnmatch.go | 80 +++++++++++++++++++ .../internal/fnmatch/fnmatch_test.go | 52 ++++++++++++ 2 files changed, 132 insertions(+) create mode 100644 clients/githubrepo/internal/fnmatch/fnmatch.go create mode 100644 clients/githubrepo/internal/fnmatch/fnmatch_test.go diff --git a/clients/githubrepo/internal/fnmatch/fnmatch.go b/clients/githubrepo/internal/fnmatch/fnmatch.go new file mode 100644 index 00000000000..8a957500630 --- /dev/null +++ b/clients/githubrepo/internal/fnmatch/fnmatch.go @@ -0,0 +1,80 @@ +package fnmatch + +import ( + "fmt" + "regexp" + "strings" +) + +func Match(pattern, path string) (bool, error) { + r := convertToRegex(pattern) + m, err := regexp.MatchString(r, path) + if err != nil { + return false, fmt.Errorf("converted regex invalid: %w", err) + } + return m, nil +} + +var specialRegexpChars = map[byte]struct{}{ + '.': {}, + '+': {}, + '*': {}, + '?': {}, + '^': {}, + '$': {}, + '(': {}, + ')': {}, + '[': {}, + ']': {}, + '{': {}, + '}': {}, + '|': {}, + '\\': {}, +} + +func convertToRegex(pattern string) string { + var regexPattern strings.Builder + regexPattern.WriteRune('^') + for len(pattern) > 0 { + matchLen := 1 + switch { + case len(pattern) > 2 && pattern[:3] == "**/": + // Matches directories recursively + regexPattern.WriteString("(?:[^/]+/?)+") + matchLen = 3 + case len(pattern) > 1 && pattern[:2] == "**": + // Matches files expansively. + regexPattern.WriteString("[^/]+") + matchLen = 2 + case len(pattern) > 1 && pattern[:1] == "\\": + writePotentialRegexpChar(®exPattern, pattern[1]) + matchLen = 2 + default: + switch pattern[0] { + case '*': + // Equivalent to ".*"" in regexp, but GitHub uses the File::FNM_PATHNAME flag for the File.fnmatch syntax + // the * wildcard does not match directory separators (/). + regexPattern.WriteString("[^/]*") + case '?': + // Matches any one character. Equivalent to ".{1}" in regexp, see FNM_PATHNAME note above. + regexPattern.WriteString("[^/]{1}") + case '[', ']': + // "[" and "]" represent character sets in fnmatch too + regexPattern.WriteByte(pattern[0]) + default: + writePotentialRegexpChar(®exPattern, pattern[0]) + } + } + pattern = pattern[matchLen:] + } + regexPattern.WriteRune('$') + return regexPattern.String() +} + +// Characters with special meaning in regexp may need escaped. +func writePotentialRegexpChar(sb *strings.Builder, b byte) { + if _, ok := specialRegexpChars[b]; ok { + sb.WriteRune('\\') + } + sb.WriteByte(b) +} diff --git a/clients/githubrepo/internal/fnmatch/fnmatch_test.go b/clients/githubrepo/internal/fnmatch/fnmatch_test.go new file mode 100644 index 00000000000..20bda25529d --- /dev/null +++ b/clients/githubrepo/internal/fnmatch/fnmatch_test.go @@ -0,0 +1,52 @@ +package fnmatch + +import ( + "testing" +) + +func TestMatch(t *testing.T) { + tests := []struct { + pattern string + path string + want bool + }{ + // Taken from https://ruby-doc.org/core-2.5.1/File.html#method-c-fnmatch, assumes File::FNM_PATHNAME + {"cat", "cat", true}, + {"cat", "category", false}, + {"c?t", "cat", true}, + {"c??t", "cat", false}, + {"c*", "cats", true}, + {"ca[a-z]", "cat", true}, + {"cat", "CAT", false}, + {"?", "/", false}, + {"*", "/", false}, + + {"\\?", "?", true}, + {"\\a", "a", true}, + + {"foo.bar", "foo.bar", true}, + {"foo.bar", "foo:bar", false}, + + {"**.rb", "main.rb", true}, + {"**.rb", "./main.rb", false}, + {"**.rb", "lib/song.rb", false}, + + {"**/foo", "a/b/c/foo", true}, + {"**foo", "a/b/c/foo", false}, + + {"main", "main", true}, + {"releases/**/*", "releases/v2", true}, + {"releases/**/**/*", "releases/v2", true}, + {"releases/**/bar/**/qux", "releases/foo/bar/baz/qux", true}, + {"users/**/*", "users/foo/bar", true}, + } + for _, tt := range tests { + got, err := Match(tt.pattern, tt.path) + if err != nil { + t.Errorf("match: %v", err) + } + if got != tt.want { + t.Errorf("Match(%q, %q) = %t, wanted %t", tt.pattern, tt.path, got, tt.want) + } + } +} From 5e302e742a09ed34f47efd9539055d5fdeaabbd7 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sat, 5 Aug 2023 14:24:08 -0400 Subject: [PATCH 03/22] good enough rulesMatchingBranch Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 66 ++++++++----- clients/githubrepo/branches_test.go | 138 ++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 21 deletions(-) create mode 100644 clients/githubrepo/branches_test.go diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index d09e2f43661..41373e255fd 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -24,6 +24,7 @@ import ( "github.com/shurcooL/githubv4" "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/clients/githubrepo/internal/fnmatch" sce "github.com/ossf/scorecard/v4/errors" ) @@ -150,11 +151,12 @@ type pullRequestRules struct { } } } +type ruleSetConditionRefs struct { + Include []string + Exclude []string +} type ruleSetCondition struct { - RefName struct { - Include []string - Exclude []string - } + RefName ruleSetConditionRefs } type repoRuleSet struct { @@ -173,9 +175,6 @@ type ruleSetData struct { Nodes []*repoRuleSet } `graphql:"rulesets(first: 100)"` } `graphql:"repository(owner: $owner, name: $name)"` - RateLimit struct { - Cost *int - } } type branchData struct { @@ -224,13 +223,20 @@ func (handler *branchesHandler) setup() error { handler.ruleSets = rulesData.Repository.Rulesets.Nodes handler.data = new(defaultBranchData) - // If the repository is using RuleSets, ignore permissions errors on branch protection as that requires admin. - if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil && len(handler.ruleSets) > 0 && isPermissionsError(err) { - handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) - return + if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil { + // If the repository is using RuleSets, ignore permissions errors on branch protection as that requires admin. + if len(handler.ruleSets) == 0 || !isPermissionsError(err) { + handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) + return + } } - rules := rulesMatchingBranch(handler.ruleSets, *handler.data.Repository.DefaultBranchRef.Name, true) + rules, err := rulesMatchingBranch(handler.ruleSets, *handler.data.Repository.DefaultBranchRef.Name, true) + if err != nil { + handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("rulesMatchingBranch: %v", err)) + return + } + fmt.Println(rules) handler.defaultBranchRef = getBranchRefFrom(handler.data.Repository.DefaultBranchRef, rules) }) return handler.errSetup @@ -249,7 +255,11 @@ func (handler *branchesHandler) query(branchName string) (*clients.BranchRef, er if err := handler.graphClient.Query(handler.ctx, queryData, vars); err != nil { return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) } - rules := rulesMatchingBranch(handler.ruleSets, branchName, branchName == *handler.data.Repository.DefaultBranchRef.Name) + defaultBranch := branchName == *handler.data.Repository.DefaultBranchRef.Name + rules, err := rulesMatchingBranch(handler.ruleSets, branchName, defaultBranch) + if err != nil { + return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("rulesMatchingBranch: %v", err)) + } return getBranchRefFrom(queryData.Repository.Ref, rules), nil } @@ -359,7 +369,12 @@ func isPermissionsError(err error) bool { return strings.Contains(err.Error(), "Resource not accessible by personal access token") } -func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) []*repoRuleSet { +const ( + ruleConditionDefaultBranch = "~DEFAULT_BRANCH" + ruleConditionAllBranches = "~ALL" +) + +func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) ([]*repoRuleSet, error) { ret := make([]*repoRuleSet, 0) nextRule: for _, rule := range rules { @@ -367,28 +382,37 @@ nextRule: continue } + // If there are no conditions, everything matches? if rule.Conditions == nil { - // TODO: is this even possible? ret = append(ret, rule) continue } // TODO: confirm this behaviour: for _, cond := range rule.Conditions.RefName.Exclude { - if cond == name { - continue nextRule - } - if cond == "~DEFAULT_BRANCH" && defaultRef { + if match, err := fnmatch.Match(cond, name); err != nil { + return nil, fmt.Errorf("exclude match error: %w", err) + } else if match { continue nextRule } } for _, cond := range rule.Conditions.RefName.Include { - if cond == name { + if cond == ruleConditionAllBranches { + ret = append(ret, rule) + break + } + if cond == ruleConditionDefaultBranch && defaultRef { ret = append(ret, rule) + break } + if match, err := fnmatch.Match(cond, name); err != nil { + return nil, fmt.Errorf("include match error: %w", err) + } else if match { + ret = append(ret, rule) + } } } - return ret + return ret, nil } diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go new file mode 100644 index 00000000000..6cbc843bdb1 --- /dev/null +++ b/clients/githubrepo/branches_test.go @@ -0,0 +1,138 @@ +package githubrepo + +import ( + "fmt" + "testing" +) + +func Test_rulesMatchingBranch(t *testing.T) { + t.Parallel() + testcases := []struct { + name string + defaultBranchNames map[string]bool + nonDefaultBranchNames map[string]bool + condition ruleSetCondition + }{ + { + name: "including all branches", + condition: ruleSetCondition{ + RefName: ruleSetConditionRefs{ + Include: []string{ruleConditionAllBranches}, + }, + }, + defaultBranchNames: map[string]bool{ + "main": true, + "foo": true, + }, + nonDefaultBranchNames: map[string]bool{ + "main": true, + "foo": true, + }, + }, + { + name: "including default branch", + condition: ruleSetCondition{ + RefName: ruleSetConditionRefs{ + Include: []string{ruleConditionDefaultBranch}, + }, + }, + defaultBranchNames: map[string]bool{ + "main": true, + "foo": true, + }, + nonDefaultBranchNames: map[string]bool{ + "main": false, + "foo": false, + }, + }, + { + name: "including branch by name", + condition: ruleSetCondition{ + RefName: ruleSetConditionRefs{ + Include: []string{"foo"}, + }, + }, + defaultBranchNames: map[string]bool{ + "main": false, + "foo": true, + }, + nonDefaultBranchNames: map[string]bool{ + "main": false, + "foo": true, + }, + }, + { + name: "including branch by fnmatch", + condition: ruleSetCondition{ + RefName: ruleSetConditionRefs{ + Include: []string{"foo/**"}, + }, + }, + defaultBranchNames: map[string]bool{ + "main": false, + "foo": false, + "foo/bar": true, + }, + nonDefaultBranchNames: map[string]bool{ + "main": false, + "foo": false, + "foo/bar": true, + }, + }, + + { + name: "include+exclude branch by fnmatch", + condition: ruleSetCondition{ + RefName: ruleSetConditionRefs{ + Include: []string{"foo/**"}, + Exclude: []string{"foo/bar"}, + }, + }, + defaultBranchNames: map[string]bool{ + "foo/bar": false, + "foo/baz": true, + }, + nonDefaultBranchNames: map[string]bool{ + "foo/bar": false, + "foo/baz": true, + }, + }, + } + + active := "ACTIVE" + for _, testcase := range testcases { + testcase := testcase + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + inputRules := []*repoRuleSet{{Enforcement: &active, Conditions: &testcase.condition}} + for branchName, expected := range testcase.defaultBranchNames { + branchName := branchName + expected := expected + t.Run(fmt.Sprintf("default branch %s", branchName), func(t *testing.T) { + t.Parallel() + matching, err := rulesMatchingBranch(inputRules, branchName, true) + if err != nil { + t.Fatalf("expected - no error, got: %v", err) + } + if matched := len(matching) == 1; matched != expected { + t.Errorf("expected %v, got %v", expected, matched) + } + }) + } + for branchName, expected := range testcase.nonDefaultBranchNames { + branchName := branchName + expected := expected + t.Run(fmt.Sprintf("non-default branch %s", branchName), func(t *testing.T) { + t.Parallel() + matching, err := rulesMatchingBranch(inputRules, branchName, false) + if err != nil { + t.Fatalf("expected - no error, got: %v", err) + } + if matched := len(matching) == 1; matched != expected { + t.Errorf("expected %v, got %v", expected, matched) + } + }) + } + }) + } +} From e25f8fb43ed97bc8275f68be971422a64fa42800 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sat, 5 Aug 2023 14:35:13 -0400 Subject: [PATCH 04/22] apply matching repo rules to branch protection settings Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 96 +++++++++++++++++++++++------ clients/githubrepo/branches_test.go | 10 +-- 2 files changed, 81 insertions(+), 25 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 41373e255fd..1ba247682fb 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -136,6 +136,13 @@ type defaultBranchData struct { Cost *int } } +type defaultBranchNameData struct { + Repository struct { + DefaultBranchRef struct { + Name *string + } + } `graphql:"repository(owner: $owner, name: $name)"` +} type pullRequestRuleParameters struct { DismissStaleReviewsOnPush *bool @@ -158,17 +165,14 @@ type ruleSetConditionRefs struct { type ruleSetCondition struct { RefName ruleSetConditionRefs } - type repoRuleSet struct { Name *string Enforcement *string - Target *string - Conditions *ruleSetCondition + Conditions ruleSetCondition PullRequestRules struct { Edges []*pullRequestRules } `graphql:"rules(first: 100, type: PULL_REQUEST)"` } - type ruleSetData struct { Repository struct { Rulesets struct { @@ -220,7 +224,13 @@ func (handler *branchesHandler) setup() error { handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) return } - handler.ruleSets = rulesData.Repository.Rulesets.Nodes + handler.ruleSets = make([]*repoRuleSet, 0) + for _, rule := range rulesData.Repository.Rulesets.Nodes { + if rule.Enforcement == nil || *rule.Enforcement != "ACTIVE" { + continue + } + handler.ruleSets = append(handler.ruleSets, rule) + } handler.data = new(defaultBranchData) if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil { @@ -229,6 +239,22 @@ func (handler *branchesHandler) setup() error { handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) return } + + // To recover, we still need to know the default branch name: + defaultBranchNameData := new(defaultBranchNameData) + if err := handler.graphClient.Query(handler.ctx, defaultBranchNameData, vars); err != nil { + handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) + return + } + handler.data = &defaultBranchData{ + Repository: struct { + DefaultBranchRef *branch + }{ + DefaultBranchRef: &branch{ + Name: defaultBranchNameData.Repository.DefaultBranchRef.Name, + }, + }, + } } rules, err := rulesMatchingBranch(handler.ruleSets, *handler.data.Repository.DefaultBranchRef.Name, true) @@ -236,7 +262,6 @@ func (handler *branchesHandler) setup() error { handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("rulesMatchingBranch: %v", err)) return } - fmt.Println(rules) handler.defaultBranchRef = getBranchRefFrom(handler.data.Repository.DefaultBranchRef, rules) }) return handler.errSetup @@ -333,7 +358,8 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { // It says nothing about what protection is enabled at all. branchRef.Protected = new(bool) if data.RefUpdateRule == nil && - data.BranchProtectionRule == nil { + data.BranchProtectionRule == nil && + len(rules) == 0 { // TODO: consider rules that match the branch here. *branchRef.Protected = false return branchRef @@ -362,6 +388,8 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { copyNonAdminSettings(rule, branchRule) } + applyRepoRules(branchRef, rules) + return branchRef } @@ -375,22 +403,12 @@ const ( ) func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) ([]*repoRuleSet, error) { + refName := fmt.Sprintf("refs/heads/%s", name) ret := make([]*repoRuleSet, 0) nextRule: for _, rule := range rules { - if rule.Enforcement == nil || *rule.Enforcement != "ACTIVE" { - continue - } - - // If there are no conditions, everything matches? - if rule.Conditions == nil { - ret = append(ret, rule) - continue - } - - // TODO: confirm this behaviour: for _, cond := range rule.Conditions.RefName.Exclude { - if match, err := fnmatch.Match(cond, name); err != nil { + if match, err := fnmatch.Match(cond, refName); err != nil { return nil, fmt.Errorf("exclude match error: %w", err) } else if match { continue nextRule @@ -407,7 +425,7 @@ nextRule: break } - if match, err := fnmatch.Match(cond, name); err != nil { + if match, err := fnmatch.Match(cond, refName); err != nil { return nil, fmt.Errorf("include match error: %w", err) } else if match { ret = append(ret, rule) @@ -416,3 +434,41 @@ nextRule: } return ret, nil } + +func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { + for _, rule := range rules { + for _, prRule := range rule.PullRequestRules.Edges { + dismissStale := readBoolPtr(prRule.Node.Parameters.PullRequestParameters.DismissStaleReviewsOnPush) + if dismissStale && !readBoolPtr(branchRef.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews) { + branchRef.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews = &dismissStale + } + codeOwner := readBoolPtr(prRule.Node.Parameters.PullRequestParameters.RequireCodeOwnerReview) + if codeOwner && !readBoolPtr(branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews) { + branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews = &codeOwner + } + lastPush := readBoolPtr(prRule.Node.Parameters.PullRequestParameters.RequireLastPushApproval) + if lastPush && !readBoolPtr(branchRef.BranchProtectionRule.RequireLastPushApproval) { + branchRef.BranchProtectionRule.RequireLastPushApproval = &lastPush + } + ruleReviewers := readIntPtr(prRule.Node.Parameters.PullRequestParameters.RequiredApprovingReviewCount) + branchReviewers := readIntPtr(branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) + if ruleReviewers > branchReviewers { + branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount = &ruleReviewers + } + } + } +} + +func readBoolPtr(b *bool) bool { + if b == nil { + return false + } + return *b +} + +func readIntPtr(i *int32) int32 { + if i == nil { + return 0 + } + return *i +} diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 6cbc843bdb1..30b62619c82 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -49,7 +49,7 @@ func Test_rulesMatchingBranch(t *testing.T) { name: "including branch by name", condition: ruleSetCondition{ RefName: ruleSetConditionRefs{ - Include: []string{"foo"}, + Include: []string{"refs/heads/foo"}, }, }, defaultBranchNames: map[string]bool{ @@ -65,7 +65,7 @@ func Test_rulesMatchingBranch(t *testing.T) { name: "including branch by fnmatch", condition: ruleSetCondition{ RefName: ruleSetConditionRefs{ - Include: []string{"foo/**"}, + Include: []string{"refs/heads/foo/**"}, }, }, defaultBranchNames: map[string]bool{ @@ -84,8 +84,8 @@ func Test_rulesMatchingBranch(t *testing.T) { name: "include+exclude branch by fnmatch", condition: ruleSetCondition{ RefName: ruleSetConditionRefs{ - Include: []string{"foo/**"}, - Exclude: []string{"foo/bar"}, + Include: []string{"refs/heads/foo/**"}, + Exclude: []string{"refs/heads/foo/bar"}, }, }, defaultBranchNames: map[string]bool{ @@ -104,7 +104,7 @@ func Test_rulesMatchingBranch(t *testing.T) { testcase := testcase t.Run(testcase.name, func(t *testing.T) { t.Parallel() - inputRules := []*repoRuleSet{{Enforcement: &active, Conditions: &testcase.condition}} + inputRules := []*repoRuleSet{{Enforcement: &active, Conditions: testcase.condition}} for branchName, expected := range testcase.defaultBranchNames { branchName := branchName expected := expected From 68f269544e64e3542a2c5d9582cf37add0918337 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sun, 6 Aug 2023 07:59:30 -0400 Subject: [PATCH 05/22] rules: consider admins and require checks Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 193 ++++++++++++++---- clients/githubrepo/branches_test.go | 14 ++ .../githubrepo/internal/fnmatch/fnmatch.go | 14 ++ .../internal/fnmatch/fnmatch_test.go | 14 ++ 4 files changed, 195 insertions(+), 40 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 1ba247682fb..bc178b6b89a 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -73,17 +73,37 @@ const ( include } } - pullRequestRules: rules(first: 100, type: PULL_REQUEST) { - edges { - node { - parameters { - ... on PullRequestParameters { - dismissStaleReviewsOnPush - requireCodeOwnerReview - requireLastPushApproval - requiredApprovingReviewCount - requiredReviewThreadResolution + bypassActors(first: 100) { + nodes { + actor { + __typename + ... on App { + name + databaseId + } + } + bypassMode + organizationAdmin + repositoryRoleName + } + } + rules(first: 100) { + nodes { + type + parameters { + ... on PullRequestParameters { + dismissStaleReviewsOnPush + requireCodeOwnerReview + requireLastPushApproval + requiredApprovingReviewCount + requiredReviewThreadResolution + } + ... on RequiredStatusChecksParameters { + requiredStatusChecks { + context + integrationId } + strictRequiredStatusChecksPolicy } } } @@ -151,11 +171,18 @@ type pullRequestRuleParameters struct { RequiredApprovingReviewCount *int32 RequiredReviewThreadResolution *bool } -type pullRequestRules struct { - Node struct { - Parameters struct { - PullRequestParameters pullRequestRuleParameters `graphql:"... on PullRequestParameters"` - } +type requiredStatusCheckParameters struct { + StrictRequiredStatusChecksPolicy *bool + RequiredStatusChecks []struct { + Context *string + IntegrationID *int64 + } +} +type repoRule struct { + Type string + Parameters struct { + PullRequestParameters pullRequestRuleParameters `graphql:"... on PullRequestParameters"` + StatusCheckParameters requiredStatusCheckParameters `graphql:"... on RequiredStatusChecksParameters"` } } type ruleSetConditionRefs struct { @@ -165,13 +192,21 @@ type ruleSetConditionRefs struct { type ruleSetCondition struct { RefName ruleSetConditionRefs } +type ruleSetBypass struct { + BypassMode *string + OrganizationAdmin *bool + RepositoryRoleName *string +} type repoRuleSet struct { - Name *string - Enforcement *string - Conditions ruleSetCondition - PullRequestRules struct { - Edges []*pullRequestRules - } `graphql:"rules(first: 100, type: PULL_REQUEST)"` + Name *string + Enforcement *string + Conditions ruleSetCondition + BypassActors struct { + Nodes []*ruleSetBypass + } `graphql:"bypassActors(first: 100)"` + Rules struct { + Nodes []*repoRule + } `graphql:"rules(first: 100)"` } type ruleSetData struct { Repository struct { @@ -360,7 +395,6 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { if data.RefUpdateRule == nil && data.BranchProtectionRule == nil && len(rules) == 0 { - // TODO: consider rules that match the branch here. *branchRef.Protected = false return branchRef } @@ -436,27 +470,106 @@ nextRule: } func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { - for _, rule := range rules { - for _, prRule := range rule.PullRequestRules.Edges { - dismissStale := readBoolPtr(prRule.Node.Parameters.PullRequestParameters.DismissStaleReviewsOnPush) - if dismissStale && !readBoolPtr(branchRef.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews) { - branchRef.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews = &dismissStale - } - codeOwner := readBoolPtr(prRule.Node.Parameters.PullRequestParameters.RequireCodeOwnerReview) - if codeOwner && !readBoolPtr(branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews) { - branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews = &codeOwner - } - lastPush := readBoolPtr(prRule.Node.Parameters.PullRequestParameters.RequireLastPushApproval) - if lastPush && !readBoolPtr(branchRef.BranchProtectionRule.RequireLastPushApproval) { - branchRef.BranchProtectionRule.RequireLastPushApproval = &lastPush - } - ruleReviewers := readIntPtr(prRule.Node.Parameters.PullRequestParameters.RequiredApprovingReviewCount) - branchReviewers := readIntPtr(branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) - if ruleReviewers > branchReviewers { - branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount = &ruleReviewers + for _, r := range rules { + var modded bool + for _, rule := range r.Rules.Nodes { + switch rule.Type { + case "DELETION": + if branchRef.BranchProtectionRule.AllowDeletions == nil || *branchRef.BranchProtectionRule.AllowDeletions { + branchRef.BranchProtectionRule.AllowDeletions = new(bool) + *branchRef.BranchProtectionRule.AllowDeletions = false + modded = true + } + case "NON_FAST_FORWARD": + if branchRef.BranchProtectionRule.AllowForcePushes == nil || *branchRef.BranchProtectionRule.AllowForcePushes { + branchRef.BranchProtectionRule.AllowForcePushes = new(bool) + *branchRef.BranchProtectionRule.AllowForcePushes = false + modded = true + } + case "PULL_REQUEST": + if applyPullRequestRepoRule(branchRef, rule) { + modded = true + } + case "REQUIRED_STATUS_CHECKS": + if applyRequiredStatusChecksRepoRule(branchRef, rule) { + modded = true + } } } + if modded { + adminEnforced := ruleAdminEnforced(r) + branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced + } + } +} + +func applyPullRequestRepoRule(branchRef *clients.BranchRef, rule *repoRule) bool { + var modded bool + branchRules := branchRef.BranchProtectionRule.RequiredPullRequestReviews + dismissStale := readBoolPtr(rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush) + if dismissStale && !readBoolPtr(branchRules.DismissStaleReviews) { + branchRef.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews = &dismissStale + modded = true + } + codeOwner := readBoolPtr(rule.Parameters.PullRequestParameters.RequireCodeOwnerReview) + if codeOwner && !readBoolPtr(branchRules.RequireCodeOwnerReviews) { + branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews = &codeOwner + modded = true + } + lastPush := readBoolPtr(rule.Parameters.PullRequestParameters.RequireLastPushApproval) + if lastPush && !readBoolPtr(branchRef.BranchProtectionRule.RequireLastPushApproval) { + branchRef.BranchProtectionRule.RequireLastPushApproval = &lastPush + modded = true + } + ruleReviewers := readIntPtr(rule.Parameters.PullRequestParameters.RequiredApprovingReviewCount) + branchReviewers := readIntPtr(branchRules.RequiredApprovingReviewCount) + if ruleReviewers > branchReviewers { + branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount = &ruleReviewers + modded = true + } + return modded +} + +func applyRequiredStatusChecksRepoRule(branchRef *clients.BranchRef, rule *repoRule) bool { + // If the branch already requires status checks, the rule does not matter. + branchRules := &branchRef.BranchProtectionRule.CheckRules + if readBoolPtr(branchRules.RequiresStatusChecks) { + return false + } + + statusParams := rule.Parameters.StatusCheckParameters + if len(statusParams.RequiredStatusChecks) == 0 { + return false + } + + enabled := true + branchRules.RequiresStatusChecks = &enabled + if branchRules.UpToDateBeforeMerge == nil && statusParams.StrictRequiredStatusChecksPolicy != nil { + copyBoolPtr(statusParams.StrictRequiredStatusChecksPolicy, &branchRules.UpToDateBeforeMerge) + } + for _, chk := range statusParams.RequiredStatusChecks { + if chk.Context == nil { + continue + } + branchRules.Contexts = append(branchRules.Contexts, *chk.Context) + } + return true +} + +func ruleAdminEnforced(rule *repoRuleSet) bool { + if len(rule.BypassActors.Nodes) == 0 { + return true + } + for _, bypass := range rule.BypassActors.Nodes { + if readBoolPtr(bypass.OrganizationAdmin) { + return false + } + if bypass.RepositoryRoleName != nil { + // this may be "admin", "write", "maintainer" or a custom role - treat all as bad + return false + } } + return true } func readBoolPtr(b *bool) bool { diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 30b62619c82..9e8b44bc385 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -1,3 +1,17 @@ +// 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. + package githubrepo import ( diff --git a/clients/githubrepo/internal/fnmatch/fnmatch.go b/clients/githubrepo/internal/fnmatch/fnmatch.go index 8a957500630..fdd26e4c3be 100644 --- a/clients/githubrepo/internal/fnmatch/fnmatch.go +++ b/clients/githubrepo/internal/fnmatch/fnmatch.go @@ -1,3 +1,17 @@ +// 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. + package fnmatch import ( diff --git a/clients/githubrepo/internal/fnmatch/fnmatch_test.go b/clients/githubrepo/internal/fnmatch/fnmatch_test.go index 20bda25529d..3059568b051 100644 --- a/clients/githubrepo/internal/fnmatch/fnmatch_test.go +++ b/clients/githubrepo/internal/fnmatch/fnmatch_test.go @@ -1,3 +1,17 @@ +// 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. + package fnmatch import ( From a808f6936e3b26f06f29c6e38bdbc6d4ac92a7b2 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sat, 12 Aug 2023 09:17:15 -0400 Subject: [PATCH 06/22] non-structural chanages from PR feedback Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index bc178b6b89a..ff0ec20fbe8 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -34,7 +34,7 @@ const ( // See https://github.community/t/graphql-api-protected-branch/14380 /* Example of query: -{ +query { repository(owner: "laurentsimon", name: "test3") { branchProtectionRules(first: 100) { edges { @@ -428,7 +428,7 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { } func isPermissionsError(err error) bool { - return strings.Contains(err.Error(), "Resource not accessible by personal access token") + return strings.Contains(err.Error(), "Resource not accessible") } const ( @@ -437,7 +437,7 @@ const ( ) func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) ([]*repoRuleSet, error) { - refName := fmt.Sprintf("refs/heads/%s", name) + refName := refPrefix + name ret := make([]*repoRuleSet, 0) nextRule: for _, rule := range rules { @@ -557,9 +557,6 @@ func applyRequiredStatusChecksRepoRule(branchRef *clients.BranchRef, rule *repoR } func ruleAdminEnforced(rule *repoRuleSet) bool { - if len(rule.BypassActors.Nodes) == 0 { - return true - } for _, bypass := range rule.BypassActors.Nodes { if readBoolPtr(bypass.OrganizationAdmin) { return false From 3e527f7545717f1f625fe9d4fbe86e23deb09b7e Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sat, 12 Aug 2023 09:39:43 -0400 Subject: [PATCH 07/22] fetch default branch name during repo rules query Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 94 ++++++++++++++++------------------ 1 file changed, 45 insertions(+), 49 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index ff0ec20fbe8..ea4c51be1bd 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -156,13 +156,6 @@ type defaultBranchData struct { Cost *int } } -type defaultBranchNameData struct { - Repository struct { - DefaultBranchRef struct { - Name *string - } - } `graphql:"repository(owner: $owner, name: $name)"` -} type pullRequestRuleParameters struct { DismissStaleReviewsOnPush *bool @@ -210,6 +203,9 @@ type repoRuleSet struct { } type ruleSetData struct { Repository struct { + DefaultBranchRef struct { + Name *string + } Rulesets struct { Nodes []*repoRuleSet } `graphql:"rulesets(first: 100)"` @@ -223,15 +219,16 @@ type branchData struct { } type branchesHandler struct { - ghClient *github.Client - graphClient *githubv4.Client - data *defaultBranchData - once *sync.Once - ctx context.Context - errSetup error - repourl *repoURL - defaultBranchRef *clients.BranchRef - ruleSets []*repoRuleSet + ghClient *github.Client + graphClient *githubv4.Client + data *defaultBranchData + once *sync.Once + ctx context.Context + errSetup error + repourl *repoURL + defaultBranchRef *clients.BranchRef + defaultBranchName string + ruleSets []*repoRuleSet } func (handler *branchesHandler) init(ctx context.Context, repourl *repoURL) { @@ -240,6 +237,8 @@ func (handler *branchesHandler) init(ctx context.Context, repourl *repoURL) { handler.errSetup = nil handler.once = new(sync.Once) handler.defaultBranchRef = nil + handler.defaultBranchName = "" + handler.ruleSets = nil handler.data = nil } @@ -254,45 +253,25 @@ func (handler *branchesHandler) setup() error { "name": githubv4.String(handler.repourl.repo), } + // Fetch default branch name and any repository rulesets, which are available with basic read permission. rulesData := new(ruleSetData) if err := handler.graphClient.Query(handler.ctx, rulesData, vars); err != nil { handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) return } - handler.ruleSets = make([]*repoRuleSet, 0) - for _, rule := range rulesData.Repository.Rulesets.Nodes { - if rule.Enforcement == nil || *rule.Enforcement != "ACTIVE" { - continue - } - handler.ruleSets = append(handler.ruleSets, rule) - } + handler.defaultBranchName = getDefaultBranchNameFrom(rulesData) + handler.ruleSets = getActiveRuleSetsFrom(rulesData) + // Attempt to fetch branch protection rules, which require admin permission. + // Ignore permissions errors if we know the repository is using rulesets, so non-admins can still get a score. handler.data = new(defaultBranchData) - if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil { - // If the repository is using RuleSets, ignore permissions errors on branch protection as that requires admin. - if len(handler.ruleSets) == 0 || !isPermissionsError(err) { - handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) - return - } - - // To recover, we still need to know the default branch name: - defaultBranchNameData := new(defaultBranchNameData) - if err := handler.graphClient.Query(handler.ctx, defaultBranchNameData, vars); err != nil { - handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) - return - } - handler.data = &defaultBranchData{ - Repository: struct { - DefaultBranchRef *branch - }{ - DefaultBranchRef: &branch{ - Name: defaultBranchNameData.Repository.DefaultBranchRef.Name, - }, - }, - } + if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil && + !isPermissionsError(err) || len(handler.ruleSets) == 0 { + handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) + return } - rules, err := rulesMatchingBranch(handler.ruleSets, *handler.data.Repository.DefaultBranchRef.Name, true) + rules, err := rulesMatchingBranch(handler.ruleSets, handler.defaultBranchName, true) if err != nil { handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("rulesMatchingBranch: %v", err)) return @@ -306,6 +285,10 @@ func (handler *branchesHandler) query(branchName string) (*clients.BranchRef, er if !strings.EqualFold(handler.repourl.commitSHA, clients.HeadSHA) { return nil, fmt.Errorf("%w: branches only supported for HEAD queries", clients.ErrUnsupportedFeature) } + // Call setup(), so we know if branchName == handler.defaultBranchName. + if err := handler.setup(); err != nil { + return nil, fmt.Errorf("error during branchesHandler.setup: %w", err) + } vars := map[string]interface{}{ "owner": githubv4.String(handler.repourl.owner), "name": githubv4.String(handler.repourl.repo), @@ -315,8 +298,7 @@ func (handler *branchesHandler) query(branchName string) (*clients.BranchRef, er if err := handler.graphClient.Query(handler.ctx, queryData, vars); err != nil { return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) } - defaultBranch := branchName == *handler.data.Repository.DefaultBranchRef.Name - rules, err := rulesMatchingBranch(handler.ruleSets, branchName, defaultBranch) + rules, err := rulesMatchingBranch(handler.ruleSets, branchName, branchName == handler.defaultBranchName) if err != nil { return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("rulesMatchingBranch: %v", err)) } @@ -331,7 +313,6 @@ func (handler *branchesHandler) getDefaultBranch() (*clients.BranchRef, error) { } func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, error) { - // TODO: has setup() been called yet? current implementation assumes yes. branchRef, err := handler.query(branch) if err != nil { return nil, fmt.Errorf("error during branchesHandler.query: %w", err) @@ -379,6 +360,21 @@ func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { } } +func getDefaultBranchNameFrom(data *ruleSetData) string { + return *data.Repository.DefaultBranchRef.Name +} + +func getActiveRuleSetsFrom(data *ruleSetData) []*repoRuleSet { + ret := make([]*repoRuleSet, 0) + for _, rule := range data.Repository.Rulesets.Nodes { + if rule.Enforcement == nil || *rule.Enforcement != "ACTIVE" { + continue + } + ret = append(ret, rule) + } + return ret +} + func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef { if data == nil { return nil From 7b908a192053ff8f4a9a5e76457b9e6cef1b41a9 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Fri, 18 Aug 2023 10:20:11 -0400 Subject: [PATCH 08/22] Testing applyRepoRules Tests assume a single rule is being applied to a branch, which might be guarded by a legacy branch protection rule. I think this logic gets problematic when there are multiple rules overlaid on the same branch: the "the existing rules does not enforce for admins, but i do and therefore this branch now does" will give false-positives. Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 39 ++++---- clients/githubrepo/branches_test.go | 149 +++++++++++++++++++++++++++- 2 files changed, 167 insertions(+), 21 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index ea4c51be1bd..31f28e0c354 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -173,10 +173,11 @@ type requiredStatusCheckParameters struct { } type repoRule struct { Type string - Parameters struct { - PullRequestParameters pullRequestRuleParameters `graphql:"... on PullRequestParameters"` - StatusCheckParameters requiredStatusCheckParameters `graphql:"... on RequiredStatusChecksParameters"` - } + Parameters repoRulesParameters +} +type repoRulesParameters struct { + PullRequestParameters pullRequestRuleParameters `graphql:"... on PullRequestParameters"` + StatusCheckParameters requiredStatusCheckParameters `graphql:"... on RequiredStatusChecksParameters"` } type ruleSetConditionRefs struct { Include []string @@ -466,22 +467,34 @@ nextRule: } func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { + baseAdminEnforced := readBoolPtr(branchRef.BranchProtectionRule.EnforceAdmins) + for _, r := range rules { + adminEnforced := len(r.BypassActors.Nodes) == 0 + var modded bool for _, rule := range r.Rules.Nodes { switch rule.Type { case "DELETION": if branchRef.BranchProtectionRule.AllowDeletions == nil || *branchRef.BranchProtectionRule.AllowDeletions { + // The base may allow deletions, but this rule does not. branchRef.BranchProtectionRule.AllowDeletions = new(bool) *branchRef.BranchProtectionRule.AllowDeletions = false - modded = true + branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced + } else if branchRef.BranchProtectionRule.AllowDeletions != nil && adminEnforced && !baseAdminEnforced { + // The base blocks deletion without admin enforcement, this rule has admin enforcement. + branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced } + case "NON_FAST_FORWARD": if branchRef.BranchProtectionRule.AllowForcePushes == nil || *branchRef.BranchProtectionRule.AllowForcePushes { branchRef.BranchProtectionRule.AllowForcePushes = new(bool) *branchRef.BranchProtectionRule.AllowForcePushes = false - modded = true + branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced + } else if branchRef.BranchProtectionRule.AllowForcePushes != nil && adminEnforced && !baseAdminEnforced { + branchRef.BranchProtectionRule.AllowForcePushes = &adminEnforced } + case "PULL_REQUEST": if applyPullRequestRepoRule(branchRef, rule) { modded = true @@ -493,7 +506,6 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { } } if modded { - adminEnforced := ruleAdminEnforced(r) branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced } } @@ -552,19 +564,6 @@ func applyRequiredStatusChecksRepoRule(branchRef *clients.BranchRef, rule *repoR return true } -func ruleAdminEnforced(rule *repoRuleSet) bool { - for _, bypass := range rule.BypassActors.Nodes { - if readBoolPtr(bypass.OrganizationAdmin) { - return false - } - if bypass.RepositoryRoleName != nil { - // this may be "admin", "write", "maintainer" or a custom role - treat all as bad - return false - } - } - return true -} - func readBoolPtr(b *bool) bool { if b == nil { return false diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 9e8b44bc385..70daa574dad 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -17,6 +17,10 @@ package githubrepo import ( "fmt" "testing" + + "github.com/google/go-cmp/cmp" + + "github.com/ossf/scorecard/v4/clients" ) func Test_rulesMatchingBranch(t *testing.T) { @@ -93,7 +97,6 @@ func Test_rulesMatchingBranch(t *testing.T) { "foo/bar": true, }, }, - { name: "include+exclude branch by fnmatch", condition: ruleSetCondition{ @@ -150,3 +153,147 @@ func Test_rulesMatchingBranch(t *testing.T) { }) } } + +func Test_applyRepoRules(t *testing.T) { + t.Parallel() + + main := "main" + trueVal := true + falseVal := false + + twoVal := int32(2) + + testcases := []struct { + base *clients.BranchRef + rule *repoRule + expected *clients.BranchRef + ruleBypass *ruleSetBypass + name string + }{ + { + name: "no repo rules", + base: &clients.BranchRef{Name: &main, Protected: &trueVal}, + expected: &clients.BranchRef{Name: &main, Protected: &trueVal}, + }, + { + name: "rule blocks deletion without bypass", + base: &clients.BranchRef{}, + rule: &repoRule{Type: "DELETION"}, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + }, + { + name: "rule blocks deletion with bypass", + base: &clients.BranchRef{}, + rule: &repoRule{Type: "DELETION"}, + ruleBypass: &ruleSetBypass{}, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + }, + }, + }, + { + name: "rule blocks deletion with bypass, but traditional branch protection blocks without bypass", + base: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + rule: &repoRule{Type: "DELETION"}, + ruleBypass: &ruleSetBypass{}, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + }, + { + name: "rule blocks deletion without bypass, while traditional branch protection blocks with bypass", + base: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + }, + }, + rule: &repoRule{Type: "DELETION"}, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + }, + { + name: "rule blocks force pushes without bypass", + base: &clients.BranchRef{}, + rule: &repoRule{Type: "NON_FAST_FORWARD"}, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + }, + { + name: "rule requires pull request without bypass", + base: &clients.BranchRef{}, + rule: &repoRule{ + Type: "PULL_REQUEST", + Parameters: repoRulesParameters{ + PullRequestParameters: pullRequestRuleParameters{ + DismissStaleReviewsOnPush: &trueVal, + RequireCodeOwnerReview: &trueVal, + RequireLastPushApproval: &trueVal, + RequiredApprovingReviewCount: &twoVal, + RequiredReviewThreadResolution: &trueVal, + }, + }, + }, + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + DismissStaleReviews: &trueVal, + RequireCodeOwnerReviews: &trueVal, + RequiredApprovingReviewCount: &twoVal, + }, + }, + }, + }, + } + + for _, testcase := range testcases { + testcase := testcase + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + + var rules []*repoRuleSet + if testcase.rule != nil { + ruleSet := &repoRuleSet{ + Rules: struct{ Nodes []*repoRule }{ + Nodes: []*repoRule{testcase.rule}, + }, + } + if testcase.ruleBypass != nil { + ruleSet.BypassActors.Nodes = []*ruleSetBypass{testcase.ruleBypass} + } + rules = append(rules, ruleSet) + } + applyRepoRules(testcase.base, rules) + + if !cmp.Equal(testcase.base, testcase.expected) { + diff := cmp.Diff(testcase.base, testcase.expected) + t.Errorf("test failed: expected - %v, got - %v. \n%s", testcase.expected, testcase.base, diff) + } + }) + } +} From be6dda3922722cc85b168943d27720f556c2b5e2 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sat, 19 Aug 2023 10:00:42 -0400 Subject: [PATCH 09/22] Test_applyRepoRules: builder and standardize names Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches_test.go | 87 +++++++++++++++-------------- 1 file changed, 46 insertions(+), 41 deletions(-) diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 70daa574dad..f9c16b921c2 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -154,31 +154,45 @@ func Test_rulesMatchingBranch(t *testing.T) { } } +type ruleSetOpt func(*repoRuleSet) + +func ruleSet(opts ...ruleSetOpt) *repoRuleSet { + r := &repoRuleSet{} + for _, opt := range opts { + opt(r) + } + return r +} + +func withRules(rules ...*repoRule) ruleSetOpt { + return func(r *repoRuleSet) { + r.Rules.Nodes = append(r.Rules.Nodes, rules...) + } +} + +func withBypass(bypass *ruleSetBypass) ruleSetOpt { + return func(r *repoRuleSet) { + r.BypassActors.Nodes = append(r.BypassActors.Nodes, bypass) + } +} + func Test_applyRepoRules(t *testing.T) { t.Parallel() - - main := "main" trueVal := true falseVal := false - twoVal := int32(2) testcases := []struct { base *clients.BranchRef - rule *repoRule + ruleSet *repoRuleSet expected *clients.BranchRef ruleBypass *ruleSetBypass name string }{ { - name: "no repo rules", - base: &clients.BranchRef{Name: &main, Protected: &trueVal}, - expected: &clients.BranchRef{Name: &main, Protected: &trueVal}, - }, - { - name: "rule blocks deletion without bypass", - base: &clients.BranchRef{}, - rule: &repoRule{Type: "DELETION"}, + name: "block deletion no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -187,10 +201,12 @@ func Test_applyRepoRules(t *testing.T) { }, }, { - name: "rule blocks deletion with bypass", - base: &clients.BranchRef{}, - rule: &repoRule{Type: "DELETION"}, - ruleBypass: &ruleSetBypass{}, + name: "block deletion with bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet( + withRules(&repoRule{Type: "DELETION"}), + withBypass(&ruleSetBypass{}), + ), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -199,15 +215,17 @@ func Test_applyRepoRules(t *testing.T) { }, }, { - name: "rule blocks deletion with bypass, but traditional branch protection blocks without bypass", + name: "block deletion with bypass, while deletion is blocked without bypass", base: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, EnforceAdmins: &trueVal, }, }, - rule: &repoRule{Type: "DELETION"}, - ruleBypass: &ruleSetBypass{}, + ruleSet: ruleSet( + withRules(&repoRule{Type: "DELETION"}), + withBypass(&ruleSetBypass{}), + ), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -216,14 +234,14 @@ func Test_applyRepoRules(t *testing.T) { }, }, { - name: "rule blocks deletion without bypass, while traditional branch protection blocks with bypass", + name: "block deletion no bypass, while deletion is blocked with bypass", base: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, EnforceAdmins: &falseVal, }, }, - rule: &repoRule{Type: "DELETION"}, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -232,9 +250,9 @@ func Test_applyRepoRules(t *testing.T) { }, }, { - name: "rule blocks force pushes without bypass", - base: &clients.BranchRef{}, - rule: &repoRule{Type: "NON_FAST_FORWARD"}, + name: "block force push no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: "NON_FAST_FORWARD"})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowForcePushes: &falseVal, @@ -243,9 +261,9 @@ func Test_applyRepoRules(t *testing.T) { }, }, { - name: "rule requires pull request without bypass", + name: "require pull request no bypass", base: &clients.BranchRef{}, - rule: &repoRule{ + ruleSet: ruleSet(withRules(&repoRule{ Type: "PULL_REQUEST", Parameters: repoRulesParameters{ PullRequestParameters: pullRequestRuleParameters{ @@ -256,7 +274,7 @@ func Test_applyRepoRules(t *testing.T) { RequiredReviewThreadResolution: &trueVal, }, }, - }, + })), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ EnforceAdmins: &trueVal, @@ -275,20 +293,7 @@ func Test_applyRepoRules(t *testing.T) { testcase := testcase t.Run(testcase.name, func(t *testing.T) { t.Parallel() - - var rules []*repoRuleSet - if testcase.rule != nil { - ruleSet := &repoRuleSet{ - Rules: struct{ Nodes []*repoRule }{ - Nodes: []*repoRule{testcase.rule}, - }, - } - if testcase.ruleBypass != nil { - ruleSet.BypassActors.Nodes = []*ruleSetBypass{testcase.ruleBypass} - } - rules = append(rules, ruleSet) - } - applyRepoRules(testcase.base, rules) + applyRepoRules(testcase.base, []*repoRuleSet{testcase.ruleSet}) if !cmp.Equal(testcase.base, testcase.expected) { diff := cmp.Diff(testcase.base, testcase.expected) From 7fbb32bc603ff3e4341bd21151d1a1b079d93c62 Mon Sep 17 00:00:00 2001 From: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Date: Sat, 19 Aug 2023 11:11:40 -0400 Subject: [PATCH 10/22] attempt to upgrade/downgrade EnforceAdmins as each rule is applied Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> --- clients/githubrepo/branches.go | 123 +++++++++++++--------------- clients/githubrepo/branches_test.go | 111 +++++++++++++++++++++---- 2 files changed, 152 insertions(+), 82 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 31f28e0c354..fe29bc4867a 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -467,101 +467,92 @@ nextRule: } func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { - baseAdminEnforced := readBoolPtr(branchRef.BranchProtectionRule.EnforceAdmins) - + falseVal := false for _, r := range rules { adminEnforced := len(r.BypassActors.Nodes) == 0 + translated := clients.BranchProtectionRule{ + EnforceAdmins: &adminEnforced, + } - var modded bool for _, rule := range r.Rules.Nodes { switch rule.Type { case "DELETION": - if branchRef.BranchProtectionRule.AllowDeletions == nil || *branchRef.BranchProtectionRule.AllowDeletions { - // The base may allow deletions, but this rule does not. - branchRef.BranchProtectionRule.AllowDeletions = new(bool) - *branchRef.BranchProtectionRule.AllowDeletions = false - branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced - } else if branchRef.BranchProtectionRule.AllowDeletions != nil && adminEnforced && !baseAdminEnforced { - // The base blocks deletion without admin enforcement, this rule has admin enforcement. - branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced - } - + translated.AllowDeletions = &falseVal case "NON_FAST_FORWARD": - if branchRef.BranchProtectionRule.AllowForcePushes == nil || *branchRef.BranchProtectionRule.AllowForcePushes { - branchRef.BranchProtectionRule.AllowForcePushes = new(bool) - *branchRef.BranchProtectionRule.AllowForcePushes = false - branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced - } else if branchRef.BranchProtectionRule.AllowForcePushes != nil && adminEnforced && !baseAdminEnforced { - branchRef.BranchProtectionRule.AllowForcePushes = &adminEnforced - } - + translated.AllowForcePushes = &falseVal case "PULL_REQUEST": - if applyPullRequestRepoRule(branchRef, rule) { - modded = true - } + translatePullRequestRepoRule(&translated, rule) case "REQUIRED_STATUS_CHECKS": - if applyRequiredStatusChecksRepoRule(branchRef, rule) { - modded = true - } + translateRequiredStatusRepoRule(&translated, rule) } } - if modded { - branchRef.BranchProtectionRule.EnforceAdmins = &adminEnforced - } + mergeBranchProtectionRules(&branchRef.BranchProtectionRule, &translated) } } -func applyPullRequestRepoRule(branchRef *clients.BranchRef, rule *repoRule) bool { - var modded bool - branchRules := branchRef.BranchProtectionRule.RequiredPullRequestReviews - dismissStale := readBoolPtr(rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush) - if dismissStale && !readBoolPtr(branchRules.DismissStaleReviews) { - branchRef.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews = &dismissStale - modded = true +func translatePullRequestRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { + if readBoolPtr(rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush) { + base.RequiredPullRequestReviews.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush } - codeOwner := readBoolPtr(rule.Parameters.PullRequestParameters.RequireCodeOwnerReview) - if codeOwner && !readBoolPtr(branchRules.RequireCodeOwnerReviews) { - branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews = &codeOwner - modded = true + if readBoolPtr(rule.Parameters.PullRequestParameters.RequireCodeOwnerReview) { + base.RequiredPullRequestReviews.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview } - lastPush := readBoolPtr(rule.Parameters.PullRequestParameters.RequireLastPushApproval) - if lastPush && !readBoolPtr(branchRef.BranchProtectionRule.RequireLastPushApproval) { - branchRef.BranchProtectionRule.RequireLastPushApproval = &lastPush - modded = true + if readBoolPtr(rule.Parameters.PullRequestParameters.RequireLastPushApproval) { + base.RequireLastPushApproval = rule.Parameters.PullRequestParameters.RequireLastPushApproval } - ruleReviewers := readIntPtr(rule.Parameters.PullRequestParameters.RequiredApprovingReviewCount) - branchReviewers := readIntPtr(branchRules.RequiredApprovingReviewCount) - if ruleReviewers > branchReviewers { - branchRef.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount = &ruleReviewers - modded = true + if reviewerCount := readIntPtr(rule.Parameters.PullRequestParameters.RequiredApprovingReviewCount); reviewerCount > 0 { + base.RequiredPullRequestReviews.RequiredApprovingReviewCount = &reviewerCount } - return modded } -func applyRequiredStatusChecksRepoRule(branchRef *clients.BranchRef, rule *repoRule) bool { - // If the branch already requires status checks, the rule does not matter. - branchRules := &branchRef.BranchProtectionRule.CheckRules - if readBoolPtr(branchRules.RequiresStatusChecks) { - return false - } - +func translateRequiredStatusRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { statusParams := rule.Parameters.StatusCheckParameters if len(statusParams.RequiredStatusChecks) == 0 { - return false + return } - enabled := true - branchRules.RequiresStatusChecks = &enabled - if branchRules.UpToDateBeforeMerge == nil && statusParams.StrictRequiredStatusChecksPolicy != nil { - copyBoolPtr(statusParams.StrictRequiredStatusChecksPolicy, &branchRules.UpToDateBeforeMerge) - } + base.CheckRules.RequiresStatusChecks = &enabled + base.CheckRules.UpToDateBeforeMerge = statusParams.StrictRequiredStatusChecksPolicy for _, chk := range statusParams.RequiredStatusChecks { if chk.Context == nil { continue } - branchRules.Contexts = append(branchRules.Contexts, *chk.Context) + base.CheckRules.Contexts = append(base.CheckRules.Contexts, *chk.Context) + } +} + +func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) { + var downgrade bool + upgrade := true + + // FIXME: upgrade/downgrade clauses need to compare every property? + if translated.AllowDeletions != nil && !*translated.AllowDeletions { + base.AllowDeletions = translated.AllowDeletions + upgrade = upgrade && (base.AllowForcePushes == nil || base.AllowForcePushes == translated.AllowForcePushes) + downgrade = downgrade || (base.AllowForcePushes != nil && !*base.AllowForcePushes) + } + if translated.AllowForcePushes != nil && !*translated.AllowForcePushes { + base.AllowForcePushes = translated.AllowForcePushes + upgrade = upgrade && (base.AllowDeletions == nil || base.AllowDeletions == translated.AllowDeletions) + downgrade = downgrade || (base.AllowDeletions != nil && !*base.AllowDeletions) + } + + // FIXME: hacks to avoid breaking existing tests + base.RequiredPullRequestReviews = translated.RequiredPullRequestReviews + base.CheckRules = translated.CheckRules + base.RequireLastPushApproval = translated.RequireLastPushApproval + + if base.EnforceAdmins == nil { + base.EnforceAdmins = translated.EnforceAdmins + } else if *base.EnforceAdmins != *translated.EnforceAdmins { + enforceAdmins := *base.EnforceAdmins + if upgrade { + enforceAdmins = true + } else if downgrade { + enforceAdmins = false + } + base.EnforceAdmins = &enforceAdmins } - return true } func readBoolPtr(b *bool) bool { diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index f9c16b921c2..9052fb86127 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -170,9 +170,9 @@ func withRules(rules ...*repoRule) ruleSetOpt { } } -func withBypass(bypass *ruleSetBypass) ruleSetOpt { +func withBypass() ruleSetOpt { return func(r *repoRuleSet) { - r.BypassActors.Nodes = append(r.BypassActors.Nodes, bypass) + r.BypassActors.Nodes = append(r.BypassActors.Nodes, &ruleSetBypass{}) } } @@ -201,12 +201,9 @@ func Test_applyRepoRules(t *testing.T) { }, }, { - name: "block deletion with bypass", - base: &clients.BranchRef{}, - ruleSet: ruleSet( - withRules(&repoRule{Type: "DELETION"}), - withBypass(&ruleSetBypass{}), - ), + name: "block deletion with bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}), withBypass()), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -215,26 +212,23 @@ func Test_applyRepoRules(t *testing.T) { }, }, { - name: "block deletion with bypass, while deletion is blocked without bypass", + name: "block deletion with bypass when block deletion no bypass", base: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, EnforceAdmins: &trueVal, }, }, - ruleSet: ruleSet( - withRules(&repoRule{Type: "DELETION"}), - withBypass(&ruleSetBypass{}), - ), + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}), withBypass()), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, - EnforceAdmins: &trueVal, + EnforceAdmins: &trueVal, // Maintain: base is more strict than rule }, }, }, { - name: "block deletion no bypass, while deletion is blocked with bypass", + name: "block deletion no bypass when block deletion with bypass", base: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -245,7 +239,92 @@ func Test_applyRepoRules(t *testing.T) { expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, - EnforceAdmins: &trueVal, + EnforceAdmins: &trueVal, // Upgrade: rule is more strict than base + }, + }, + }, + { + name: "block deletion no bypass when block force push no bypass", + base: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness + }, + }, + }, + { + name: "block deletion and force push no bypass when block deletion with bypass", + base: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + }, + }, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}, &repoRule{Type: "NON_FAST_FORWARD"})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, // Upgrade: rule is more strict than base + }, + }, + }, + { + name: "block deletion and force push with bypass when block force push no bypass", + base: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}, &repoRule{Type: "NON_FAST_FORWARD"}), withBypass()), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins + }, + }, + }, + { + name: "block deletion no bypass while force push is blocked with bypass", + base: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, + }, + }, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Maintain: deletion enforces but forcepush does not + }, + }, + }, + { + name: "block deletion no bypass while force push is blocked no bypass", + base: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness }, }, }, From e2a0fd9aa86525f5dd1fa708c37fd04b38588ae4 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 13:59:50 -0700 Subject: [PATCH 11/22] simplify enforce admin for now. Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 31 ++++---------- clients/githubrepo/branches_test.go | 66 ----------------------------- 2 files changed, 9 insertions(+), 88 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index fe29bc4867a..8ad4d1c347a 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -522,37 +522,24 @@ func translateRequiredStatusRepoRule(base *clients.BranchProtectionRule, rule *r } func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) { - var downgrade bool - upgrade := true - - // FIXME: upgrade/downgrade clauses need to compare every property? - if translated.AllowDeletions != nil && !*translated.AllowDeletions { + if base.AllowDeletions == nil || translated.AllowDeletions != nil && !*translated.AllowDeletions { base.AllowDeletions = translated.AllowDeletions - upgrade = upgrade && (base.AllowForcePushes == nil || base.AllowForcePushes == translated.AllowForcePushes) - downgrade = downgrade || (base.AllowForcePushes != nil && !*base.AllowForcePushes) } - if translated.AllowForcePushes != nil && !*translated.AllowForcePushes { + if base.AllowForcePushes == nil || translated.AllowForcePushes != nil && !*translated.AllowForcePushes { base.AllowForcePushes = translated.AllowForcePushes - upgrade = upgrade && (base.AllowDeletions == nil || base.AllowDeletions == translated.AllowDeletions) - downgrade = downgrade || (base.AllowDeletions != nil && !*base.AllowDeletions) + } + if base.EnforceAdmins == nil || translated.EnforceAdmins != nil && !*translated.EnforceAdmins { + // this is an over simplification to get preliminary support for repo rules merged. + // A more complete approach would process all rules without bypass actors first, + // then process those with bypass actors. If no settings improve (due to rule layering), + // then we can ignore the bypass actors. + base.EnforceAdmins = translated.EnforceAdmins } // FIXME: hacks to avoid breaking existing tests base.RequiredPullRequestReviews = translated.RequiredPullRequestReviews base.CheckRules = translated.CheckRules base.RequireLastPushApproval = translated.RequireLastPushApproval - - if base.EnforceAdmins == nil { - base.EnforceAdmins = translated.EnforceAdmins - } else if *base.EnforceAdmins != *translated.EnforceAdmins { - enforceAdmins := *base.EnforceAdmins - if upgrade { - enforceAdmins = true - } else if downgrade { - enforceAdmins = false - } - base.EnforceAdmins = &enforceAdmins - } } func readBoolPtr(b *bool) bool { diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 9052fb86127..26bd9a590de 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -211,72 +211,6 @@ func Test_applyRepoRules(t *testing.T) { }, }, }, - { - name: "block deletion with bypass when block deletion no bypass", - base: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - EnforceAdmins: &trueVal, - }, - }, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}), withBypass()), - expected: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - EnforceAdmins: &trueVal, // Maintain: base is more strict than rule - }, - }, - }, - { - name: "block deletion no bypass when block deletion with bypass", - base: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - EnforceAdmins: &falseVal, - }, - }, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), - expected: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - EnforceAdmins: &trueVal, // Upgrade: rule is more strict than base - }, - }, - }, - { - name: "block deletion no bypass when block force push no bypass", - base: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, - }, - }, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), - expected: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness - }, - }, - }, - { - name: "block deletion and force push no bypass when block deletion with bypass", - base: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - EnforceAdmins: &falseVal, - }, - }, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}, &repoRule{Type: "NON_FAST_FORWARD"})), - expected: &clients.BranchRef{ - BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, // Upgrade: rule is more strict than base - }, - }, - }, { name: "block deletion and force push with bypass when block force push no bypass", base: &clients.BranchRef{ From 44fa7952ff9736713176ab925805252a2af02c6a Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 14:12:46 -0700 Subject: [PATCH 12/22] handle merging pull request reviews Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 8ad4d1c347a..e59b01390db 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -537,11 +537,23 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) } // FIXME: hacks to avoid breaking existing tests - base.RequiredPullRequestReviews = translated.RequiredPullRequestReviews + mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews) base.CheckRules = translated.CheckRules base.RequireLastPushApproval = translated.RequireLastPushApproval } +func mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) { + if readIntPtr(translated.RequiredApprovingReviewCount) > readIntPtr(base.RequiredApprovingReviewCount) { + base.RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount + } + if base.DismissStaleReviews == nil || readBoolPtr(translated.DismissStaleReviews) { + base.DismissStaleReviews = translated.DismissStaleReviews + } + if base.RequireCodeOwnerReviews == nil || readBoolPtr(translated.RequireCodeOwnerReviews) { + base.RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews + } +} + func readBoolPtr(b *bool) bool { if b == nil { return false From 02bdcaf62d6f48ef8835d2b6e5e0a52f277ba7ea Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 14:21:44 -0700 Subject: [PATCH 13/22] handle merging check rules Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index e59b01390db..b333aa7daa7 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-github/v53/github" "github.com/shurcooL/githubv4" + "golang.org/x/exp/slices" "github.com/ossf/scorecard/v4/clients" "github.com/ossf/scorecard/v4/clients/githubrepo/internal/fnmatch" @@ -538,10 +539,25 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) // FIXME: hacks to avoid breaking existing tests mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews) - base.CheckRules = translated.CheckRules + mergeCheckRules(&base.CheckRules, &translated.CheckRules) base.RequireLastPushApproval = translated.RequireLastPushApproval } +func mergeCheckRules(base, translated *clients.StatusChecksRule) { + if base.UpToDateBeforeMerge == nil || readBoolPtr(translated.UpToDateBeforeMerge) { + base.UpToDateBeforeMerge = translated.UpToDateBeforeMerge + } + if base.RequiresStatusChecks == nil || readBoolPtr(translated.RequiresStatusChecks) { + base.RequiresStatusChecks = translated.RequiresStatusChecks + } + for _, context := range translated.Contexts { + // this isn't optimal, but probably not a bottleneck. + if !slices.Contains(base.Contexts, context) { + base.Contexts = append(base.Contexts, context) + } + } +} + func mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) { if readIntPtr(translated.RequiredApprovingReviewCount) > readIntPtr(base.RequiredApprovingReviewCount) { base.RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount From f2fba75306c8602f1388a3919a2a769c3f4c9741 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 14:26:31 -0700 Subject: [PATCH 14/22] handle last push approval Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index b333aa7daa7..accf7901ed5 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -536,11 +536,11 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) // then we can ignore the bypass actors. base.EnforceAdmins = translated.EnforceAdmins } - - // FIXME: hacks to avoid breaking existing tests + if base.RequireLastPushApproval == nil || readBoolPtr(translated.RequireLastPushApproval) { + base.RequireLastPushApproval = translated.RequireLastPushApproval + } mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews) mergeCheckRules(&base.CheckRules, &translated.CheckRules) - base.RequireLastPushApproval = translated.RequireLastPushApproval } func mergeCheckRules(base, translated *clients.StatusChecksRule) { From f4d236b25f7957b4a4cf7b60097eb481427eb927 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 14:33:27 -0700 Subject: [PATCH 15/22] handle linear history Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 6 ++++++ clients/githubrepo/branches_test.go | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index accf7901ed5..5a5787bc15d 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -469,6 +469,7 @@ nextRule: func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { falseVal := false + trueVal := true for _, r := range rules { adminEnforced := len(r.BypassActors.Nodes) == 0 translated := clients.BranchProtectionRule{ @@ -481,6 +482,8 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { translated.AllowDeletions = &falseVal case "NON_FAST_FORWARD": translated.AllowForcePushes = &falseVal + case "REQUIRED_LINEAR_HISTORY": + translated.RequireLinearHistory = &trueVal case "PULL_REQUEST": translatePullRequestRepoRule(&translated, rule) case "REQUIRED_STATUS_CHECKS": @@ -539,6 +542,9 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) if base.RequireLastPushApproval == nil || readBoolPtr(translated.RequireLastPushApproval) { base.RequireLastPushApproval = translated.RequireLastPushApproval } + if base.RequireLinearHistory == nil || readBoolPtr(translated.RequireLinearHistory) { + base.RequireLinearHistory = translated.RequireLinearHistory + } mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews) mergeCheckRules(&base.CheckRules, &translated.CheckRules) } diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 26bd9a590de..1ed8123b5ed 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -273,6 +273,17 @@ func Test_applyRepoRules(t *testing.T) { }, }, }, + { + name: "require linear history no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: "REQUIRED_LINEAR_HISTORY"})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + RequireLinearHistory: &trueVal, + EnforceAdmins: &trueVal, + }, + }, + }, { name: "require pull request no bypass", base: &clients.BranchRef{}, From 7e66fc069d216d90110aee94816a347f66f8af86 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 15:59:40 -0700 Subject: [PATCH 16/22] use constants for github rule types. Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 15 ++++++++++----- clients/githubrepo/branches_test.go | 16 ++++++++-------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 5a5787bc15d..0f58629332a 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -432,6 +432,11 @@ func isPermissionsError(err error) bool { const ( ruleConditionDefaultBranch = "~DEFAULT_BRANCH" ruleConditionAllBranches = "~ALL" + ruleDeletion = "DELETION" + ruleForcePush = "NON_FAST_FORWARD" + ruleLinear = "REQUIRED_LINEAR_HISTORY" + rulePullRequest = "PULL_REQUEST" + ruleStatusCheck = "REQUIRED_STATUS_CHECKS" ) func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) ([]*repoRuleSet, error) { @@ -478,15 +483,15 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { for _, rule := range r.Rules.Nodes { switch rule.Type { - case "DELETION": + case ruleDeletion: translated.AllowDeletions = &falseVal - case "NON_FAST_FORWARD": + case ruleForcePush: translated.AllowForcePushes = &falseVal - case "REQUIRED_LINEAR_HISTORY": + case ruleLinear: translated.RequireLinearHistory = &trueVal - case "PULL_REQUEST": + case rulePullRequest: translatePullRequestRepoRule(&translated, rule) - case "REQUIRED_STATUS_CHECKS": + case ruleStatusCheck: translateRequiredStatusRepoRule(&translated, rule) } } diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 1ed8123b5ed..9ac5426c3d0 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -192,7 +192,7 @@ func Test_applyRepoRules(t *testing.T) { { name: "block deletion no bypass", base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), + ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -203,7 +203,7 @@ func Test_applyRepoRules(t *testing.T) { { name: "block deletion with bypass", base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}), withBypass()), + ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion}), withBypass()), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -219,7 +219,7 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, }, }, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"}, &repoRule{Type: "NON_FAST_FORWARD"}), withBypass()), + ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion}, &repoRule{Type: ruleForcePush}), withBypass()), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -236,7 +236,7 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &falseVal, }, }, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), + ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -253,7 +253,7 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, }, }, - ruleSet: ruleSet(withRules(&repoRule{Type: "DELETION"})), + ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: &falseVal, @@ -265,7 +265,7 @@ func Test_applyRepoRules(t *testing.T) { { name: "block force push no bypass", base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: "NON_FAST_FORWARD"})), + ruleSet: ruleSet(withRules(&repoRule{Type: ruleForcePush})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ AllowForcePushes: &falseVal, @@ -276,7 +276,7 @@ func Test_applyRepoRules(t *testing.T) { { name: "require linear history no bypass", base: &clients.BranchRef{}, - ruleSet: ruleSet(withRules(&repoRule{Type: "REQUIRED_LINEAR_HISTORY"})), + ruleSet: ruleSet(withRules(&repoRule{Type: ruleLinear})), expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ RequireLinearHistory: &trueVal, @@ -288,7 +288,7 @@ func Test_applyRepoRules(t *testing.T) { name: "require pull request no bypass", base: &clients.BranchRef{}, ruleSet: ruleSet(withRules(&repoRule{ - Type: "PULL_REQUEST", + Type: rulePullRequest, Parameters: repoRulesParameters{ PullRequestParameters: pullRequestRuleParameters{ DismissStaleReviewsOnPush: &trueVal, From 89a3f75e5b6e30d766ba5e61d2022c7449f2e337 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 16:08:58 -0700 Subject: [PATCH 17/22] add status check test. Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 9 +++++---- clients/githubrepo/branches_test.go | 31 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 0f58629332a..2b41116a9b5 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -167,10 +167,11 @@ type pullRequestRuleParameters struct { } type requiredStatusCheckParameters struct { StrictRequiredStatusChecksPolicy *bool - RequiredStatusChecks []struct { - Context *string - IntegrationID *int64 - } + RequiredStatusChecks []statusCheck +} +type statusCheck struct { + Context *string + IntegrationID *int64 } type repoRule struct { Type string diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index 9ac5426c3d0..73a9401e141 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -311,6 +311,33 @@ func Test_applyRepoRules(t *testing.T) { }, }, }, + { + name: "required status checks no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{ + Type: ruleStatusCheck, + Parameters: repoRulesParameters{ + StatusCheckParameters: requiredStatusCheckParameters{ + StrictRequiredStatusChecksPolicy: &trueVal, + RequiredStatusChecks: []statusCheck{ + { + Context: stringPtr("foo"), + }, + }, + }, + }, + })), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + EnforceAdmins: &trueVal, + CheckRules: clients.StatusChecksRule{ + UpToDateBeforeMerge: &trueVal, + RequiresStatusChecks: &trueVal, + Contexts: []string{"foo"}, + }, + }, + }, + }, } for _, testcase := range testcases { @@ -326,3 +353,7 @@ func Test_applyRepoRules(t *testing.T) { }) } } + +func stringPtr(s string) *string { + return &s +} From 48f85eb97d7366d8b3ba212a05adee300950f874 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 6 Sep 2023 16:35:02 -0700 Subject: [PATCH 18/22] add e2e test for repo rules. Signed-off-by: Spencer Schrock --- e2e/branch_protection_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index 7a386273346..114412d1158 100644 --- a/e2e/branch_protection_test.go +++ b/e2e/branch_protection_test.go @@ -143,3 +143,34 @@ var _ = Describe("E2E TEST GITHUB_TOKEN:"+checks.CheckBranchProtection, func() { }) }) }) + +var _ = Describe("E2E TEST:"+checks.CheckBranchProtection+" (repo rules)", func() { + Context("E2E TEST:Validating branch protection with repo rules", func() { + It("Should be able to read repo rules", func() { + dl := scut.TestDetailLogger{} + // no force push, no deletion, no bypass, dismiss stale reviews + repo, err := githubrepo.MakeGithubRepo("ossf-tests/scorecard-check-repo-rules-e2e") + Expect(err).Should(BeNil()) + repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger) + err = repoClient.InitRepo(repo, clients.HeadSHA, 0) + Expect(err).Should(BeNil()) + req := checker.CheckRequest{ + Ctx: context.Background(), + RepoClient: repoClient, + Repo: repo, + Dlogger: &dl, + } + expected := scut.TestReturn{ + Error: nil, + Score: 3, + NumberOfWarn: 2, + NumberOfInfo: 4, + NumberOfDebug: 2, + } + result := checks.BranchProtection(&req) + Expect(result.Error).Should(BeNil()) + Expect(scut.ValidateTestReturn(nil, "repo rules accessible", &expected, &result, &dl)).Should(BeTrue()) + Expect(repoClient.Close()).Should(BeNil()) + }) + }) +}) From 42083ca13f06991549191aad6b7a212cc8513db9 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 13 Sep 2023 11:58:03 -0700 Subject: [PATCH 19/22] handle nil branch name data Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 2b41116a9b5..3c65430c4b0 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -364,6 +364,9 @@ func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { } func getDefaultBranchNameFrom(data *ruleSetData) string { + if data == nil || data.Repository.DefaultBranchRef.Name == nil { + return "" + } return *data.Repository.DefaultBranchRef.Name } From a7fa71f2b4a3666e43518213306d12a2b35a7071 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 13 Sep 2023 12:51:16 -0700 Subject: [PATCH 20/22] add tracking issue. Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 1 + 1 file changed, 1 insertion(+) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 3c65430c4b0..e94537b301f 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -546,6 +546,7 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) // A more complete approach would process all rules without bypass actors first, // then process those with bypass actors. If no settings improve (due to rule layering), // then we can ignore the bypass actors. + // https://github.com/ossf/scorecard/issues/3480 base.EnforceAdmins = translated.EnforceAdmins } if base.RequireLastPushApproval == nil || readBoolPtr(translated.RequireLastPushApproval) { From e26cb913556f8697d35783275790d77b4ff34036 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 13 Sep 2023 13:42:44 -0700 Subject: [PATCH 21/22] fix precedence in if statement Signed-off-by: Spencer Schrock --- clients/githubrepo/branches.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index e94537b301f..3ddfac7054d 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -269,7 +269,7 @@ func (handler *branchesHandler) setup() error { // Ignore permissions errors if we know the repository is using rulesets, so non-admins can still get a score. handler.data = new(defaultBranchData) if err := handler.graphClient.Query(handler.ctx, handler.data, vars); err != nil && - !isPermissionsError(err) || len(handler.ruleSets) == 0 { + (!isPermissionsError(err) || len(handler.ruleSets) == 0) { handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("githubv4.Query: %v", err)) return } From 237637a31055b3b0ef2e2847bfda228714fee3aa Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 14 Sep 2023 11:58:38 -0700 Subject: [PATCH 22/22] include repo rules in the check docs. Signed-off-by: Spencer Schrock --- docs/checks.md | 8 +++++--- docs/checks/internal/checks.yaml | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/checks.md b/docs/checks.md index 8f7d5c026d0..9232cefac21 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -59,7 +59,8 @@ Allowed by Scorecard: Risk: `High` (vulnerable to intentional malicious code injection) This check determines whether a project's default and release branches are -protected with GitHub's [branch protection](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches) settings. +protected with GitHub's [branch protection](https://docs.github.com/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches) +or [repository rules](https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets) settings. Branch protection allows maintainers to define rules that enforce certain workflows for branches, such as requiring review or passing certain status checks before acceptance into a main branch, or preventing rewriting of @@ -68,8 +69,9 @@ public history. Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmins`, `RequireLastPushApproval`, `RequiresStatusChecks` and `UpToDateBeforeMerge`. If the provided token does not have admin access, the check will query the branch settings accessible to non-admins and provide results based only on these settings. -Even so, we recommend using a non-admin token, which provides a thorough enough -result to meet most user needs. +However, all of these settings are accessible via Repo Rules. `EnforceAdmins` is calculated slightly differently. +This setting is calculated as `false` if any [Bypass Actors](https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/creating-rulesets-for-a-repository#granting-bypass-permissions-for-your-ruleset) + are defined on any rule, regardless of if they are admins. Different types of branch protection protect against different risks: diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 6151c64f516..a6112220150 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -148,7 +148,8 @@ checks: Risk: `High` (vulnerable to intentional malicious code injection) This check determines whether a project's default and release branches are - protected with GitHub's [branch protection](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches) settings. + protected with GitHub's [branch protection](https://docs.github.com/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches) + or [repository rules](https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets) settings. Branch protection allows maintainers to define rules that enforce certain workflows for branches, such as requiring review or passing certain status checks before acceptance into a main branch, or preventing rewriting of @@ -157,8 +158,9 @@ checks: Note: The following settings queried by the Branch-Protection check require an admin token: `DismissStaleReviews`, `EnforceAdmins`, `RequireLastPushApproval`, `RequiresStatusChecks` and `UpToDateBeforeMerge`. If the provided token does not have admin access, the check will query the branch settings accessible to non-admins and provide results based only on these settings. - Even so, we recommend using a non-admin token, which provides a thorough enough - result to meet most user needs. + However, all of these settings are accessible via Repo Rules. `EnforceAdmins` is calculated slightly differently. + This setting is calculated as `false` if any [Bypass Actors](https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/creating-rulesets-for-a-repository#granting-bypass-permissions-for-your-ruleset) + are defined on any rule, regardless of if they are admins. Different types of branch protection protect against different risks: