feat(api): outbound workspace webhooks with signed delivery and logs - #306
Merged
Merged
Conversation
Add workspace-scoped outbound webhooks. Admins register HTTPS endpoints, choose which events they subscribe to (issues, projects, modules, cycles, issue comments), and inspect recent delivery attempts. Issue create/update/ delete now dispatch to matching active webhooks. Delivery runs through the existing RabbitMQ queue: each request is signed with HMAC-SHA256 (X-Devlane-Signature) using a per-webhook secret shown once at creation, retried with backoff, and recorded in webhook_logs. A dial-time SSRF guard resolves the target host and refuses non-public addresses (also covering DNS rebinding); literal private/loopback IPs are rejected at create time so admins get a clear error instead of a webhook that silently fails. Adds the management UI under Settings -> Webhooks (list, create, pause/ resume, delete, delivery-log viewer). Closes Devlaner#195 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds workspace webhook CRUD, event-based issue dispatch, signed and SSRF-protected delivery with logs, authenticated API routes, and a settings UI for webhook management. ChangesWorkspace webhook lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant IssueService
participant WebhookService
participant WebhookStore
participant RabbitMQ
participant NewWebhookDeliverer
participant WebhookEndpoint
IssueService->>WebhookService: Dispatch issue event
WebhookService->>WebhookStore: List active hooks for event
WebhookService->>RabbitMQ: Publish WebhookPayload
RabbitMQ->>NewWebhookDeliverer: Handle webhook payload
NewWebhookDeliverer->>WebhookEndpoint: POST signed JSON payload
NewWebhookDeliverer->>WebhookStore: Create delivery log
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
apps/api/internal/handler/webhook_test.go (1)
28-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage that
secret_keyis not re-exposed by List/Update.Once the List/Update secret-leak (see
handler/webhook.go) is fixed, add assertions here thatlist[0]["secret_key"]andupdated["secret_key"]are empty/absent, so a regression is caught by CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/handler/webhook_test.go` around lines 28 - 40, Add regression assertions in the webhook test after unmarshalling the List and Update responses to verify list[0]["secret_key"] and updated["secret_key"] are empty or absent. Keep the existing status, length, and is_active assertions unchanged.apps/web/src/components/settings/WebhooksSettings.tsx (1)
58-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate webhook-list fetch logic between
loadand the mount effect.
load(58-72) and the mountuseEffect(74-99) implement the same fetch + 403-aware error handling twice — the effect never callsload(). Consider having the effect just callload()(with a cancellation guard insideloaditself), so the two code paths can't drift apart.♻️ Suggested consolidation
useEffect(() => { let cancelled = false; - setLoading(true); - setLoadError(null); - webhookService - .list(workspaceSlug) - .then((res) => { - if (!cancelled) setWebhooks(res); - }) - .catch((err) => { - if (cancelled) return; - const status = (err as { response?: { status?: number } })?.response?.status; - setWebhooks([]); - setLoadError( - status === 403 - ? 'Only workspace admins can manage webhooks.' - : 'Could not load webhooks.', - ); - }) - .finally(() => { - if (!cancelled) setLoading(false); - }); + load(); return () => { cancelled = true; }; }, [workspaceSlug, load]);(Requires
loadto itself no-op on a stale request, e.g. via a ref, if strict double-invoke ordering matters.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/settings/WebhooksSettings.tsx` around lines 58 - 99, Consolidate the duplicate webhook-fetch logic by having the mount useEffect invoke the existing load callback instead of reimplementing webhookService.list, error handling, and loading state updates. Update load to guard against stale or cancelled requests as needed, using the existing workspaceSlug dependency, so strict-mode or unmount ordering cannot apply outdated state updates.apps/api/internal/service/webhook.go (1)
182-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Dispatchsilently discards queue publish errors.The
_ = s.queue.PublishWebhook(...)pattern provides no observability when dispatch fails. If the queue is temporarily down, webhooks are silently dropped with no log or metric to indicate the failure. Adding a logger toWebhookServiceand logging publish errors would make delivery failures debuggable.♻️ Suggested refactor: add logger to WebhookService
type WebhookService struct { webhooks *store.WebhookStore ws *store.WorkspaceStore queue *queue.Publisher // optional; when nil, dispatch is a no-op + log *slog.Logger // optional; when nil, errors are silently dropped } func NewWebhookService(webhooks *store.WebhookStore, ws *store.WorkspaceStore, q *queue.Publisher) *WebhookService { - return &WebhookService{webhooks: webhooks, ws: ws, queue: q} + return &WebhookService{webhooks: webhooks, ws: ws, queue: q, log: slog.Default()} } +func (s *WebhookService) SetLogger(l *slog.Logger) { s.log = l } + func (s *WebhookService) Dispatch(ctx context.Context, workspaceID uuid.UUID, event string, payload map[string]interface{}) { if s == nil || s.queue == nil || !store.IsValidWebhookEvent(event) { return } hooks, err := s.webhooks.ListActiveByWorkspaceAndEvent(ctx, workspaceID, event) if err != nil { + if s.log != nil { + s.log.Warn("webhook dispatch: list active webhooks failed", "workspace_id", workspaceID, "event", event, "error", err) + } return } for i := range hooks { - _ = s.queue.PublishWebhook(ctx, queue.WebhookPayload{ + if err := s.queue.PublishWebhook(ctx, queue.WebhookPayload{ WebhookID: hooks[i].ID.String(), WorkspaceID: workspaceID.String(), URL: hooks[i].URL, Secret: hooks[i].SecretKey, Event: event, Payload: payload, - }) + }); err != nil && s.log != nil { + s.log.Warn("webhook dispatch: publish failed", "webhook_id", hooks[i].ID, "event", event, "error", err) + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/webhook.go` around lines 182 - 200, Update WebhookService to include its logger dependency, then modify Dispatch to capture each PublishWebhook error and log it with relevant webhook and workspace context instead of discarding it. Preserve the existing dispatch loop and payload behavior.apps/api/internal/service/webhook_delivery_internal_test.go (1)
10-22: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAdd a CGNAT test case to
TestIsPublicIP.The test covers RFC 1918, loopback, link-local, and unspecified ranges but not 100.64.0.0/10 (RFC 6598 CGNAT). Adding it would prevent regressions if the
isPublicIPguard is later extended.✨ Suggested addition
blocked := []string{"127.0.0.1", "::1", "10.0.0.5", "192.168.1.1", "172.16.0.1", "169.254.169.254", "0.0.0.0", + "100.64.0.1", }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/webhook_delivery_internal_test.go` around lines 10 - 22, Add an address from the RFC 6598 100.64.0.0/10 CGNAT range to the blocked cases in TestIsPublicIP, preserving the existing assertions and public-IP cases.apps/api/internal/service/webhook_delivery.go (2)
108-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog all sent request headers for audit completeness.
RequestHeadersrecordsContent-Type,X-Devlane-Event, andX-Devlane-Signature, but omitsX-Devlane-DeliveryandUser-Agentwhich are actually sent indeliverOnce. This creates an audit gap when debugging delivery issues from logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/webhook_delivery.go` around lines 108 - 111, Update the request-header logging construction in deliverOnce to include every header actually sent, especially X-Devlane-Delivery and User-Agent, alongside the existing Content-Type, X-Devlane-Event, and optional X-Devlane-Signature entries. Keep the logged values synchronized with the headers used by the outgoing request.
62-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse context-aware backoff instead of
time.Sleep.
time.Sleepblocks the consumer goroutine without checkingctx.Done(). During shutdown, this delays graceful termination by up to 1.5 seconds per in-flight delivery. Replace with aselectthat also listens onctx.Done().♻️ Proposed fix
if attempts < webhookMaxAttempts { - time.Sleep(time.Duration(attempts) * 500 * time.Millisecond) + select { + case <-time.After(time.Duration(attempts) * 500 * time.Millisecond): + case <-ctx.Done(): + return ctx.Err() + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/webhook_delivery.go` around lines 62 - 71, Replace the time.Sleep backoff in the webhook delivery retry loop with a context-aware select that waits for the existing attempt-based duration or returns promptly when ctx.Done() is signaled. Preserve the retry limit and successful-delivery behavior around deliverOnce.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/internal/handler/webhook.go`:
- Around line 41-53: Redact secret_key from webhook responses returned by the
List and Update handler paths. Introduce or reuse a response DTO/mapper that
preserves the other webhook fields while omitting secret_key, and apply it
before the c.JSON calls in WebhookHandler.List and the corresponding Update
method; do not alter persistence or creation behavior.
In `@apps/api/internal/model/webhook.go`:
- Around line 10-29: Update the SecretKey field in Webhook to use json:"-" so it
is never serialized through the model in list, create, or update responses. Add
or reuse a dedicated create-response type that explicitly includes the secret
only for the create flow, and adjust the create handler to return that type
while preserving the existing secret value there.
In `@apps/api/internal/service/webhook_delivery.go`:
- Around line 164-170: Update isPublicIP to explicitly reject RFC 6598 CGNAT
addresses in the 100.64.0.0/10 range, alongside the existing non-public IP
checks, while preserving the current behavior for all other addresses.
- Around line 78-86: Generate the delivery UUID once before the retry loop, then
pass that value through each retry invocation into deliverOnce. Update
deliverOnce to accept and use the provided UUID for X-Devlane-Delivery, removing
the per-attempt uuid.New().String() call so all retries share the same
identifier.
---
Nitpick comments:
In `@apps/api/internal/handler/webhook_test.go`:
- Around line 28-40: Add regression assertions in the webhook test after
unmarshalling the List and Update responses to verify list[0]["secret_key"] and
updated["secret_key"] are empty or absent. Keep the existing status, length, and
is_active assertions unchanged.
In `@apps/api/internal/service/webhook_delivery_internal_test.go`:
- Around line 10-22: Add an address from the RFC 6598 100.64.0.0/10 CGNAT range
to the blocked cases in TestIsPublicIP, preserving the existing assertions and
public-IP cases.
In `@apps/api/internal/service/webhook_delivery.go`:
- Around line 108-111: Update the request-header logging construction in
deliverOnce to include every header actually sent, especially X-Devlane-Delivery
and User-Agent, alongside the existing Content-Type, X-Devlane-Event, and
optional X-Devlane-Signature entries. Keep the logged values synchronized with
the headers used by the outgoing request.
- Around line 62-71: Replace the time.Sleep backoff in the webhook delivery
retry loop with a context-aware select that waits for the existing attempt-based
duration or returns promptly when ctx.Done() is signaled. Preserve the retry
limit and successful-delivery behavior around deliverOnce.
In `@apps/api/internal/service/webhook.go`:
- Around line 182-200: Update WebhookService to include its logger dependency,
then modify Dispatch to capture each PublishWebhook error and log it with
relevant webhook and workspace context instead of discarding it. Preserve the
existing dispatch loop and payload behavior.
In `@apps/web/src/components/settings/WebhooksSettings.tsx`:
- Around line 58-99: Consolidate the duplicate webhook-fetch logic by having the
mount useEffect invoke the existing load callback instead of reimplementing
webhookService.list, error handling, and loading state updates. Update load to
guard against stale or cancelled requests as needed, using the existing
workspaceSlug dependency, so strict-mode or unmount ordering cannot apply
outdated state updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34aafa82-3c49-44e0-a5e3-8d08bf688979
📒 Files selected for processing (16)
apps/api/cmd/api/main.goapps/api/internal/handler/webhook.goapps/api/internal/handler/webhook_test.goapps/api/internal/model/webhook.goapps/api/internal/queue/consumer.goapps/api/internal/queue/queue.goapps/api/internal/router/router.goapps/api/internal/service/issue.goapps/api/internal/service/webhook.goapps/api/internal/service/webhook_delivery.goapps/api/internal/service/webhook_delivery_internal_test.goapps/api/internal/store/webhook.goapps/web/src/api/types.tsapps/web/src/components/settings/WebhooksSettings.tsxapps/web/src/pages/SettingsPage.tsxapps/web/src/services/webhookService.ts
…GNAT Address review findings on the webhooks feature: - The signing secret was serialized on every webhook response (secret_key json tag), so admins could read it on any list/update fetch. Mark it json:"-" and return it exactly once, from the create handler. - X-Devlane-Delivery was regenerated per retry attempt, so retries looked like distinct events to receivers doing idempotency dedup. Generate one delivery id per delivery, reuse it across attempts, and record it in the delivery log. - isPublicIP now also rejects CGNAT shared address space (RFC 6598, 100.64.0.0/10), which ip.IsPrivate() does not cover. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/internal/handler/webhook_test.go (1)
28-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlso verify that PATCH responses omit
secret_key.The test covers secret omission from the list response, but the subsequent PATCH response is only checked for
is_active. Since the contract is “returned only once at creation,” assert thatupdatedalso lackssecret_key; otherwise an update-path secret leak could go undetected.Proposed test assertion
require.Equal(t, false, updated["is_active"]) +require.NotContains(t, updated, "secret_key", "the secret must not leak on update")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/handler/webhook_test.go` around lines 28 - 35, Extend the PATCH response assertions in the webhook test to verify that the updated response, referenced as updated, does not contain secret_key. Keep the existing is_active assertion and creation/list secret-omission coverage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/internal/handler/webhook_test.go`:
- Around line 28-35: Extend the PATCH response assertions in the webhook test to
verify that the updated response, referenced as updated, does not contain
secret_key. Keep the existing is_active assertion and creation/list
secret-omission coverage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83d06c96-ba89-499b-894b-d6dff248fb41
📒 Files selected for processing (5)
apps/api/internal/handler/webhook.goapps/api/internal/handler/webhook_test.goapps/api/internal/model/webhook.goapps/api/internal/service/webhook_delivery.goapps/api/internal/service/webhook_delivery_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/internal/service/webhook_delivery_internal_test.go
- apps/api/internal/model/webhook.go
- apps/api/internal/service/webhook_delivery.go
- apps/api/internal/handler/webhook.go
Contributor
Author
|
@martian56 this one is green now, CI passing and all CodeRabbit comments addressed (secret no longer returned after creation, stable delivery id across retries, and CGNAT blocked in the SSRF guard). Good to merge whenever you're happy with it. |
martian56
approved these changes
Jul 13, 2026
10 tasks
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
2 participants
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.
Feature summary
Workspace admins can now register outbound webhooks that fire signed HTTP POST payloads to their own endpoints when things happen in the workspace, and inspect each delivery attempt without leaving Settings.
Linked issues / discussion
Closes #195
User-facing behavior
Under Settings → Webhooks, an admin adds an endpoint URL and picks which events it subscribes to (issues, projects, modules, cycles, issue comments). On save, a signing secret is shown once. From then on, each subscribed event POSTs a JSON payload to the endpoint with an
X-Devlane-Signature: sha256=<hmac>header (andX-Devlane-Event) that the receiver verifies with the secret. Admins can pause/resume, delete, and open a delivery-log viewer showing recent attempts with their response status and retry count. Issue create/update/delete are wired to dispatch today.What changed
API (
apps/api/)GET/api/workspaces/:slug/webhooks/POST/api/workspaces/:slug/webhooks/PATCH/api/workspaces/:slug/webhooks/:webhookId/DELETE/api/workspaces/:slug/webhooks/:webhookId/GET/api/workspaces/:slug/webhooks/:webhookId/logs/QueueWebhooksconsumer: sign body with HMAC-SHA256, POST with retries + backoff, persist awebhook_logsrow (request/response, retry count).IssueServicedispatchescreated/updated/deletedto matching active webhooks (drafts skipped).UI (
apps/web/)WebhooksSettingscomponent mounted inSettingsPageunder the existing?section=webhookstab (replaces the old placeholder), pluswebhookService.tsand response types.Database
webhooksandwebhook_logstables already exist in the init schema; the old model incorrectly mapped a non-existentproject_idcolumn, which this PR corrects.Why this design
Reused the existing RabbitMQ publish/consume path rather than delivering inline so a slow or failing endpoint never blocks the user action that triggered it, and so retries are natural. The real SSRF enforcement lives at connect time (the only place that defeats DNS rebinding); the create-time literal-IP check is just fast, clear feedback. Events are stored as boolean columns per the existing table shape, so subscription filtering is a plain indexed query.
Test plan
npm run validategreen (typecheck + lint + prettier + go vet + go test)webhook_logsrow written and visible in the log viewer127.0.0.1,10.0.0.5,169.254.169.254,[::1]; valid public URL acceptedAI assistance
Claude Code (Claude Opus 4.8)— and AI-assisted commits include aCo-Authored-By:trailerChecklist
internal/config/config.go(none added)Summary by CodeRabbit