MM-10201: querystring for get slash commands (#8779)

* pass GET slash command payloads through query string

Previously, both GET and POST requests received the payload via the
body, but this was incorrect for GET requests. Now, the payloads for GET
requests is sent via the query string.

* reorder tests for clarity

* switch command tests to use httptest servers

* restore original test command endpoints

2 files changed, 135 insertions, 60 deletions

diff --git a/api4/command_test.go b/api4/command_test.go
index 8c4ce5d50..0d37d7440 100644
--- a/api4/command_test.go
+++ b/api4/command_test.go
@@ -5,9 +5,13 @@ package api4
 
 import (
 	"fmt"
-	"strings"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/mattermost/mattermost-server/model"
 )
 
@@ -392,7 +396,7 @@ func TestRegenToken(t *testing.T) {
 	}
 }
 
-func TestExecuteCommand(t *testing.T) {
+func TestExecuteInvalidCommand(t *testing.T) {
 	th := Setup().InitBasic().InitSystemAdmin()
 	defer th.TearDown()
 	Client := th.Client
@@ -407,101 +411,170 @@ func TestExecuteCommand(t *testing.T) {
 		})
 	}()
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true })
-	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost" })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" })
 
-	postCmd := &model.Command{
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		rc := &model.CommandResponse{}
+
+		w.Write([]byte(rc.ToJson()))
+	}))
+	defer ts.Close()
+
+	getCmd := &model.Command{
 		CreatorId: th.BasicUser.Id,
 		TeamId:    th.BasicTeam.Id,
-		URL:       fmt.Sprintf("http://localhost:%v", th.App.Srv.ListenAddr.Port) + model.API_URL_SUFFIX_V4 + "/teams/command_test",
-		Method:    model.COMMAND_METHOD_POST,
-		Trigger:   "postcommand",
+		URL:       fmt.Sprintf("%s/%s/teams/command_test", ts.URL, model.API_URL_SUFFIX_V4),
+		Method:    model.COMMAND_METHOD_GET,
+		Trigger:   "getcommand",
 	}
 
-	if _, err := th.App.CreateCommand(postCmd); err != nil {
-		t.Fatal("failed to create post command")
+	if _, err := th.App.CreateCommand(getCmd); err != nil {
+		t.Fatal("failed to create get command")
 	}
 
-	commandResponse, resp := Client.ExecuteCommand(channel.Id, "/postcommand")
+	_, resp := Client.ExecuteCommand(channel.Id, "")
+	CheckBadRequestStatus(t, resp)
+
+	_, resp = Client.ExecuteCommand(channel.Id, "/")
+	CheckBadRequestStatus(t, resp)
+
+	_, resp = Client.ExecuteCommand(channel.Id, "getcommand")
+	CheckBadRequestStatus(t, resp)
+
+	_, resp = Client.ExecuteCommand(channel.Id, "/junk")
+	CheckNotFoundStatus(t, resp)
+
+	otherUser := th.CreateUser()
+	Client.Login(otherUser.Email, otherUser.Password)
+
+	_, resp = Client.ExecuteCommand(channel.Id, "/getcommand")
+	CheckForbiddenStatus(t, resp)
+
+	Client.Logout()
+
+	_, resp = Client.ExecuteCommand(channel.Id, "/getcommand")
+	CheckUnauthorizedStatus(t, resp)
+
+	_, resp = th.SystemAdminClient.ExecuteCommand(channel.Id, "/getcommand")
 	CheckNoError(t, resp)
+}
 
-	if commandResponse == nil {
-		t.Fatal("command response should have returned")
-	}
+func TestExecuteGetCommand(t *testing.T) {
+	th := Setup().InitBasic().InitSystemAdmin()
+	defer th.TearDown()
+	Client := th.Client
+	channel := th.BasicChannel
 
-	posts, err := th.App.GetPostsPage(channel.Id, 0, 10)
-	if err != nil || posts == nil || len(posts.Order) != 3 {
-		t.Fatal("Test command failed to send")
+	enableCommands := *th.App.Config().ServiceSettings.EnableCommands
+	allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections
+	defer func() {
+		th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands })
+		th.App.UpdateConfig(func(cfg *model.Config) {
+			cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections
+		})
+	}()
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" })
+
+	token := model.NewId()
+	expectedCommandResponse := &model.CommandResponse{
+		Text:         "test get command response",
+		ResponseType: model.COMMAND_RESPONSE_TYPE_IN_CHANNEL,
+		Type:         "custom_test",
+		Props:        map[string]interface{}{"someprop": "somevalue"},
 	}
 
-	cmdPosted := false
-	for _, post := range posts.Posts {
-		if strings.Contains(post.Message, "test command response") {
-			if post.Type != "custom_test" {
-				t.Fatal("wrong type set in slash command post")
-			}
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, http.MethodGet, r.Method)
 
-			if post.Props["someprop"] != "somevalue" {
-				t.Fatal("wrong prop set in slash command post")
-			}
+		values, err := url.ParseQuery(r.URL.RawQuery)
+		require.NoError(t, err)
 
-			cmdPosted = true
-			break
-		}
-	}
+		require.Equal(t, token, values.Get("token"))
+		require.Equal(t, th.BasicTeam.Name, values.Get("team_domain"))
 
