From a0cd25b9c3161ca8a3ac5a9f5b5fdd9aa81c55b9 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Wed, 18 Dec 2024 17:42:19 +0100 Subject: [PATCH] fix: handle nil repo in GitHub event payloads 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. Signed-off-by: Chmouel Boudjnah --- pkg/provider/github/parse_payload.go | 15 +++++++++++++++ pkg/provider/github/parse_payload_test.go | 18 ++++++++++++++++++ test/invalid_event_test.go | 2 +- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index 586e67a11..2ff3bb6d8 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -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() @@ -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() @@ -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() @@ -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() @@ -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() diff --git a/pkg/provider/github/parse_payload_test.go b/pkg/provider/github/parse_payload_test.go index b6cbbc89f..ab827becc 100644 --- a/pkg/provider/github/parse_payload_test.go +++ b/pkg/provider/github/parse_payload_test.go @@ -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" @@ -88,6 +89,9 @@ var samplePRAnother = github.PullRequest{ } func TestParsePayLoad(t *testing.T) { + samplePRNoRepo := samplePRevent + samplePRNoRepo.Repo = nil + tests := []struct { name string wantErrString string @@ -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", diff --git a/test/invalid_event_test.go b/test/invalid_event_test.go index 0de2ff53e..4a3714377 100644 --- a/test/invalid_event_test.go +++ b/test/invalid_event_test.go @@ -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) {