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 <wxiaoguang@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>

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:

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()
 

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)

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"}`,