Repository navigation
Conversation
nishantmonu51
added
Type:Feature
be85820 to
25a6448
Compare
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
Four findings inline; the credential-forwarding one on runtime/drivers/webhook/notifier.go is the most significant.
The branch also no longer merges cleanly with main: besides api.pb.go, git merge-tree reports content conflicts in AlertDialogDeliveryTab.svelte, BaseScheduledReportForm.svelte, ScheduledReportDialog.svelte, en.json and es.json, because main refactored the submit options and made columns optional for canvas reports. A rebase is needed before this can be evaluated end to end.
Two smaller notes:
retryablehttp.NewClient()builds a freshhttp.Clientand pooled transport on everysend, andCloseIdleConnectionsonly runs on the failure path, so each successful delivery leaves a transport's idle connections open untilIdleConnTimeout. Building the client once innewNotifieravoids this.- The
popCurrentExecutiondrive-by fix described in the PR body is not in the diff; nothing underruntime/reconcilers/is touched, andmainalready guardsadminMetain the non-email path atruntime/reconcilers/alert.go:931.
| continue | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| for k, v := range n.headers { |
There was a problem hiding this comment.
Every header from the connector config is set on every request regardless of destination, and the documented use case for headers is an Authorization bearer token for an API gateway. CreateAlerts/CreateReports are granted to every project viewer (admin/database/postgres/migrations/0023.sql:2 sets create_alerts = read_prod) and the dialog accepts any http(s) URL, so a viewer who creates an alert with urls: [https://attacker.example] receives the project's gateway token on the next execution. Slack does not have this problem because its bot token only ever goes to slack.com. Separately, there is no private-IP or metadata guard and the default http.Client follows redirects, so the runtime will also POST (blind, with retries) to internal services reachable from its network. An allowed_url_prefixes allowlist, following the precedent of path_prefixes in runtime/drivers/https/https.go:58 and made mandatory whenever headers is set, plus a private-IP guard, closes both holes.
There was a problem hiding this comment.
Fixed in 663a4ab. headers now requires allowed_url_prefixes. This is checked in Ping, so it shows as a connector error, and again when the notifier is built. A URL outside the list fails on its own and the other URLs are still delivered.
The match is not a plain string prefix like path_prefixes: scheme and host (with port) must be equal, and the path must start with the entry's path. So https://example.com does not admit https://example.com.evil.io.
Without an allowlist, loopback, private and link-local addresses are blocked when dialing, after DNS resolution. In that case the client also connects directly instead of through an environment proxy, because through a proxy the guard would check the proxy's address. A listed URL skips the guard, for localhost during development or an internal receiver in a self-hosted setup. Redirects are not followed anymore: a 3xx counts as a failed delivery.
| client := retryablehttp.NewClient() | ||
| client.Logger = nil | ||
| client.RetryMax = n.retryMax | ||
| client.RetryWaitMin = n.retryWaitMin | ||
| client.RetryWaitMax = n.retryWaitMax | ||
| client.HTTPClient.Timeout = n.requestTimeout | ||
|
|
||
| var errs []error | ||
| for _, u := range dedupe(n.props.URLs) { | ||
| req, err := retryablehttp.NewRequest(http.MethodPost, u, body) |
There was a problem hiding this comment.
retryablehttp's DefaultBackoff returns a server-supplied Retry-After verbatim on 429/503 without capping it at RetryWaitMax (go-retryablehttp@v0.7.8/client.go:551-566; parseRetryAfterHeader accepts any non-negative integer or future HTTP-date), and retryablehttp.NewRequest carries context.Background(), so the select on req.Context().Done() inside Do never fires. A receiver at any URL a viewer supplies can answer 429 with Retry-After: 86400 and block SendAlertStatus for a day per retry, holding the alert's reconcile goroutine, surviving an edit or delete of the alert, and delaying runtime shutdown. Even the benign worst case is roughly 33 s per URL, sent sequentially with no shared deadline. Set a custom Backoff that clamps to RetryWaitMax, and give the request a context with an overall deadline: Notifier takes no context today, so a context.WithTimeout inside send is the minimum.
There was a problem hiding this comment.
Fixed in 7bcebf6. The backoff now caps Retry-After at RetryWaitMax, and requests carry a context with one 60s deadline for the whole send. URLs are now delivered in parallel, up to 8 at a time, under that deadline, so one slow receiver does not delay the others.
| repeated string slack_users = 10; | ||
| repeated string slack_channels = 11; | ||
| repeated string slack_webhooks = 12; | ||
| repeated string webhook_urls = 23; |
There was a problem hiding this comment.
metrics_view_filters = 23 landed in ReportOptions on main in #9770 (canvas PDF export), after this branch's 2026-07-21 merge-base. A three-way merge of this file succeeds textually but produces two fields numbered 23 in the message (verified with git merge-tree), so make proto.generate fails on the merged tree and the committed api.pb.go conflicts as well. Renumber to 24, since 19 is reserved. AlertOptions.webhook_urls = 18 is still free.
There was a problem hiding this comment.
Done in the rebase: ReportOptions.webhook_urls is now 24, and the generated code is regenerated. AlertOptions.webhook_urls stays 18.
| Channels []string `yaml:"channels"` | ||
| Webhooks []string `yaml:"webhooks"` | ||
| } | ||
| Webhook struct { |
There was a problem hiding this comment.
This nested struct has no omitempty, so yamlForManagedAlert, yamlForManagedReport and the unsubscribe re-marshal at admin/server/alerts.go:383 always serialize webhook: {urls: []}. The runtime parser decodes alerts and reports with KnownFields(true) (runtime/parser/parse_node.go:370), and in Rill Cloud the admin service and project runtimes deploy independently, so a runtime that has not been upgraded yet rejects every alert or report created, edited or unsubscribed after the admin deploy with an unknown-field parse error. yaml:"webhook,omitempty" here and on the matching struct at admin/server/reports.go:866 keeps the output byte-identical unless URLs are actually set.
There was a problem hiding this comment.
Fixed in 5802ead, on both structs. A test checks the managed and committed YAML of alerts and reports: no notify.webhook key without URLs, and the key is there when URLs are set.
25a6448 to
4673a20
Compare
|
Thanks for the review. I rebased on main and resolved the conflicts you listed. Each finding has its own commit, see the replies in the threads. About the two notes:
We also found and fixed two more things:
One question: Co-authored with Claude. |
4673a20 to
929ab0d
Compare
|
Hi @nishantmonu51, just rebased this so we keep it mergeable until it's done or closed. The only new commit is 929ab0d, which adds the |
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up. All four threads are resolved well, and the allowlist matching on scheme and canonical host, the dial-time IP check, and the final status in the retry error are all improvements over what I asked for.
The branch has drifted again, though, in two ways that a textual merge won't catch. First, main has since used both of the new proto field numbers: ReportOptions.export_attach = 24 landed in #10031 and AlertOptions.export_attach = 18 landed in #10060. api.proto merges cleanly but ends up with duplicate numbers in both messages, so webhook_urls needs to move to 25 and 22 respectively. Second, #10009 added AsMCP() to drivers.Handle, so the webhook handle no longer compiles against main. I confirmed that adding the usual return nil, false stub is enough for the driver to build and its tests to pass on a merge preview. Beyond those, there are mechanical conflicts in api.pb.go, parser_test.go, AlertMetadata.svelte, ScheduledReportDialog.svelte, en.json and es.json. The AlertMetadata.svelte conflict comes only from moving the m import, so leaving that import in place avoids it.
runtime/parser/schema/project.schema.yaml also needs updating. notify_properties still requires slack or email and doesn't list webhook, and there is no Webhook connector entry. That schema generates the reference pages for alerts, reports and connectors, and it is what the developer agent receives through JSONSchemaForResourceType. As it stands, the reference docs won't mention notify.webhook, and the agent is told that a webhook-only notify block is invalid. Please add both entries and run make docs.generate. While you're in the docs, linking the new page from the services index card list and from the "Notification Configuration" section in developers/build/alerts.md would make it discoverable.
On your question about notify.slack.webhooks: I'd prefer a separate PR. The same private-address guard and a request timeout make sense there, but that changes existing Slack behavior and deserves its own review and release note.
The failing Go test and e2e jobs are caused by secrets that aren't available to fork PRs (S3, GCS, Druid DSN and GCP auth), not by this change. The webhook, parser and admin server packages pass in that run.
Developed in collaboration with Claude Code
| repeated string slack_users = 10; | ||
| repeated string slack_channels = 11; | ||
| repeated string slack_webhooks = 12; | ||
| repeated string webhook_urls = 24; |
There was a problem hiding this comment.
main now has bool export_attach = 24; in ReportOptions (#10031). The three-way merge succeeds textually and leaves two fields numbered 24 in this message. Please renumber this field to 25; 19 is reserved, and 7 is better left unused.
There was a problem hiding this comment.
Renumbered to 25 in the rebase. I moved it in the commit that adds the field, so no commit in the branch has a duplicate number.
| repeated string slack_users = 9; | ||
| repeated string slack_channels = 10; | ||
| repeated string slack_webhooks = 11; | ||
| repeated string webhook_urls = 18; |
There was a problem hiding this comment.
Same issue in AlertOptions. main added bool export_attach = 18; in #10060, and 19 through 21 are also taken now. The next free number is 22.
There was a problem hiding this comment.
Renumbered to 22, also in the commit that adds the field.
| logger *zap.Logger | ||
| } | ||
|
|
||
| var _ drivers.Handle = &handle{} |
There was a problem hiding this comment.
After #10009, drivers.Handle requires AsMCP() (drivers.MCPService, bool), so this assertion and the return conn in Open fail to compile on a merge with main. The Slack driver's stub at runtime/drivers/slack/slack.go:166 on main is the pattern to copy.
There was a problem hiding this comment.
Added the stub in 5ca7852, following the Slack driver.
| // link-local (which includes cloud metadata endpoints such as 169.254.169.254) or unspecified. | ||
| func isPrivateAddress(ip netip.Addr) bool { | ||
| ip = ip.Unmap() | ||
| return ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() |
There was a problem hiding this comment.
IsPrivate only covers RFC 1918 and RFC 4193, so 100.64.0.0/10 (RFC 6598) and NAT64 64:ff9b::/96 pass the guard. I checked that 100.64.0.1, 100.100.100.200 (the Alibaba Cloud metadata address) and 64:ff9b::a00:1 all return false. Shared address space is a supported Pod and Service range on GKE and is common with Tailscale, so it would be good to block it too, and to unwrap NAT64 addresses to their embedded IPv4 before checking.
There was a problem hiding this comment.
Fixed in b955bcf. The guard now blocks 100.64.0.0/10, and an address under 64:ff9b::/96 is checked as its embedded IPv4, so 64:ff9b::a00:1 is treated as 10.0.0.1. NAT64 addresses of public IPv4s are still allowed, so receivers stay reachable from IPv6-only networks. The test covers your three addresses and the edges of the shared range.
| return nil, fmt.Errorf("webhook driver can't be shared") | ||
| } | ||
| conf := &configProperties{} | ||
| err := mapstructure.Decode(config, conf) |
There was a problem hiding this comment.
Connector variables always arrive as strings, so connector.webhook.allowed_url_prefixes=https://hooks.example.com/ makes Open fail with "source data must be an array or slice, got string", and every alert using the connector then fails in AcquireHandle. mapstructure.WeakDecode, which the https driver uses, turns a single string into a one-element slice.
There was a problem hiding this comment.
Fixed in 3f60fd0. Open now uses mapstructure.WeakDecode, and a test opens the connector with the prefix as a single string.
| } | ||
| host := canonicalHost(u) | ||
| for _, p := range prefixes { | ||
| if u.Scheme == p.scheme && host == p.host && strings.HasPrefix(u.Path, p.path) { |
There was a problem hiding this comment.
This prefix check isn't bounded at a path segment. With an entry of https://gw.example.com/hooks, both /hooks-admin/x and /hooksx are admitted and receive the static headers. Requiring the path to equal the entry's path or to continue with / would close this. Normalizing entries to a trailing slash in parseURLPrefixes would work as well.
There was a problem hiding this comment.
Fixed in 951ff9f. The path must now equal the entry's path or continue with /. I kept the entries as written instead of adding a trailing slash, so https://gw.example.com/hooks still admits /hooks itself. An entry that already ends with / works as before. The docs describe the rule.
| if err != nil { | ||
| return fmt.Errorf(`invalid value %q for property "notify.webhook.urls": %w`, u, err) | ||
| } | ||
| if parsed.Scheme != "http" && parsed.Scheme != "https" { |
There was a problem hiding this comment.
This accepts URLs without a host, such as https:///hook or https:hook.example.com. They parse successfully and then fail on every execution with "http: no Host in request URL". Checking parsed.Host != "" here would match the error message and surface the mistake at parse time.
There was a problem hiding this comment.
Fixed in b40b274. URLs without a host are now rejected at parse time, for alerts and reports.
| } | ||
| // Webhook settings | ||
| if len(tmp.Notify.Webhook.URLs) > 0 { | ||
| if err := validateWebhookURLs(tmp.Notify.Webhook.URLs); err != nil { |
There was a problem hiding this comment.
This repeats the validation that already ran at line 220, before insertResource, so it can be removed.
| import MetadataValue from "@rilldata/web-admin/features/scheduled-reports/metadata/MetadataValue.svelte"; | ||
| import { extractNotifier } from "@rilldata/web-admin/features/scheduled-reports/metadata/notifiers-utils"; | ||
| import { formatRefreshSchedule } from "@rilldata/web-admin/features/scheduled-reports/metadata/utils.ts"; | ||
| import { m } from "@rilldata/web-common/lib/i18n/gen/messages"; |
There was a problem hiding this comment.
Moving this import isn't needed for the feature, and it is the only reason this file conflicts with main, which changed the formatRefreshSchedule import above it. The same move happens in ReportMetadata.svelte.
There was a problem hiding this comment.
Done in the rebase. The m import stays where it was in both AlertMetadata.svelte and ReportMetadata.svelte.
| slackChannels: array().of(string()), | ||
| slackUsers: array().of(string().email(m.report_form_invalid_email())), | ||
| enableWebhookNotification: boolean(), | ||
| webhookUrls: array().of(string().url("Invalid URL")), |
There was a problem hiding this comment.
The slackUsers validator next to this one uses m.report_form_invalid_email(). Could this use a new report_form_invalid_url message, with its Spanish translation, so the Spanish locale doesn't show English?
There was a problem hiding this comment.
Added report_form_invalid_url with its Spanish translation in dbfb552. I left the alert form as it is, since its other validators in form-utils.ts (Invalid email, Required) are also plain English strings.
(cherry picked from commit 87406d7854fd20684392e467c8f2af021763c8d1)
Inert until the Spanish locale is registered in the inlang project settings (pending in rilldata#9635); included here so the two PRs compose cleanly in either merge order.
…scribes Unsubscribe deleted the resource once no email recipient or Slack target was left, without counting webhook URLs. Unsubscribing the last email recipient therefore deleted an alert or report that still delivered to a webhook.
Runtimes that predate the webhook notifier parse alerts and reports with
KnownFields(true), and the admin service and runtimes deploy independently.
Always emitting `webhook: {urls: []}` made those runtimes reject every alert
or report created, edited or unsubscribed after the admin deploy.
A receiver could stall delivery indefinitely: the default backoff honors a Retry-After header verbatim, and requests carried context.Background(), so `429` with `Retry-After: 86400` held the alert's reconcile for a day. URLs were also sent one after another with no shared deadline. - Cap the backoff, including Retry-After, at RetryWaitMax. - Deliver to up to 8 URLs at a time under one 60s deadline for the whole send. - Close idle connections at the end of each send. The reconcilers build a notifier per execution, so a client built once per notifier would not reuse connections either, and successful deliveries left idle connections open until the transport's idle timeout.
Anyone who can create an alert or report (every project viewer by default) chooses its webhook URLs, so the connector's static headers, typically an API gateway token, could be sent to any server, and the runtime would POST to internal services reachable from its network. - Add `allowed_url_prefixes` to the webhook connector, required whenever `headers` is set. The scheme and host must match exactly and the path must start with the entry's path, unlike a plain string prefix, which would let https://example.com admit https://example.com.evil.io. - Without an allowlist, block loopback, private and link-local destinations (including cloud metadata endpoints). The check runs on the dialed address, after DNS resolution, and the client connects directly instead of through an environment proxy, whose address it would check instead. Listing a receiver admits it, e.g. localhost during development or an internal receiver in a self-hosted deployment. - Don't follow redirects; a 3xx counts as a failed delivery. - Ping validates the connector config, so a missing allowlist surfaces on the connector instead of on the first delivery.
With logging disabled, retryablehttp's default error on exhausted retries only says how many attempts were made: a receiver answering 503 three times surfaced as "giving up after 3 attempt(s)". The execution's error is the only place a failed delivery shows up, so keep the final status or error in it.
929ab0d to
279efe2
Compare
|
Thanks for checking this against
One thing the rebase brought up that a textual merge would miss: #9972 added reports created from an embed, which Co-authored with Claude. |
Conflict resolutions: - admin/server/reports.go: keep main's embed-owner unsubscribe guard, then use the PR's reportYAML.unsubscribe helper. - runtime/parser/parser_test.go: keep both TestValidateWebhookURLs and main's TestTemplatesRejectProjectVariables. - BaseScheduledReportForm.svelte: keep main's embed owner email input; keep the PR's Slack-or-webhooks comment. - ScheduledReportDialog.svelte: count main's embed owner email as a recipient alongside the PR's webhook URLs. - api.proto: renumber ReportOptions.webhook_urls to 26, since main took 25 for embed_owner_email; regenerate. Fixes for main's changes: - Pass the new embedOwnerEmail argument to yamlForManagedReport in the PR's tests. - Add Korean translations for the webhook strings and update report_email_validation in ko.json. - Restore main's key order for report_email_validation in en.json and es.json.
|
Ready for another look when you have time. Everything from your review is addressed in the round from November 2, and the branch is current after your merge. The Slack hardening will go in its own PR, as you asked. |
Alerts and scheduled reports can currently notify via email and Slack only. Integrating
with anything else (PagerDuty, Opsgenie, n8n/Zapier/Make flows, or an in-house system)
requires abusing Slack incoming webhooks or polling the API.
This PR adds a generic
webhooknotifier driver, completing the connector-agnosticNotifiers {connector, properties}design introduced in #4371. Because that groundworkexists, the change is contained: no runtime proto changes and no new Go dependencies
(delivery uses
hashicorp/go-retryablehttp, already a direct dependency; signing is stdlibcrypto/hmac). The only proto change is two additivewebhook_urlsfields on the adminAPI's
AlertOptions/ReportOptionsfor the UI dialogs.YAML surface (alerts and reports)
Parity with the
slackblock: one entry appended toNotifiers, multiplicity insideproperties. The optional
connector:key selects a named connector instance (thereconciler already resolves notifiers by connector name), enabling per-receiver
secrets/headers without new machinery.
Connector config (
connector.<name>.*variables or a connector YAML; secret values go invariables, not in committed files):
signing_secret(secret),headers(map, e.g. anAuthorizationheader for receivers behind an API gateway, same pattern as thehttpsdriver) and
allowed_url_prefixes. Anyone who can create an alert chooses its URLs, soheadersrequiresallowed_url_prefixesand credentials only go to the listed receivers.Without an allowlist, loopback, private and link-local destinations are blocked, and
deliveries don't go through an environment proxy.
Payload
Versioned envelope, two event types mirroring the
Notifierinterface:{ "id": "unique-per-delivery", "type": "alert.status", // or "report.scheduled" "version": 1, "timestamp": "2026-07-02T15:04:05Z", "data": { "display_name": "…", "execution_time": "…", "status": "PASS | FAIL | ERROR", "is_recover": false, "fail_row": { "…": "…" }, "execution_error": "", "open_link": "…", "edit_link": "…" } }report.scheduledcarriesdisplay_name,report_time,download_format,summary,open_linkanddownload_link.fail_rowis included for parity with the email/Slacknotifiers;
ToEmail/ToNameare excluded (email-specific, per the #4371 review), and nounsubscribe link is sent (webhook receivers are anonymous).
Signing: Standard Webhooks
Signatures follow the Standard Webhooks spec exactly
(
webhook-id/webhook-timestamp/webhook-signatureheaders, HMAC-SHA256 over{id}.{timestamp}.{body},whsec_secret format), so receivers can verify with theexisting libraries in 11+ languages. Implemented with ~15 lines of stdlib code and
unit-tested against the spec's official test vectors. Without a
signing_secrettherequest is sent unsigned (capability-URL receivers like Zapier/n8n, same spirit as Slack
incoming webhooks working without a bot token). The connector's
Pingvalidates that thesecret is well-formed so misconfigurations surface early.
Delivery semantics
go-retryablehttp: 3 attempts, waiting 1s then 2s (Retry-Afteris honored, capped at4s), 10s per-attempt timeout; retries on 5xx except 501, 429 and network errors, hard-fails
on other 4xx. Success = any 2xx. Redirects are not followed.
notification.
errors.Joinand surface asthe execution's
ERRORresult. Since executions are the only delivery-visibilitysurface, each per-URL error is self-sufficient (URL + status + attempts).
scope for a notifier driver. The stable
idlets receivers deduplicate.UI
Mirrors the Slack pattern: a "Webhook notifications" section (toggle + URL multi-input) in
the alert and report create/edit dialogs, gated on
ListNotifierConnectorsreporting aconfigured
webhookconnector (with a "not configured" docs hint otherwise), and thedestinations listed on the alert/report detail pages. The
connector:key stays YAML-only,like named Slack instances today. New strings use paraglide messages in
en.json, with theirSpanish translations in
es.json.Tested
Unit tests for the driver (signing vectors, retry policy,
Retry-Aftercap, deliverydeadline, concurrency cap, final status in errors, per-URL error aggregation, dedupe,
unsigned mode, URL allowlist, private-address guard, proxy, redirects, Ping), the parser
(alerts, reports, URL validation, default and named connectors) and the admin YAML
(
notify.webhookomitted when empty, unsubscribe).Before the review, exercised end-to-end on our self-hosted deployment: alert and report
created through the dialogs,
alert.statusandreport.scheduled(ad hoc and cron)delivered and signature-verified against a local receiver. After the review changes, the
allowlist, the private-address guard and the headers check were exercised with
rill startagainst local receivers.
Open question
Docs: added as
docs/docs/developers/build/connectors/services/webhook.md. Should thepayload JSON schema be published more formally?