PLT-7077: ignore null array items in slack attachments (#6904)

* ignore null array items in incoming webhooks / command responses

* consolidate code, process announcements in command response as well

* make a bit more idiomatic, add tests

* add missing file

6 files changed, 135 insertions, 58 deletions

diff --git a/model/command_response.go b/model/command_response.go
index 1b2e06cdf..e3253acc0 100644
--- a/model/command_response.go
+++ b/model/command_response.go
@@ -5,7 +5,6 @@ package model
 
 import (
 	"encoding/json"
-	"fmt"
 	"io"
 )
 
@@ -15,12 +14,12 @@ const (
 )
 
 type CommandResponse struct {
-	ResponseType string             `json:"response_type"`
-	Text         string             `json:"text"`
-	Username     string             `json:"username"`
-	IconURL      string             `json:"icon_url"`
-	GotoLocation string             `json:"goto_location"`
-	Attachments  []*SlackAttachment `json:"attachments"`
+	ResponseType string           `json:"response_type"`
+	Text         string           `json:"text"`
+	Username     string           `json:"username"`
+	IconURL      string           `json:"icon_url"`
+	GotoLocation string           `json:"goto_location"`
+	Attachments  SlackAttachments `json:"attachments"`
 }
 
 func (o *CommandResponse) ToJson() string {
@@ -40,14 +39,8 @@ func CommandResponseFromJson(data io.Reader) *CommandResponse {
 		return nil
 	}
 
-	// Ensure attachment fields are stored as strings
-	for _, attachment := range o.Attachments {
-		for _, field := range attachment.Fields {
-			if field.Value != nil {
-				field.Value = fmt.Sprintf("%v", field.Value)
-			}
-		}
-	}
+	o.Text = ExpandAnnouncement(o.Text)
+	o.Attachments.Process()
 
 	return &o
 }
diff --git a/model/command_response_test.go b/model/command_response_test.go
index b57a77608..df478ff2c 100644
--- a/model/command_response_test.go
+++ b/model/command_response_test.go
@@ -82,3 +82,17 @@ func TestCommandResponseFromJson(t *testing.T) {
 		t.Fatal("should've received correct second attachment value")
 	}
 }
+
+func TestCommandResponseNullArrayItems(t *testing.T) {
+	payload := `{"attachments":[{"fields":[{"title":"foo","value":"bar","short":true}, null]}, null]}`
+	cr := CommandResponseFromJson(strings.NewReader(payload))
+	if cr == nil {
+		t.Fatal("CommandResponse should not be nil")
+	}
+	if len(cr.Attachments) != 1 {
+		t.Fatalf("expected one attachment")
+	}
+	if len(cr.Attachments[0].Fields) != 1 {
+		t.Fatalf("expected one field")
+	}
+}
diff --git a/model/incoming_webhook.go b/model/incoming_webhook.go
index 2235cb2c6..4ebc7e8b1 100644
--- a/model/incoming_webhook.go
+++ b/model/incoming_webhook.go
@@ -6,10 +6,8 @@ package model
 import (
 	"bytes"
 	"encoding/json"
-	"fmt"
 	"io"
 	"regexp"
-	"strings"
 )
 
 const (
@@ -29,13 +27,13 @@ type IncomingWebhook struct {
 }
 
 type IncomingWebhookRequest struct {
-	Text        string             `json:"text"`
-	Username    string             `json:"username"`
-	IconURL     string             `json:"icon_url"`
-	ChannelName string             `json:"channel"`
-	Props       StringInterface    `json:"props"`
-	Attachments []*SlackAttachment `json:"attachments"`
-	Type        string             `json:"type"`
+	Text        string           `json:"text"`
+	Username    string           `json:"username"`
+	IconURL     string           `json:"icon_url"`
+	ChannelName string           `json:"channel"`
+	Props       StringInterface  `json:"props"`
+	Attachments SlackAttachments `json:"attachments"`
+	Type        string           `json:"type"`
 }
 
 func (o *IncomingWebhook) ToJson() string {
@@ -193,39 +191,6 @@ func decodeIncomingWebhookRequest(by []byte) (*IncomingWebhookRequest, error) {
 	}
 }
 
-// To mention @channel via a webhook in Slack, the message should contain
-// <!channel>, as explained at the bottom of this article:
-// https://get.slack.help/hc/en-us/articles/202009646-Making-announcements
-func expandAnnouncement(text string) string {
-	c1 := "<!channel>"
-	c2 := "@channel"
-	if strings.Contains(text, c1) {
-		return strings.Replace(text, c1, c2, -1)
-	}
-	return text
-}
-
-// Expand announcements in incoming webhooks from Slack. Those announcements
-// can be found in the text attribute, or in the pretext, text, title and value
-// attributes of the attachment structure. The Slack attachment structure is
-// documented here: https://api.slack.com/docs/attachments
-func expandAnnouncements(i *IncomingWebhookRequest) {
-	i.Text = expandAnnouncement(i.Text)
-
-	for _, attachment := range i.Attachments {
-		attachment.Pretext = expandAnnouncement(attachment.Pretext)
-		attachment.Text = expandAnnouncement(attachment.Text)
-		attachment.Title = expandAnnouncement(attachment.Title)
-
-		for _, field := range attachment.Fields {
-			if field.Value != nil {
-				// Ensure the value is set to a string if it is set
-				field.Value = expandAnnouncement(fmt.Sprintf("%v", field.Value))
-			}
-		}
-	}
-}
-
 func IncomingWebhookRequestFromJson(data io.Reader) *IncomingWebhookRequest {
 	buf := new(bytes.Buffer)
 	buf.ReadFrom(data)
@@ -241,7 +206,8 @@ func IncomingWebhookRequestFromJson(data io.Reader) *IncomingWebhookRequest {
 		}
 	}
 
-	expandAnnouncements(o)
+	o.Text = ExpandAnnouncement(o.Text)
+	o.Attachments.Process()
 
 	return o
 }
