Skip to content
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

Validate webhook's payload #4940

Merged
merged 53 commits into from
Feb 27, 2019

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 30, 2018

I tested it with github and gitlab, it works, I'm returning a 403 when the payload isn't valid.

screenshot_2018-12-04 settings santos gallegos rtd-test 1

screenshot_2018-12-04 settings santos gallegos rtd-test

Close #4886

@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Nov 30, 2018
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for now.

readthedocs/restapi/views/integrations.py Outdated Show resolved Hide resolved
readthedocs/restapi/views/integrations.py Outdated Show resolved Hide resolved
readthedocs/integrations/models.py Outdated Show resolved Hide resolved
readthedocs/integrations/utils.py Show resolved Hide resolved
readthedocs/integrations/utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 3, 2018

Codecov Report

Merging #4940 into master will decrease coverage by 0.03%.
The diff coverage is 89.7%.

@@            Coverage Diff             @@
##           master    #4940      +/-   ##
==========================================
- Coverage   76.85%   76.82%   -0.04%     
==========================================
  Files         158      158              
  Lines        9991     9998       +7     
  Branches     1254     1248       -6     
==========================================
+ Hits         7679     7681       +2     
- Misses       1978     1984       +6     
+ Partials      334      333       -1
Impacted Files Coverage Δ
readthedocs/restapi/views/integrations.py 93.33% <100%> (+1.37%) ⬆️
readthedocs/oauth/services/base.py 47.31% <100%> (+1.15%) ⬆️
readthedocs/projects/forms.py 79.44% <50%> (-0.56%) ⬇️
readthedocs/oauth/services/gitlab.py 52.53% <50%> (-0.04%) ⬇️
readthedocs/integrations/models.py 81.56% <60%> (-0.73%) ⬇️
readthedocs/integrations/utils.py 43.75% <75%> (-18.75%) ⬇️
readthedocs/oauth/services/github.py 43.31% <83.33%> (-0.28%) ⬇️
readthedocs/vcs_support/backends/svn.py 29.5% <0%> (-9.89%) ⬇️
readthedocs/core/utils/__init__.py 74.73% <0%> (-5.68%) ⬇️
... and 14 more

@readthedocs readthedocs deleted a comment from codecov bot Dec 4, 2018
@stsewd
Copy link
Member Author

stsewd commented Dec 4, 2018

So, there are a couple of decisions here. I'm generating a secret by default, when applying the migrations, all the integrations will have a secret, and we don't want that, because those don't have a secret set on github/gitlab.

Also, when users create a webhook, a secret is generated, but they can't see the secret, so the can't create a webhook manually.

I can suggest these things:

  • Only generate a secret when creating a webhook using the provider's api
  • Let the user see the secret (it can't be changed by the user), or we can let the user change this value too, so they can set up a secret on their own, or don't let the user see the secret and only generate one when creating an integration using the provider's api.

@stsewd
Copy link
Member Author

stsewd commented Dec 4, 2018

So, I changed the code to only create a secret when rtd create the webhook, if the user creates the webhook manually it can't set a secret. Also, users can't see the secrets right now.

@stsewd stsewd requested a review from a team January 17, 2019 23:17
@stsewd stsewd requested a review from a team January 24, 2019 18:09
@agjohnson agjohnson modified the milestones: 3.2, 3.3 Jan 25, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Believe this is ready 👍 We likely need to update the migration names tho.

@stsewd
Copy link
Member Author

stsewd commented Feb 26, 2019

Merged with master, migrations are still valid

@stsewd stsewd requested review from humitos and agjohnson and removed request for humitos February 26, 2019 19:01
@ericholscher ericholscher merged commit 9f9d240 into readthedocs:master Feb 27, 2019
@stsewd stsewd deleted the validate-payload-webhooks branch February 27, 2019 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants