Conversation
is_valid_signature() raised instead of returning False for several headers: "sha1" with no "=" (ValueError), an unknown algorithm name (TypeError from hmac.new with digestmod=None), "new=..." (hashlib.new used as the digest), and a non-ASCII digest (TypeError from compare_digest on str). Each of these turned the CI web hook into a 500 instead of the configured abort code. Looking the algorithm up in hashlib.__dict__ also meant any hashlib digest was accepted, e.g. md5. Only accept sha1 and sha256, the hashes GitHub signs with, treat anything malformed as an invalid signature, and compare the digests as bytes.
SajalDevX
requested review from
canihavesomecoffee and
thealphadollar
as code owners
September 26, 2026 16:32
X-Hub-Signature is always HMAC-SHA1. GitHub also sends X-Hub-Signature-256 on every delivery that has a secret, so only accept sha256 signatures and drop SHA-1 from the signature path.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
utility.is_valid_signature()raises on severalX-Hub-Signaturevalues instead of returningFalse, so the CI web hook answers 500 rather than the configured abort code:sha1(no=)ValueError: not enough values to unpackunknown=abcTypeError: Missing required argument 'digestmod'new=abcTypeError(hashlib.newends up as the digest)sha1=éTypeError: comparing strings with non-ASCII characters is not supportedThe algorithm was also looked up in
hashlib.__dict__, so a correctly keyedmd5=...signature was accepted too.Change: only
sha1andsha256are accepted, since those are the hashes GitHub signs with. Anything missing or malformed returnsFalse, and the digests are compared as bytes.Tests: two new tests in
tests/test_utility.py, one for the accepted hashes and one for the rejected headers. The second fails onmaster. The fullnose2run passes (805 tests), as do isort, pydocstyle, pycodestyle and mypy.I haven't changed which header is read. GitHub also sends
X-Hub-Signature-256, and I'm happy to switch the web hook to prefer it in a follow-up if you'd like.I used Claude to help find this and write the tests; I checked the change and ran the suite locally.
Update: SonarCloud flagged the SHA-1 signature path, so the check now reads
X-Hub-Signature-256instead ofX-Hub-Signature. GitHub sends that header on every delivery that has a secret, sosha256is the only algorithm accepted now;sha1,md5and anything else are rejected. The test helpers sign with SHA-256 as well.tests.test_utilityandtests.test_cipass (249 tests).