Skip to content

Commit

Permalink
Refactor Webhook + Add X-Hub-Signature (go-gitea#16176)
Browse files Browse the repository at this point in the history
This PR removes multiple unneeded fields from the `HookTask` struct and adds the two headers `X-Hub-Signature` and `X-Hub-Signature-256`.

## ⚠️ BREAKING ⚠️ 

* 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 go-gitea#16115
Fixes go-gitea#7788
Fixes go-gitea#11755

Co-authored-by: zeripath <[email protected]>
  • Loading branch information
2 people authored and AbdulrhmnGhanem committed Aug 10, 2021
1 parent 31fc453 commit c50e175
Show file tree
Hide file tree
Showing 18 changed files with 130 additions and 179 deletions.
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 46 additions & 0 deletions models/migrations/v187.go
Original file line number Diff line number Diff line change
@@ -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()
}
52 changes: 23 additions & 29 deletions models/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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"`
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand All @@ -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:"-"`
Expand Down
14 changes: 0 additions & 14 deletions models/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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(),
Expand All @@ -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,
}
Expand All @@ -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(),
Expand All @@ -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(),
Expand All @@ -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,
}
Expand All @@ -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(),
Expand Down
55 changes: 0 additions & 55 deletions modules/structs/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ type EditHookOption struct {

// Payloader payload is some part of one hook
type Payloader interface {
SetSecret(string)
JSONPayload() ([]byte, error)
}

Expand Down Expand Up @@ -124,19 +123,13 @@ var (

// CreatePayload FIXME
type CreatePayload struct {
Secret string `json:"secret"`
Sha string `json:"sha"`
Ref string `json:"ref"`
RefType string `json:"ref_type"`
Repo *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the CreatePayload
func (p *CreatePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload return payload information
func (p *CreatePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -181,19 +174,13 @@ const (

// DeletePayload represents delete payload
type DeletePayload struct {
Secret string `json:"secret"`
Ref string `json:"ref"`
RefType string `json:"ref_type"`
PusherType PusherType `json:"pusher_type"`
Repo *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the DeletePayload
func (p *DeletePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *DeletePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -209,17 +196,11 @@ func (p *DeletePayload) JSONPayload() ([]byte, error) {

// ForkPayload represents fork payload
type ForkPayload struct {
Secret string `json:"secret"`
Forkee *Repository `json:"forkee"`
Repo *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the ForkPayload
func (p *ForkPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *ForkPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -238,7 +219,6 @@ const (

// IssueCommentPayload represents a payload information of issue comment event.
type IssueCommentPayload struct {
Secret string `json:"secret"`
Action HookIssueCommentAction `json:"action"`
Issue *Issue `json:"issue"`
Comment *Comment `json:"comment"`
Expand All @@ -248,11 +228,6 @@ type IssueCommentPayload struct {
IsPull bool `json:"is_pull"`
}

// SetSecret modifies the secret of the IssueCommentPayload
func (p *IssueCommentPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *IssueCommentPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -278,18 +253,12 @@ const (

// ReleasePayload represents a payload information of release event.
type ReleasePayload struct {
Secret string `json:"secret"`
Action HookReleaseAction `json:"action"`
Release *Release `json:"release"`
Repository *Repository `json:"repository"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the ReleasePayload
func (p *ReleasePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload implements Payload
func (p *ReleasePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand All @@ -305,7 +274,6 @@ func (p *ReleasePayload) JSONPayload() ([]byte, error) {

// PushPayload represents a payload information of push event.
type PushPayload struct {
Secret string `json:"secret"`
Ref string `json:"ref"`
Before string `json:"before"`
After string `json:"after"`
Expand All @@ -317,11 +285,6 @@ type PushPayload struct {
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the PushPayload
func (p *PushPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload FIXME
func (p *PushPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -389,7 +352,6 @@ const (

// IssuePayload represents the payload information that is sent along with an issue event.
type IssuePayload struct {
Secret string `json:"secret"`
Action HookIssueAction `json:"action"`
Index int64 `json:"number"`
Changes *ChangesPayload `json:"changes,omitempty"`
Expand All @@ -398,11 +360,6 @@ type IssuePayload struct {
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the IssuePayload.
func (p *IssuePayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload encodes the IssuePayload to JSON, with an indentation of two spaces.
func (p *IssuePayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -430,7 +387,6 @@ type ChangesPayload struct {

// PullRequestPayload represents a payload information of pull request event.
type PullRequestPayload struct {
Secret string `json:"secret"`
Action HookIssueAction `json:"action"`
Index int64 `json:"number"`
Changes *ChangesPayload `json:"changes,omitempty"`
Expand All @@ -440,11 +396,6 @@ type PullRequestPayload struct {
Review *ReviewPayload `json:"review"`
}

// SetSecret modifies the secret of the PullRequestPayload.
func (p *PullRequestPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload FIXME
func (p *PullRequestPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down Expand Up @@ -476,18 +427,12 @@ const (

// RepositoryPayload payload for repository webhooks
type RepositoryPayload struct {
Secret string `json:"secret"`
Action HookRepoAction `json:"action"`
Repository *Repository `json:"repository"`
Organization *User `json:"organization"`
Sender *User `json:"sender"`
}

// SetSecret modifies the secret of the RepositoryPayload
func (p *RepositoryPayload) SetSecret(secret string) {
p.Secret = secret
}

// JSONPayload JSON representation of the payload
func (p *RepositoryPayload) JSONPayload() ([]byte, error) {
json := jsoniter.ConfigCompatibleWithStandardLibrary
Expand Down
Loading

0 comments on commit c50e175

Please sign in to comment.