Skip to content

Commit

Permalink
fix(github): prevent null pointer dereferencing when using AllowMerge…
Browse files Browse the repository at this point in the history
…ableBypassApply with no required checks on branch protection (#3672)

* Implement test for allow mergeable bypass apply with no branch protection checks

* Report true combined status if no branch protection required status checks

* Remove warning on feature flag utilization
  • Loading branch information
arturhoo authored Aug 15, 2023
1 parent cec25e5 commit 049c146
Show file tree
Hide file tree
Showing 6 changed files with 386 additions and 5 deletions.
5 changes: 0 additions & 5 deletions runatlantis.io/docs/server-configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -564,11 +564,6 @@ This is useful when you have many projects and want to keep the pull request cle
```
Feature flag to enable ability to use `mergeable` mode with required apply status check.

::: warning NOTE
If there aren't any required checks set in the Github branch protection settings then this will cause atlantis to fail.
See issue https://github.com/runatlantis/atlantis/issues/2663.
:::

### `--gitlab-hostname`
```bash
atlantis server --gitlab-hostname="my.gitlab.enterprise.com"
Expand Down
4 changes: 4 additions & 0 deletions server/events/vcs/github_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,10 @@ func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *githu
return false, errors.Wrap(err, "getting required status checks")
}

if required.RequiredStatusChecks == nil {
return true, nil
}

//check check suite/check run api
checksuites, _, err := g.client.Checks.ListCheckSuitesForRef(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil)
if err != nil {
Expand Down
108 changes: 108 additions & 0 deletions server/events/vcs/github_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,114 @@ func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T)
}
}

func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApplyButWithNoBranchProtectionChecks(t *testing.T) {
vcsStatusName := "atlantis"
cases := []struct {
state string
reviewDecision string
expMergeable bool
}{
{
"blocked",
`"REVIEW_REQUIRED"`,
false,
},
}

// Use a real GitHub json response and edit the mergeable_state field.
jsBytes, err := os.ReadFile("testdata/github-pull-request.json")
Ok(t, err)
prJSON := string(jsBytes)

// Status Check Response
jsBytes, err = os.ReadFile("testdata/github-commit-status-full.json")
Ok(t, err)
commitJSON := string(jsBytes)

// Branch protection Response
jsBytes, err = os.ReadFile("testdata/github-branch-protection-no-required-checks.json")
Ok(t, err)
branchProtectionJSON := string(jsBytes)

// List check suites Response
jsBytes, err = os.ReadFile("testdata/github-commit-check-suites-completed.json")
Ok(t, err)
checkSuites := string(jsBytes)

// List check runs in a check suite
jsBytes, err = os.ReadFile("testdata/github-commit-check-suites-check-runs-completed.json")
Ok(t, err)
checkRuns := string(jsBytes)

for _, c := range cases {
t.Run(c.state, func(t *testing.T) {
response := strings.Replace(prJSON,
`"mergeable_state": "clean"`,
fmt.Sprintf(`"mergeable_state": "%s"`, c.state),
1,
)

// reviewDecision Response
reviewDecision := fmt.Sprintf(`{
"data": {
"repository": {
"pullRequest": {
"reviewDecision": %s
}
}
}
}`, c.reviewDecision)

testServer := httptest.NewTLSServer(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.RequestURI {
case "/api/v3/repos/octocat/Hello-World/pulls/1":
w.Write([]byte(response)) // nolint: errcheck
return
case "/api/v3/repos/octocat/Hello-World/pulls/1/reviews?per_page=300":
w.Write([]byte("[]")) // nolint: errcheck
return
case "/api/v3/repos/octocat/Hello-World/commits/new-topic/status":
w.Write([]byte(commitJSON)) // nolint: errcheck
case "/api/graphql":
w.Write([]byte(reviewDecision)) // nolint: errcheck
case "/api/v3/repos/octocat/Hello-World/branches/main/protection":
w.Write([]byte(branchProtectionJSON)) // nolint: errcheck
case "/api/v3/repos/octocat/Hello-World/commits/new-topic/check-suites":
w.Write([]byte(checkSuites)) // nolint: errcheck
case "/api/v3/repos/octocat/Hello-World/check-suites/1234567890/check-runs":
w.Write([]byte(checkRuns)) // nolint: errcheck
default:
t.Errorf("got unexpected request at %q", r.RequestURI)
http.Error(w, "not found", http.StatusNotFound)
return
}
}))
testServerURL, err := url.Parse(testServer.URL)
Ok(t, err)
client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{"user", "pass"}, vcs.GithubConfig{AllowMergeableBypassApply: true}, logging.NewNoopLogger(t))
Ok(t, err)
defer disableSSLVerification()()

actMergeable, err := client.PullIsMergeable(models.Repo{
FullName: "octocat/Hello-World",
Owner: "octocat",
Name: "Hello-World",
CloneURL: "",
SanitizedCloneURL: "",
VCSHost: models.VCSHost{
Type: models.Github,
Hostname: "github.com",
},
}, models.PullRequest{
Num: 1,
}, vcsStatusName)
Ok(t, err)
Equals(t, c.expMergeable, actMergeable)
})
}
}

