Add foundational webhook package (Phase 1 of dynamic webhook middleware)#3840
Add foundational webhook package (Phase 1 of dynamic webhook middleware)#3840JAORMX merged 8 commits intostacklok:mainfrom
Conversation
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3840 +/- ##
==========================================
+ Coverage 68.77% 69.14% +0.36%
==========================================
Files 473 477 +4
Lines 47919 48074 +155
==========================================
+ Hits 32955 33239 +284
+ Misses 12299 12253 -46
+ Partials 2665 2582 -83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
JAORMX
left a comment
There was a problem hiding this comment.
Hey @Sanskarzz, thanks for putting this together! The foundation looks solid -- clean package structure, good error hierarchy with proper Unwrap() chains, and the HMAC signing implementation is well done. Let's dig into a few things though.
The main blocker is the ValidatingTransport not being applied when TLS config is nil. That leaves the SSRF protections only active in the TLS branch, which doesn't match the #nosec comment's claim. The fix is straightforward (see inline comment), and we should also think about how to handle in-cluster HTTP webhooks (e.g., http://service.namespace.svc.cluster.local) where TLS isn't practical -- probably a configurable opt-in.
Other things to address:
t.Context()instead ofcontext.Background()in tests (project convention)classifyErrormissingcontext.DeadlineExceededhandlingwebhookTypestored but never read
Nice-to-haves / things to think about for follow-ups:
time.DurationJSON serialization (will matter when config integration lands)Principal.Claimsasmap[string]anyfor real-world JWT compatibility- HTTP 422 distinction per RFC failure mode table
- Replay protection docs on
VerifySignature
The scope here is a solid subset of Phase 1 -- types, client, signing, errors. Looking forward to seeing the middleware integration and config wiring in follow-up PRs!
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
JAORMX
left a comment
There was a problem hiding this comment.
All feedback addressed. time.Duration serialization deferred to config integration PR as agreed. LGTM.
Address review feedback on the webhook wire format: auth.Identity carries Token, TokenType, and Metadata fields that have no business in external webhook payloads — even redacted. Extract the credential-free subset (Subject, Name, Email, Groups, Claims) into auth.PrincipalInfo and embed it in Identity. This gives structural safety: webhook.Request.Principal is now *auth.PrincipalInfo, so tokens can never enter the webhook payload. The JSON wire format uses "sub" with omitempty, matching the original webhook.Principal contract from #3840. Field access on Identity (e.g., identity.Subject) continues to work transparently via Go embedding — only struct literals needed updating. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Replace webhook.Principal with auth.Identity The webhook package defined its own Principal struct, but auth.Identity already represents the same concept with a superset of fields. Maintaining two parallel identity types creates sync burden and will require conversion logic in the middleware layer. Since the webhook package has no external consumers yet (v0.1.0), consolidate now before the API solidifies. Closes #4315 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Introduce auth.PrincipalInfo and embed in Identity Address review feedback on the webhook wire format: auth.Identity carries Token, TokenType, and Metadata fields that have no business in external webhook payloads — even redacted. Extract the credential-free subset (Subject, Name, Email, Groups, Claims) into auth.PrincipalInfo and embed it in Identity. This gives structural safety: webhook.Request.Principal is now *auth.PrincipalInfo, so tokens can never enter the webhook payload. The JSON wire format uses "sub" with omitempty, matching the original webhook.Principal contract from #3840. Field access on Identity (e.g., identity.Subject) continues to work transparently via Go embedding — only struct literals needed updating. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix: #3396
Summary
Implements Phase 1 of RFC THV-0017, creating the core pkg/webhook/ package with types, HTTP client, HMAC signing, and error handling.
ToolHive currently has no mechanism for external policy enforcement or request transformation. This PR lays the foundation for the dynamic webhook middleware system that will allow operators to configure validating and mutating webhooks for MCP request interception.
Changes
Design decisions
Testing
Large PR Justification
New foundational package (
pkg/webhook/) — the 4 source files are tightly coupled (client depends on types, errors, signing) and form the minimum viable unit for Phase 1. Over 65% of lines are tests.