verify payload signature if present (#2732)

Fixes: #2731.
google · Apr 1, 2023 · b1c53f8 · b1c53f8
1 parent 388d921
commit b1c53f8
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 28 deletions.
diff --git 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
 		}

diff --git 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)