Repository navigation
fix: close an empty-secret webhook bypass and an unbounded token wait - #61
Merged
Merged
Conversation
HMAC accepts a zero-length key without complaint, so a deployment that called Middleware(nil) or read a missing secret from the environment verified every delivery against a key anyone can reproduce. Forged payloads reached the downstream handler as authentic. Verify now returns ErrMissingSecret when the secret is empty. A misconfigured deployment fails closed and surfaces as failed deliveries rather than silently accepting forgeries. FuzzVerify asserted that a freshly computed signature always verifies against its inputs, and its seed corpus included an empty secret, so the invariant encoded the vulnerability as correct behaviour. It now demands ErrMissingSecret for an empty secret and keeps the round-trip property for real ones. Also corrects the Middleware comment claiming the default error handler writes no body; it writes a short plain-text reason.
Dial, TLS handshake and idle connections were all capped, but nothing limited how long the transport would wait for a response header. A server that accepted the connection and then went quiet hung Token() forever: the default context is context.Background(), and the token cache holds its mutex across a refresh, so one stalled request blocked every concurrent caller. Sets ResponseHeaderTimeout to 30s rather than a whole-request client.Timeout, which would also cut off legitimately slow bodies.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 96.44% 96.47% +0.03%
==========================================
Files 4 4
Lines 281 284 +3
==========================================
+ Hits 271 274 +3
Misses 10 10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
An empty webhook secret is used as an HMAC key rather than refused. HMAC accepts a zero-length key, so a deployment that passes a missing config value verifies every delivery against a key anyone can reproduce, and forged payloads reach the downstream handler as authentic. Separately the HTTP client bounds dial, TLS handshake and idle connections, but nothing bounds the wait for a response header, so a server that accepts a connection and then goes quiet hangs token retrieval forever. Both now fail closed.
Changes
Verifyreturns a newErrMissingSecretfor an empty secret, soMiddlewareanswers 401 instead of passing the delivery downstreamResponseHeaderTimeoutis set to 30s on the default transport, with a test asserting every transport stage stays boundedFuzzVerifyasserted that a freshly computed signature always verifies and seeded an empty secret, so it encoded this bug as correct behaviour; it now requiresErrMissingSecretin that case and keeps the round-trip property for real secretsMiddlewarecomment claiming the default error handler writes no bodyRationale
ResponseHeaderTimeoutrather than a whole-requestclient.Timeout, which would also sever legitimately slow response bodies. An empty secret fails closed rather than panicking at construction, so a misconfigured deployment surfaces as failed deliveries instead of taking the server down on deploy.Migration Notes
VerifyandMiddlewarenow reject an empty secret. Any caller depending on the previous behaviour was accepting forged deliveries.