Add webhook notification channel to API layer#626
Conversation
Please add a PR description 🙂Respect the reviewers — a description helps others understand the changes and review them faster. Keep it short, clear, and to the point. It also serves as documentation for future reference. The PR was moved to Draft until a description is added. |
Dobrunia
changed the title
Please add a PR description 🙂Respect the reviewers — a description helps others understand the changes and review them faster. Keep it short, clear, and to the point. It also serves as documentation for future reference. The PR was moved to Draft until a description is added. |
|
Thanks for adding a description — the PR is now marked as Ready for Review. |
…erty to NotificationsChannelsDBScheme interface
… and related tests. Integrate private IP validation into ipValidator module for improved code organization.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #626 +/- ##
===========================================
- Coverage 58.30% 41.34% -16.96%
===========================================
Files 19 44 +25
Lines 518 2177 +1659
Branches 95 464 +369
===========================================
+ Hits 302 900 +598
- Misses 216 1210 +994
- Partials 0 67 +67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds first-class support for a webhook notification channel across the API layer, so clients can configure webhook endpoints via GraphQL and the backend can enqueue webhook deliveries through RabbitMQ.
Changes:
- Registers
webhookas a notification channel in GraphQL types/inputs and DB channel typings. - Enqueues webhook notifications via a new RabbitMQ queue/worker path.
- Introduces SSRF-oriented webhook endpoint validation (protocol/port/hostname/DNS) with new unit tests.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks updated dependency graph for @hawk.so/types@^0.5.9. |
| package.json | Bumps version and updates @hawk.so/types to ^0.5.9. |
| src/types/notification-channels.d.ts | Adds webhook to NotificationsChannelsDBScheme. |
| src/typeDefs/notificationsInput.ts | Adds webhook to GraphQL NotificationsChannelsInput. |
| src/typeDefs/notifications.ts | Adds webhook to GraphQL NotificationsChannels. |
| src/rabbitmq.ts | Adds Queues.Webhook and WorkerPaths.Webhook. |
| src/utils/personalNotifications.ts | Enqueues webhook deliveries when the channel is enabled. |
| src/utils/ipValidator.ts | Adds isPrivateIP helper for private/reserved IP detection. |
| src/utils/webhookEndpointValidator.ts | Adds async webhook URL validator with DNS resolution checks. |
| src/resolvers/userNotifications.ts | Validates webhook endpoint when updating user notification channels. |
| src/resolvers/projectNotifications.ts | Validates webhook endpoint when creating/updating project rules. |
| test/utils/ipValidator.test.ts | Unit tests for isPrivateIP. |
| test/utils/webhookEndpointValidator.test.ts | Unit tests for validateWebhookEndpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const hostname = url.hostname; | ||
|
|
||
| if (BLOCKED_HOSTNAMES.some((pattern) => pattern.test(hostname))) { | ||
| return `Webhook hostname "${hostname}" is not allowed`; | ||
| } | ||
|
|
||
| if (isPrivateIP(hostname)) { | ||
| return 'Webhook URL points to a private/reserved IP address'; | ||
| } |
There was a problem hiding this comment.
isPrivateIP(hostname) is being applied to any hostname string. This will incorrectly reject valid public domains that merely start with a private-looking prefix (e.g. 10.example.com, 192.168.my-domain.com) because the regexes only check string prefixes, not whether the hostname is an actual IP literal. Consider only running the private-IP check when hostname is an IP address (e.g. via net.isIP on a normalized hostname), and rely on the DNS lookup results for domain names. Adding a test case for a domain like 10.example.com would prevent regressions.
| export function isPrivateIP(ip: string): boolean { | ||
| const bare = ip.split('%')[0]; | ||
|
|
||
| return PRIVATE_IP_PATTERNS.some((pattern) => pattern.test(bare)); | ||
| } |
There was a problem hiding this comment.
isPrivateIP currently treats any string as an IP and runs prefix regexes against it. This makes the function return true for non-IP inputs like 10.example.com, which is inconsistent with the function name/docs and can cause false positives in callers. Consider first validating the normalized input with net.isIP (after stripping any zone id) and returning false when it isn't a valid IPv4/IPv6 literal.
src/resolvers/userNotifications.ts
Outdated
There was a problem hiding this comment.
Webhook endpoint validation is skipped when input.webhook.endpoint is an empty string because the condition requires it to be truthy. Since GraphQL String! can still be empty, this allows isEnabled: true with an invalid/empty endpoint to bypass validation and be persisted. Consider validating whenever input.webhook?.isEnabled is true (and failing if the endpoint is empty/invalid), rather than gating on truthiness.
There was a problem hiding this comment.
validateWebhookChannel skips validation when channels.webhook.endpoint is an empty string due to the truthy check. Because the GraphQL input type uses String!, an empty string is still a valid value and would bypass validation while the channel is enabled. Consider validating whenever channels.webhook?.isEnabled is true (and treating empty endpoints as invalid).
…mproved error handling. Update validation logic to await results from channel checks, enhancing overall reliability.
… ALLOWED_PORTS constants from ipValidator module, improving code organization and maintainability.
…o remove endpoint checks when isEnabled is true, streamlining the validation process.
3 participants
Summary