Skip to content

Harden the webhook handler and stop billing polling on shutdown - #300

Merged
cpanato merged 1 commit into
mainfrom
fix/webhook-signature-hardening
Oct 9, 2026
Merged

cpanato merged 1 commit into
mainfrom
fix/webhook-signature-hardening

Conversation

@cpanato

@cpanato cpanato commented Oct 9, 2026

Copy link
Copy Markdown
Owner

Follow-up from the repo review.

Webhook handler

  • Verify X-Hub-Signature-256 (preferred); fall back to the legacy SHA-1 X-Hub-Signature only when the SHA-256 header is absent (an invalid SHA-256 signature is not rescued by a valid SHA-1 one).
  • Compare digests with hmac.Equal (constant time).
  • A header without = (e.g. X-Hub-Signature: sha1) used to index out of range and panic the handler; malformed headers now return 403.
  • The mismatch error used to log the expected valid signature; it no longer does.
  • Limit the body to 25 MB (GitHub's maximum) with http.MaxBytesReader, responding 413 above it, instead of reading an unbounded body before authenticating.
  • Fix the inverted pingEvent == nil check: a valid ping is now logged, an undecodable one logs an error and returns 400.

Lifecycle

  • Billing pollers were started with context.TODO() and never stopped; Server.Shutdown now cancels them. The org poller also stops its ticker like the user one.

Behaviour notes

  • Valid SHA-1 webhooks keep working unchanged; GitHub sends both headers, so SHA-256 is used automatically.
  • Error responses are the same status codes as before, except 413 for oversized bodies.

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 -race and golangci-lint pass.

- 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>
@cpanato
cpanato merged commit ac4c1fb into main Oct 9, 2026
3 checks passed
@cpanato
cpanato deleted the fix/webhook-signature-hardening branch October 9, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant