Conversation
There was a problem hiding this comment.
Pull request overview
Adds an outgoing webhook system so pipeline events (e.g., WACS events and repo analysis results) can be delivered to registered external endpoints.
Changes:
- Added HTTP endpoints to register/unregister outgoing webhooks backed by Azure Table Storage.
- Added worker Service Bus triggers + dispatcher to POST matching messages to registered webhook URLs.
- Introduced shared webhook models/storage service in
PipelineCommon, plus resilience/retry support for dispatching.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| ScriptureRenderingPipelineWorker/WebhookDispatcherTrigger.cs | New Service Bus triggers to invoke webhook dispatch for WACS + repo analysis result messages. |
| ScriptureRenderingPipelineWorker/WebhookDispatcher.cs | New dispatcher that filters registered webhooks and POSTs payloads with retry/resilience. |
| ScriptureRenderingPipelineWorker/ScriptureRenderingPipelineWorker.csproj | Adds dependency needed for the resilience pipeline. |
| ScriptureRenderingPipelineWorker/Program.cs | Registers Table client, HttpClient, and webhook services for the worker. |
| ScriptureRenderingPipeline/ScriptureRenderingPipeline.csproj | Adds Azure Tables package for webhook storage support. |
| ScriptureRenderingPipeline/Program.cs | Registers Table client and webhook storage service for the API app. |
| ScriptureRenderingPipeline/OutgoingWebhook.cs | New HTTP functions to register and unregister webhook definitions. |
| PipelineCommon/PipelineCommon.csproj | Adds Azure Tables + Azure client factory dependencies for shared webhook storage implementation. |
| PipelineCommon/Models/WebhookDefinition.cs | New model representing a webhook subscription (URL + message/event type). |
| PipelineCommon/Models/Webhook/WebhookResponses.cs | New response DTOs for webhook registration/deletion endpoints. |
| PipelineCommon/IWebhookService.cs | New interface for webhook registration, deletion, existence checks, and listing. |
| PipelineCommon/Helpers/AzureStorageWebhookStorage.cs | New Azure Table Storage implementation of IWebhookService. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Function("RegisterWebhook")] | ||
| public async Task<HttpResponseData> Register([HttpTrigger(AuthorizationLevel.Anonymous, "post")] HttpRequestData req) | ||
| { | ||
| try | ||
| { | ||
| var requestBody = await new StreamReader(req.Body).ReadToEndAsync(); | ||
| var webhookDefinition = JsonSerializer.Deserialize<WebhookDefinition>(requestBody); | ||
|
|
||
| if (webhookDefinition == null || string.IsNullOrWhiteSpace(webhookDefinition.Url)) | ||
| { | ||
| var badResponse = req.CreateResponse(HttpStatusCode.BadRequest); | ||
| await badResponse.WriteStringAsync("Invalid webhook definition: URL is required"); |
There was a problem hiding this comment.
RegisterWebhook is AuthorizationLevel.Anonymous and persists arbitrary URLs provided by the caller. This enables unauthenticated modification of webhook configuration and can create an SSRF vector when the worker later POSTs to those URLs. Consider requiring at least Function auth (or other authn/z), and/or validating/allow-listing destinations (e.g., allowed hostnames, require HTTPS).
| [Function("RegisterWebhook")] | |
| public async Task<HttpResponseData> Register([HttpTrigger(AuthorizationLevel.Anonymous, "post")] HttpRequestData req) | |
| { | |
| try | |
| { | |
| var requestBody = await new StreamReader(req.Body).ReadToEndAsync(); | |
| var webhookDefinition = JsonSerializer.Deserialize<WebhookDefinition>(requestBody); | |
| if (webhookDefinition == null || string.IsNullOrWhiteSpace(webhookDefinition.Url)) | |
| { | |
| var badResponse = req.CreateResponse(HttpStatusCode.BadRequest); | |
| await badResponse.WriteStringAsync("Invalid webhook definition: URL is required"); | |
| private static bool IsValidWebhookUrl(string url) | |
| { | |
| if (string.IsNullOrWhiteSpace(url)) | |
| { | |
| return false; | |
| } | |
| if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) | |
| { | |
| return false; | |
| } | |
| // Only allow HTTPS webhooks to reduce risk of SSRF and ensure transport security. | |
| return string.Equals(uri.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase); | |
| } | |
| [Function("RegisterWebhook")] | |
| public async Task<HttpResponseData> Register([HttpTrigger(AuthorizationLevel.Function, "post")] HttpRequestData req) | |
| { | |
| try | |
| { | |
| var requestBody = await new StreamReader(req.Body).ReadToEndAsync(); | |
| var webhookDefinition = JsonSerializer.Deserialize<WebhookDefinition>(requestBody); | |
| if (webhookDefinition == null || !IsValidWebhookUrl(webhookDefinition.Url)) | |
| { | |
| var badResponse = req.CreateResponse(HttpStatusCode.BadRequest); | |
| await badResponse.WriteStringAsync("Invalid webhook definition: a valid HTTPS URL is required"); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Function("RegisterWebhook")] | ||
| public async Task<HttpResponseData> Register([HttpTrigger(AuthorizationLevel.Anonymous, "post")] HttpRequestData req) | ||
| { | ||
| try | ||
| { | ||
| using var reader = new StreamReader(req.Body); | ||
| var requestBody = await reader.ReadToEndAsync(); | ||
| var webhookDefinition = JsonSerializer.Deserialize<WebhookDefinition>(requestBody); | ||
|
|
||
| if (webhookDefinition == null || string.IsNullOrWhiteSpace(webhookDefinition.Url)) | ||
| { | ||
| var badResponse = req.CreateResponse(HttpStatusCode.BadRequest); | ||
| await badResponse.WriteStringAsync("Invalid webhook definition: URL is required"); | ||
| return badResponse; | ||
| } |
There was a problem hiding this comment.
This endpoint is AuthorizationLevel.Anonymous and accepts an arbitrary Url that will later be POSTed to by the worker. That’s a direct SSRF/abuse vector (e.g., registering internal IPs/metadata endpoints) and also allows anyone to register/delete webhooks. Require authentication/authorization and add strict URL validation (https-only, host allowlist, block private/link-local IP ranges, etc.).
| using var reader = new StreamReader(req.Body); | ||
| var requestBody = await reader.ReadToEndAsync(); | ||
| var webhookDefinition = JsonSerializer.Deserialize<WebhookDefinition>(requestBody); | ||
|
|
There was a problem hiding this comment.
JsonSerializer.Deserialize<WebhookDefinition>(requestBody) uses default options (case-sensitive by default). Clients sending conventional camelCase JSON (url, messageType, eventType) will fail to bind, and MessageType may silently fall back to the model default. Use a configured JsonSerializerOptions (e.g., case-insensitive) or add WebhookDefinition to a source-generated JsonSerializerContext and deserialize with that for consistent API behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async Task DispatchGenericMessageAsync<T>(string messageType, string eventType, T message) | ||
| { | ||
| try | ||
| { | ||
| var webhooks = await _webhookService.GetWebhooksAsync(); | ||
|
|
||
| // Filter webhooks matching the event type | ||
| var matchingWebhooks = webhooks | ||
| .Where(w => string.Equals(w.EventType, eventType, StringComparison.OrdinalIgnoreCase) && | ||
| string.Equals(w.MessageType, messageType, StringComparison.OrdinalIgnoreCase)) | ||
| .ToList(); | ||
|
|
There was a problem hiding this comment.
New webhook dispatch logic is core behavior and would benefit from automated tests (e.g., filtering by MessageType+EventType, retry behavior, and that failed webhook calls don’t block others). There are existing NUnit tests under SRPTests; consider adding unit tests for this dispatcher and webhook storage integration points.
| [Function("RegisterWebhook")] | ||
| public async Task<HttpResponseData> Register([HttpTrigger(AuthorizationLevel.Anonymous, "post")] HttpRequestData req) | ||
| { | ||
| try | ||
| { | ||
| using var reader = new StreamReader(req.Body); | ||
| var requestBody = await reader.ReadToEndAsync(); | ||
| var webhookDefinition = JsonSerializer.Deserialize<WebhookDefinition>(requestBody); | ||
|
|
||
| if (webhookDefinition == null || string.IsNullOrWhiteSpace(webhookDefinition.Url)) | ||
| { | ||
| var badResponse = req.CreateResponse(HttpStatusCode.BadRequest); | ||
| await badResponse.WriteStringAsync("Invalid webhook definition: URL is required"); | ||
| return badResponse; | ||
| } | ||
|
|
||
| if (!AllowedMessageTypes.Any(i => i == webhookDefinition.MessageType)) | ||
| { | ||
| var badResponse = req.CreateResponse(HttpStatusCode.BadRequest); | ||
| await badResponse.WriteStringAsync($"Invalid webhook definition: MessageType '{webhookDefinition.MessageType}' is not allowed"); | ||
| return badResponse; | ||
| } |
There was a problem hiding this comment.
This endpoint is AuthorizationLevel.Anonymous and allows registering arbitrary webhook URLs. That enables SSRF and data exfiltration (the worker will POST internal data to attacker-controlled URLs). Require authentication (function key/AAD) and add URL validation/allowlisting (e.g., only https, block private IP ranges, optional domain allowlist) before persisting the webhook.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@PurpleGuitar What do you think about the needing or not needing a secret function key to create and or delete webhooks? |
|
Ended up making the register webhook require a function key. We can remove that later if we want. |
This pull request introduces a robust outgoing webhook infrastructure for the pipeline, enabling registration, deletion, listing, and resilient dispatching of webhook notifications. The main changes include implementing Azure Table Storage-backed webhook management, providing HTTP endpoints for webhook registration and deletion, and adding a resilient dispatcher for outgoing webhook messages.
Webhook Infrastructure Implementation
IWebhookServiceinterface andAzureStorageWebhookStorageimplementation for managing webhook registration, deletion, existence checks, and listing, backed by Azure Table Storage (PipelineCommon/IWebhookService.cs,PipelineCommon/Helpers/AzureStorageWebhookStorage.cs). [1] [2]WebhookDefinitionand response models for registration and deletion operations (PipelineCommon/Models/WebhookDefinition.cs,PipelineCommon/Models/Webhook/WebhookResponses.cs). [1] [2]HTTP Endpoint Integration
RegisterWebhook) and deletion (UnregisterWebhook), including validation and error handling (ScriptureRenderingPipeline/OutgoingWebhook.cs).Webhook Dispatching and Resilience
WebhookDispatcherclass for sending messages to registered webhooks, using Polly-based retry and resilience strategies to handle transient failures (ScriptureRenderingPipelineWorker/WebhookDispatcher.cs).ScriptureRenderingPipeline/Program.cs,ScriptureRenderingPipelineWorker/Program.cs). [1] [2]Dependency and Project Configuration
PipelineCommon/PipelineCommon.csproj,ScriptureRenderingPipeline/ScriptureRenderingPipeline.csproj,ScriptureRenderingPipelineWorker/ScriptureRenderingPipelineWorker.csproj). [1] [2] [3]- WIP