batches: don't panic on unexpected GitHub PR action (#20571)

When converting a raw payload to changeset event kind, we need to be
able to handle payloads of unexpected types, especially nil, since we
don't control the inputs that come in via webhooks.

The specific case that triggered this was a GitHub pull request event
that had an unknown action: this results in a nil event keyer being
returned, and that caused a panic in ChangesetEventKindFor().

CHANGELOG.md

@@ -28,6 +28,7 @@ All notable changes to Sourcegraph are documented in this file.
 - `repo:contains(...)` built-in did not respect parameters that affect repo filtering (e.g., `repogroup`, `fork`). It now respects these. [#20339](https://github.com/sourcegraph/sourcegraph/pull/20339)
 - An issue where duplicate results would render for certain `or`-expressions. [#20480](https://github.com/sourcegraph/sourcegraph/pull/20480)
 - Issue where the search query bar suggests that some `lang` values are not valid. [#20534](https://github.com/sourcegraph/sourcegraph/pull/20534)
+- Pull request event webhooks received from GitHub with unexpected actions no longer cause panics. [#20571](https://github.com/sourcegraph/sourcegraph/pull/20571)
 
 ### Removed
 

enterprise/internal/batches/reconciler/executor.go

@@ -16,6 +16,7 @@ import (
 	btypes "github.com/sourcegraph/sourcegraph/enterprise/internal/batches/types"
 	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/database"
+	"github.com/sourcegraph/sourcegraph/internal/errcode"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
 	"github.com/sourcegraph/sourcegraph/internal/types"
 )
@@ -112,7 +113,11 @@ func (e *executor) Run(ctx context.Context, plan *Plan) (err error) {
 		}
 	}
 
-	events := e.ch.Events()
+	events, err := e.ch.Events()
+	if err != nil {
+		log15.Error("Events", "err", err)
+		return errcode.MakeNonRetryable(err)
+	}
 	state.SetDerivedState(ctx, e.tx.Repos(), e.ch, events)
 
 	if err := e.tx.UpsertChangesetEvents(ctx, events...); err != nil {

enterprise/internal/batches/resolvers/changeset_event_connection_test.go

@@ -88,7 +88,10 @@ func TestChangesetEventConnectionResolver(t *testing.T) {
 	})
 
 	// Create ChangesetEvents from the timeline items in the metadata.
-	events := changeset.Events()
+	events, err := changeset.Events()
+	if err != nil {
+		t.Fatal(err)
+	}
 	if err := cstore.UpsertChangesetEvents(ctx, events...); err != nil {
 		t.Fatal(err)
 	}

enterprise/internal/batches/resolvers/changeset_test.go

@@ -156,7 +156,10 @@ func TestChangesetResolver(t *testing.T) {
 			UpdatedAt: now,
 		},
 	})
-	events := syncedGitHubChangeset.Events()
+	events, err := syncedGitHubChangeset.Events()
+	if err != nil {
+		t.Fatal(err)
+	}
 	if err := cstore.UpsertChangesetEvents(ctx, events...); err != nil {
 		t.Fatal(err)
 	}

enterprise/internal/batches/syncer/syncer.go

@@ -456,7 +456,10 @@ func SyncChangeset(ctx context.Context, syncStore SyncStore, source sources.Chan
 		}
 	}
 
-	events := c.Events()
+	events, err := c.Events()
+	if err != nil {
+		return err
+	}
 	state.SetDerivedState(ctx, syncStore.Repos(), c, events)
 
 	tx, err := syncStore.Transact(ctx)

enterprise/internal/batches/types/changeset.go

@@ -469,7 +469,7 @@ func (c *Changeset) URL() (s string, err error) {
 }
 
 // Events returns the deduplicated list of ChangesetEvents from the Changeset's metadata.
