[GH-16199] add IsValid to model OpenDialogRequest (#26526)

* [GH-16199] add IsValid to model OpenDialogRequest * [GH-16199] add IsValid to model OpenDialogRequest, revert remove of translations * [GH-16199] fix tests after revert * [GH-16199] add IsValid to model OpenDialogRequest * [GH-16199] revert validation of icon url * [GH-16199] update go sum * [GH-16199] log warning for invalid dialog * [GH-16199] log error for invalid dialog * [GH-16199] log warning for invalid dialog * [GH-16199] fix golang-ci --------- Co-authored-by: Lukas Eipert <git@leipert.io> Co-authored-by: Mattermost Build <build@mattermost.com>
2025-02-25 18:55:24 -06:00 · 2024-04-30 18:27:07 +02:00
parent b70e657271
commit 48b047aa0c
10 changed files with 408 additions and 8 deletions
--- a/server/channels/api4/integration_action.go
+++ b/server/channels/api4/integration_action.go
@@ -84,7 +84,7 @@ func openDialog(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	if appErr := c.App.OpenInteractiveDialog(dialog); appErr != nil {
+	if appErr := c.App.OpenInteractiveDialog(c.AppContext, dialog); appErr != nil {
 		c.Err = appErr
 		return
 	}
--- a/server/channels/api4/integration_action_test.go
+++ b/server/channels/api4/integration_action_test.go
@@ -16,6 +16,8 @@ import (
 	"github.com/stretchr/testify/require"

 	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/mattermost/mattermost/server/public/shared/mlog"
+	"github.com/mattermost/mattermost/server/v8/channels/testlib"
 )

 type testHandler struct {
@@ -209,6 +211,53 @@ func TestOpenDialog(t *testing.T) {
 		require.NoError(t, err)
 	})

+	t.Run("Should pass with too long display name of elements", func(t *testing.T) {
+		request.Dialog.Elements = []model.DialogElement{
+			{
+				DisplayName: "Very very long Element Name",
+				Name:        "element_name",
+				Type:        "text",
+				Placeholder: "Enter a value",
+			},
+		}
+
+		buffer := &mlog.Buffer{}
+		err := mlog.AddWriterTarget(th.TestLogger, buffer, true, mlog.StdAll...)
+		require.NoError(t, err)
+
+		_, err = client.OpenInteractiveDialog(context.Background(), request)
+		require.NoError(t, err)
+
+		require.NoError(t, th.TestLogger.Flush())
+		testlib.AssertLog(t, buffer, mlog.LvlWarn.Name, "Interactive dialog is invalid")
+	})
+
+	t.Run("Should pass with same elements", func(t *testing.T) {
+		request.Dialog.Elements = []model.DialogElement{
+			{
+				DisplayName: "Element Name",
+				Name:        "element_name",
+				Type:        "text",
+				Placeholder: "Enter a value",
+			},
+			{
+				DisplayName: "Element Name",
+				Name:        "element_name",
+				Type:        "text",
+				Placeholder: "Enter a value",
+			},
+		}
+		buffer := &mlog.Buffer{}
+		err := mlog.AddWriterTarget(th.TestLogger, buffer, true, mlog.StdAll...)
+		require.NoError(t, err)
+
+		_, err = client.OpenInteractiveDialog(context.Background(), request)
+		require.NoError(t, err)
+
+		require.NoError(t, th.TestLogger.Flush())
+		testlib.AssertLog(t, buffer, mlog.LvlWarn.Name, "Interactive dialog is invalid")
+	})
+
 	t.Run("Should pass with nil elements slice", func(t *testing.T) {
 		request.Dialog.Elements = nil
 		_, err := client.OpenInteractiveDialog(context.Background(), request)
--- a/server/channels/app/app_iface.go
+++ b/server/channels/app/app_iface.go
@@ -967,7 +967,7 @@ type AppIface interface {
 	NotificationsLog() *mlog.Logger
 	NotifySelfHostedSignupProgress(progress string, userId string)
 	NotifySharedChannelUserUpdate(user *model.User)
-	OpenInteractiveDialog(request model.OpenDialogRequest) *model.AppError
+	OpenInteractiveDialog(c request.CTX, request model.OpenDialogRequest) *model.AppError
 	OriginChecker() func(*http.Request) bool
 	OutgoingOAuthConnections() einterfaces.OutgoingOAuthConnectionInterface
 	PatchChannel(c request.CTX, channel *model.Channel, patch *model.ChannelPatch, userID string) (*model.Channel, *model.AppError)
--- a/server/channels/app/integration_action.go
+++ b/server/channels/app/integration_action.go
@@ -453,13 +453,17 @@ func (a *App) DoLocalRequest(c request.CTX, rawURL string, body []byte) (*http.R
 	return a.doPluginRequest(c, "POST", rawURL, nil, body)
 }

-func (a *App) OpenInteractiveDialog(request model.OpenDialogRequest) *model.AppError {
+func (a *App) OpenInteractiveDialog(c request.CTX, request model.OpenDialogRequest) *model.AppError {
 	timeout := time.Duration(*a.Config().ServiceSettings.OutgoingIntegrationRequestsTimeout) * time.Second
 	clientTriggerId, userID, appErr := request.DecodeAndVerifyTriggerId(a.AsymmetricSigningKey(), timeout)
 	if appErr != nil {
 		return appErr
 	}

+	if dialogErr := request.IsValid(); dialogErr != nil {
+		c.Logger().Warn("Interactive dialog is invalid", mlog.Err(dialogErr))
+	}
+
 	request.TriggerId = clientTriggerId

 	jsonRequest, err := json.Marshal(request)
--- a/server/channels/app/opentracing/opentracing_layer.go
+++ b/server/channels/app/opentracing/opentracing_layer.go
@@ -13181,7 +13181,7 @@ func (a *OpenTracingAppLayer) OnSharedChannelsSyncMsg(msg *model.SyncMsg, rc *mo
 	return resultVar0, resultVar1
 }

-func (a *OpenTracingAppLayer) OpenInteractiveDialog(request model.OpenDialogRequest) *model.AppError {
+func (a *OpenTracingAppLayer) OpenInteractiveDialog(c request.CTX, request model.OpenDialogRequest) *model.AppError {
 	origCtx := a.ctx
 	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.OpenInteractiveDialog")

@@ -13193,7 +13193,7 @@ func (a *OpenTracingAppLayer) OpenInteractiveDialog(request model.OpenDialogRequ
 	}()

 	defer span.Finish()
-	resultVar0 := a.app.OpenInteractiveDialog(request)
+	resultVar0 := a.app.OpenInteractiveDialog(c, request)

 	if resultVar0 != nil {
 		span.LogFields(spanlog.Error(resultVar0))
--- a/server/channels/app/plugin_api.go
+++ b/server/channels/app/plugin_api.go
@@ -849,7 +849,7 @@ func (api *PluginAPI) SetTeamIcon(teamID string, data []byte) *model.AppError {
 }

 func (api *PluginAPI) OpenInteractiveDialog(dialog model.OpenDialogRequest) *model.AppError {
-	return api.app.OpenInteractiveDialog(dialog)
+	return api.app.OpenInteractiveDialog(api.ctx, dialog)
 }

 func (api *PluginAPI) RemoveTeamIcon(teamID string) *model.AppError {
--- a/server/public/go.mod
+++ b/server/public/go.mod
@@ -13,6 +13,7 @@ require (
 	github.com/gorilla/mux v1.8.1
 	github.com/gorilla/websocket v1.5.1
 	github.com/hashicorp/go-hclog v1.6.2
+	github.com/hashicorp/go-multierror v1.1.1
 	github.com/hashicorp/go-plugin v1.6.0
 	github.com/lib/pq v1.10.9
 	github.com/mattermost/go-i18n v1.11.1-0.20211013152124-5c415071e404
@@ -40,6 +41,7 @@ require (
 	github.com/go-asn1-ber/asn1-ber v1.5.5 // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/google/uuid v1.6.0 // indirect
+	github.com/hashicorp/errwrap v1.0.0 // indirect
 	github.com/hashicorp/yamux v0.1.1 // indirect
 	github.com/kr/pretty v0.3.0 // indirect
 	github.com/mattn/go-colorable v0.1.13 // indirect
--- a/server/public/go.sum
+++ b/server/public/go.sum
@@ -77,8 +77,12 @@ github.com/gorilla/websocket v1.5.1 h1:gmztn0JnHVt9JZquRuzLw3g4wouNVzKL15iLr/zn/
 github.com/gorilla/websocket v1.5.1/go.mod h1:x3kM2JMyaluk02fnUJpQuwD2dCS5NDG2ZHL0uE0tcaY=
 github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
 github.com/grpc-ecosystem/grpc-gateway v1.5.0/go.mod h1:RSKVYQBd5MCa4OVpNdGskqpgL2+G+NZTnrVHpWWfpdw=
+github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA=
+github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
 github.com/hashicorp/go-hclog v1.6.2 h1:NOtoftovWkDheyUM/8JW3QMiXyxJK3uHRK7wV04nD2I=
 github.com/hashicorp/go-hclog v1.6.2/go.mod h1:W4Qnvbt70Wk/zYJryRzDRU/4r0kIg0PVHBcfoyhpF5M=
+github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo=
+github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM=
 github.com/hashicorp/go-plugin v1.6.0 h1:wgd4KxHJTVGGqWBq4QPB1i5BZNEx9BR8+OFmHDmTk8A=
 github.com/hashicorp/go-plugin v1.6.0/go.mod h1:lBS5MtSSBZk0SHc66KACcjjlU6WzEVP/8pwz68aMkCI=
 github.com/hashicorp/yamux v0.1.1 h1:yrQxtgseBDrq9Y652vSRDvsKCJKOUD+GzTS4Y0Y8pvE=
--- a/server/public/model/integration_action.go
+++ b/server/public/model/integration_action.go
@@ -20,11 +20,22 @@ import (
 	"strconv"
 	"strings"
 	"time"
+
+	"github.com/hashicorp/go-multierror"
+	"github.com/pkg/errors"
 )

 const (
-	PostActionTypeButton = "button"
-	PostActionTypeSelect = "select"
+	PostActionTypeButton              = "button"
+	PostActionTypeSelect              = "select"
+	DialogTitleMaxLength              = 24
+	DialogElementDisplayNameMaxLength = 24
+	DialogElementNameMaxLength        = 300
+	DialogElementHelpTextMaxLength    = 150
+	DialogElementTextMaxLength        = 150
+	DialogElementTextareaMaxLength    = 3000
+	DialogElementSelectMaxLength      = 3000
+	DialogElementBoolMaxLength        = 150
 )

 var PostActionRetainPropKeys = []string{"from_webhook", "override_username", "override_icon_url"}
@@ -335,6 +346,148 @@ func (r *OpenDialogRequest) DecodeAndVerifyTriggerId(s *ecdsa.PrivateKey, timeou
 	return DecodeAndVerifyTriggerId(r.TriggerId, s, timeout)
 }

+func (r *OpenDialogRequest) IsValid() error {
+	var multiErr *multierror.Error
+	if r.URL == "" {
+		multiErr = multierror.Append(multiErr, errors.New("empty URL"))
+	}
+
+	if r.TriggerId == "" {
+		multiErr = multierror.Append(multiErr, errors.New("empty trigger id"))
+	}
+
+	err := r.Dialog.IsValid()
+	if err != nil {
+		multiErr = multierror.Append(multiErr, err)
+	}
+
+	return multiErr.ErrorOrNil()
+}
+
+func (d *Dialog) IsValid() error {
+	var multiErr *multierror.Error
+
+	if d.Title == "" || len(d.Title) > DialogTitleMaxLength {
+		multiErr = multierror.Append(multiErr, errors.Errorf("invalid dialog title %q", d.Title))
+	}
+
+	if d.IconURL != "" && !IsValidHTTPURL(d.IconURL) {
+		multiErr = multierror.Append(multiErr, errors.New("invalid icon url"))
+	}
+
+	if len(d.Elements) != 0 {
+		elementMap := make(map[string]bool)
+
+		for _, element := range d.Elements {
+			if elementMap[element.Name] {
+				multiErr = multierror.Append(multiErr, errors.Errorf("duplicate dialog element %q", element.Name))
+			}
+			elementMap[element.Name] = true
+
+			err := element.IsValid()
+			if err != nil {
+				multiErr = multierror.Append(multiErr, errors.Wrapf(err, "%q field is not valid", element.Name))
+			}
+		}
+	}
+	return multiErr.ErrorOrNil()
+}
+
+func (e *DialogElement) IsValid() error {
+	var multiErr *multierror.Error
+	textSubTypes := map[string]bool{
+		"":         true,
+		"text":     true,
+		"email":    true,
+		"number":   true,
+		"tel":      true,
+		"url":      true,
+		"password": true,
+	}
+
+	if e.MinLength < 0 {
+		multiErr = multierror.Append(multiErr, errors.Errorf("min length cannot be a negative number, got %d", e.MinLength))
+	}
+	if e.MinLength > e.MaxLength {
+		multiErr = multierror.Append(multiErr, errors.Errorf("min length should be less then max length, got %d > %d", e.MinLength, e.MaxLength))
+	}
+
+	multiErr = multierror.Append(multiErr, checkMaxLength("DisplayName", e.DisplayName, DialogElementDisplayNameMaxLength))
+	multiErr = multierror.Append(multiErr, checkMaxLength("Name", e.Name, DialogElementNameMaxLength))
+	multiErr = multierror.Append(multiErr, checkMaxLength("HelpText", e.HelpText, DialogElementHelpTextMaxLength))
+
+	switch e.Type {
+	case "text":
+		multiErr = multierror.Append(multiErr, checkMaxLength("Default", e.Default, DialogElementTextMaxLength))
+		multiErr = multierror.Append(multiErr, checkMaxLength("Placeholder", e.Placeholder, DialogElementTextMaxLength))
+		if _, ok := textSubTypes[e.SubType]; !ok {
+			multiErr = multierror.Append(multiErr, errors.Errorf("invalid subtype %q", e.Type))
+		}
+
+	case "textarea":
+		multiErr = multierror.Append(multiErr, checkMaxLength("Default", e.Default, DialogElementTextareaMaxLength))
+		multiErr = multierror.Append(multiErr, checkMaxLength("Placeholder", e.Placeholder, DialogElementTextareaMaxLength))
+
+		if _, ok := textSubTypes[e.SubType]; !ok {
+			multiErr = multierror.Append(multiErr, errors.Errorf("invalid subtype %q", e.Type))
+		}
+
+	case "select":
+		multiErr = multierror.Append(multiErr, checkMaxLength("Default", e.Default, DialogElementSelectMaxLength))
+		multiErr = multierror.Append(multiErr, checkMaxLength("Placeholder", e.Placeholder, DialogElementSelectMaxLength))
+		if e.DataSource != "" && e.DataSource != "users" && e.DataSource != "channels" {
+			multiErr = multierror.Append(multiErr, errors.Errorf("invalid data source %q, allowed are 'users' or 'channels'", e.DataSource))
+		}
+		if e.DataSource == "" && !isDefaultInOptions(e.Default, e.Options) {
+			multiErr = multierror.Append(multiErr, errors.Errorf("default value %q doesn't exist in options ", e.Default))
+		}
+
+	case "bool":
+		if e.Default != "" && e.Default != "true" && e.Default != "false" {
+			multiErr = multierror.Append(multiErr, errors.New("invalid default of bool"))
+		}
+		multiErr = multierror.Append(multiErr, checkMaxLength("Placeholder", e.Placeholder, DialogElementBoolMaxLength))
+
+	case "radio":
+		if !isDefaultInOptions(e.Default, e.Options) {
+			multiErr = multierror.Append(multiErr, errors.Errorf("default value %q doesn't exist in options ", e.Default))
+		}
+
+	default:
+		multiErr = multierror.Append(multiErr, errors.Errorf("invalid element type: %q", e.Type))
+	}
+
+	return multiErr.ErrorOrNil()
+}
+
+func isDefaultInOptions(defaultValue string, options []*PostActionOptions) bool {
+	if defaultValue == "" {
+		return true
+	}
+
+	for _, option := range options {
+		if defaultValue == option.Value {
+			return true
+		}
+	}
+
+	return false
+}
+
+func checkMaxLength(fieldName string, field string, length int) error {
+	var valid bool
+	// DisplayName and Name are required fields
+	if fieldName == "DisplayName" || fieldName == "Name" {
+		valid = len(field) > 0 && len(field) > length
+	} else {
+		valid = len(field) > length
+	}
+	if valid {
+		return errors.Errorf("%v cannot be longer than %d characters", fieldName, length)
+	}
+	return nil
+}
+
 func (o *Post) StripActionIntegrations() {
 	attachments := o.Attachments()
 	if o.GetProp("attachments") != nil {
--- a/server/public/model/integration_action_test.go
+++ b/server/public/model/integration_action_test.go
@@ -8,6 +8,7 @@ import (
 	"crypto/elliptic"
 	"crypto/rand"
 	"encoding/base64"
+	"strings"
 	"testing"
 	"time"

@@ -180,3 +181,190 @@ func TestPostActionIntegrationEquals(t *testing.T) {
 		require.True(t, pa1.Equals(pa2))
 	})
 }
+
+func TestOpenDialogRequestIsValid(t *testing.T) {
+	getBaseOpenDialogRequest := func() OpenDialogRequest {
+		return OpenDialogRequest{
+			TriggerId: "triggerId",
+			URL:       "http://localhost:8065",
+			Dialog: Dialog{
+				CallbackId: "callbackid",
+				Title:      "Some Title",
+				Elements: []DialogElement{
+					{
+						DisplayName: "Element Name",
+						Name:        "element_name",
+						Type:        "text",
+						Placeholder: "Enter a value",
+					},
+				},
+				SubmitLabel:    "Submit",
+				NotifyOnCancel: false,
+				State:          "somestate",
+			},
+		}
+	}
+
+	t.Run("should pass validation", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		err := request.IsValid()
+		assert.NoError(t, err)
+	})
+
+	t.Run("should fail on empty url", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.URL = ""
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "empty URL")
+	})
+
+	t.Run("should fail on empty trigger", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.TriggerId = ""
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "empty trigger id")
+	})
+
+	t.Run("should fail on empty dialog title", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Title = ""
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "invalid dialog title")
+	})
+
+	t.Run("should fail on wrong subtype and long dialog title", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements[0].SubType = "wrong SubType"
+		request.Dialog.Title = "Very very long Dialog Name"
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "invalid subtype")
+		assert.ErrorContains(t, err, "invalid dialog title")
+		t.Cleanup(func() {
+			request.Dialog.Elements[0].SubType = ""
+			request.Dialog.Title = "Some Title"
+		})
+	})
+
+	t.Run("should fail on wrong dialog icon url", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.IconURL = "wrong url"
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "invalid icon url")
+	})
+
+	t.Run("should pass on empty dialog icon url", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.IconURL = ""
+		err := request.IsValid()
+		assert.NoError(t, err)
+	})
+
+	t.Run("should fail on wrong minimal and maximal element length", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements[0].MinLength = 10
+		request.Dialog.Elements[0].MaxLength = 9
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "field is not valid")
+		assert.ErrorContains(t, err, "min length should be less then max length")
+	})
+
+	t.Run("should fail on wrong element type", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements[0].Type = "wrong type"
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "invalid element type")
+	})
+
+	t.Run("should fail on duplicate element name", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements = append(request.Dialog.Elements, DialogElement{
+			DisplayName: "Radio element name",
+			Name:        "element_name",
+			Type:        "radio",
+		})
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "duplicate dialog element")
+	})
+
+	t.Run("should fail on wrong bool default value", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements = append(request.Dialog.Elements, DialogElement{
+			DisplayName: "Bool element name",
+			Name:        "bool_element_name",
+			Type:        "bool",
+			Default:     "wrong default",
+		})
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "invalid default of bool")
+	})
+
+	t.Run("should pass on bool default value", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements = append(request.Dialog.Elements, DialogElement{
+			DisplayName: "Bool element name",
+			Name:        "bool_element_name",
+			Type:        "bool",
+			Default:     "true",
+		})
+		err := request.IsValid()
+		assert.NoError(t, err)
+	})
+
+	t.Run("should fail on wrong select datasource value", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements = append(request.Dialog.Elements, DialogElement{
+			DisplayName: "Select element name",
+			Name:        "select_element_name",
+			Type:        "select",
+			DataSource:  "wrong DataSource",
+		})
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "invalid data source")
+	})
+
+	t.Run("should fail on wrong radio default value", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements = append(request.Dialog.Elements, DialogElement{
+			DisplayName: "Radio element name",
+			Name:        "radio_element_name",
+			Type:        "radio",
+			Default:     "default",
+			Options: []*PostActionOptions{
+				{
+					Text:  "Text 1",
+					Value: "value 1",
+				},
+			},
+		})
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "default value \"default\" doesn't exist in options")
+	})
+	t.Run("should pass radio default value", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements = append(request.Dialog.Elements, DialogElement{
+			DisplayName: "Radio element name",
+			Name:        "radio_element_name",
+			Type:        "radio",
+			Default:     "default",
+			Options: []*PostActionOptions{
+				{
+					Text:  "Text 1",
+					Value: "value 1",
+				},
+				{
+					Text:  "Text 2",
+					Value: "default",
+				},
+			},
+		})
+		err := request.IsValid()
+		assert.NoError(t, err)
+	})
+
+	t.Run("should fail on too long text placeholder", func(t *testing.T) {
+		request := getBaseOpenDialogRequest()
+		request.Dialog.Elements[0].Placeholder = strings.Repeat("x", 151)
+		err := request.IsValid()
+		assert.ErrorContains(t, err, "Placeholder cannot be longer than 150 characters")
+	})
+}