Refactor Webhook + Add X-Hub-Signature (#16176)

This PR removes multiple unneeded fields from the `HookTask` struct and adds the two headers `X-Hub-Signature` and `X-Hub-Signature-256`.

## :warning: BREAKING :warning: 

* The `Secret` field is no longer passed as part of the payload.
* "Breaking" change (or fix?): The webhook history shows the real called url and not the url registered in the webhook (`deliver.go`@129).

Close #16115
Fixes #7788
Fixes #11755

Co-authored-by: zeripath <art27@cantab.net>

4 files changed, 71 insertions, 43 deletions

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 978ba6368f..0e8b0d0e3d 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -323,6 +323,8 @@ var migrations = []Migration{
 	NewMigration("Add new table repo_archiver", addRepoArchiver),
 	// v186 -> v187
 	NewMigration("Create protected tag table", createProtectedTagTable),
+	// v187 -> v188
+	NewMigration("Drop unneeded webhook related columns", dropWebhookColumns),
 }
 
 // GetCurrentDBVersion returns the current db version
diff --git a/models/migrations/v187.go b/models/migrations/v187.go
new file mode 100644
index 0000000000..627423717a
--- /dev/null
+++ b/models/migrations/v187.go
@@ -0,0 +1,46 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"xorm.io/xorm"
+)
+
+func dropWebhookColumns(x *xorm.Engine) error {
+	// Make sure the columns exist before dropping them
+	type Webhook struct {
+		Signature string `xorm:"TEXT"`
+		IsSSL     bool   `xorm:"is_ssl"`
+	}
+	if err := x.Sync2(new(Webhook)); err != nil {
+		return err
+	}
+
+	type HookTask struct {
+		Typ         string `xorm:"VARCHAR(16) index"`
+		URL         string `xorm:"TEXT"`
+		Signature   string `xorm:"TEXT"`
+		HTTPMethod  string `xorm:"http_method"`
+		ContentType int
+		IsSSL       bool
+	}
+	if err := x.Sync2(new(HookTask)); err != nil {
+		return err
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+	if err := dropTableColumns(sess, "webhook", "signature", "is_ssl"); err != nil {
+		return err
+	}
+	if err := dropTableColumns(sess, "hook_task", "typ", "url", "signature", "http_method", "content_type", "is_ssl"); err != nil {
+		return err
+	}
+
+	return sess.Commit()
+}
diff --git a/models/webhook.go b/models/webhook.go
index 24510cc6f7..29cfcf6ed4 100644
--- a/models/webhook.go
+++ b/models/webhook.go
@@ -109,6 +109,22 @@ type HookEvent struct {
 	HookEvents `json:"events"`
 }
 
+// HookType is the type of a webhook
+type HookType = string
+
+// Types of webhooks
+const (
+	GITEA    HookType = "gitea"
+	GOGS     HookType = "gogs"
+	SLACK    HookType = "slack"
+	DISCORD  HookType = "discord"
+	DINGTALK HookType = "dingtalk"
+	TELEGRAM HookType = "telegram"
+	MSTEAMS  HookType = "msteams"
+	FEISHU   HookType = "feishu"
+	MATRIX   HookType = "matrix"
+)
+
 // HookStatus is the status of a web hook
 type HookStatus int
 
@@ -126,17 +142,15 @@ type Webhook struct {
 	OrgID           int64 `xorm:"INDEX"`
 	IsSystemWebhook bool
 	URL             string `xorm:"url TEXT"`
-	Signature       string `xorm:"TEXT"`
 	HTTPMethod      string `xorm:"http_method"`
 	ContentType     HookContentType
 	Secret          string `xorm:"TEXT"`
 	Events          string `xorm:"TEXT"`
 	*HookEvent      `xorm:"-"`
-	IsSSL           bool         `xorm:"is_ssl"`
-	IsActive        bool         `xorm:"INDEX"`
-	Type            HookTaskType `xorm:"VARCHAR(16) 'type'"`
-	Meta            string       `xorm:"TEXT"` // store hook-specific attributes
-	LastStatus      HookStatus   // Last delivery status
+	IsActive        bool       `xorm:"INDEX"`
+	Type            HookType   `xorm:"VARCHAR(16) 'type'"`
+	Meta            string     `xorm:"TEXT"` // store hook-specific attributes
+	LastStatus      HookStatus // Last delivery status
 
 	CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
 	UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
@@ -558,22 +572,6 @@ func copyDefaultWebhooksToRepo(e Engine, repoID int64) error {
 //  \___|_  / \____/ \____/|__|_ \ |____|  (____  /____  >__|_ \
 //        \/                    \/              \/     \/     \/
 
-// HookTaskType is the type of an hook task
-type HookTaskType = string
-
-// Types of hook tasks
-const (
-	GITEA    HookTaskType = "gitea"
-	GOGS     HookTaskType = "gogs"
-	SLACK    HookTaskType = "slack"
-	DISCORD  HookTaskType = "discord"
-	DINGTALK HookTaskType = "dingtalk"
-	TELEGRAM HookTaskType = "telegram"
-	MSTEAMS  HookTaskType = "msteams"
-	FEISHU   HookTaskType = "feishu"
-	MATRIX   HookTaskType = "matrix"
-)
-
 // HookEventType is the type of an hook event
 type HookEventType string
 
@@ -635,7 +633,9 @@ func (h HookEventType) Event() string {
 
 // HookRequest represents hook task request information.
 type HookRequest struct {
-	Headers map[string]string `json:"headers"`
+	URL        string            `json:"url"`
+	HTTPMethod string            `json:"http_method"`
+	Headers    map[string]string `json:"headers"`
 }
 
 // HookResponse represents hook task response information.
@@ -651,15 +651,9 @@ type HookTask struct {
 	RepoID          int64 `xorm:"INDEX"`
 	HookID          int64
 	UUID            string
-	Typ             HookTaskType `xorm:"VARCHAR(16) index"`
-	URL             string       `xorm:"TEXT"`
-	Signature       string       `xorm:"TEXT"`
 	api.Payloader   `xorm:"-"`
 	PayloadContent  string `xorm:"TEXT"`
-	HTTPMethod      string `xorm:"http_method"`
-	ContentType     HookContentType
 	EventType       HookEventType
-	IsSSL           bool
 	IsDelivered     bool
 	Delivered       int64
 	DeliveredString string `xorm:"-"`
diff --git a/models/webhook_test.go b/models/webhook_test.go
index 88b2d40a39..cab44a120e 100644
--- a/models/webhook_test.go
+++ b/models/webhook_test.go
@@ -207,8 +207,6 @@ func TestCreateHookTask(t *testing.T) {
 	hookTask := &HookTask{
 		RepoID:    3,
 		HookID:    3,
-		Typ:       GITEA,
-		URL:       "http://www.example.com/unit_test",
 		Payloader: &api.PushPayload{},
 	}
 	AssertNotExistsBean(t, hookTask)
@@ -233,8 +231,6 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) {
 	hookTask := &HookTask{
 		RepoID:      3,
 		HookID:      3,
-		Typ:         GITEA,
-		URL:         "http://www.example.com/unit_test",
 		Payloader:   &api.PushPayload{},
 		IsDelivered: true,
 		Delivered:   time.Now().UnixNano(),
@@ -252,8 +248,6 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) {
 	hookTask := &HookTask{
 		RepoID:      2,
 		HookID:      4,
-		Typ:         GITEA,
-		URL:         "http://www.example.com/unit_test",
 		Payloader:   &api.PushPayload{},
 		IsDelivered: false,
 	}
@@ -270,8 +264,6 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) {
 	hookTask := &HookTask{
 		RepoID:      2,
 		HookID:      4,
-		Typ:         GITEA,
-		URL:         "http://www.example.com/unit_test",
 		Payloader:   &api.PushPayload{},
 		IsDelivered: true,
 		Delivered:   time.Now().UnixNano(),
@@ -289,8 +281,6 @@ func TestCleanupHookTaskTable_OlderThan_DeletesDelivered(t *testing.T) {
 	hookTask := &HookTask{
 		RepoID:      3,
 		HookID:      3,
-		Typ:         GITEA,
-		URL:         "http://www.example.com/unit_test",
 		Payloader:   &api.PushPayload{},
 		IsDelivered: true,
 		Delivered:   time.Now().AddDate(0, 0, -8).UnixNano(),
@@ -308,8 +298,6 @@ func TestCleanupHookTaskTable_OlderThan_LeavesUndelivered(t *testing.T) {
 	hookTask := &HookTask{
 		RepoID:      2,
 		HookID:      4,
-		Typ:         GITEA,
-		URL:         "http://www.example.com/unit_test",
 		Payloader:   &api.PushPayload{},
 		IsDelivered: false,
 	}
@@ -326,8 +314,6 @@ func TestCleanupHookTaskTable_OlderThan_LeavesTaskEarlierThanAgeToDelete(t *test
 	hookTask := &HookTask{
 		RepoID:      2,
 		HookID:      4,
-		Typ:         GITEA,
-		URL:         "http://www.example.com/unit_test",
 		Payloader:   &api.PushPayload{},
 		IsDelivered: true,
 		Delivered:   time.Now().AddDate(0, 0, -6).UnixNano(),