From 27fab41c1c43516a3d04c8fbf55976ce7cce28d1 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Wed, 2 Sep 2026 12:45:15 -0500 Subject: [PATCH] appview: make the webhook cap agree between creation and delivery The UI and the dispatcher asked the same question and got different answers. getWebhookLimits short-circuited on a disabled billing manager and returned unlimited without consulting it, while server.go hands BillingManager.GetWebhookLimits straight to NewDispatcher, bypassing that short-circuit entirely. With billing compiled out the stub answers non-captains with (1, false). So a non-captain saw "N / unlimited webhooks configured", could create as many as they liked, and only the oldest was ever delivered. allTriggers was false on that same path, so even the surviving one was restricted to FreeTriggerMask; a webhook set to a scan trigger fired nothing at all, with no message anywhere and only an INFO line server-side. Route both paths through the manager so they cannot drift. The intended non-billing policy is one webhook with TriggerFirst | TriggerPush | TriggerQuota, which is what the stub already returned and what the shipped config's Free tier specifies (max_webhooks: 1, webhook_all_triggers: false), so enabling billing leaves free users exactly where they were and only unlocks upward. That also removes a downgrade cliff: nobody can accumulate webhooks under a phantom unlimited and lose them when billing turns on. Drop the dead webhookLimits{Max: 1}, overwritten on the following line, and give a nil manager the same policy rather than a third answer. Latent, not live: production has zero webhooks configured today. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk --- pkg/appview/handlers/webhook_limits_test.go | 83 +++++++++++++++++++++ pkg/appview/handlers/webhooks.go | 28 +++++-- 2 files changed, 103 insertions(+), 8 deletions(-) create mode 100644 pkg/appview/handlers/webhook_limits_test.go diff --git a/pkg/appview/handlers/webhook_limits_test.go b/pkg/appview/handlers/webhook_limits_test.go new file mode 100644 index 0000000..995c09e --- /dev/null +++ b/pkg/appview/handlers/webhook_limits_test.go @@ -0,0 +1,83 @@ +//go:build !billing + +// These tests pin the policy a billing-compiled-out build runs with, which is +// what production ships today (finding 1). They construct the stub manager, so +// they cannot build under -tags billing where billing.New requires a real +// config. A billing-enabled counterpart asserting the same agreement property +// against the tier ladder is worth adding when billing is turned on. + +package handlers + +import ( + "testing" + + "atcr.io/pkg/appview/webhooks" + "atcr.io/pkg/billing" +) + +// The creation gate and the delivery path must answer the same question the +// same way. They are wired separately — the UI calls getWebhookLimits, while +// the dispatcher gets BillingManager.GetWebhookLimits handed to it directly in +// server.go — so nothing but a test stops them drifting apart again. +// +// Finding 22: getWebhookLimits used to short-circuit on a disabled manager and +// answer "unlimited" without consulting it. The UI offered unlimited webhooks +// while delivery capped non-captains at one and dropped every trigger outside +// FreeTriggerMask, with no message anywhere. +func TestGetWebhookLimits_AgreesWithDeliveryPath(t *testing.T) { + // The stub manager is what a billing-compiled-out build runs with. + mgr := billing.New(nil, nil, "", nil, "", nil) + h := &BaseUIHandler{BillingManager: mgr} + + const userDID = "did:plc:notacaptain" + + uiLimits := h.getWebhookLimits(userDID) + + // This is exactly what NewDispatcher receives in server.go. + deliveryMax, deliveryAllTriggers := mgr.GetWebhookLimits(userDID) + + if uiLimits.Max != deliveryMax { + t.Errorf("UI offers Max=%d but delivery allows %d", uiLimits.Max, deliveryMax) + } + if uiLimits.AllTriggers != deliveryAllTriggers { + t.Errorf("UI AllTriggers=%v but delivery uses %v", uiLimits.AllTriggers, deliveryAllTriggers) + } +} + +// The intended non-billing policy: one webhook, restricted to the free +// triggers. This mirrors the Free tier in the shipped config (max_webhooks: 1, +// webhook_all_triggers: false), so turning billing on does not change what a +// free user already had. +func TestGetWebhookLimits_NonBillingDefault(t *testing.T) { + h := &BaseUIHandler{BillingManager: billing.New(nil, nil, "", nil, "", nil)} + + limits := h.getWebhookLimits("did:plc:notacaptain") + + if limits.Max != 1 { + t.Errorf("non-billing default should be 1 webhook, got %d", limits.Max) + } + if limits.AllTriggers { + t.Error("non-billing default must not grant triggers outside FreeTriggerMask") + } + + // Name the three that survive, so a change to FreeTriggerMask has to come + // past this test rather than silently altering what a free user can pick. + want := webhooks.TriggerFirst | webhooks.TriggerPush | webhooks.TriggerQuota + if webhooks.FreeTriggerMask != want { + t.Errorf("FreeTriggerMask = %d, want TriggerFirst|TriggerPush|TriggerQuota (%d)", + webhooks.FreeTriggerMask, want) + } +} + +// A nil manager must land on the same policy rather than inventing a third +// answer, since the dispatcher would still be applying the real one. +func TestGetWebhookLimits_NilManagerMatchesPolicy(t *testing.T) { + h := &BaseUIHandler{} + + limits := h.getWebhookLimits("did:plc:whoever") + + if limits.Max != 1 || limits.AllTriggers { + t.Errorf("nil manager gave Max=%d AllTriggers=%v, want 1/false", + limits.Max, limits.AllTriggers) + } +} diff --git a/pkg/appview/handlers/webhooks.go b/pkg/appview/handlers/webhooks.go index e23b581..1b1a402 100644 --- a/pkg/appview/handlers/webhooks.go +++ b/pkg/appview/handlers/webhooks.go @@ -243,16 +243,28 @@ func (h *TestWebhookHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // ---- Shared helpers ---- -// getWebhookLimits returns the webhook limits for a user based on their billing tier. -// When the billing manager is absent or disabled we treat the deployment as -// "all features free": unlimited webhooks and all trigger types allowed. -// Without this, self-hosted instances without billing config silently capped -// users at 1 webhook with restricted triggers. +// getWebhookLimits returns the webhook limits for a user based on their billing +// tier. +// +// This must agree with the delivery path, which wires +// BillingManager.GetWebhookLimits straight into the dispatcher (see +// NewDispatcher in server.go). An earlier version short-circuited on a disabled +// manager and answered "unlimited" without consulting it, so the UI offered +// unlimited webhooks while delivery silently capped non-captains at one and +// dropped every trigger outside FreeTriggerMask. Both paths now go through the +// same method so they cannot drift again. +// +// With billing compiled out the manager's stub is the policy: hold captains get +// unlimited, everyone else gets one webhook restricted to FreeTriggerMask +// (TriggerFirst | TriggerPush | TriggerQuota). func (h *BaseUIHandler) getWebhookLimits(userDID string) webhookLimits { - if h.BillingManager == nil || !h.BillingManager.Enabled() { - return webhookLimits{Max: -1, AllTriggers: true} + if h.BillingManager == nil { + // No manager at all, which happens in tests and partially constructed + // handlers. Mirror the non-billing policy rather than inventing a third + // answer that neither path would agree with. + return webhookLimits{Max: 1} } - limits := webhookLimits{Max: 1} + limits := webhookLimits{} limits.Max, limits.AllTriggers = h.BillingManager.GetWebhookLimits(userDID) limits.PaidTierName = h.BillingManager.GetFirstTierWithAllTriggers() return limits