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>

10 files changed, 55 insertions, 77 deletions

diff --git a/services/webhook/deliver.go b/services/webhook/deliver.go
index a417a9e846..8243fde1bb 100644
--- a/services/webhook/deliver.go
+++ b/services/webhook/deliver.go
@@ -6,8 +6,13 @@ package webhook
 
 import (
 	"context"
+	"crypto/hmac"
+	"crypto/sha1"
+	"crypto/sha256"
 	"crypto/tls"
+	"encoding/hex"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net"
 	"net/http"
@@ -26,27 +31,32 @@ import (
 
 // Deliver deliver hook task
 func Deliver(t *models.HookTask) error {
+	w, err := models.GetWebhookByID(t.HookID)
+	if err != nil {
+		return err
+	}
+
 	defer func() {
 		err := recover()
 		if err == nil {
 			return
 		}
 		// There was a panic whilst delivering a hook...
-		log.Error("PANIC whilst trying to deliver webhook[%d] for repo[%d] to %s Panic: %v\nStacktrace: %s", t.ID, t.RepoID, t.URL, err, log.Stack(2))
+		log.Error("PANIC whilst trying to deliver webhook[%d] for repo[%d] to %s Panic: %v\nStacktrace: %s", t.ID, t.RepoID, w.URL, err, log.Stack(2))
 	}()
+
 	t.IsDelivered = true
 
 	var req *http.Request
-	var err error
 
-	switch t.HTTPMethod {
+	switch w.HTTPMethod {
 	case "":
 		log.Info("HTTP Method for webhook %d empty, setting to POST as default", t.ID)
 		fallthrough
 	case http.MethodPost:
-		switch t.ContentType {
+		switch w.ContentType {
 		case models.ContentTypeJSON:
-			req, err = http.NewRequest("POST", t.URL, strings.NewReader(t.PayloadContent))
+			req, err = http.NewRequest("POST", w.URL, strings.NewReader(t.PayloadContent))
 			if err != nil {
 				return err
 			}
@@ -57,16 +67,15 @@ func Deliver(t *models.HookTask) error {
 				"payload": []string{t.PayloadContent},
 			}
 
-			req, err = http.NewRequest("POST", t.URL, strings.NewReader(forms.Encode()))
+			req, err = http.NewRequest("POST", w.URL, strings.NewReader(forms.Encode()))
 			if err != nil {
-
 				return err
 			}
 
 			req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 		}
 	case http.MethodGet:
-		u, err := url.Parse(t.URL)
+		u, err := url.Parse(w.URL)
 		if err != nil {
 			return err
 		}
@@ -78,31 +87,48 @@ func Deliver(t *models.HookTask) error {
 			return err
 		}
 	case http.MethodPut:
-		switch t.Typ {
+		switch w.Type {
 		case models.MATRIX:
-			req, err = getMatrixHookRequest(t)
+			req, err = getMatrixHookRequest(w, t)
 			if err != nil {
 				return err
 			}
 		default:
-			return fmt.Errorf("Invalid http method for webhook: [%d] %v", t.ID, t.HTTPMethod)
+			return fmt.Errorf("Invalid http method for webhook: [%d] %v", t.ID, w.HTTPMethod)
 		}
 	default:
-		return fmt.Errorf("Invalid http method for webhook: [%d] %v", t.ID, t.HTTPMethod)
+		return fmt.Errorf("Invalid http method for webhook: [%d] %v", t.ID, w.HTTPMethod)
+	}
+
+	var signatureSHA1 string
+	var signatureSHA256 string
+	if len(w.Secret) > 0 {
+		sig1 := hmac.New(sha1.New, []byte(w.Secret))
+		sig256 := hmac.New(sha256.New, []byte(w.Secret))
+		_, err = io.MultiWriter(sig1, sig256).Write([]byte(t.PayloadContent))
+		if err != nil {
+			log.Error("prepareWebhooks.sigWrite: %v", err)
+		}
+		signatureSHA1 = hex.EncodeToString(sig1.Sum(nil))
+		signatureSHA256 = hex.EncodeToString(sig256.Sum(nil))
 	}
 
 	req.Header.Add("X-Gitea-Delivery", t.UUID)
 	req.Header.Add("X-Gitea-Event", t.EventType.Event())
-	req.Header.Add("X-Gitea-Signature", t.Signature)
+	req.Header.Add("X-Gitea-Signature", signatureSHA256)
 	req.Header.Add("X-Gogs-Delivery", t.UUID)
 	req.Header.Add("X-Gogs-Event", t.EventType.Event())
-	req.Header.Add("X-Gogs-Signature", t.Signature)
+	req.Header.Add("X-Gogs-Signature", signatureSHA256)
+	req.Header.Add("X-Hub-Signature", "sha1="+signatureSHA1)
+	req.Header.Add("X-Hub-Signature-256", "sha256="+signatureSHA256)
 	req.Header["X-GitHub-Delivery"] = []string{t.UUID}
 	req.Header["X-GitHub-Event"] = []string{t.EventType.Event()}
 
 	// Record delivery information.
 	t.RequestInfo = &models.HookRequest{
-		Headers: map[string]string{},
+		URL:        req.URL.String(),
+		HTTPMethod: req.Method,
+		Headers:    map[string]string{},
 	}
 	for k, vals := range req.Header {
 		t.RequestInfo.Headers[k] = strings.Join(vals, ",")
@@ -125,11 +151,6 @@ func Deliver(t *models.HookTask) error {
 		}
 
 		// Update webhook last delivery status.
-		w, err := models.GetWebhookByID(t.HookID)
-		if err != nil {
-			log.Error("GetWebhookByID: %v", err)
-			return
-		}
 		if t.IsSucceed {
 			w.LastStatus = models.HookStatusSucceed
 		} else {
diff --git a/services/webhook/dingtalk.go b/services/webhook/dingtalk.go
index d781b8c87d..49e161ea57 100644
--- a/services/webhook/dingtalk.go
+++ b/services/webhook/dingtalk.go
@@ -25,9 +25,6 @@ var (
 	_ PayloadConvertor = &DingtalkPayload{}
 )
 
-// SetSecret sets the dingtalk secret
-func (d *DingtalkPayload) SetSecret(_ string) {}
-
 // JSONPayload Marshals the DingtalkPayload to json
 func (d *DingtalkPayload) JSONPayload() ([]byte, error) {
 	json := jsoniter.ConfigCompatibleWithStandardLibrary
diff --git a/services/webhook/discord.go b/services/webhook/discord.go
index 378d9ff725..ea3879f198 100644
--- a/services/webhook/discord.go
+++ b/services/webhook/discord.go
@@ -97,9 +97,6 @@ var (
 	redColor         = color("ff3232")
 )
 
-// SetSecret sets the discord secret
-func (d *DiscordPayload) SetSecret(_ string) {}
-
 // JSONPayload Marshals the DiscordPayload to json
 func (d *DiscordPayload) JSONPayload() ([]byte, error) {
 	json := jsoniter.ConfigCompatibleWithStandardLibrary
diff --git a/services/webhook/feishu.go b/services/webhook/feishu.go
index 5c80efb820..b280e67759 100644
--- a/services/webhook/feishu.go
+++ b/services/webhook/feishu.go
@@ -35,9 +35,6 @@ func newFeishuTextPayload(text string) *FeishuPayload {
 	}
 }
 
-// SetSecret sets the Feishu secret
-func (f *FeishuPayload) SetSecret(_ string) {}
-
 // JSONPayload Marshals the FeishuPayload to json
 func (f *FeishuPayload) JSONPayload() ([]byte, error) {
 	json := jsoniter.ConfigCompatibleWithStandardLibrary
diff --git a/services/webhook/matrix.go b/services/webhook/matrix.go
index 1658dd4b44..6fca67ca84 100644
--- a/services/webhook/matrix.go
+++ b/services/webhook/matrix.go
@@ -76,9 +76,6 @@ type MatrixPayloadSafe struct {
 	Commits       []*api.PayloadCommit `json:"io.gitea.commits,omitempty"`
 }
 
-// SetSecret sets the Matrix secret
-func (m *MatrixPayloadUnsafe) SetSecret(_ string) {}
-
 // JSONPayload Marshals the MatrixPayloadUnsafe to json
 func (m *MatrixPayloadUnsafe) JSONPayload() ([]byte, error) {
 	json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -263,7 +260,7 @@ func getMessageBody(htmlText string) string {
 
 // getMatrixHookRequest creates a new request which contains an Authorization header.
 // The access_token is removed from t.PayloadContent
-func getMatrixHookRequest(t *models.HookTask) (*http.Request, error) {
+func getMatrixHookRequest(w *models.Webhook, t *models.HookTask) (*http.Request, error) {
 	payloadunsafe := MatrixPayloadUnsafe{}
 	json := jsoniter.ConfigCompatibleWithStandardLibrary
 	if err := json.Unmarshal([]byte(t.PayloadContent), &payloadunsafe); err != nil {
@@ -288,9 +285,9 @@ func getMatrixHookRequest(t *models.HookTask) (*http.Request, error) {
 		return nil, fmt.Errorf("getMatrixHookRequest: unable to hash payload: %+v", err)
 	}
 
-	t.URL = fmt.Sprintf("%s/%s", t.URL, txnID)
+	url := fmt.Sprintf("%s/%s", w.URL, txnID)
 
-	req, err := http.NewRequest(t.HTTPMethod, t.URL, strings.NewReader(string(payload)))
+	req, err := http.NewRequest(w.HTTPMethod, url, strings.NewReader(string(payload)))
 	if err != nil {
 		return nil, err
 	}
diff --git a/services/webhook/matrix_test.go b/services/webhook/matrix_test.go
index 7b10e21cfa..451dff6949 100644
--- a/services/webhook/matrix_test.go
+++ b/services/webhook/matrix_test.go
@@ -184,6 +184,8 @@ func TestMatrixJSONPayload(t *testing.T) {
 }
 
 func TestMatrixHookRequest(t *testing.T) {
+	w := &models.Webhook{}
+
 	h := &models.HookTask{
 		PayloadContent: `{
   "body": "[[user1/test](http://localhost:3000/user1/test)] user1 pushed 1 commit to [master](http://localhost:3000/user1/test/src/branch/master):\n[5175ef2](http://localhost:3000/user1/test/commit/5175ef26201c58b035a3404b3fe02b4e8d436eee): Merge pull request 'Change readme.md' (#2) from add-matrix-webhook into master\n\nReviewed-on: http://localhost:3000/user1/test/pulls/2\n - user1",
@@ -245,7 +247,7 @@ func TestMatrixHookRequest(t *testing.T) {
   ]
 }`
 
-	req, err := getMatrixHookRequest(h)
+	req, err := getMatrixHookRequest(w, h)
 	require.NoError(t, err)
 	require.NotNil(t, req)
 
diff --git a/services/webhook/msteams.go b/services/webhook/msteams.go
index 51bb28a361..035dbc1c4c 100644
--- a/services/webhook/msteams.go
+++ b/services/webhook/msteams.go
@@ -55,9 +55,6 @@ type (
 	}
 )
 
-// SetSecret sets the MSTeams secret
-func (m *MSTeamsPayload) SetSecret(_ string) {}
-
 // JSONPayload Marshals the MSTeamsPayload to json
 func (m *MSTeamsPayload) JSONPayload() ([]byte, error) {
 	json := jsoniter.ConfigCompatibleWithStandardLibrary
diff --git a/services/webhook/slack.go b/services/webhook/slack.go
index 1de9c9c37c..f522ca35f2 100644
--- a/services/webhook/slack.go
+++ b/services/webhook/slack.go
@@ -56,9 +56,6 @@ type SlackAttachment struct {
 	Text      string `json:"text"`
 }
 
-// SetSecret sets the slack secret
-func (s *SlackPayload) SetSecret(_ string) {}
-
 // JSONPayload Marshals the SlackPayload to json
 func (s *SlackPayload) JSONPayload() ([]byte, error) {
 	json := jsoniter.ConfigCompatibleWithStandardLibrary
diff --git a/services/webhook/telegram.go b/services/webhook/telegram.go
index f71352141d..4c4230759d 100644
--- a/services/webhook/telegram.go
+++ b/services/webhook/telegram.go
@@ -45,9 +45,6 @@ var (
 	_ PayloadConvertor = &TelegramPayload{}
 )
 
-// SetSecret sets the telegram secret
-func (t *TelegramPayload) SetSecret(_ string) {}
-
 // JSONPayload Marshals the TelegramPayload to json
 func (t *TelegramPayload) JSONPayload() ([]byte, error) {
 	t.ParseMode = "HTML"
diff --git a/services/webhook/webhook.go b/services/webhook/webhook.go
index cc79ec15d1..d094a7754b 100644
--- a/services/webhook/webhook.go
+++ b/services/webhook/webhook.go
@@ -5,9 +5,6 @@
 package webhook
 
 import (
-	"crypto/hmac"
-	"crypto/sha256"
-	"encoding/hex"
 	"fmt"
 	"strings"
 
@@ -21,12 +18,12 @@ import (
 )
 
 type webhook struct {
-	name           models.HookTaskType
+	name           models.HookType
 	payloadCreator func(p api.Payloader, event models.HookEventType, meta string) (api.Payloader, error)
 }
 
 var (
-	webhooks = map[models.HookTaskType]*webhook{
+	webhooks = map[models.HookType]*webhook{
 		models.SLACK: {
 			name:           models.SLACK,
 			payloadCreator: GetSlackPayload,
@@ -60,7 +57,7 @@ var (
 
 // RegisterWebhook registers a webhook
 func RegisterWebhook(name string, webhook *webhook) {
-	webhooks[models.HookTaskType(name)] = webhook
+	webhooks[models.HookType(name)] = webhook
 }
 
 // IsValidHookTaskType returns true if a webhook registered
@@ -68,7 +65,7 @@ func IsValidHookTaskType(name string) bool {
 	if name == models.GITEA || name == models.GOGS {
 		return true
 	}
-	_, ok := webhooks[models.HookTaskType(name)]
+	_, ok := webhooks[models.HookType(name)]
 	return ok
 }
 
@@ -161,35 +158,14 @@ func prepareWebhook(w *models.Webhook, repo *models.Repository, event models.Hoo
 			return fmt.Errorf("create payload for %s[%s]: %v", w.Type, event, err)
 		}
 	} else {
-		p.SetSecret(w.Secret)
 		payloader = p
 	}
 
-	var signature string
-	if len(w.Secret) > 0 {
-		data, err := payloader.JSONPayload()
-		if err != nil {
-			log.Error("prepareWebhooks.JSONPayload: %v", err)
-		}
-		sig := hmac.New(sha256.New, []byte(w.Secret))
-		_, err = sig.Write(data)
-		if err != nil {
-			log.Error("prepareWebhooks.sigWrite: %v", err)
-		}
-		signature = hex.EncodeToString(sig.Sum(nil))
-	}
-
 	if err = models.CreateHookTask(&models.HookTask{
-		RepoID:      repo.ID,
-		HookID:      w.ID,
-		Typ:         w.Type,
-		URL:         w.URL,
-		Signature:   signature,
-		Payloader:   payloader,
-		HTTPMethod:  w.HTTPMethod,
-		ContentType: w.ContentType,
-		EventType:   event,
-		IsSSL:       w.IsSSL,
+		RepoID:    repo.ID,
+		HookID:    w.ID,
+		Payloader: payloader,
+		EventType: event,
 	}); err != nil {
 		return fmt.Errorf("CreateHookTask: %v", err)
 	}