Only send webhook events to active system webhooks and only deliver to active hooks (#19234) (#19248)

Backport #19234

There is a bug in the system webhooks whereby the active state is not checked when
webhooks are prepared and there is a bug that deactivating webhooks do not prevent
queued deliveries.

* Only add SystemWebhooks to the prepareWebhooks list if they are active
* At the time of delivery if the underlying webhook is not active mark it
as "delivered" but with a failed delivery so it does not get delivered.

Fix #19220

Signed-off-by: Andrew Thornton <art27@cantab.net>

models/webhook/webhook.go

@@ -497,16 +497,21 @@ func GetSystemOrDefaultWebhook(id int64) (*Webhook, error) {
 }
 
 // GetSystemWebhooks returns all admin system webhooks.
-func GetSystemWebhooks() ([]*Webhook, error) {
-	return getSystemWebhooks(db.GetEngine(db.DefaultContext))
+func GetSystemWebhooks(isActive util.OptionalBool) ([]*Webhook, error) {
+	return getSystemWebhooks(db.GetEngine(db.DefaultContext), isActive)
 }
 
-func getSystemWebhooks(e db.Engine) ([]*Webhook, error) {
+func getSystemWebhooks(e db.Engine, isActive util.OptionalBool) ([]*Webhook, error) {
 	webhooks := make([]*Webhook, 0, 5)
+	if isActive.IsNone() {
 		return webhooks, e.
 			Where("repo_id=? AND org_id=? AND is_system_webhook=?", 0, 0, true).
 			Find(&webhooks)
 	}
+	return webhooks, e.
+		Where("repo_id=? AND org_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, true, isActive.IsTrue()).
+		Find(&webhooks)
+}
 
 // UpdateWebhook updates information of webhook.
 func UpdateWebhook(w *Webhook) error {

routers/web/admin/hooks.go

@@ -11,6 +11,7 @@ import (
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
 const (
@@ -34,7 +35,7 @@ func DefaultOrSystemWebhooks(ctx *context.Context) {
 
 	sys["Title"] = ctx.Tr("admin.systemhooks")
 	sys["Description"] = ctx.Tr("admin.systemhooks.desc")
-	sys["Webhooks"], err = webhook.GetSystemWebhooks()
+	sys["Webhooks"], err = webhook.GetSystemWebhooks(util.OptionalBoolNone)
 	sys["BaseLink"] = setting.AppSubURL + "/admin/hooks"
 	sys["BaseLinkNew"] = setting.AppSubURL + "/admin/system-hooks"
 	if err != nil {

services/webhook/deliver.go

@@ -148,6 +148,8 @@ func Deliver(t *webhook_model.HookTask) error {
 		t.Delivered = time.Now().UnixNano()
 		if t.IsSucceed {
 			log.Trace("Hook delivered: %s", t.UUID)
+		} else if !w.IsActive {
+			log.Trace("Hook delivery skipped as webhook is inactive: %s", t.UUID)
 		} else {
 			log.Trace("Hook delivery failed: %s", t.UUID)
 		}
@@ -172,6 +174,10 @@ func Deliver(t *webhook_model.HookTask) error {
 		return fmt.Errorf("webhook task skipped (webhooks disabled): [%d]", t.ID)
 	}
 
+	if !w.IsActive {
+		return nil
+	}
+
 	resp, err := webhookHTTPClient.Do(req.WithContext(graceful.GetManager().ShutdownContext()))
 	if err != nil {
 		t.ResponseInfo.Body = fmt.Sprintf("Delivery: %v", err)

services/webhook/webhook.go

@@ -212,7 +212,7 @@ func prepareWebhooks(repo *repo_model.Repository, event webhook_model.HookEventT
 	}
 
 	// Add any admin-defined system webhooks
-	systemHooks, err := webhook_model.GetSystemWebhooks()
+	systemHooks, err := webhook_model.GetSystemWebhooks(util.OptionalBoolTrue)
 	if err != nil {
 		return fmt.Errorf("GetSystemWebhooks: %v", err)
 	}