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

Update GitHub signature header to use sha256 #123

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

nlecoy
Copy link
Contributor

@nlecoy nlecoy commented Oct 5, 2021

Hi !

First of all, thanks for the work done on Drone.

Just a quick PR to update the header used to validate webhooks from GitHub.
https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks#validating-payloads-from-github

GitHub still supports X-Hub-Signature for backward-compatibility but recommend using the X-Hub-Signature-256

@CLAassistant
Copy link

CLAassistant commented Oct 5, 2021

CLA assistant check
All committers have signed the CLA.

@tphoney
Copy link
Contributor

tphoney commented Oct 12, 2021

Hey @nlecoy thanks for raising this up, unfortunately using a better encryption is normally the way to go. This library needs to always work, and with this change we could potentially break older installations of github server.
IE we will lean here towards compatibility rather than security here.

Though we are open to further discussion.

@nlecoy nlecoy force-pushed the chore/github-webhook-signature branch from 7e0d71c to 05ca388 Compare January 17, 2022 07:47
@nlecoy
Copy link
Contributor Author

nlecoy commented Jan 17, 2022

Hi there,

Sorry to comme back to you just now (didn't see first answer). I added your suggestion @bradrydzewski and added some test.

@tphoney
Copy link
Contributor

tphoney commented Jan 25, 2022

Hey @nlecoy thanks for the PR then the additional work. !

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

Successfully merging this pull request may close these issues.

None yet

4 participants