Skip to content

Commit 9b1b4b5

Browse files
KN4CK3Rzeripath
KN4CK3R
and
zeripath
authored
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`. ## ⚠️ 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 #16115 Fixes #7788 Fixes #11755 Co-authored-by: zeripath <art27@cantab.net>
1 parent 0b27b93 commit 9b1b4b5

18 files changed

Lines changed: 130 additions & 179 deletions

File tree

‎models/migrations/migrations.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,8 @@ var migrations = []Migration{
323323
NewMigration("Add new table repo_archiver", addRepoArchiver),
324324
// v186 -> v187
325325
NewMigration("Create protected tag table", createProtectedTagTable),
326+
// v187 -> v188
327+
NewMigration("Drop unneeded webhook related columns", dropWebhookColumns),
326328
}
327329

328330
// GetCurrentDBVersion returns the current db version

‎models/migrations/v187.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Copyright 2021 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package migrations
6+
7+
import (
8+
"xorm.io/xorm"
9+
)
10+
11+
func dropWebhookColumns(x *xorm.Engine) error {
12+
// Make sure the columns exist before dropping them
13+
type Webhook struct {
14+
Signature string `xorm:"TEXT"`
15+
IsSSL bool `xorm:"is_ssl"`
16+
}
17+
if err := x.Sync2(new(Webhook)); err != nil {
18+
return err
19+
}
20+
21+
type HookTask struct {
22+
Typ string `xorm:"VARCHAR(16) index"`
23+
URL string `xorm:"TEXT"`
24+
Signature string `xorm:"TEXT"`
25+
HTTPMethod string `xorm:"http_method"`
26+
ContentType int
27+
IsSSL bool
28+
}
29+
if err := x.Sync2(new(HookTask)); err != nil {
30+
return err
31+
}
32+
33+
sess := x.NewSession()
34+
defer sess.Close()
35+
if err := sess.Begin(); err != nil {
36+
return err
37+
}
38+
if err := dropTableColumns(sess, "webhook", "signature", "is_ssl"); err != nil {
39+
return err
40+
}
41+
if err := dropTableColumns(sess, "hook_task", "typ", "url", "signature", "http_method", "content_type", "is_ssl"); err != nil {
42+
return err
43+
}
44+
45+
return sess.Commit()
46+
}

‎models/webhook.go‎

Lines changed: 23 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,22 @@ type HookEvent struct {
109109
HookEvents `json:"events"`
110110
}
111111

112+
// HookType is the type of a webhook
113+
type HookType = string
114+
115+
// Types of webhooks
116+
const (
117+
GITEA HookType = "gitea"
118+
GOGS HookType = "gogs"
119+
SLACK HookType = "slack"
120+
DISCORD HookType = "discord"
121+
DINGTALK HookType = "dingtalk"
122+
TELEGRAM HookType = "telegram"
123+
MSTEAMS HookType = "msteams"
124+
FEISHU HookType = "feishu"
125+
MATRIX HookType = "matrix"
126+
)
127+
112128
// HookStatus is the status of a web hook
113129
type HookStatus int
114130

@@ -126,17 +142,15 @@ type Webhook struct {
126142
OrgID int64 `xorm:"INDEX"`
127143
IsSystemWebhook bool
128144
URL string `xorm:"url TEXT"`
129-
Signature string `xorm:"TEXT"`
130145
HTTPMethod string `xorm:"http_method"`
131146
ContentType HookContentType
132147
Secret string `xorm:"TEXT"`
133148
Events string `xorm:"TEXT"`
134149
*HookEvent `xorm:"-"`
135-
IsSSL bool `xorm:"is_ssl"`
136-
IsActive bool `xorm:"INDEX"`
137-
Type HookTaskType `xorm:"VARCHAR(16) 'type'"`
138-
Meta string `xorm:"TEXT"` // store hook-specific attributes
139-
LastStatus HookStatus // Last delivery status
150+
IsActive bool `xorm:"INDEX"`
151+
Type HookType `xorm:"VARCHAR(16) 'type'"`
152+
Meta string `xorm:"TEXT"` // store hook-specific attributes
153+
LastStatus HookStatus // Last delivery status
140154

141155
CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
142156
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
@@ -558,22 +572,6 @@ func copyDefaultWebhooksToRepo(e Engine, repoID int64) error {
558572
// \___|_ / \____/ \____/|__|_ \ |____| (____ /____ >__|_ \
559573
// \/ \/ \/ \/ \/
560574

561-
// HookTaskType is the type of an hook task
562-
type HookTaskType = string
563-
564-
// Types of hook tasks
565-
const (
566-
GITEA HookTaskType = "gitea"
567-
GOGS HookTaskType = "gogs"
568-
SLACK HookTaskType = "slack"
569-
DISCORD HookTaskType = "discord"
570-
DINGTALK HookTaskType = "dingtalk"
571-
TELEGRAM HookTaskType = "telegram"
572-
MSTEAMS HookTaskType = "msteams"
573-
FEISHU HookTaskType = "feishu"
574-
MATRIX HookTaskType = "matrix"
575-
)
576-
577575
// HookEventType is the type of an hook event
578576
type HookEventType string
579577

@@ -635,7 +633,9 @@ func (h HookEventType) Event() string {
635633

636634
// HookRequest represents hook task request information.
637635
type HookRequest struct {
638-
Headers map[string]string `json:"headers"`
636+
URL string `json:"url"`
637+
HTTPMethod string `json:"http_method"`
638+
Headers map[string]string `json:"headers"`
639639
}
640640

641641
// HookResponse represents hook task response information.
@@ -651,15 +651,9 @@ type HookTask struct {
651651
RepoID int64 `xorm:"INDEX"`
652652
HookID int64
653653
UUID string
654-
Typ HookTaskType `xorm:"VARCHAR(16) index"`
655-
URL string `xorm:"TEXT"`
656-
Signature string `xorm:"TEXT"`
657654
api.Payloader `xorm:"-"`
658655
PayloadContent string `xorm:"TEXT"`
659-
HTTPMethod string `xorm:"http_method"`
660-
ContentType HookContentType
661656
EventType HookEventType
662-
IsSSL bool
663657
IsDelivered bool
664658
Delivered int64
665659
DeliveredString string `xorm:"-"`

‎models/webhook_test.go‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -207,8 +207,6 @@ func TestCreateHookTask(t *testing.T) {
207207
hookTask := &HookTask{
208208
RepoID: 3,
209209
HookID: 3,
210-
Typ: GITEA,
211-
URL: "http://www.example.com/unit_test",
212210
Payloader: &api.PushPayload{},
213211
}
214212
AssertNotExistsBean(t, hookTask)
@@ -233,8 +231,6 @@ func TestCleanupHookTaskTable_PerWebhook_DeletesDelivered(t *testing.T) {
233231
hookTask := &HookTask{
234232
RepoID: 3,
235233
HookID: 3,
236-
Typ: GITEA,
237-
URL: "http://www.example.com/unit_test",
238234
Payloader: &api.PushPayload{},
239235
IsDelivered: true,
240236
Delivered: time.Now().UnixNano(),
@@ -252,8 +248,6 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesUndelivered(t *testing.T) {
252248
hookTask := &HookTask{
253249
RepoID: 2,
254250
HookID: 4,
255-
Typ: GITEA,
256-
URL: "http://www.example.com/unit_test",
257251
Payloader: &api.PushPayload{},
258252
IsDelivered: false,
259253
}
@@ -270,8 +264,6 @@ func TestCleanupHookTaskTable_PerWebhook_LeavesMostRecentTask(t *testing.T) {
270264
hookTask := &HookTask{
271265
RepoID: 2,
272266
HookID: 4,
273-
Typ: GITEA,
274-
URL: "http://www.example.com/unit_test",
275267
Payloader: &api.PushPayload{},
276268
IsDelivered: true,
277269
Delivered: time.Now().UnixNano(),
@@ -289,8 +281,6 @@ func TestCleanupHookTaskTable_OlderThan_DeletesDelivered(t *testing.T) {
289281
hookTask := &HookTask{
290282
RepoID: 3,
291283
HookID: 3,
292-
Typ: GITEA,
293-
URL: "http://www.example.com/unit_test",
294284
Payloader: &api.PushPayload{},
295285
IsDelivered: true,
296286
Delivered: time.Now().AddDate(0, 0, -8).UnixNano(),
@@ -308,8 +298,6 @@ func TestCleanupHookTaskTable_OlderThan_LeavesUndelivered(t *testing.T) {
308298
hookTask := &HookTask{
309299
RepoID: 2,
310300
HookID: 4,
311-
Typ: GITEA,
312-
URL: "http://www.example.com/unit_test",
313301
Payloader: &api.PushPayload{},
314302
IsDelivered: false,
315303
}
@@ -326,8 +314,6 @@ func TestCleanupHookTaskTable_OlderThan_LeavesTaskEarlierThanAgeToDelete(t *test
326314
hookTask := &HookTask{
327315
RepoID: 2,
328316
HookID: 4,
329-
Typ: GITEA,
330-
URL: "http://www.example.com/unit_test",
331317
Payloader: &api.PushPayload{},
332318
IsDelivered: true,
333319
Delivered: time.Now().AddDate(0, 0, -6).UnixNano(),

‎modules/structs/hook.go‎

Lines changed: 0 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ type EditHookOption struct {
6262

6363
// Payloader payload is some part of one hook
6464
type Payloader interface {
65-
SetSecret(string)
6665
JSONPayload() ([]byte, error)
6766
}
6867

@@ -124,19 +123,13 @@ var (
124123

125124
// CreatePayload FIXME
126125
type CreatePayload struct {
127-
Secret string `json:"secret"`
128126
Sha string `json:"sha"`
129127
Ref string `json:"ref"`
130128
RefType string `json:"ref_type"`
131129
Repo *Repository `json:"repository"`
132130
Sender *User `json:"sender"`
133131
}
134132

135-
// SetSecret modifies the secret of the CreatePayload
136-
func (p *CreatePayload) SetSecret(secret string) {
137-
p.Secret = secret
138-
}
139-
140133
// JSONPayload return payload information
141134
func (p *CreatePayload) JSONPayload() ([]byte, error) {
142135
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -181,19 +174,13 @@ const (
181174

182175
// DeletePayload represents delete payload
183176
type DeletePayload struct {
184-
Secret string `json:"secret"`
185177
Ref string `json:"ref"`
186178
RefType string `json:"ref_type"`
187179
PusherType PusherType `json:"pusher_type"`
188180
Repo *Repository `json:"repository"`
189181
Sender *User `json:"sender"`
190182
}
191183

192-
// SetSecret modifies the secret of the DeletePayload
193-
func (p *DeletePayload) SetSecret(secret string) {
194-
p.Secret = secret
195-
}
196-
197184
// JSONPayload implements Payload
198185
func (p *DeletePayload) JSONPayload() ([]byte, error) {
199186
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -209,17 +196,11 @@ func (p *DeletePayload) JSONPayload() ([]byte, error) {
209196

210197
// ForkPayload represents fork payload
211198
type ForkPayload struct {
212-
Secret string `json:"secret"`
213199
Forkee *Repository `json:"forkee"`
214200
Repo *Repository `json:"repository"`
215201
Sender *User `json:"sender"`
216202
}
217203

218-
// SetSecret modifies the secret of the ForkPayload
219-
func (p *ForkPayload) SetSecret(secret string) {
220-
p.Secret = secret
221-
}
222-
223204
// JSONPayload implements Payload
224205
func (p *ForkPayload) JSONPayload() ([]byte, error) {
225206
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -238,7 +219,6 @@ const (
238219

239220
// IssueCommentPayload represents a payload information of issue comment event.
240221
type IssueCommentPayload struct {
241-
Secret string `json:"secret"`
242222
Action HookIssueCommentAction `json:"action"`
243223
Issue *Issue `json:"issue"`
244224
Comment *Comment `json:"comment"`
@@ -248,11 +228,6 @@ type IssueCommentPayload struct {
248228
IsPull bool `json:"is_pull"`
249229
}
250230

251-
// SetSecret modifies the secret of the IssueCommentPayload
252-
func (p *IssueCommentPayload) SetSecret(secret string) {
253-
p.Secret = secret
254-
}
255-
256231
// JSONPayload implements Payload
257232
func (p *IssueCommentPayload) JSONPayload() ([]byte, error) {
258233
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -278,18 +253,12 @@ const (
278253

279254
// ReleasePayload represents a payload information of release event.
280255
type ReleasePayload struct {
281-
Secret string `json:"secret"`
282256
Action HookReleaseAction `json:"action"`
283257
Release *Release `json:"release"`
284258
Repository *Repository `json:"repository"`
285259
Sender *User `json:"sender"`
286260
}
287261

288-
// SetSecret modifies the secret of the ReleasePayload
289-
func (p *ReleasePayload) SetSecret(secret string) {
290-
p.Secret = secret
291-
}
292-
293262
// JSONPayload implements Payload
294263
func (p *ReleasePayload) JSONPayload() ([]byte, error) {
295264
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -305,7 +274,6 @@ func (p *ReleasePayload) JSONPayload() ([]byte, error) {
305274

306275
// PushPayload represents a payload information of push event.
307276
type PushPayload struct {
308-
Secret string `json:"secret"`
309277
Ref string `json:"ref"`
310278
Before string `json:"before"`
311279
After string `json:"after"`
@@ -317,11 +285,6 @@ type PushPayload struct {
317285
Sender *User `json:"sender"`
318286
}
319287

320-
// SetSecret modifies the secret of the PushPayload
321-
func (p *PushPayload) SetSecret(secret string) {
322-
p.Secret = secret
323-
}
324-
325288
// JSONPayload FIXME
326289
func (p *PushPayload) JSONPayload() ([]byte, error) {
327290
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -389,7 +352,6 @@ const (
389352

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

401-
// SetSecret modifies the secret of the IssuePayload.
402-
func (p *IssuePayload) SetSecret(secret string) {
403-
p.Secret = secret
404-
}
405-
406363
// JSONPayload encodes the IssuePayload to JSON, with an indentation of two spaces.
407364
func (p *IssuePayload) JSONPayload() ([]byte, error) {
408365
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -430,7 +387,6 @@ type ChangesPayload struct {
430387

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

443-
// SetSecret modifies the secret of the PullRequestPayload.
444-
func (p *PullRequestPayload) SetSecret(secret string) {
445-
p.Secret = secret
446-
}
447-
448399
// JSONPayload FIXME
449400
func (p *PullRequestPayload) JSONPayload() ([]byte, error) {
450401
json := jsoniter.ConfigCompatibleWithStandardLibrary
@@ -476,18 +427,12 @@ const (
476427

477428
// RepositoryPayload payload for repository webhooks
478429
type RepositoryPayload struct {
479-
Secret string `json:"secret"`
480430
Action HookRepoAction `json:"action"`
481431
Repository *Repository `json:"repository"`
482432
Organization *User `json:"organization"`
483433
Sender *User `json:"sender"`
484434
}
485435

486-
// SetSecret modifies the secret of the RepositoryPayload
487-
func (p *RepositoryPayload) SetSecret(secret string) {
488-
p.Secret = secret
489-
}
490-
491436
// JSONPayload JSON representation of the payload
492437
func (p *RepositoryPayload) JSONPayload() ([]byte, error) {
493438
json := jsoniter.ConfigCompatibleWithStandardLibrary

0 commit comments

Comments
 (0)