go-github: add utilities for processing payload messages #294

gmlewis · 2016-02-23T18:15:07Z

Change-Id: I1c80b8eb7816efc9b83e6b8be956430c68809d5e

willnorris · 2016-05-06T18:33:28Z

messages/messages.go

+// GenSignature generates a signature for the given payload.
+// hashType possible values are "sha1", "sha256", "sha512".
+// If hashType is empty or invalid, "sha1" is used.
+func GenSignature(payload, secretKey []byte, hashType string) string {


Do we really need this function? I can't think of a case where a user of the go-github library should ever need to generate a signature. I know that you're using it in tests below, but I actually think we should use hardcoded, known signature values for tests.

SGTM. I'll remove it.

gmlewis · 2016-05-06T19:54:07Z

Files updated. PTAL.

parkr · 2016-05-26T20:01:33Z

would love to use this.

Remove most of messages/* when google/go-github#294 ships.

parkr · 2016-06-02T22:42:13Z

@willnorris Any update here? 🙏 FWIW, I am using this code and it works brilliantly. I'd just love to see it in master. 😄

parkr · 2016-06-06T08:02:02Z

github/messages.go

+	sha256Prefix = "sha256"
+	sha512Prefix = "sha512"
+	// eventHeader is the Github header key used to pass the event type.
+	eventHeader = "X-Github-Event"


I believe this should be X-GitHub-Event, with the capital H in GitHub. 😄

The same change should probably be applied to the comments too; the canonical brand name spelling is "GitHub", not "Github". But if that's a wider problem than just in this PR, it might be acceptable to address it in a different PR.

@parkr - It turns out that canonical headers don't work that way. You can verify this yourself using curl or bat.
Here is an example response I just got from the GitHub API:

X-Github-Request-Id : 68840050:7F91:291081F8:5755B423 X-Github-Media-Type : github.v3

I tried searching for the RFC that defines this but came up empty. I'm sure it is out there somewhere.

As for the comments correcting the brand name spelling... yes, that could all be cleaned up by someone.
edit: I'll go ahead and clean that up for this new PR.

It turns out that with all the code I removed from the PR, that those header constants were no longer needed anyway, so I removed them.

gmlewis · 2016-06-10T22:11:44Z

@shurcooL - when you get a chance, could you PTAL?

dmitshur · 2016-06-11T22:11:36Z

github/messages.go

+
+// ValidatedPayload validates an incoming GitHub Webhook event request
+// and returns the (JSON) payload.
+// secretKey is the GitHub Webhook secret message.


I don't have a lot of context on how ValidatedPayload is meant to be used (perhaps it's a good idea to provide an example). So I have the following question.

The client must close the response body when finished with it

Looking at the implementation, I see that ValidatedPayload reads the request body. It does not close the request body.

Is the user expected to close the request body after calling ValidatedPayload?

Also, ValidatedPayload is not documented in a way that makes it clear it will read the request body. Perhaps it can be inferred from "validates the request, returns JSON payload". But maybe it should be explicitly stated that it reads the request body and doesn't close it, if it's indeed the user's responsibility to close it?

Or should it close the request body itself?

Sorry, obviously some more comments are needed. ValidatePayload will be used by a server and GitHub will send events to it. ~~A server does not close the response body... only clients do that, and~~ in this case, GitHub is the client.
Edit: that was poorly worded. I should have said that the http.Server will close the request body for us, so the handler doesn't need to.

How about if I add the comment:

// ValidatePayload is called by the server and therefore does not need to close the request body.

but here we're talking about a request body, not a response body. Isn't it the server's job to close that when finished reading? I'm not sure how strict that needs to be with requests.

In https://godoc.corp.google.com/pkg/net/http it says:

// For server requests the Request Body is always non-nil
// but will return EOF immediately when no body is present.
// The Server will close the request body. The ServeHTTP
// Handler does not need to.

So I don't think it needs closing. Do you agree?

yep, I agree. That sounds pretty clear.

Ah, I completely missed that this is being done on inbound server requests. Right, this is for webhooks, so GitHub is making the request to user code, and this func is used to validate GitHub's incoming request.

How about if I add the comment:

// ValidatePayload is called by the server and therefore does not need to close the request body.

Do you think this comment is helpful and clear? I'm not sure. It almost raises more questions than answers, like "by what server"? It could be the HTTP server that the user is running, or GitHub's server.

If you think it's clear, then feel free to leave it as is.

Otherwise, I think it's completely fine to just remove it. The net/http package covers this pretty well, so I don't think it's needed to repeat it. As long as people realize that ValidatePayload is used to validate requests coming from GitHub. To that end, if issue #361 is going to be resolved by adding a short example of ValidatePayload being used somewhere, then that's even more reason to leave this out. I think seeing ValidatePayload used in context of http.HandleFunc makes it very clear:

http.HandleFunc("/hook", func(w http.ResponseWriter, req *http.Request) { payload, err := github.ValidatePayload(req, ghWebhookSignature) if err != nil { log.Println(err) http.Error(w, "invalid signature", http.StatusForbidden) return } // Process payload ... })

This is really nitpicky so feel free to ignore.

SGTM. I've removed the unclear text and replaced with an example to address #361.

dmitshur · 2016-06-11T22:14:34Z

I've spotted one issue with the request body and/or documentation. Aside from that, I'm not seeing any other issues. I think if we resolve that, this will have my LGTM.

dmitshur · 2016-06-11T22:18:47Z

github/messages.go

+		return errors.New("payload signature check failed")
+	}
+	return nil
+}


Also, what's the difference between ValidatedPayload and ValidateSignature? Is it intentional that one uses adjective "validated" and the other uses verb "validate"? It seems unintentional, since they are both phrased as "X validates ...":

// ValidatedPayload validates ... // ValidateSignature validates ...

The more exported symbols there are, the harder the API is to learn and to use (users will need to learn both, and figure out which one is more appropriate for them to use). Is it beneficial to have both of them exported, or can we make one of them unexported?

ValidatedPayload looks to be a simple wrapper on top of ValidateSignature and does not really do anything different. It really looks to me that we can make one of them unexported and improve the API. What do you think?

SGTM. I will stop exporting validateSignature and rename the other to ValidatePayload.

gmlewis · 2016-06-14T23:59:56Z

@shurcooL - I believe I addressed your comments. PTAL.

dmitshur · 2016-06-15T08:07:31Z

I've looked at the updates, and all changes look great. I've left one more comment about the request body closing misunderstanding here. You can optionally address that.

LGTM.

gmlewis · 2016-06-15T16:02:03Z

Cool, thanks for your reviews, @shurcooL - I appreciate it!
Submitting.

Fixes google#361. Change-Id: I1c80b8eb7816efc9b83e6b8be956430c68809d5e

gmlewis force-pushed the add-messages branch from c386db0 to 579682b Compare February 23, 2016 23:36

gmlewis force-pushed the add-messages branch from 579682b to 6c7e0ff Compare March 4, 2016 23:59

willnorris reviewed May 6, 2016
View reviewed changes

gmlewis force-pushed the add-messages branch from 6c7e0ff to 8807ed8 Compare May 6, 2016 19:53

gmlewis force-pushed the add-messages branch from 8807ed8 to 30c935e Compare May 24, 2016 16:15

willnorris mentioned this pull request May 26, 2016

Webhooks: Add example for signatures #361

Closed

parkr added a commit to parkr/auto-reply that referenced this pull request May 26, 2016

Verify payload with secrets.

d031584

Remove most of messages/* when google/go-github#294 ships.

parkr reviewed Jun 6, 2016
View reviewed changes

gmlewis force-pushed the add-messages branch 4 times, most recently from 81e76a2 to 1cb50ac Compare June 10, 2016 22:09

dmitshur reviewed Jun 11, 2016
View reviewed changes

dmitshur mentioned this pull request Jun 11, 2016

Review https://github.com/google/go-github/pull/294. shurcooL-legacy/backlog#24

Closed

gmlewis force-pushed the add-messages branch from 1cb50ac to 418bb0c Compare June 14, 2016 23:58

gmlewis force-pushed the add-messages branch from 418bb0c to fcf07f0 Compare June 15, 2016 16:02

add utilities for processing payload messages

a8b8751

Fixes google#361. Change-Id: I1c80b8eb7816efc9b83e6b8be956430c68809d5e

gmlewis force-pushed the add-messages branch from fcf07f0 to a8b8751 Compare June 15, 2016 16:13

gmlewis merged commit a8b8751 into google:master Jun 15, 2016

gmlewis deleted the add-messages branch June 15, 2016 16:18

willnorris mentioned this pull request Mar 31, 2023

ValidatePayload should not accept nil or empty secretToken slice #2731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go-github: add utilities for processing payload messages #294

go-github: add utilities for processing payload messages #294

gmlewis commented Feb 23, 2016

willnorris May 6, 2016

gmlewis May 6, 2016

gmlewis commented May 6, 2016

parkr commented May 26, 2016

parkr commented Jun 2, 2016 •

edited

Loading

parkr Jun 6, 2016

dmitshur Jun 6, 2016

gmlewis Jun 6, 2016 •

edited

Loading

gmlewis Jun 6, 2016

gmlewis commented Jun 10, 2016

dmitshur Jun 11, 2016

gmlewis Jun 14, 2016 •

edited

Loading

willnorris Jun 14, 2016

gmlewis Jun 15, 2016

willnorris Jun 15, 2016

dmitshur Jun 15, 2016 •

edited

Loading

gmlewis Jun 15, 2016

dmitshur commented Jun 11, 2016

dmitshur Jun 11, 2016 •

edited

Loading

gmlewis Jun 14, 2016

gmlewis commented Jun 14, 2016

dmitshur commented Jun 15, 2016

gmlewis commented Jun 15, 2016

go-github: add utilities for processing payload messages #294

go-github: add utilities for processing payload messages #294

Conversation

gmlewis commented Feb 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis commented May 6, 2016

parkr commented May 26, 2016

parkr commented Jun 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis commented Jun 10, 2016

Choose a reason for hiding this comment

gmlewis Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Jun 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur commented Jun 11, 2016

dmitshur Jun 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis commented Jun 14, 2016

dmitshur commented Jun 15, 2016

gmlewis commented Jun 15, 2016

parkr commented Jun 2, 2016 •

edited

Loading

gmlewis Jun 6, 2016 •

edited

Loading

gmlewis Jun 14, 2016 •

edited

Loading

dmitshur Jun 15, 2016 •

edited

Loading

dmitshur Jun 11, 2016 •

edited

Loading