-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Validate webhook's payload #4940
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
Conversation
There was a problem hiding this 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.
Codecov Report
@@ 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
|
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:
|
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. |
There was a problem hiding this 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.
Merged with master, migrations are still valid |
I tested it with github and gitlab, it works, I'm returning a 403 when the payload isn't valid.
Close #4886