Repository navigation
Harden the webhook handler and stop billing polling on shutdown - #300
Merged
Merged
Conversation
- verify X-Hub-Signature-256 (preferred) and fall back to the legacy SHA-1 X-Hub-Signature only when it is absent - compare digests with hmac.Equal and reject malformed signature headers instead of panicking on a header without '=' - no longer log the expected valid signature on a mismatch - limit the webhook body to 25 MB (GitHub's maximum), 413 above it - fix the inverted nil check of the ping event - cancel the billing pollers on Shutdown and stop the org ticker Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Carlos Panato <ctadeu@gmail.com>
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
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.
Follow-up from the repo review.
Webhook handler
X-Hub-Signature-256(preferred); fall back to the legacy SHA-1X-Hub-Signatureonly when the SHA-256 header is absent (an invalid SHA-256 signature is not rescued by a valid SHA-1 one).hmac.Equal(constant time).=(e.g.X-Hub-Signature: sha1) used to index out of range and panic the handler; malformed headers now return 403.http.MaxBytesReader, responding 413 above it, instead of reading an unbounded body before authenticating.pingEvent == nilcheck: a valid ping is now logged, an undecodable one logs an error and returns 400.Lifecycle
context.TODO()and never stopped;Server.Shutdownnow cancels them. The org poller also stops its ticker like the user one.Behaviour notes
Tests
New tests for SHA-256, precedence over SHA-1, SHA-1 fallback, wrong secret, 11 malformed header cases, body at/over the limit, undecodable ping, and the org poller stopping on context cancel. I confirmed the new webhook tests fail against the previous handler.
go test -raceandgolangci-lintpass.