Skip to content

feat: add webhook delivery dispatcher with SSRF-safe HTTP client, Standard Webhooks signing, and per-endpoint retry/concurrency tuning - #5249

Merged
Pratham-Mishra04 merged 1 commit into
devfrom
07-14-feat_webhook_signing_and_delivery_dispatcher
Jul 17, 2026
Merged

feat: add webhook delivery dispatcher with SSRF-safe HTTP client, Standard Webhooks signing, and per-endpoint retry/concurrency tuning#5249
Pratham-Mishra04 merged 1 commit into
devfrom
07-14-feat_webhook_signing_and_delivery_dispatcher

Conversation

@Pratham-Mishra04

Copy link
Copy Markdown
Collaborator

Summary

This PR introduces the webhook delivery subsystem for Bifrost. It implements the full pipeline for signing, dispatching, and recording webhook notifications when async jobs reach a terminal state, using at-least-once delivery semantics backed by a persistent job queue.

Changes

  • signer.go: Implements Standard Webhooks-compliant HMAC-SHA256 signing (v1,<base64> wire format). The signed content covers {webhookID}.{unix timestamp}.{body}, matching the reference specification vector.
  • client.go: Introduces deliveryClient, which maintains two HTTP clients — a strict SSRF-safe client that blocks private/link-local IPs, and a private client for endpoints registered with allow_private_network that still blocks unspecified and link-local (e.g. cloud metadata) addresses. Both clients refuse redirects and require TLS ≥ 1.2. Timeouts are fully delegated to the per-attempt context rather than held on the client, allowing safe connection pool sharing.
  • payload.go: Renders the JSON delivery envelope (eventEnvelope) for live async job rows, with optional response inlining bounded by a per-endpoint payload cap. Provides a degraded renderExpiredPayload path for when the async job row has TTL-lapsed before the delivery fires, delivering a result_expired: true body instead of dropping the notification.
  • dispatcher.go: Implements Dispatcher, the queue worker that scans for due jobs, atomically claims them with a lease, performs one delivery attempt per claim, records history, and either reschedules (retryable failures) or retires (success, permanent failure, exhausted budget) the job. Retry backoff is exponential with ±20% jitter, capped at a configurable max. Per-endpoint concurrency is bounded by an in-process slot counter. EnqueueJobEvent inserts queue rows at job completion and wakes the worker. DeliverTest sends a signed sample event through the full production path without touching the queue or counters.
  • Tests: Full coverage across signing (including the Standard Webhooks reference vector), payload rendering (thin, include-response, oversized, expired), and dispatcher behavior (success, retryable failure, exhausted budget, permanent failure, redirect refusal, SSRF blocking, expired job degraded payload, endpoint-gone retirement without counter movement, claim fencing, lease recovery, test delivery, custom headers, per-endpoint tuning overrides).

Notable design decisions:

  • The claim-and-lease pattern means the dispatcher can run on every node with no coordination; the atomic claim decides a single owner per attempt.
  • History insert failure intentionally leaves the job claimed so the lease expiry re-offers it, and the receiver's webhook-id deduplication absorbs any resulting duplicate delivery.
  • Reserved delivery headers (webhook-id, webhook-timestamp, webhook-signature, Content-Type, User-Agent, X-Bifrost-Event) always win over endpoint-configured custom headers, even if validation was bypassed.

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

go test ./framework/webhooks/...

The test suite spins up local httptest servers and in-memory fakes for the config store, log store, and endpoint resolver. Key scenarios covered:

  • A successful delivery retires the queue row and increments the endpoint success counter.
  • A 5xx response reschedules the job with exponential backoff and increments the failure counter.
  • After the attempt budget is exhausted, the job is retired with outcome exhausted.
  • A 3xx response is treated as a permanent failure (redirects are never followed).
  • A loopback receiver is unreachable without allow_private_network; link-local addresses are blocked even with it.
  • An expired async job row results in a degraded result_expired payload being delivered rather than the notification being dropped.
  • A job with an expired lease from a dead node is reclaimed and delivered on restart.
  • Custom endpoint headers are forwarded; reserved header names are silently dropped.