func TestGithubClient_MergePullHandlesError(t *testing.T) {
cases := []struct {
code int
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"url": "https://api.github.com/repos/octocat/repo/branches/master/protection",
"required_pull_request_reviews": {
"url": "https://api.github.com/repos/octocat/repo/branches/master/protection/required_pull_request_reviews",
"dismiss_stale_reviews": false,
"require_code_owner_reviews": false,
"require_last_push_approval": false,
"required_approving_review_count": 1
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
{
"total_count": 1,
"check_runs": [
{
"id": 4,
"head_sha": "ce587453ced02b1526dfb4cb910479d431683101",
"node_id": "MDg6Q2hlY2tSdW40",
"external_id": "",
"url": "https://api.github.com/repos/github/hello-world/check-runs/4",
"html_url": "https://github.com/github/hello-world/runs/4",
"details_url": "https://example.com",
"status": "completed",
"conclusion": "success",
"started_at": "2018-05-04T01:14:52Z",
"completed_at": "2018-05-04T01:14:52Z",
"output": {
"title": "Mighty Readme report",
"summary": "There are 0 failures, 2 warnings, and 1 notice.",
"text": "You may have some misspelled words on lines 2 and 4. You also may want to add a section in your README about how to install your app.",
"annotations_count": 2,
"annotations_url": "https://api.github.com/repos/github/hello-world/check-runs/4/annotations"
},
"name": "mighty_readme",
"check_suite": {
"id": 5
},
"app": {
"id": 1,
"slug": "octoapp",
"node_id": "MDExOkludGVncmF0aW9uMQ==",
"owner": {
"login": "github",
"id": 1,
"node_id": "MDEyOk9yZ2FuaXphdGlvbjE=",
"url": "https://api.github.com/orgs/github",
"repos_url": "https://api.github.com/orgs/github/repos",
"events_url": "https://api.github.com/orgs/github/events",
"avatar_url": "https://github.com/images/error/octocat_happy.gif",
"gravatar_id": "",
"html_url": "https://github.com/octocat",
"followers_url": "https://api.github.com/users/octocat/followers",
"following_url": "https://api.github.com/users/octocat/following{/other_user}",
"gists_url": "https://api.github.com/users/octocat/gists{/gist_id}",
"starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/octocat/subscriptions",
"organizations_url": "https://api.github.com/users/octocat/orgs",
"received_events_url": "https://api.github.com/users/octocat/received_events",
"type": "User",
"site_admin": true
},
"name": "Octocat App",
"description": "",
"external_url": "https://example.com",
"html_url": "https://github.com/apps/octoapp",
"created_at": "2017-07-08T16:18:44-04:00",
"updated_at": "2017-07-08T16:18:44-04:00",
"permissions": {
"metadata": "read",
"contents": "read",
"issues": "write",
"single_file": "write"
},
"events": [
"push",
"pull_request"
]
},
"pull_requests": [
{
"url": "https://api.github.com/repos/github/hello-world/pulls/1",
"id": 1934,
"number": 3956,
"head": {
"ref": "say-hello",
"sha": "3dca65fa3e8d4b3da3f3d056c59aee1c50f41390",
"repo": {
"id": 526,
"url": "https://api.github.com/repos/github/hello-world",
"name": "hello-world"
}
},
"base": {
"ref": "master",
"sha": "e7fdf7640066d71ad16a86fbcbb9c6a10a18af4f",
"repo": {
"id": 526,
"url": "https://api.github.com/repos/github/hello-world",
"name": "hello-world"
}
}
}
]
}
]
}
Loading

0 comments on commit 049c146

Please sign in to comment.