Repository: google/go-github
Pull request #1127 (2019-02-25 14:12:19)

No webhook secret
Fixes https://github.com/google/go-github/issues/1126

googlebot: 
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: **Please visit <https://cla.developers.google.com/> to sign.**

Once you've signed (or fixed any issues), please reply here (e.g. `I signed it!`) and we'll verify it.

----

#### What to do if you already signed the CLA

##### Individual signers

*   It's possible we don't have your GitHub username or you're using a different email address on your commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).

##### Corporate signers

*   Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to [go/cla#troubleshoot](http://go/cla#troubleshoot) ([Public version](https://opensource.google.com/docs/cla/#troubleshoot)).
*   The email used to register you as an authorized contributor must be the email used for the Git commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).
*   The email used to register you as an authorized contributor must also be [attached to your GitHub account](https://github.com/settings/emails).
		

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fgoogle%2Fgo-github%2Fpull%2F1127) for more info**.

<!-- need_sender_cla -->
jbrockopp: I signed it!
googlebot: CLAs look good, thanks!

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fgoogle%2Fgo-github%2Fpull%2F1127) for more info**.

<!-- ok -->
jbrockopp: @gmlewis I have made changes per your feedback. Please let me know if anything else is required 👍 
jbrockopp: @kshitij10496 @gmlewis I have made changes per your feedback. Please let me know if anything else is required 👍