Breaking changes

  • Yes
  • No

Security considerations

  • The strict delivery client uses SSRF-safe dial logic that re-resolves and re-validates IPs at dial time, preventing DNS rebinding attacks where a hostname resolves to a public IP at registration but flips to a private one at delivery.
  • The private-network client still blocks unspecified (0.0.0.0) and link-local (169.254.x.x) addresses, preventing access to cloud metadata endpoints even for endpoints explicitly granted private network access.
  • Signing secrets are base64-decoded at sign time; an empty or malformed secret is rejected before any I/O occurs.
  • Reserved delivery headers (webhook-signature, etc.) are always set by the delivery client and cannot be overridden by endpoint-configured custom headers.
  • Both clients require TLS ≥ 1.2 and do not follow redirects.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

Pratham-Mishra04 commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: aaf2539a-e403-4223-ba40-f300851e2b96

📥 Commits

Reviewing files that changed from the base of the PR and between e552924 and 077c8ae.

📒 Files selected for processing (7)
  • framework/webhooks/client.go
  • framework/webhooks/dispatcher.go
  • framework/webhooks/dispatcher_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/payload_test.go
  • framework/webhooks/signer.go
  • framework/webhooks/signer_test.go

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added asynchronous webhook delivery with signed requests and configurable retry handling.
    • Added standardized event payloads for completed and failed jobs, including optional response data.
    • Added webhook delivery status tracking, concurrency limits, endpoint-specific tuning, and test deliveries.
    • Added protections against unsafe destinations, redirects, and unauthorized header overrides.
  • Bug Fixes

    • Expired or unavailable job results now produce a clear degraded webhook payload.
    • Failed deliveries are classified for retry or permanent failure handling.

Walkthrough

The PR adds webhook payload rendering, Standard Webhooks signing, secure HTTP delivery, and an asynchronous dispatcher with leasing, retries, delivery history, endpoint tuning, concurrency limits, and behavioral tests.

Changes

Webhook delivery

