MM-18636: limit configuration writes to 4Mb (#12266)

* MM-18636: limit configuration writes to 4Mb

By default, MySQL silently truncates writes that exceed the column type in question. Change the column type from `TEXT` to `MEDIUMTEXT` to allow writes to the `Configurations` and `ConfigurationFiles` table to exceed 65535 bytes.  This is a backwards compatible migration, but does require a rewrite of the table.

However, MySQL is further constrained by the default `max_allowed_packet` value of 4Mb, so limit writes accordingly.

Fixes: https://mattermost.atlassian.net/browse/MM-18636

* simplify unit tests

* fix import

config/database.go

@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"database/sql"
 	"io/ioutil"
+	"regexp"
 	"strings"
 
 	"github.com/jmoiron/sqlx"
@@ -21,6 +22,14 @@ import (
 	_ "github.com/lib/pq"
 )
 
+// MaxWriteLength defines the maximum length accepted for write to the Configurations or
+// ConfigurationFiles table.
+//
+// It is imposed by MySQL's default max_allowed_packet value of 4Mb.
+const MaxWriteLength = 4 * 1024 * 1024
+
+var tcpStripper = regexp.MustCompile(`@tcp\((.*)\)`)
+
 // DatabaseStore is a config store backed by a database.
 type DatabaseStore struct {
 	commonStore
@@ -61,6 +70,8 @@ func NewDatabaseStore(dsn string) (ds *DatabaseStore, err error) {
 }
 
 // initializeConfigurationsTable ensures the requisite tables in place to form the backing store.
+//
+// Uses MEDIUMTEXT on MySQL, and TEXT on sane databases.
 func initializeConfigurationsTable(db *sqlx.DB) error {
 	_, err := db.Exec(`
 		CREATE TABLE IF NOT EXISTS Configurations (
@@ -86,6 +97,20 @@ func initializeConfigurationsTable(db *sqlx.DB) error {
 		return errors.Wrap(err, "failed to create ConfigurationFiles table")
 	}
 
+	// Change from TEXT (65535 limit) to MEDIUM TEXT (16777215) on MySQL. This is a
+	// backwards-compatible migration for any existing schema.
+	if db.DriverName() == "mysql" {
+		_, err = db.Exec(`ALTER TABLE Configurations MODIFY Value MEDIUMTEXT`)
+		if err != nil {
+			return errors.Wrap(err, "failed to alter Configurations table")
+		}
+
+		_, err = db.Exec(`ALTER TABLE ConfigurationFiles MODIFY Data MEDIUMTEXT`)
+		if err != nil {
+			return errors.Wrap(err, "failed to alter ConfigurationFiles table")
+		}
+	}
+
 	return nil
 }
 
@@ -126,6 +151,15 @@ func (ds *DatabaseStore) Set(newCfg *model.Config) (*model.Config, error) {
 	return ds.commonStore.set(newCfg, true, ds.commonStore.validate, ds.persist)
 }
 
+// maxLength identifies the maximum length of a configuration or configuration file
+func (ds *DatabaseStore) checkLength(length int) error {
+	if ds.db.DriverName() == "mysql" && length > MaxWriteLength {
+		return errors.Errorf("value is too long: %d > %d bytes", length, MaxWriteLength)
+	}
+
+	return nil
+}
+
 // persist writes the configuration to the configured database.
 func (ds *DatabaseStore) persist(cfg *model.Config) error {
 	b, err := marshalConfig(cfg)
@@ -137,6 +171,11 @@ func (ds *DatabaseStore) persist(cfg *model.Config) error {
 	value := string(b)
 	createAt := model.GetMillis()
 
+	err = ds.checkLength(len(value))
+	if err != nil {
+		return errors.Wrap(err, "marshalled configuration failed length check")
+	}
+
 	tx, err := ds.db.Beginx()
 	if err != nil {
 		return errors.Wrap(err, "failed to begin transaction")
@@ -232,6 +271,11 @@ func (ds *DatabaseStore) GetFile(name string) ([]byte, error) {
 
 // SetFile sets or replaces the contents of a configuration file.
 func (ds *DatabaseStore) SetFile(name string, data []byte) error {
+	err := ds.checkLength(len(data))
+	if err != nil {
+		return errors.Wrap(err, "file data failed length check")
+	}
+
 	params := map[string]interface{}{
 		"name":      name,
 		"data":      data,

config/database_test.go

@@ -451,7 +451,6 @@ func TestDatabaseStoreSet(t *testing.T) {
 	})
 
 	t.Run("persist failed", func(t *testing.T) {
-		t.Skip("skipping persistence test inside Set")
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
@@ -466,13 +465,29 @@ func TestDatabaseStoreSet(t *testing.T) {
 		newCfg := &model.Config{}
 
 		_, err = ds.Set(newCfg)
-		if assert.Error(t, err) {
-			assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: failed to write to database"))
-		}
+		require.Error(t, err)
+		assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: failed to query active configuration"), "unexpected error: "+err.Error())
 
 		assert.Equal(t, "", *ds.Get().ServiceSettings.SiteURL)
 	})
 
+	t.Run("persist failed: too long", func(t *testing.T) {
+		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
+		defer tearDown()
+
+		ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *sqlSettings.DriverName, *sqlSettings.DataSource))
+		require.NoError(t, err)
+		defer ds.Close()
+
+		longSiteURL := fmt.Sprintf("http://%s", strings.Repeat("a", config.MaxWriteLength))
+		newCfg := emptyConfig.Clone()
+		newCfg.ServiceSettings.SiteURL = sToP(longSiteURL)
+
+		_, err = ds.Set(newCfg)
+		require.Error(t, err)
+		assert.True(t, strings.HasPrefix(err.Error(), "failed to persist: marshalled configuration failed length check: value is too long"), "unexpected error: "+err.Error())
+	})
+
 	t.Run("listeners notified", func(t *testing.T) {
 		activeId, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
@@ -809,6 +824,22 @@ func TestDatabaseSetFile(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, []byte("overwritten file"), data)
 	})
+
+	t.Run("max length", func(t *testing.T) {
+		longFile := bytes.Repeat([]byte{0x0}, config.MaxWriteLength)
+
+		err := ds.SetFile("toolong", longFile)
+		require.NoError(t, err)
+	})
+
+	t.Run("too long", func(t *testing.T) {
+		longFile := bytes.Repeat([]byte{0x0}, config.MaxWriteLength+1)
+
+		err := ds.SetFile("toolong", longFile)
+		if assert.Error(t, err) {
+			assert.True(t, strings.HasPrefix(err.Error(), "file data failed length check: value is too long"))
+		}
+	})
 }
 
 func TestDatabaseHasFile(t *testing.T) {