mirror of
				https://gitee.com/gitea/gitea
				synced 2025-11-04 08:30:25 +08:00 
			
		
		
		
	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 <art27@cantab.net> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
This commit is contained in:
		@@ -498,14 +498,19 @@ func GetSystemOrDefaultWebhook(id int64) (*Webhook, error) {
 | 
				
			|||||||
}
 | 
					}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
// GetSystemWebhooks returns all admin system webhooks.
 | 
					// GetSystemWebhooks returns all admin system webhooks.
 | 
				
			||||||
func GetSystemWebhooks() ([]*Webhook, error) {
 | 
					func GetSystemWebhooks(isActive util.OptionalBool) ([]*Webhook, error) {
 | 
				
			||||||
	return getSystemWebhooks(db.GetEngine(db.DefaultContext))
 | 
						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)
 | 
						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.
 | 
						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)
 | 
							Find(&webhooks)
 | 
				
			||||||
}
 | 
					}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -11,6 +11,7 @@ import (
 | 
				
			|||||||
	"code.gitea.io/gitea/modules/base"
 | 
						"code.gitea.io/gitea/modules/base"
 | 
				
			||||||
	"code.gitea.io/gitea/modules/context"
 | 
						"code.gitea.io/gitea/modules/context"
 | 
				
			||||||
	"code.gitea.io/gitea/modules/setting"
 | 
						"code.gitea.io/gitea/modules/setting"
 | 
				
			||||||
 | 
						"code.gitea.io/gitea/modules/util"
 | 
				
			||||||
)
 | 
					)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
const (
 | 
					const (
 | 
				
			||||||
@@ -34,7 +35,7 @@ func DefaultOrSystemWebhooks(ctx *context.Context) {
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
	sys["Title"] = ctx.Tr("admin.systemhooks")
 | 
						sys["Title"] = ctx.Tr("admin.systemhooks")
 | 
				
			||||||
	sys["Description"] = ctx.Tr("admin.systemhooks.desc")
 | 
						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["BaseLink"] = setting.AppSubURL + "/admin/hooks"
 | 
				
			||||||
	sys["BaseLinkNew"] = setting.AppSubURL + "/admin/system-hooks"
 | 
						sys["BaseLinkNew"] = setting.AppSubURL + "/admin/system-hooks"
 | 
				
			||||||
	if err != nil {
 | 
						if err != nil {
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -148,6 +148,8 @@ func Deliver(t *webhook_model.HookTask) error {
 | 
				
			|||||||
		t.Delivered = time.Now().UnixNano()
 | 
							t.Delivered = time.Now().UnixNano()
 | 
				
			||||||
		if t.IsSucceed {
 | 
							if t.IsSucceed {
 | 
				
			||||||
			log.Trace("Hook delivered: %s", t.UUID)
 | 
								log.Trace("Hook delivered: %s", t.UUID)
 | 
				
			||||||
 | 
							} else if !w.IsActive {
 | 
				
			||||||
 | 
								log.Trace("Hook delivery skipped as webhook is inactive: %s", t.UUID)
 | 
				
			||||||
		} else {
 | 
							} else {
 | 
				
			||||||
			log.Trace("Hook delivery failed: %s", t.UUID)
 | 
								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)
 | 
							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()))
 | 
						resp, err := webhookHTTPClient.Do(req.WithContext(graceful.GetManager().ShutdownContext()))
 | 
				
			||||||
	if err != nil {
 | 
						if err != nil {
 | 
				
			||||||
		t.ResponseInfo.Body = fmt.Sprintf("Delivery: %v", err)
 | 
							t.ResponseInfo.Body = fmt.Sprintf("Delivery: %v", err)
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -214,7 +214,7 @@ func prepareWebhooks(repo *repo_model.Repository, event webhook_model.HookEventT
 | 
				
			|||||||
	}
 | 
						}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
	// Add any admin-defined system webhooks
 | 
						// Add any admin-defined system webhooks
 | 
				
			||||||
	systemHooks, err := webhook_model.GetSystemWebhooks()
 | 
						systemHooks, err := webhook_model.GetSystemWebhooks(util.OptionalBoolTrue)
 | 
				
			||||||
	if err != nil {
 | 
						if err != nil {
 | 
				
			||||||
		return fmt.Errorf("GetSystemWebhooks: %v", err)
 | 
							return fmt.Errorf("GetSystemWebhooks: %v", err)
 | 
				
			||||||
	}
 | 
						}
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user