Add ValidationError type (#22)

When a payload fails validation, due to missing headers, an invalid
signature, or other reasons, create a ValidationError so that error
handlers can identify this case. Modify the default handler to return a
400 error in this case instead of a 500 or 202 response.

Author: bluekeyes

githubapp/dispatcher.go

@@ -16,6 +16,7 @@ package githubapp
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 
 	"github.com/google/go-github/github"
@@ -71,6 +72,18 @@ func WithResponseCallback(onResponse ResponseCallback) DispatcherOption {
 	}
 }
 
+// ValidationError is passed to error callbacks when the webhook payload fails
+// validation.
+type ValidationError struct {
+	EventType  string
+	DeliveryID string
+	Cause      error
+}
+
+func (ve ValidationError) Error() string {
+	return fmt.Sprintf("invalid event: %v", ve.Cause)
+}
+
 type eventDispatcher struct {
 	handlerMap map[string]EventHandler
 	secret     string
@@ -131,8 +144,11 @@ func (d *eventDispatcher) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	deliveryID := r.Header.Get("X-GitHub-Delivery")
 
 	if eventType == "" {
-		// ACK payload that was received but won't be processed
-		w.WriteHeader(http.StatusAccepted)
+		d.onError(w, r, ValidationError{
+			EventType:  eventType,
+			DeliveryID: deliveryID,
+			Cause:      errors.New("missing event type"),
+		})
 		return
 	}
 
@@ -147,7 +163,11 @@ func (d *eventDispatcher) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	payloadBytes, err := github.ValidatePayload(r, []byte(d.secret))
 	if err != nil {
-		d.onError(w, r, errors.Wrapf(err, "failed to validate webhook payload"))
+		d.onError(w, r, ValidationError{
+			EventType:  eventType,
+			DeliveryID: deliveryID,
+			Cause:      err,
+		})
 		return
 	}
 
@@ -166,6 +186,13 @@ func (d *eventDispatcher) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 // DefaultErrorCallback logs errors and responds with a 500 status code.
 func DefaultErrorCallback(w http.ResponseWriter, r *http.Request, err error) {
 	logger := zerolog.Ctx(r.Context())
+
+	if ve, ok := err.(ValidationError); ok {
+		logger.Warn().Err(ve.Cause).Msgf("Received invalid webhook headers or payload")
+		http.Error(w, "Invalid webhook headers or payload", http.StatusBadRequest)
+		return
+	}
+
 	logger.Error().Err(err).Msg("Unexpected error handling webhook request")
 	http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
 }

githubapp/dispatcher_test.go

@@ -38,6 +38,7 @@ func TestEventDispatcher(t *testing.T) {
 		Handler TestEventHandler
 		Options []DispatcherOption
 		Event   string
+		Invalid bool
 
 		ResponseCode int
 		ResponseBody string
@@ -65,7 +66,7 @@ func TestEventDispatcher(t *testing.T) {
 			ResponseCode: 200,
 			CallCount:    1,
 		},
-		"defaultErrorHandlerReturns500": {
+		"defaultErrorHandlerReturns500OnError": {
 			Handler: TestEventHandler{
 				Types: []string{"pull_request"},
 				Fn: func(ctx context.Context, eventType, deliveryID string, payload []byte) error {
@@ -77,6 +78,15 @@ func TestEventDispatcher(t *testing.T) {
 			ResponseBody: "Internal Server Error\n",
 			CallCount:    1,
 		},
+		"defaultErrorHandlerReturns400OnInvalid": {
+			Handler: TestEventHandler{
+				Types: []string{"pull_request"},
+			},
+			Event:        "pull_request",
+			Invalid:      true,
+			ResponseCode: 400,
+			ResponseBody: "Invalid webhook headers or payload\n",
+		},
 		"callsCustomErrorCallback": {
 			Handler: TestEventHandler{
 				Types: []string{"pull_request"},
@@ -151,7 +161,7 @@ func TestEventDispatcher(t *testing.T) {
 			h := test.Handler
 			d := NewEventDispatcher([]EventHandler{&h}, testHookSecret, test.Options...)
 
-			req := newSignedRequest(test.Event, name)
+			req := newHookRequest(test.Event, name, !test.Invalid)
 			res := httptest.NewRecorder()
 			d.ServeHTTP(res, req)
 
@@ -181,17 +191,19 @@ func TestSetAndGetResponder(t *testing.T) {
 	})
 }
 
-func newSignedRequest(eventType, id string) *http.Request {
+func newHookRequest(eventType, id string, signed bool) *http.Request {
 	body := []byte(fmt.Sprintf(`{"type":"%s"}`, eventType))
 
 	req := httptest.NewRequest(http.MethodPost, "/api/github/hook", bytes.NewReader(body))
 	req.Header.Set("Content-Type", "application/json")
 	req.Header.Set("X-Github-Event", eventType)
 	req.Header.Set("X-Github-Delivery", id)
 
-	mac := hmac.New(sha1.New, []byte(testHookSecret))
-	mac.Write(body)
-	req.Header.Set("X-Hub-Signature", fmt.Sprintf("sha1=%x", mac.Sum(nil)))
+	if signed {
+		mac := hmac.New(sha1.New, []byte(testHookSecret))
+		mac.Write(body)
+		req.Header.Set("X-Hub-Signature", fmt.Sprintf("sha1=%x", mac.Sum(nil)))
+	}
 
 	log := zerolog.New(os.Stdout)
 	req = req.WithContext(log.WithContext(req.Context()))