Repository: google/go-github (go)

Issue:
ValidatePayload should not accept nil or empty secretToken slice
https://github.com/google/go-github/pull/1127 added support for calling `ValidatePayload` with a `nil` or empty `secretToken` slice. The justification for this change is explained in the [current docs](https://pkg.go.dev/github.com/google/go-github/v45@v45.2.0/github#ValidatePayload):

> If your webhook does not contain a secret token, you can pass nil or an empty slice. This is intended for local development purposes only and all webhooks should ideally set up a secret token.

I think this leaves `ValidatePayload` with dangerous default behaviour. Because now every caller of this function has to check that they have a non-`nil` and non-empty `secretToken` slice if they want to validate a payload. If they don't, it's possible to accidentally call the function with a `nil` or empty slice (bad configuration, etc) and never know about it. This is not a safe default. Better would be to have the function `panic` or return an error in case `secretToken` is `nil` or empty - because such a value can never be used to validate a payload, which is obviously what the caller intended to do in calling the function.

The same applies to `ValidatePayloadFromBody()`. 

I haven't looked at other functions/methods.

Returning to the goal of #1126, I would instead have pushed back and suggested that developers who are in development mode change their calling code to simply not call `ValidatePayload` when they detect they are in such a mode.

Pull request description:
verify payload signature if present
Verify the payload signature if the request has a signature present in HTTP headers, or if a non-empty secretToken is passed to the ValidatePayload method, indicating that a signature is expected.

This modifies the behavior added in #1127, but not the spirit of what was requested in #1126, which is to support webhooks that don't have configured secrets.

Specifically, this no longer allows signature checking to be skipped entirely, even for webhooks with a configured secret, simply by passing an empty secretToken to ValidatePayload.

This also cleans up some documentation and unused fields in the accompanying test.

Fixes #2731 

/cc @myitcv

Patch:
diff --git a/github/messages.go b/github/messages.go
index 0943b9e9..925e73e9 100644
--- a/github/messages.go
+++ b/github/messages.go
@@ -148,13 +148,13 @@ func messageMAC(signature string) ([]byte, func() hash.Hash, error) {
 	return buf, hashFunc, nil
 }
 
-// ValidatePayload validates an incoming GitHub Webhook event request body
+// ValidatePayloadFromBody validates an incoming GitHub Webhook event request body
 // and returns the (JSON) payload.
 // The Content-Type header of the payload can be "application/json" or "application/x-www-form-urlencoded".
 // If the Content-Type is neither then an error is returned.
 // secretToken is the GitHub Webhook secret token.
-// If your webhook does not contain a secret token, you can pass nil or an empty slice.
-// This is intended for local development purposes only and all webhooks should ideally set up a secret token.
+// If your webhook does not contain a secret token, you can pass an empty secretToken.
+// Webhooks without a secret token are not secure and should be avoided.
 //
 // Example usage:
 //
@@ -201,9 +201,8 @@ func ValidatePayloadFromBody(contentType string, readable io.Reader, signature s
 		return nil, fmt.Errorf("webhook request has unsupported Content-Type %q", contentType)
 	}
 
-	// Only validate the signature if a secret token exists. This is intended for
-	// local development only and all webhooks should ideally set up a secret token.
-	if len(secretToken) > 0 {
+	// Validate the signature if present or if one is expected (secretToken is non-empty).
+	if len(secretToken) > 0 || len(signature) > 0 {
 		if err := ValidateSignature(signature, body, secretToken); err != nil {
 			return nil, err
 		}


Test patch:
diff --git a/github/messages_test.go b/github/messages_test.go
index ee374104..04f29420 100644
--- a/github/messages_test.go
+++ b/github/messages_test.go
@@ -30,51 +30,42 @@ func TestValidatePayload(t *testing.T) {
 	const defaultSignature = "sha1=126f2c800419c60137ce748d7672e77b65cf16d6"
 	secretKey := []byte("0123456789abcdef")
 	tests := []struct {
+		secretKey       []byte
 		signature       string
 		signatureHeader string
-		eventID         string
-		event           string
-		wantEventID     string
-		wantEvent       string
 		wantPayload     string
 	}{
 		// The following tests generate expected errors:
-		{},                         // Missing signature
-		{signature: "yo"},          // Missing signature prefix
-		{signature: "sha1=yo"},     // Signature not hex string
-		{signature: "sha1=012345"}, // Invalid signature
+		{secretKey: secretKey},                           // Missing signature
+		{secretKey: secretKey, signature: "yo"},          // Missing signature prefix
+		{secretKey: secretKey, signature: "sha1=yo"},     // Signature not hex string
+		{secretKey: secretKey, signature: "sha1=012345"}, // Invalid signature
+		{signature: defaultSignature},                    // signature without secretKey
+
 		// The following tests expect err=nil:
 		{
-			signature:   defaultSignature,
-			eventID:     "dead-beef",
-			event:       "ping",
-			wantEventID: "dead-beef",
-			wantEvent:   "ping",
+			// no secretKey and no signature still passes validation
 			wantPayload: defaultBody,
 		},
 		{
+			secretKey:   secretKey,
 			signature:   defaultSignature,
-			event:       "ping",
-			wantEvent:   "ping",
 			wantPayload: defaultBody,
 		},
 		{
+			secretKey:   secretKey,
 			signature:   "sha256=b1f8020f5b4cd42042f807dd939015c4a418bc1ff7f604dd55b0a19b5d953d9b",
-			event:       "ping",
-			wantEvent:   "ping",
 			wantPayload: defaultBody,
 		},
 		{
+			secretKey:       secretKey,
 			signature:       "sha256=b1f8020f5b4cd42042f807dd939015c4a418bc1ff7f604dd55b0a19b5d953d9b",
 			signatureHeader: SHA256SignatureHeader,
-			event:           "ping",
-			wantEvent:       "ping",
 			wantPayload:     defaultBody,
 		},
 		{
+			secretKey:   secretKey,
 			signature:   "sha512=8456767023c1195682e182a23b3f5d19150ecea598fde8cb85918f7281b16079471b1329f92b912c4d8bd7455cb159777db8f29608b20c7c87323ba65ae62e1f",
-			event:       "ping",
-			wantEvent:   "ping",
 			wantPayload: defaultBody,
 		},
 	}
@@ -94,7 +85,7 @@ func TestValidatePayload(t *testing.T) {
 		}
 		req.Header.Set("Content-Type", "application/json")
 
-		got, err := ValidatePayload(req, secretKey)
+		got, err := ValidatePayload(req, test.secretKey)
 		if err != nil {
 			if test.wantPayload != "" {
 				t.Errorf("ValidatePayload(%#v): err = %v, want nil", test, err)