From e515c2a8bd6ac92a08e275d021551adae8f780d4 Mon Sep 17 00:00:00 2001 From: Pete Wagner <1559510+thepwagner@users.noreply.github.com> Date: Thu, 14 Sep 2023 16:56:41 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Support=20Branch-Protection=20via?= =?UTF-8?q?=20GitHub=20Repository=20Rules=20(#3354)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * repo rulesets via v4 api Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * good enough fnmatch implementation. Signed-off-by: Spencer Schrock * good enough rulesMatchingBranch Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * apply matching repo rules to branch protection settings Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * rules: consider admins and require checks Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * non-structural chanages from PR feedback Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * fetch default branch name during repo rules query Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * 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> * Test_applyRepoRules: builder and standardize names Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * attempt to upgrade/downgrade EnforceAdmins as each rule is applied Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> * simplify enforce admin for now. Signed-off-by: Spencer Schrock * handle merging pull request reviews Signed-off-by: Spencer Schrock * handle merging check rules Signed-off-by: Spencer Schrock * handle last push approval Signed-off-by: Spencer Schrock * handle linear history Signed-off-by: Spencer Schrock * use constants for github rule types. Signed-off-by: Spencer Schrock * add status check test. Signed-off-by: Spencer Schrock * add e2e test for repo rules. Signed-off-by: Spencer Schrock * handle nil branch name data Signed-off-by: Spencer Schrock * add tracking issue. Signed-off-by: Spencer Schrock * fix precedence in if statement Signed-off-by: Spencer Schrock * include repo rules in the check docs. Signed-off-by: Spencer Schrock --------- Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com> Signed-off-by: Spencer Schrock Co-authored-by: Spencer Schrock --- clients/githubrepo/branches.go | 389 ++++++++++++++++-- clients/githubrepo/branches_test.go | 359 ++++++++++++++++ .../githubrepo/internal/fnmatch/fnmatch.go | 94 +++++ .../internal/fnmatch/fnmatch_test.go | 66 +++ docs/checks.md | 8 +- docs/checks/internal/checks.yaml | 8 +- e2e/branch_protection_test.go | 31 ++ 7 files changed, 920 insertions(+), 35 deletions(-) create mode 100644 clients/githubrepo/branches_test.go create mode 100644 clients/githubrepo/internal/fnmatch/fnmatch.go create mode 100644 clients/githubrepo/internal/fnmatch/fnmatch_test.go diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 5779d72c110..3ddfac7054d 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -22,8 +22,10 @@ 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" sce "github.com/ossf/scorecard/v4/errors" ) @@ -33,23 +35,23 @@ const ( // See https://github.community/t/graphql-api-protected-branch/14380 /* Example of query: - 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 +59,56 @@ const ( refUpdateRule { requiredApprovingReviewCount allowsForcePushes - ... + } + } + } + rulesets(first: 100) { + edges { + node { + name + enforcement + target + conditions { + refName { + exclude + include + } + } + 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 + } + } + } + } } } } @@ -107,6 +158,63 @@ type defaultBranchData struct { } } +type pullRequestRuleParameters struct { + DismissStaleReviewsOnPush *bool + RequireCodeOwnerReview *bool + RequireLastPushApproval *bool + RequiredApprovingReviewCount *int32 + RequiredReviewThreadResolution *bool +} +type requiredStatusCheckParameters struct { + StrictRequiredStatusChecksPolicy *bool + RequiredStatusChecks []statusCheck +} +type statusCheck struct { + Context *string + IntegrationID *int64 +} +type repoRule struct { + Type string + Parameters repoRulesParameters +} +type repoRulesParameters struct { + PullRequestParameters pullRequestRuleParameters `graphql:"... on PullRequestParameters"` + StatusCheckParameters requiredStatusCheckParameters `graphql:"... on RequiredStatusChecksParameters"` +} +type ruleSetConditionRefs struct { + Include []string + Exclude []string +} +type ruleSetCondition struct { + RefName ruleSetConditionRefs +} +type ruleSetBypass struct { + BypassMode *string + OrganizationAdmin *bool + RepositoryRoleName *string +} +type repoRuleSet struct { + 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 { + DefaultBranchRef struct { + Name *string + } + Rulesets struct { + Nodes []*repoRuleSet + } `graphql:"rulesets(first: 100)"` + } `graphql:"repository(owner: $owner, name: $name)"` +} + type branchData struct { Repository struct { Ref *branch `graphql:"ref(qualifiedName: $branchRefName)"` @@ -114,14 +222,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 + 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) { @@ -130,6 +240,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 } @@ -143,12 +255,31 @@ func (handler *branchesHandler) setup() error { "owner": githubv4.String(handler.repourl.owner), "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.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 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 } - handler.defaultBranchRef = getBranchRefFrom(handler.data.Repository.DefaultBranchRef) + + rules, err := rulesMatchingBranch(handler.ruleSets, handler.defaultBranchName, true) + if err != nil { + handler.errSetup = sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("rulesMatchingBranch: %v", err)) + return + } + handler.defaultBranchRef = getBranchRefFrom(handler.data.Repository.DefaultBranchRef, rules) }) return handler.errSetup } @@ -157,6 +288,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), @@ -166,7 +301,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)) } - return getBranchRefFrom(queryData.Repository.Ref), nil + rules, err := rulesMatchingBranch(handler.ruleSets, branchName, branchName == handler.defaultBranchName) + if err != nil { + return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("rulesMatchingBranch: %v", err)) + } + return getBranchRefFrom(queryData.Repository.Ref, rules), nil } func (handler *branchesHandler) getDefaultBranch() (*clients.BranchRef, error) { @@ -224,7 +363,25 @@ func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { } } -func getBranchRefFrom(data *branch) *clients.BranchRef { +func getDefaultBranchNameFrom(data *ruleSetData) string { + if data == nil || data.Repository.DefaultBranchRef.Name == nil { + return "" + } + 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 } @@ -238,7 +395,8 @@ func getBranchRefFrom(data *branch) *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 { *branchRef.Protected = false return branchRef } @@ -266,5 +424,178 @@ func getBranchRefFrom(data *branch) *clients.BranchRef { copyNonAdminSettings(rule, branchRule) } + applyRepoRules(branchRef, rules) + return branchRef } + +func isPermissionsError(err error) bool { + return strings.Contains(err.Error(), "Resource not accessible") +} + +const ( + ruleConditionDefaultBranch = "~DEFAULT_BRANCH" + ruleConditionAllBranches = "~ALL" + ruleDeletion = "DELETION" + ruleForcePush = "NON_FAST_FORWARD" + ruleLinear = "REQUIRED_LINEAR_HISTORY" + rulePullRequest = "PULL_REQUEST" + ruleStatusCheck = "REQUIRED_STATUS_CHECKS" +) + +func rulesMatchingBranch(rules []*repoRuleSet, name string, defaultRef bool) ([]*repoRuleSet, error) { + refName := refPrefix + name + ret := make([]*repoRuleSet, 0) +nextRule: + for _, rule := range rules { + for _, cond := range rule.Conditions.RefName.Exclude { + if match, err := fnmatch.Match(cond, refName); err != nil { + return nil, fmt.Errorf("exclude match error: %w", err) + } else if match { + continue nextRule + } + } + + for _, cond := range rule.Conditions.RefName.Include { + if cond == ruleConditionAllBranches { + ret = append(ret, rule) + break + } + if cond == ruleConditionDefaultBranch && defaultRef { + ret = append(ret, rule) + break + } + + if match, err := fnmatch.Match(cond, refName); err != nil { + return nil, fmt.Errorf("include match error: %w", err) + } else if match { + ret = append(ret, rule) + } + } + } + return ret, nil +} + +func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { + falseVal := false + trueVal := true + for _, r := range rules { + adminEnforced := len(r.BypassActors.Nodes) == 0 + translated := clients.BranchProtectionRule{ + EnforceAdmins: &adminEnforced, + } + + for _, rule := range r.Rules.Nodes { + switch rule.Type { + case ruleDeletion: + translated.AllowDeletions = &falseVal + case ruleForcePush: + translated.AllowForcePushes = &falseVal + case ruleLinear: + translated.RequireLinearHistory = &trueVal + case rulePullRequest: + translatePullRequestRepoRule(&translated, rule) + case ruleStatusCheck: + translateRequiredStatusRepoRule(&translated, rule) + } + } + mergeBranchProtectionRules(&branchRef.BranchProtectionRule, &translated) + } +} + +func translatePullRequestRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { + if readBoolPtr(rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush) { + base.RequiredPullRequestReviews.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush + } + if readBoolPtr(rule.Parameters.PullRequestParameters.RequireCodeOwnerReview) { + base.RequiredPullRequestReviews.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview + } + if readBoolPtr(rule.Parameters.PullRequestParameters.RequireLastPushApproval) { + base.RequireLastPushApproval = rule.Parameters.PullRequestParameters.RequireLastPushApproval + } + if reviewerCount := readIntPtr(rule.Parameters.PullRequestParameters.RequiredApprovingReviewCount); reviewerCount > 0 { + base.RequiredPullRequestReviews.RequiredApprovingReviewCount = &reviewerCount + } +} + +func translateRequiredStatusRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { + statusParams := rule.Parameters.StatusCheckParameters + if len(statusParams.RequiredStatusChecks) == 0 { + return + } + enabled := true + base.CheckRules.RequiresStatusChecks = &enabled + base.CheckRules.UpToDateBeforeMerge = statusParams.StrictRequiredStatusChecksPolicy + for _, chk := range statusParams.RequiredStatusChecks { + if chk.Context == nil { + continue + } + base.CheckRules.Contexts = append(base.CheckRules.Contexts, *chk.Context) + } +} + +func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) { + if base.AllowDeletions == nil || translated.AllowDeletions != nil && !*translated.AllowDeletions { + base.AllowDeletions = translated.AllowDeletions + } + if base.AllowForcePushes == nil || translated.AllowForcePushes != nil && !*translated.AllowForcePushes { + base.AllowForcePushes = translated.AllowForcePushes + } + 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. + // https://github.com/ossf/scorecard/issues/3480 + base.EnforceAdmins = translated.EnforceAdmins + } + 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) +} + +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 + } + 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 + } + 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 new file mode 100644 index 00000000000..73a9401e141 --- /dev/null +++ b/clients/githubrepo/branches_test.go @@ -0,0 +1,359 @@ +// 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 ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + + "github.com/ossf/scorecard/v4/clients" +) + +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{"refs/heads/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{"refs/heads/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{"refs/heads/foo/**"}, + Exclude: []string{"refs/heads/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) + } + }) + } + }) + } +} + +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() ruleSetOpt { + return func(r *repoRuleSet) { + r.BypassActors.Nodes = append(r.BypassActors.Nodes, &ruleSetBypass{}) + } +} + +func Test_applyRepoRules(t *testing.T) { + t.Parallel() + trueVal := true + falseVal := false + twoVal := int32(2) + + testcases := []struct { + base *clients.BranchRef + ruleSet *repoRuleSet + expected *clients.BranchRef + ruleBypass *ruleSetBypass + name string + }{ + { + name: "block deletion no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + }, + { + name: "block deletion with bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: ruleDeletion}), withBypass()), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + }, + }, + }, + { + 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: ruleDeletion}, &repoRule{Type: ruleForcePush}), 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: ruleDeletion})), + 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: ruleDeletion})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness + }, + }, + }, + { + name: "block force push no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: ruleForcePush})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + }, + }, + }, + { + name: "require linear history no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{Type: ruleLinear})), + expected: &clients.BranchRef{ + BranchProtectionRule: clients.BranchProtectionRule{ + RequireLinearHistory: &trueVal, + EnforceAdmins: &trueVal, + }, + }, + }, + { + name: "require pull request no bypass", + base: &clients.BranchRef{}, + ruleSet: ruleSet(withRules(&repoRule{ + Type: rulePullRequest, + 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, + }, + }, + }, + }, + { + 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 { + testcase := testcase + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + applyRepoRules(testcase.base, []*repoRuleSet{testcase.ruleSet}) + + 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) + } + }) + } +} + +func stringPtr(s string) *string { + return &s +} diff --git a/clients/githubrepo/internal/fnmatch/fnmatch.go b/clients/githubrepo/internal/fnmatch/fnmatch.go new file mode 100644 index 00000000000..fdd26e4c3be --- /dev/null +++ b/clients/githubrepo/internal/fnmatch/fnmatch.go @@ -0,0 +1,94 @@ +// 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 ( + "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..3059568b051 --- /dev/null +++ b/clients/githubrepo/internal/fnmatch/fnmatch_test.go @@ -0,0 +1,66 @@ +// 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 ( + "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) + } + } +} diff --git a/docs/checks.md b/docs/checks.md index ed52b0c96f3..a7a130e03a6 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 2d2fd889186..352cc1ad107 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: 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()) + }) + }) +})