From 94845020e8af67b7ee541560eb6ce6fab0e48b40 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 7 Feb 2025 19:26:50 +0000 Subject: [PATCH] feat: add commit limit for webhook payload (#6797) - Adds a new option `[webhook].PAYLOAD_COMMIT_LIMIT` that limits the amount of commits is sent for each webhook payload, this was previously done via `[ui].FEED_MAX_COMMIT_NUM` which feels incorrect. - The default is 15 for this new option, purely arbitary. - Resolves forgejo/forgejo#6780 - Added unit testing, it's quite a lot because this the notification area is not really easy to test and rather should've been a integration test but that ends up having more complicated than trying doing an unit test. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6797 Reviewed-by: Otto Reviewed-by: 0ko <0ko@noreply.codeberg.org> Co-authored-by: Gusted Co-committed-by: Gusted --- modules/setting/webhook.go | 31 +++-- services/feed/action.go | 9 ++ services/feed/action_test.go | 91 +++++++++++++ services/repository/push.go | 4 - services/webhook/TestPushCommits/webhook.yml | 9 ++ services/webhook/notifier.go | 8 ++ services/webhook/notifier_test.go | 134 +++++++++++++++++++ 7 files changed, 268 insertions(+), 18 deletions(-) create mode 100644 services/webhook/TestPushCommits/webhook.yml create mode 100644 services/webhook/notifier_test.go diff --git a/modules/setting/webhook.go b/modules/setting/webhook.go index 7b1ab4db1f..dd60acf210 100644 --- a/modules/setting/webhook.go +++ b/modules/setting/webhook.go @@ -11,21 +11,23 @@ import ( // Webhook settings var Webhook = struct { - QueueLength int - DeliverTimeout int - SkipTLSVerify bool - AllowedHostList string - PagingNum int - ProxyURL string - ProxyURLFixed *url.URL - ProxyHosts []string + QueueLength int + DeliverTimeout int + SkipTLSVerify bool + AllowedHostList string + PagingNum int + ProxyURL string + ProxyURLFixed *url.URL + ProxyHosts []string + PayloadCommitLimit int }{ - QueueLength: 1000, - DeliverTimeout: 5, - SkipTLSVerify: false, - PagingNum: 10, - ProxyURL: "", - ProxyHosts: []string{}, + QueueLength: 1000, + DeliverTimeout: 5, + SkipTLSVerify: false, + PagingNum: 10, + ProxyURL: "", + ProxyHosts: []string{}, + PayloadCommitLimit: 15, } func loadWebhookFrom(rootCfg ConfigProvider) { @@ -45,4 +47,5 @@ func loadWebhookFrom(rootCfg ConfigProvider) { } } Webhook.ProxyHosts = sec.Key("PROXY_HOSTS").Strings(",") + Webhook.PayloadCommitLimit = sec.Key("PAYLOAD_COMMIT_LIMIT").MustInt(15) } diff --git a/services/feed/action.go b/services/feed/action.go index 83daaa1438..2d6a6cb09a 100644 --- a/services/feed/action.go +++ b/services/feed/action.go @@ -17,6 +17,7 @@ import ( "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" notify_service "code.gitea.io/gitea/services/notify" ) @@ -319,6 +320,10 @@ func (*actionNotifier) NotifyPullRevieweDismiss(ctx context.Context, doer *user_ } func (a *actionNotifier) PushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { + if len(commits.Commits) > setting.UI.FeedMaxCommitNum { + commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum] + } + data, err := json.Marshal(commits) if err != nil { log.Error("Marshal: %v", err) @@ -390,6 +395,10 @@ func (a *actionNotifier) DeleteRef(ctx context.Context, doer *user_model.User, r } func (a *actionNotifier) SyncPushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { + if len(commits.Commits) > setting.UI.FeedMaxCommitNum { + commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum] + } + data, err := json.Marshal(commits) if err != nil { log.Error("json.Marshal: %v", err) diff --git a/services/feed/action_test.go b/services/feed/action_test.go index 87bb13330a..037cf08dfe 100644 --- a/services/feed/action_test.go +++ b/services/feed/action_test.go @@ -12,10 +12,15 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" _ "code.gitea.io/gitea/models/actions" _ "code.gitea.io/gitea/models/forgefed" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -51,3 +56,89 @@ func TestRenameRepoAction(t *testing.T) { unittest.AssertExistsAndLoadBean(t, actionBean) unittest.CheckConsistencyFor(t, &activities_model.Action{}) } + +func pushCommits() *repository.PushCommits { + pushCommits := repository.NewPushCommits() + pushCommits.Commits = []*repository.PushCommit{ + { + Sha1: "69554a6", + CommitterEmail: "user2@example.com", + CommitterName: "User2", + AuthorEmail: "user2@example.com", + AuthorName: "User2", + Message: "not signed commit", + }, + { + Sha1: "27566bd", + CommitterEmail: "user2@example.com", + CommitterName: "User2", + AuthorEmail: "user2@example.com", + AuthorName: "User2", + Message: "good signed commit (with not yet validated email)", + }, + { + Sha1: "5099b81", + CommitterEmail: "user2@example.com", + CommitterName: "User2", + AuthorEmail: "user2@example.com", + AuthorName: "User2", + Message: "good signed commit", + }, + } + pushCommits.HeadCommit = &repository.PushCommit{Sha1: "69554a6"} + return pushCommits +} + +func TestSyncPushCommits(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID}) + + t.Run("All commits", func(t *testing.T) { + defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 10)() + + maxID := unittest.GetCount(t, &activities_model.Action{}) + NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master")}, pushCommits()) + + newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/master"}, unittest.Cond("id > ?", maxID)) + assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"27566bd","Message":"good signed commit (with not yet validated email)","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"5099b81","Message":"good signed commit","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content) + }) + + t.Run("Only one commit", func(t *testing.T) { + defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 1)() + + maxID := unittest.GetCount(t, &activities_model.Action{}) + NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main")}, pushCommits()) + + newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/main"}, unittest.Cond("id > ?", maxID)) + assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content) + }) +} + +func TestPushCommits(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID}) + + t.Run("All commits", func(t *testing.T) { + defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 10)() + + maxID := unittest.GetCount(t, &activities_model.Action{}) + NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master")}, pushCommits()) + + newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/master"}, unittest.Cond("id > ?", maxID)) + assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"27566bd","Message":"good signed commit (with not yet validated email)","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"5099b81","Message":"good signed commit","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content) + }) + + t.Run("Only one commit", func(t *testing.T) { + defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 1)() + + maxID := unittest.GetCount(t, &activities_model.Action{}) + NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main")}, pushCommits()) + + newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/main"}, unittest.Cond("id > ?", maxID)) + assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"user2@example.com","AuthorName":"User2","CommitterEmail":"user2@example.com","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content) + }) +} diff --git a/services/repository/push.go b/services/repository/push.go index a8e7d0f3b6..0c2b66aa3e 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -252,10 +252,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { commits.CompareURL = "" } - if len(commits.Commits) > setting.UI.FeedMaxCommitNum { - commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum] - } - notify_service.PushCommits(ctx, pusher, repo, opts, commits) // Cache for big repository diff --git a/services/webhook/TestPushCommits/webhook.yml b/services/webhook/TestPushCommits/webhook.yml new file mode 100644 index 0000000000..c1ee7ceb9e --- /dev/null +++ b/services/webhook/TestPushCommits/webhook.yml @@ -0,0 +1,9 @@ +- + id: 1001 + repo_id: 2 + type: forgejo + url: http://www.example.com/blÄhaj + http_method: POST + content_type: 1 # json + events: '{"send_everything":true,"branch_filter":"{master*,main*}"}' + is_active: true diff --git a/services/webhook/notifier.go b/services/webhook/notifier.go index fed33d8008..ddd7002890 100644 --- a/services/webhook/notifier.go +++ b/services/webhook/notifier.go @@ -599,6 +599,10 @@ func (m *webhookNotifier) IssueChangeMilestone(ctx context.Context, doer *user_m } func (m *webhookNotifier) PushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { + if len(commits.Commits) > setting.Webhook.PayloadCommitLimit { + commits.Commits = commits.Commits[:setting.Webhook.PayloadCommitLimit] + } + apiPusher := convert.ToUser(ctx, pusher, nil) apiCommits, apiHeadCommit, err := commits.ToAPIPayloadCommits(ctx, repo.RepoPath(), repo.HTMLURL()) if err != nil { @@ -840,6 +844,10 @@ func (m *webhookNotifier) DeleteRelease(ctx context.Context, doer *user_model.Us } func (m *webhookNotifier) SyncPushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { + if len(commits.Commits) > setting.Webhook.PayloadCommitLimit { + commits.Commits = commits.Commits[:setting.Webhook.PayloadCommitLimit] + } + apiPusher := convert.ToUser(ctx, pusher, nil) apiCommits, apiHeadCommit, err := commits.ToAPIPayloadCommits(ctx, repo.RepoPath(), repo.HTMLURL()) if err != nil { diff --git a/services/webhook/notifier_test.go b/services/webhook/notifier_test.go new file mode 100644 index 0000000000..36ec3b8bf1 --- /dev/null +++ b/services/webhook/notifier_test.go @@ -0,0 +1,134 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package webhook + +import ( + "path/filepath" + "testing" + + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + webhook_model "code.gitea.io/gitea/models/webhook" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/test" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func pushCommits() *repository.PushCommits { + pushCommits := repository.NewPushCommits() + pushCommits.Commits = []*repository.PushCommit{ + { + Sha1: "2c54faec6c45d31c1abfaecdab471eac6633738a", + CommitterEmail: "user2@example.com", + CommitterName: "User2", + AuthorEmail: "user2@example.com", + AuthorName: "User2", + Message: "not signed commit", + }, + { + Sha1: "205ac761f3326a7ebe416e8673760016450b5cec", + CommitterEmail: "user2@example.com", + CommitterName: "User2", + AuthorEmail: "user2@example.com", + AuthorName: "User2", + Message: "good signed commit (with not yet validated email)", + }, + { + Sha1: "1032bbf17fbc0d9c95bb5418dabe8f8c99278700", + CommitterEmail: "user2@example.com", + CommitterName: "User2", + AuthorEmail: "user2@example.com", + AuthorName: "User2", + Message: "good signed commit", + }, + } + pushCommits.HeadCommit = &repository.PushCommit{Sha1: "2c54faec6c45d31c1abfaecdab471eac6633738a"} + return pushCommits +} + +func TestSyncPushCommits(t *testing.T) { + defer unittest.OverrideFixtures( + unittest.FixturesOptions{ + Dir: filepath.Join(setting.AppWorkPath, "models/fixtures/"), + Base: setting.AppWorkPath, + Dirs: []string{"services/webhook/TestPushCommits"}, + }, + )() + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: user.ID}) + + t.Run("All commits", func(t *testing.T) { + defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 10)() + + NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master-1")}, pushCommits()) + + hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%master-1%'")) + + var payloadContent structs.PushPayload + require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent)) + assert.Len(t, payloadContent.Commits, 3) + }) + + t.Run("Only one commit", func(t *testing.T) { + defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 1)() + + NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main-1")}, pushCommits()) + + hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%main-1%'")) + + var payloadContent structs.PushPayload + require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent)) + assert.Len(t, payloadContent.Commits, 1) + assert.EqualValues(t, "2c54faec6c45d31c1abfaecdab471eac6633738a", payloadContent.Commits[0].ID) + }) +} + +func TestPushCommits(t *testing.T) { + defer unittest.OverrideFixtures( + unittest.FixturesOptions{ + Dir: filepath.Join(setting.AppWorkPath, "models/fixtures/"), + Base: setting.AppWorkPath, + Dirs: []string{"services/webhook/TestPushCommits"}, + }, + )() + require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: user.ID}) + + t.Run("All commits", func(t *testing.T) { + defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 10)() + + NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master-2")}, pushCommits()) + + hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%master-2%'")) + + var payloadContent structs.PushPayload + require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent)) + assert.Len(t, payloadContent.Commits, 3) + }) + + t.Run("Only one commit", func(t *testing.T) { + defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 1)() + + NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main-2")}, pushCommits()) + + hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%main-2%'")) + + var payloadContent structs.PushPayload + require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent)) + assert.Len(t, payloadContent.Commits, 1) + assert.EqualValues(t, "2c54faec6c45d31c1abfaecdab471eac6633738a", payloadContent.Commits[0].ID) + }) +}