fix(api): keep webhook settings that are omitted in an edit request (#40282)

`PATCH .../hooks/{id}` previously reset webhook events to push-only and
cleared the branch filter and authorization header whenever they were
not included in the payload. Disabling a hook with `{"active": false}`
silently destroyed its configuration.

`BranchFilter` and `AuthorizationHeader` are now pointers so omitted
fields remain untouched, while empty strings can still be passed to
clear them explicitly.

---------

Co-authored-by: silverwind <me@silverwind.io>

modules/structs/hook.go

@@ -120,12 +120,12 @@ type CreateHookOption struct {
 type EditHookOption struct {
 	// Configuration settings for the webhook
 	Config map[string]string `json:"config"`
-	// List of events that trigger this webhook
+	// List of events that trigger this webhook, omitted or empty keeps the current events
 	Events []string `json:"events"`
-	// Branch filter pattern to determine which branches trigger the webhook
-	BranchFilter string `json:"branch_filter" binding:"GlobPattern"`
-	// Authorization header to include in webhook requests
-	AuthorizationHeader string `json:"authorization_header"`
+	// Branch filter pattern to determine which branches trigger the webhook, omitted keeps the current value
+	BranchFilter *string `json:"branch_filter" binding:"GlobPattern"`
+	// Authorization header to include in webhook requests, omitted keeps the current value unless the url changes
+	AuthorizationHeader *string `json:"authorization_header"`
 	// Whether the webhook is active and will be triggered
 	Active *bool `json:"active"`
 	// Optional human-readable name

routers/api/v1/utils/hook.go

@@ -344,6 +344,9 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *webhook.Webh
 				ctx.APIError(http.StatusUnprocessableEntity, "Invalid url")
 				return false
 			}
+			if url != w.URL {
+				w.HeaderAuthorizationEncrypted = "" // the API never reveals the header, so it must not follow the hook to a new url
+			}
 			w.URL = url
 		}
 		if ct, ok := form.Config["content_type"]; ok {
@@ -371,17 +374,20 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *webhook.Webh
 		}
 	}
 
-	// Update events
-	w.HookEvents = updateHookEvents(form.Events)
-	w.PushOnly = false
-	w.SendEverything = false
-	w.ChooseEvents = true
-	w.BranchFilter = form.BranchFilter
-
-	err := w.SetHeaderAuthorization(form.AuthorizationHeader)
-	if err != nil {
-		ctx.APIErrorInternal(err)
-		return false
+	if len(form.Events) > 0 {
+		w.HookEvents = updateHookEvents(form.Events)
+		w.PushOnly = false
+		w.SendEverything = false
+		w.ChooseEvents = true
+	}
+	if form.BranchFilter != nil {
+		w.BranchFilter = *form.BranchFilter
+	}
+	if form.AuthorizationHeader != nil {
+		if err := w.SetHeaderAuthorization(*form.AuthorizationHeader); err != nil {
+			ctx.APIErrorInternal(err)
+			return false
+		}
 	}
 
 	if err := w.UpdateEvent(); err != nil {

routers/api/v1/utils/hook_test.go

@@ -7,11 +7,15 @@ import (
 	"net/http"
 	"testing"
 
+	"gitea.dev/models/db"
 	"gitea.dev/models/unittest"
+	"gitea.dev/models/webhook"
 	"gitea.dev/modules/structs"
+	webhook_module "gitea.dev/modules/webhook"
 	"gitea.dev/services/contexttest"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestTestHookValidation(t *testing.T) {
@@ -43,3 +47,46 @@ func TestTestHookValidation(t *testing.T) {
 		})
 	}
 }
+
+func TestEditHook(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	hook := &webhook.Webhook{
+		RepoID:      1,
+		URL:         "https://example.com/webhook",
+		ContentType: webhook.ContentTypeJSON,
+		IsActive:    true,
+		HookEvent: &webhook_module.HookEvent{
+			BranchFilter: "main",
+			ChooseEvents: true,
+			HookEvents:   webhook_module.HookEvents{webhook_module.HookEventIssues: true},
+		},
+	}
+	require.NoError(t, hook.SetHeaderAuthorization("Bearer secret"))
+	require.NoError(t, hook.UpdateEvent())
+	require.NoError(t, db.Insert(t.Context(), hook))
+
+	t.Run("KeepsOmittedFields", func(t *testing.T) {
+		ctx, _ := contexttest.MockAPIContext(t, "user2/repo1/hooks")
+		assert.True(t, editHook(ctx, &structs.EditHookOption{
+			Active: new(false),
+			Events: []string{},
+			Config: map[string]string{"url": hook.URL},
+		}, hook))
+
+		updated := unittest.AssertExistsAndLoadBean(t, &webhook.Webhook{ID: hook.ID})
+		assert.False(t, updated.IsActive)
+		assert.Equal(t, "main", updated.BranchFilter)
+		assert.True(t, updated.HasEvent(webhook_module.HookEventIssues))
+		assert.False(t, updated.HasEvent(webhook_module.HookEventPush))
+		header, err := updated.HeaderAuthorization()
+		require.NoError(t, err)
+		assert.Equal(t, "Bearer secret", header)
+	})
+
+	t.Run("NewURLClearsAuthorizationHeader", func(t *testing.T) {
+		ctx, _ := contexttest.MockAPIContext(t, "user2/repo1/hooks")
+		assert.True(t, editHook(ctx, &structs.EditHookOption{Config: map[string]string{"url": "https://example.org/webhook"}}, hook))
+		assert.Empty(t, unittest.AssertExistsAndLoadBean(t, &webhook.Webhook{ID: hook.ID}).HeaderAuthorizationEncrypted)
+	})
+}

templates/swagger/v1-openapi3.generated.json

@@ -5543,12 +5543,12 @@
             "x-go-name": "Active"
           },
           "authorization_header": {
-            "description": "Authorization header to include in webhook requests",
+            "description": "Authorization header to include in webhook requests, omitted keeps the current value unless the url changes",
             "type": "string",
             "x-go-name": "AuthorizationHeader"
           },
           "branch_filter": {
-            "description": "Branch filter pattern to determine which branches trigger the webhook",
+            "description": "Branch filter pattern to determine which branches trigger the webhook, omitted keeps the current value",
             "type": "string",
             "x-go-name": "BranchFilter"
           },
@@ -5561,7 +5561,7 @@
             "x-go-name": "Config"
           },
           "events": {
-            "description": "List of events that trigger this webhook",
+            "description": "List of events that trigger this webhook, omitted or empty keeps the current events",
             "items": {
               "type": "string"
             },

templates/swagger/v1-swagger.generated.json

@@ -27932,12 +27932,12 @@
           "x-go-name": "Active"
         },
         "authorization_header": {
-          "description": "Authorization header to include in webhook requests",
+          "description": "Authorization header to include in webhook requests, omitted keeps the current value unless the url changes",
           "type": "string",
           "x-go-name": "AuthorizationHeader"
         },
         "branch_filter": {
-          "description": "Branch filter pattern to determine which branches trigger the webhook",
+          "description": "Branch filter pattern to determine which branches trigger the webhook, omitted keeps the current value",
           "type": "string",
           "x-go-name": "BranchFilter"
         },
@@ -27950,7 +27950,7 @@
           "x-go-name": "Config"
         },
         "events": {
-          "description": "List of events that trigger this webhook",
+          "description": "List of events that trigger this webhook, omitted or empty keeps the current events",
           "type": "array",
           "items": {
             "type": "string"