From 340287890a78b69f1659aa161d715636dec19b1f Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 27 Sep 2019 09:10:38 -0300 Subject: [PATCH] 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 | 44 +++++++++++++++++++++++++++++++++++++++++ config/database_test.go | 39 ++++++++++++++++++++++++++++++++---- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/config/database.go b/config/database.go index e6f83c63cb..b6bc765941 100644 --- a/config/database.go +++ b/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, diff --git a/config/database_test.go b/config/database_test.go index 6119fc2305..b63390fd6b 100644 --- a/config/database_test.go +++ b/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) {