-func (c *Changeset) Events() (events []*ChangesetEvent) {
+func (c *Changeset) Events() (events []*ChangesetEvent, err error) {
 	uniqueEvents := make(map[string]struct{})
 
 	appendEvent := func(e *ChangesetEvent) {
@@ -493,7 +493,9 @@ func (c *Changeset) Events() (events []*ChangesetEvent) {
 				for _, c := range e.Comments {
 					ev := ev
 					ev.Key = c.Key()
-					ev.Kind = ChangesetEventKindFor(c)
+					if ev.Kind, err = ChangesetEventKindFor(c); err != nil {
+						return
+					}
 					ev.Metadata = c
 					appendEvent(&ev)
 				}
@@ -506,70 +508,95 @@ func (c *Changeset) Events() (events []*ChangesetEvent) {
 					continue
 				}
 				ev.Key = e.Key()
-				ev.Kind = ChangesetEventKindFor(e)
+				if ev.Kind, err = ChangesetEventKindFor(e); err != nil {
+					return
+				}
 				ev.Metadata = e
 				appendEvent(&ev)
 
 			default:
 				ev.Key = ti.Item.(Keyer).Key()
-				ev.Kind = ChangesetEventKindFor(ti.Item)
+				if ev.Kind, err = ChangesetEventKindFor(ti.Item); err != nil {
+					return
+				}
 				ev.Metadata = ti.Item
 				appendEvent(&ev)
 			}
 		}
 
 	case *bitbucketserver.PullRequest:
 		events = make([]*ChangesetEvent, 0, len(m.Activities)+len(m.CommitStatus))
-		addEvent := func(e Keyer) {
+
+		addEvent := func(e Keyer) error {
+			kind, err := ChangesetEventKindFor(e)
+			if err != nil {
+				return err
+			}
+
 			appendEvent(&ChangesetEvent{
 				ChangesetID: c.ID,
 				Key:         e.Key(),
-				Kind:        ChangesetEventKindFor(e),
+				Kind:        kind,
 				Metadata:    e,
 			})
+			return nil
 		}
 		for _, a := range m.Activities {
-			addEvent(a)
+			if err = addEvent(a); err != nil {
+				return
+			}
 		}
 		for _, s := range m.CommitStatus {
-			addEvent(s)
+			if err = addEvent(s); err != nil {
+				return
+			}
 		}
 
 	case *gitlab.MergeRequest:
 		events = make([]*ChangesetEvent, 0, len(m.Notes)+len(m.ResourceStateEvents)+len(m.Pipelines))
+		var kind ChangesetEventKind
 
 		for _, note := range m.Notes {
 			if event := note.ToEvent(); event != nil {
+				if kind, err = ChangesetEventKindFor(event); err != nil {
+					return
+				}
 				appendEvent(&ChangesetEvent{
 					ChangesetID: c.ID,
 					Key:         event.(Keyer).Key(),
-					Kind:        ChangesetEventKindFor(event),
+					Kind:        kind,
 					Metadata:    event,
 				})
 			}
 		}
 
 		for _, e := range m.ResourceStateEvents {
 			if event := e.ToEvent(); event != nil {
+				if kind, err = ChangesetEventKindFor(event); err != nil {
+					return
+				}
 				appendEvent(&ChangesetEvent{
 					ChangesetID: c.ID,
 					Key:         event.(Keyer).Key(),
-					Kind:        ChangesetEventKindFor(event),
+					Kind:        kind,
 					Metadata:    event,
 				})
 			}
 		}
 
 		for _, pipeline := range m.Pipelines {
+			if kind, err = ChangesetEventKindFor(pipeline); err != nil {
+				return
+			}
 			appendEvent(&ChangesetEvent{
 				ChangesetID: c.ID,
 				Key:         pipeline.Key(),
-				Kind:        ChangesetEventKindFor(pipeline),
+				Kind:        kind,
 				Metadata:    pipeline,
 			})
 		}
 	}
-	return events
+	return events, nil
 }
 
 // HeadRefOid returns the git ObjectID of the HEAD reference associated with
@@ -831,76 +858,75 @@ type ChangesetsStats struct {
 
 // ChangesetEventKindFor returns the ChangesetEventKind for the given
 // specific code host event.
-func ChangesetEventKindFor(e interface{}) ChangesetEventKind {
+func ChangesetEventKindFor(e interface{}) (ChangesetEventKind, error) {
 	switch e := e.(type) {
 	case *github.AssignedEvent:
-		return ChangesetEventKindGitHubAssigned
+		return ChangesetEventKindGitHubAssigned, nil
 	case *github.ClosedEvent:
-		return ChangesetEventKindGitHubClosed
+		return ChangesetEventKindGitHubClosed, nil
 	case *github.IssueComment:
-		return ChangesetEventKindGitHubCommented
+		return ChangesetEventKindGitHubCommented, nil
 	case *github.RenamedTitleEvent:
-		return ChangesetEventKindGitHubRenamedTitle
+		return ChangesetEventKindGitHubRenamedTitle, nil
 	case *github.MergedEvent:
-		return ChangesetEventKindGitHubMerged
+		return ChangesetEventKindGitHubMerged, nil
 	case *github.PullRequestReview:
-		return ChangesetEventKindGitHubReviewed
+		return ChangesetEventKindGitHubReviewed, nil
 	case *github.PullRequestReviewComment:
-		return ChangesetEventKindGitHubReviewCommented
+		return ChangesetEventKindGitHubReviewCommented, nil
 	case *github.ReopenedEvent:
-		return ChangesetEventKindGitHubReopened
+		return ChangesetEventKindGitHubReopened, nil
 	case *github.ReviewDismissedEvent:
-		return ChangesetEventKindGitHubReviewDismissed
+		return ChangesetEventKindGitHubReviewDismissed, nil
 	case *github.ReviewRequestRemovedEvent:
-		return ChangesetEventKindGitHubReviewRequestRemoved
+		return ChangesetEventKindGitHubReviewRequestRemoved, nil
 	case *github.ReviewRequestedEvent:
-		return ChangesetEventKindGitHubReviewRequested
+		return ChangesetEventKindGitHubReviewRequested, nil
 	case *github.ReadyForReviewEvent:
-		return ChangesetEventKindGitHubReadyForReview
+		return ChangesetEventKindGitHubReadyForReview, nil
 	case *github.ConvertToDraftEvent:
-		return ChangesetEventKindGitHubConvertToDraft
+		return ChangesetEventKindGitHubConvertToDraft, nil
 	case *github.UnassignedEvent:
-		return ChangesetEventKindGitHubUnassigned
+		return ChangesetEventKindGitHubUnassigned, nil
 	case *github.PullRequestCommit:
-		return ChangesetEventKindGitHubCommit
+		return ChangesetEventKindGitHubCommit, nil
 	case *github.LabelEvent:
 		if e.Removed {
-			return ChangesetEventKindGitHubUnlabeled
+			return ChangesetEventKindGitHubUnlabeled, nil
 		}
-		return ChangesetEventKindGitHubLabeled
+		return ChangesetEventKindGitHubLabeled, nil
 	case *github.CommitStatus:
-		return ChangesetEventKindCommitStatus
+		return ChangesetEventKindCommitStatus, nil
 	case *github.CheckSuite:
-		return ChangesetEventKindCheckSuite
+		return ChangesetEventKindCheckSuite, nil
 	case *github.CheckRun:
-		return ChangesetEventKindCheckRun
+		return ChangesetEventKindCheckRun, nil
 	case *bitbucketserver.Activity:
-		return ChangesetEventKind("bitbucketserver:" + strings.ToLower(string(e.Action)))
+		return ChangesetEventKind("bitbucketserver:" + strings.ToLower(string(e.Action))), nil
 	case *bitbucketserver.ParticipantStatusEvent:
-		return ChangesetEventKind("bitbucketserver:participant_status:" + strings.ToLower(string(e.Action)))
+		return ChangesetEventKind("bitbucketserver:participant_status:" + strings.ToLower(string(e.Action))), nil
 	case *bitbucketserver.CommitStatus:
-		return ChangesetEventKindBitbucketServerCommitStatus
+		return ChangesetEventKindBitbucketServerCommitStatus, nil
 	case *gitlab.Pipeline:
-		return ChangesetEventKindGitLabPipeline
+		return ChangesetEventKindGitLabPipeline, nil
 	case *gitlab.ReviewApprovedEvent:
-		return ChangesetEventKindGitLabApproved
+		return ChangesetEventKindGitLabApproved, nil
 	case *gitlab.ReviewUnapprovedEvent:
-		return ChangesetEventKindGitLabUnapproved
+		return ChangesetEventKindGitLabUnapproved, nil
 	case *gitlab.MarkWorkInProgressEvent:
-		return ChangesetEventKindGitLabMarkWorkInProgress
+		return ChangesetEventKindGitLabMarkWorkInProgress, nil
 	case *gitlab.UnmarkWorkInProgressEvent:
-		return ChangesetEventKindGitLabUnmarkWorkInProgress
+		return ChangesetEventKindGitLabUnmarkWorkInProgress, nil
 
 	case *gitlab.MergeRequestClosedEvent:
-		return ChangesetEventKindGitLabClosed
+		return ChangesetEventKindGitLabClosed, nil
 	case *gitlab.MergeRequestReopenedEvent:
-		return ChangesetEventKindGitLabReopened
+		return ChangesetEventKindGitLabReopened, nil
 	case *gitlab.MergeRequestMergedEvent:
-		return ChangesetEventKindGitLabMerged
-
-	default:
-		panic(errors.Errorf("unknown changeset event kind for %T", e))
+		return ChangesetEventKindGitLabMerged, nil
 	}
+
+	return ChangesetEventKindInvalid, errors.Errorf("unknown changeset event kind for %T", e)
 }
 
 // NewChangesetEventMetadata returns a new metadata object for the given

enterprise/internal/batches/types/changeset_event.go

@@ -77,6 +77,8 @@ const (
 	ChangesetEventKindGitLabUnapproved           ChangesetEventKind = "gitlab:unapproved"
 	ChangesetEventKindGitLabMarkWorkInProgress   ChangesetEventKind = "gitlab:mark_wip"
 	ChangesetEventKindGitLabUnmarkWorkInProgress ChangesetEventKind = "gitlab:unmark_wip"
+
+	ChangesetEventKindInvalid ChangesetEventKind = "invalid"
 )
 
 // A ChangesetEvent is an event that happened in the lifetime

enterprise/internal/batches/types/changeset_event_test.go

@@ -307,7 +307,10 @@ func TestChangesetEvent(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			have := tc.changeset.Events()
+			have, err := tc.changeset.Events()
+			if err != nil {
+				t.Fatal(err)
+			}
 			want := tc.events
 
 			if diff := cmp.Diff(have, want); diff != "" {

enterprise/internal/batches/webhooks/github_test.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	gh "github.com/google/go-github/v28/github"
 
 	"github.com/sourcegraph/sourcegraph/cmd/frontend/webhooks"
 	"github.com/sourcegraph/sourcegraph/enterprise/internal/batches/sources"
@@ -214,5 +215,24 @@ func testGitHubWebhook(db *sql.DB, userID int32) func(*testing.T) {
 				}
 			})
 		}
+
+		t.Run("unexpected payload", func(t *testing.T) {
+			// GitHub pull request events are processed based on the action
+			// embedded within them, but that action is just a string that could
+			// be anything. We need to ensure that this is hardened against
+			// unexpected input.
+			n := 10156
+			action := "this is a bad action"
+
+			if err := hook.handleGitHubWebhook(ctx, extSvc, &gh.PullRequestEvent{
+				Number: &n,
+				Repo: &gh.Repository{
+					NodeID: &githubRepo.ExternalRepo.ID,
+				},
+				Action: &action,
+			}); err == nil {
+				t.Error("unexpected nil error")
+			}
+		})
 	}
 }

