From d4e56f1e49bf622e6e6979fe800a165b6d314bcf Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Fri, 17 Jul 2026 16:17:07 +0530 Subject: [PATCH 01/15] feat(reconcile): add a Webhook kind --- cmd/reconcile.go | 6 +- internal/reconcile/webhook.go | 191 ++++++++++++++++++ internal/reconcile/webhook_reconciler.go | 165 +++++++++++++++ internal/reconcile/webhook_reconciler_test.go | 176 ++++++++++++++++ internal/reconcile/webhook_test.go | 158 +++++++++++++++ 5 files changed, 694 insertions(+), 2 deletions(-) create mode 100644 internal/reconcile/webhook.go create mode 100644 internal/reconcile/webhook_reconciler.go create mode 100644 internal/reconcile/webhook_reconciler_test.go create mode 100644 internal/reconcile/webhook_test.go diff --git a/cmd/reconcile.go b/cmd/reconcile.go index eee6f8fe6..e89fc4f39 100644 --- a/cmd/reconcile.go +++ b/cmd/reconcile.go @@ -23,8 +23,9 @@ func ReconcileCommand(cliConfig *Config) *cli.Command { Make platform resources match a desired-state YAML file, through the admin API. Kinds: PlatformUser (platform admins and members), Permission (custom - permissions), Role (platform-level roles), and Preference (platform - settings). Deleting a permission or a custom role needs an explicit + permissions), Role (platform-level roles), Preference (platform + settings), and Webhook (webhook endpoints). Deleting a permission, a + custom role, or a webhook needs an explicit 'delete: true' on its entry; nothing is deleted by omission, and a predefined role cannot be deleted. A preference left out of the file resets to its default. Log in as a superuser (for example the bootstrap service account) @@ -88,6 +89,7 @@ func buildReconcileRegistry(host, header string) (map[string]reconcile.Reconcile reconcile.KindPermission: reconcile.NewPermissionReconciler(api, header), reconcile.KindRole: reconcile.NewRoleReconciler(api, header), reconcile.KindPreference: reconcile.NewPreferenceReconciler(api, header), + reconcile.KindWebhook: reconcile.NewWebhookReconciler(adminClient, header), }, nil } diff --git a/internal/reconcile/webhook.go b/internal/reconcile/webhook.go new file mode 100644 index 000000000..c03e728d9 --- /dev/null +++ b/internal/reconcile/webhook.go @@ -0,0 +1,191 @@ +package reconcile + +import ( + "fmt" + "net/url" + "sort" + "strings" +) + +// KindWebhook is the desired-state document kind for webhook endpoints. +const KindWebhook = "Webhook" + +// Webhook states. The server enables a new endpoint by default, so an entry +// that does not set a state leaves the server's value in place. +const ( + webhookStateEnabled = "enabled" + webhookStateDisabled = "disabled" +) + +// WebhookSpec is one desired webhook endpoint. The URL is the identity and +// never changes. Description, subscribed events, and state are managed: a field +// that is present is set, and a field that is omitted keeps the server value. +// Signing secrets are server-owned — the server generates one on create and +// never returns it on read — so they are not part of the spec. +type WebhookSpec struct { + URL string `yaml:"url"` + Description string `yaml:"description,omitempty"` + SubscribedEvents []string `yaml:"subscribed_events,omitempty"` + State string `yaml:"state,omitempty"` + Delete bool `yaml:"delete,omitempty"` +} + +// currentWebhook is one endpoint as returned by ListWebhooks. Headers and +// metadata are carried through an update untouched, so a reconcile that only +// changes a managed field does not wipe values an operator set elsewhere. +// Secrets are never read: the server redacts them on list. +type currentWebhook struct { + ID string + URL string + Description string + SubscribedEvents []string + State string + Headers map[string]string + Metadata map[string]any +} + +// webhookOp is a single planned change. spec carries the final values to send; +// id is set for updates and deletes; headers and metadata are the endpoint's +// current values, carried through an update untouched. +type webhookOp struct { + action opAction + spec WebhookSpec + id string + detail string + headers map[string]string + metadata map[string]any +} + +func (o webhookOp) String() string { + switch o.action { + case opRemove: + return fmt.Sprintf("delete webhook %s", o.spec.URL) + case opUpdate: + return fmt.Sprintf("update webhook %s (%s)", o.spec.URL, o.detail) + default: + return fmt.Sprintf("add webhook %s [%s]", o.spec.URL, strings.Join(o.spec.SubscribedEvents, ", ")) + } +} + +// validateWebhookSpec rejects entries the flow cannot manage. A delete entry +// needs only a valid URL; everything else must name the events it subscribes to. +func validateWebhookSpec(s WebhookSpec) error { + if strings.TrimSpace(s.URL) == "" { + return fmt.Errorf("url is required") + } + if u, err := url.Parse(s.URL); err != nil || !u.IsAbs() { + return fmt.Errorf("url %q must be a valid absolute URL", s.URL) + } + if s.Delete { + return nil + } + if len(s.SubscribedEvents) == 0 { + return fmt.Errorf("a webhook must subscribe to at least one event") + } + if s.State != "" && s.State != webhookStateEnabled && s.State != webhookStateDisabled { + return fmt.Errorf("state must be %q or %q", webhookStateEnabled, webhookStateDisabled) + } + return nil +} + +// diffWebhooks returns the ops that make the current webhook endpoints match the +// desired spec. The URL is the identity: every endpoint on the server must +// appear in the file — kept, or marked delete — so nothing is removed by +// omission, and an unaccounted endpoint fails the plan. Because the server does +// not enforce URL uniqueness, two endpoints sharing a URL make the identity +// ambiguous and also fail the plan, naming the ids to clean up. +func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, error) { + idsByURL := map[string][]string{} + for _, c := range current { + idsByURL[c.URL] = append(idsByURL[c.URL], c.ID) + } + var ambiguous []string + for u, ids := range idsByURL { + if len(ids) > 1 { + sort.Strings(ids) + ambiguous = append(ambiguous, fmt.Sprintf("%s (ids: %s)", u, strings.Join(ids, ", "))) + } + } + if len(ambiguous) > 0 { + sort.Strings(ambiguous) + return nil, fmt.Errorf("the server has webhooks that share a url, so the url identity is ambiguous: %s; delete the extra one by hand, then reconcile", strings.Join(ambiguous, "; ")) + } + + byURL := make(map[string]currentWebhook, len(current)) + for _, c := range current { + byURL[c.URL] = c + } + + seen := map[string]struct{}{} + var adds, updates, removes []webhookOp + for _, s := range desired { + if err := validateWebhookSpec(s); err != nil { + return nil, fmt.Errorf("invalid webhook spec %q: %w", s.URL, err) + } + if _, dup := seen[s.URL]; dup { + return nil, fmt.Errorf("webhook %q is listed more than once", s.URL) + } + seen[s.URL] = struct{}{} + + cur, exists := byURL[s.URL] + if s.Delete { + if exists { + removes = append(removes, webhookOp{action: opRemove, spec: s, id: cur.ID}) + } + continue + } + if !exists { + adds = append(adds, webhookOp{action: opAdd, spec: WebhookSpec{ + URL: s.URL, + Description: s.Description, + SubscribedEvents: sortedCopy(s.SubscribedEvents), + State: s.State, + }}) + continue + } + + merged := WebhookSpec{ + URL: s.URL, + Description: cur.Description, + SubscribedEvents: sortedCopy(cur.SubscribedEvents), + State: cur.State, + } + var changes []string + if s.Description != "" && s.Description != cur.Description { + merged.Description = s.Description + changes = append(changes, "description") + } + if !stringSetsEqual(sortedCopy(s.SubscribedEvents), sortedCopy(cur.SubscribedEvents)) { + merged.SubscribedEvents = sortedCopy(s.SubscribedEvents) + changes = append(changes, "subscribed_events") + } + if s.State != "" && s.State != cur.State { + merged.State = s.State + changes = append(changes, "state") + } + if len(changes) > 0 { + updates = append(updates, webhookOp{ + action: opUpdate, + spec: merged, + id: cur.ID, + detail: strings.Join(changes, ", "), + headers: cur.Headers, + metadata: cur.Metadata, + }) + } + } + + var unaccounted []string + for u := range byURL { + if _, ok := seen[u]; !ok { + unaccounted = append(unaccounted, u) + } + } + if len(unaccounted) > 0 { + sort.Strings(unaccounted) + return nil, fmt.Errorf("webhooks exist on the server but are not in the file: %s; keep them or mark them 'delete: true'", strings.Join(unaccounted, ", ")) + } + + ops := append(adds, updates...) + return append(ops, removes...), nil +} diff --git a/internal/reconcile/webhook_reconciler.go b/internal/reconcile/webhook_reconciler.go new file mode 100644 index 000000000..cfb5e57de --- /dev/null +++ b/internal/reconcile/webhook_reconciler.go @@ -0,0 +1,165 @@ +package reconcile + +import ( + "context" + "fmt" + "sort" + + "connectrpc.com/connect" + frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" + "google.golang.org/protobuf/types/known/structpb" +) + +// WebhookAPI is the API subset the webhook reconciler needs. Every webhook +// operation lives on the admin service. +type WebhookAPI interface { + ListWebhooks(context.Context, *connect.Request[frontierv1beta1.ListWebhooksRequest]) (*connect.Response[frontierv1beta1.ListWebhooksResponse], error) + CreateWebhook(context.Context, *connect.Request[frontierv1beta1.CreateWebhookRequest]) (*connect.Response[frontierv1beta1.CreateWebhookResponse], error) + UpdateWebhook(context.Context, *connect.Request[frontierv1beta1.UpdateWebhookRequest]) (*connect.Response[frontierv1beta1.UpdateWebhookResponse], error) + DeleteWebhook(context.Context, *connect.Request[frontierv1beta1.DeleteWebhookRequest]) (*connect.Response[frontierv1beta1.DeleteWebhookResponse], error) +} + +// WebhookReconciler makes webhook endpoints match the desired spec. The URL is +// the identity; description, subscribed events, and state are the managed +// fields. A missing endpoint fails the plan, and deletion needs an explicit +// delete flag. +type WebhookReconciler struct { + client WebhookAPI + header string +} + +func NewWebhookReconciler(client WebhookAPI, header string) *WebhookReconciler { + return &WebhookReconciler{client: client, header: header} +} + +func (r *WebhookReconciler) Kind() string { return KindWebhook } + +func (r *WebhookReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { + var specs []WebhookSpec + if err := decodeSpec(spec, &specs); err != nil { + return Report{}, fmt.Errorf("parse %s spec: %w", KindWebhook, err) + } + + current, err := r.fetchCurrent(ctx) + if err != nil { + return Report{}, err + } + + ops, err := diffWebhooks(specs, current) + if err != nil { + return Report{}, err + } + + rep := Report{Kind: KindWebhook, DryRun: dryRun} + for _, op := range ops { + rep.Planned = append(rep.Planned, op.String()) + } + if dryRun { + return rep, nil + } + for _, op := range ops { + if err := r.apply(ctx, op); err != nil { + return rep, fmt.Errorf("apply [%s]: %w", op, err) + } + rep.Applied++ + } + return rep, nil +} + +// Export returns the current webhook endpoints as a desired-state spec, sorted +// by url. State is written only when it is not the default ("enabled") and an +// empty description is left out, so an omitted field on reconcile keeps the +// server value and the export round-trips to no changes. Secrets are never read +// from the server, so they can never appear in an export. +func (r *WebhookReconciler) Export(ctx context.Context) (any, error) { + current, err := r.fetchCurrent(ctx) + if err != nil { + return nil, err + } + sort.Slice(current, func(i, j int) bool { return current[i].URL < current[j].URL }) + + specs := make([]WebhookSpec, 0, len(current)) + for _, c := range current { + entry := WebhookSpec{ + URL: c.URL, + Description: c.Description, + SubscribedEvents: sortedCopy(c.SubscribedEvents), + } + if c.State != "" && c.State != webhookStateEnabled { + entry.State = c.State + } + specs = append(specs, entry) + } + return specs, nil +} + +func (r *WebhookReconciler) fetchCurrent(ctx context.Context) ([]currentWebhook, error) { + resp, err := r.client.ListWebhooks(ctx, authReq(&frontierv1beta1.ListWebhooksRequest{}, r.header)) + if err != nil { + return nil, fmt.Errorf("list webhooks: %w", err) + } + var current []currentWebhook + for _, w := range resp.Msg.GetWebhooks() { + var md map[string]any + if m := w.GetMetadata(); m != nil { + md = m.AsMap() + } + current = append(current, currentWebhook{ + ID: w.GetId(), + URL: w.GetUrl(), + Description: w.GetDescription(), + SubscribedEvents: w.GetSubscribedEvents(), + State: w.GetState(), + Headers: w.GetHeaders(), + Metadata: md, + }) + } + return current, nil +} + +func (r *WebhookReconciler) apply(ctx context.Context, op webhookOp) error { + switch op.action { + case opAdd: + body, err := webhookBody(op.spec, nil, nil) + if err != nil { + return err + } + _, err = r.client.CreateWebhook(ctx, authReq(&frontierv1beta1.CreateWebhookRequest{Body: body}, r.header)) + return err + case opUpdate: + body, err := webhookBody(op.spec, op.headers, op.metadata) + if err != nil { + return err + } + _, err = r.client.UpdateWebhook(ctx, authReq(&frontierv1beta1.UpdateWebhookRequest{Id: op.id, Body: body}, r.header)) + return err + case opRemove: + _, err := r.client.DeleteWebhook(ctx, authReq(&frontierv1beta1.DeleteWebhookRequest{Id: op.id}, r.header)) + return err + default: + return fmt.Errorf("unknown op action %q", op.action) + } +} + +// webhookBody builds the request body for an add or update. On an update, the +// endpoint's current headers and metadata are carried through so an update that +// only changes a managed field does not wipe values set elsewhere. The signing +// secret is never sent: the server owns it and generates one on create. +func webhookBody(spec WebhookSpec, headers map[string]string, metadata map[string]any) (*frontierv1beta1.WebhookRequestBody, error) { + var md *structpb.Struct + if len(metadata) > 0 { + var err error + md, err = structpb.NewStruct(metadata) + if err != nil { + return nil, fmt.Errorf("build webhook metadata: %w", err) + } + } + return &frontierv1beta1.WebhookRequestBody{ + Url: spec.URL, + Description: spec.Description, + SubscribedEvents: spec.SubscribedEvents, + State: spec.State, + Headers: headers, + Metadata: md, + }, nil +} diff --git a/internal/reconcile/webhook_reconciler_test.go b/internal/reconcile/webhook_reconciler_test.go new file mode 100644 index 000000000..e414880c0 --- /dev/null +++ b/internal/reconcile/webhook_reconciler_test.go @@ -0,0 +1,176 @@ +package reconcile + +import ( + "context" + "errors" + "testing" + + "connectrpc.com/connect" + frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" + "github.com/stretchr/testify/assert" + "google.golang.org/protobuf/types/known/structpb" +) + +type fakeWebhookAPI struct { + webhooks []*frontierv1beta1.Webhook + created []*frontierv1beta1.WebhookRequestBody + updated map[string]*frontierv1beta1.WebhookRequestBody + deleted []string + createErr error +} + +func (f *fakeWebhookAPI) ListWebhooks(_ context.Context, _ *connect.Request[frontierv1beta1.ListWebhooksRequest]) (*connect.Response[frontierv1beta1.ListWebhooksResponse], error) { + return connect.NewResponse(&frontierv1beta1.ListWebhooksResponse{Webhooks: f.webhooks}), nil +} + +func (f *fakeWebhookAPI) CreateWebhook(_ context.Context, req *connect.Request[frontierv1beta1.CreateWebhookRequest]) (*connect.Response[frontierv1beta1.CreateWebhookResponse], error) { + if f.createErr != nil { + return nil, f.createErr + } + f.created = append(f.created, req.Msg.GetBody()) + return connect.NewResponse(&frontierv1beta1.CreateWebhookResponse{}), nil +} + +func (f *fakeWebhookAPI) UpdateWebhook(_ context.Context, req *connect.Request[frontierv1beta1.UpdateWebhookRequest]) (*connect.Response[frontierv1beta1.UpdateWebhookResponse], error) { + if f.updated == nil { + f.updated = map[string]*frontierv1beta1.WebhookRequestBody{} + } + f.updated[req.Msg.GetId()] = req.Msg.GetBody() + return connect.NewResponse(&frontierv1beta1.UpdateWebhookResponse{}), nil +} + +func (f *fakeWebhookAPI) DeleteWebhook(_ context.Context, req *connect.Request[frontierv1beta1.DeleteWebhookRequest]) (*connect.Response[frontierv1beta1.DeleteWebhookResponse], error) { + f.deleted = append(f.deleted, req.Msg.GetId()) + return connect.NewResponse(&frontierv1beta1.DeleteWebhookResponse{}), nil +} + +func webhookPB(id, url, desc, state string, events ...string) *frontierv1beta1.Webhook { + return &frontierv1beta1.Webhook{Id: id, Url: url, Description: desc, State: state, SubscribedEvents: events} +} + +func TestWebhookReconciler(t *testing.T) { + t.Run("applies add, update, and delete", func(t *testing.T) { + api := &fakeWebhookAPI{webhooks: []*frontierv1beta1.Webhook{ + webhookPB("w1", "https://keep.example/hook", "old", webhookStateEnabled, "org.created"), + webhookPB("w2", "https://gone.example/hook", "", webhookStateEnabled, "org.created"), + }} + spec := []byte(` +- {url: "https://new.example/hook", subscribed_events: [org.created]} +- {url: "https://keep.example/hook", description: updated, subscribed_events: [org.created]} +- {url: "https://gone.example/hook", delete: true} +`) + rep, err := NewWebhookReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, 3, rep.Applied) + if assert.Len(t, api.created, 1) { + assert.Equal(t, "https://new.example/hook", api.created[0].GetUrl()) + assert.Equal(t, []string{"org.created"}, api.created[0].GetSubscribedEvents()) + } + if body := api.updated["w1"]; assert.NotNil(t, body) { + assert.Equal(t, "https://keep.example/hook", body.GetUrl()) // identity never changes + assert.Equal(t, "updated", body.GetDescription()) + } + assert.Equal(t, []string{"w2"}, api.deleted) + }) + + t.Run("an update preserves headers and metadata it does not manage", func(t *testing.T) { + md, _ := structpb.NewStruct(map[string]any{"team": "platform"}) + wh := webhookPB("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created") + wh.Headers = map[string]string{"X-Token": "abc"} + wh.Metadata = md + api := &fakeWebhookAPI{webhooks: []*frontierv1beta1.Webhook{wh}} + // only the event set changes; headers and metadata must survive the update + spec := []byte("- {url: \"https://a.example/hook\", subscribed_events: [org.created, org.deleted]}\n") + + rep, err := NewWebhookReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, 1, rep.Applied) + if body := api.updated["w1"]; assert.NotNil(t, body) { + assert.Equal(t, map[string]string{"X-Token": "abc"}, body.GetHeaders()) + assert.Equal(t, "platform", body.GetMetadata().GetFields()["team"].GetStringValue()) + } + }) + + t.Run("a dry run plans without applying", func(t *testing.T) { + api := &fakeWebhookAPI{} + spec := []byte("- {url: \"https://a.example/hook\", subscribed_events: [org.created]}\n") + + rep, err := NewWebhookReconciler(api, "").Reconcile(context.Background(), spec, true) + + assert.NoError(t, err) + assert.Equal(t, []string{"add webhook https://a.example/hook [org.created]"}, rep.Planned) + assert.Zero(t, rep.Applied) + assert.Empty(t, api.created) + }) + + t.Run("an unknown field in the spec fails the plan", func(t *testing.T) { + api := &fakeWebhookAPI{} + // `delet` instead of `delete`: must fail, not silently ignore it + spec := []byte("- {url: \"https://a.example/hook\", subscribed_events: [org.created], delet: true}\n") + + _, err := NewWebhookReconciler(api, "").Reconcile(context.Background(), spec, true) + + assert.ErrorContains(t, err, "parse Webhook spec") + assert.Empty(t, api.created) + }) + + t.Run("a create rejected by the server stops the run with the op named", func(t *testing.T) { + api := &fakeWebhookAPI{createErr: connect.NewError(connect.CodeInvalidArgument, errors.New("bad request"))} + spec := []byte("- {url: \"https://a.example/hook\", subscribed_events: [org.created]}\n") + + rep, err := NewWebhookReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.ErrorContains(t, err, "apply [add webhook https://a.example/hook [org.created]]") + assert.Zero(t, rep.Applied) + assert.Equal(t, []string{"add webhook https://a.example/hook [org.created]"}, rep.Planned) + }) + + t.Run("export round-trips to no changes and never leaks secrets", func(t *testing.T) { + wh := webhookPB("w1", "https://a.example/hook", "desc", webhookStateDisabled, "org.created", "org.deleted") + // even if a secret leaked into the response, the exporter must not carry it: + // the spec has no secret field at all. + wh.Secrets = []*frontierv1beta1.Webhook_Secret{{Id: "1", Value: "supersecret"}} + api := &fakeWebhookAPI{webhooks: []*frontierv1beta1.Webhook{wh}} + registry := map[string]Reconciler{KindWebhook: NewWebhookReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindWebhook) + assert.NoError(t, err) + assert.Contains(t, string(out), "https://a.example/hook") + assert.Contains(t, string(out), "disabled") // a non-default state is written + assert.NotContains(t, string(out), "supersecret") + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("export omits the default state and round-trips", func(t *testing.T) { + api := &fakeWebhookAPI{webhooks: []*frontierv1beta1.Webhook{ + webhookPB("w1", "https://a.example/hook", "", webhookStateEnabled, "org.created"), + }} + registry := map[string]Reconciler{KindWebhook: NewWebhookReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindWebhook) + assert.NoError(t, err) + assert.NotContains(t, string(out), "state:") // enabled is the default, so it is left out + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("export with no endpoints yields an empty list", func(t *testing.T) { + api := &fakeWebhookAPI{} + registry := map[string]Reconciler{KindWebhook: NewWebhookReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindWebhook) + assert.NoError(t, err) + assert.Contains(t, string(out), "spec: []") + }) +} diff --git a/internal/reconcile/webhook_test.go b/internal/reconcile/webhook_test.go new file mode 100644 index 000000000..a0a5d5b7e --- /dev/null +++ b/internal/reconcile/webhook_test.go @@ -0,0 +1,158 @@ +package reconcile + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func cw(id, url, desc, state string, events ...string) currentWebhook { + return currentWebhook{ID: id, URL: url, Description: desc, State: state, SubscribedEvents: events} +} + +func TestDiffWebhooks(t *testing.T) { + t.Run("adds a new endpoint", func(t *testing.T) { + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", SubscribedEvents: []string{"org.created"}}}, + nil, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, opAdd, ops[0].action) + assert.Equal(t, "add webhook https://a.example/hook [org.created]", ops[0].String()) + } + }) + + t.Run("updates only the changed managed fields, matched by url", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "old", webhookStateEnabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "new", SubscribedEvents: []string{"org.deleted", "org.created"}}}, + current, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, opUpdate, ops[0].action) + assert.Equal(t, "w1", ops[0].id) + assert.Equal(t, "update webhook https://a.example/hook (description, subscribed_events)", ops[0].String()) + assert.Equal(t, "new", ops[0].spec.Description) + assert.Equal(t, []string{"org.created", "org.deleted"}, ops[0].spec.SubscribedEvents) + } + }) + + t.Run("a converged endpoint plans no change", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "desc", SubscribedEvents: []string{"org.created"}}}, + current, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("an omitted state keeps the server value", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateDisabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "desc", SubscribedEvents: []string{"org.created"}}}, + current, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("state change is planned when listed and different", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "desc", SubscribedEvents: []string{"org.created"}, State: webhookStateDisabled}}, + current, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update webhook https://a.example/hook (state)", ops[0].String()) + assert.Equal(t, webhookStateDisabled, ops[0].spec.State) + } + }) + + t.Run("delete removes a listed endpoint", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "", webhookStateEnabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Delete: true}}, + current, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, opRemove, ops[0].action) + assert.Equal(t, "w1", ops[0].id) + assert.Equal(t, "delete webhook https://a.example/hook", ops[0].String()) + } + }) + + t.Run("deleting an endpoint that is not on the server is a no-op", func(t *testing.T) { + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://gone.example/hook", Delete: true}}, + nil, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("an endpoint on the server but not in the file fails the plan", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "", webhookStateEnabled, "org.created")} + _, err := diffWebhooks([]WebhookSpec{}, current) + assert.ErrorContains(t, err, "webhooks exist on the server but are not in the file") + assert.ErrorContains(t, err, "https://a.example/hook") + }) + + t.Run("adds and updates run before removes", func(t *testing.T) { + current := []currentWebhook{ + cw("w1", "https://update.example/hook", "old", webhookStateEnabled, "org.created"), + cw("w2", "https://delete.example/hook", "", webhookStateEnabled, "org.created"), + } + ops, err := diffWebhooks([]WebhookSpec{ + {URL: "https://add.example/hook", SubscribedEvents: []string{"org.created"}}, + {URL: "https://update.example/hook", Description: "new", SubscribedEvents: []string{"org.created"}}, + {URL: "https://delete.example/hook", Delete: true}, + }, current) + assert.NoError(t, err) + if assert.Len(t, ops, 3) { + assert.Equal(t, opAdd, ops[0].action) + assert.Equal(t, opUpdate, ops[1].action) + assert.Equal(t, opRemove, ops[2].action) + } + }) + + t.Run("a url listed twice in the file fails", func(t *testing.T) { + _, err := diffWebhooks([]WebhookSpec{ + {URL: "https://a.example/hook", SubscribedEvents: []string{"org.created"}}, + {URL: "https://a.example/hook", SubscribedEvents: []string{"org.deleted"}}, + }, nil) + assert.ErrorContains(t, err, "listed more than once") + }) + + t.Run("two server endpoints sharing a url fail with their ids", func(t *testing.T) { + current := []currentWebhook{ + cw("w1", "https://a.example/hook", "", webhookStateEnabled, "org.created"), + cw("w2", "https://a.example/hook", "", webhookStateEnabled, "org.created"), + } + _, err := diffWebhooks([]WebhookSpec{ + {URL: "https://a.example/hook", SubscribedEvents: []string{"org.created"}}, + }, current) + assert.ErrorContains(t, err, "ambiguous") + assert.ErrorContains(t, err, "w1") + assert.ErrorContains(t, err, "w2") + }) + + t.Run("validation rejects a url that is not absolute", func(t *testing.T) { + _, err := diffWebhooks([]WebhookSpec{{URL: "not-a-url", SubscribedEvents: []string{"org.created"}}}, nil) + assert.ErrorContains(t, err, "absolute URL") + }) + + t.Run("validation rejects an entry with no events", func(t *testing.T) { + _, err := diffWebhooks([]WebhookSpec{{URL: "https://a.example/hook"}}, nil) + assert.ErrorContains(t, err, "at least one event") + }) + + t.Run("validation rejects an unknown state", func(t *testing.T) { + _, err := diffWebhooks([]WebhookSpec{{URL: "https://a.example/hook", SubscribedEvents: []string{"org.created"}, State: "paused"}}, nil) + assert.ErrorContains(t, err, "state must be") + }) +} From 03118fe660ccea50ab21d6a80dce79e274b38e31 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Fri, 17 Jul 2026 16:17:07 +0530 Subject: [PATCH 02/15] docs(reconcile): document the Webhook kind --- docs/content/docs/reconcile.mdx | 35 ++++++++++++++++++++++++++++- docs/content/docs/reference/cli.mdx | 11 ++++----- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/docs/content/docs/reconcile.mdx b/docs/content/docs/reconcile.mdx index b37de5ed2..754a66b4c 100644 --- a/docs/content/docs/reconcile.mdx +++ b/docs/content/docs/reconcile.mdx @@ -194,6 +194,39 @@ spec: - Export writes only the preferences whose value differs from the default, so settings at their default stay out of the file. +## The Webhook kind + +`Webhook` manages webhook endpoints: a URL, the events it subscribes to, and whether it is +enabled. The URL is the identity and never changes. + +```yaml +apiVersion: v1 +kind: Webhook +spec: + - url: https://hooks.example.org/frontier + description: Ops notifications + subscribed_events: + - app.user.created + - app.group.created + state: enabled + - url: https://old.example.org/frontier + delete: true +``` + +- The URL must be a valid absolute URL, and it is the identity. If two endpoints on the + server share a URL, the identity is ambiguous: the plan fails and names the ids so you can + remove the extra one by hand. +- `subscribed_events` is required for a live entry and compares as a set. `description` and + `state` (`enabled` or `disabled`) are managed too; a field you leave out keeps its server + value. +- Every endpoint on the server must appear in the file, kept or marked `delete: true`. One + that is missing fails the plan; nothing is deleted just because it is missing. +- The signing secret is server-owned. The server generates it when the endpoint is created + and never returns it on read, so it is not part of the file, never shows up in a plan, and + can never appear in an export. +- Export leaves out `state` when it is the default `enabled`, and headers and metadata set + through other tools are carried through an update untouched. + ## Running it Log in as a superuser. The bootstrap service user exists for exactly this; its client id @@ -236,7 +269,7 @@ The kind argument is case-insensitive and accepts a plural, so `platformuser` an ## More kinds -This page covers `PlatformUser`, `Permission`, `Role`, and `Preference`. The design and +This page covers `PlatformUser`, `Permission`, `Role`, `Preference`, and `Webhook`. The design and the rules every kind follows live in [RFC 0001](https://github.com/raystack/frontier/blob/main/docs/rfcs/0001-declarative-reconcile.md), which also lists the kinds proposed next. The flag reference for both commands is in the diff --git a/docs/content/docs/reference/cli.mdx b/docs/content/docs/reference/cli.mdx index f57048dff..0a101b572 100644 --- a/docs/content/docs/reference/cli.mdx +++ b/docs/content/docs/reference/cli.mdx @@ -34,7 +34,7 @@ List of supported environment variables Export the current state of a kind as a desired-state YAML file, printed to stdout. The output is the format `frontier reconcile` reads: reconciling it changes nothing. Supported kinds: `PlatformUser`, `Permission`, `Role`, -`Preference`. See the +`Preference`, `Webhook`. See the [Reconcile guide](../reconcile.md) for the file format and the flow. ``` @@ -249,10 +249,11 @@ View a project Make platform resources match a desired-state YAML file, through the admin API. Supported kinds: `PlatformUser` (anyone listed is added, anyone not listed is removed), `Permission` (custom permissions), `Role` -(platform-level roles), and `Preference` (platform settings, where a setting -left out of the file resets to its default). Deleting a permission or a custom -role needs an explicit `delete: true` on its entry; nothing is deleted by -omission, and a predefined role cannot be deleted. Use +(platform-level roles), `Preference` (platform settings, where a setting +left out of the file resets to its default), and `Webhook` (webhook endpoints). +Deleting a permission, a custom role, or a webhook needs an explicit +`delete: true` on its entry; nothing is deleted by omission, and a predefined +role cannot be deleted. Use `frontier export` to print the current state in this file format, and see the [Reconcile guide](../reconcile.md) for the full flow. From e772c6d024ea0ee751f51f354dce03c93f04648d Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Sat, 18 Jul 2026 13:40:09 +0530 Subject: [PATCH 03/15] feat(reconcile): make webhook subscribed_events optional, empty means all events --- internal/reconcile/webhook.go | 24 +++++++----- internal/reconcile/webhook_reconciler_test.go | 30 +++++++++++++++ internal/reconcile/webhook_test.go | 37 +++++++++++++++++-- 3 files changed, 79 insertions(+), 12 deletions(-) diff --git a/internal/reconcile/webhook.go b/internal/reconcile/webhook.go index c03e728d9..498a6313f 100644 --- a/internal/reconcile/webhook.go +++ b/internal/reconcile/webhook.go @@ -20,8 +20,9 @@ const ( // WebhookSpec is one desired webhook endpoint. The URL is the identity and // never changes. Description, subscribed events, and state are managed: a field // that is present is set, and a field that is omitted keeps the server value. -// Signing secrets are server-owned — the server generates one on create and -// never returns it on read — so they are not part of the spec. +// An empty event list means the endpoint receives every event, which is the +// server's own default. Signing secrets are server-owned: the server generates +// one on create and never returns it on read, so they are not part of the spec. type WebhookSpec struct { URL string `yaml:"url"` Description string `yaml:"description,omitempty"` @@ -63,12 +64,17 @@ func (o webhookOp) String() string { case opUpdate: return fmt.Sprintf("update webhook %s (%s)", o.spec.URL, o.detail) default: - return fmt.Sprintf("add webhook %s [%s]", o.spec.URL, strings.Join(o.spec.SubscribedEvents, ", ")) + events := "all events" + if len(o.spec.SubscribedEvents) > 0 { + events = strings.Join(o.spec.SubscribedEvents, ", ") + } + return fmt.Sprintf("add webhook %s [%s]", o.spec.URL, events) } } -// validateWebhookSpec rejects entries the flow cannot manage. A delete entry -// needs only a valid URL; everything else must name the events it subscribes to. +// validateWebhookSpec rejects entries the flow cannot manage. A live entry needs +// only a valid URL: an empty event list is allowed and means the endpoint +// receives every event, which is the server's own default. func validateWebhookSpec(s WebhookSpec) error { if strings.TrimSpace(s.URL) == "" { return fmt.Errorf("url is required") @@ -79,9 +85,6 @@ func validateWebhookSpec(s WebhookSpec) error { if s.Delete { return nil } - if len(s.SubscribedEvents) == 0 { - return fmt.Errorf("a webhook must subscribe to at least one event") - } if s.State != "" && s.State != webhookStateEnabled && s.State != webhookStateDisabled { return fmt.Errorf("state must be %q or %q", webhookStateEnabled, webhookStateDisabled) } @@ -155,7 +158,10 @@ func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, merged.Description = s.Description changes = append(changes, "description") } - if !stringSetsEqual(sortedCopy(s.SubscribedEvents), sortedCopy(cur.SubscribedEvents)) { + // Events are managed only when the entry lists them. An omitted list + // (nil) keeps the server's set; an explicit empty list ([]) sets the + // endpoint to receive every event. + if s.SubscribedEvents != nil && !stringSetsEqual(sortedCopy(s.SubscribedEvents), sortedCopy(cur.SubscribedEvents)) { merged.SubscribedEvents = sortedCopy(s.SubscribedEvents) changes = append(changes, "subscribed_events") } diff --git a/internal/reconcile/webhook_reconciler_test.go b/internal/reconcile/webhook_reconciler_test.go index e414880c0..b62a8cf02 100644 --- a/internal/reconcile/webhook_reconciler_test.go +++ b/internal/reconcile/webhook_reconciler_test.go @@ -93,6 +93,36 @@ func TestWebhookReconciler(t *testing.T) { } }) + t.Run("creates a webhook subscribed to all events when events are omitted", func(t *testing.T) { + api := &fakeWebhookAPI{} + spec := []byte("- {url: \"https://a.example/hook\"}\n") + + rep, err := NewWebhookReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, []string{"add webhook https://a.example/hook [all events]"}, rep.Planned) + if assert.Len(t, api.created, 1) { + assert.Empty(t, api.created[0].GetSubscribedEvents()) + } + }) + + t.Run("an endpoint subscribed to all events round-trips", func(t *testing.T) { + api := &fakeWebhookAPI{webhooks: []*frontierv1beta1.Webhook{ + webhookPB("w1", "https://a.example/hook", "", webhookStateEnabled), // no events = all events + }} + registry := map[string]Reconciler{KindWebhook: NewWebhookReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindWebhook) + assert.NoError(t, err) + assert.NotContains(t, string(out), "subscribed_events") // an empty event set is left out + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + t.Run("a dry run plans without applying", func(t *testing.T) { api := &fakeWebhookAPI{} spec := []byte("- {url: \"https://a.example/hook\", subscribed_events: [org.created]}\n") diff --git a/internal/reconcile/webhook_test.go b/internal/reconcile/webhook_test.go index a0a5d5b7e..0fc9e644b 100644 --- a/internal/reconcile/webhook_test.go +++ b/internal/reconcile/webhook_test.go @@ -59,6 +59,32 @@ func TestDiffWebhooks(t *testing.T) { assert.Empty(t, ops) }) + t.Run("an omitted event list keeps the server's events", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "old", webhookStateEnabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "new"}}, // events omitted + current, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update webhook https://a.example/hook (description)", ops[0].String()) + assert.Equal(t, []string{"org.created"}, ops[0].spec.SubscribedEvents) // kept, not wiped + } + }) + + t.Run("an explicit empty event list subscribes an endpoint to all events", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "desc", SubscribedEvents: []string{}}}, + current, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update webhook https://a.example/hook (subscribed_events)", ops[0].String()) + assert.Empty(t, ops[0].spec.SubscribedEvents) + } + }) + t.Run("state change is planned when listed and different", func(t *testing.T) { current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created")} ops, err := diffWebhooks( @@ -146,9 +172,14 @@ func TestDiffWebhooks(t *testing.T) { assert.ErrorContains(t, err, "absolute URL") }) - t.Run("validation rejects an entry with no events", func(t *testing.T) { - _, err := diffWebhooks([]WebhookSpec{{URL: "https://a.example/hook"}}, nil) - assert.ErrorContains(t, err, "at least one event") + t.Run("an entry with no events adds an endpoint subscribed to all events", func(t *testing.T) { + ops, err := diffWebhooks([]WebhookSpec{{URL: "https://a.example/hook"}}, nil) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, opAdd, ops[0].action) + assert.Empty(t, ops[0].spec.SubscribedEvents) + assert.Equal(t, "add webhook https://a.example/hook [all events]", ops[0].String()) + } }) t.Run("validation rejects an unknown state", func(t *testing.T) { From 49bcca0222593b6a7072ebd11437083926d30451 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Sat, 18 Jul 2026 13:40:10 +0530 Subject: [PATCH 04/15] docs(reconcile): note webhook subscribed_events is optional --- docs/content/docs/reconcile.mdx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/content/docs/reconcile.mdx b/docs/content/docs/reconcile.mdx index 754a66b4c..759046b7c 100644 --- a/docs/content/docs/reconcile.mdx +++ b/docs/content/docs/reconcile.mdx @@ -216,9 +216,10 @@ spec: - The URL must be a valid absolute URL, and it is the identity. If two endpoints on the server share a URL, the identity is ambiguous: the plan fails and names the ids so you can remove the extra one by hand. -- `subscribed_events` is required for a live entry and compares as a set. `description` and - `state` (`enabled` or `disabled`) are managed too; a field you leave out keeps its server - value. +- `subscribed_events` is optional and compares as a set. Leave it out to keep the server's + current events; a new endpoint with none subscribes to every event, which is the server + default. `description` and `state` (`enabled` or `disabled`) are managed the same way: a + field you leave out keeps its server value. - Every endpoint on the server must appear in the file, kept or marked `delete: true`. One that is missing fails the plan; nothing is deleted just because it is missing. - The signing secret is server-owned. The server generates it when the endpoint is created From a8ffd1802265b7f652db8eee24938ab019af162d Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Sun, 19 Jul 2026 18:59:00 +0530 Subject: [PATCH 05/15] feat(reconcile): treat webhook subscribed_events as the full desired set --- internal/reconcile/webhook.go | 23 ++++++++++-------- internal/reconcile/webhook_reconciler_test.go | 2 +- internal/reconcile/webhook_test.go | 24 ++++++++++++++----- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/internal/reconcile/webhook.go b/internal/reconcile/webhook.go index 498a6313f..1dd162e15 100644 --- a/internal/reconcile/webhook.go +++ b/internal/reconcile/webhook.go @@ -18,15 +18,18 @@ const ( ) // WebhookSpec is one desired webhook endpoint. The URL is the identity and -// never changes. Description, subscribed events, and state are managed: a field -// that is present is set, and a field that is omitted keeps the server value. -// An empty event list means the endpoint receives every event, which is the -// server's own default. Signing secrets are server-owned: the server generates -// one on create and never returns it on read, so they are not part of the spec. +// never changes. Subscribed events are the full desired set for the endpoint, +// not a per-field overlay: an empty list (or none) means the endpoint receives +// every event, which is the server's own default, and export always writes the +// field so an all-events endpoint reads as an explicit `subscribed_events: []`. +// Description and state are managed the ordinary way: present is set, omitted +// keeps the server value. Signing secrets are server-owned: the server +// generates one on create and never returns it on read, so they are not part of +// the spec. type WebhookSpec struct { URL string `yaml:"url"` Description string `yaml:"description,omitempty"` - SubscribedEvents []string `yaml:"subscribed_events,omitempty"` + SubscribedEvents []string `yaml:"subscribed_events"` State string `yaml:"state,omitempty"` Delete bool `yaml:"delete,omitempty"` } @@ -158,10 +161,10 @@ func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, merged.Description = s.Description changes = append(changes, "description") } - // Events are managed only when the entry lists them. An omitted list - // (nil) keeps the server's set; an explicit empty list ([]) sets the - // endpoint to receive every event. - if s.SubscribedEvents != nil && !stringSetsEqual(sortedCopy(s.SubscribedEvents), sortedCopy(cur.SubscribedEvents)) { + // Events are the full desired set, always compared. An empty or omitted + // list means every event, so leaving it out sets the endpoint to all + // events rather than keeping the server's set. + if !stringSetsEqual(sortedCopy(s.SubscribedEvents), sortedCopy(cur.SubscribedEvents)) { merged.SubscribedEvents = sortedCopy(s.SubscribedEvents) changes = append(changes, "subscribed_events") } diff --git a/internal/reconcile/webhook_reconciler_test.go b/internal/reconcile/webhook_reconciler_test.go index b62a8cf02..abdf98775 100644 --- a/internal/reconcile/webhook_reconciler_test.go +++ b/internal/reconcile/webhook_reconciler_test.go @@ -114,7 +114,7 @@ func TestWebhookReconciler(t *testing.T) { out, err := Export(context.Background(), registry, KindWebhook) assert.NoError(t, err) - assert.NotContains(t, string(out), "subscribed_events") // an empty event set is left out + assert.Contains(t, string(out), "subscribed_events: []") // an all-events endpoint is written explicitly reports, err := Run(context.Background(), registry, out, true) assert.NoError(t, err) diff --git a/internal/reconcile/webhook_test.go b/internal/reconcile/webhook_test.go index 0fc9e644b..c50dbc8b9 100644 --- a/internal/reconcile/webhook_test.go +++ b/internal/reconcile/webhook_test.go @@ -59,20 +59,22 @@ func TestDiffWebhooks(t *testing.T) { assert.Empty(t, ops) }) - t.Run("an omitted event list keeps the server's events", func(t *testing.T) { - current := []currentWebhook{cw("w1", "https://a.example/hook", "old", webhookStateEnabled, "org.created")} + t.Run("dropping events from an entry widens the endpoint to all events", func(t *testing.T) { + // Events are the full desired set, so omitting them is not "keep": it + // sets the endpoint to every event. + current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created")} ops, err := diffWebhooks( - []WebhookSpec{{URL: "https://a.example/hook", Description: "new"}}, // events omitted + []WebhookSpec{{URL: "https://a.example/hook", Description: "desc"}}, // events omitted current, ) assert.NoError(t, err) if assert.Len(t, ops, 1) { - assert.Equal(t, "update webhook https://a.example/hook (description)", ops[0].String()) - assert.Equal(t, []string{"org.created"}, ops[0].spec.SubscribedEvents) // kept, not wiped + assert.Equal(t, "update webhook https://a.example/hook (subscribed_events)", ops[0].String()) + assert.Empty(t, ops[0].spec.SubscribedEvents) } }) - t.Run("an explicit empty event list subscribes an endpoint to all events", func(t *testing.T) { + t.Run("an explicit empty event list behaves the same as omitting it", func(t *testing.T) { current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created")} ops, err := diffWebhooks( []WebhookSpec{{URL: "https://a.example/hook", Description: "desc", SubscribedEvents: []string{}}}, @@ -85,6 +87,16 @@ func TestDiffWebhooks(t *testing.T) { } }) + t.Run("an all-events endpoint with no events in the entry plans no change", func(t *testing.T) { + current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled)} // no events = all + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "desc"}}, + current, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + t.Run("state change is planned when listed and different", func(t *testing.T) { current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created")} ops, err := diffWebhooks( From 63acd9ce24c5e358022487fd761869dd7db57af0 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Sun, 19 Jul 2026 18:59:00 +0530 Subject: [PATCH 06/15] docs(reconcile): describe webhook subscribed_events as the full desired set --- docs/content/docs/reconcile.mdx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/content/docs/reconcile.mdx b/docs/content/docs/reconcile.mdx index 759046b7c..52c6cd0b9 100644 --- a/docs/content/docs/reconcile.mdx +++ b/docs/content/docs/reconcile.mdx @@ -216,10 +216,12 @@ spec: - The URL must be a valid absolute URL, and it is the identity. If two endpoints on the server share a URL, the identity is ambiguous: the plan fails and names the ids so you can remove the extra one by hand. -- `subscribed_events` is optional and compares as a set. Leave it out to keep the server's - current events; a new endpoint with none subscribes to every event, which is the server - default. `description` and `state` (`enabled` or `disabled`) are managed the same way: a - field you leave out keeps its server value. +- `subscribed_events` is the full set of events the endpoint receives, compared as a set. An + empty list, or leaving the field out, means every event, which is the server default. It is + the complete desired set, not a keep-if-omitted field: dropping it sets the endpoint to all + events rather than keeping the current set, and export always writes it, showing `[]` for an + all-events endpoint. `description` and `state` (`enabled` or `disabled`) are ordinary managed + fields: leave one out to keep its server value. - Every endpoint on the server must appear in the file, kept or marked `delete: true`. One that is missing fails the plan; nothing is deleted just because it is missing. - The signing secret is server-owned. The server generates it when the endpoint is created From 385797df102bd95a5c8e4910dba48bdca485bf7b Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Sun, 19 Jul 2026 19:58:44 +0530 Subject: [PATCH 07/15] fix(reconcile): dedup webhook subscribed_events to avoid spurious plans --- internal/reconcile/role.go | 20 ++++++++++++++++++++ internal/reconcile/webhook.go | 13 ++++++++----- internal/reconcile/webhook_reconciler.go | 2 +- internal/reconcile/webhook_test.go | 24 ++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/internal/reconcile/role.go b/internal/reconcile/role.go index b435f92ad..8235197d9 100644 --- a/internal/reconcile/role.go +++ b/internal/reconcile/role.go @@ -167,6 +167,26 @@ func sortedCopy(in []string) []string { return out } +// uniqueSorted returns the input as a sorted, deduplicated slice. It is used for +// set-valued fields whose input can hold duplicates (a hand-written list may +// repeat a value), so the compare and the value sent to the server both use the +// same canonical set. +func uniqueSorted(in []string) []string { + if len(in) == 0 { + return nil + } + set := make(map[string]struct{}, len(in)) + for _, v := range in { + set[v] = struct{}{} + } + out := make([]string, 0, len(set)) + for v := range set { + out = append(out, v) + } + sort.Strings(out) + return out +} + func stringSetsEqual(a, b []string) bool { if len(a) != len(b) { return false diff --git a/internal/reconcile/webhook.go b/internal/reconcile/webhook.go index 1dd162e15..bded849fe 100644 --- a/internal/reconcile/webhook.go +++ b/internal/reconcile/webhook.go @@ -140,11 +140,12 @@ func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, } continue } + desiredEvents := uniqueSorted(s.SubscribedEvents) if !exists { adds = append(adds, webhookOp{action: opAdd, spec: WebhookSpec{ URL: s.URL, Description: s.Description, - SubscribedEvents: sortedCopy(s.SubscribedEvents), + SubscribedEvents: desiredEvents, State: s.State, }}) continue @@ -153,7 +154,7 @@ func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, merged := WebhookSpec{ URL: s.URL, Description: cur.Description, - SubscribedEvents: sortedCopy(cur.SubscribedEvents), + SubscribedEvents: uniqueSorted(cur.SubscribedEvents), State: cur.State, } var changes []string @@ -163,9 +164,11 @@ func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, } // Events are the full desired set, always compared. An empty or omitted // list means every event, so leaving it out sets the endpoint to all - // events rather than keeping the server's set. - if !stringSetsEqual(sortedCopy(s.SubscribedEvents), sortedCopy(cur.SubscribedEvents)) { - merged.SubscribedEvents = sortedCopy(s.SubscribedEvents) + // events rather than keeping the server's set. The set is deduplicated so + // a hand-written list that repeats a value does not send duplicates or + // plan a spurious update. + if !stringSetsEqual(desiredEvents, uniqueSorted(cur.SubscribedEvents)) { + merged.SubscribedEvents = desiredEvents changes = append(changes, "subscribed_events") } if s.State != "" && s.State != cur.State { diff --git a/internal/reconcile/webhook_reconciler.go b/internal/reconcile/webhook_reconciler.go index cfb5e57de..c3bb2f0b8 100644 --- a/internal/reconcile/webhook_reconciler.go +++ b/internal/reconcile/webhook_reconciler.go @@ -83,7 +83,7 @@ func (r *WebhookReconciler) Export(ctx context.Context) (any, error) { entry := WebhookSpec{ URL: c.URL, Description: c.Description, - SubscribedEvents: sortedCopy(c.SubscribedEvents), + SubscribedEvents: uniqueSorted(c.SubscribedEvents), } if c.State != "" && c.State != webhookStateEnabled { entry.State = c.State diff --git a/internal/reconcile/webhook_test.go b/internal/reconcile/webhook_test.go index c50dbc8b9..161a814dc 100644 --- a/internal/reconcile/webhook_test.go +++ b/internal/reconcile/webhook_test.go @@ -194,6 +194,30 @@ func TestDiffWebhooks(t *testing.T) { } }) + t.Run("duplicate events in an entry are deduped and plan no change", func(t *testing.T) { + // the file repeats org.created; the server stored the set once, so this + // must converge to no change, not a spurious update. + current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateEnabled, "org.created", "org.deleted")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", Description: "desc", SubscribedEvents: []string{"org.created", "org.deleted", "org.created"}}}, + current, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("duplicate events on add are deduped before sending", func(t *testing.T) { + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", SubscribedEvents: []string{"org.created", "org.created", "org.deleted"}}}, + nil, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, opAdd, ops[0].action) + assert.Equal(t, []string{"org.created", "org.deleted"}, ops[0].spec.SubscribedEvents) + } + }) + t.Run("validation rejects an unknown state", func(t *testing.T) { _, err := diffWebhooks([]WebhookSpec{{URL: "https://a.example/hook", SubscribedEvents: []string{"org.created"}, State: "paused"}}, nil) assert.ErrorContains(t, err, "state must be") From cb9c6495131777cea5896109b03f428911ac4df5 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Sun, 19 Jul 2026 19:58:44 +0530 Subject: [PATCH 08/15] test(reconcile): assert a webhook update preserves its state --- internal/reconcile/webhook_reconciler_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/reconcile/webhook_reconciler_test.go b/internal/reconcile/webhook_reconciler_test.go index abdf98775..836211bcf 100644 --- a/internal/reconcile/webhook_reconciler_test.go +++ b/internal/reconcile/webhook_reconciler_test.go @@ -90,6 +90,9 @@ func TestWebhookReconciler(t *testing.T) { if body := api.updated["w1"]; assert.NotNil(t, body) { assert.Equal(t, map[string]string{"X-Token": "abc"}, body.GetHeaders()) assert.Equal(t, "platform", body.GetMetadata().GetFields()["team"].GetStringValue()) + // state was not in the file; the update must carry the server value + // through, not drop it to empty (which would reset it on the server). + assert.Equal(t, webhookStateEnabled, body.GetState()) } }) From 14828be8519867b9c4c2349de417f391afe3cc40 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Sun, 19 Jul 2026 19:58:44 +0530 Subject: [PATCH 09/15] docs(reconcile): tidy reconcile command help wrapping --- cmd/reconcile.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cmd/reconcile.go b/cmd/reconcile.go index e89fc4f39..bdb26e521 100644 --- a/cmd/reconcile.go +++ b/cmd/reconcile.go @@ -25,11 +25,10 @@ func ReconcileCommand(cliConfig *Config) *cli.Command { Kinds: PlatformUser (platform admins and members), Permission (custom permissions), Role (platform-level roles), Preference (platform settings), and Webhook (webhook endpoints). Deleting a permission, a - custom role, or a webhook needs an explicit - 'delete: true' on its entry; nothing is deleted by omission, and a predefined - role cannot be deleted. A preference left out of the file resets to its - default. Log in as a superuser (for example the bootstrap service account) - with --header. + custom role, or a webhook needs an explicit 'delete: true' on its entry; + nothing is deleted by omission, and a predefined role cannot be deleted. A + preference left out of the file resets to its default. Log in as a superuser + (for example the bootstrap service account) with --header. Use "frontier export " to print the current state in this file format. `), From 8db057c4b9aa510e6bdd2965b1d28f77dede4d8a Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Mon, 20 Jul 2026 20:36:47 +0530 Subject: [PATCH 10/15] fix(webhook): validate url and state and enforce url uniqueness server-side --- core/webhook/service.go | 49 ++++++++++++++++++++ core/webhook/service_test.go | 90 ++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 core/webhook/service_test.go diff --git a/core/webhook/service.go b/core/webhook/service.go index bccedc7f5..2eb684a9c 100644 --- a/core/webhook/service.go +++ b/core/webhook/service.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "log/slog" + "net/url" + "strings" "time" "github.com/raystack/frontier/pkg/server/consts" @@ -48,6 +50,13 @@ func (s Service) CreateEndpoint(ctx context.Context, endpoint Endpoint) (Endpoin if endpoint.State == "" { endpoint.State = Enabled } + endpoint.URL = strings.TrimSpace(endpoint.URL) + if err := validateEndpoint(endpoint); err != nil { + return Endpoint{}, err + } + if err := s.ensureURLIsFree(ctx, endpoint.URL, endpoint.ID); err != nil { + return Endpoint{}, err + } // generate a random secret in hex secretHex, err := crypt.NewEncryptionKeyInHex() @@ -65,6 +74,13 @@ func (s Service) UpdateEndpoint(ctx context.Context, endpoint Endpoint) (Endpoin if endpoint.ID == "" { return Endpoint{}, ErrInvalidUUID } + endpoint.URL = strings.TrimSpace(endpoint.URL) + if err := validateEndpoint(endpoint); err != nil { + return Endpoint{}, err + } + if err := s.ensureURLIsFree(ctx, endpoint.URL, endpoint.ID); err != nil { + return Endpoint{}, err + } updated, err := s.eRepo.UpdateByID(ctx, endpoint) if err != nil { return Endpoint{}, err @@ -73,6 +89,39 @@ func (s Service) UpdateEndpoint(ctx context.Context, endpoint Endpoint) (Endpoin return updated, nil } +// validateEndpoint checks the operator-supplied fields the reconcile flow relies +// on. The URL is that flow's identity for an endpoint and the state is managed +// as enabled/disabled, so the server only stores values that reconcile can +// represent and round-trip: a valid absolute URL, and a known state. +func validateEndpoint(endpoint Endpoint) error { + if u, err := url.Parse(endpoint.URL); err != nil || !u.IsAbs() { + return fmt.Errorf("%w: url must be a valid absolute URL", ErrInvalidDetail) + } + switch endpoint.State { + case "", Enabled, Disabled: + default: + return fmt.Errorf("%w: state must be %q or %q", ErrInvalidDetail, Enabled, Disabled) + } + return nil +} + +// ensureURLIsFree rejects a URL that another endpoint already uses. The reconcile +// flow uses the URL as the endpoint's identity, so two endpoints sharing a URL +// would be ambiguous. excludeID is the endpoint being updated, so it does not +// conflict with itself. +func (s Service) ensureURLIsFree(ctx context.Context, rawURL, excludeID string) error { + existing, err := s.eRepo.List(ctx, EndpointFilter{}) + if err != nil { + return err + } + for _, e := range existing { + if e.ID != excludeID && e.URL == rawURL { + return fmt.Errorf("%w: url %q is already used by another webhook", ErrConflict, rawURL) + } + } + return nil +} + func (s Service) DeleteEndpoint(ctx context.Context, id string) error { return s.eRepo.Delete(ctx, id) } diff --git a/core/webhook/service_test.go b/core/webhook/service_test.go new file mode 100644 index 000000000..68b2748ca --- /dev/null +++ b/core/webhook/service_test.go @@ -0,0 +1,90 @@ +package webhook_test + +import ( + "context" + "testing" + + "github.com/raystack/frontier/core/webhook" + "github.com/stretchr/testify/assert" +) + +// fakeEndpointRepo is an in-memory webhook.EndpointRepository for service tests. +type fakeEndpointRepo struct { + items []webhook.Endpoint +} + +func (f *fakeEndpointRepo) Create(_ context.Context, e webhook.Endpoint) (webhook.Endpoint, error) { + f.items = append(f.items, e) + return e, nil +} + +func (f *fakeEndpointRepo) UpdateByID(_ context.Context, e webhook.Endpoint) (webhook.Endpoint, error) { + for i := range f.items { + if f.items[i].ID == e.ID { + f.items[i] = e + return e, nil + } + } + return webhook.Endpoint{}, webhook.ErrNotFound +} + +func (f *fakeEndpointRepo) Delete(_ context.Context, _ string) error { return nil } + +func (f *fakeEndpointRepo) List(_ context.Context, _ webhook.EndpointFilter) ([]webhook.Endpoint, error) { + return f.items, nil +} + +func TestServiceCreateEndpointValidation(t *testing.T) { + t.Run("rejects a non-absolute url", func(t *testing.T) { + s := webhook.NewService(&fakeEndpointRepo{}) + _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "not-a-url"}) + assert.ErrorIs(t, err, webhook.ErrInvalidDetail) + }) + + t.Run("rejects an empty url", func(t *testing.T) { + s := webhook.NewService(&fakeEndpointRepo{}) + _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: " "}) + assert.ErrorIs(t, err, webhook.ErrInvalidDetail) + }) + + t.Run("rejects an unknown state", func(t *testing.T) { + s := webhook.NewService(&fakeEndpointRepo{}) + _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "https://a.example/hook", State: "paused"}) + assert.ErrorIs(t, err, webhook.ErrInvalidDetail) + }) + + t.Run("rejects a url another endpoint already uses", func(t *testing.T) { + repo := &fakeEndpointRepo{items: []webhook.Endpoint{{ID: "e1", URL: "https://a.example/hook"}}} + _, err := webhook.NewService(repo).CreateEndpoint(context.Background(), webhook.Endpoint{URL: "https://a.example/hook"}) + assert.ErrorIs(t, err, webhook.ErrConflict) + }) + + t.Run("creates a valid endpoint, defaulting state and generating a secret", func(t *testing.T) { + got, err := webhook.NewService(&fakeEndpointRepo{}).CreateEndpoint( + context.Background(), webhook.Endpoint{URL: "https://a.example/hook"}) + assert.NoError(t, err) + assert.Equal(t, webhook.Enabled, got.State) // defaulted + assert.Len(t, got.Secrets, 1) // server-generated signing secret + }) +} + +func TestServiceUpdateEndpointURLUniqueness(t *testing.T) { + t.Run("rejects a url used by a different endpoint", func(t *testing.T) { + repo := &fakeEndpointRepo{items: []webhook.Endpoint{ + {ID: "e1", URL: "https://a.example/hook", State: webhook.Enabled}, + {ID: "e2", URL: "https://b.example/hook", State: webhook.Enabled}, + }} + _, err := webhook.NewService(repo).UpdateEndpoint(context.Background(), + webhook.Endpoint{ID: "e2", URL: "https://a.example/hook", State: webhook.Enabled}) + assert.ErrorIs(t, err, webhook.ErrConflict) + }) + + t.Run("lets an endpoint keep its own url", func(t *testing.T) { + repo := &fakeEndpointRepo{items: []webhook.Endpoint{ + {ID: "e1", URL: "https://a.example/hook", State: webhook.Enabled}, + }} + _, err := webhook.NewService(repo).UpdateEndpoint(context.Background(), + webhook.Endpoint{ID: "e1", URL: "https://a.example/hook", State: webhook.Disabled}) + assert.NoError(t, err) + }) +} From 9f2cc058e9a0a41448e263fbebf1032e9629917f Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Mon, 20 Jul 2026 20:36:47 +0530 Subject: [PATCH 11/15] fix(webhook): map webhook validation errors to proper status codes --- internal/api/v1beta1connect/webhook.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/internal/api/v1beta1connect/webhook.go b/internal/api/v1beta1connect/webhook.go index eced182f0..1218dcf9c 100644 --- a/internal/api/v1beta1connect/webhook.go +++ b/internal/api/v1beta1connect/webhook.go @@ -2,6 +2,7 @@ package v1beta1connect import ( "context" + "errors" "fmt" "connectrpc.com/connect" @@ -11,6 +12,22 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" ) +// webhookErrCode maps a webhook service error to the connect status code the +// caller should see, so a bad request reads as invalid-argument rather than an +// internal error. +func webhookErrCode(err error) connect.Code { + switch { + case errors.Is(err, webhook.ErrInvalidDetail), errors.Is(err, webhook.ErrInvalidUUID): + return connect.CodeInvalidArgument + case errors.Is(err, webhook.ErrConflict): + return connect.CodeAlreadyExists + case errors.Is(err, webhook.ErrNotFound): + return connect.CodeNotFound + default: + return connect.CodeInternal + } +} + func (h *ConnectHandler) CreateWebhook(ctx context.Context, req *connect.Request[frontierv1beta1.CreateWebhookRequest]) (*connect.Response[frontierv1beta1.CreateWebhookResponse], error) { var metaDataMap metadata.Metadata if req.Msg.GetBody().GetMetadata() != nil { @@ -25,7 +42,7 @@ func (h *ConnectHandler) CreateWebhook(ctx context.Context, req *connect.Request Metadata: metaDataMap, }) if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("CreateWebhook: url=%s: %w", req.Msg.GetBody().GetUrl(), err)) + return nil, connect.NewError(webhookErrCode(err), fmt.Errorf("CreateWebhook: url=%s: %w", req.Msg.GetBody().GetUrl(), err)) } endpointPb, err := toProtoWebhookEndpoint(endpoint) if err != nil { @@ -53,7 +70,7 @@ func (h *ConnectHandler) UpdateWebhook(ctx context.Context, req *connect.Request Metadata: metaDataMap, }) if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("UpdateWebhook: webhook_id=%s url=%s: %w", webhookID, req.Msg.GetBody().GetUrl(), err)) + return nil, connect.NewError(webhookErrCode(err), fmt.Errorf("UpdateWebhook: webhook_id=%s url=%s: %w", webhookID, req.Msg.GetBody().GetUrl(), err)) } endpointPb, err := toProtoWebhookEndpoint(endpoint) if err != nil { From 05029c80f3268d4fd98894e083c601326cc6558f Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Mon, 20 Jul 2026 22:57:00 +0530 Subject: [PATCH 12/15] feat(reconcile): implement Validate for the Webhook kind --- internal/reconcile/webhook_reconciler.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/reconcile/webhook_reconciler.go b/internal/reconcile/webhook_reconciler.go index c3bb2f0b8..d19850f8c 100644 --- a/internal/reconcile/webhook_reconciler.go +++ b/internal/reconcile/webhook_reconciler.go @@ -34,6 +34,24 @@ func NewWebhookReconciler(client WebhookAPI, header string) *WebhookReconciler { func (r *WebhookReconciler) Kind() string { return KindWebhook } +func (r *WebhookReconciler) Validate(spec []byte) error { + var specs []WebhookSpec + if err := decodeSpec(spec, &specs); err != nil { + return fmt.Errorf("parse %s spec: %w", KindWebhook, err) + } + seen := map[string]struct{}{} + for _, s := range specs { + if err := validateWebhookSpec(s); err != nil { + return fmt.Errorf("invalid webhook spec %q: %w", s.URL, err) + } + if _, dup := seen[s.URL]; dup { + return fmt.Errorf("webhook %q is listed more than once", s.URL) + } + seen[s.URL] = struct{}{} + } + return nil +} + func (r *WebhookReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { var specs []WebhookSpec if err := decodeSpec(spec, &specs); err != nil { From ac9457c2e6ca19bba09beb14a105cfe1a72f5ce4 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 21 Jul 2026 11:11:06 +0530 Subject: [PATCH 13/15] fix(webhook): map list and delete webhook errors to status codes --- internal/api/v1beta1connect/webhook.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/v1beta1connect/webhook.go b/internal/api/v1beta1connect/webhook.go index 1218dcf9c..d47417e5b 100644 --- a/internal/api/v1beta1connect/webhook.go +++ b/internal/api/v1beta1connect/webhook.go @@ -85,7 +85,7 @@ func (h *ConnectHandler) ListWebhooks(ctx context.Context, req *connect.Request[ filter := webhook.EndpointFilter{} endpoints, err := h.webhookService.ListEndpoints(ctx, filter) if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("ListWebhooks: %w", err)) + return nil, connect.NewError(webhookErrCode(err), fmt.Errorf("ListWebhooks: %w", err)) } var webhooks []*frontierv1beta1.Webhook for _, endpoint := range endpoints { @@ -105,7 +105,7 @@ func (h *ConnectHandler) DeleteWebhook(ctx context.Context, req *connect.Request err := h.webhookService.DeleteEndpoint(ctx, webhookID) if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteWebhook: webhook_id=%s: %w", webhookID, err)) + return nil, connect.NewError(webhookErrCode(err), fmt.Errorf("DeleteWebhook: webhook_id=%s: %w", webhookID, err)) } return connect.NewResponse(&frontierv1beta1.DeleteWebhookResponse{}), nil } From 9c7aa147a8654b82d65869bacdf015fcc91fea1b Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 21 Jul 2026 11:11:06 +0530 Subject: [PATCH 14/15] fix(webhook): restrict urls to http(s) and normalize them before matching --- core/webhook/service.go | 8 ++++- core/webhook/service_test.go | 6 ++++ internal/reconcile/webhook.go | 40 +++++++++++++++++++----- internal/reconcile/webhook_reconciler.go | 13 ++------ internal/reconcile/webhook_test.go | 14 +++++++++ 5 files changed, 62 insertions(+), 19 deletions(-) diff --git a/core/webhook/service.go b/core/webhook/service.go index 2eb684a9c..ea164a8f0 100644 --- a/core/webhook/service.go +++ b/core/webhook/service.go @@ -94,9 +94,15 @@ func (s Service) UpdateEndpoint(ctx context.Context, endpoint Endpoint) (Endpoin // as enabled/disabled, so the server only stores values that reconcile can // represent and round-trip: a valid absolute URL, and a known state. func validateEndpoint(endpoint Endpoint) error { - if u, err := url.Parse(endpoint.URL); err != nil || !u.IsAbs() { + u, err := url.Parse(endpoint.URL) + if err != nil || !u.IsAbs() { return fmt.Errorf("%w: url must be a valid absolute URL", ErrInvalidDetail) } + // The server dispatches events to this URL, so restrict it to http(s). This + // keeps other schemes (file, gopher, ...) out of the delivery path. + if u.Scheme != "http" && u.Scheme != "https" { + return fmt.Errorf("%w: url scheme must be http or https", ErrInvalidDetail) + } switch endpoint.State { case "", Enabled, Disabled: default: diff --git a/core/webhook/service_test.go b/core/webhook/service_test.go index 68b2748ca..4c85901c9 100644 --- a/core/webhook/service_test.go +++ b/core/webhook/service_test.go @@ -47,6 +47,12 @@ func TestServiceCreateEndpointValidation(t *testing.T) { assert.ErrorIs(t, err, webhook.ErrInvalidDetail) }) + t.Run("rejects a non-http(s) scheme", func(t *testing.T) { + s := webhook.NewService(&fakeEndpointRepo{}) + _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "ftp://a.example/hook"}) + assert.ErrorIs(t, err, webhook.ErrInvalidDetail) + }) + t.Run("rejects an unknown state", func(t *testing.T) { s := webhook.NewService(&fakeEndpointRepo{}) _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "https://a.example/hook", State: "paused"}) diff --git a/internal/reconcile/webhook.go b/internal/reconcile/webhook.go index bded849fe..a3cdc9c38 100644 --- a/internal/reconcile/webhook.go +++ b/internal/reconcile/webhook.go @@ -82,9 +82,15 @@ func validateWebhookSpec(s WebhookSpec) error { if strings.TrimSpace(s.URL) == "" { return fmt.Errorf("url is required") } - if u, err := url.Parse(s.URL); err != nil || !u.IsAbs() { + u, err := url.Parse(s.URL) + if err != nil || !u.IsAbs() { return fmt.Errorf("url %q must be a valid absolute URL", s.URL) } + // The server only dispatches over http(s) and rejects other schemes, so + // reject them at plan time too and keep the export round-trip consistent. + if u.Scheme != "http" && u.Scheme != "https" { + return fmt.Errorf("url %q must use http or https", s.URL) + } if s.Delete { return nil } @@ -94,6 +100,27 @@ func validateWebhookSpec(s WebhookSpec) error { return nil } +// normalizeWebhookSpecs trims each url so it matches the server's own +// normalization (the service trims before it stores), validates every entry, +// and rejects a url listed more than once. It returns the normalized specs so +// diffWebhooks and Validate work from identical, deduplicated input. +func normalizeWebhookSpecs(specs []WebhookSpec) ([]WebhookSpec, error) { + seen := map[string]struct{}{} + out := make([]WebhookSpec, 0, len(specs)) + for _, s := range specs { + s.URL = strings.TrimSpace(s.URL) + if err := validateWebhookSpec(s); err != nil { + return nil, fmt.Errorf("invalid webhook spec %q: %w", s.URL, err) + } + if _, dup := seen[s.URL]; dup { + return nil, fmt.Errorf("webhook %q is listed more than once", s.URL) + } + seen[s.URL] = struct{}{} + out = append(out, s) + } + return out, nil +} + // diffWebhooks returns the ops that make the current webhook endpoints match the // desired spec. The URL is the identity: every endpoint on the server must // appear in the file — kept, or marked delete — so nothing is removed by @@ -122,15 +149,14 @@ func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, byURL[c.URL] = c } + desired, err := normalizeWebhookSpecs(desired) + if err != nil { + return nil, err + } + seen := map[string]struct{}{} var adds, updates, removes []webhookOp for _, s := range desired { - if err := validateWebhookSpec(s); err != nil { - return nil, fmt.Errorf("invalid webhook spec %q: %w", s.URL, err) - } - if _, dup := seen[s.URL]; dup { - return nil, fmt.Errorf("webhook %q is listed more than once", s.URL) - } seen[s.URL] = struct{}{} cur, exists := byURL[s.URL] diff --git a/internal/reconcile/webhook_reconciler.go b/internal/reconcile/webhook_reconciler.go index d19850f8c..a65f02402 100644 --- a/internal/reconcile/webhook_reconciler.go +++ b/internal/reconcile/webhook_reconciler.go @@ -39,17 +39,8 @@ func (r *WebhookReconciler) Validate(spec []byte) error { if err := decodeSpec(spec, &specs); err != nil { return fmt.Errorf("parse %s spec: %w", KindWebhook, err) } - seen := map[string]struct{}{} - for _, s := range specs { - if err := validateWebhookSpec(s); err != nil { - return fmt.Errorf("invalid webhook spec %q: %w", s.URL, err) - } - if _, dup := seen[s.URL]; dup { - return fmt.Errorf("webhook %q is listed more than once", s.URL) - } - seen[s.URL] = struct{}{} - } - return nil + _, err := normalizeWebhookSpecs(specs) + return err } func (r *WebhookReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { diff --git a/internal/reconcile/webhook_test.go b/internal/reconcile/webhook_test.go index 161a814dc..003f1b646 100644 --- a/internal/reconcile/webhook_test.go +++ b/internal/reconcile/webhook_test.go @@ -184,6 +184,20 @@ func TestDiffWebhooks(t *testing.T) { assert.ErrorContains(t, err, "absolute URL") }) + t.Run("validation rejects a non-http(s) scheme", func(t *testing.T) { + _, err := diffWebhooks([]WebhookSpec{{URL: "ftp://a.example/hook"}}, nil) + assert.ErrorContains(t, err, "http or https") + }) + + t.Run("surrounding whitespace in the url is trimmed to match the server url", func(t *testing.T) { + // the server trims before it stores, so an untrimmed file url must still + // match the stored endpoint rather than plan a spurious add. + current := []currentWebhook{cw("w1", "https://a.example/hook", "", webhookStateEnabled)} + ops, err := diffWebhooks([]WebhookSpec{{URL: " https://a.example/hook "}}, current) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + t.Run("an entry with no events adds an endpoint subscribed to all events", func(t *testing.T) { ops, err := diffWebhooks([]WebhookSpec{{URL: "https://a.example/hook"}}, nil) assert.NoError(t, err) From e7a40439291b5f3a7a0c53c356cb09cfaeb32a1a Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Wed, 22 Jul 2026 22:43:35 +0530 Subject: [PATCH 15/15] fix(reconcile): converge webhook description and state to their defaults Webhook description and state used keep-if-omitted: an omitted field kept the server value, so you could not clear a description or reset a disabled endpoint by leaving the field out. That breaks rule 2's one field model. resolve() now lays the file's present fields over the defaults (description empty, state enabled), so an omitted field converges to its default and nothing is merged from the server. Export already drops default-valued fields, so the round-trip still plans zero ops. --- internal/reconcile/webhook.go | 75 +++++++++++-------- internal/reconcile/webhook_reconciler.go | 7 +- internal/reconcile/webhook_reconciler_test.go | 7 +- internal/reconcile/webhook_test.go | 25 ++++++- 4 files changed, 73 insertions(+), 41 deletions(-) diff --git a/internal/reconcile/webhook.go b/internal/reconcile/webhook.go index a3cdc9c38..271e3c6d8 100644 --- a/internal/reconcile/webhook.go +++ b/internal/reconcile/webhook.go @@ -10,22 +10,25 @@ import ( // KindWebhook is the desired-state document kind for webhook endpoints. const KindWebhook = "Webhook" -// Webhook states. The server enables a new endpoint by default, so an entry -// that does not set a state leaves the server's value in place. +// Webhook states. The server gives a new endpoint the enabled state, so enabled +// is the default an entry converges to when it does not set a state. const ( webhookStateEnabled = "enabled" webhookStateDisabled = "disabled" ) // WebhookSpec is one desired webhook endpoint. The URL is the identity and -// never changes. Subscribed events are the full desired set for the endpoint, -// not a per-field overlay: an empty list (or none) means the endpoint receives +// never changes. Every other field states the whole desired value under the one +// field model: a field written in the file is used as-is, and an omitted field +// takes its default, nothing is merged from the server. Subscribed events are +// the full desired set: an empty list (or none) means the endpoint receives // every event, which is the server's own default, and export always writes the // field so an all-events endpoint reads as an explicit `subscribed_events: []`. -// Description and state are managed the ordinary way: present is set, omitted -// keeps the server value. Signing secrets are server-owned: the server -// generates one on create and never returns it on read, so they are not part of -// the spec. +// Description defaults to empty, so omitting it clears any description on the +// server. State defaults to enabled, the state the server gives a new endpoint, +// so omitting it converges the endpoint to enabled. Signing secrets are +// server-owned: the server generates one on create and never returns it on read, +// so they are not part of the spec. type WebhookSpec struct { URL string `yaml:"url"` Description string `yaml:"description,omitempty"` @@ -121,6 +124,26 @@ func normalizeWebhookSpecs(specs []WebhookSpec) ([]WebhookSpec, error) { return out, nil } +// resolve returns the whole desired state for one entry under the one field +// model. A field present in the file is used as written; an omitted field takes +// its default. Description defaults to empty, so omitting it clears the server's +// description. Subscribed events default to the full set, so an empty or omitted +// list means every event. State defaults to enabled, the state the server gives +// a new endpoint. Nothing is merged from the server: omitting a field converges +// it to its default rather than keeping whatever the server holds. +func (s WebhookSpec) resolve() WebhookSpec { + want := WebhookSpec{ + URL: s.URL, + Description: s.Description, + SubscribedEvents: uniqueSorted(s.SubscribedEvents), + State: s.State, + } + if want.State == "" { + want.State = webhookStateEnabled + } + return want +} + // diffWebhooks returns the ops that make the current webhook endpoints match the // desired spec. The URL is the identity: every endpoint on the server must // appear in the file — kept, or marked delete — so nothing is removed by @@ -166,45 +189,31 @@ func diffWebhooks(desired []WebhookSpec, current []currentWebhook) ([]webhookOp, } continue } - desiredEvents := uniqueSorted(s.SubscribedEvents) + + want := s.resolve() if !exists { - adds = append(adds, webhookOp{action: opAdd, spec: WebhookSpec{ - URL: s.URL, - Description: s.Description, - SubscribedEvents: desiredEvents, - State: s.State, - }}) + adds = append(adds, webhookOp{action: opAdd, spec: want}) continue } - merged := WebhookSpec{ - URL: s.URL, - Description: cur.Description, - SubscribedEvents: uniqueSorted(cur.SubscribedEvents), - State: cur.State, - } + // want is the whole desired state, nothing merged from the server: an + // omitted field has already converged to its default in resolve. Events + // are the full desired set (an empty set means every event), deduplicated + // so a hand-written list that repeats a value plans no spurious update. var changes []string - if s.Description != "" && s.Description != cur.Description { - merged.Description = s.Description + if want.Description != cur.Description { changes = append(changes, "description") } - // Events are the full desired set, always compared. An empty or omitted - // list means every event, so leaving it out sets the endpoint to all - // events rather than keeping the server's set. The set is deduplicated so - // a hand-written list that repeats a value does not send duplicates or - // plan a spurious update. - if !stringSetsEqual(desiredEvents, uniqueSorted(cur.SubscribedEvents)) { - merged.SubscribedEvents = desiredEvents + if !stringSetsEqual(want.SubscribedEvents, uniqueSorted(cur.SubscribedEvents)) { changes = append(changes, "subscribed_events") } - if s.State != "" && s.State != cur.State { - merged.State = s.State + if want.State != cur.State { changes = append(changes, "state") } if len(changes) > 0 { updates = append(updates, webhookOp{ action: opUpdate, - spec: merged, + spec: want, id: cur.ID, detail: strings.Join(changes, ", "), headers: cur.Headers, diff --git a/internal/reconcile/webhook_reconciler.go b/internal/reconcile/webhook_reconciler.go index a65f02402..834273169 100644 --- a/internal/reconcile/webhook_reconciler.go +++ b/internal/reconcile/webhook_reconciler.go @@ -77,9 +77,10 @@ func (r *WebhookReconciler) Reconcile(ctx context.Context, spec []byte, dryRun b // Export returns the current webhook endpoints as a desired-state spec, sorted // by url. State is written only when it is not the default ("enabled") and an -// empty description is left out, so an omitted field on reconcile keeps the -// server value and the export round-trips to no changes. Secrets are never read -// from the server, so they can never appear in an export. +// empty description is left out. Because reconcile converges an omitted field to +// its default, dropping a default-valued field keeps the export minimal and +// still round-trips to no changes. Secrets are never read from the server, so +// they can never appear in an export. func (r *WebhookReconciler) Export(ctx context.Context) (any, error) { current, err := r.fetchCurrent(ctx) if err != nil { diff --git a/internal/reconcile/webhook_reconciler_test.go b/internal/reconcile/webhook_reconciler_test.go index 836211bcf..21ecffc1d 100644 --- a/internal/reconcile/webhook_reconciler_test.go +++ b/internal/reconcile/webhook_reconciler_test.go @@ -81,7 +81,7 @@ func TestWebhookReconciler(t *testing.T) { wh.Metadata = md api := &fakeWebhookAPI{webhooks: []*frontierv1beta1.Webhook{wh}} // only the event set changes; headers and metadata must survive the update - spec := []byte("- {url: \"https://a.example/hook\", subscribed_events: [org.created, org.deleted]}\n") + spec := []byte("- {url: \"https://a.example/hook\", description: desc, subscribed_events: [org.created, org.deleted]}\n") rep, err := NewWebhookReconciler(api, "").Reconcile(context.Background(), spec, false) @@ -90,8 +90,9 @@ func TestWebhookReconciler(t *testing.T) { if body := api.updated["w1"]; assert.NotNil(t, body) { assert.Equal(t, map[string]string{"X-Token": "abc"}, body.GetHeaders()) assert.Equal(t, "platform", body.GetMetadata().GetFields()["team"].GetStringValue()) - // state was not in the file; the update must carry the server value - // through, not drop it to empty (which would reset it on the server). + // state was not in the file; it converges to the enabled default, which + // matches the server here, so the update keeps it enabled rather than + // dropping it to empty (which would reset it on the server). assert.Equal(t, webhookStateEnabled, body.GetState()) } }) diff --git a/internal/reconcile/webhook_test.go b/internal/reconcile/webhook_test.go index 003f1b646..3b9324047 100644 --- a/internal/reconcile/webhook_test.go +++ b/internal/reconcile/webhook_test.go @@ -49,14 +49,35 @@ func TestDiffWebhooks(t *testing.T) { assert.Empty(t, ops) }) - t.Run("an omitted state keeps the server value", func(t *testing.T) { + t.Run("an omitted state resets the endpoint to the enabled default", func(t *testing.T) { + // Under the one field model an omitted field converges to its default, and + // the state default is enabled. A disabled endpoint with no state in the + // file must plan a reset to enabled, not keep the server value. current := []currentWebhook{cw("w1", "https://a.example/hook", "desc", webhookStateDisabled, "org.created")} ops, err := diffWebhooks( []WebhookSpec{{URL: "https://a.example/hook", Description: "desc", SubscribedEvents: []string{"org.created"}}}, current, ) assert.NoError(t, err) - assert.Empty(t, ops) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update webhook https://a.example/hook (state)", ops[0].String()) + assert.Equal(t, webhookStateEnabled, ops[0].spec.State) + } + }) + + t.Run("omitting a description clears it", func(t *testing.T) { + // Description defaults to empty, so leaving it out of the file clears any + // description the server holds rather than keeping it. + current := []currentWebhook{cw("w1", "https://a.example/hook", "old", webhookStateEnabled, "org.created")} + ops, err := diffWebhooks( + []WebhookSpec{{URL: "https://a.example/hook", SubscribedEvents: []string{"org.created"}}}, + current, + ) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update webhook https://a.example/hook (description)", ops[0].String()) + assert.Equal(t, "", ops[0].spec.Description) + } }) t.Run("dropping events from an entry widens the endpoint to all events", func(t *testing.T) {