From 7f420f8d82386a701c10b6f1e8bb8c2165d2e8f7 Mon Sep 17 00:00:00 2001 From: Shivansh Garg Date: Sun, 18 Oct 2026 14:11:39 +0530 Subject: [PATCH] fix: avoid index out-of-range panic in webhook assignee formatting (#39827) getPullRequestInfo and getIssuesInfo indexed into the last element of an empty Assignees slice when Action was HookIssueAssigned, causing a panic under a race where the assignee list is empty by the time the webhook payload is built. Fixes https://github.com/go-gitea/gitea/issues/39826 Co-authored-by: wxiaoguang Co-authored-by: silverwind --- modules/structs/hook.go | 8 ++- services/actions/notifier.go | 65 ++++++++----------- services/notify/notify.go | 108 +++++++++++++++++++++++++++++++ services/notify/notify_test.go | 54 ++++++++++++++++ services/webhook/general.go | 52 +++++++++++---- services/webhook/general_test.go | 45 +++++++++++++ services/webhook/notifier.go | 23 ++++--- services/webhook/webhook_test.go | 52 +++++++++++++++ 8 files changed, 347 insertions(+), 60 deletions(-) create mode 100644 services/notify/notify_test.go diff --git a/modules/structs/hook.go b/modules/structs/hook.go index 4c55f369855..afa3f27b610 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -393,7 +393,7 @@ type IssuePayload struct { Action HookIssueAction `json:"action"` // The index number of the issue Index int64 `json:"number"` - // Changes made to the issue (for edit actions) + // Changes made to the issue Changes *ChangesPayload `json:"changes,omitempty"` // The issue that was acted upon Issue *Issue `json:"issue"` @@ -430,6 +430,10 @@ type ChangesPayload struct { AddedLabels []*Label `json:"added_labels"` // Changes made to the labels removed RemovedLabels []*Label `json:"removed_labels"` + // Assignees added + AddedAssignees []*User `json:"added_assignees,omitempty"` + // Assignees removed + RemovedAssignees []*User `json:"removed_assignees,omitempty"` } // PullRequestPayload represents a payload information of pull request event. @@ -442,7 +446,7 @@ type PullRequestPayload struct { After string `json:"after,omitempty"` // The index number of the pull request Index int64 `json:"number"` - // Changes made to the pull request (for edit actions) + // Changes made to the pull request Changes *ChangesPayload `json:"changes,omitempty"` // The pull request that was acted upon PullRequest *PullRequest `json:"pull_request"` diff --git a/services/actions/notifier.go b/services/actions/notifier.go index 10ecc12bc7c..8e21e2bfd3e 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -158,11 +158,13 @@ func (n *actionsNotifier) IssueChangeStatus(ctx context.Context, doer *user_mode func (n *actionsNotifier) IssueChangeAssignee(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, assignee *user_model.User, removed bool, comment *issues_model.Comment) { ctx = withMethod(ctx, "IssueChangeAssignee") - var action api.HookIssueAction + action := api.HookIssueAssigned + changes := &api.ChangesPayload{} if removed { action = api.HookIssueUnassigned + changes.RemovedAssignees = []*api.User{convert.ToUser(ctx, assignee, nil)} } else { - action = api.HookIssueAssigned + changes.AddedAssignees = []*api.User{convert.ToUser(ctx, assignee, nil)} } hookEvent := webhook_module.HookEventIssueAssign @@ -170,7 +172,7 @@ func (n *actionsNotifier) IssueChangeAssignee(ctx context.Context, doer *user_mo hookEvent = webhook_module.HookEventPullRequestAssign } - notifyIssueChange(ctx, doer, issue, hookEvent, action, nil, nil) + notifyIssueChange(ctx, doer, issue, hookEvent, action, changes) } // IssueChangeMilestone notifies assignee to notifiers @@ -189,7 +191,7 @@ func (n *actionsNotifier) IssueChangeMilestone(ctx context.Context, doer *user_m hookEvent = webhook_module.HookEventPullRequestMilestone } - notifyIssueChange(ctx, doer, issue, hookEvent, action, nil, nil) + notifyIssueChange(ctx, doer, issue, hookEvent, action, &api.ChangesPayload{}) } func (n *actionsNotifier) IssueChangeLabels(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, @@ -202,40 +204,35 @@ func (n *actionsNotifier) IssueChangeLabels(ctx context.Context, doer *user_mode hookEvent = webhook_module.HookEventPullRequestLabel } - notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelUpdated, addedLabels, removedLabels) -} - -func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, event webhook_module.HookEventType, action api.HookIssueAction, addedLabels, removedLabels []*issues_model.Label) { - var err error - if err = issue.LoadRepo(ctx); err != nil { + if err := issue.LoadRepo(ctx); err != nil { log.Error("LoadRepo: %v", err) return } - if err = issue.LoadPoster(ctx); err != nil { + changes := &api.ChangesPayload{} + if addedLabels != nil { + changes.AddedLabels = convert.ToLabelList(addedLabels, issue.Repo, doer) + } + + if removedLabels != nil { + changes.RemovedLabels = convert.ToLabelList(removedLabels, issue.Repo, doer) + } + + notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelUpdated, changes) +} + +func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, event webhook_module.HookEventType, action api.HookIssueAction, changes *api.ChangesPayload) { + if err := issue.LoadRepo(ctx); err != nil { + log.Error("LoadRepo: %v", err) + return + } + if err := issue.LoadPoster(ctx); err != nil { log.Error("LoadPoster: %v", err) return } - var addedAPILabels []*api.Label - if addedLabels != nil { - addedAPILabels = make([]*api.Label, 0, len(addedLabels)) - for _, label := range addedLabels { - addedAPILabels = append(addedAPILabels, convert.ToLabel(label, issue.Repo, doer)) - } - } - - // Get removed labels from context if present - var removedAPILabels []*api.Label - if removedLabels != nil { - removedAPILabels = make([]*api.Label, 0, len(removedLabels)) - for _, label := range removedLabels { - removedAPILabels = append(removedAPILabels, convert.ToLabel(label, issue.Repo, doer)) - } - } - if issue.IsPull { - if err = issue.LoadPullRequest(ctx); err != nil { + if err := issue.LoadPullRequest(ctx); err != nil { log.Error("loadPullRequest: %v", err) return } @@ -246,10 +243,7 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues PullRequest: convert.ToAPIPullRequest(ctx, issue.PullRequest, nil), Repository: convert.ToRepo(ctx, issue.Repo, access_model.Permission{AccessMode: perm_model.AccessModeNone}), Sender: convert.ToUser(ctx, doer, nil), - Changes: &api.ChangesPayload{ - AddedLabels: addedAPILabels, - RemovedLabels: removedAPILabels, - }, + Changes: changes, } newNotifyInputFromIssue(issue, event). @@ -267,10 +261,7 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues Issue: convert.ToAPIIssue(ctx, doer, issue), Repository: convert.ToRepo(ctx, issue.Repo, permission), Sender: convert.ToUser(ctx, doer, nil), - Changes: &api.ChangesPayload{ - AddedLabels: addedAPILabels, - RemovedLabels: removedAPILabels, - }, + Changes: changes, } newNotifyInputFromIssue(issue, event). diff --git a/services/notify/notify.go b/services/notify/notify.go index b61588a0639..02c8c01a459 100644 --- a/services/notify/notify.go +++ b/services/notify/notify.go @@ -19,6 +19,12 @@ import ( var notifiers []Notifier +func recoverPanic() { + if err := recover(); err != nil { + log.Error("PANIC while notifying: %v\n%s", err, log.Stack(2)) + } +} + // RegisterNotifier providers method to receive notify messages func RegisterNotifier(notifier Notifier) { go notifier.Run() @@ -27,6 +33,8 @@ func RegisterNotifier(notifier Notifier) { // NewWikiPage notifies creating new wiki pages to notifiers func NewWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, page, comment string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.NewWikiPage(ctx, doer, repo, page, comment) } @@ -34,6 +42,8 @@ func NewWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model.Re // EditWikiPage notifies editing or renaming wiki pages to notifiers func EditWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, page, comment string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.EditWikiPage(ctx, doer, repo, page, comment) } @@ -41,6 +51,8 @@ func EditWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model.R // DeleteWikiPage notifies deleting wiki pages to notifiers func DeleteWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, page string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.DeleteWikiPage(ctx, doer, repo, page) } @@ -61,6 +73,8 @@ func shouldSendCommentChangeNotification(ctx context.Context, comment *issues_mo func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, comment *issues_model.Comment, mentions []*user_model.User, ) { + defer recoverPanic() + if !shouldSendCommentChangeNotification(ctx, comment) { return } @@ -72,6 +86,8 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m // NewIssue notifies new issue to notifiers func NewIssue(ctx context.Context, issue *issues_model.Issue, mentions []*user_model.User) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.NewIssue(ctx, issue, mentions) } @@ -79,6 +95,8 @@ func NewIssue(ctx context.Context, issue *issues_model.Issue, mentions []*user_m // IssueChangeStatus notifies close or reopen issue to notifiers func IssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueChangeStatus(ctx, doer, commitID, issue, actionComment, closeOrReopen) } @@ -86,6 +104,8 @@ func IssueChangeStatus(ctx context.Context, doer *user_model.User, commitID stri // DeleteIssue notify when some issue deleted func DeleteIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Issue) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.DeleteIssue(ctx, doer, issue) } @@ -93,6 +113,8 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, issue *issues_model // MergePullRequest notifies merge pull request to notifiers func MergePullRequest(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.MergePullRequest(ctx, doer, pr) } @@ -100,6 +122,8 @@ func MergePullRequest(ctx context.Context, doer *user_model.User, pr *issues_mod // AutoMergePullRequest notifies merge pull request to notifiers func AutoMergePullRequest(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.AutoMergePullRequest(ctx, doer, pr) } @@ -107,6 +131,8 @@ func AutoMergePullRequest(ctx context.Context, doer *user_model.User, pr *issues // NewPullRequest notifies new pull request to notifiers func NewPullRequest(ctx context.Context, pr *issues_model.PullRequest, mentions []*user_model.User) { + defer recoverPanic() + if err := pr.LoadIssue(ctx); err != nil { log.Error("LoadIssue failed: %v", err) return @@ -121,6 +147,8 @@ func NewPullRequest(ctx context.Context, pr *issues_model.PullRequest, mentions // PullRequestSynchronized notifies Synchronized pull request func PullRequestSynchronized(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest, before, after string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PullRequestSynchronized(ctx, doer, pr, before, after) } @@ -128,6 +156,8 @@ func PullRequestSynchronized(ctx context.Context, doer *user_model.User, pr *iss // PullRequestReview notifies new pull request review func PullRequestReview(ctx context.Context, pr *issues_model.PullRequest, review *issues_model.Review, comment *issues_model.Comment, mentions []*user_model.User) { + defer recoverPanic() + if err := review.LoadReviewer(ctx); err != nil { log.Error("LoadReviewer failed: %v", err) return @@ -139,6 +169,8 @@ func PullRequestReview(ctx context.Context, pr *issues_model.PullRequest, review // PullRequestCodeComment notifies new pull request code comment func PullRequestCodeComment(ctx context.Context, pr *issues_model.PullRequest, comment *issues_model.Comment, mentions []*user_model.User) { + defer recoverPanic() + if err := comment.LoadPoster(ctx); err != nil { log.Error("LoadPoster: %v", err) return @@ -150,6 +182,8 @@ func PullRequestCodeComment(ctx context.Context, pr *issues_model.PullRequest, c // PullRequestChangeTargetBranch notifies when a pull request's target branch was changed func PullRequestChangeTargetBranch(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest, oldBranch string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PullRequestChangeTargetBranch(ctx, doer, pr, oldBranch) } @@ -157,6 +191,8 @@ func PullRequestChangeTargetBranch(ctx context.Context, doer *user_model.User, p // PullRequestPushCommits notifies when push commits to pull request's head branch func PullRequestPushCommits(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest, comment *issues_model.Comment) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PullRequestPushCommits(ctx, doer, pr, comment) } @@ -164,6 +200,8 @@ func PullRequestPushCommits(ctx context.Context, doer *user_model.User, pr *issu // PullReviewDismiss notifies when a review was dismissed by repo admin func PullReviewDismiss(ctx context.Context, doer *user_model.User, review *issues_model.Review, comment *issues_model.Comment) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PullReviewDismiss(ctx, doer, review, comment) } @@ -171,6 +209,8 @@ func PullReviewDismiss(ctx context.Context, doer *user_model.User, review *issue // UpdateComment notifies update comment to notifiers func UpdateComment(ctx context.Context, doer *user_model.User, c *issues_model.Comment, oldContent string) { + defer recoverPanic() + if !shouldSendCommentChangeNotification(ctx, c) { return } @@ -182,6 +222,8 @@ func UpdateComment(ctx context.Context, doer *user_model.User, c *issues_model.C // DeleteComment notifies delete comment to notifiers func DeleteComment(ctx context.Context, doer *user_model.User, c *issues_model.Comment) { + defer recoverPanic() + if !shouldSendCommentChangeNotification(ctx, c) { return } @@ -193,6 +235,8 @@ func DeleteComment(ctx context.Context, doer *user_model.User, c *issues_model.C // NewRelease notifies new release to notifiers func NewRelease(ctx context.Context, rel *repo_model.Release) { + defer recoverPanic() + if err := rel.LoadAttributes(ctx); err != nil { log.Error("LoadPublisher: %v", err) return @@ -204,6 +248,8 @@ func NewRelease(ctx context.Context, rel *repo_model.Release) { // UpdateRelease notifies update release to notifiers func UpdateRelease(ctx context.Context, doer *user_model.User, rel *repo_model.Release) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.UpdateRelease(ctx, doer, rel) } @@ -211,6 +257,8 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, rel *repo_model.R // DeleteRelease notifies delete release to notifiers func DeleteRelease(ctx context.Context, doer *user_model.User, rel *repo_model.Release) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.DeleteRelease(ctx, doer, rel) } @@ -218,6 +266,8 @@ func DeleteRelease(ctx context.Context, doer *user_model.User, rel *repo_model.R // IssueChangeMilestone notifies change milestone to notifiers func IssueChangeMilestone(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldMilestoneID int64) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueChangeMilestone(ctx, doer, issue, oldMilestoneID) } @@ -225,6 +275,8 @@ func IssueChangeMilestone(ctx context.Context, doer *user_model.User, issue *iss // IssueChangeContent notifies change content to notifiers func IssueChangeContent(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldContent string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueChangeContent(ctx, doer, issue, oldContent) } @@ -232,6 +284,8 @@ func IssueChangeContent(ctx context.Context, doer *user_model.User, issue *issue // IssueChangeAssignee notifies change content to notifiers func IssueChangeAssignee(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, assignee *user_model.User, removed bool, comment *issues_model.Comment) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueChangeAssignee(ctx, doer, issue, assignee, removed, comment) } @@ -239,6 +293,8 @@ func IssueChangeAssignee(ctx context.Context, doer *user_model.User, issue *issu // PullRequestReviewRequest notifies Request Review change func PullRequestReviewRequest(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, reviewer *user_model.User, isRequest bool, comment *issues_model.Comment) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PullRequestReviewRequest(ctx, doer, issue, reviewer, isRequest, comment) } @@ -246,6 +302,8 @@ func PullRequestReviewRequest(ctx context.Context, doer *user_model.User, issue // IssueClearLabels notifies clear labels to notifiers func IssueClearLabels(ctx context.Context, doer *user_model.User, issue *issues_model.Issue) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueClearLabels(ctx, doer, issue) } @@ -253,6 +311,8 @@ func IssueClearLabels(ctx context.Context, doer *user_model.User, issue *issues_ // IssueChangeTitle notifies change title to notifiers func IssueChangeTitle(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldTitle string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueChangeTitle(ctx, doer, issue, oldTitle) } @@ -260,6 +320,8 @@ func IssueChangeTitle(ctx context.Context, doer *user_model.User, issue *issues_ // IssueChangeRef notifies change reference to notifiers func IssueChangeRef(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldRef string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueChangeRef(ctx, doer, issue, oldRef) } @@ -269,6 +331,8 @@ func IssueChangeRef(ctx context.Context, doer *user_model.User, issue *issues_mo func IssueChangeLabels(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, addedLabels, removedLabels []*issues_model.Label, ) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.IssueChangeLabels(ctx, doer, issue, addedLabels, removedLabels) } @@ -276,6 +340,8 @@ func IssueChangeLabels(ctx context.Context, doer *user_model.User, issue *issues // CreateRepository notifies create repository to notifiers func CreateRepository(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.CreateRepository(ctx, doer, u, repo) } @@ -283,6 +349,8 @@ func CreateRepository(ctx context.Context, doer, u *user_model.User, repo *repo_ // AdoptRepository notifies the adoption of a repository to notifiers func AdoptRepository(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.AdoptRepository(ctx, doer, u, repo) } @@ -290,6 +358,8 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, repo *repo_m // MigrateRepository notifies create repository to notifiers func MigrateRepository(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.MigrateRepository(ctx, doer, u, repo) } @@ -297,6 +367,8 @@ func MigrateRepository(ctx context.Context, doer, u *user_model.User, repo *repo // TransferRepository notifies create repository to notifiers func TransferRepository(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, oldOwnerName string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.TransferRepository(ctx, doer, repo, oldOwnerName) } @@ -304,6 +376,8 @@ func TransferRepository(ctx context.Context, doer *user_model.User, repo *repo_m // DeleteRepository notifies delete repository to notifiers func DeleteRepository(ctx context.Context, doer *user_model.User, repo *repo_model.Repository) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.DeleteRepository(ctx, doer, repo) } @@ -311,6 +385,8 @@ func DeleteRepository(ctx context.Context, doer *user_model.User, repo *repo_mod // ForkRepository notifies fork repository to notifiers func ForkRepository(ctx context.Context, doer *user_model.User, oldRepo, repo *repo_model.Repository) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.ForkRepository(ctx, doer, oldRepo, repo) } @@ -318,6 +394,8 @@ func ForkRepository(ctx context.Context, doer *user_model.User, oldRepo, repo *r // RenameRepository notifies repository renamed func RenameRepository(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, oldName string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.RenameRepository(ctx, doer, repo, oldName) } @@ -325,6 +403,8 @@ func RenameRepository(ctx context.Context, doer *user_model.User, repo *repo_mod // PushCommits notifies commits pushed to notifiers func PushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PushCommits(ctx, pusher, repo, opts, commits) } @@ -332,6 +412,8 @@ func PushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model. // CreateRef notifies branch or tag creation to notifiers func CreateRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, refFullName git.RefName, refID string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.CreateRef(ctx, pusher, repo, refFullName, refID) } @@ -339,6 +421,8 @@ func CreateRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Re // DeleteRef notifies branch or tag deletion to notifiers func DeleteRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, refFullName git.RefName) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.DeleteRef(ctx, pusher, repo, refFullName) } @@ -346,6 +430,8 @@ func DeleteRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Re // SyncPushCommits notifies commits pushed to notifiers func SyncPushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.SyncPushCommits(ctx, pusher, repo, opts, commits) } @@ -353,6 +439,8 @@ func SyncPushCommits(ctx context.Context, pusher *user_model.User, repo *repo_mo // SyncCreateRef notifies branch or tag creation to notifiers func SyncCreateRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, refFullName git.RefName, refID string) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.SyncCreateRef(ctx, pusher, repo, refFullName, refID) } @@ -360,6 +448,8 @@ func SyncCreateRef(ctx context.Context, pusher *user_model.User, repo *repo_mode // SyncDeleteRef notifies branch or tag deletion to notifiers func SyncDeleteRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, refFullName git.RefName) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.SyncDeleteRef(ctx, pusher, repo, refFullName) } @@ -367,6 +457,8 @@ func SyncDeleteRef(ctx context.Context, pusher *user_model.User, repo *repo_mode // RepoPendingTransfer notifies creation of pending transfer to notifiers func RepoPendingTransfer(ctx context.Context, doer, newOwner *user_model.User, repo *repo_model.Repository) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.RepoPendingTransfer(ctx, doer, newOwner, repo) } @@ -374,6 +466,8 @@ func RepoPendingTransfer(ctx context.Context, doer, newOwner *user_model.User, r // PackageCreate notifies creation of a package to notifiers func PackageCreate(ctx context.Context, doer *user_model.User, pd *packages_model.PackageDescriptor) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PackageCreate(ctx, doer, pd) } @@ -381,6 +475,8 @@ func PackageCreate(ctx context.Context, doer *user_model.User, pd *packages_mode // PackageDelete notifies deletion of a package to notifiers func PackageDelete(ctx context.Context, doer *user_model.User, pd *packages_model.PackageDescriptor) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.PackageDelete(ctx, doer, pd) } @@ -388,12 +484,16 @@ func PackageDelete(ctx context.Context, doer *user_model.User, pd *packages_mode // ChangeDefaultBranch notifies change default branch to notifiers func ChangeDefaultBranch(ctx context.Context, repo *repo_model.Repository) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.ChangeDefaultBranch(ctx, repo) } } func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, commit *repository.PushCommit, sender *user_model.User, status *git_model.CommitStatus) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.CreateCommitStatus(ctx, repo, commit, sender, status) } @@ -403,6 +503,8 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, commit // Prefer the helpers in services/actions/notify.go over calling this directly; // unless you are sure the caller has already resolved the correct sender and paired notifications. func WorkflowRunStatusUpdate(ctx context.Context, repo *repo_model.Repository, sender *user_model.User, run *actions_model.ActionRun) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.WorkflowRunStatusUpdate(ctx, repo, sender, run) } @@ -412,6 +514,8 @@ func WorkflowRunStatusUpdate(ctx context.Context, repo *repo_model.Repository, s // Prefer the helpers in services/actions/notify.go over calling this directly; // unless you are sure the caller has already resolved the correct sender and paired notifications. func WorkflowJobStatusUpdate(ctx context.Context, repo *repo_model.Repository, sender *user_model.User, job *actions_model.ActionRunJob, task *actions_model.ActionTask) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.WorkflowJobStatusUpdate(ctx, repo, sender, job, task) } @@ -419,6 +523,8 @@ func WorkflowJobStatusUpdate(ctx context.Context, repo *repo_model.Repository, s // Callers must invoke this after any DB write affecting the user's unread count. func NotificationCountChange(ctx context.Context, userID int64) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.NotificationCountChange(ctx, userID) } @@ -426,6 +532,8 @@ func NotificationCountChange(ctx context.Context, userID int64) { // Callers must invoke this after any stopwatch start/stop/cancel so the user's connected tabs refresh. func StopwatchChanged(ctx context.Context, user *user_model.User) { + defer recoverPanic() + for _, notifier := range notifiers { notifier.StopwatchChanged(ctx, user) } diff --git a/services/notify/notify_test.go b/services/notify/notify_test.go new file mode 100644 index 00000000000..98b080706db --- /dev/null +++ b/services/notify/notify_test.go @@ -0,0 +1,54 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package notify + +import ( + "context" + "errors" + "testing" + "time" + + issues_model "gitea.dev/models/issues" + user_model "gitea.dev/models/user" + "gitea.dev/modules/log" + "gitea.dev/modules/test" + + "github.com/stretchr/testify/assert" +) + +type panicNotifier struct { + NullNotifier + panicValue any + called bool +} + +func (n *panicNotifier) NewIssue(context.Context, *issues_model.Issue, []*user_model.User) { + n.called = true + panic(n.panicValue) +} + +func TestNotificationPanicRecovery(t *testing.T) { + notifier := &panicNotifier{} + defer test.MockVariableValue(¬ifiers, []Notifier{notifier})() + + for _, panicValue := range []any{"notification panic", errors.New("notification error"), nil} { + notifier.panicValue = panicValue + notifier.called = false + assert.NotPanics(t, func() { NewIssue(t.Context(), nil, nil) }) + assert.True(t, notifier.called) + } + + checker, cleanup := test.NewLogChecker(log.DEFAULT) + defer cleanup() + checker.Filter("PANIC while notifying: notification panic", "notify_test.go", "notify.go").StopMark("notification recovery complete") + notifier.panicValue = "notification panic" + assert.NotPanics(t, func() { NewIssue(t.Context(), nil, nil) }) + log.Info("notification recovery complete") + filtered, stopped := checker.Check(time.Second) + assert.True(t, stopped) + assert.Equal(t, []bool{true, true, true}, filtered) + + assert.NotPanics(t, func() { CreateIssueComment(t.Context(), nil, nil, nil, nil, nil) }) + assert.NotPanics(t, func() { NewPullRequest(t.Context(), nil, nil) }) +} diff --git a/services/webhook/general.go b/services/webhook/general.go index 82d56d03556..01828ce58f5 100644 --- a/services/webhook/general.go +++ b/services/webhook/general.go @@ -41,9 +41,15 @@ func getPullRequestInfo(p *api.PullRequestPayload) (title, link, by, operator, o } switch p.Action { case api.HookIssueAssigned: - operateResult = fmt.Sprintf("%s assign this to %s", p.Sender.UserName, assignList[len(assignList)-1].UserName) + operateResult = p.Sender.UserName + " assigned this" + if names := getChangedAssigneeNames(p.Changes, p.Action, noneLinkFormatter); names != "" { + operateResult = fmt.Sprintf("%s assign this to %s", p.Sender.UserName, names) + } case api.HookIssueUnassigned: operateResult = p.Sender.UserName + " unassigned this for someone" + if names := getChangedAssigneeNames(p.Changes, p.Action, noneLinkFormatter); names != "" { + operateResult = fmt.Sprintf("%s unassigned this for %s", p.Sender.UserName, names) + } case api.HookIssueMilestoned: operateResult = fmt.Sprintf("%s/milestone/%d", p.Repository.HTMLURL, p.PullRequest.Milestone.ID) } @@ -67,9 +73,15 @@ func getIssuesInfo(p *api.IssuePayload) (issueTitle, link, by, operator, operate } switch p.Action { case api.HookIssueAssigned: - operateResult = fmt.Sprintf("%s assign this to %s", p.Sender.UserName, assignList[len(assignList)-1].UserName) + operateResult = p.Sender.UserName + " assigned this" + if names := getChangedAssigneeNames(p.Changes, p.Action, noneLinkFormatter); names != "" { + operateResult = fmt.Sprintf("%s assign this to %s", p.Sender.UserName, names) + } case api.HookIssueUnassigned: operateResult = p.Sender.UserName + " unassigned this for someone" + if names := getChangedAssigneeNames(p.Changes, p.Action, noneLinkFormatter); names != "" { + operateResult = fmt.Sprintf("%s unassigned this for %s", p.Sender.UserName, names) + } case api.HookIssueMilestoned: operateResult = fmt.Sprintf("%s/milestone/%d", p.Repository.HTMLURL, p.Issue.Milestone.ID) } @@ -95,6 +107,21 @@ func getIssuesCommentInfo(p *api.IssueCommentPayload) (title, link, by, operator return title, link, by, operator } +func getChangedAssigneeNames(changes *api.ChangesPayload, action api.HookIssueAction, linkFormatter linkFormatter) string { + if changes == nil { + return "" + } + assignees := changes.AddedAssignees + if action == api.HookIssueUnassigned { + assignees = changes.RemovedAssignees + } + list := make([]string, len(assignees)) + for i, user := range assignees { + list[i] = linkFormatter(setting.AppURL+url.PathEscape(user.UserName), user.UserName) + } + return strings.Join(list, ", ") +} + func getIssuesPayloadInfo(p *api.IssuePayload, linkFormatter linkFormatter, withSender bool) (text, issueTitle, extraMarkdown string, color int) { color = yellowColor issueTitle = fmt.Sprintf("#%d %s", p.Index, p.Issue.Title) @@ -113,14 +140,16 @@ func getIssuesPayloadInfo(p *api.IssuePayload, linkFormatter linkFormatter, with case api.HookIssueEdited: text = fmt.Sprintf("[%s] Issue edited: %s", repoLink, titleLink) case api.HookIssueAssigned: - list := make([]string, len(p.Issue.Assignees)) - for i, user := range p.Issue.Assignees { - list[i] = linkFormatter(setting.AppURL+url.PathEscape(user.UserName), user.UserName) + text = fmt.Sprintf("[%s] Issue assigned: %s", repoLink, titleLink) + if names := getChangedAssigneeNames(p.Changes, p.Action, linkFormatter); names != "" { + text = fmt.Sprintf("[%s] Issue assigned to %s: %s", repoLink, names, titleLink) } - text = fmt.Sprintf("[%s] Issue assigned to %s: %s", repoLink, strings.Join(list, ", "), titleLink) color = greenColor case api.HookIssueUnassigned: text = fmt.Sprintf("[%s] Issue unassigned: %s", repoLink, titleLink) + if names := getChangedAssigneeNames(p.Changes, p.Action, linkFormatter); names != "" { + text = fmt.Sprintf("[%s] Issue unassigned from %s: %s", repoLink, names, titleLink) + } case api.HookIssueLabelUpdated: text = fmt.Sprintf("[%s] Issue labels updated: %s", repoLink, titleLink) case api.HookIssueLabelCleared: @@ -170,15 +199,16 @@ func getPullRequestPayloadInfo(p *api.PullRequestPayload, linkFormatter linkForm text = fmt.Sprintf("[%s] Pull request edited: %s", repoLink, titleLink) extraMarkdown = p.PullRequest.Body case api.HookIssueAssigned: - list := make([]string, len(p.PullRequest.Assignees)) - for i, user := range p.PullRequest.Assignees { - list[i] = linkFormatter(setting.AppURL+user.UserName, user.UserName) + text = fmt.Sprintf("[%s] Pull request assigned: %s", repoLink, titleLink) + if names := getChangedAssigneeNames(p.Changes, p.Action, linkFormatter); names != "" { + text = fmt.Sprintf("[%s] Pull request assigned to %s: %s", repoLink, names, titleLink) } - text = fmt.Sprintf("[%s] Pull request assigned to %s: %s", repoLink, - strings.Join(list, ", "), titleLink) color = greenColor case api.HookIssueUnassigned: text = fmt.Sprintf("[%s] Pull request unassigned: %s", repoLink, titleLink) + if names := getChangedAssigneeNames(p.Changes, p.Action, linkFormatter); names != "" { + text = fmt.Sprintf("[%s] Pull request unassigned from %s: %s", repoLink, names, titleLink) + } case api.HookIssueLabelUpdated: text = fmt.Sprintf("[%s] Pull request labels updated: %s", repoLink, titleLink) case api.HookIssueLabelCleared: diff --git a/services/webhook/general_test.go b/services/webhook/general_test.go index 879f0f620b7..c7a50ca8fed 100644 --- a/services/webhook/general_test.go +++ b/services/webhook/general_test.go @@ -343,6 +343,7 @@ func packageTestPayload() *api.PackagePayload { func TestGetIssuesPayloadInfo(t *testing.T) { p := issueTestPayload() + p.Changes = &api.ChangesPayload{AddedAssignees: p.Issue.Assignees} cases := []struct { action api.HookIssueAction @@ -442,6 +443,7 @@ func TestGetIssuesPayloadInfo(t *testing.T) { func TestGetPullRequestPayloadInfo(t *testing.T) { p := pullRequestTestPayload() + p.Changes = &api.ChangesPayload{AddedAssignees: p.PullRequest.Assignees} cases := []struct { action api.HookIssueAction @@ -539,6 +541,49 @@ func TestGetPullRequestPayloadInfo(t *testing.T) { } } +func TestAssigneeChanges(t *testing.T) { + changes := &api.ChangesPayload{ + AddedAssignees: []*api.User{{UserName: "user2"}, {UserName: "user3"}}, + RemovedAssignees: []*api.User{{UserName: "user4"}}, + } + cases := []struct { + name string + action api.HookIssueAction + changes *api.ChangesPayload + operation string + text string + }{ + {"assigned", api.HookIssueAssigned, changes, "user1 assign this to user2, user3", "assigned to user2, user3"}, + {"unassigned", api.HookIssueUnassigned, changes, "user1 unassigned this for user4", "unassigned from user4"}, + {"assigned without changes", api.HookIssueAssigned, nil, "user1 assigned this", "assigned"}, + {"unassigned without changes", api.HookIssueUnassigned, nil, "user1 unassigned this for someone", "unassigned"}, + {"assigned with empty changes", api.HookIssueAssigned, &api.ChangesPayload{}, "user1 assigned this", "assigned"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + for _, assignees := range [][]*api.User{nil, {{UserName: "unrelated"}}} { + issue := issueTestPayload() + issue.Action = c.action + issue.Changes = c.changes + issue.Issue.Assignees = assignees + _, _, _, _, operation, _ := getIssuesInfo(issue) + assert.Equal(t, c.operation, operation) + text, _, _, _ := getIssuesPayloadInfo(issue, noneLinkFormatter, true) + assert.Equal(t, "[test/repo] Issue "+c.text+": #2 crash by user1", text) + + pull := pullRequestTestPayload() + pull.Action = c.action + pull.Changes = c.changes + pull.PullRequest.Assignees = assignees + _, _, _, _, operation, _ = getPullRequestInfo(pull) + assert.Equal(t, c.operation, operation) + text, _, _, _ = getPullRequestPayloadInfo(pull, noneLinkFormatter, true) + assert.Equal(t, "[test/repo] Pull request "+c.text+": #12 Fix bug by user1", text) + } + }) + } +} + func TestGetWikiPayloadInfo(t *testing.T) { p := wikiTestPayload() diff --git a/services/webhook/notifier.go b/services/webhook/notifier.go index 944044618f5..3a201f9cbbb 100644 --- a/services/webhook/notifier.go +++ b/services/webhook/notifier.go @@ -159,6 +159,15 @@ func (m *webhookNotifier) MigrateRepository(ctx context.Context, doer, u *user_m } func (m *webhookNotifier) IssueChangeAssignee(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, assignee *user_model.User, removed bool, comment *issues_model.Comment) { + action := api.HookIssueAssigned + changes := &api.ChangesPayload{} + if removed { + action = api.HookIssueUnassigned + changes.RemovedAssignees = []*api.User{convert.ToUser(ctx, assignee, nil)} + } else { + changes.AddedAssignees = []*api.User{convert.ToUser(ctx, assignee, nil)} + } + if issue.IsPull { permission, _ := access_model.GetDoerRepoPermission(ctx, issue.Repo, doer) @@ -167,16 +176,13 @@ func (m *webhookNotifier) IssueChangeAssignee(ctx context.Context, doer *user_mo return } apiPullRequest := &api.PullRequestPayload{ + Action: action, + Changes: changes, Index: issue.Index, PullRequest: convert.ToAPIPullRequest(ctx, issue.PullRequest, doer), Repository: convert.ToRepo(ctx, issue.Repo, permission), Sender: convert.ToUser(ctx, doer, nil), } - if removed { - apiPullRequest.Action = api.HookIssueUnassigned - } else { - apiPullRequest.Action = api.HookIssueAssigned - } // Assignee comment triggers a webhook if err := PrepareWebhooks(ctx, EventSource{Repository: issue.Repo}, webhook_module.HookEventPullRequestAssign, apiPullRequest); err != nil { log.Error("PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err) @@ -185,16 +191,13 @@ func (m *webhookNotifier) IssueChangeAssignee(ctx context.Context, doer *user_mo } else { permission, _ := access_model.GetDoerRepoPermission(ctx, issue.Repo, doer) apiIssue := &api.IssuePayload{ + Action: action, + Changes: changes, Index: issue.Index, Issue: convert.ToAPIIssue(ctx, doer, issue), Repository: convert.ToRepo(ctx, issue.Repo, permission), Sender: convert.ToUser(ctx, doer, nil), } - if removed { - apiIssue.Action = api.HookIssueUnassigned - } else { - apiIssue.Action = api.HookIssueAssigned - } // Assignee comment triggers a webhook if err := PrepareWebhooks(ctx, EventSource{Repository: issue.Repo}, webhook_module.HookEventIssueAssign, apiIssue); err != nil { log.Error("PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err) diff --git a/services/webhook/webhook_test.go b/services/webhook/webhook_test.go index 6ecf24337c7..848607dbefe 100644 --- a/services/webhook/webhook_test.go +++ b/services/webhook/webhook_test.go @@ -7,11 +7,13 @@ import ( "testing" "gitea.dev/models/db" + issues_model "gitea.dev/models/issues" repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" webhook_model "gitea.dev/models/webhook" "gitea.dev/modules/git" + "gitea.dev/modules/json" "gitea.dev/modules/setting" api "gitea.dev/modules/structs" "gitea.dev/modules/test" @@ -26,6 +28,7 @@ func TestWebhookService(t *testing.T) { unittest.PrepareTestEnv(t) t.Run("GetSlackHook", testWebhookGetSlackHook) t.Run("PrepareWebhooks", testWebhookPrepare) + t.Run("AssigneeChanges", testWebhookAssigneeChanges) t.Run("PrepareBranchFilterMatch", testWebhookPrepareBranchFilterMatch) t.Run("PrepareBranchFilterNoMatch", testWebhookPrepareBranchFilterNoMatch) t.Run("WebhookUserMail", testWebhookUserMail) @@ -33,6 +36,55 @@ func TestWebhookService(t *testing.T) { t.Run("PrepareTestWebhookIgnoresGates", testPrepareTestWebhookIgnoresGates) } +func testWebhookAssigneeChanges(t *testing.T) { + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + assignee := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + for _, c := range []struct { + name string + issueID int64 + eventType webhook_module.HookEventType + }{ + {"issue", 1, webhook_module.HookEventIssueAssign}, + {"pull request", 2, webhook_module.HookEventPullRequestAssign}, + } { + t.Run(c.name, func(t *testing.T) { + for _, removed := range []bool{false, true} { + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: c.issueID}) + require.NoError(t, issue.LoadAttributes(t.Context())) + hook := &webhook_model.Webhook{ + RepoID: issue.RepoID, + URL: "http://localhost/gitea-webhook-test-assignee-changes", + ContentType: webhook_model.ContentTypeJSON, + Events: `{"send_everything":true}`, + IsActive: true, + } + require.NoError(t, db.Insert(t.Context(), hook)) + + (&webhookNotifier{}).IssueChangeAssignee(t.Context(), doer, issue, assignee, removed, nil) + + task := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{HookID: hook.ID, EventType: c.eventType}) + var payload struct { + Action api.HookIssueAction `json:"action"` + Changes *api.ChangesPayload `json:"changes"` + } + require.NoError(t, json.Unmarshal([]byte(task.PayloadContent), &payload)) + require.NotNil(t, payload.Changes) + if removed { + assert.Equal(t, api.HookIssueUnassigned, payload.Action) + assert.Empty(t, payload.Changes.AddedAssignees) + require.Len(t, payload.Changes.RemovedAssignees, 1) + assert.Equal(t, assignee.ID, payload.Changes.RemovedAssignees[0].ID) + } else { + assert.Equal(t, api.HookIssueAssigned, payload.Action) + assert.Empty(t, payload.Changes.RemovedAssignees) + require.Len(t, payload.Changes.AddedAssignees, 1) + assert.Equal(t, assignee.ID, payload.Changes.AddedAssignees[0].ID) + } + } + }) + } +} + func testWebhookGetSlackHook(t *testing.T) { w := &webhook_model.Webhook{ Meta: `{"channel": "foo", "username": "username", "color": "blue"}`,