diff --git a/model/incoming_webhook_test.go b/model/incoming_webhook_test.go
index f6baca988..36f8ed6e6 100644
--- a/model/incoming_webhook_test.go
+++ b/model/incoming_webhook_test.go
@@ -230,3 +230,17 @@ func TestIncomingWebhookRequestFromJson(t *testing.T) {
 		}
 	}
 }
+
+func TestIncomingWebhookNullArrayItems(t *testing.T) {
+	payload := `{"attachments":[{"fields":[{"title":"foo","value":"bar","short":true}, null]}, null]}`
+	iwr := IncomingWebhookRequestFromJson(strings.NewReader(payload))
+	if iwr == nil {
+		t.Fatal("IncomingWebhookRequest should not be nil")
+	}
+	if len(iwr.Attachments) != 1 {
+		t.Fatalf("expected one attachment")
+	}
+	if len(iwr.Attachments[0].Fields) != 1 {
+		t.Fatalf("expected one field")
+	}
+}
diff --git a/model/slack_attachment.go b/model/slack_attachment.go
index 6fd0071b4..a3199c44c 100644
--- a/model/slack_attachment.go
+++ b/model/slack_attachment.go
@@ -3,6 +3,11 @@
 
 package model
 
+import (
+	"fmt"
+	"strings"
+)
+
 type SlackAttachment struct {
 	Id         int64                   `json:"id"`
 	Fallback   string                  `json:"fallback"`
@@ -27,3 +32,50 @@ type SlackAttachmentField struct {
 	Value interface{} `json:"value"`
 	Short bool        `json:"short"`
 }
+
+type SlackAttachments []*SlackAttachment
+
+// To mention @channel via a webhook in Slack, the message should contain
+// <!channel>, as explained at the bottom of this article:
+// https://get.slack.help/hc/en-us/articles/202009646-Making-announcements
+func ExpandAnnouncement(text string) string {
+	c1 := "<!channel>"
+	c2 := "@channel"
+	if strings.Contains(text, c1) {
+		return strings.Replace(text, c1, c2, -1)
+	}
+	return text
+}
+
+// Expand announcements in incoming webhooks from Slack. Those announcements
+// can be found in the text attribute, or in the pretext, text, title and value
+// attributes of the attachment structure. The Slack attachment structure is
+// documented here: https://api.slack.com/docs/attachments
+func (a *SlackAttachments) Process() {
+	var nonNilAttachments []*SlackAttachment
+	for _, attachment := range *a {
+		if attachment == nil {
+			continue
+		}
+		nonNilAttachments = append(nonNilAttachments, attachment)
+
+		attachment.Pretext = ExpandAnnouncement(attachment.Pretext)
+		attachment.Text = ExpandAnnouncement(attachment.Text)
+		attachment.Title = ExpandAnnouncement(attachment.Title)
+
+		var nonNilFields []*SlackAttachmentField
+		for _, field := range attachment.Fields {
+			if field == nil {
+				continue
+			}
+			nonNilFields = append(nonNilFields, field)
+
+			if field.Value != nil {
+				// Ensure the value is set to a string if it is set
+				field.Value = ExpandAnnouncement(fmt.Sprintf("%v", field.Value))
+			}
+		}
+		attachment.Fields = nonNilFields
+	}
+	*a = nonNilAttachments
+}
diff --git a/model/slack_attachment_test.go b/model/slack_attachment_test.go
new file mode 100644
index 000000000..ff4df888f
--- /dev/null
+++ b/model/slack_attachment_test.go
@@ -0,0 +1,38 @@
+package model
+
+import (
+	"testing"
+)
+
+func TestExpandAnnouncement(t *testing.T) {
+	if ExpandAnnouncement("<!channel> foo <!channel>") != "@channel foo @channel" {
+		t.Fail()
+	}
+}
+
+func TestSlackAnnouncementProcess(t *testing.T) {
+	attachments := SlackAttachments{
+		{
+			Pretext: "<!channel> pretext",
+			Text:    "<!channel> text",
+			Title:   "<!channel> title",
+			Fields: []*SlackAttachmentField{
+				{
+					Title: "foo",
+					Value: "<!channel> bar",
+					Short: true,
+				}, nil,
+			},
+		}, nil,
+	}
+	attachments.Process()
+	if len(attachments) != 1 || len(attachments[0].Fields) != 1 {
+		t.Fail()
+	}
+	if attachments[0].Pretext != "@channel pretext" ||
+		attachments[0].Text != "@channel text" ||
+		attachments[0].Title != "@channel title" ||
+		attachments[0].Fields[0].Value != "@channel bar" {
+		t.Fail()
+	}
+}