Added additional validation for slack attachment format on server (#5680)

5 files changed, 143 insertions, 57 deletions

diff --git a/model/command_response.go b/model/command_response.go
index bbb70418e..f69772353 100644
--- a/model/command_response.go
+++ b/model/command_response.go
@@ -5,6 +5,7 @@ package model
 
 import (
 	"encoding/json"
+	"fmt"
 	"io"
 )
 
@@ -14,12 +15,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  interface{} `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  []*SlackAttachment `json:"attachments"`
 }
 
 func (o *CommandResponse) ToJson() string {
@@ -34,10 +35,19 @@ func (o *CommandResponse) ToJson() string {
 func CommandResponseFromJson(data io.Reader) *CommandResponse {
 	decoder := json.NewDecoder(data)
 	var o CommandResponse
-	err := decoder.Decode(&o)
-	if err == nil {
-		return &o
-	} else {
+
+	if err := decoder.Decode(&o); err != nil {
 		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)
+			}
+		}
+	}
+
+	return &o
 }
diff --git a/model/command_response_test.go b/model/command_response_test.go
index 7aa3e984b..131d87789 100644
--- a/model/command_response_test.go
+++ b/model/command_response_test.go
@@ -17,3 +17,68 @@ func TestCommandResponseJson(t *testing.T) {
 		t.Fatal("Ids do not match")
 	}
 }
+
+func TestCommandResponseFromJson(t *testing.T) {
+	json := `{
+		"response_type": "ephemeral",
+		"text": "response text",
+		"username": "response username",
+		"icon_url": "response icon url",
+		"goto_location": "response goto location",
+		"attachments": [{
+			"text": "attachment 1 text",
+			"pretext": "attachment 1 pretext"
+		},{
+			"text": "attachment 2 text",
+			"fields": [{
+				"title": "field 1",
+				"value": "value 1",
+				"short": true
+			},{
+				"title": "field 2",
+				"value": [],
+				"short": false
+			}]
+		}]
+	}`
+
+	response := CommandResponseFromJson(strings.NewReader(json))
+
+	if response == nil {
+		t.Fatal("should've received non-nil CommandResponse")
+	}
+
+	if response.ResponseType != "ephemeral" {
+		t.Fatal("should've received correct response type")
+	} else if response.Text != "response text" {
+		t.Fatal("should've received correct response text")
+	} else if response.Username != "response username" {
+		t.Fatal("should've received correct response username")
+	} else if response.IconURL != "response icon url" {
+		t.Fatal("should've received correct response icon url")
+	} else if response.GotoLocation != "response goto location" {
+		t.Fatal("should've received correct response goto location")
+	}
+
+	attachments := response.Attachments
+	if len(attachments) != 2 {
+		t.Fatal("should've received 2 attachments")
+	} else if attachments[0].Text != "attachment 1 text" {
+		t.Fatal("should've received correct first attachment text")
+	} else if attachments[0].Pretext != "attachment 1 pretext" {
+		t.Fatal("should've received correct first attachment pretext")
+	} else if attachments[1].Text != "attachment 2 text" {
+		t.Fatal("should've received correct second attachment text")
+	}
+
+	fields := attachments[1].Fields
+	if len(fields) != 2 {
+		t.Fatal("should've received 2 fields")
+	} else if fields[0].Value.(string) != "value 1" {
+		t.Fatal("should've received correct first attachment value")
+	} else if _, ok := fields[1].Value.(string); !ok {
+		t.Fatal("should've received second attachment value parsed as a string")
+	} else if fields[1].Value.(string) != "[]" {
+		t.Fatal("should've received correct second attachment value")
+	}
+}
diff --git a/model/incoming_webhook.go b/model/incoming_webhook.go
index 72fa3e54c..2cc26cbca 100644
--- a/model/incoming_webhook.go
+++ b/model/incoming_webhook.go
@@ -29,13 +29,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 interface{}     `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 []*SlackAttachment `json:"attachments"`
+	Type        string             `json:"type"`
 }
 
 func (o *IncomingWebhook) ToJson() string {
@@ -212,31 +212,15 @@ func expandAnnouncement(text string) string {
 func expandAnnouncements(i *IncomingWebhookRequest) {
 	i.Text = expandAnnouncement(i.Text)
 
-	if i.Attachments != nil {
-		attachments := i.Attachments.([]interface{})
-		for _, attachment := range attachments {
-			a := attachment.(map[string]interface{})
+	for _, attachment := range i.Attachments {
+		attachment.Pretext = expandAnnouncement(attachment.Pretext)
+		attachment.Text = expandAnnouncement(attachment.Text)
+		attachment.Title = expandAnnouncement(attachment.Title)
 
-			if a["pretext"] != nil {
-				a["pretext"] = expandAnnouncement(a["pretext"].(string))
-			}
-
-			if a["text"] != nil {
-				a["text"] = expandAnnouncement(a["text"].(string))
-			}
-
-			if a["title"] != nil {
-				a["title"] = expandAnnouncement(a["title"].(string))
-			}
-
-			if a["fields"] != nil {
-				fields := a["fields"].([]interface{})
-				for _, field := range fields {
-					f := field.(map[string]interface{})
-					if f["value"] != nil {
-						f["value"] = expandAnnouncement(fmt.Sprintf("%v", f["value"]))
-					}
-				}
+		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))
 			}
 		}
 	}
diff --git a/model/incoming_webhook_test.go b/model/incoming_webhook_test.go
index 8246b6c0a..46e5b6743 100644
--- a/model/incoming_webhook_test.go
+++ b/model/incoming_webhook_test.go
@@ -142,21 +142,20 @@ func TestIncomingWebhookRequestFromJson_Announcements(t *testing.T) {
 		t.Fatal("IncomingWebhookRequest should not be nil")
 	}
 
-	attachments := iwr.Attachments.([]interface{})
-	attachment := attachments[0].(map[string]interface{})
-	if attachment["pretext"] != expected {
-		t.Fatalf("Sample attachment pretext should be: %s, got: %s", expected, attachment["pretext"])
+	attachment := iwr.Attachments[0]
+	if attachment.Pretext != expected {
+		t.Fatalf("Sample attachment pretext should be:%s, got: %s", expected, attachment.Pretext)
 	}
-	if attachment["text"] != expected {
-		t.Fatalf("Sample attachment text should be: %s, got: %s", expected, attachment["text"])
+	if attachment.Text != expected {
+		t.Fatalf("Sample attachment text should be: %s, got: %s", expected, attachment.Text)
 	}
-	if attachment["title"] != expected {
-		t.Fatalf("Sample attachment title should be: %s, got: %s", expected, attachment["title"])
+	if attachment.Title != expected {
+		t.Fatalf("Sample attachment title should be: %s, got: %s", expected, attachment.Title)
 	}
-	fields := attachment["fields"].([]interface{})
-	field := fields[0].(map[string]interface{})
-	if field["value"] != expected {
-		t.Fatalf("Sample attachment field value should be: %s, got: %s", expected, field["value"])
+
+	field := attachment.Fields[0]
+	if field.Value != expected {
+		t.Fatalf("Sample attachment field value should be: %s, got: %s", expected, field.Value)
 	}
 }
 
@@ -224,10 +223,10 @@ func TestIncomingWebhookRequestFromJson(t *testing.T) {
 		if iwr.Text != expected {
 			t.Fatalf("Sample %d text should be: %s, got: %s", i, expected, iwr.Text)
 		}
-		attachments := iwr.Attachments.([]interface{})
-		attachment := attachments[0].(map[string]interface{})
-		if attachment["text"] != expected {
-			t.Fatalf("Sample %d attachment text should be: %s, got: %s", i, expected, attachment["text"])
+
+		attachment := iwr.Attachments[0]
+		if attachment.Text != expected {
+			t.Fatalf("Sample %d attachment text should be: %s, got: %s", i, expected, attachment.Text)
 		}
 	}
 }
diff --git a/model/slack_attachment.go b/model/slack_attachment.go
new file mode 100644
index 000000000..d68fe406f
--- /dev/null
+++ b/model/slack_attachment.go
@@ -0,0 +1,28 @@
+// Copyright (c) 2017 Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package model
+
+type SlackAttachment struct {
+	Fallback   string                  `json:"fallback"`
+	Color      string                  `json:"color"`
+	Pretext    string                  `json:"pretext"`
+	AuthorName string                  `json:"author_name"`
+	AuthorLink string                  `json:"author_link"`
+	AuthorIcon string                  `json:"author_icon"`
+	Title      string                  `json:"title"`
+	TitleLink  string                  `json:"title_link"`
+	Text       string                  `json:"text"`
+	Fields     []*SlackAttachmentField `json:"fields"`
+	ImageURL   string                  `json:"image_url"`
+	ThumbURL   string                  `json:"thumb_url"`
+	Footer     string                  `json:"footer"`
+	FooterIcon string                  `json:"footer_icon"`
+	Timestamp  int64                   `json:"ts"`
+}
+
+type SlackAttachmentField struct {
+	Title string      `json:"title"`
+	Value interface{} `json:"value"`
+	Short bool        `json:"short"`
+}