Patch:
diff --git a/github/messages.go b/github/messages.go
index 75bc7705..e8c624a7 100644
--- a/github/messages.go
+++ b/github/messages.go
@@ -129,7 +129,9 @@ func messageMAC(signature string) ([]byte, func() hash.Hash, error) {
 // 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.
-// secretKey is the GitHub Webhook secret message.
+// 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.
 //
 // Example usage:
 //
@@ -139,7 +141,7 @@ func messageMAC(signature string) ([]byte, func() hash.Hash, error) {
 //       // Process payload...
 //     }
 //
-func ValidatePayload(r *http.Request, secretKey []byte) (payload []byte, err error) {
+func ValidatePayload(r *http.Request, secretToken []byte) (payload []byte, err error) {
 	var body []byte // Raw body that GitHub uses to calculate the signature.
 
 	switch ct := r.Header.Get("Content-Type"); ct {
@@ -175,25 +177,30 @@ func ValidatePayload(r *http.Request, secretKey []byte) (payload []byte, err err
 		return nil, fmt.Errorf("Webhook request has unsupported Content-Type %q", ct)
 	}
 
-	sig := r.Header.Get(signatureHeader)
-	if err := ValidateSignature(sig, body, secretKey); err != nil {
-		return nil, err
+	// 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 {
+		sig := r.Header.Get(signatureHeader)
+		if err := ValidateSignature(sig, body, secretToken); err != nil {
+			return nil, err
+		}
 	}
+
 	return payload, nil
 }
 
 // ValidateSignature validates the signature for the given payload.
 // signature is the GitHub hash signature delivered in the X-Hub-Signature header.
 // payload is the JSON payload sent by GitHub Webhooks.
-// secretKey is the GitHub Webhook secret message.
+// secretToken is the GitHub Webhook secret token.
 //
 // GitHub API docs: https://developer.github.com/webhooks/securing/#validating-payloads-from-github
-func ValidateSignature(signature string, payload, secretKey []byte) error {
+func ValidateSignature(signature string, payload, secretToken []byte) error {
 	messageMAC, hashFunc, err := messageMAC(signature)
 	if err != nil {
 		return err
 	}
-	if !checkMAC(payload, messageMAC, secretKey, hashFunc) {
+	if !checkMAC(payload, messageMAC, secretToken, hashFunc) {
 		return errors.New("payload signature check failed")
 	}
 	return nil
diff --git a/github/repos_hooks.go b/github/repos_hooks.go
index 56374b3e..7674947d 100644
--- a/github/repos_hooks.go
+++ b/github/repos_hooks.go
@@ -92,6 +92,7 @@ func (h Hook) String() string {
 // information.
 type createHookRequest struct {
 	// Config is required.
+	Name   string                 `json:"name"`
 	Config map[string]interface{} `json:"config,omitempty"`
 	Events []string               `json:"events,omitempty"`
 	Active *bool                  `json:"active,omitempty"`
@@ -108,6 +109,7 @@ func (s *RepositoriesService) CreateHook(ctx context.Context, owner, repo string
 	u := fmt.Sprintf("repos/%v/%v/hooks", owner, repo)
 
 	hookReq := &createHookRequest{
+		Name:   "web",
 		Events: hook.Events,
 		Active: hook.Active,
 		Config: hook.Config,
diff --git a/github/search.go b/github/search.go
index abaf5e1f..24156f31 100644
--- a/github/search.go
+++ b/github/search.go
@@ -8,9 +8,7 @@ package github
 import (
 	"context"
 	"fmt"
-	"net/url"
 	"strconv"
-	"strings"
 
 	qs "github.com/google/go-querystring/query"
 )
@@ -218,20 +216,19 @@ func (s *SearchService) Labels(ctx context.Context, repoID int64, query string,
 
 // Helper function that executes search queries against different
 // GitHub search types (repositories, commits, code, issues, users, labels)
+//
+// If searchParameters.Query includes multiple condition, it MUST NOT include "+" as condition separator.
+// For example, querying with "language:c++" and "leveldb", then searchParameters.Query should be "language:c++ leveldb" but not "language:c+++leveldb".
 func (s *SearchService) search(ctx context.Context, searchType string, parameters *searchParameters, opt *SearchOptions, result interface{}) (*Response, error) {
 	params, err := qs.Values(opt)
 	if err != nil {
 		return nil, err
 	}
-	q := strings.Replace(parameters.Query, " ", "+", -1)
 	if parameters.RepositoryID != nil {
 		params.Set("repository_id", strconv.FormatInt(*parameters.RepositoryID, 10))
 	}
-	query := "q=" + url.PathEscape(q)
-	if v := params.Encode(); v != "" {
-		query = query + "&" + v
-	}
-	u := fmt.Sprintf("search/%s?%s", searchType, query)
+	params.Set("q", parameters.Query)
+	u := fmt.Sprintf("search/%s?%s", searchType, params.Encode())
 
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {


Test patch:
diff --git a/github/messages_test.go b/github/messages_test.go
index b63f1946..bde9d76b 100644
--- a/github/messages_test.go
+++ b/github/messages_test.go
@@ -110,7 +110,7 @@ func TestValidatePayload_FormGet(t *testing.T) {
 
 	// check that if payload is invalid we get error
 	req.Header.Set(signatureHeader, "invalid signature")
-	if _, err = ValidatePayload(req, nil); err == nil {
+	if _, err = ValidatePayload(req, []byte{0}); err == nil {
 		t.Error("ValidatePayload = nil, want err")
 	}
 }
@@ -140,7 +140,7 @@ func TestValidatePayload_FormPost(t *testing.T) {
 
 	// check that if payload is invalid we get error
 	req.Header.Set(signatureHeader, "invalid signature")
-	if _, err = ValidatePayload(req, nil); err == nil {
+	if _, err = ValidatePayload(req, []byte{0}); err == nil {
 		t.Error("ValidatePayload = nil, want err")
 	}
 }
@@ -156,6 +156,27 @@ func TestValidatePayload_InvalidContentType(t *testing.T) {
 	}
 }
 
+func TestValidatePayload_NoSecretKey(t *testing.T) {
+	payload := `{"yo":true}`
+
+	form := url.Values{}
+	form.Set("payload", payload)
+	buf := bytes.NewBufferString(form.Encode())
+	req, err := http.NewRequest("POST", "http://localhost/event", buf)
+	if err != nil {
+		t.Fatalf("NewRequest: %v", err)
+	}
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+	got, err := ValidatePayload(req, nil)
+	if err != nil {
+		t.Errorf("ValidatePayload(%#v): err = %v, want nil", payload, err)
+	}
+	if string(got) != payload {
+		t.Errorf("ValidatePayload = %q, want %q", got, payload)
+	}
+}
+
 func TestParseWebHook(t *testing.T) {
 	tests := []struct {
 		payload     interface{}
diff --git a/github/repos_hooks_test.go b/github/repos_hooks_test.go
index 1223b442..9e4691c3 100644
--- a/github/repos_hooks_test.go
+++ b/github/repos_hooks_test.go
@@ -25,7 +25,7 @@ func TestRepositoriesService_CreateHook(t *testing.T) {
 		json.NewDecoder(r.Body).Decode(v)
 
 		testMethod(t, r, "POST")
-		want := &createHookRequest{}
+		want := &createHookRequest{Name: "web"}
 		if !reflect.DeepEqual(v, want) {
 			t.Errorf("Request body = %+v, want %+v", v, want)
 		}
diff --git a/github/search_test.go b/github/search_test.go
index f6e40d21..4b686884 100644
--- a/github/search_test.go
+++ b/github/search_test.go
@@ -115,7 +115,7 @@ func TestSearchService_Issues_withQualifiersNoOpts(t *testing.T) {
 	client, mux, _, teardown := setup()
 	defer teardown()
 
-	const q = "gopher is:issue label:bug language:go pushed:>=2018-01-01 stars:>=200"
+	const q = "gopher is:issue label:bug language:c++ pushed:>=2018-01-01 stars:>=200"
 
 	var requestURI string
 	mux.HandleFunc("/search/issues", func(w http.ResponseWriter, r *http.Request) {
@@ -134,7 +134,7 @@ func TestSearchService_Issues_withQualifiersNoOpts(t *testing.T) {
 		t.Errorf("Search.Issues returned error: %v", err)
 	}
 
-	if want := "/api-v3/search/issues?q=gopher+is:issue+label:bug+language:go+pushed:%3E=2018-01-01+stars:%3E=200"; requestURI != want {
+	if want := "/api-v3/search/issues?q=gopher+is%3Aissue+label%3Abug+language%3Ac%2B%2B+pushed%3A%3E%3D2018-01-01+stars%3A%3E%3D200"; requestURI != want {
 		t.Fatalf("URI encoding failed: got %v, want %v", requestURI, want)
 	}
 
@@ -152,7 +152,7 @@ func TestSearchService_Issues_withQualifiersAndOpts(t *testing.T) {
 	client, mux, _, teardown := setup()
 	defer teardown()
 
-	const q = "gopher is:issue label:bug language:go pushed:>=2018-01-01 stars:>=200"
+	const q = "gopher is:issue label:bug language:c++ pushed:>=2018-01-01 stars:>=200"
 
 	var requestURI string
 	mux.HandleFunc("/search/issues", func(w http.ResponseWriter, r *http.Request) {
@@ -172,7 +172,7 @@ func TestSearchService_Issues_withQualifiersAndOpts(t *testing.T) {
 		t.Errorf("Search.Issues returned error: %v", err)
 	}
 
-	if want := "/api-v3/search/issues?q=gopher+is:issue+label:bug+language:go+pushed:%3E=2018-01-01+stars:%3E=200&sort=forks"; requestURI != want {
+	if want := "/api-v3/search/issues?q=gopher+is%3Aissue+label%3Abug+language%3Ac%2B%2B+pushed%3A%3E%3D2018-01-01+stars%3A%3E%3D200&sort=forks"; requestURI != want {
 		t.Fatalf("URI encoding failed: got %v, want %v", requestURI, want)
 	}