-	if !cmdPosted {
-		t.Fatal("Test command response failed to post")
-	}
+		w.Header().Set("Content-Type", "application/json")
+		w.Write([]byte(expectedCommandResponse.ToJson()))
+	}))
+	defer ts.Close()
 
 	getCmd := &model.Command{
 		CreatorId: th.BasicUser.Id,
 		TeamId:    th.BasicTeam.Id,
-		URL:       fmt.Sprintf("http://localhost:%v", th.App.Srv.ListenAddr.Port) + model.API_URL_SUFFIX_V4 + "/teams/command_test",
+		URL:       fmt.Sprintf("%s/%s/teams/command_test", ts.URL, model.API_URL_SUFFIX_V4),
 		Method:    model.COMMAND_METHOD_GET,
 		Trigger:   "getcommand",
+		Token:     token,
 	}
 
 	if _, err := th.App.CreateCommand(getCmd); err != nil {
 		t.Fatal("failed to create get command")
 	}
 
-	commandResponse, resp = Client.ExecuteCommand(channel.Id, "/getcommand")
+	commandResponse, resp := Client.ExecuteCommand(channel.Id, "/getcommand")
 	CheckNoError(t, resp)
 
-	if commandResponse == nil {
-		t.Fatal("command response should have returned")
-	}
+	expectedCommandResponse.Props["from_webhook"] = "true"
+	require.Equal(t, expectedCommandResponse, commandResponse)
+}
 
-	posts, err = th.App.GetPostsPage(channel.Id, 0, 10)
-	if err != nil || posts == nil || len(posts.Order) != 4 {
-		t.Fatal("Test command failed to send")
-	}
+func TestExecutePostCommand(t *testing.T) {
+	th := Setup().InitBasic().InitSystemAdmin()
+	defer th.TearDown()
+	Client := th.Client
+	channel := th.BasicChannel
 
-	_, resp = Client.ExecuteCommand(channel.Id, "")
-	CheckBadRequestStatus(t, resp)
+	enableCommands := *th.App.Config().ServiceSettings.EnableCommands
+	allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections
+	defer func() {
+		th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableCommands = &enableCommands })
+		th.App.UpdateConfig(func(cfg *model.Config) {
+			cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections
+		})
+	}()
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableCommands = true })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "127.0.0.0/8" })
 
-	_, resp = Client.ExecuteCommand(channel.Id, "/")
-	CheckBadRequestStatus(t, resp)
+	token := model.NewId()
+	expectedCommandResponse := &model.CommandResponse{
+		Text:         "test post command response",
+		ResponseType: model.COMMAND_RESPONSE_TYPE_IN_CHANNEL,
+		Type:         "custom_test",
+		Props:        map[string]interface{}{"someprop": "somevalue"},
+	}
 
-	_, resp = Client.ExecuteCommand(channel.Id, "getcommand")
-	CheckBadRequestStatus(t, resp)
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, http.MethodPost, r.Method)
 
-	_, resp = Client.ExecuteCommand(channel.Id, "/junk")
-	CheckNotFoundStatus(t, resp)
+		r.ParseForm()
 
-	otherUser := th.CreateUser()
-	Client.Login(otherUser.Email, otherUser.Password)
+		require.Equal(t, token, r.FormValue("token"))
+		require.Equal(t, th.BasicTeam.Name, r.FormValue("team_domain"))
 
-	_, resp = Client.ExecuteCommand(channel.Id, "/getcommand")
-	CheckForbiddenStatus(t, resp)
+		w.Header().Set("Content-Type", "application/json")
+		w.Write([]byte(expectedCommandResponse.ToJson()))
+	}))
+	defer ts.Close()
 
-	Client.Logout()
+	getCmd := &model.Command{
+		CreatorId: th.BasicUser.Id,
+		TeamId:    th.BasicTeam.Id,
+		URL:       fmt.Sprintf("%s/%s/teams/command_test", ts.URL, model.API_URL_SUFFIX_V4),
+		Method:    model.COMMAND_METHOD_POST,
+		Trigger:   "postcommand",
+		Token:     token,
+	}
 
-	_, resp = Client.ExecuteCommand(channel.Id, "/getcommand")
-	CheckUnauthorizedStatus(t, resp)
+	if _, err := th.App.CreateCommand(getCmd); err != nil {
+		t.Fatal("failed to create get command")
+	}
 
-	_, resp = th.SystemAdminClient.ExecuteCommand(channel.Id, "/getcommand")
+	commandResponse, resp := Client.ExecuteCommand(channel.Id, "/postcommand")
 	CheckNoError(t, resp)
+
+	expectedCommandResponse.Props["from_webhook"] = "true"
+	require.Equal(t, expectedCommandResponse, commandResponse)
+
 }
 
 func TestExecuteCommandAgainstChannelOnAnotherTeam(t *testing.T) {
diff --git a/app/command.go b/app/command.go
index 796d656a7..92c35865a 100644
--- a/app/command.go
+++ b/app/command.go
@@ -230,12 +230,14 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, *
 					p.Set("response_url", args.SiteURL+"/hooks/commands/"+hook.Id)
 				}
 
-				method := "POST"
+				var req *http.Request
 				if cmd.Method == model.COMMAND_METHOD_GET {
-					method = "GET"
+					req, _ = http.NewRequest(http.MethodGet, cmd.URL, nil)
+					req.URL.RawQuery = p.Encode()
+				} else {
+					req, _ = http.NewRequest(http.MethodPost, cmd.URL, strings.NewReader(p.Encode()))
 				}
 
-				req, _ := http.NewRequest(method, cmd.URL, strings.NewReader(p.Encode()))
 				req.Header.Set("Accept", "application/json")
 				req.Header.Set("Authorization", "Token "+cmd.Token)
 				if cmd.Method == model.COMMAND_METHOD_POST {