Layer / File(s) Summary
Payload rendering and signing
framework/webhooks/payload.go, framework/webhooks/signer.go, framework/webhooks/*_test.go
Adds event payload schemas, async result routing, expired-job payloads, and Standard Webhooks HMAC signing with focused tests.
Secure webhook HTTP delivery
framework/webhooks/client.go, framework/webhooks/dispatcher_test.go
Adds TLS- and redirect-restricted clients, private-network dialing rules, signed POST construction, protected headers, response handling, and outcome classification.
Dispatcher contracts and queue control
framework/webhooks/dispatcher.go, framework/webhooks/dispatcher_test.go
Adds dispatcher stores, lifecycle controls, event enqueueing, wakeups, endpoint tuning, concurrency limits, test delivery, and queue-control tests.
Claiming, attempts, and finalization
framework/webhooks/dispatcher.go, framework/webhooks/dispatcher_test.go
Claims due jobs, renders payloads, performs deliveries, records history and counters, retries with jittered backoff, retires terminal jobs, and tests recovery and failure handling.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Dispatcher
  participant ConfigStore
  participant EndpointResolver
  participant deliveryClient
  participant LogStore
  Dispatcher->>ConfigStore: List and claim due webhook job
  Dispatcher->>EndpointResolver: Resolve endpoint configuration
  Dispatcher->>LogStore: Load async job
  Dispatcher->>deliveryClient: Render and send signed payload
  deliveryClient-->>Dispatcher: Return delivery result
  Dispatcher->>LogStore: Record delivery history
  Dispatcher->>ConfigStore: Reschedule or retire job
Loading

Possibly related PRs

Suggested reviewers: akshaydeo, danpiths

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed Title clearly summarizes the dispatcher, signing, SSRF-safe client, and tuning changes.
Description check ✅ Passed Description covers the required sections and includes summary, changes, testing, type, security, and checklist details.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 07-14-feat_webhook_signing_and_delivery_dispatcher

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"


Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

This looks safe to merge.

  • The render-failure path now records a permanent outcome and retires the queue row.
  • Lease fencing is passed consistently through final queue updates.
  • The delivery client blocks plaintext HTTP without explicit private-network opt-in.
  • No blocking issues remain in the reviewed changes.

Important Files Changed

Filename Overview
framework/webhooks/dispatcher.go Adds queue claiming, delivery attempts, retries, history recording, fenced finalization, and endpoint concurrency limits.
framework/webhooks/client.go Adds signed HTTP delivery with strict and private-network destination policies.
framework/webhooks/payload.go Adds bounded webhook envelopes for live and expired async job results.
framework/webhooks/signer.go Adds Standard Webhooks-compatible HMAC-SHA256 signing.
framework/webhooks/dispatcher_test.go Covers delivery outcomes, retries, lease recovery, SSRF controls, payload failures, and endpoint tuning.

Reviews (5): Last reviewed commit: "feat: webhook signing and delivery dispa..." | Re-trigger Greptile

Comment thread framework/webhooks/dispatcher.go
Comment thread framework/webhooks/client.go
Comment thread framework/webhooks/dispatcher.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
framework/webhooks/dispatcher.go (1)

183-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer bifrost.Ptr() over &now for the pointer field.

CompletedAt: &now takes the address of an existing, unmodified local variable to satisfy a *time.Time field. Based on learnings, this repo's convention is to use bifrost.Ptr() for simple unmodified values rather than the & operator.

-		CompletedAt: &now,
+		CompletedAt: bifrost.Ptr(now),
🤖 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 `@framework/webhooks/dispatcher.go` around lines 183 - 192, Update the sample
AsyncJob initialization in Dispatcher.DeliverTest to use bifrost.Ptr(now) for
CompletedAt instead of taking the address with &now, preserving the existing
timestamp value and pointer field behavior.

Source: Learnings

framework/webhooks/payload.go (1)

87-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor: prefer Ptr() helper over &job.CreatedAt.

job.CreatedAt is a simple, unmodified value being addressed for the JSON pointer field; the repo convention favors schemas.Ptr(value)/bifrost.Ptr(value) over the & operator for this case.

♻️ Proposed tweak
-		CreatedAt:       &job.CreatedAt,
+		CreatedAt:       schemas.Ptr(job.CreatedAt),

Based on learnings, "prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically... Only use schemas.Ptr()/bifrost.Ptr() for simple unmodified values."

🤖 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 `@framework/webhooks/payload.go` around lines 87 - 112, In renderPayload,
replace the direct address expression used for eventData.CreatedAt with the
repository’s bifrost.Ptr() helper for the unmodified job.CreatedAt value,
preserving the existing JSON pointer behavior.

Source: Learnings

framework/webhooks/dispatcher_test.go (1)

339-340: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Fragile single-shot Read() for capturing request bodies.

All three receiver handlers size the buffer from r.ContentLength and call r.Body.Read(buf) once. io.Reader.Read isn't guaranteed to fill the buffer in one call, so this can silently truncate the captured body on a partial read — risking flaky failures, most notably the byte-exact HMAC comparison against received.body in TestDeliverySuccessFlow (line 377-379).

♻️ Suggested fix (apply at all three sites)
-		received.body = make([]byte, r.ContentLength)
-		_, _ = r.Body.Read(received.body)
+		received.body, _ = io.ReadAll(r.Body)

Also applies to: 516-517, 712-713

🤖 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 `@framework/webhooks/dispatcher_test.go` around lines 339 - 340, Replace the
single-shot r.Body.Read calls in all three receiver handlers with a read
operation that consumes the entire request body, such as io.ReadAll, and assign
the returned bytes to received.body. Update the handlers near the existing
received.body assignments while preserving their current body-capture behavior
and error handling conventions.
🤖 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 `@framework/webhooks/dispatcher.go`:
- Around line 98-119: Validate historyRetention in NewDispatcher and reject zero
or negative durations before constructing the Dispatcher, preserving the
documented positive-value precondition. Use the existing constructor’s
established error-handling convention, or add an appropriate failure mechanism
if none exists, and ensure invalid values cannot reach finalize’s expires_at
calculation.
- Around line 459-470: Update retryBackoff to prevent time.Duration overflow
when calculating exponential backoff for high retry counts. Replace the direct
power-and-multiply expression with saturating doubling or equivalent
overflow-safe logic, ensuring the result never becomes negative and remains
capped by tuning.retryBackoffMax before applying jitter.

---

Nitpick comments:
In `@framework/webhooks/dispatcher_test.go`:
- Around line 339-340: Replace the single-shot r.Body.Read calls in all three
receiver handlers with a read operation that consumes the entire request body,
such as io.ReadAll, and assign the returned bytes to received.body. Update the
handlers near the existing received.body assignments while preserving their
current body-capture behavior and error handling conventions.

In `@framework/webhooks/dispatcher.go`:
- Around line 183-192: Update the sample AsyncJob initialization in
Dispatcher.DeliverTest to use bifrost.Ptr(now) for CompletedAt instead of taking
the address with &now, preserving the existing timestamp value and pointer field
behavior.

In `@framework/webhooks/payload.go`:
- Around line 87-112: In renderPayload, replace the direct address expression
used for eventData.CreatedAt with the repository’s bifrost.Ptr() helper for the
unmodified job.CreatedAt value, preserving the existing JSON pointer behavior.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 27e4732b-53c1-42d1-829a-38d590608952

📥 Commits

Reviewing files that changed from the base of the PR and between e623f4b and ea3781b.

📒 Files selected for processing (7)
  • framework/webhooks/client.go
  • framework/webhooks/dispatcher.go
  • framework/webhooks/dispatcher_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/payload_test.go
  • framework/webhooks/signer.go
  • framework/webhooks/signer_test.go

Comment thread framework/webhooks/dispatcher.go
Comment thread framework/webhooks/dispatcher.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@framework/webhooks/client.go`:
- Around line 63-84: Update newPrivateDialContext to handle an empty ips result
after LookupIP succeeds, returning a descriptive error before the loop or ips[0]
access; preserve the existing validation and dialing behavior when at least one
address is returned.

In `@framework/webhooks/dispatcher_test.go`:
- Around line 339-340: Update the test request-body reads at all three locations
to use io.ReadAll(r.Body) rather than a single Read into a preallocated buffer,
and handle the returned error explicitly. Preserve the existing received.body
assignment and apply the same deterministic pattern in each affected test.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d3e3fcb-fcd5-4256-9c9a-12557e37be79

📥 Commits

Reviewing files that changed from the base of the PR and between ea3781b and 34e748e.

📒 Files selected for processing (7)
  • framework/webhooks/client.go
  • framework/webhooks/dispatcher.go
  • framework/webhooks/dispatcher_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/payload_test.go
  • framework/webhooks/signer.go
  • framework/webhooks/signer_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • framework/webhooks/payload_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/signer_test.go
  • framework/webhooks/dispatcher.go

Comment thread framework/webhooks/client.go
Comment thread framework/webhooks/dispatcher_test.go Outdated
@Pratham-Mishra04
Pratham-Mishra04 force-pushed the 07-14-feat_webhook_delivery_queue_and_history_stores branch from 44ef0a1 to f759018 Compare July 16, 2026 09:41
@Pratham-Mishra04
Pratham-Mishra04 force-pushed the 07-14-feat_webhook_signing_and_delivery_dispatcher branch from 34e748e to 3b2b219 Compare July 16, 2026 09:41
Comment thread framework/webhooks/dispatcher.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@framework/webhooks/signer.go`:
- Around line 45-54: Update decodeSecret to validate the decoded key length
after base64 decoding, rejecting keys shorter than 24 bytes or longer than 64
bytes with an appropriate error; continue treating the whsec_ prefix as optional
and preserve the existing empty-input and decode-error handling.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1fe04663-22d6-44e7-9230-e81cba58e922

📥 Commits

Reviewing files that changed from the base of the PR and between 34e748e and 3b2b219.

📒 Files selected for processing (7)
  • framework/webhooks/client.go
  • framework/webhooks/dispatcher.go
  • framework/webhooks/dispatcher_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/payload_test.go
  • framework/webhooks/signer.go
  • framework/webhooks/signer_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • framework/webhooks/signer_test.go
  • framework/webhooks/payload_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/dispatcher.go

Comment thread framework/webhooks/signer.go
@Pratham-Mishra04
Pratham-Mishra04 force-pushed the 07-14-feat_webhook_delivery_queue_and_history_stores branch from f759018 to ffbfc01 Compare July 16, 2026 13:04
@Pratham-Mishra04
Pratham-Mishra04 force-pushed the 07-14-feat_webhook_signing_and_delivery_dispatcher branch from 3b2b219 to e552924 Compare July 16, 2026 13:04

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@framework/webhooks/client.go`:
- Around line 169-177: Update attemptResult and classify so deterministic local
failures such as invalid URLs, invalid secrets, and disallowed plaintext HTTP
carry an explicit permanent-failure classification, while transient network
failures remain retryable. Make retry decisions use that classification instead
of treating every statusCode 0 as retryable, and extend
TestDeliverRefusesPlaintextHTTPWithoutOptIn to verify no retry occurs.
- Around line 143-160: Update the webhook attempt error handling around
client.Do and the non-2xx response path to avoid persisting receiver-controlled
details: replace URL-bearing err.Error() values with a stable sanitized
transport-error category, and replace snippet-based errText with a sanitized
status/category that excludes response bodies. Keep statusCode available where
present and remove the raw body content from delivery history.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: df31c299-c7c4-404b-a1d8-7bf4d8d78f3f

📥 Commits

Reviewing files that changed from the base of the PR and between 3b2b219 and e552924.

📒 Files selected for processing (7)
  • framework/webhooks/client.go
  • framework/webhooks/dispatcher.go
  • framework/webhooks/dispatcher_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/payload_test.go
  • framework/webhooks/signer.go
  • framework/webhooks/signer_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • framework/webhooks/signer_test.go
  • framework/webhooks/payload_test.go
  • framework/webhooks/payload.go
  • framework/webhooks/dispatcher.go

Comment thread framework/webhooks/client.go
Comment thread framework/webhooks/client.go
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 16, 2026

Pratham-Mishra04 commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Merge activity

  • Jul 17, 8:36 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jul 17, 8:45 AM UTC: Graphite rebased this pull request as part of a merge.
  • Jul 17, 8:46 AM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

@Pratham-Mishra04
Pratham-Mishra04 changed the base branch from 07-14-feat_webhook_delivery_queue_and_history_stores to graphite-base/5249 July 17, 2026 08:42
@Pratham-Mishra04
Pratham-Mishra04 changed the base branch from graphite-base/5249 to dev July 17, 2026 08:44
@Pratham-Mishra04
Pratham-Mishra04 dismissed coderabbitai[bot]’s stale review July 17, 2026 08:44

The base branch was changed.

@Pratham-Mishra04
Pratham-Mishra04 force-pushed the 07-14-feat_webhook_signing_and_delivery_dispatcher branch from e552924 to 077c8ae Compare July 17, 2026 08:44
@Pratham-Mishra04
Pratham-Mishra04 merged commit b9380b8 into dev Jul 17, 2026
14 of 15 checks passed
@Pratham-Mishra04
Pratham-Mishra04 deleted the 07-14-feat_webhook_signing_and_delivery_dispatcher branch July 17, 2026 08:46
akshaydeo pushed a commit that referenced this pull request Jul 17, 2026
…ndard Webhooks signing, and per-endpoint retry/concurrency tuning (#5249)

## Summary

This PR introduces the webhook delivery subsystem for Bifrost. It implements the full pipeline for signing, dispatching, and recording webhook notifications when async jobs reach a terminal state, using at-least-once delivery semantics backed by a persistent job queue.

## Changes

- **`signer.go`**: Implements Standard Webhooks-compliant HMAC-SHA256 signing (`v1,<base64>` wire format). The signed content covers `{webhookID}.{unix timestamp}.{body}`, matching the reference specification vector.
- **`client.go`**: Introduces `deliveryClient`, which maintains two HTTP clients — a strict SSRF-safe client that blocks private/link-local IPs, and a private client for endpoints registered with `allow_private_network` that still blocks unspecified and link-local (e.g. cloud metadata) addresses. Both clients refuse redirects and require TLS ≥ 1.2. Timeouts are fully delegated to the per-attempt context rather than held on the client, allowing safe connection pool sharing.
- **`payload.go`**: Renders the JSON delivery envelope (`eventEnvelope`) for live async job rows, with optional response inlining bounded by a per-endpoint payload cap. Provides a degraded `renderExpiredPayload` path for when the async job row has TTL-lapsed before the delivery fires, delivering a `result_expired: true` body instead of dropping the notification.
- **`dispatcher.go`**: Implements `Dispatcher`, the queue worker that scans for due jobs, atomically claims them with a lease, performs one delivery attempt per claim, records history, and either reschedules (retryable failures) or retires (success, permanent failure, exhausted budget) the job. Retry backoff is exponential with ±20% jitter, capped at a configurable max. Per-endpoint concurrency is bounded by an in-process slot counter. `EnqueueJobEvent` inserts queue rows at job completion and wakes the worker. `DeliverTest` sends a signed sample event through the full production path without touching the queue or counters.
- **Tests**: Full coverage across signing (including the Standard Webhooks reference vector), payload rendering (thin, include-response, oversized, expired), and dispatcher behavior (success, retryable failure, exhausted budget, permanent failure, redirect refusal, SSRF blocking, expired job degraded payload, endpoint-gone retirement without counter movement, claim fencing, lease recovery, test delivery, custom headers, per-endpoint tuning overrides).

Notable design decisions:
- The claim-and-lease pattern means the dispatcher can run on every node with no coordination; the atomic claim decides a single owner per attempt.
- History insert failure intentionally leaves the job claimed so the lease expiry re-offers it, and the receiver's `webhook-id` deduplication absorbs any resulting duplicate delivery.
- Reserved delivery headers (`webhook-id`, `webhook-timestamp`, `webhook-signature`, `Content-Type`, `User-Agent`, `X-Bifrost-Event`) always win over endpoint-configured custom headers, even if validation was bypassed.

## Type of change

- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go test ./framework/webhooks/...
```

The test suite spins up local `httptest` servers and in-memory fakes for the config store, log store, and endpoint resolver. Key scenarios covered:

- A successful delivery retires the queue row and increments the endpoint success counter.
- A 5xx response reschedules the job with exponential backoff and increments the failure counter.
- After the attempt budget is exhausted, the job is retired with outcome `exhausted`.
- A 3xx response is treated as a permanent failure (redirects are never followed).
- A loopback receiver is unreachable without `allow_private_network`; link-local addresses are blocked even with it.
- An expired async job row results in a degraded `result_expired` payload being delivered rather than the notification being dropped.
- A job with an expired lease from a dead node is reclaimed and delivered on restart.
- Custom endpoint headers are forwarded; reserved header names are silently dropped.

## Breaking changes

- [ ] Yes
- [x] No

## Security considerations

- The strict delivery client uses SSRF-safe dial logic that re-resolves and re-validates IPs at dial time, preventing DNS rebinding attacks where a hostname resolves to a public IP at registration but flips to a private one at delivery.
- The private-network client still blocks unspecified (`0.0.0.0`) and link-local (`169.254.x.x`) addresses, preventing access to cloud metadata endpoints even for endpoints explicitly granted private network access.
- Signing secrets are base64-decoded at sign time; an empty or malformed secret is rejected before any I/O occurs.
- Reserved delivery headers (`webhook-signature`, etc.) are always set by the delivery client and cannot be overridden by endpoint-configured custom headers.
- Both clients require TLS ≥ 1.2 and do not follow redirects.

## Checklist

- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
akshaydeo pushed a commit that referenced this pull request Jul 18, 2026
…ndard Webhooks signing, and per-endpoint retry/concurrency tuning (#5249)

## Summary

This PR introduces the webhook delivery subsystem for Bifrost. It implements the full pipeline for signing, dispatching, and recording webhook notifications when async jobs reach a terminal state, using at-least-once delivery semantics backed by a persistent job queue.

## Changes

- **`signer.go`**: Implements Standard Webhooks-compliant HMAC-SHA256 signing (`v1,<base64>` wire format). The signed content covers `{webhookID}.{unix timestamp}.{body}`, matching the reference specification vector.
- **`client.go`**: Introduces `deliveryClient`, which maintains two HTTP clients — a strict SSRF-safe client that blocks private/link-local IPs, and a private client for endpoints registered with `allow_private_network` that still blocks unspecified and link-local (e.g. cloud metadata) addresses. Both clients refuse redirects and require TLS ≥ 1.2. Timeouts are fully delegated to the per-attempt context rather than held on the client, allowing safe connection pool sharing.
- **`payload.go`**: Renders the JSON delivery envelope (`eventEnvelope`) for live async job rows, with optional response inlining bounded by a per-endpoint payload cap. Provides a degraded `renderExpiredPayload` path for when the async job row has TTL-lapsed before the delivery fires, delivering a `result_expired: true` body instead of dropping the notification.
- **`dispatcher.go`**: Implements `Dispatcher`, the queue worker that scans for due jobs, atomically claims them with a lease, performs one delivery attempt per claim, records history, and either reschedules (retryable failures) or retires (success, permanent failure, exhausted budget) the job. Retry backoff is exponential with ±20% jitter, capped at a configurable max. Per-endpoint concurrency is bounded by an in-process slot counter. `EnqueueJobEvent` inserts queue rows at job completion and wakes the worker. `DeliverTest` sends a signed sample event through the full production path without touching the queue or counters.
- **Tests**: Full coverage across signing (including the Standard Webhooks reference vector), payload rendering (thin, include-response, oversized, expired), and dispatcher behavior (success, retryable failure, exhausted budget, permanent failure, redirect refusal, SSRF blocking, expired job degraded payload, endpoint-gone retirement without counter movement, claim fencing, lease recovery, test delivery, custom headers, per-endpoint tuning overrides).

Notable design decisions:
- The claim-and-lease pattern means the dispatcher can run on every node with no coordination; the atomic claim decides a single owner per attempt.
- History insert failure intentionally leaves the job claimed so the lease expiry re-offers it, and the receiver's `webhook-id` deduplication absorbs any resulting duplicate delivery.
- Reserved delivery headers (`webhook-id`, `webhook-timestamp`, `webhook-signature`, `Content-Type`, `User-Agent`, `X-Bifrost-Event`) always win over endpoint-configured custom headers, even if validation was bypassed.

## Type of change

- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go test ./framework/webhooks/...
```

The test suite spins up local `httptest` servers and in-memory fakes for the config store, log store, and endpoint resolver. Key scenarios covered:

- A successful delivery retires the queue row and increments the endpoint success counter.
- A 5xx response reschedules the job with exponential backoff and increments the failure counter.
- After the attempt budget is exhausted, the job is retired with outcome `exhausted`.
- A 3xx response is treated as a permanent failure (redirects are never followed).
- A loopback receiver is unreachable without `allow_private_network`; link-local addresses are blocked even with it.
- An expired async job row results in a degraded `result_expired` payload being delivered rather than the notification being dropped.
- A job with an expired lease from a dead node is reclaimed and delivered on restart.
- Custom endpoint headers are forwarded; reserved header names are silently dropped.

## Breaking changes

- [ ] Yes
- [x] No

## Security considerations

- The strict delivery client uses SSRF-safe dial logic that re-resolves and re-validates IPs at dial time, preventing DNS rebinding attacks where a hostname resolves to a public IP at registration but flips to a private one at delivery.
- The private-network client still blocks unspecified (`0.0.0.0`) and link-local (`169.254.x.x`) addresses, preventing access to cloud metadata endpoints even for endpoints explicitly granted private network access.
- Signing secrets are base64-decoded at sign time; an empty or malformed secret is rejected before any I/O occurs.
- Reserved delivery headers (`webhook-signature`, etc.) are always set by the delivery client and cannot be overridden by endpoint-configured custom headers.
- Both clients require TLS ≥ 1.2 and do not follow redirects.

## Checklist

- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
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.

2 participants