From d6fa138e7ce7c36ce253a3c847e3218fd31452c4 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 28 Mar 2022 04:17:21 +0100 Subject: [PATCH] Only send webhook events to active system webhooks and only deliver to active hooks (#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 Co-authored-by: Lunny Xiao --- models/webhook/webhook.go | 13 +++++++++---- routers/web/admin/hooks.go | 3 ++- services/webhook/deliver.go | 6 ++++++ services/webhook/webhook.go | 2 +- 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index ffc9b72b64d8..d61d1ed642f8 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -498,14 +498,19 @@ 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=?", 0, 0, true). + Where("repo_id=? AND org_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, true, isActive.IsTrue()). Find(&webhooks) } diff --git a/routers/web/admin/hooks.go b/routers/web/admin/hooks.go index 8cb99e1d1e57..1483d0959dbe 100644 --- a/routers/web/admin/hooks.go +++ b/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 { diff --git a/services/webhook/deliver.go b/services/webhook/deliver.go index 88b709cb41e7..f45e9d08d87b 100644 --- a/services/webhook/deliver.go +++ b/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) diff --git a/services/webhook/webhook.go b/services/webhook/webhook.go index 607fac963452..557dd147bfe8 100644 --- a/services/webhook/webhook.go +++ b/services/webhook/webhook.go @@ -214,7 +214,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) }