Skip to content

Commit

Permalink
fix: handle nil repo in GitHub event payloads
Browse files Browse the repository at this point in the history
We were getting a crash on the controller due to a nil pointer dereference
when the repository was nil in the payload. This commit adds checks to
ensure the repository is not nil before accessing its properties.

TestSkippedEvent shows the issue but it wasn't detected that the controller
was crashing and happily returned ok.
  • Loading branch information
chmouel committed Dec 19, 2024
1 parent 59c1272 commit 7fe2797
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 1 deletion.
15 changes: 15 additions & 0 deletions pkg/provider/github/parse_payload.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,9 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt
return nil, err
}
case *github.PushEvent:
if gitEvent.GetRepo() == nil {
return nil, errors.New("error parsing payload the repository should not be nil")
}
processedEvent.Organization = gitEvent.GetRepo().GetOwner().GetLogin()
processedEvent.Repository = gitEvent.GetRepo().GetName()
processedEvent.DefaultBranch = gitEvent.GetRepo().GetDefaultBranch()
Expand All @@ -274,6 +277,9 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt
processedEvent.HeadURL = processedEvent.BaseURL // in push events Head URL is the same as BaseURL
case *github.PullRequestEvent:
processedEvent.Repository = gitEvent.GetRepo().GetName()
if gitEvent.GetRepo() == nil {
return nil, errors.New("error parsing payload the repository should not be nil")
}
processedEvent.Organization = gitEvent.GetRepo().Owner.GetLogin()
processedEvent.DefaultBranch = gitEvent.GetRepo().GetDefaultBranch()
processedEvent.SHA = gitEvent.GetPullRequest().Head.GetSHA()
Expand Down Expand Up @@ -306,6 +312,9 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt

func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.CheckRunEvent) (*info.Event, error) {
runevent := info.NewEvent()
if event.GetRepo() == nil {
return nil, errors.New("error parsing payload the repository should not be nil")
}
runevent.Organization = event.GetRepo().GetOwner().GetLogin()
runevent.Repository = event.GetRepo().GetName()
runevent.URL = event.GetRepo().GetHTMLURL()
Expand All @@ -331,6 +340,9 @@ func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.Check

func (v *Provider) handleCheckSuites(ctx context.Context, event *github.CheckSuiteEvent) (*info.Event, error) {
runevent := info.NewEvent()
if event.GetRepo() == nil {
return nil, errors.New("error parsing payload the repository should not be nil")
}
runevent.Organization = event.GetRepo().GetOwner().GetLogin()
runevent.Repository = event.GetRepo().GetName()
runevent.URL = event.GetRepo().GetHTMLURL()
Expand Down Expand Up @@ -393,6 +405,9 @@ func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.Is
func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.CommitCommentEvent) (*info.Event, error) {
action := "push"
runevent := info.NewEvent()
if event.GetRepo() == nil {
return nil, errors.New("error parsing payload the repository should not be nil")
}
runevent.Organization = event.GetRepo().GetOwner().GetLogin()
runevent.Repository = event.GetRepo().GetName()
runevent.Sender = event.GetSender().GetLogin()
Expand Down
18 changes: 18 additions & 0 deletions pkg/provider/github/parse_payload_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github"
"github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger"
Expand Down Expand Up @@ -88,6 +89,9 @@ var samplePRAnother = github.PullRequest{
}

func TestParsePayLoad(t *testing.T) {
samplePRNoRepo := samplePRevent
samplePRNoRepo.Repo = nil

tests := []struct {
name string
wantErrString string
Expand Down Expand Up @@ -185,6 +189,20 @@ func TestParsePayLoad(t *testing.T) {
},
shaRet: "samplePRsha",
},
{
name: "bad/pull request",
eventType: "pull_request",
triggerTarget: triggertype.PullRequest.String(),
payloadEventStruct: samplePRNoRepo,
wantErrString: "error parsing payload the repository should not be nil",
},
{
name: "bad/push",
eventType: "push",
triggerTarget: triggertype.Push.String(),
payloadEventStruct: github.PushEvent{},
wantErrString: "error parsing payload the repository should not be nil",
},
{
// specific run from a check_suite
name: "good/rerequest check_run on pull request",
Expand Down
2 changes: 1 addition & 1 deletion test/invalid_event_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestSkippedEvent(t *testing.T) {
assert.NilError(t, err)
defer resp.Body.Close()

assert.Equal(t, resp.StatusCode, http.StatusOK, "%s reply expected 200 OK", elURL)
assert.Equal(t, resp.StatusCode, http.StatusAccepted, "%s reply expected 202 OK", elURL)
}

func TestGETCall(t *testing.T) {
Expand Down

0 comments on commit 7fe2797

Please sign in to comment.