From 8d1febdbbf1d1a088cb7f181ff45e552791f1557 Mon Sep 17 00:00:00 2001 From: Jordan Brockopp Date: Mon, 25 Feb 2019 08:11:06 -0600 Subject: [PATCH 1/4] only validate signature if secret key is not nil --- github/messages.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/github/messages.go b/github/messages.go index 75bc770512d..2cf70407fdd 100644 --- a/github/messages.go +++ b/github/messages.go @@ -130,6 +130,8 @@ func messageMAC(signature string) ([]byte, func() hash.Hash, error) { // 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. +// If your webhook does not contain a secret message, you can pass nil. This +// is intended for local development purposes only and all webhooks should contain a secret message. // // Example usage: // @@ -175,10 +177,15 @@ 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 message exists. This is intended for + // local development only and all webhooks should contain a secret message. + if secretKey != nil { + sig := r.Header.Get(signatureHeader) + if err := ValidateSignature(sig, body, secretKey); err != nil { + return nil, err + } } + return payload, nil } From 13c47d5efc70fd518a6b0279b400c2bb9da3792d Mon Sep 17 00:00:00 2001 From: Jordan Brockopp Date: Mon, 25 Feb 2019 08:11:38 -0600 Subject: [PATCH 2/4] add test for validating payload if secret key is nil --- github/messages_test.go | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/github/messages_test.go b/github/messages_test.go index b63f1946b05..d39035e9193 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{}); 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{}); 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{} From dfc74929c79530695bdfc200aeb73738eb1e64b6 Mon Sep 17 00:00:00 2001 From: Jordan Brockopp Date: Tue, 26 Feb 2019 08:22:49 -0600 Subject: [PATCH 3/4] check against length of secret key per feedback --- github/messages.go | 6 +++--- github/messages_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/github/messages.go b/github/messages.go index 2cf70407fdd..4d798968428 100644 --- a/github/messages.go +++ b/github/messages.go @@ -130,8 +130,8 @@ func messageMAC(signature string) ([]byte, func() hash.Hash, error) { // 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. -// If your webhook does not contain a secret message, you can pass nil. This -// is intended for local development purposes only and all webhooks should contain a secret message. +// If your webhook does not contain a secret message, you can pass nil or an empty slice. +// This is intended for local development purposes only and all webhooks should contain a secret message. // // Example usage: // @@ -179,7 +179,7 @@ func ValidatePayload(r *http.Request, secretKey []byte) (payload []byte, err err // Only validate the signature if a secret message exists. This is intended for // local development only and all webhooks should contain a secret message. - if secretKey != nil { + if len(secretKey) > 0 { sig := r.Header.Get(signatureHeader) if err := ValidateSignature(sig, body, secretKey); err != nil { return nil, err diff --git a/github/messages_test.go b/github/messages_test.go index d39035e9193..bde9d76baca 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, []byte{}); 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, []byte{}); err == nil { + if _, err = ValidatePayload(req, []byte{0}); err == nil { t.Error("ValidatePayload = nil, want err") } } From f4a906255fb9d3a33b2cb94cfd6a3055fa45513b Mon Sep 17 00:00:00 2001 From: Jordan Brockopp Date: Wed, 27 Feb 2019 09:38:53 -0600 Subject: [PATCH 4/4] update docs and vars to reflect github terms per feedback --- github/messages.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/github/messages.go b/github/messages.go index 4d798968428..e8c624a72c0 100644 --- a/github/messages.go +++ b/github/messages.go @@ -129,9 +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. -// If your webhook does not contain a secret message, you can pass nil or an empty slice. -// This is intended for local development purposes only and all webhooks should contain a 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: // @@ -141,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 { @@ -177,11 +177,11 @@ func ValidatePayload(r *http.Request, secretKey []byte) (payload []byte, err err return nil, fmt.Errorf("Webhook request has unsupported Content-Type %q", ct) } - // Only validate the signature if a secret message exists. This is intended for - // local development only and all webhooks should contain a secret message. - if len(secretKey) > 0 { + // 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, secretKey); err != nil { + if err := ValidateSignature(sig, body, secretToken); err != nil { return nil, err } } @@ -192,15 +192,15 @@ func ValidatePayload(r *http.Request, secretKey []byte) (payload []byte, err err // 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