No webhook secret #1127

jbrockopp · 2019-02-25T14:12:19Z

googlebot · 2019-02-25T14:12:23Z

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).

📝 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 and verify that your email is set on your git commits.

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 (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

jbrockopp · 2019-02-25T14:14:47Z

I signed it!

googlebot · 2019-02-25T14:14:50Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gmlewis

Thank you, @jbrockopp!

Just a couple minor tweaks, please, and then we will await a second LGTM before merging.

gmlewis · 2019-02-26T02:42:15Z

github/messages.go

-		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 {


In Go, it is more idiomatic with slices to check against a length of zero which will work for both nil and a zero-length slice. Checking against nil is discouraged because it doesn't work for a zero-length slice.

So please change this to if len(secretKey) > 0 {.

gmlewis · 2019-02-26T02:43:12Z

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 {


This and line 143 will now need to be changed... since it is checking for an invalid signature, please just go ahead and use something like []byte{0} here and below.

gmlewis · 2019-02-26T02:44:32Z

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


How about ...pass nil or an empty slice. ?

jbrockopp · 2019-02-26T14:24:28Z

@gmlewis I have made changes per your feedback. Please let me know if anything else is required 👍

gmlewis

Thank you, @jbrockopp!

Awaiting second LGTM before merging.

kshitij10496 · 2019-02-27T05:22:02Z

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 or an empty slice.
+// This is intended for local development purposes only and all webhooks should contain a secret message.


minor nit: I believe we should maintain semantic consistency between GitHub documentation and the library's documentation so that the user can easily navigate between both of them with ease. Thus, I request that we update this to read "... all webhooks should ideally set up a secret token."

Thoughts @gmlewis @jbrockopp

Sounds great! Let's please go ahead and address this change in this PR.

@kshitij10496 @gmlewis sure I can do that. I was just following the previously used verbiage in the existing code.

// secretKey is the GitHub Webhook secret message.

kshitij10496

I have a minor, non-blocking suggestion about the general documentation which can be addressed in a separate change. So, LGTM on this PR! 👍

Thanks, @jbrockopp! 🎉

jbrockopp · 2019-02-27T15:39:40Z

@kshitij10496 @gmlewis I have made changes per your feedback. Please let me know if anything else is required 👍

gauntface

Thank you for the fix 👍

gmlewis · 2019-02-28T02:54:42Z

Thank you, @gauntface!
Merging.

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. Fixes #2731

jbrockopp added 2 commits February 25, 2019 08:11

only validate signature if secret key is not nil

8d1febd

add test for validating payload if secret key is nil

13c47d5

googlebot added the cla: no label Feb 25, 2019

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Feb 25, 2019

jbrockopp mentioned this pull request Feb 25, 2019

Support Webhooks without a Secret #1126

Closed

gmlewis requested changes Feb 26, 2019

View reviewed changes

gmlewis requested a review from gauntface February 26, 2019 02:45

check against length of secret key per feedback

dfc7492

gmlewis approved these changes Feb 26, 2019

View reviewed changes

kshitij10496 reviewed Feb 27, 2019

View reviewed changes

kshitij10496 approved these changes Feb 27, 2019

View reviewed changes

update docs and vars to reflect github terms per feedback

f4a9062

gauntface approved these changes Feb 27, 2019

View reviewed changes

gmlewis merged commit 361aadb into google:master Feb 28, 2019

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

No webhook secret (google#1127)

e4fcf2b

myitcv mentioned this pull request Mar 31, 2023

ValidatePayload should not accept nil or empty secretToken slice #2731

Closed

willnorris mentioned this pull request Mar 31, 2023

verify payload signature if present #2732

Merged

No webhook secret #1127

No webhook secret #1127

Uh oh!

Conversation

jbrockopp commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Feb 25, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

jbrockopp commented Feb 25, 2019

Uh oh!

googlebot commented Feb 25, 2019

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

gmlewis Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

gmlewis Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

jbrockopp commented Feb 26, 2019

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

kshitij10496 Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

gmlewis Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

jbrockopp Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

kshitij10496 left a comment

Choose a reason for hiding this comment

Uh oh!

jbrockopp commented Feb 27, 2019

Uh oh!

gauntface left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Feb 28, 2019

Uh oh!

Uh oh!

jbrockopp commented Feb 25, 2019 •

edited

Loading