From 7d499d2750c35b027058844f7f52924cda26b7b9 Mon Sep 17 00:00:00 2001
From: Agniva De Sarker <agnivade@yahoo.co.in>
Date: Thu, 12 Dec 2019 23:13:33 +0530
Subject: [PATCH] MM-21116: Improve channelSearch SQL query generation (#13371)

* MM-21116: Improve channelSearch SQL query generation

The likeTerm from buildFulltextClause was being built inefficient using strings.
We change it to use squirrel to build the query.

And while at it, we also change the len check with checking with
an empty string which is more idiomatic. Both compile to the same
code, so there is no difference performance wise.

* Use a better variable name
---
 store/sqlstore/channel_store.go      | 12 ++++++++----
 store/sqlstore/channel_store_test.go | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go
index 989f3b5ed9..da7bd40840 100644
--- a/store/sqlstore/channel_store.go
+++ b/store/sqlstore/channel_store.go
@@ -2253,11 +2253,15 @@ func (s SqlChannelStore) channelSearchQuery(term string, opts store.ChannelSearc
 	}
 
 	likeClause, likeTerm := s.buildLIKEClause(term, "c.Name, c.DisplayName, c.Purpose")
-	if len(likeTerm) > 0 {
-		likeClause = strings.ReplaceAll(likeClause, ":LikeTerm", "'"+likeTerm+"'")
+	if likeTerm != "" {
+		likeClause = strings.ReplaceAll(likeClause, ":LikeTerm", "?")
 		fulltextClause, fulltextTerm := s.buildFulltextClause(term, "c.Name, c.DisplayName, c.Purpose")
-		fulltextClause = strings.ReplaceAll(fulltextClause, ":FulltextTerm", "'"+fulltextTerm+"'")
-		query = query.Where("(" + likeClause + " OR " + fulltextClause + ")")
+		fulltextClause = strings.ReplaceAll(fulltextClause, ":FulltextTerm", "?")
+		query = query.Where(sq.Or{
+			sq.Expr(likeClause, likeTerm, likeTerm, likeTerm), // Keep the number of likeTerms same as the number
+			// of columns (c.Name, c.DisplayName, c.Purpose)
+			sq.Expr(fulltextClause, fulltextTerm),
+		})
 	}
 
 	if len(opts.ExcludeChannelNames) > 0 {
diff --git a/store/sqlstore/channel_store_test.go b/store/sqlstore/channel_store_test.go
index 0cca273f54..99880fa686 100644
--- a/store/sqlstore/channel_store_test.go
+++ b/store/sqlstore/channel_store_test.go
@@ -8,8 +8,10 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/mattermost/mattermost-server/v5/store"
 	"github.com/mattermost/mattermost-server/v5/store/storetest"
 )
 
@@ -17,6 +19,22 @@ func TestChannelStore(t *testing.T) {
 	StoreTestWithSqlSupplier(t, storetest.TestChannelStore)
 }
 
+func TestChannelSearchQuerySQLInjection(t *testing.T) {
+	for _, st := range storeTypes {
+		t.Run(st.Name, func(t *testing.T) {
+			s := &SqlChannelStore{
+				SqlStore: st.SqlSupplier,
+			}
+
+			opts := store.ChannelSearchOpts{}
+			builder := s.channelSearchQuery("'or'1'=sleep(3))); -- -", opts, false)
+			query, _, err := builder.ToSql()
+			require.Nil(t, err)
+			assert.NotContains(t, query, "sleep")
+		})
+	}
+}
+
 func TestChannelStoreInternalDataTypes(t *testing.T) {
 	t.Run("NewChannelMemberFromModel", func(t *testing.T) { testNewChannelMemberFromModel(t) })
 	t.Run("ChannelMemberWithSchemeRolesToModel", func(t *testing.T) { testChannelMemberWithSchemeRolesToModel(t) })