enterprise/internal/batches/webhooks/webhooks.go

@@ -109,6 +109,11 @@ func (h Webhook) upsertChangesetEvent(
 		return nil
 	}
 
+	var kind btypes.ChangesetEventKind
+	if kind, err = btypes.ChangesetEventKindFor(ev); err != nil {
+		return err
+	}
+
 	cs, err := tx.GetChangeset(ctx, store.GetChangesetOpts{
 		RepoID:              r.ID,
 		ExternalID:          strconv.FormatInt(pr.ID, 10),
@@ -124,7 +129,7 @@ func (h Webhook) upsertChangesetEvent(
 	now := h.Store.Clock()()
 	event := &btypes.ChangesetEvent{
 		ChangesetID: cs.ID,
-		Kind:        btypes.ChangesetEventKindFor(ev),
+		Kind:        kind,
 		Key:         ev.Key(),
 		CreatedAt:   now,
 		UpdatedAt:   now,

internal/errcode/code.go

@@ -182,6 +182,15 @@ func IsNonRetryable(err error) bool {
 	})
 }
 
+// MakeNonRetryable makes any error non-retryable.
+func MakeNonRetryable(err error) error {
+	return nonRetryableError{err}
+}
+
+type nonRetryableError struct{ error }
+
+func (nonRetryableError) NonRetryable() bool { return true }
+
 // isErrorPredicate returns true if err or one of its causes returns true when
 // passed to p.
 func isErrorPredicate(err error, p func(err error) bool) bool {