New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept application/x-www-form-urlencoded webhooks #693

Merged
merged 6 commits into from Oct 9, 2017

Conversation

Projects
None yet
4 participants
@lkysow
Contributor

lkysow commented Aug 15, 2017

  • Enable ValidatePayload to work for webhooks with Content-Type application/x-www-form-urlencoded in addition to application/json.
  • Reject any Content-Type that is not application/x-www-form-urlencoded or application/json.
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Aug 15, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot commented Aug 15, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.
@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Aug 15, 2017

Contributor

I signed it!

Contributor

lkysow commented Aug 15, 2017

I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Aug 15, 2017

CLAs look good, thanks!

googlebot commented Aug 15, 2017

CLAs look good, thanks!

@dmitshur

I have some initial questions, as I'd like to understand this change better.

We found that users would sometimes accidentally use application/x-www-form-urlencoded because it's the default type in GitHub.

Is this documented somewhere in GitHub documentation? Where could I learn more about this?

Show outdated Hide outdated github/messages.go
Show outdated Hide outdated github/messages.go
Show outdated Hide outdated github/messages_test.go
@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Aug 15, 2017

Contributor

@shurcooL regarding your question, the two different types of webhooks are documented here:
https://developer.github.com/webhooks/creating/#content-type

If you click "Add webhook" this is the screen you'll see where by default the formencoded Content-Type is selected. In our application that asks users to add webhooks we found that sometimes users would add the formencoded content-type (even though our docs say use json), so we wanted to support both webhook types.

image

Contributor

lkysow commented Aug 15, 2017

@shurcooL regarding your question, the two different types of webhooks are documented here:
https://developer.github.com/webhooks/creating/#content-type

If you click "Add webhook" this is the screen you'll see where by default the formencoded Content-Type is selected. In our application that asks users to add webhooks we found that sometimes users would add the formencoded content-type (even though our docs say use json), so we wanted to support both webhook types.

image

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 15, 2017

Member

Awesome, thanks, that answers my question. Since GitHub supports both types, it would be a good enhancement for go-github to support them too. Thanks for sending the PR. My next step will be to review the code, I will get to that in a bit.

Member

dmitshur commented Aug 15, 2017

Awesome, thanks, that answers my question. Since GitHub supports both types, it would be a good enhancement for go-github to support them too. Thanks for sending the PR. My next step will be to review the code, I will get to that in a bit.

@dmitshur

Overall, this is a good start. I have some suggestions for improvements, this is the first pass.

Show outdated Hide outdated github/messages_test.go
Show outdated Hide outdated github/messages_test.go
Show outdated Hide outdated github/messages_test.go
Show outdated Hide outdated github/messages_test.go
Show outdated Hide outdated github/messages.go
Show outdated Hide outdated github/messages_test.go
@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Aug 16, 2017

Contributor

@shurcooL updated

Contributor

lkysow commented Aug 16, 2017

@shurcooL updated

Show outdated Hide outdated github/messages.go

@dmitshur dmitshur requested a review from gmlewis Aug 19, 2017

@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Aug 20, 2017

Contributor

@shurcooL thanks for the review! Updated again based on comments.

Contributor

lkysow commented Aug 20, 2017

@shurcooL thanks for the review! Updated again based on comments.

@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Sep 22, 2017

Contributor

@shurcooL @gmlewis any chance you guys can look at this again? I've made all requested changed.

Contributor

lkysow commented Sep 22, 2017

@shurcooL @gmlewis any chance you guys can look at this again? I've made all requested changed.

@gmlewis

LGTM.
Thank you, @lkysow, and I apologize for the delay.
@shurcooL will merge once he is happy with the PR.

@dmitshur

I have one suggestion. What do you think of it?

Show outdated Hide outdated github/messages.go
Show outdated Hide outdated github/messages.go

@googlebot googlebot added the cla: yes label Sep 28, 2017

lkysow and others added some commits Sep 28, 2017

Move payloadFormParam const into case where it's needed.
This way, it has a smaller scope, no larger than neccessary.

Add blank line before setting payload to mirror the two cases.
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Sep 28, 2017

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot commented Sep 28, 2017

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 28, 2017

@dmitshur

dmitshur approved these changes Sep 28, 2017 edited

I've made a minor change on top of yours @lkysow. Let me know if you're okay with it, or would like to change anything. (Ignore the comment from @googlebot, it's a non-issue.)

This has my LGTM. I'll ask @gmlewis to confirm the latest changes are okay with him too. Then, we can merge whenever it's ready from your side @lkysow.

Let's also create that issue to track and investigate the need for a cap on reading the body.

@dmitshur dmitshur requested a review from gmlewis Sep 28, 2017

@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Sep 28, 2017

Contributor

Sounds good to me 👍 thanks for your excellent code review and explaining why you request certain changes. Really appreciate it.

Contributor

lkysow commented Sep 28, 2017

Sounds good to me 👍 thanks for your excellent code review and explaining why you request certain changes. Really appreciate it.

@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Sep 29, 2017

Contributor

Created #735 to track cap on body size

Contributor

lkysow commented Sep 29, 2017

Created #735 to track cap on body size

@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Oct 6, 2017

Contributor

@gmlewis

I'll ask @gmlewis to confirm the latest changes are okay with him too.

from above

Contributor

lkysow commented Oct 6, 2017

@gmlewis

I'll ask @gmlewis to confirm the latest changes are okay with him too.

from above

@gmlewis gmlewis added cla: yes and removed cla: no labels Oct 6, 2017

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Oct 6, 2017

Collaborator

Sorry for the delay... taking a look now.

Collaborator

gmlewis commented Oct 6, 2017

Sorry for the delay... taking a look now.

@gmlewis

Just a few tiny nits for Go style if you don't mind.
Thank you, @lkysow!

Show outdated Hide outdated github/messages_test.go
Show outdated Hide outdated github/messages_test.go
Show outdated Hide outdated github/messages_test.go
Show outdated Hide outdated github/messages.go
Show outdated Hide outdated github/messages.go
@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Oct 6, 2017

Collaborator

Please ignore the googletbot comment as I have verified that the CLAs all LGTM.

Collaborator

gmlewis commented Oct 6, 2017

Please ignore the googletbot comment as I have verified that the CLAs all LGTM.

@lkysow

This comment has been minimized.

Show comment
Hide comment
@lkysow

lkysow Oct 7, 2017

Contributor

@gmlewis done!

Contributor

lkysow commented Oct 7, 2017

@gmlewis done!

@gmlewis

gmlewis approved these changes Oct 9, 2017

LGTM. Thank you, @lkysow and @shurcooL!
Merging.

@gmlewis gmlewis merged commit c004bf0 into google:master Oct 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lkysow lkysow deleted the lkysow:webhook-form branch Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment