Skip to content

Commit d6fa138

Browse files
zeripathlunny
andauthored
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 <[email protected]> Co-authored-by: Lunny Xiao <[email protected]>
1 parent 04601d2 commit d6fa138

File tree

4 files changed

+18
-6
lines changed

4 files changed

+18
-6
lines changed

models/webhook/webhook.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -498,14 +498,19 @@ func GetSystemOrDefaultWebhook(id int64) (*Webhook, error) {
498498
}
499499

500500
// GetSystemWebhooks returns all admin system webhooks.
501-
func GetSystemWebhooks() ([]*Webhook, error) {
502-
return getSystemWebhooks(db.GetEngine(db.DefaultContext))
501+
func GetSystemWebhooks(isActive util.OptionalBool) ([]*Webhook, error) {
502+
return getSystemWebhooks(db.GetEngine(db.DefaultContext), isActive)
503503
}
504504

505-
func getSystemWebhooks(e db.Engine) ([]*Webhook, error) {
505+
func getSystemWebhooks(e db.Engine, isActive util.OptionalBool) ([]*Webhook, error) {
506506
webhooks := make([]*Webhook, 0, 5)
507+
if isActive.IsNone() {
508+
return webhooks, e.
509+
Where("repo_id=? AND org_id=? AND is_system_webhook=?", 0, 0, true).
510+
Find(&webhooks)
511+
}
507512
return webhooks, e.
508-
Where("repo_id=? AND org_id=? AND is_system_webhook=?", 0, 0, true).
513+
Where("repo_id=? AND org_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, true, isActive.IsTrue()).
509514
Find(&webhooks)
510515
}
511516

routers/web/admin/hooks.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"code.gitea.io/gitea/modules/base"
1212
"code.gitea.io/gitea/modules/context"
1313
"code.gitea.io/gitea/modules/setting"
14+
"code.gitea.io/gitea/modules/util"
1415
)
1516

1617
const (
@@ -34,7 +35,7 @@ func DefaultOrSystemWebhooks(ctx *context.Context) {
3435

3536
sys["Title"] = ctx.Tr("admin.systemhooks")
3637
sys["Description"] = ctx.Tr("admin.systemhooks.desc")
37-
sys["Webhooks"], err = webhook.GetSystemWebhooks()
38+
sys["Webhooks"], err = webhook.GetSystemWebhooks(util.OptionalBoolNone)
3839
sys["BaseLink"] = setting.AppSubURL + "/admin/hooks"
3940
sys["BaseLinkNew"] = setting.AppSubURL + "/admin/system-hooks"
4041
if err != nil {

services/webhook/deliver.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,8 @@ func Deliver(t *webhook_model.HookTask) error {
148148
t.Delivered = time.Now().UnixNano()
149149
if t.IsSucceed {
150150
log.Trace("Hook delivered: %s", t.UUID)
151+
} else if !w.IsActive {
152+
log.Trace("Hook delivery skipped as webhook is inactive: %s", t.UUID)
151153
} else {
152154
log.Trace("Hook delivery failed: %s", t.UUID)
153155
}
@@ -172,6 +174,10 @@ func Deliver(t *webhook_model.HookTask) error {
172174
return fmt.Errorf("webhook task skipped (webhooks disabled): [%d]", t.ID)
173175
}
174176

177+
if !w.IsActive {
178+
return nil
179+
}
180+
175181
resp, err := webhookHTTPClient.Do(req.WithContext(graceful.GetManager().ShutdownContext()))
176182
if err != nil {
177183
t.ResponseInfo.Body = fmt.Sprintf("Delivery: %v", err)

services/webhook/webhook.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ func prepareWebhooks(repo *repo_model.Repository, event webhook_model.HookEventT
214214
}
215215

216216
// Add any admin-defined system webhooks
217-
systemHooks, err := webhook_model.GetSystemWebhooks()
217+
systemHooks, err := webhook_model.GetSystemWebhooks(util.OptionalBoolTrue)
218218
if err != nil {
219219
return fmt.Errorf("GetSystemWebhooks: %v", err)
220220
}

0 commit comments

Comments
 (0)