fix: make "test push webhook" always work (#38425) (#38455)

Backport #38425

* fix #38309
* fix #26238
* fix #37886

Co-authored-by: Harsh Satyajit Thakur <f20240223@goa.bits-pilani.ac.in>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

options/locale/locale_en-US.json

@@ -2251,7 +2251,6 @@
   "repo.settings.webhook_deletion_success": "The webhook has been removed.",
   "repo.settings.webhook.test_delivery": "Test Push Event",
   "repo.settings.webhook.test_delivery_desc": "Test this webhook with a fake push event.",
-  "repo.settings.webhook.test_delivery_desc_disabled": "To test this webhook with a fake event, activate it.",
   "repo.settings.webhook.request": "Request",
   "repo.settings.webhook.response": "Response",
   "repo.settings.webhook.headers": "Headers",

routers/api/v1/repo/hook.go

@@ -177,7 +177,7 @@ func TestHook(ctx *context.APIContext) {
 	commit := convert.ToPayloadCommit(ctx, ctx.Repo.Repository, ctx.Repo.Commit)
 
 	commitID := ctx.Repo.Commit.ID.String()
-	if err := webhook_service.PrepareWebhook(ctx, hook, webhook_module.HookEventPush, &api.PushPayload{
+	if err := webhook_service.PrepareTestWebhook(ctx, hook, webhook_module.HookEventPush, &api.PushPayload{
 		Ref:          ref,
 		Before:       commitID,
 		After:        commitID,

routers/web/repo/setting/webhook.go

@@ -664,19 +664,14 @@ func TestWebhook(ctx *context.Context) {
 		return
 	}
 
-	// Grab latest commit or fake one if it's empty repository.
-	// Note: in old code, the "ctx.Repo.Commit" is the last commit of the default branch.
-	// New code doesn't set that commit, so it always uses the fake commit to test webhook.
-	commit := ctx.Repo.Commit
-	if commit == nil {
-		ghost := user_model.NewGhostUser()
-		objectFormat := git.ObjectFormatFromName(ctx.Repo.Repository.ObjectFormatName)
-		commit = &git.Commit{
-			ID:            objectFormat.EmptyObjectID(),
-			Author:        ghost.NewGitSig(),
-			Committer:     ghost.NewGitSig(),
-			CommitMessage: git.CommitMessage{MessageRaw: "This is a fake commit"},
-		}
+	// use a fake commit to test webhook
+	ghostUser := user_model.NewGhostUser()
+	objectFormat := git.ObjectFormatFromName(ctx.Repo.Repository.ObjectFormatName)
+	commit := &git.Commit{
+		ID:            objectFormat.EmptyObjectID(),
+		Author:        ghostUser.NewGitSig(),
+		Committer:     ghostUser.NewGitSig(),
+		CommitMessage: git.CommitMessage{MessageRaw: "This is a fake commit for webhook push test"},
 	}
 
 	apiUser := convert.ToUserWithAccessMode(ctx, ctx.Doer, perm.AccessModeNone)
@@ -697,7 +692,7 @@ func TestWebhook(ctx *context.Context) {
 
 	commitID := commit.ID.String()
 	p := &api.PushPayload{
-		Ref:          git.BranchPrefix + ctx.Repo.Repository.DefaultBranch,
+		Ref:          git.RefNameFromBranch(ctx.Repo.Repository.DefaultBranch).String(),
 		Before:       commitID,
 		After:        commitID,
 		CompareURL:   setting.AppURL + ctx.Repo.Repository.ComposeCompareURL(commitID, commitID),
@@ -708,8 +703,8 @@ func TestWebhook(ctx *context.Context) {
 		Pusher:       apiUser,
 		Sender:       apiUser,
 	}
-	if err := webhook_service.PrepareWebhook(ctx, w, webhook_module.HookEventPush, p); err != nil {
-		ctx.Flash.Error("PrepareWebhook: " + err.Error())
+	if err := webhook_service.PrepareTestWebhook(ctx, w, webhook_module.HookEventPush, p); err != nil {
+		ctx.Flash.Error("PrepareTestWebhook: " + err.Error())
 		ctx.Status(http.StatusInternalServerError)
 	} else {
 		ctx.Flash.Info(ctx.Tr("repo.settings.webhook.delivery.success"))

services/webhook/webhook.go

@@ -129,6 +129,33 @@ func checkBranchFilter(branchFilter string, ref git.RefName) bool {
 	return g.Match(ref.String())
 }
 
+// PrepareTestWebhook always creates and enqueues a hook task for manual testing.
+// Unlike PrepareWebhook, it ignores event subscriptions and branch filters so the
+// Test Push Event control can verify delivery even when those gates would suppress
+// a real event.
+func PrepareTestWebhook(ctx context.Context, w *webhook_model.Webhook, event webhook_module.HookEventType, p api.Payloader) error {
+	if setting.DisableWebhooks {
+		return nil
+	}
+
+	payload, err := p.JSONPayload()
+	if err != nil {
+		return fmt.Errorf("JSONPayload for %s: %w", event, err)
+	}
+
+	task, err := webhook_model.CreateHookTask(ctx, &webhook_model.HookTask{
+		HookID:         w.ID,
+		PayloadContent: string(payload),
+		EventType:      event,
+		PayloadVersion: 2,
+	})
+	if err != nil {
+		return fmt.Errorf("CreateHookTask for %s: %w", event, err)
+	}
+
+	return enqueueHookTask(task.ID)
+}
+
 // PrepareWebhook creates a hook task and enqueues it for processing.
 // The payload is saved as-is. The adjustments depending on the webhook type happen
 // right before delivery, in the [Deliver] method.

services/webhook/webhook_test.go

@@ -30,6 +30,7 @@ func TestWebhookService(t *testing.T) {
 	t.Run("PrepareBranchFilterNoMatch", testWebhookPrepareBranchFilterNoMatch)
 	t.Run("WebhookUserMail", testWebhookUserMail)
 	t.Run("CheckBranchFilter", testWebhookCheckBranchFilter)
+	t.Run("PrepareTestWebhookIgnoresGates", testPrepareTestWebhookIgnoresGates)
 }
 
 func testWebhookGetSlackHook(t *testing.T) {
@@ -132,3 +133,37 @@ func testWebhookCheckBranchFilter(t *testing.T) {
 		assert.Equal(t, v.match, checkBranchFilter(v.filter, v.ref), "filter: %q ref: %q", v.filter, v.ref)
 	}
 }
+
+func testPrepareTestWebhookIgnoresGates(t *testing.T) {
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+	hook := &webhook_model.Webhook{
+		RepoID:      repo.ID,
+		URL:         "http://localhost/gitea-webhook-test-prepare_test_webhook",
+		ContentType: webhook_model.ContentTypeJSON,
+		IsActive:    true,
+		HookEvent: &webhook_module.HookEvent{
+			ChooseEvents: true,
+			BranchFilter: "dev",
+			HookEvents: webhook_module.HookEvents{
+				webhook_module.HookEventWorkflowRun: true,
+			},
+		},
+	}
+	require.NoError(t, hook.UpdateEvent())
+	require.NoError(t, db.Insert(t.Context(), hook))
+
+	payload := &api.PushPayload{
+		Ref:     "refs/heads/master",
+		Commits: []*api.PayloadCommit{{}},
+	}
+	hookTask := &webhook_model.HookTask{HookID: hook.ID, EventType: webhook_module.HookEventPush}
+
+	// Real deliveries stay gated: no push event + branch filter mismatch => nothing queued.
+	unittest.AssertNotExistsBean(t, hookTask)
+	require.NoError(t, PrepareWebhook(t.Context(), hook, webhook_module.HookEventPush, payload))
+	unittest.AssertNotExistsBean(t, hookTask)
+
+	// Manual test delivery always queues so the endpoint can be verified.
+	require.NoError(t, PrepareTestWebhook(t.Context(), hook, webhook_module.HookEventPush, payload))
+	unittest.AssertExistsAndLoadBean(t, hookTask)
+}

templates/repo/settings/webhook/history.tmpl

@@ -1,16 +1,12 @@
-{{$isNew:=or .PageIsSettingsHooksNew .PageIsAdminDefaultHooksNew .PageIsAdminSystemHooksNew}}
 {{if .PageIsSettingsHooksEdit}}
-	<h4 class="ui top attached header">
-		{{ctx.Locale.Tr "repo.settings.recent_deliveries"}}
+	<h4 class="ui top attached header flex-left-right">
+		<span>{{ctx.Locale.Tr "repo.settings.recent_deliveries"}}</span>
 		{{if .Permission.IsAdmin}}
-			<div class="ui right">
-				<!-- the button is wrapped with a span because the tooltip doesn't show on hover if we put data-tooltip-content directly on the button -->
-				<span data-tooltip-content="{{if or $isNew .Webhook.IsActive}}{{ctx.Locale.Tr "repo.settings.webhook.test_delivery_desc"}}{{else}}{{ctx.Locale.Tr "repo.settings.webhook.test_delivery_desc_disabled"}}{{end}}">
-					<button class="ui tiny button{{if not (or $isNew .Webhook.IsActive)}} disabled{{end}}" id="test-delivery" data-link="{{.Link}}/test">
-						<span class="text">{{ctx.Locale.Tr "repo.settings.webhook.test_delivery"}}</span>
-					</button>
-			</span>
-			</div>
+			<button class="ui tiny button" id="test-delivery" data-link="{{.Link}}/test"
+				data-tooltip-content="{{ctx.Locale.Tr "repo.settings.webhook.test_delivery_desc"}}"
+			>
+				{{ctx.Locale.Tr "repo.settings.webhook.test_delivery"}}
+			</button>
 		{{end}}
 	</h4>
 	<div class="ui attached segment">